Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2491903002:
Apply connect-src for link preload with no `as` value (Closed)
|
Created:
4 years, 1 month ago by Yoav Weiss Modified:
4 years, 1 month ago Reviewers:
Mike West CC:
blink-reviews, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionApply connect-src for link preload with no `as` value
This fixes an issue where connect-src was not applied to preloaded resources, due to their Context.
BUG=663620
Committed: https://crrev.com/872f27a9bceebb042082cd1b2f9043e5dd208200
Cr-Commit-Position: refs/heads/master@{#432158}
Patch Set 1 #
Total comments: 1
Messages
Total messages: 20 (9 generated)
The CQ bit was checked by yoav@yoav.ws 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...
Description was changed from ========== Apply connect-src for link preload with no `as` value BUG=663620 ========== to ========== Apply connect-src for link preload with no `as` value This fixes an issue where connect-src was not applied to preloaded resources, due to their Context. BUG=663620 ==========
yoav@yoav.ws changed reviewers: + mkwst@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2491903002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): https://codereview.chromium.org/2491903002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:833: case WebURLRequest::RequestContextSubresource: How confident are you that `Subresource` is targeted to `<link rel="preload">`? Based on https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res... it looks like other types might be effected.
On 2016/11/10 18:59:10, Mike West wrote: > https://codereview.chromium.org/2491903002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): > > https://codereview.chromium.org/2491903002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:833: case > WebURLRequest::RequestContextSubresource: > How confident are you that `Subresource` is targeted to `<link rel="preload">`? > Based on > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res... > it looks like other types might be effected. Do you know what other types use Resource::Raw and go through ResourceFetcher? I see it only used for preload with no `as` as well as (potentially, not sure when) inside DocumentThreadableLoader. If there are such resources, then maybe https://codereview.chromium.org/2257943002/ was a mistake. Also, what CSP rules are those resources currently subject to?
On 2016/11/10 at 22:18:02, yoav wrote: > On 2016/11/10 18:59:10, Mike West wrote: > > https://codereview.chromium.org/2491903002/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp (right): > > > > https://codereview.chromium.org/2491903002/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:833: case > > WebURLRequest::RequestContextSubresource: > > How confident are you that `Subresource` is targeted to `<link rel="preload">`? > > Based on > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res... > > it looks like other types might be effected. > > Do you know what other types use Resource::Raw and go through ResourceFetcher? I see it only used for preload with no `as` as well as (potentially, not sure when) inside DocumentThreadableLoader. Skimming through RawResource, it looks like the various things that run through it get assigned reasonable values. If you're pretty sure this is the way it works, I'd suggest doing a quick sanity check by `printf`ing resource requests in `CSP::allowRequest` that have a raw resource type, and then browsing around a bit in `content_shell`. If nothing pops up, then great!
On 2016/11/11 08:05:16, Mike West wrote: > On 2016/11/10 at 22:18:02, yoav wrote: > > On 2016/11/10 18:59:10, Mike West wrote: > > > > https://codereview.chromium.org/2491903002/diff/1/third_party/WebKit/Source/c... > > > File third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp > (right): > > > > > > > https://codereview.chromium.org/2491903002/diff/1/third_party/WebKit/Source/c... > > > third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp:833: case > > > WebURLRequest::RequestContextSubresource: > > > How confident are you that `Subresource` is targeted to `<link > rel="preload">`? > > > Based on > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Res... > > > it looks like other types might be effected. > > > > Do you know what other types use Resource::Raw and go through ResourceFetcher? > I see it only used for preload with no `as` as well as (potentially, not sure > when) inside DocumentThreadableLoader. > > Skimming through RawResource, it looks like the various things that run through > it get assigned reasonable values. > > If you're pretty sure this is the way it works, I'd suggest doing a quick sanity > check by `printf`ing resource requests in `CSP::allowRequest` that have a raw > resource type, and then browsing around a bit in `content_shell`. If nothing > pops up, then great! printfed, and got nothing out the ordinary.
LGTM. *shrug* What's the worst that can happen?
On 2016/11/15 08:29:25, Mike West wrote: > LGTM. *shrug* What's the worst that can happen? Famous last words! :) I guess we'll see
On 2016/11/15 08:35:25, Yoav Weiss wrote: > On 2016/11/15 08:29:25, Mike West wrote: > > LGTM. *shrug* What's the worst that can happen? > > Famous last words! :) > > I guess we'll see On a slightly more serious note, I think we should eventually move away from contexts and into Fetch destination, once we're sure enough that they're here to stay.
The CQ bit was checked by yoav@yoav.ws
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Apply connect-src for link preload with no `as` value This fixes an issue where connect-src was not applied to preloaded resources, due to their Context. BUG=663620 ========== to ========== Apply connect-src for link preload with no `as` value This fixes an issue where connect-src was not applied to preloaded resources, due to their Context. BUG=663620 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Apply connect-src for link preload with no `as` value This fixes an issue where connect-src was not applied to preloaded resources, due to their Context. BUG=663620 ========== to ========== Apply connect-src for link preload with no `as` value This fixes an issue where connect-src was not applied to preloaded resources, due to their Context. BUG=663620 Committed: https://crrev.com/872f27a9bceebb042082cd1b2f9043e5dd208200 Cr-Commit-Position: refs/heads/master@{#432158} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/872f27a9bceebb042082cd1b2f9043e5dd208200 Cr-Commit-Position: refs/heads/master@{#432158} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
