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.

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

Add chunk type checking of requestForServiceWorker's body's stream #1199

Merged
merged 7 commits into from
Apr 15, 2021

Conversation

yoichio
Copy link
Contributor

@yoichio yoichio commented Mar 19, 2021

(See WHATWG Working Mode: Changes for more details.)

This change addresses the inconsistency discussed in #267 that a chunk type in stream body on uploading to the network is
limited only to Uint8Array but passing it to a service worker is not restricted.
This PR changes later to limiting Uint8Array as well.


Preview | Diff

fetch.bs Outdated Show resolved Hide resolved
domenic added a commit to whatwg/streams that referenced this pull request Mar 19, 2021
@yoichio
Copy link
Contributor Author

yoichio commented Apr 5, 2021

Ping?

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
domenic added a commit to whatwg/streams that referenced this pull request Apr 5, 2021
Copy link
Member

@yutakahirano yutakahirano left a comment

Choose a reason for hiding this comment

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

Please see https://github.com/whatwg/fetch#formatting for the formatting rules (and add missing rules if there are).

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Looks good to me modulo nits. Is everyone else happy with this as well. @ricea?

fetch.bs Outdated Show resolved Hide resolved
fetch.bs Outdated Show resolved Hide resolved
annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Apr 14, 2021
Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

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

lgtm

@annevk
Copy link
Member

annevk commented Apr 14, 2021

Tentative commit message:

Check chunk type of body's stream when passed to a service worker

Tests: https://github.com/web-platform-tests/wpt/pull/28203

@yoichio it's not clear to me how this fixes #267 as we still don't use BYOB streams, right? It sounds like that only matters for responses so doesn't block upload streams, but it's still something that needs to be done?

@ricea
Copy link
Collaborator

ricea commented Apr 14, 2021

I wonder if it's too strict? Do implementations accept other types than Uint8Array?

For comparison, a readable byte stream will accept any kind of ArrayBufferView and convert it to a Uint8Array.

@annevk
Copy link
Member

annevk commented Apr 14, 2021

For response streams I think all implementations have this restriction. Request streams are a "new" feature that's still being developed. As mentioned in #267 we decided on the current restriction in yutakahirano/fetch-with-streams#53, but as mentioned there we could revisit it. If we want to do that let's do it through a new issue though and make the change for all streams.

@yoichio
Copy link
Contributor Author

yoichio commented Apr 15, 2021

Updated the description.

@ricea
Copy link
Collaborator

ricea commented Apr 15, 2021

For response streams I think all implementations have this restriction. Request streams are a "new" feature that's still being developed. As mentioned in #267 we decided on the current restriction in yutakahirano/fetch-with-streams#53, but as mentioned there we could revisit it. If we want to do that let's do it through a new issue though and make the change for all streams.

Okay, thanks for the background!

@annevk annevk merged commit fc6f26f into whatwg:main Apr 15, 2021
@annevk
Copy link
Member

annevk commented Apr 15, 2021

Thanks @yoichio! And thanks @ricea for the review.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Apr 24, 2021
…one Uint8Array ReadableStream., a=testonly

Automatic update from web-platform-tests
Add FetchEvent test that should reject none Uint8Array ReadableStream

For whatwg/fetch#1199.
--

wpt-commits: 6b817d451a291d554b4fb9d8b5650d8b2d12ecc9
wpt-pr: 28203
yutakahirano pushed a commit to yutakahirano/streams that referenced this pull request Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants