[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 2 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
27 changes: 22 additions & 5 deletions lighthouse-core/lib/manifest-parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
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 @@ -229,8 +229,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.6 "This algorithm will return a URL or undefined."
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
src.warning = `ERROR: invalid icon url will be ignored ${raw.src}`;
patrickhulce marked this conversation as resolved.
Show resolved Hide resolved
src.value = undefined;
}
}

const type = parseString(raw.type, true);
Expand Down Expand Up @@ -333,7 +339,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 +466,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}`;
Copy link
Member

Choose a reason for hiding this comment

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

if the url was completely invalid, it seems like this should be throwing, a la the undefined check at the top?

(in theory it really should never happen since something was fetched in order to get here)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm, yeah I agree in principle and it should never happen but seeing as it tanks the entire LH run, that seems like a bad outcome and it was reasonable to warn when the rest of the manifest can still be parsed?

Copy link
Member

Choose a reason for hiding this comment

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

hm, yeah I agree in principle and it should never happen but seeing as it tanks the entire LH run, that seems like a bad outcome and it was reasonable to warn when the rest of the manifest can still be parsed?

arg, very good point. I forgot it's not in a gatherer anymore.

I can't find anywhere that we surface the warning if value is defined, but we can think about that with the other manifest error stuff.

}

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

Expand Down
23 changes: 23 additions & 0 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 Down Expand Up @@ -47,6 +48,16 @@ describe('Manifest Parser', function() {
// prefer_related_applications
});

it('should warn on invalid manifest parser URL', function() {
const parsedManifest = manifestParser('{}', 'not a URL', EXAMPLE_DOC_URL);
assert.ok(parsedManifest.warning);
});

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);
assert.ok(parsedManifest.warning);
Copy link
Member

Choose a reason for hiding this comment

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

we should probably learn our lesson and do some kind of real assertion :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

heh, alright alright

});

describe('icon parsing', function() {
// 9.7
it('gives an empty array and an error for erroneous icons entry', () => {
Expand Down Expand Up @@ -110,6 +121,18 @@ 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;
assert.equal(icons.value.length, 0);
Copy link
Member

Choose a reason for hiding this comment

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

check the warning?

});

it('parses icons and discards any with undefined or empty string src values', () => {
const manifestSrc = JSON.stringify({
icons: [{
Expand Down