core(source-maps): workaround CORS for fetching maps #9459
Conversation
connorjclark
requested review from
brendankenny,
patrickhulce and
paulirish
as code owners
|
|
||
| <body> | ||
| <script> | ||
| console.log('a'); |
There was a problem hiding this comment.
does the second one need to have content?
There was a problem hiding this comment.
neither do - I didn't meant to commit this.
lighthouse-core/gather/driver.js
Outdated
| } | ||
|
|
||
| const responseBody = await this.sendCommand('Fetch.getResponseBody', {requestId}); | ||
| if (responseBody.base64Encoded) { |
There was a problem hiding this comment.
is this just based on if chrome thinks the resource is text-based or not?
There was a problem hiding this comment.
yeah, IDK how the agent decides but there is no way to configure it from the protocol.
lighthouse-core/gather/driver.js
Outdated
|
|
||
| const responseBody = await this.sendCommand('Fetch.getResponseBody', {requestId}); | ||
| if (responseBody.base64Encoded) { | ||
| resolve(Buffer.from(responseBody.body, 'base64').toString()); |
There was a problem hiding this comment.
this feels like a tricky way to handle buffers, could we return a complex type {buffer: Buffer}|{text: string} or something?
There was a problem hiding this comment.
maybe return both cases as buffer?
instead of the string case: Buffer.from(bufStr, 'utf8');
There was a problem hiding this comment.
we currently fetch the robots.txt, sourcemaps, the start_url (HTML)
so we dont need non-text resources at all right now. fail any base64Encoded and we'll handle it later if we need to actually get these payloads
lighthouse-core/gather/driver.js
Outdated
|
|
||
| // The first requestPaused event is for the request stage. Continue it. | ||
| if (!responseStatusCode) { | ||
| // Remove same-site cookies so we aren't buying stuff on Amazon. |
There was a problem hiding this comment.
I guess we're not concerned with the sort of manifest/user profile state being stored in a samesite cookie that made us want to deal with cookies at all in the first place?
There was a problem hiding this comment.
idk how cookies work tbh. so you're saying this bit of code totally invalidates any source maps behind cookie-authentication?
There was a problem hiding this comment.
any cookie auth that they've marked as SameSite, yeah
marking auth cookies as samesite is a decent way to prevent the annoying site-style logout/language attack
There was a problem hiding this comment.
I still think this should wait at least until there's an audit using source maps and we can see what the actual benefits would be. This isn't blocking anything in #9097 so it would be nice to have a much better picture of the tradeoffs involved before committing to this approach.
lighthouse-core/gather/driver.js
Outdated
| if (!responseStatusCode) { | ||
| // Remove same-site cookies so we aren't buying stuff on Amazon. | ||
| const sameSiteCookies = await this.sendCommand('Network.getCookies', {urls: [url]}); | ||
| const sameSiteCookiesKeyValueSet = new Set(); |
There was a problem hiding this comment.
this could split into its own fn for easier testing.
There was a problem hiding this comment.
punting on testing b/c i don't really know how the cookie story plays out here, may be this is just removed and we totally delete all cookies?
There was a problem hiding this comment.
to keep it simple and safe at first I would propose deleting all cookies, until we observe the need for them
lighthouse-core/gather/driver.js
Outdated
|
|
||
| const responseBody = await this.sendCommand('Fetch.getResponseBody', {requestId}); | ||
| if (responseBody.base64Encoded) { | ||
| resolve(Buffer.from(responseBody.body, 'base64').toString()); |
There was a problem hiding this comment.
we currently fetch the robots.txt, sourcemaps, the start_url (HTML)
so we dont need non-text resources at all right now. fail any base64Encoded and we'll handle it later if we need to actually get these payloads
lighthouse-core/gather/driver.js
Outdated
| /** @type {HTMLIFrameElement} */ | ||
| const iframe = document.createElement('iframe'); | ||
| // Try really hard not to affect the page. | ||
| iframe.style.display = 'none'; |
There was a problem hiding this comment.
think you also want to set top.
iframe.style.cssText = `
display: none;
visibility: hidden;
position: absolute;
left: -100px;
top: -100px;
width: 1px;
height: 1px;
`;
lighthouse-core/gather/driver.js
Outdated
| resolve(responseBody.body); | ||
| } | ||
|
|
||
| // Fail the request (from the page's perspective) so that the iframe never loads. |
There was a problem hiding this comment.
can you remind me (perhaps in a comment) why we do this?
There was a problem hiding this comment.
Sorry, I don't have any more to add than what the comment says. I assume Blink will do more work if the iframe actually gets its contents loaded, even if all the styling on it makes it not visible. but i'm just guessing.
lighthouse-core/gather/driver.js
Outdated
| } | ||
|
|
||
| // Fail the request (from the page's perspective) so that the iframe never loads. | ||
| this.sendCommand('Fetch.failRequest', {requestId, errorReason: 'Aborted'}); |
There was a problem hiding this comment.
this is an enum, can't give custom messages.
lighthouse-core/gather/fetcher.js
Outdated
| * 3) if multiple commands to continue the same request are sent, protocol errors occur. | ||
| * | ||
| * So instead we have one global `Fetch.enable` / `Fetch.requestPaused` pair, and allow specific | ||
| * urls to be intercepted via `driver.setOnRequestPausedHandler`. |
There was a problem hiding this comment.
| * urls to be intercepted via `driver.setOnRequestPausedHandler`. | |
| * urls to be intercepted via `fetcher.setOnRequestPausedHandler`. |
lighthouse-core/gather/fetcher.js
Outdated
| * @param {string} url | ||
| * @param {(event: LH.Crdp.Fetch.RequestPausedEvent) => void} handler | ||
| */ | ||
| async setOnRequestPausedHandler(url, handler) { |
lighthouse-core/gather/fetcher.js
Outdated
| * @return {Promise<string>} | ||
| */ | ||
| async fetchResource(url, timeoutInMs = 500) { | ||
| if (!this.driver.isDomainEnabled('Fetch')) { |
There was a problem hiding this comment.
also if we are going to be replacing this with a simple "Page.fetchTheThing" protocol method soon, then the whole enabling/disabling domain thing from outside is going to need to go away so I definitely vote to keep that internal to this class if possible :)
| .mockResponse('Debugger.disable', {}); | ||
| .mockResponse('Debugger.disable', {}) | ||
| .mockResponse('Fetch.enable', {}) | ||
| .mockResponse('Fetch.disable', {}); |
There was a problem hiding this comment.
simplifies all this testing too :)
|
are we still pausing on this until we get a sense of the protocol method timeline/feature set? |
I don't think we were ever pausing, just writing in a way that keeps the transition smooth :) |
sorry, this was from a side conversation where (I believe) we agreed to evaluate the viability of the protocol method first |
then somewhere else I think @paulirish reached out to @sigurdschneider and learned things are still very WIP. Looking forward to a protocol solution but don't wanna block this. It's unknown when protocol support will land. |
This patch adds an interface over the Fetch domain. See comments for hows and whys.
Deferring unit tests until after feedback. I added a simple smoke test to confirm that CORS was vanquished. Not certain on the cookie stuff, definitely give that special attention.
cont. #9101
related: #9097