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

Spec: split up response validation into two stages, and fix a bug in it #1175

Merged
merged 5 commits into from
Jun 6, 2024
Merged

Spec: split up response validation into two stages, and fix a bug in it #1175

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce1fa78
Spec: split up response validation into two stages, and fix a bug in it
May 15, 2024
a2dbb0d
Document WASM behavior.
May 24, 2024
8214851
Elaborate
May 24, 2024
2deb061
Fix link, more precise whitespace.
May 24, 2024
4bedf6b
Fix the exists thing + encoding check
Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 39 additions & 13 deletions spec.bs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ spec: WebAssembly; urlPrefix: https://webassembly.github.io/spec/core/
spec: WebAssembly-js-api; urlPrefix: https://webassembly.github.io/spec/js-api/
type: dfn
text: compiling a WebAssembly module; url: #compile-a-webassembly-module
type: dfn
text: compiling a potential WebAssembly response; url: #compile-a-potential-webassembly-response
spec: WebIDL; urlPrefix: https://webidl.spec.whatwg.org/
type: dfn
text: convert a Web IDL arguments list to an ECMAScript arguments list; url: #web-idl-arguments-list-converting
Expand Down Expand Up @@ -2078,23 +2080,49 @@ The <dfn http-header><code>Ad-Auction-Allowed</code></dfn> HTTP response header
[=structured header=] whose value must be a [=structured header/boolean=].

<div algorithm>
To <dfn>validate fetching response</dfn> given a [=response=] |response|, null, failure, or a
[=byte sequence=] |responseBody|, and a [=string=] |mimeType|:
To <dfn>validate fetching response headers</dfn> given a [=response=] |response|:

1. If |responseBody| is null or failure, return false.
1. If [=header list/getting a structured field value|getting=] [:Ad-Auction-Allowed:] and
"`item`" from |response|'s [=response/header list=] does not return a true value, return false.
"`item`" from |response|'s [=response/header list=] does not return a true value, return false.
1. If |response|'s [=response/status=] is not an [=ok status=], return false.
1. Return true.
</div>

<div algorithm>
To <dfn>validate fetching response mime and body</dfn> given a [=response=] |response|, null,
failure, or a [=byte sequence=] |responseBody|, and a [=string=] |mimeType|:

1. If |responseBody| is null or failure, return false.
1. Let |headerMimeType| be the result of [=header list/extracting a MIME type=] from |response|'s
[=response/header list=].
1. Return false if any of the following conditions hold:
* |headerMimeType| is failure;
* |mimeType| is "`text/javascript`" and |headerMimeType| is not a [=JavaScript MIME type=];
* |mimeType| is "`application/json`" and |headerMimeType| is not a [=JSON MIME type=].
1. Let |mimeTypeCharset| be |headerMimeType|'s [=MIME type/parameters=]["`charset`"].
1. Return false if any of the following conditions hold:
* |mimeTypeCharset| does not [=map/exist=], or |mimeTypeCharset| is "utf-8", and |responseBody|
is not [=UTF-8=] encoded;
* |mimeTypeCharset| is "us-ascii", and not all bytes in |responseBody| are [=ASCII bytes=].
* |mimeType| is "`application/wasm`" and the result of [=header list/getting=] "`Content-Type`"
from |response|'s [=response/header list=] is null or not [=byte-case-insensitive=] equal to
"`application/wasm`".

Note: This was intended to match the behavior of [=compiling a potential WebAssembly
response=], but diverges by failing to remove leading and trailing HTTP whitespace.
Copy link
Collaborator

@qingxinwu qingxinwu May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Use [=HTTP tab or space=] since that's what [=get, decode, and split=] (called by extract a mime type).

HTTP whitespace contains U+000A LF, U+000D CR, and [=HTTP tab or space=], so it's different.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this case, the removal is motivated by https://webassembly.github.io/spec/web-api/#compile-a-potential-webassembly-response step 4 (which probably is motivated by proper parsing)

Changed to http tab or space, thanks.

I am not sure on changing our code simply because we're supposed to be super-conservative about backwards compatibility (and the change can break things by making wasm load when it failed to before), but it is a reasonable thing to consider changing in the code in my book since things that break would break would do so because they were doing something silly, not something reasonable.


1. If |mimeType| is not "`application/wasm`":
1. Let |mimeTypeCharset| be |headerMimeType|'s [=MIME type/parameters=]["`charset`"].
1. Return false if any of the following conditions hold:
* |mimeTypeCharset| does not [=map/exist=], or |mimeTypeCharset| is "utf-8", and |responseBody|
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is really weird to use [=map/exist=] anywhere away from the exact location where we pull the entry, even though I can technically understand the intent. For example, in https://infra.spec.whatwg.org/#example-map-get we only ever have map["test"] without "exists" following it after we check "exists" first, so I would try and move the "exists" condition up above to where we try and pull the value out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. Also noticed this had a logic error in that we don't actually accept things other than utf-8 or us-ascii, and fixed that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't seem done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might help if I commit and push....

is not [=UTF-8=] encoded;
* |mimeTypeCharset| is "us-ascii", and not all bytes in |responseBody| are [=ASCII bytes=].
1. Return true.
</div>

<div algorithm>
To <dfn>validate fetching response</dfn> given a [=response=] |response|, null, failure, or a
[=byte sequence=] |responseBody|, and a [=string=] |mimeType|:

1. If the result of [=validating fetching response headers=] given |response| is false, then
return false.
1. If the result of [=validating fetching response mime and body=] given |response|,
|responseBody|, |mimeType| is false, then return false.
1. Return true.
</div>

Expand Down Expand Up @@ -2159,10 +2187,8 @@ To <dfn>fetch WebAssembly</dfn> given a [=URL=] |url|:
1. Let |moduleObject| be null.
1. [=Fetch=] |request| with [=fetch/processResponseConsumeBody=] set to the following steps given
a [=response=] |response| and null, failure, or a [=byte sequence=] |responseBody|:
1. Set |moduleObject| to failure and return, if any of the following conditions hold:
* |responseBody| is null or failure;
* [=header list/getting a structured field value|Getting=] [:Ad-Auction-Allowed:] and "`item`"
from |response|'s [=response/header list=] does not return a true value.
1. If [=validate fetching response=] with |response|, |responseBody| and "`application/wasm`"
returns false, set |moduleObject| to failure and return.
1. Let |module| be the result of [=compiling a WebAssembly module=] |response|.
1. If |module| is [=error=], set |moduleObject| to failure.
1. Otherwise, set |moduleObject| to |module|.
Expand Down
Loading