Re: [whatwg/fetch] Allow range header to be set by APIs (#560)

annevk commented on this pull request.

This generally looks pretty reasonable, assuming I understand it all correctly. It's a little hard to think through all the implications in the abstract. Lots of nits though.

> @@ -1277,6 +1288,29 @@ or "<code>worker</code>".
   </ol>
 </ol>
 
+<hr>
+
+<p>To <dfn export for=request id=concept-request-add-range-header>add a range header</dfn> to a <a
+for=/>request</a> <var>request</var>, with an integer <var>first</var>, and an optional integer
+<var>last</var>, run these steps:
+
+<ol>
+ <li><p>Let <var>rangeValue</var> be `<code>bytes </code>`.
+
+ <li><p><a>Encode</a> <var>first</var> and append the result to <var>rangeValue</var>.

Encode takes two parameters, neither of which is an integer. You probably want to phrase this differently somehow.

> @@ -1277,6 +1288,29 @@ or "<code>worker</code>".
   </ol>
 </ol>
 
+<hr>
+
+<p>To <dfn export for=request id=concept-request-add-range-header>add a range header</dfn> to a <a
+for=/>request</a> <var>request</var>, with an integer <var>first</var>, and an optional integer
+<var>last</var>, run these steps:
+
+<ol>
+ <li><p>Let <var>rangeValue</var> be `<code>bytes </code>`.
+
+ <li><p><a>Encode</a> <var>first</var> and append the result to <var>rangeValue</var>.
+
+ <li><p>Append `<code>-</code>` to <var>rangeValue</var>.
+
+ <li><p>If <var>last</var> is provided, <a>encode</a> it and append the result to

then encode* (though encode is wrong per above)

I prefer to follow "If" conditionals by "then" for clarity.

> +
+<ol>
+ <li><p>Let <var>rangeValue</var> be `<code>bytes </code>`.
+
+ <li><p><a>Encode</a> <var>first</var> and append the result to <var>rangeValue</var>.
+
+ <li><p>Append `<code>-</code>` to <var>rangeValue</var>.
+
+ <li><p>If <var>last</var> is provided, <a>encode</a> it and append the result to
+ <var>rangeValue</var>.
+
+ <li><p><a for="header list">Append</a> `<code>Range</code>`/<var>rangeValue</var> to
+ <var>request</var>'s <a for=request>header list</a>.
+</ol>
+
+<p class=note>Ranges are inclusive. A range where <var>first</var> is 0 and <var>last</var> is

Perhaps say something like "A Range header denotes an inclusive byte range." as ranges in general (as a concept) are not necessarily inclusive.

> @@ -3186,6 +3224,27 @@ optional <i>CORS flag</i> and <i>CORS-preflight flag</i>, run these steps:
     <!-- not resetting actualResponse since it's no longer used anyway -->
   </ol>
 
+ <li>
+  <p>If <var>response</var>'s <a for=response>status</a> is <code>206</code>,
+  and <var>response</var>'s <a for=response>range requested flag</a> is set, and
+  <var>request</var>'s <a for=request>header list</a> does not <a for="header list">contain</a>
+  "<code>`Range`</code>", then return a <a>network error</a>.

Please only use a single "and" when there's multiple conditions that need to be met.

> @@ -3186,6 +3224,27 @@ optional <i>CORS flag</i> and <i>CORS-preflight flag</i>, run these steps:
     <!-- not resetting actualResponse since it's no longer used anyway -->
   </ol>
 
+ <li>
+  <p>If <var>response</var>'s <a for=response>status</a> is <code>206</code>,

Shouldn't you look at actualResponse here?

> @@ -3186,6 +3224,27 @@ optional <i>CORS flag</i> and <i>CORS-preflight flag</i>, run these steps:
     <!-- not resetting actualResponse since it's no longer used anyway -->
   </ol>
 
+ <li>
+  <p>If <var>response</var>'s <a for=response>status</a> is <code>206</code>,
+  and <var>response</var>'s <a for=response>range requested flag</a> is set, and
+  <var>request</var>'s <a for=request>header list</a> does not <a for="header list">contain</a>
+  "<code>`Range`</code>", then return a <a>network error</a>.
+
+  <div class=note>
+   <p>Traditionally, APIs accept a ranged response even if a range wasn't requested. However, we
+   need to prevent a partial response from an earlier ranged request being provided to an API that
+   didn't make a range request.
+
+   <p>Example attack: A media element is used to request a range of a cross-origin HTML resource.

Instead of saying "Example attack" it would be more consistent to use class=example here, though I don't know how that would render. I guess we should just try. Perhaps just make it a note followed by an example, each as their own paragraph.

> @@ -4442,9 +4501,8 @@ objects.</span>
  "<code>request</code>" and <var>name</var> is a <a>forbidden header name</a>,
  return.
 
- <li><p>Otherwise, if <a for=Headers>guard</a> is
- "<code>request-no-cors</code>" and <var>name</var>/<var>value</var> is not a
- <a>CORS-safelisted request-header</a>, return.
+ <li><p>Otherwise, if <a for=Headers>guard</a> is "<code>request-no-cors</code>" and
+ <var>name</var>/<var>value</var> is not a <a>CORS-safelisted request-header</a>, return.

It seems you didn't change this line, either undo the changes or make it say "then return".

> @@ -4509,9 +4587,9 @@ method, when invoked, must run these steps:
  "<code>request</code>" and <var>name</var> is a <a>forbidden header name</a>,
  return.
 
- <li><p>Otherwise, if <a for=Headers>guard</a> is
- "<code>request-no-cors</code>" and <var>name</var>/`<code>invalid</code>` is
- not a <a>CORS-safelisted request-header</a>, then return.
+ <li><p>Otherwise, if <a for=Headers>guard</a> is "<code>request-no-cors</code>" and
+ <var>name</var>/`<code>invalid</code>` is not a <a>CORS-safelisted request-header</a>, and
+ <var>name</var> is not a <a>privileged no-cors request-header name</a>, then return.

Just one "and" please.

> @@ -4522,6 +4600,9 @@ method, when invoked, must run these steps:
 
  <li><p><a for="header list">Delete</a> <var>name</var> from
  <a for=Headers>header list</a>.
+
+ <li><p>If <a for=Headers>guard</a> is "<code>request-no-cors</code>", then
+ <a for=Headers>remove privileged no-cors request headers</a> from <a>context object</a>.

I'm not sure how consistent Fetch is about it, but I've started using "the context object" elsewhere.

> @@ -4566,9 +4647,8 @@ method, when invoked, must run these steps:
  "<code>request</code>" and <var>name</var> is a <a>forbidden header name</a>,
  return.
 
- <li><p>Otherwise, if <a for=Headers>guard</a> is
- "<code>request-no-cors</code>" and <var>name</var>/<var>value</var> is not a
- <a>CORS-safelisted request-header</a>, return.
+ <li><p>Otherwise, if <a for=Headers>guard</a> is "<code>request-no-cors</code>" and
+ <var>name</var>/<var>value</var> is not a <a>CORS-safelisted request-header</a>, return.

then return* or undo the formatting changes.

> @@ -4577,6 +4657,9 @@ method, when invoked, must run these steps:
  <li><p><a for="header list">Set</a>
  <var>name</var>/<var>value</var> in
  <a for=Headers>header list</a>.
+
+ <li><p>If <a for=Headers>guard</a> is "<code>request-no-cors</code>", then
+ <a for=Headers>remove privileged no-cors request headers</a> from <a>context object</a>.

the context object*

> - <a for=request>credentials mode</a>,
- <a for=request>cache mode</a> is
- <var>request</var>'s
- <a for=request>cache mode</a>,
- <a for=request>redirect mode</a> is
- <var>request</var>'s
- <a for=request>redirect mode</a>,
- <a for=request>integrity metadata</a> is
- <var>request</var>'s
- <a for=request>integrity metadata</a>, and
- <a>keepalive flag</a> is <var>request</var>'s <a>keepalive flag</a>.
+ <li>
+  <p>Set <var>request</var> to a new <a for=/>request</a> with the following properties:
+
+  <dl>
+   <dt><a for=request>url</a> <dd><var>request</var>'s <a for=request>current url</a>.

`<dd>` on a new line

> @@ -5156,17 +5244,19 @@ constructor must run these steps:
    <li><p>Set <var>request</var>'s <a for=request>referrer</a> to
    "<code>client</code>"
 
-   <li><p>Set <var>request</var>'s
-   <a for=request>referrer policy</a> to the empty string.
+   <li>
+    <p>Set <var>request</var>'s <a for=request>referrer policy</a> to the empty string.
+
+    <p class=note>This is done to ensure that when a service worker "redirects" a request, e.g.,
+    from an image in a cross-origin stylesheet, and makes modifications, it no longer appears to

style sheet* (throughout)

> @@ -5156,17 +5244,19 @@ constructor must run these steps:
    <li><p>Set <var>request</var>'s <a for=request>referrer</a> to
    "<code>client</code>"
 
-   <li><p>Set <var>request</var>'s
-   <a for=request>referrer policy</a> to the empty string.
+   <li>
+    <p>Set <var>request</var>'s <a for=request>referrer policy</a> to the empty string.
+
+    <p class=note>This is done to ensure that when a service worker "redirects" a request, e.g.,
+    from an image in a cross-origin stylesheet, and makes modifications, it no longer appears to
+    come from the original source (i.e., the cross-origin stylesheet), but instead from the service
+    worker that "redirected" the request. This is important as the original source might not even be
+    able to generate the same kind of requests as the service worker. Services that trust the
+    original source could therefore be exploited were this not done, although that is somewhat
+    farfetched.

Why did you move this note by the way? It really applies to the whole "if any of init's member are present" clause.

> @@ -5451,10 +5550,18 @@ run these steps:
 
  <li><p>Set <var>clonedRequestObject</var>'s <a for=Request>request</a> to <var>clonedRequest</var>.
 
- <li><p>Set <var>clonedRequestObject</var>'s <a for=Request>headers</a> to a new {{Headers}} object
- whose <a for=Headers>header list</a> is set to <var>clonedRequest</var>'s
- <a for=Headers>header list</a>, and <a for=Headers>guard</a> is <a>context object</a>'s
- <a for=Request>headers</a>' <a for=Headers>guard</a>.
+ <li>
+  <p>Set <var>clonedRequestObject</var>'s <a for=Request>headers</a> to a new {{Headers}} object
+  whose:
+
+  <ul>
+   <li><p><a for=Headers>header list</a> is set to <var>clonedRequest</var>'s
+   <a for=Headers>header list</a>.
+
+   <li><p><a for=Headers>guard</a> is <a>context object</a>'s <a for=Request>headers</a>'s

the context object*

> @@ -5451,10 +5550,18 @@ run these steps:
 
  <li><p>Set <var>clonedRequestObject</var>'s <a for=Request>request</a> to <var>clonedRequest</var>.
 
- <li><p>Set <var>clonedRequestObject</var>'s <a for=Request>headers</a> to a new {{Headers}} object
- whose <a for=Headers>header list</a> is set to <var>clonedRequest</var>'s
- <a for=Headers>header list</a>, and <a for=Headers>guard</a> is <a>context object</a>'s
- <a for=Request>headers</a>' <a for=Headers>guard</a>.
+ <li>
+  <p>Set <var>clonedRequestObject</var>'s <a for=Request>headers</a> to a new {{Headers}} object
+  whose:

It's a little weird we use `<ol>` here and for a similar thing above we use `<dl>`. I'm not really sure what style I prefer though.

> @@ -1360,6 +1394,10 @@ specified. [[!CSP]]
 `<a http-header><code>Access-Control-Expose-Headers</code></a>` header. This list is used by a
 <a>CORS filtered response</a> to determine which headers to expose.
 
+<p>A <a for=/>response</a> has an associated
+<dfn for=response id=concept-response-range-requested-flag>range requested flag</dfn>, which is
+initially unset.

It'd be good to add a note here about the purpose. Also, "range-requested" perhaps?

> @@ -1360,6 +1394,10 @@ specified. [[!CSP]]
 `<a http-header><code>Access-Control-Expose-Headers</code></a>` header. This list is used by a
 <a>CORS filtered response</a> to determine which headers to expose.
 
+<p>A <a for=/>response</a> has an associated
+<dfn for=response id=concept-response-range-requested-flag>range requested flag</dfn>, which is
+initially unset.

Also, do we need to export this or is it fine for just internal usage?

> @@ -4477,6 +4538,23 @@ run these steps:
  <var>key</var>/<var>value</var> to <var>headers</var>.
 </ol>
 
+<p>To
+<dfn for=Headers id=concept-headers-remove-privileged-no-cors-request-headers>remove privileged no-cors request headers</dfn>
+from a {{Headers}} object (<var>headers</var>), run these steps:

It seems this would be a little better as instead iterating over the privileged no-cors request-header names and then deleting (concept-header-list-delete) them.

> @@ -446,6 +446,17 @@ documented in <a href=#cors-protocol-exceptions>CORS protocol exceptions</a>.
 <p>A <dfn export>CORS non-wildcard request-header name</dfn> is a <a>byte-case-insensitive</a> match
 for `<code>Authorization</code>`.
 
+<p>A <dfn export>privileged no-cors request-header name</dfn> is a <a for=/>header</a>
+<a for=header>name</a> that is a <a>byte-case-insensitive</a> match for one of
+
+<ul class=brief>
+ <li>`<code>Range</code>`
+</ul>
+
+<p class="note no-backref">These are headers that <span class=allow-2119>may</span> be set by

Use can or could instead? No reason to use may I think. If you think it should be a normative statement then this should not be a note.

> @@ -446,6 +446,17 @@ documented in <a href=#cors-protocol-exceptions>CORS protocol exceptions</a>.
 <p>A <dfn export>CORS non-wildcard request-header name</dfn> is a <a>byte-case-insensitive</a> match
 for `<code>Authorization</code>`.
 
+<p>A <dfn export>privileged no-cors request-header name</dfn> is a <a for=/>header</a>
+<a for=header>name</a> that is a <a>byte-case-insensitive</a> match for one of
+
+<ul class=brief>
+ <li>`<code>Range</code>`
+</ul>
+
+<p class="note no-backref">These are headers that <span class=allow-2119>may</span> be set by

Perhaps this note should also point out the "add a range header" primitive.

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