Re: [whatwg/fetch] Define opaque-response blocking (updated) (PR #1755)

annevk left a comment (whatwg/fetch#1755)

<details>
<summary>AI feedback</summary>

Reviewed at 958e7eb. The CI build passes. The `fetch()` filtering in 958e7eb now matches both engines' defaults (Gecko's [`filterFetchResponse` = 2](https://github.com/mozilla-firefox/firefox/blob/dcafde486e11b7a7776137b92fec2a6f56744eab/modules/libpref/init/StaticPrefList.yaml#L2296-L2317) and Chromium's [empty response for blocked `fetch()` requests](https://github.com/chromium/chromium/blob/7cdaeeeed73bbe835e4517aabcd49bf570e5bd40/services/network/public/cpp/orb/orb_impl.cc#L500-L517)). Apart from the points below, the safelist check matches the explainer step for step. The claims about engine behaviour were checked against Gecko's `OpaqueResponseUtils.cpp`, Chromium's `orb_impl.cc`, and the tentative WPTs in `fetch/orb`.

**Blockers**

- [Step 3.4](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3555) lost "is an opaque-response-blocklisted MIME type or". A nosniff response labelled HTML, JSON, or XML whose bytes sniff as an image is now allowed at the image-sniffing step. [`img-mime-types-coverage.tentative.sub.html`](https://github.com/web-platform-tests/wpt/blob/095dcead130cf3e37eec684e2a8a95ad7c05ce74/fetch/orb/tentative/img-mime-types-coverage.tentative.sub.html#L37-L51) serves a PNG with nosniff and expects it to be blocked as `text/html`, the JSON types, and the XML types. #1442, the explainer, [Gecko](https://github.com/mozilla-firefox/firefox/blob/dcafde486e11b7a7776137b92fec2a6f56744eab/netwerk/protocol/http/OpaqueResponseUtils.cpp#L173-L177), and [Chromium](https://github.com/chromium/chromium/blob/7cdaeeeed73bbe835e4517aabcd49bf570e5bd40/services/network/public/cpp/orb/orb_impl.cc#L322-L344) all block these. The condition needs to be restored.
- [`audio/aac`, `audio/aacp`, `audio/mpeg`, and `multipart/x-mixed-replace`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3891-L3897) are now never-sniffed. That check runs before the media state and sniffing, so an `<audio>` pointing at a cross-origin MP3 served as `audio/mpeg` is always blocked, even though MP3s sniff fine. None of these four is in the explainer's or Gecko's never-sniffed list. In Gecko they're in [`IsOpaqueSafeListedSpecBreakingMIMEType`](https://github.com/mozilla-firefox/firefox/blob/dcafde486e11b7a7776137b92fec2a6f56744eab/netwerk/protocol/http/OpaqueResponseUtils.cpp#L53-L90), which is the *allow* list, under the default [`mediaExceptionsStrategy` = 1](https://github.com/mozilla-firefox/firefox/blob/dcafde486e11b7a7776137b92fec2a6f56744eab/modules/libpref/init/StaticPrefList.yaml#L2319-L2330). Chromium [lets all `audio/*` through without sniffing](https://github.com/chromium/chromium/blob/7cdaeeeed73bbe835e4517aabcd49bf570e5bd40/services/network/public/cpp/orb/orb_impl.cc#L94-L116). They should be removed. If the engines' exceptions are wanted, they could be specified as allowances, with the reason (MIME Sniffing can't detect ADTS AAC or MJPEG).
- [*obtain a copy of the first 1024 bytes of response*](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3713-L3763) never finishes for a body with at least one chunk. A TransformStream that is [set up](https://streams.spec.whatwg.org/#transformstream-set-up) has a readable high-water mark of 0, so it starts with backpressure, and its [sink write](https://streams.spec.whatwg.org/#transform-stream-default-sink-write-algorithm) doesn't run the transform until something pulls the readable side. Nothing reads the body until this check returns, so `first1024Bytes` stays null and the wait never ends. (The "in parallel" block also races with that wait.) Consider [teeing](https://streams.spec.whatwg.org/#readablestream-tee) the body's stream, as *clone a body* does, putting one branch back on the response, and reading from the other until there are 1024 bytes or the stream ends, then canceling it.
- [`ParseText`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3831) returns "a Parse Node or a non-empty List of SyntaxError objects" ([ECMA-262](https://tc39.es/ecma262/#sec-parsetext)), never a Script Record. So this step never returns true, and every mislabeled script that reaches it is blocked. One example is [the UTF-16-with-BOM script served as `application/json`](https://github.com/web-platform-tests/wpt/blob/095dcead130cf3e37eec684e2a8a95ad7c05ce74/fetch/orb/tentative/known-mime-type.sub.any.js#L56-L63). Checking for a Parse Node instead would fix this, and the [Script Record anchor](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L62) can then go.
- ["subsequent"](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3559) lets everything through apart from the MIME-type early exits, but nothing limits when a caller may [set it](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L2285-L2288).
  - whatwg/html#12914 sets it for every later range request to the media URL, and those requests follow redirects. So an attacker's media server can serve a real first range, then redirect a later range request to a cross-origin endpoint that ignores `Range` and returns 200 JSON or HTML. That response passes.
  - The explainer's [implementation considerations](https://github.com/annevk/orb#implementation-considerations) only allow "subsequent" for a URL that has already sniffed as audio or video. Chromium enforces this: it [allows mid-resource range responses only for URLs it stored after sniffing](https://github.com/chromium/chromium/blob/7cdaeeeed73bbe835e4517aabcd49bf570e5bd40/services/network/public/cpp/orb/orb_impl.cc#L366-L374).
  - Callers can't simply turn off redirects, because [main fetch rejects no-cors requests whose redirect mode isn't "follow"](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L5474-L5475).
  - Consider honouring "subsequent" only when the request wasn't redirected, and stating on the field that it may only be used for the URL of an initial response that sniffed as audio or video.
- Process:
  - The tests are still tentative, and they disagree with this PR in two places: the nosniff case above, and [`expected_block`](https://github.com/web-platform-tests/wpt/blob/095dcead130cf3e37eec684e2a8a95ad7c05ce74/fetch/orb/resources/utils.js#L70-L76), which expects a blocked no-cors `fetch()` to reject rather than resolve with an empty body as the new note says.
  - No implementation bugs are filed.
  - The PR description doesn't have a commit message yet; the first commit's message is a squash of work-in-progress notes.
  - The participation check for @sefeng211 is still pending.

**Suggestions**

- Making [DASH, HLS, and VTT](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3858) never-sniffed matches the explainer, but both engines let these types through before the blocklist: Gecko's ["Avoid trouble with DASH/HLS"](https://github.com/mozilla-firefox/firefox/blob/dcafde486e11b7a7776137b92fec2a6f56744eab/netwerk/protocol/http/OpaqueResponseUtils.cpp#L55-L61), and Chromium [treats them as audio/video MIME types](https://github.com/chromium/chromium/blob/7cdaeeeed73bbe835e4517aabcd49bf570e5bd40/services/network/public/cpp/orb/orb_impl.cc#L38-L86). As specified, a cross-origin no-cors native HLS or DASH stream (`<video src=….m3u8>` without `crossorigin`) can't play. Consider confirming that this is intended.
- The [`Request` constructor](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L9394-L9459) doesn't copy the two new request fields, so `fetch(event.request)` resets them to "N/A" and UTF-8. Since 958e7eb runs the check for `fetch()`, a pass-through service worker can no longer serve cross-origin no-cors media: media that sniffs as audio or video with "N/A" returns false, so the body is removed. Consider copying the fields, which is safe once "subsequent" is tied to a URL as described above.
- Consider saying what ["initial" and "subsequent"](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L2285-L2296) mean and who sets them. The neighbouring notes name the caller, e.g. "for exclusive use by HTML's navigate algorithm".
- The [Content-Range ABNF](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L4339-L4378) sits in the CORS "HTTP new-header syntax" section, and integer parsing is [left as XXX](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3644-L3645). Consider a step-by-step *parse a single byte content-range value* next to [*parse a single range header value*](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L1374). That would settle the XXX with "interpreted as decimal number", [as that algorithm does](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L1400). It would also make the railroad diagram unnecessary, return a tuple, and leave room for the [RFC 9110](https://httpwg.org/specs/rfc9110.html#field.content-range) validity checks that [Gecko already does](https://github.com/mozilla-firefox/firefox/blob/dcafde486e11b7a7776137b92fec2a6f56744eab/netwerk/protocol/http/OpaqueResponseUtils.cpp#L197).
- The `previousResponse` branch of [*validate a partial response*](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3651-L3699) can't be reached from Fetch. [Background Fetch](https://wicg.github.io/background-fetch/#validate-a-partial-response) still has its own unexported copies of all three definitions, and HTML's [*verify a media response*](https://html.spec.whatwg.org/multipage/media.html#verify-a-media-response) links to Background Fetch's *extract content-range values*. Consider exporting these from Fetch and following up in Background Fetch and HTML.
- Consider removing the leftover [`<p class=XXX>`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3644-L3645) and the ["Maybe that is better?" comment](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3833-L3835). The PR description says the ParseText question is settled.
- In [*determine if response is JavaScript and not JSON*](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3774-L3839):
  - `processBody` records the bytes before it replaces the body, so the waiting step can continue too early. Consider swapping the two steps.
  - The JSON check decodes as UTF-8, while the JavaScript check uses the legacy encoding. Also, [*parse JSON bytes to a JavaScript value*](https://infra.spec.whatwg.org/#parse-json-bytes-to-a-javascript-value) calls `%JSON.parse%`, which needs a realm that isn't available in parallel. Consider decoding once and testing the text against the ECMA-404 JSON grammar.
  - Consider restoring the explainer's note that this full read can leak the response size (annevk/orb#22).
- Both null-body steps ([beacon/ping](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L6341-L6342) and [`fetch`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L6352-L6353)) could reuse main fetch's wording, ["set *internalResponse*'s body to null and disregard any enqueuing toward it (if any)"](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L5633-L5635). Also, `response` and `internalResponse` are the same object there; consider using just one.
- A 3xx response without `Location` is [returned as-is](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L6462), so its body [skips this check](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L6336-L6337). Consider exempting only redirects that are actually followed (see the [question on #1442](https://github.com/whatwg/fetch/pull/1442#discussion_r885556748)).
- [`<code dfn-type=http-header>`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3524) doesn't create a link. Consider `<a http-header><code>Cross-Origin-Resource-Policy</code></a>`, [as used elsewhere](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L4470).
- Consider adding Sean Feng to the [Acknowledgments](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L11033-L11034).

**Nits**

- [`<a>Fully read</a>`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3801): the file uses `<a for=body>fully read</a>`.
- [`{{fetch()}}`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L6358): the file writes `<a method><code>fetch()</code></a>` [elsewhere](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L183).
- The never-sniffed list isn't sorted: [`audio/mpeg` should precede `audio/mpegurl`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3893-L3894), and [`text/csv` should precede `text/event-stream`](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3898-L3899).
- ["New MIME type sets"](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3842) is explainer wording; something like "Opaque-response MIME type sets" fits the standard better.
- The [railroad note](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L4351-L4380) uses quoted attributes, 2-space indentation, and no `<p>`, and there's only one blank line before the next `<h4>`.
- ["N/A"](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L2286-L2288) stands out among Fetch's lowercase enum values. Something like "none" would be cheap to switch to now.
- [This step](https://github.com/whatwg/fetch/blob/958e7ebf0a1249ae914f6c176f0951fc597fe290/fetch.bs#L3658-L3660) destructures the result before checking for failure.

</details>


-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/1755#issuecomment-6627472873
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/pull/1755/c6627472873@github.com>