Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2367583002:
Check CORS policy on redirect in TextTrackLoader (Closed)
|
Created:
4 years, 3 months ago by Nate Chapin Modified:
4 years, 2 months ago CC:
blink-reviews, chromium-reviews, eric.carlson_apple.com, gavinp+loader_chromium.org, gasubic, loading-reviews_chromium.org, nessy, tyoshino+watch_chromium.org, vcarbune.chromium Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCheck CORS policy on redirect in TextTrackLoader
BUG=633885
TEST=new case in http/tests/security/text-track-crossorigin.html
Committed: https://crrev.com/e99cc8e5a48ff4978d401c48a64f06649f647f3f
Cr-Commit-Position: refs/heads/master@{#421919}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 21 (8 generated)
The CQ bit was checked by japhet@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
japhet@chromium.org changed reviewers: + foolip@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
foolip@chromium.org changed reviewers: + fs@opera.com
LGTM assuming I've correctly understood the issue. FYI fs@opera.com https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html (right): https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: log('Loading <b>without</b> Access-Control-Allow-Origin header, with a redirect, no "crossorigin" attribute on <video>'); So, the problem was that if the redirect response itself didn't have Access-Control-Allow-Origin, we'd still happily use the resource redirected to as long as that passes the CORS check?
https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html (right): https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: log('Loading <b>without</b> Access-Control-Allow-Origin header, with a redirect, no "crossorigin" attribute on <video>'); On 2016/09/23 at 09:16:10, foolip wrote: > So, the problem was that if the redirect response itself didn't have Access-Control-Allow-Origin, we'd still happily use the resource redirected to as long as that passes the CORS check? Isn't it a "same-origin request, cross-origin redirect" case?
On 2016/09/23 10:22:42, fs wrote: > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > File > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html > (right): > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: > log('Loading <b>without</b> Access-Control-Allow-Origin header, with a redirect, > no "crossorigin" attribute on <video>'); > On 2016/09/23 at 09:16:10, foolip wrote: > > So, the problem was that if the redirect response itself didn't have > Access-Control-Allow-Origin, we'd still happily use the resource redirected to > as long as that passes the CORS check? > > Isn't it a "same-origin request, cross-origin redirect" case? Same-origin request, cross-origin redirect case. We'd do the CORS policy check for the initial request, then blindly trust the redirect, even if the redirect was cross-origin.
On 2016/09/23 16:22:49, Nate Chapin wrote: > On 2016/09/23 10:22:42, fs wrote: > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > File > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html > > (right): > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: > > log('Loading <b>without</b> Access-Control-Allow-Origin header, with a > redirect, > > no "crossorigin" attribute on <video>'); > > On 2016/09/23 at 09:16:10, foolip wrote: > > > So, the problem was that if the redirect response itself didn't have > > Access-Control-Allow-Origin, we'd still happily use the resource redirected to > > as long as that passes the CORS check? > > > > Isn't it a "same-origin request, cross-origin redirect" case? > > Same-origin request, cross-origin redirect case. We'd do the CORS policy check > for the initial request, then blindly trust the redirect, even if the redirect > was cross-origin. foolip@, you still OK with this given this explanation?
On 2016/09/28 at 23:25:41, japhet wrote: > On 2016/09/23 16:22:49, Nate Chapin wrote: > > On 2016/09/23 10:22:42, fs wrote: > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > File > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html > > > (right): > > > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: > > > log('Loading <b>without</b> Access-Control-Allow-Origin header, with a > > redirect, > > > no "crossorigin" attribute on <video>'); > > > On 2016/09/23 at 09:16:10, foolip wrote: > > > > So, the problem was that if the redirect response itself didn't have > > > Access-Control-Allow-Origin, we'd still happily use the resource redirected to > > > as long as that passes the CORS check? > > > > > > Isn't it a "same-origin request, cross-origin redirect" case? > > > > Same-origin request, cross-origin redirect case. We'd do the CORS policy check > > for the initial request, then blindly trust the redirect, even if the redirect > > was cross-origin. > > foolip@, you still OK with this given this explanation? FWIW, this change LGTM. Tangentially though, it would be nice if loaders/client didn't have to deal with this, but could rather just set up the request (per fetch rules.) *nudge* *nudge* *wink* *wink*
On 2016/09/29 at 07:52:01, fs wrote: > On 2016/09/28 at 23:25:41, japhet wrote: > > On 2016/09/23 16:22:49, Nate Chapin wrote: > > > On 2016/09/23 10:22:42, fs wrote: > > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > > File > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html > > > > (right): > > > > > > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: > > > > log('Loading <b>without</b> Access-Control-Allow-Origin header, with a > > > redirect, > > > > no "crossorigin" attribute on <video>'); > > > > On 2016/09/23 at 09:16:10, foolip wrote: > > > > > So, the problem was that if the redirect response itself didn't have > > > > Access-Control-Allow-Origin, we'd still happily use the resource redirected to > > > > as long as that passes the CORS check? > > > > > > > > Isn't it a "same-origin request, cross-origin redirect" case? > > > > > > Same-origin request, cross-origin redirect case. We'd do the CORS policy check > > > for the initial request, then blindly trust the redirect, even if the redirect > > > was cross-origin. > > > > foolip@, you still OK with this given this explanation? > > FWIW, this change LGTM. Tangentially though, it would be nice if loaders/client didn't have to deal with this, but could rather just set up the request (per fetch rules.) *nudge* *nudge* *wink* *wink* (Maybe worth asking too, but wouldn't this issue potentially affect other types of resources too?)
On 2016/09/28 23:25:41, Nate Chapin wrote: > On 2016/09/23 16:22:49, Nate Chapin wrote: > > On 2016/09/23 10:22:42, fs wrote: > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > File > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html > > > (right): > > > > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: > > > log('Loading <b>without</b> Access-Control-Allow-Origin header, with a > > redirect, > > > no "crossorigin" attribute on <video>'); > > > On 2016/09/23 at 09:16:10, foolip wrote: > > > > So, the problem was that if the redirect response itself didn't have > > > Access-Control-Allow-Origin, we'd still happily use the resource redirected > to > > > as long as that passes the CORS check? > > > > > > Isn't it a "same-origin request, cross-origin redirect" case? > > > > Same-origin request, cross-origin redirect case. We'd do the CORS policy check > > for the initial request, then blindly trust the redirect, even if the redirect > > was cross-origin. > > foolip@, you still OK with this given this explanation? Yes, LGTM, thanks for the explanation!
On 2016/09/29 07:53:51, fs wrote: > On 2016/09/29 at 07:52:01, fs wrote: > > On 2016/09/28 at 23:25:41, japhet wrote: > > > On 2016/09/23 16:22:49, Nate Chapin wrote: > > > > On 2016/09/23 10:22:42, fs wrote: > > > > > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > > > File > > > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html > > > > > (right): > > > > > > > > > > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > > > > > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: > > > > > log('Loading <b>without</b> Access-Control-Allow-Origin header, with a > > > > redirect, > > > > > no "crossorigin" attribute on <video>'); > > > > > On 2016/09/23 at 09:16:10, foolip wrote: > > > > > > So, the problem was that if the redirect response itself didn't have > > > > > Access-Control-Allow-Origin, we'd still happily use the resource > redirected to > > > > > as long as that passes the CORS check? > > > > > > > > > > Isn't it a "same-origin request, cross-origin redirect" case? > > > > > > > > Same-origin request, cross-origin redirect case. We'd do the CORS policy > check > > > > for the initial request, then blindly trust the redirect, even if the > redirect > > > > was cross-origin. > > > > > > foolip@, you still OK with this given this explanation? > > > > FWIW, this change LGTM. Tangentially though, it would be nice if > loaders/client didn't have to deal with this, but could rather just set up the > request (per fetch rules.) *nudge* *nudge* *wink* *wink* > > (Maybe worth asking too, but wouldn't this issue potentially affect other types > of resources too?) I don't think so? I'm ~clueless about the details of <track>s, but the current implementation seems to take as a given that we can immediately reject a track request with a corsAttributeState of "No CORS" if it is not same-origin. Most (I think all?) of the other users of crossorigin *can* be requested from a different origin when not specifying a crossorigin attribute. I'm going to go ahead and and land this, and if I've misunderstood, I'm happy to clean this up in a follow up CL.
The CQ bit was checked by japhet@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/29 at 19:00:17, japhet wrote: > On 2016/09/29 07:53:51, fs wrote: > > On 2016/09/29 at 07:52:01, fs wrote: > > > On 2016/09/28 at 23:25:41, japhet wrote: > > > > On 2016/09/23 16:22:49, Nate Chapin wrote: > > > > > On 2016/09/23 10:22:42, fs wrote: > > > > > > > > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > > > > File > > > > > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html > > > > > > (right): > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2367583002/diff/1/third_party/WebKit/LayoutTe... > > > > > > > > > > > > > third_party/WebKit/LayoutTests/http/tests/security/text-track-crossorigin.html:41: > > > > > > log('Loading <b>without</b> Access-Control-Allow-Origin header, with a > > > > > redirect, > > > > > > no "crossorigin" attribute on <video>'); > > > > > > On 2016/09/23 at 09:16:10, foolip wrote: > > > > > > > So, the problem was that if the redirect response itself didn't have > > > > > > Access-Control-Allow-Origin, we'd still happily use the resource > > redirected to > > > > > > as long as that passes the CORS check? > > > > > > > > > > > > Isn't it a "same-origin request, cross-origin redirect" case? > > > > > > > > > > Same-origin request, cross-origin redirect case. We'd do the CORS policy > > check > > > > > for the initial request, then blindly trust the redirect, even if the > > redirect > > > > > was cross-origin. > > > > > > > > foolip@, you still OK with this given this explanation? > > > > > > FWIW, this change LGTM. Tangentially though, it would be nice if > > loaders/client didn't have to deal with this, but could rather just set up the > > request (per fetch rules.) *nudge* *nudge* *wink* *wink* > > > > (Maybe worth asking too, but wouldn't this issue potentially affect other types > > of resources too?) > > I don't think so? I'm ~clueless about the details of <track>s, but the current implementation seems to take as a given that we can immediately reject a track request with a corsAttributeState of "No CORS" if it is not same-origin. Most (I think all?) of the other users of crossorigin *can* be requested from a different origin when not specifying a crossorigin attribute. Well, it's specified like that =) - older iterations of HTML I believe used to have a snazzy term for that, "cross-origin, default reject" or something. At that point in time <track> was the only thing using that "method" (maybe still is the case.) I suspect there may have been a gap in the CORS support when that was first implemented or something? I thought that at least DocumentResource and XSLStyleSheetResource(?) was no-cors same-origin too. I guess I can try to make a TC (if I remember.) > I'm going to go ahead and and land this, and if I've misunderstood, I'm happy to clean this up in a follow up CL. Sure, the above was just a thought, and trying to make similar gaps are plugged sooner rather than later (I can't see the bug, so I can only theorize of course...)
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Check CORS policy on redirect in TextTrackLoader BUG=633885 TEST=new case in http/tests/security/text-track-crossorigin.html ========== to ========== Check CORS policy on redirect in TextTrackLoader BUG=633885 TEST=new case in http/tests/security/text-track-crossorigin.html Committed: https://crrev.com/e99cc8e5a48ff4978d401c48a64f06649f647f3f Cr-Commit-Position: refs/heads/master@{#421919} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/e99cc8e5a48ff4978d401c48a64f06649f647f3f Cr-Commit-Position: refs/heads/master@{#421919} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
