Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Take tainted origin flag into account for the same origin check #834

Merged
merged 1 commit into from
Nov 20, 2018
Merged

Take tainted origin flag into account for the same origin check #834

merged 1 commit into from
Nov 20, 2018

Conversation

annevk
Copy link
Member

@annevk annevk commented Nov 16, 2018 •
edited by pr-preview bot
Loading

This also addresses #737 in that now A -> B -> A would be considered cross-origin even for "no-cors", but leaving that open for further plumbing in HTML et al to override that in select cases (e.g., <img>).

Fixes #756.

Preview | Diff

@annevk
Take tainted origin flag into account for the same origin check
dca2295 
This also addresses #737 in that now A -> B -> A would be considered cross-origin even for "no-cors", but leaving that open for further plumbing in HTML et al to override that in select cases (e.g., <img>).

Fixes #756.
@annevk annevk mentioned this pull request Nov 16, 2018
Open
@yutakahirano
Copy link
Member

but leaving that open for further plumbing in HTML et al to override that in select cases (e.g., ).

Can you elaborate on this part?

domfarolino
domfarolino approved these changes Nov 19, 2018
View reviewed changes
Copy link
Member

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would also like to hear about how overriding in something like HTML might be done.

@annevk
Copy link
Member Author

annevk commented Nov 19, 2018

My thinking was that for <img> A -> B -> A (without CORS) results in a "same origin" image that can be read through <canvas>. HTML would have to poke through the opaque response filter Fetch adds to enable that. (However, given #737 (comment) we might be able to change things around here if we wanted.)

@domfarolino
Copy link
Member

Ok so my understanding is that this spec change will make A -> B -> A "no-cors" <img> responses "opaque", thus asserting the behavior in web-platform-tests/wpt#14112. Having a way for HTML to poke through would allow browsers to avoid that if they wanted. FWIW Chrome is OK with this change, but will collect some metrics before implementation to see if it might break things.

So I guess if we can get all browsers on board then we wouldn't need to add prose allowing HTML to effectively get around the opaqueness of the response?

@annevk
Copy link
Member Author

annevk commented Nov 20, 2018

Yes, I'm leaving #737 open to study whether or not to change HTML around this, but I agree it would be nice not to change it, even if you can still workaround some cases with <iframe>.

@annevk annevk merged commit 986618a into master Nov 20, 2018
@annevk annevk deleted the annevk/same-origin-unless-tainted-origin-flag-is-set branch November 20, 2018 09:09
@domfarolino domfarolino mentioned this pull request Nov 23, 2018
Merged
aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 5, 2018
@yutakahirano
A=>B=>A redirect for "no-cors" should result in "opaque" response type
73b133f 
This CL follows whatwg/fetch#834. This CL
changes services/network only because I accidentally did the same thing
for blink::ResourceLoader, and what blink::ThreadableLoader does is
not important because blink::FetchManager has its own response type
setting logic.

Bug: 911959

Change-Id: I62f6ea9401e4fd7d87d01f6cb021feef8e06bb88
Reviewed-on: https://chromium-review.googlesource.com/c/1362774
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613956}
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Dec 11, 2018
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 11, 2018
@mfalken @chromium-wpt-export-bot
WPT: CSS: Add cross-origin redirect tests.
1178857 
This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 11, 2018
@mfalken
WPT: CSS: Add cross-origin redirect tests.
02d7d52 
This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
Reviewed-on: https://chromium-review.googlesource.com/c/1370162
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615475}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 11, 2018
@mfalken @chromium-wpt-export-bot
WPT: CSS: Add cross-origin redirect tests.
dd4f01d 
This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
Reviewed-on: https://chromium-review.googlesource.com/c/1370162
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615475}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Dec 11, 2018
@mfalken @chromium-wpt-export-bot
WPT: CSS: Add cross-origin redirect tests.
600dd6c 
This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
Reviewed-on: https://chromium-review.googlesource.com/c/1370162
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615475}
aarongable pushed a commit to chromium/chromium that referenced this pull request Dec 14, 2018
@mfalken
CSS: Use the response URL for base URL and type for security decisions.
8b30f52 
Use ResourceResponse::ResponseUrl() to set the base URL, and use
ResponseResponse::GetType() to determine whether the resonse is
CORS-same-origin.

This CL has three web-exposed changes.

1. Use the response URL rather than the last request URL as the base URL
   of the stylesheet. This aligns with the standard. See
   whatwg/fetch#146 and WPT results indicate
   Firefox, Edge, and Safari use the response URL. This only matters if
   the response came from a service worker, as the URLs only differ
   when the service worker intercepts the request and responds with a
   different URL via respondWith(fetch(other_url)).

   This is covered by the WPT:
   service-workers/service-worker/fetch-request-css-base-url.https.html

   The test doesn't completely pass yet because the search query part of
   the URL gets chopped off for FetchEvent.request.referrer, but the base
   URL is correct.

   Chrome Status: https://www.chromestatus.com/feature/5642183499579392

2. Consider A->B->A redirects to be cross-origin rather than
   same-origin. Previously, this was considered same-origin. See the
   discussion in whatwg/fetch#737 and change
   whatwg/fetch#834.

   This change makes the following WPT test pass:
   css/cssom/stylesheet-same-origin.sub.html

   It also affects the web test:
   http/tests/security/cannot-read-cssrules-redirect.html

   This test is updated to match the behavior change. It can be removed
   later since it is redundant with the WPT test.

3. Consider load failures to be cross-origin rather than same-origin.
   That is, accessing |styleSheet.rules| throws a SecurityError if the
   load failed.  This aligns with the specification:
   - cssRules checks the `origin-clean` flag:
     https://drafts.csswg.org/cssom/#dom-cssstylesheet-cssrules
   - This is set to true iff CORS-same-origin:
     https://html.spec.whatwg.org/multipage/links.html#link-type-stylesheet
   - CORS-same-origin is false on kError:
     https://html.spec.whatwg.org/multipage/urls-and-fetching.html#cors-same-origin

   This change makes the following WPT test pass:
   css/cssom/stylesheet-same-origin.sub.html

   It also affects the web tests:
   register-bypassing-scheme-partial.html
   require-sri-for-style-blocked.php

   These tests are updated to match the behavior change.

Chrome Status: https://www.chromestatus.com/feature/5642183499579392
Intent to Ship: https://groups.google.com/a/chromium.org/d/msg/blink-dev/7OSy00oxVpk/siufiQVBBwAJ

Bug: 911974
Change-Id: I9add3162596963eee66f60f339cfd9911bc151cd
Reviewed-on: https://chromium-review.googlesource.com/c/1367331
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616580}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 14, 2018
@mfalken @moz-wptsync-bot
Bug 1513156 [wpt PR 14452] - WPT: CSS: Add cross-origin redirect test…
207fb2f 
…s., a=testonly

Automatic update from web-platform-tests
WPT: CSS: Add cross-origin redirect tests.

This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
Reviewed-on: https://chromium-review.googlesource.com/c/1370162
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615475}

--

wpt-commits: 600dd6cb4295d0bcfc867b8877287d485d3b0e4e
wpt-pr: 14452
mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 15, 2018
@mfalken @moz-wptsync-bot
Bug 1513156 [wpt PR 14452] - WPT: CSS: Add cross-origin redirect test…
0009ac8 
…s., a=testonly

Automatic update from web-platform-tests
WPT: CSS: Add cross-origin redirect tests.

This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
Reviewed-on: https://chromium-review.googlesource.com/c/1370162
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615475}

--

wpt-commits: 600dd6cb4295d0bcfc867b8877287d485d3b0e4e
wpt-pr: 14452
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
@marco-c
Bug 1513156 [wpt PR 14452] - WPT: CSS: Add cross-origin redirect test…
32312c4 
…s., a=testonly

Automatic update from web-platform-tests
WPT: CSS: Add cross-origin redirect tests.

This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
Reviewed-on: https://chromium-review.googlesource.com/c/1370162
Reviewed-by: Rune Lillesveen <futharkchromium.org>
Commit-Queue: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#615475}

--

wpt-commits: 600dd6cb4295d0bcfc867b8877287d485d3b0e4e
wpt-pr: 14452

UltraBlame original commit: 004e8fbaeead9b556d1972586b1a17e7647b09e8
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
@marco-c
Bug 1513156 [wpt PR 14452] - WPT: CSS: Add cross-origin redirect test…
4a5e231 
…s., a=testonly

Automatic update from web-platform-tests
WPT: CSS: Add cross-origin redirect tests.

This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
Reviewed-on: https://chromium-review.googlesource.com/c/1370162
Reviewed-by: Rune Lillesveen <futharkchromium.org>
Commit-Queue: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#615475}

--

wpt-commits: 600dd6cb4295d0bcfc867b8877287d485d3b0e4e
wpt-pr: 14452

UltraBlame original commit: 004e8fbaeead9b556d1972586b1a17e7647b09e8
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
@marco-c
Bug 1513156 [wpt PR 14452] - WPT: CSS: Add cross-origin redirect test…
4252dc8 
…s., a=testonly

Automatic update from web-platform-tests
WPT: CSS: Add cross-origin redirect tests.

This adds tests that stylesheets that result from requests that were
redirected cross-origin are considered cross-origin.

Note that A->B->A redirects, which redirect from cross-origin to
same-origin, are considered cross-origin. See
whatwg/fetch#737 and
whatwg/fetch#834.

In Blink, we have redirect tests at
http/tests/security/cannot-read-cssrules-redirect.html. This WPT
addition will supersede that test, but I won't yet remove it since
it asserts the opposite for the A->B->A case. I can remove the test
when Blink changes to pass this WPT test.

Bug: 911974
Change-Id: Ie015c0390829299de7c29cff6685ddfcd774c66f
Reviewed-on: https://chromium-review.googlesource.com/c/1370162
Reviewed-by: Rune Lillesveen <futharkchromium.org>
Commit-Queue: Matt Falkenhagen <falkenchromium.org>
Cr-Commit-Position: refs/heads/master{#615475}

--

wpt-commits: 600dd6cb4295d0bcfc867b8877287d485d3b0e4e
wpt-pr: 14452

UltraBlame original commit: 004e8fbaeead9b556d1972586b1a17e7647b09e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@domfarolino domfarolino domfarolino approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@annevk @yutakahirano @domfarolino