Avoid using the CORS flag to reset request's origin in redirects #594
Conversation
annevk
added
the
needs tests
|
@davidben thanks, this also needs some tests still. Have you written any per chance? |
|
This affects canvas tainting and friends, right? |
|
Very good point, it would and that's not desirable. |
annevk
added
the
do not merge yet
|
So the alternative I can think of here is that we always preserve request's origin to its initial value and instead set a tainted origin flag during redirects. We then use that flag for the |
Otherwise things go wrong for "no-cors" POST redirects. Fixes #593.
annevk
force-pushed
the
annevk/redirect-origin-reset
branch
from
1a8272d
to
49420f8
Compare
|
I pushed a commit that does that, what do you think @yutakahirano? |
|
Sounds reasonable but don't we need to update the origin usage of preflight cache? |
|
I think that was already somewhat broken since it stored the actual origin, which for an opaque origin would always be unique. I guess we could add a guard to not add an entry to the cache if the tainted origin flag is set? |
|
I suspect the problem with the preflight cache dates back from when I added support redirects following a successful preflight. I don't think I fully considered how the preflight cache should behave in cases where the request's origin was opaque. Most performant would probably be to change the origin key into a byte sequence and use |
|
I tried to address the problem with the CORS-preflight cache by using a serialized origin as key. @yutakahirano @mikewest let me know what you all think. |
|
I also filed #735 on defining the CORS-preflight cache in terms of proper data structures. |
| <a for=request>referrer policy</a> is <var>request</var>'s <a for=request>referrer policy</a>. | ||
| <a for=request>referrer</a> is <var>request</var>'s <a for=request>referrer</a>, | ||
| <a for=request>referrer policy</a> is <var>request</var>'s <a for=request>referrer policy</a>, and | ||
| <a for=request>tainted origin flag</a> is <var>request</var>'s |
There was a problem hiding this comment.
Never mind, I was wrong, it's needed.
|
Thanks. I created a basic test at web-platform-tests/wpt#11164 that can be expanded. It seems that Safari omits the |
annevk
removed
needs tests
|
I have a question; Don't we need to set CORS flag when tainted in https://fetch.spec.whatwg.org/#http-redirect-fetch? That flag was set previously due to the origin manipulation. |
|
Hmm, should we change the first conditional of step 5 of main fetch to:
I think setting the CORS flag would not work since that would make "no-cors" follow CORS code paths. |
|
Filed #756 to track this, thanks! |
Automatic update from web-platform-testsOrigin header and 308 redirect For whatwg/fetch#594. -- wpt-commits: 89ae808d4ba430f83f8bc185b69108cf10070599 wpt-pr: 11164
Automatic update from web-platform-testsOrigin header and 308 redirect For whatwg/fetch#594. -- wpt-commits: 89ae808d4ba430f83f8bc185b69108cf10070599 wpt-pr: 11164
Automatic update from web-platform-testsOrigin header and 308 redirect For whatwg/fetch#594. -- wpt-commits: 89ae808d4ba430f83f8bc185b69108cf10070599 wpt-pr: 11164 UltraBlame original commit: cd57c997c98b496ed5c775316bc9bcafc3c6677c
Automatic update from web-platform-testsOrigin header and 308 redirect For whatwg/fetch#594. -- wpt-commits: 89ae808d4ba430f83f8bc185b69108cf10070599 wpt-pr: 11164 UltraBlame original commit: cd57c997c98b496ed5c775316bc9bcafc3c6677c
Automatic update from web-platform-testsOrigin header and 308 redirect For whatwg/fetch#594. -- wpt-commits: 89ae808d4ba430f83f8bc185b69108cf10070599 wpt-pr: 11164 UltraBlame original commit: cd57c997c98b496ed5c775316bc9bcafc3c6677c
Otherwise things go wrong for "no-cors" POST redirects.
Fixes #593.
Preview | Diff