Summary: | Tainted origin flag logic should affect no-cors redirects | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rob Buis <rbuis> | ||||||||||||||||||
Component: | Page Loading | Assignee: | Rob Buis <rbuis> | ||||||||||||||||||
Status: | RESOLVED DUPLICATE | ||||||||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, cdumez, dbates, ews-watchlist, fred.wang, japhet, youennf | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Description
Rob Buis
2019-08-25 12:43:52 PDT
Created attachment 377224 [details]
Patch
Created attachment 377225 [details]
Patch
Created attachment 377230 [details]
Patch
Created attachment 377321 [details]
Patch
Is it ready for review? I'm a bit confused where the tainting flag comes in play in this patch. Created attachment 377743 [details]
Patch
(In reply to Frédéric Wang (:fredw) from comment #5) > Is it ready for review? I'm a bit confused where the tainting flag comes in > play in this patch. It was not, but is now :) Comment on attachment 377743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377743&action=review > Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:328 > + request.setHTTPOrigin(SecurityOrigin::createUnique()->toString()); We should probably also set m_origin here. This will ensure that the CORP check is fine. It might be interesting to do some more refactoring. In particular, add a routine that implements https://fetch.spec.whatwg.org/#append-a-request-origin-header. This will make sure we add the Origin header for POST but not for GET. We should probably add a test for GET to check that no Origin header in no-cors is set after a cross origin redirection. doesNotNeedCORSCheck includes navigate mode for which we do not want to add a null origin. We might want to remove doesNotNeedCORSCheck and add some early checks for navigate mode and same origin mode. Then we could have some common code for no-cors and cors cases. This should get us closer to the fetch spec. > LayoutTests/imported/w3c/web-platform-tests/fetch/security/embedded-credentials.tentative.sub-expected.txt:3 > Blocked access to external URL http://user:pass@www.localhost:8800/images/red.png This test should be updated to use hosts[alt]. Created attachment 377877 [details]
Patch
Created attachment 385755 [details]
Patch
Created attachment 385768 [details]
Patch
It is possible the code in these patches is still valuable, but in the end 186030 is similar/same so let's try to fix that one instead. *** This bug has been marked as a duplicate of bug 186030 *** |