-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
CSP integration for javascript: URLs seems to be broken #4651
Comments
OK, so the action here is to write tests involving multi-global situations, where one global has CSP applied and one doesn't, and to try to invoke javascript: URLs. When we figure out which global's CSP policy applies, we can use that to specify a client. (Unless there is a clearly-sensible answer a priori, that we should get browsers to converge on regardless of what they currently do?) |
It's not at all clear to me what the "right" answer is, even from first principles of what the goal of preventing inline script via CSP is. And yes, I agree that we need tests to see what browsers are actually doing here. |
OK, I wrote some tests:
(They're not WPTs because I needed a simple way to run them in release browsers). The first two tests test doing javascript: loads in subframes that may or may not have CSP that blocks scripts. The second test has a CSP that blocks inline scripts on the toplevel page. The third and fourth tests are similar but use auxiliary browsing contexts (popups) instead of subframes. These tests do not try to tease out the difference between incumbent and entry objects for location sets. They also do not try to tease out when and whether CSP gets snapshotted and/or the exact timing of the CSP checks (this can matter given that Observed behavior seems to be as follows, as far as I can tell, where source and target correspond to the documents of source and browsingContext in https://html.spec.whatwg.org/multipage/browsing-the-web.html#javascript-protocol
|
It also matters because CSP violations get reported, and these reports are observable (possibly even within the page, with reporting API). If the Chrome behavior is decided on, then obviously the order of the two CSP checks needs to be specified, because it will affect the reporting that happens. |
Given the state of the spec and implementations, by the way, I'm pretty curious what the WPTs at https://wpt.fyi/results/content-security-policy/navigation are based on, if anything. |
So one plausible path to interop and greater security here is:
|
@mikewest Could you, or whoever is actively doing CSP work in Chrome, please take a look at #4651 (comment) and respond with thoughts? |
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047 --HG-- extra : moz-landing-system : lando
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047 --HG-- extra : moz-landing-system : lando
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047
Yeah, we had multiple issues open for this, and I fixed it on https://bugs.chromium.org/p/chromium/issues/detail?id=944213. I've duped 889891, thanks for pointing it out.
@andypaicu or @otherdaniel would probably know better than I do on CSP-specific stuff. It's not clear to me how intentional chrome's behavior in (2) was, but since it seems web-compatible and the most strict option, it seems good. One weird aspect, though, is that chromium currently performs the client CSP check before queueing the task to execute the javascript url, but performs the CSP check against the targeted document during the queued task. I have a WIP to do both checks before queueing the task (https://chromium-review.googlesource.com/c/chromium/src/+/1648755). That doesn't fit the spec's flow very well, but it avoids having to snapshot the client's CSP as described in (1). For (3), chromium currently omits all CSP checks for user-initated and extension-initiated navigations, though there are experiments in progress that might put some CSP restrictions on extensions (https://bugs.chromium.org/p/chromium/issues/detail?id=896041) |
The "client's CSP" doesn't depend on the targeted document, right? We could add more special-casing for javascript:, but ideally it would just behave as much like other navigations as possible. And other navigations really do want to snapshot the client CSP, not least because they may need access to it from a different process. |
That's probably right. I wasn't thinking about other navigations genuinely needing it. |
Other navigations need it for |
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047 UltraBlame original commit: 764fab821251784de22d5b45325974bc5598bacd
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047 UltraBlame original commit: 9276bdf5374dbc043838bc894a5e903af1cf428d
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047 UltraBlame original commit: 764fab821251784de22d5b45325974bc5598bacd
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047 UltraBlame original commit: 9276bdf5374dbc043838bc894a5e903af1cf428d
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047 UltraBlame original commit: 764fab821251784de22d5b45325974bc5598bacd
…erschb The removal of the second CSP check is fixing a regression introduced in bug 965637. See whatwg/html#4651 (comment) for details. We may want to re-introduce that check depending on the outcome of that issue, but if so we should do that only if the target document's principal subsumes our triggering principal. This commit will not allow bookmarklets to access subresources that CSP blocks, but will at least allow them to run. Differential Revision: https://phabricator.services.mozilla.com/D33047 UltraBlame original commit: 9276bdf5374dbc043838bc894a5e903af1cf428d
The spec seems to have changed since filing this ticket, since step 12 doesn't do that.
That id doesn't exist anymore in the spec.
Presumably, the roughly equivalent part of the current version of the spec is step 5 of https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate-to-a-javascript:-url.
So also in the current spec, there's no client, but one is needed. |
Don't know what they're based on, but here's a summary of some (not all) tests there:
Passes in all major browsers except Gecko. In the latter it'd pass, if 1 were fixed.
Passes in all major browsers. Footnotes |
"fetch navigations" set their request's client from their This requires checking WebKit/Safari's current behavior though {see 4).
Footnotes
|
https://github.com/web-platform-tests/wpt/blob/cce6f2d13f/content-security-policy/navigation/to-javascript-parent-initiated-parent-csp.html covers checking the source's CSP for |
WebKit (Gnome Web 46) behaves differently for
The former respects the source's CSP, the latter doesn't, for CC @annevk |
@mbrodesser-Igalia it's so awesome that you're investigating this in detail. I can't say I've been following everything, since I haven't paged in the comments from 2019 myself. But if you have concrete spec change proposals, or need someone to review web platform tests, please let me know, and I'd love to help. |
@natechapin: Edit: according to the explainer 3, no navigation events are fired for @domenic: WDYT? Footnotes |
Given Footnotes |
I think it still matters? Specifying deterministic behavior (via snapshotting) seems better than specifying nondeterministic behavior (where which type of task web developers queue might cause them to see the old vs. new CSP). |
…lobal hasn't. The behavior is currently insufficently specified, see [1] and other comments of that issue. The new tests cover the cases of [2] where only the parent global has a CSP. With additional tests for the <area> element. For Chrome and Firefox all tests pass. For WebKit some fail because of [3]. The set of tested APIs which accept `javascript:` URLs may not be exhaustive. To my knowledge there is no explicit list of all APIs which accept `javascript:` URLs. Tests for when the target global has CSP and the source global has not will be added in separate patches. [1] whatwg/html#4651 (comment) [2] whatwg/html#4651 (comment) [3] https://bugs.webkit.org/show_bug.cgi?id=13543 Differential Revision: https://phabricator.services.mozilla.com/D227223 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1567058 gecko-commit: 30e5049bce4199ff021bfdec23156f2c7c7b1fda gecko-reviewers: freddyb
…lobal hasn't. The behavior is currently insufficently specified, see [1] and other comments of that issue. The new tests cover the cases of [2] where only the parent global has a CSP. With additional tests for the <area> element. For Chrome and Firefox all tests pass. For WebKit some fail because of [3]. The set of tested APIs which accept `javascript:` URLs may not be exhaustive. To my knowledge there is no explicit list of all APIs which accept `javascript:` URLs. Tests for when the target global has CSP and the source global has not will be added in separate patches. [1] whatwg/html#4651 (comment) [2] whatwg/html#4651 (comment) [3] https://bugs.webkit.org/show_bug.cgi?id=13543 Differential Revision: https://phabricator.services.mozilla.com/D227223 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1567058 gecko-commit: 30e5049bce4199ff021bfdec23156f2c7c7b1fda gecko-reviewers: freddyb
… the target global hasn't. r=freddyb The behavior is currently insufficently specified, see [1] and other comments of that issue. The new tests cover the cases of [2] where only the parent global has a CSP. With additional tests for the <area> element. For Chrome and Firefox all tests pass. For WebKit some fail because of [3]. The set of tested APIs which accept `javascript:` URLs may not be exhaustive. To my knowledge there is no explicit list of all APIs which accept `javascript:` URLs. Tests for when the target global has CSP and the source global has not will be added in separate patches. [1] whatwg/html#4651 (comment) [2] whatwg/html#4651 (comment) [3] https://bugs.webkit.org/show_bug.cgi?id=13543 Differential Revision: https://phabricator.services.mozilla.com/D227223
… the target global hasn't. r=freddyb The behavior is currently insufficently specified, see [1] and other comments of that issue. The new tests cover the cases of [2] where only the parent global has a CSP. With additional tests for the <area> element. For Chrome and Firefox all tests pass. For WebKit some fail because of [3]. The set of tested APIs which accept `javascript:` URLs may not be exhaustive. To my knowledge there is no explicit list of all APIs which accept `javascript:` URLs. Tests for when the target global has CSP and the source global has not will be added in separate patches. [1] whatwg/html#4651 (comment) [2] whatwg/html#4651 (comment) [3] https://bugs.webkit.org/show_bug.cgi?id=13543 Differential Revision: https://phabricator.services.mozilla.com/D227223
… the target global hasn't. r=freddyb The behavior is currently insufficently specified, see [1] and other comments of that issue. The new tests cover the cases of [2] where only the parent global has a CSP. With additional tests for the <area> element. For Chrome and Firefox all tests pass. For WebKit some fail because of [3]. The set of tested APIs which accept `javascript:` URLs may not be exhaustive. To my knowledge there is no explicit list of all APIs which accept `javascript:` URLs. Tests for when the target global has CSP and the source global has not will be added in separate patches. [1] whatwg/html#4651 (comment) [2] whatwg/html#4651 (comment) [3] https://bugs.webkit.org/show_bug.cgi?id=13543 Differential Revision: https://phabricator.services.mozilla.com/D227223
…vigation request's client's global object's CSP list" Complements #494 in order make the spec consistent. Preparation to fix whatwg/html#4651.
… was snapshotted Preparation for fixing <whatwg/html#4651>
…pshotted CSP is checked during task creation, not during task execution (#49403) Preparation for fixing <whatwg/html#4651>.
Corresponding WPTs are now available:
Missing WPTs:
Footnotes
|
…which checks the parent's snapshotted CSP is checked during task creation, not during task execution, a=testonly Automatic update from web-platform-tests Add a `javascript:` URL navigation test which checks the parent's snapshotted CSP is checked during task creation, not during task execution (#49403) Preparation for fixing <whatwg/html#4651>. -- wpt-commits: 4d2142460b93bbc38274c950e04108ec2770a65f wpt-pr: 49403
…which checks the parent's snapshotted CSP is checked during task creation, not during task execution, a=testonly Automatic update from web-platform-tests Add a `javascript:` URL navigation test which checks the parent's snapshotted CSP is checked during task creation, not during task execution (#49403) Preparation for fixing <whatwg/html#4651>. -- wpt-commits: 4d2142460b93bbc38274c950e04108ec2770a65f wpt-pr: 49403
…which checks the parent's snapshotted CSP is checked during task creation, not during task execution, a=testonly Automatic update from web-platform-tests Add a `javascript:` URL navigation test which checks the parent's snapshotted CSP is checked during task creation, not during task execution (#49403) Preparation for fixing <whatwg/html#4651>. -- wpt-commits: 4d2142460b93bbc38274c950e04108ec2770a65f wpt-pr: 49403 UltraBlame original commit: 6b3c16be2a50b7d3e1a3b839c22bd7345780ebba
…which checks the parent's snapshotted CSP is checked during task creation, not during task execution, a=testonly Automatic update from web-platform-tests Add a `javascript:` URL navigation test which checks the parent's snapshotted CSP is checked during task creation, not during task execution (#49403) Preparation for fixing <whatwg/html#4651>. -- wpt-commits: 4d2142460b93bbc38274c950e04108ec2770a65f wpt-pr: 49403 UltraBlame original commit: 6b3c16be2a50b7d3e1a3b839c22bd7345780ebba
…which checks the parent's snapshotted CSP is checked during task creation, not during task execution, a=testonly Automatic update from web-platform-tests Add a `javascript:` URL navigation test which checks the parent's snapshotted CSP is checked during task creation, not during task execution (#49403) Preparation for fixing <whatwg/html#4651>. -- wpt-commits: 4d2142460b93bbc38274c950e04108ec2770a65f wpt-pr: 49403 UltraBlame original commit: 6b3c16be2a50b7d3e1a3b839c22bd7345780ebba
…vigation request's client's global object's CSP list" (#692) Complements #494 in order make the spec consistent. Preparation to fix whatwg/html#4651.
For simplicity, let's start at https://html.spec.whatwg.org/multipage/links.html#following-hyperlinks-2 for a javascript: URL.
Step 12 creates a new request. It's not clear what its client is at this point (see whatwg/fetch#907), but given that various parts of the navigation algorithm set the client to things (e.g. in https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch), I'm going to assume it's
null
at the moment.We then call into https://html.spec.whatwg.org/multipage/browsing-the-web.html#navigate which in step 13 calls into https://html.spec.whatwg.org/multipage/browsing-the-web.html#javascript-protocol. Step 2 of this calls into https://w3c.github.io/webappsec-csp/#should-block-navigation-request which in step 2 does:
So by that point we should really have a client set up, but we don't seem to.
In terms of what implementations do... For the specific case of
<a href>
, Chrome doesn't support targeting it, so there is only one sane global to use around. But forlocation.href
sets or modifications of thesrc
attribute of<iframe>
, it should be observable which global's CSP gets used here.The text was updated successfully, but these errors were encountered: