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

TypeError on Request.integrity with no-cors mode is a foot gun #583

Closed
wanderview opened this Issue Aug 24, 2017 · 5 comments

Comments

4 participants
@wanderview
Member

wanderview commented Aug 24, 2017

It seems a lot of people trying to use SRI with service workers where Request.integrity is implemented are running into problems:

https://bugzilla.mozilla.org/show_bug.cgi?id=1393439

The issue is that its pretty common to do <script integrity="hash"> in your document. This creates a no-cors request by default. If their service worker script passes through with fetch(evt.request) then they will get a TypeError. This is required by step 32.3 here:

https://fetch.spec.whatwg.org/#dom-request

Perhaps we should move this check out of the Request() constructor and into the fetch algorithm at the point an opaque Response is created. It seems its only really important if we are going to be returning an opaque Response and need to hide any information leakage about the contents.

@annevk

This comment has been minimized.

Member

annevk commented Aug 24, 2017

The Fetch algorithm already requires that the integrity check fails on an opaque response.

I agree that we should remove this exception as we've already removed similar exceptions in the past. Do you know which tests need to be updated or are you willing to update them in the eventual Firefox patch?

annevk added a commit that referenced this issue Aug 24, 2017

Using integrity with "no-cors" is fine same-origin
This exception also broke simple service worker passthrough handling.

Tests: ...

Fixes #583.

@wanderview wanderview changed the title from TypeError on Request.integrity with no-cors mode is a foot gub to TypeError on Request.integrity with no-cors mode is a foot gun Aug 24, 2017

@wanderview

This comment has been minimized.

Member

wanderview commented Aug 29, 2017

Note, we are going to move forward with this fix since its impacting sites. Also, I'm asking Tom to add a WPT test in gecko that will be sync'ed upstream.

@annevk

This comment has been minimized.

Member

annevk commented Aug 30, 2017

Thanks @wanderview, sounds good. Once that's done I'll merge this since Chrome hasn't implemented integrity yet it sounds like and they probably want to do the same as us.

cc @tyoshino @jakearchibald

@annevk annevk closed this in #584 Sep 1, 2017

annevk added a commit that referenced this issue Sep 1, 2017

Using integrity with "no-cors" is fine same-origin
This exception also broke simple service worker passthrough handling.

Tests: see https://bugzilla.mozilla.org/show_bug.cgi?id=1393439.

Fixes #583.
@nico3333fr

This comment has been minimized.

nico3333fr commented Sep 12, 2017

Just wanted to say thank you for moving on quickly on this, my 2 websites that are impacted say thanks too ;)

@rakuco

This comment has been minimized.

Contributor

rakuco commented Sep 21, 2017

For future reference: Chromium's tracking bug is https://bugs.chromium.org/p/chromium/issues/detail?id=766975. There's some flakiness with the WPT changes that landed a few days ago, but I'm still checking if it's caused by the tests or Blink.

MXEBot pushed a commit to mirror/chromium that referenced this issue Sep 22, 2017

Fetch: Do not check |integrity| in no-cors mode when constructing a R…
…equest

Adapt to whatwg/fetch#584, which removed the check
for |integrity|'s value when constructing a Request in no-cors mode.

Per whatwg/fetch#583: "the issue is that it's
pretty common to do <script integrity="hash"> in your document. This creates
a no-cors request by default. If their service worker script passes through
with fetch(evt.request) then they will get a TypeError."

Note that the external tests added in
web-platform-tests/wpt@96adf8a
are still flaky.

Bug: 766975, 766983
Change-Id: I1a787ee39ed94db49ba18f8675edc7dc3019ed26
Reviewed-on: https://chromium-review.googlesource.com/674694
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503412}

nareix pushed a commit to nareix/webrtc.third_party that referenced this issue Mar 5, 2018

Fetch: Do not check |integrity| in no-cors mode when constructing a R…
…equest

Adapt to whatwg/fetch#584, which removed the check
for |integrity|'s value when constructing a Request in no-cors mode.

Per whatwg/fetch#583: "the issue is that it's
pretty common to do <script integrity="hash"> in your document. This creates
a no-cors request by default. If their service worker script passes through
with fetch(evt.request) then they will get a TypeError."

Note that the external tests added in
web-platform-tests/wpt@96adf8a
are still flaky.

Bug: 766975, 766983
Change-Id: I1a787ee39ed94db49ba18f8675edc7dc3019ed26
Reviewed-on: https://chromium-review.googlesource.com/674694
Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#503412}
Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src
Cr-Mirrored-Commit: 7d97c3a8550e35ce8af356fa6e3e8a0553a61140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment