Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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(source-maps): workaround CORS for fetching maps #9459
core(source-maps): workaround CORS for fetching maps #9459
Changes from 2 commits
eb2b32e
637f8f4
f20e927
6fcfd56
61d0c2f
457f8c9
f7a7652
9265a01
1f3ec5d
496ca2c
b6f3aa5
6687cf7
5b66242
087cbc0
586dbf8
d66ed67
800c9e5
32847c9
8ed46c2
1ebdc33
0d7c09a
ea35a53
adc0adf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could split into its own fn for easier testing.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to keep it simple and safe at first I would propose deleting all cookies, until we observe the need for them
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.
is this just based on if chrome thinks the resource is text-based or not?
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.
yeah, IDK how the agent decides but there is no way to configure it from the protocol.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe return both cases as buffer?
instead of the string case:
Buffer.from(bufStr, 'utf8');
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.
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 payloadsThere 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.
sgtm
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.
can you remind me (perhaps in a comment) why we do this?
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.
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.
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.
Aborted by Lighthouse
?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.
this is an enum, can't give custom messages.
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.
think you also want to set
top
.