[go: nahoru, domu]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

core(manifest-parser): handle blob manifests #9088

Merged
merged 5 commits into from
Jun 1, 2019
Merged

core(manifest-parser): handle blob manifests #9088

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b48c7ab
core(manifest-parser): handle blob manifests
patrickhulce May 30, 2019
45b075d
text tweak
patrickhulce May 30, 2019
59b9ee3
feedback
patrickhulce May 31, 2019
a5df88d
lint
patrickhulce May 31, 2019
fd4bd7a
Apply suggestions from code review
patrickhulce May 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 41 additions & 12 deletions lighthouse-core/lib/manifest-parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const ALLOWED_DISPLAY_VALUES = [
];
/**
* All display-mode fallbacks, including when unset, lead to default display mode 'browser'.
* @see https://w3c.github.io/manifest/#dfn-default-display-mode
* @see https://www.w3.org/TR/2016/WD-appmanifest-20160825/#dfn-default-display-mode
*/
const DEFAULT_DISPLAY_MODE = 'browser';

Expand Down Expand Up @@ -111,7 +111,7 @@ function checkSameOrigin(url1, url2) {
}

/**
* https://w3c.github.io/manifest/#start_url-member
* https://www.w3.org/TR/2016/WD-appmanifest-20160825/#start_url-member
* @param {*} jsonInput
* @param {string} manifestUrl
* @param {string} documentUrl
Expand Down Expand Up @@ -151,7 +151,7 @@ function parseStartUrl(jsonInput, manifestUrl, documentUrl) {
return {
raw,
value: documentUrl,
warning: 'ERROR: invalid start_url relative to ${manifestUrl}',
warning: `ERROR: invalid start_url relative to ${manifestUrl}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, that's certainly one big problem with testing via assert.ok(parsedUrl.warning)...

};
}

Expand Down Expand Up @@ -218,6 +218,7 @@ function parseOrientation(jsonInput) {
}

/**
* @see https://www.w3.org/TR/2016/WD-appmanifest-20160825/#src-member
* @param {*} raw
* @param {string} manifestUrl
*/
Expand All @@ -229,8 +230,14 @@ function parseIcon(raw, manifestUrl) {
src.value = undefined;
}
if (src.value) {
// 9.4(4) - construct URL with manifest URL as the base
src.value = new URL(src.value, manifestUrl).href;
try {
// 9.4(4) - construct URL with manifest URL as the base
src.value = new URL(src.value, manifestUrl).href;
} catch (_) {
// 9.4 "This algorithm will return a URL or undefined."
src.warning = `ERROR: invalid icon url will be ignored: '${raw.src}'`;
src.value = undefined;
}
}

const type = parseString(raw.type, true);
Expand Down Expand Up @@ -301,20 +308,31 @@ function parseIcons(jsonInput, manifestUrl) {
};
}

// TODO(bckenny): spec says to skip icons missing `src`, so debug messages on
// individual icons are lost. Warn instead?
const value = raw
const parsedIcons = raw
// 9.6(3)(1)
.filter(icon => icon.src !== undefined)
// 9.6(3)(2)(1)
.map(icon => parseIcon(icon, manifestUrl))
.map(icon => parseIcon(icon, manifestUrl));

// NOTE: we still lose the specific message on these icons, but it's not possible to surface them
// without a massive change to the structure and paradigms of `manifest-parser`.
const ignoredIconsWithWarnings = parsedIcons
.filter(icon => {
const possibleWarnings = [icon.warning, icon.value.type.warning, icon.value.src.warning,
icon.value.sizes.warning, icon.value.density.warning].filter(Boolean);
const hasSrc = !!icon.value.src.value;
return !!possibleWarnings.length && !hasSrc;
});

const value = parsedIcons
// 9.6(3)(2)(2)
.filter(parsedIcon => parsedIcon.value.src.value !== undefined);

return {
raw,
value,
warning: undefined,
warning: ignoredIconsWithWarnings.length ?
'WARNING: Some icons were ignored due to warnings.' : '',
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
};
}

Expand All @@ -333,7 +351,7 @@ function parseApplication(raw) {
appUrl.value = new URL(appUrl.value).href;
} catch (e) {
appUrl.value = undefined;
appUrl.warning = 'ERROR: invalid application URL ${raw.url}';
appUrl.warning = `ERROR: invalid application URL ${raw.url}`;
}
}

Expand Down Expand Up @@ -460,10 +478,21 @@ function parse(string, manifestUrl, documentUrl) {
};
/* eslint-enable camelcase */

/** @type {string|undefined} */
let manifestUrlWarning;
try {
const manifestUrlParsed = new URL(manifestUrl);
if (!manifestUrlParsed.protocol.startsWith('http')) {
manifestUrlWarning = `WARNING: manifest URL not available over a valid network protocol`;
}
} catch (_) {
manifestUrlWarning = `ERROR: invalid manifest URL: '${manifestUrl}'`;
}

return {
raw: string,
value: manifest,
warning: undefined,
warning: manifestUrlWarning,
};
}

Expand Down
97 changes: 66 additions & 31 deletions lighthouse-core/test/lib/manifest-parser-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const manifestStub = require('../fixtures/manifest.json');

const EXAMPLE_MANIFEST_URL = 'https://example.com/manifest.json';
const EXAMPLE_DOC_URL = 'https://example.com/index.html';
const EXAMPLE_MANIFEST_BLOB_URL = 'blob:https://example.com/manifest.json';

/**
* Simple manifest parsing helper when the manifest URLs aren't material to the
Expand All @@ -27,52 +28,70 @@ function noUrlManifestParser(manifestSrc) {
describe('Manifest Parser', function() {
it('should not parse empty string input', function() {
const parsedManifest = noUrlManifestParser('');
assert.ok(parsedManifest.warning);
expect(parsedManifest.warning)
.toEqual('ERROR: file isn\'t valid JSON: SyntaxError: Unexpected end of JSON input');
});

it('accepts empty dictionary', function() {
const parsedManifest = noUrlManifestParser('{}');
assert(!parsedManifest.warning);
assert.equal(parsedManifest.value.name.value, undefined);
assert.equal(parsedManifest.value.short_name.value, undefined);
assert.equal(parsedManifest.value.start_url.value, EXAMPLE_DOC_URL);
assert.equal(parsedManifest.value.display.value, 'browser');
assert.equal(parsedManifest.value.orientation.value, undefined);
assert.equal(parsedManifest.value.theme_color.value, undefined);
assert.equal(parsedManifest.value.background_color.value, undefined);
assert.ok(Array.isArray(parsedManifest.value.icons.value));
assert.ok(parsedManifest.value.icons.value.length === 0);
expect(parsedManifest.warning).toBeUndefined();
expect(parsedManifest.value.name.value).toBe(undefined);
expect(parsedManifest.value.short_name.value).toBe(undefined);
expect(parsedManifest.value.start_url.value).toBe(EXAMPLE_DOC_URL);
expect(parsedManifest.value.display.value).toBe('browser');
expect(parsedManifest.value.orientation.value).toBe(undefined);
expect(parsedManifest.value.theme_color.value).toBe(undefined);
expect(parsedManifest.value.background_color.value).toBe(undefined);
expect(parsedManifest.value.icons.value).toHaveLength(0);
// TODO:
// related_applications
// prefer_related_applications
});

it('should warn on invalid manifest parser URL', function() {
const parsedManifest = manifestParser('{}', 'not a URL', EXAMPLE_DOC_URL);
expect(parsedManifest.warning)
.toEqual('ERROR: invalid manifest URL: \'not a URL\'');
});

it('should warn on valid but non-HTTPS manifest parser URL', function() {
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
const parsedManifest = manifestParser('{}', EXAMPLE_MANIFEST_BLOB_URL, EXAMPLE_DOC_URL);
expect(parsedManifest.warning)
.toEqual('WARNING: manifest URL not available over a valid network protocol');
});

describe('icon parsing', function() {
// 9.7
it('gives an empty array and an error for erroneous icons entry', () => {
const parsedManifest = manifestParser('{"icons": {"16": "img/icons/icon16.png"}}',
EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
assert.ok(!parsedManifest.warning);
const parsedManifest = manifestParser(
'{"icons": {"16": "img/icons/icon16.png"}}',
EXAMPLE_MANIFEST_URL,
EXAMPLE_DOC_URL
);

expect(parsedManifest.warning).toBeUndefined();
const icons = parsedManifest.value.icons;
assert.ok(Array.isArray(icons.value));
assert.equal(icons.value.length, 0);
assert.ok(icons.warning);
});

it('gives an empty array and no error for missing icons entry', () => {
const parsedManifest = manifestParser('{}',
EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
assert.ok(!parsedManifest.warning);
const parsedManifest = manifestParser('{}', EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
expect(parsedManifest.warning).toBeUndefined();
const icons = parsedManifest.value.icons;
assert.ok(Array.isArray(icons.value));
assert.equal(icons.value.length, 0);
assert.ok(!icons.warning);
});

it('parses basic string', function() {
const parsedManifest = manifestParser('{"icons": [{"src": "192.png", "sizes": "192x192"}]}',
EXAMPLE_MANIFEST_URL, EXAMPLE_DOC_URL);
assert(!parsedManifest.warning);
const parsedManifest = manifestParser(
'{"icons": [{"src": "192.png", "sizes": "192x192"}]}',
EXAMPLE_MANIFEST_URL,
EXAMPLE_DOC_URL
);
expect(parsedManifest.warning).toBeUndefined();
const icons = parsedManifest.value.icons;
assert(!icons.warning);
const icon192 = icons.value[0];
Expand All @@ -81,9 +100,12 @@ describe('Manifest Parser', function() {
});

it('finds four icons in the stub manifest', function() {
const parsedManifest = manifestParser(JSON.stringify(manifestStub), EXAMPLE_MANIFEST_URL,
EXAMPLE_DOC_URL);
assert(!parsedManifest.warning);
const parsedManifest = manifestParser(
JSON.stringify(manifestStub),
EXAMPLE_MANIFEST_URL,
EXAMPLE_DOC_URL
);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.icons.value.length, 4);
});

Expand All @@ -110,6 +132,19 @@ describe('Manifest Parser', function() {
assert.equal(icons.value.length, 0);
});

it('parses icons and discards any with invalid base URL values', () => {
const manifestSrc = JSON.stringify({
icons: [{
src: '/valid/path',
}],
});
const parsedManifest = manifestParser(manifestSrc, EXAMPLE_MANIFEST_BLOB_URL,
EXAMPLE_DOC_URL);
const icons = parsedManifest.value.icons;
expect(icons.value).toHaveLength(0);
expect(icons.warning).toEqual('WARNING: Some icons were ignored due to warnings.');
});

it('parses icons and discards any with undefined or empty string src values', () => {
const manifestSrc = JSON.stringify({
icons: [{
Expand Down Expand Up @@ -139,47 +174,47 @@ describe('Manifest Parser', function() {
describe('name parsing', function() {
it('parses basic string', function() {
const parsedManifest = noUrlManifestParser('{"name":"foo"}');
assert(!parsedManifest.warning);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.name.value, 'foo');
});

it('trims whitespaces', function() {
const parsedManifest = noUrlManifestParser('{"name":" foo "}');
assert(!parsedManifest.warning);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.name.value, 'foo');
});

it('doesn\'t parse non-string', function() {
let parsedManifest = noUrlManifestParser('{"name": {} }');
assert(!parsedManifest.warning);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.name.value, undefined);

parsedManifest = noUrlManifestParser('{"name": 42 }');
assert(!parsedManifest.warning);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.name.value, undefined);
});
});

describe('short_name parsing', function() {
it('parses basic string', function() {
const parsedManifest = noUrlManifestParser('{"short_name":"foo"}');
assert(!parsedManifest.warning);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.short_name.value, 'foo');
});

it('trims whitespaces', function() {
const parsedManifest = noUrlManifestParser('{"short_name":" foo "}');
assert(!parsedManifest.warning);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.short_name.value, 'foo');
});

it('doesn\'t parse non-string', function() {
let parsedManifest = noUrlManifestParser('{"short_name": {} }');
assert(!parsedManifest.warning);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.short_name.value, undefined);

parsedManifest = noUrlManifestParser('{"short_name": 42 }');
assert(!parsedManifest.warning);
expect(parsedManifest.warning).toBeUndefined();
assert.equal(parsedManifest.value.short_name.value, undefined);
});
});
Expand Down