core(source-maps): workaround CORS for fetching maps by connorjclark · Pull Request #9459 · GoogleChrome/lighthouse

connorjclark · 2019-07-25T20:53:49Z

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

patrickhulce

really clever workaround I like it! still a bit unsure about needing to start with cookies at all since I think it potentially opens the door to having Lighthouse as a auto-CSRF tool 😆but OK

does devtools fetch source maps with cookies?

patrickhulce · 2019-07-25T20:57:02Z

+
+<body>
+    <script>
+        console.log('a');


does the second one need to have content?

neither do - I didn't meant to commit this.

patrickhulce · 2019-07-25T21:03:05Z

+        }
+
+        const responseBody = await this.sendCommand('Fetch.getResponseBody', {requestId});
+        if (responseBody.base64Encoded) {


is this just based on if chrome thinks the resource is text-based or not?

yeah, IDK how the agent decides but there is no way to configure it from the protocol.

patrickhulce · 2019-07-25T21:04:38Z

+
+        const responseBody = await this.sendCommand('Fetch.getResponseBody', {requestId});
+        if (responseBody.base64Encoded) {
+          resolve(Buffer.from(responseBody.body, 'base64').toString());


this feels like a tricky way to handle buffers, could we return a complex type {buffer: Buffer}|{text: string} or something?

maybe return both cases as buffer?

instead of the string case: Buffer.from(bufStr, 'utf8');

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

patrickhulce · 2019-07-25T21:08:51Z

+
+        // 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.


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?

idk how cookies work tbh. so you're saying this bit of code totally invalidates any source maps behind cookie-authentication?

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

brendankenny

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.

paulirish · 2019-07-25T21:48:22Z

+        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();


this could split into its own fn for easier testing.

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?

to keep it simple and safe at first I would propose deleting all cookies, until we observe the need for them

paulirish · 2019-07-25T21:53:23Z

+
+        const responseBody = await this.sendCommand('Fetch.getResponseBody', {requestId});
+        if (responseBody.base64Encoded) {
+          resolve(Buffer.from(responseBody.body, 'base64').toString());


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

paulirish · 2019-07-25T21:57:09Z

+      /** @type {HTMLIFrameElement} */
+      const iframe = document.createElement('iframe');
+      // Try really hard not to affect the page.
+      iframe.style.display = 'none';


think you also want to set top.

iframe.style.cssText = ` display: none; visibility: hidden; position: absolute; left: -100px; top: -100px; width: 1px; height: 1px; `;

paulirish · 2019-07-25T22:00:04Z

+          resolve(responseBody.body);
+        }
+
+        // Fail the request (from the page's perspective) so that the iframe never loads.


can you remind me (perhaps in a comment) why we do this?

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.

paulirish · 2019-07-25T22:00:51Z

+        }
+
+        // Fail the request (from the page's perspective) so that the iframe never loads.
+        this.sendCommand('Fetch.failRequest', {requestId, errorReason: 'Aborted'});


Aborted by Lighthouse?

this is an enum, can't give custom messages.

patrickhulce · 2020-03-04T17:02:24Z

+   * 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`.


Suggested change

* urls to be intercepted via `driver.setOnRequestPausedHandler`.

* urls to be intercepted via `fetcher.setOnRequestPausedHandler`.

patrickhulce · 2020-03-04T17:03:35Z

+   * @param {string} url
+   * @param {(event: LH.Crdp.Fetch.RequestPausedEvent) => void} handler
+   */
+  async setOnRequestPausedHandler(url, handler) {


so...... private? :)

patrickhulce · 2020-03-04T17:05:35Z

+   * @return {Promise<string>}
+   */
+  async fetchResource(url, timeoutInMs = 500) {
+    if (!this.driver.isDomainEnabled('Fetch')) {


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 :)

patrickhulce · 2020-03-04T17:07:54Z

-      .mockResponse('Debugger.disable', {});
+      .mockResponse('Debugger.disable', {})
+      .mockResponse('Fetch.enable', {})
+      .mockResponse('Fetch.disable', {});


simplifies all this testing too :)

patrickhulce

LGTM

stale

brendankenny · 2020-03-05T21:53:24Z

are we still pausing on this until we get a sense of the protocol method timeline/feature set?

patrickhulce · 2020-03-05T23:35:18Z

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 :)

brendankenny · 2020-03-13T20:03:02Z

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

connorjclark · 2020-03-17T23:29:13Z

i kicked off an email thread about this. because it would be cool.
in the meantime though, lets not block this improvement. in the future i hope we can improve the Fetcher even more.
#9459 (comment)

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.

core(source-maps): workaround CORS for fetching maps

eb2b32e

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners July 25, 2019 20:53

googlebot added the cla: yes label Jul 25, 2019

patrickhulce reviewed Jul 25, 2019

View reviewed changes

brendankenny reviewed Jul 25, 2019

View reviewed changes

changes

637f8f4

vercel Bot deployed to staging July 25, 2019 21:34 View deployment

paulirish previously requested changes Jul 25, 2019

View reviewed changes

fetcher.js

f20e927

vercel Bot deployed to staging July 25, 2019 22:36 View deployment

top, isoloation

6fcfd56

vercel Bot deployed to staging July 25, 2019 22:38 View deployment

more dum dum styles

61d0c2f

vercel Bot deployed to staging July 25, 2019 22:41 View deployment

samesite

457f8c9

vercel Bot deployed to staging July 25, 2019 22:46 View deployment

connorjclark added the chillin ❄ label Aug 19, 2019

connorjclark mentioned this pull request Sep 28, 2019

Can Lighthouse Plugins specify URL query parameters #9754

Closed

connorjclark commented Sep 28, 2019

View reviewed changes

Comment thread lighthouse-core/gather/fetcher.js Outdated

Update lighthouse-core/gather/fetcher.js

f7a7652

vercel Bot deployed to staging September 28, 2019 05:48 View deployment

Merge remote-tracking branch 'origin/master' into fetch-no-cors

9265a01

vercel Bot deployed to staging November 27, 2019 01:20 View deployment

lint

1f3ec5d

vercel Bot deployed to staging November 27, 2019 01:25 View deployment

del

496ca2c

vercel Bot deployed to staging November 27, 2019 02:03 View deployment

connorjclark mentioned this pull request Dec 4, 2019

Discussion PR: bundle analysis #10064

Closed

connorjclark added waiting4reviewer and removed waiting4committer labels Mar 4, 2020

patrickhulce reviewed Mar 4, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Mar 4, 2020

connorjclark added 2 commits March 4, 2020 10:51

private

800c9e5

smoke, cleanup

32847c9

vercel Bot deployed to Preview March 4, 2020 19:21 View deployment

connorjclark added 2 commits March 4, 2020 14:18

comment

8ed46c2

update

1ebdc33

vercel Bot deployed to Preview March 4, 2020 22:20 View deployment

connorjclark added waiting4reviewer and removed waiting4committer labels Mar 5, 2020

fix

0d7c09a

vercel Bot deployed to Preview March 5, 2020 01:03 View deployment

fix

ea35a53

vercel Bot deployed to Preview March 5, 2020 01:08 View deployment

patrickhulce approved these changes Mar 5, 2020

View reviewed changes

Comment thread lighthouse-core/gather/gather-runner.js

patrickhulce added waiting4committer and removed waiting4reviewer labels Mar 5, 2020

comment

adc0adf

vercel Bot deployed to Preview March 5, 2020 21:31 View deployment

connorjclark merged commit 72f0774 into master Mar 17, 2020

connorjclark deleted the fetch-no-cors branch March 17, 2020 23:52

paulirish mentioned this pull request Feb 9, 2021

Improve generalized/sourcemap fetcher #12070

Closed

	* urls to be intercepted via `driver.setOnRequestPausedHandler`.
	* urls to be intercepted via `fetcher.setOnRequestPausedHandler`.

Conversation

connorjclark commented Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark Jul 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brendankenny commented Mar 5, 2020

Uh oh!

patrickhulce commented Mar 5, 2020

Uh oh!

brendankenny commented Mar 13, 2020

Uh oh!

connorjclark commented Mar 17, 2020

Uh oh!

connorjclark commented Jul 25, 2019 •

edited

Loading

connorjclark Jul 25, 2019 •

edited

Loading