-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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.
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
core(preload): only allow same origin (domain + subdomains) #5065
Changes from 6 commits
af8118e
5a7c041
71f6153
7b697ad
a681ce7
295d016
acc39e1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,10 @@ const Util = require('../report/html/renderer/util.js'); | |
const URL = /** @type {!Window["URL"]} */ (typeof self !== 'undefined' && self.URL) || | ||
require('url').URL; | ||
|
||
const listOfTlds = [ | ||
'com', 'co', 'gov', 'edu', 'ac', 'org', 'go', 'gob', 'or', 'net', 'in', 'ne', 'nic', 'gouv', | ||
'web', 'spb', 'blog', 'jus', 'kiev', 'mil', 'wi', 'qc', 'ca', 'bel', 'on', | ||
]; | ||
/** | ||
* There is fancy URL rewriting logic for the chrome://settings page that we need to work around. | ||
* Why? Special handling was added by Chrome team to allow a pushState transition between chrome:// pages. | ||
|
@@ -87,6 +91,52 @@ class URLShim extends URL { | |
} | ||
} | ||
|
||
/** | ||
* Gets the tld of a domain | ||
* | ||
* @param {string} hostname | ||
* @return {string} tld | ||
*/ | ||
static getTld(hostname) { | ||
const tlds = hostname.split('.').slice(-2); | ||
|
||
if (!listOfTlds.includes(tlds[0])) { | ||
return `.${tlds[tlds.length - 1]}`; | ||
} | ||
|
||
return `.${tlds.join('.')}`; | ||
} | ||
|
||
/** | ||
* Check if rootDomains matches | ||
* | ||
* @param {string} urlA | ||
* @param {string} urlB | ||
*/ | ||
static rootDomainsMatch(urlA, urlB) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add some tests in url-shim-test? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah forgot about that one :) |
||
let urlAInfo; | ||
let urlBInfo; | ||
try { | ||
urlAInfo = new URL(urlA); | ||
urlBInfo = new URL(urlB); | ||
} catch (err) { | ||
return false; | ||
} | ||
|
||
if (!urlAInfo.hostname || !urlBInfo.hostname) { | ||
return false; | ||
} | ||
|
||
const tldA = URLShim.getTld(urlAInfo.hostname); | ||
const tldB = URLShim.getTld(urlBInfo.hostname); | ||
|
||
// get the string before the tld | ||
const urlARootDomain = urlAInfo.hostname.replace(new RegExp(`${tldA}$`), '').split('.').splice(-1)[0]; | ||
const urlBRootDomain = urlBInfo.hostname.replace(new RegExp(`${tldB}$`), '').split('.').splice(-1)[0]; | ||
|
||
return urlARootDomain === urlBRootDomain; | ||
} | ||
|
||
/** | ||
* @param {string} url | ||
* @param {{numPathParts: number, preserveQuery: boolean, preserveHost: boolean}=} options | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,14 +50,18 @@ describe('Performance: uses-rel-preload audit', () => { | |
|
||
it('should suggest preload resource', () => { | ||
const rootNode = buildNode(1, 'http://example.com'); | ||
const mainDocumentNode = buildNode(2, 'http://www.example.com'); | ||
const mainDocumentNode = buildNode(2, 'http://www.example.com:3000'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we're gonna need some graph creation helpers soon to make all this stuff :) |
||
const scriptNode = buildNode(3, 'http://www.example.com/script.js'); | ||
const scriptAddedNode = buildNode(4, 'http://www.example.com/script-added.js'); | ||
const scriptSubNode = buildNode(5, 'http://sub.example.com/script-sub.js'); | ||
const scriptOtherNode = buildNode(6, 'http://otherdomain.com/script-other.js'); | ||
|
||
mainDocumentNode.setIsMainDocument(true); | ||
mainDocumentNode.addDependency(rootNode); | ||
scriptNode.addDependency(mainDocumentNode); | ||
scriptAddedNode.addDependency(scriptNode); | ||
scriptSubNode.addDependency(scriptNode); | ||
scriptOtherNode.addDependency(scriptNode); | ||
|
||
mockGraph = rootNode; | ||
mockSimulator = { | ||
|
@@ -69,41 +73,63 @@ describe('Performance: uses-rel-preload audit', () => { | |
const mainDocumentNodeLocal = nodesByUrl.get(mainDocumentNode.record.url); | ||
const scriptNodeLocal = nodesByUrl.get(scriptNode.record.url); | ||
const scriptAddedNodeLocal = nodesByUrl.get(scriptAddedNode.record.url); | ||
const scriptSubNodeLocal = nodesByUrl.get(scriptSubNode.record.url); | ||
const scriptOtherNodeLocal = nodesByUrl.get(scriptOtherNode.record.url); | ||
|
||
const nodeTimings = new Map([ | ||
[rootNodeLocal, {starTime: 0, endTime: 500}], | ||
[mainDocumentNodeLocal, {startTime: 500, endTime: 1000}], | ||
[scriptNodeLocal, {startTime: 1000, endTime: 2000}], | ||
[scriptAddedNodeLocal, {startTime: 2000, endTime: 3250}], | ||
[scriptSubNodeLocal, {startTime: 2000, endTime: 3000}], | ||
[scriptOtherNodeLocal, {startTime: 2000, endTime: 3500}], | ||
]); | ||
|
||
if (scriptAddedNodeLocal.getDependencies()[0] === mainDocumentNodeLocal) { | ||
nodeTimings.set(scriptAddedNodeLocal, {startTime: 1000, endTime: 2000}); | ||
} | ||
|
||
return {timeInMs: 3250, nodeTimings}; | ||
if (scriptSubNodeLocal.getDependencies()[0] === mainDocumentNodeLocal) { | ||
nodeTimings.set(scriptSubNodeLocal, {startTime: 1000, endTime: 2000}); | ||
} | ||
|
||
if (scriptOtherNodeLocal.getDependencies()[0] === mainDocumentNodeLocal) { | ||
nodeTimings.set(scriptOtherNodeLocal, {startTime: 1000, endTime: 2500}); | ||
} | ||
|
||
return {timeInMs: 3500, nodeTimings}; | ||
}, | ||
}; | ||
|
||
const mainResource = Object.assign({}, defaultMainResource, { | ||
url: 'http://www.example.com', | ||
url: 'http://www.example.com:3000', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this just to exercise extra hostname checks? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes it's to make sure that origins with a port number are handled correct by the rootDomainMatch |
||
redirects: [''], | ||
}); | ||
const networkRecords = [ | ||
{ | ||
requestId: '2', | ||
_isLinkPreload: false, | ||
_url: 'http://www.example.com', | ||
url: 'http://www.example.com:3000', | ||
}, | ||
{ | ||
requestId: '3', | ||
_isLinkPreload: false, | ||
_url: 'http://www.example.com/script.js', | ||
url: 'http://www.example.com/script.js', | ||
}, | ||
{ | ||
requestId: '4', | ||
_isLinkPreload: false, | ||
_url: 'http://www.example.com/script-added.js', | ||
url: 'http://www.example.com/script-added.js', | ||
}, | ||
{ | ||
requestId: '5', | ||
_isLinkPreload: false, | ||
url: 'http://sub.example.com/script-sub.js', | ||
}, | ||
{ | ||
requestId: '6', | ||
_isLinkPreload: false, | ||
url: 'http://otherdomain.com/script-other.js', | ||
}, | ||
]; | ||
|
||
|
@@ -120,6 +146,14 @@ describe('Performance: uses-rel-preload audit', () => { | |
request: networkRecords[2], | ||
children: {}, | ||
}, | ||
'5': { | ||
request: networkRecords[3], | ||
children: {}, | ||
}, | ||
'6': { | ||
request: networkRecords[4], | ||
children: {}, | ||
}, | ||
}, | ||
}, | ||
}, | ||
|
@@ -131,7 +165,7 @@ describe('Performance: uses-rel-preload audit', () => { | |
return UsesRelPreload.audit(mockArtifacts(networkRecords, chains, mainResource), {}).then( | ||
output => { | ||
assert.equal(output.rawValue, 1250); | ||
assert.equal(output.details.items.length, 1); | ||
assert.equal(output.details.items.length, 2); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe overkill but assert the URL matches too? |
||
} | ||
); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment that somewhat documents what happened here re: coverage and whatnot? maybe just link to the best comment thread on this PR :)