Re: [whatwg/fetch] HTTPS upgrades proposal (PR #1655)

@annevk commented on this pull request.

Apologies for the delay here. I blame European summer.

So this still applies:

> It would help a lot if this was rebased as a single commit describing the changes it is making. (I attempted to rebase locally against main in order to address the CI issue but immediately ran into a merge conflict.)

In particular it seems Build (CI) hasn't even run for this change, which is a problem.

> @@ -5143,12 +5267,14 @@ these steps:
      <a>filtered response</a>; otherwise to <var>response</var>'s
      <a for="filtered response">internal response</a>.
 
+     <li><p>If <var>response</var>'s <a for=response>type</a> is "<code>error</code>" then

```suggestion
     <li><p>If <var>response</var>'s <a for=response>type</a> is "<code>error</code>", then
```

> @@ -4422,6 +4538,14 @@ steps:
 
  <li><p><a>Upgrade <var>request</var> to a potentially trustworthy URL, if appropriate</a>.
 
+ <li>
+  <p>Optionally, run <a>upgrade an HTTP request</a> algorithm on <var>request</var>, if appropriate</a>.
+
+  <p class=note>HTTPS upgrading only applies to requests with <a>HTTP(S) scheme</a>s, but it's done
+  in <a>main fetch</a> instead of <a>HTTP fetch</a> to ensure that <a>upgrade a mixed content

`<a>` elements are always on a single line, so you need to wrap before (and likely after because this is a long one).

> +  <p class=note>This means that the upgrade failed and initiates a fallback load.
+  <ol>
+   <li><p>Let <var>fallbackResponse</var> be a new <a for=/>Response</a> with its
+   <code>Location</code> header set to <var>request</var>'s <a for=request>HTTPS upgrade fallback
+   URL</a>, and its <a for="response">status</a> set to 307.
+
+   <li>Return <var>fallbackResponse</var>.
+  </ol>
+
+ <li>
+ <p>Return <var>response</var>.
+ <p class=note>This means the upgrade was successful.
+
+</ol>
+
+<p class=note>

No need for a newline here.

> + <var>response</var>.
+
+ <li>
+  <p>If <var>response</var> is a network error, run the following steps:
+  <p class=note>This means that the upgrade failed and initiates a fallback load.
+  <ol>
+   <li><p>Let <var>fallbackResponse</var> be a new <a for=/>Response</a> with its
+   <code>Location</code> header set to <var>request</var>'s <a for=request>HTTPS upgrade fallback
+   URL</a>, and its <a for="response">status</a> set to 307.
+
+   <li>Return <var>fallbackResponse</var>.
+  </ol>
+
+ <li>
+ <p>Return <var>response</var>.
+ <p class=note>This means the upgrade was successful.

Indentation here is wrong.

>  
+   <li><p><var>request</var>'s <a for="request">URL</a>'s <a for="url">scheme</a> is not
+   "<code>http</code>"
+
+   <li>
+    <p><var>request</var>'s <a for="request">URL</a>'s <a for="url">host</a> is exempted from
+    upgrades in an <a>implementation-defined</a> way.
+
+    <p class=example id="example-https-upgrades-exempted-hosts">If <a for=url>host</a> is a
+    non-registrable or non-assignable domain name such as .local or an IP address that falls in a
+    range reserved for non-publicly routable networks, the implementation might return without
+    modifying <var>request</var>.

Do we really need to leave this implementation-defined? I guess we might have to for some things, but a lot of this is defined.

> @@ -3241,7 +3253,111 @@ through TLS using ALPN. The protocol cannot be spoofed through HTTP requests in
 </ol>
 </div>
 
+<h3 id=https-upgrades>HTTPS upgrading</h3>
+
+<p>User agents may optionally upgrade requests with URLs that are not <a>potentially trustworthy

No wrapping inside `<a>`.

> @@ -3241,7 +3253,111 @@ through TLS using ALPN. The protocol cannot be spoofed through HTTP requests in
 </ol>
 </div>
 
+<h3 id=https-upgrades>HTTPS upgrading</h3>

Needs more newlines before the `<h3>` I think. (Might in general want to double check this PR is not eating newlines in front of headings.)

> +    upgrades in an <a>implementation-defined</a> way.
+
+    <p class=example id="example-https-upgrades-exempted-hosts">If <a for=url>host</a> is a
+    non-registrable or non-assignable domain name such as .local or an IP address that falls in a
+    range reserved for non-publicly routable networks, the implementation might return without
+    modifying <var>request</var>.
+  </ul>
+ </li>
+
+ <li>
+  <p>If <var>request</var>'s <a for=request>HTTPS upgrade fallback URL</a> is non-null, clear
+  <a for=request>is HTTPS upgrade</a> and <a for=request>HTTPS upgrade fallback URL</a> and return.
+
+  <p class=note>This is a fallback request that shouldn't be upgraded again.
+
+ <li><p>Otherwise, set the following fields:

The wrapping here is wrong. `<li>` has multiple children so `<p>` needs to be on its own line.

> @@ -2133,6 +2133,18 @@ Unless stated otherwise, it is false.
 
 <p class=note>This flag is for exclusive use by HTML's render-blocking mechanism. [[!HTML]]
 
+<p>A <a for=/>request</a> has an associated boolean <dfn export for=request>is HTTPS upgrade</dfn>.
+Unless stated otherwise, it is false.
+
+<p class=note>This flag is for exclusive use by HTTPS Upgrading algorithm.
+
+<p>A <a for=/>request</a> has an associated <dfn export for=request>HTTPS upgrade fallback URL</dfn>

```suggestion
<p>A <a for=/>request</a> has an associated <dfn export for=request>HTTPS upgrade fallback URL</dfn>,
```

> +   <a for="request">URL</a>.
+
+   <li><p>Set <var>request</var>'s <a for="request">URL</a>'s <a for="url">scheme</a> to
+   "<code>https</code>".
+
+   <li><p>Set <a for=request>is HTTPS upgrade</a> to true.
+  </ul>
+ </li>
+</ol>
+</div>
+
+<div algorithm>
+<h4 id=https-upgrades-fallback>Fallback algorithm</h4>
+
+<p>To run <dfn>HTTPS upgrade fallback</dfn> given a <a for=/>request</a> <var>request</var> and
+<a for=/>response</a> <var>response</var>, run these steps:

```suggestion
<a for=/>response</a> <var>response</var>:
```

> +<p>To <dfn>upgrade an HTTP request</dfn> given a <a for=/>request</a> <var>request</var>, run these
+steps:

```suggestion
<p>To <dfn>upgrade an HTTP request</dfn> given a <a for=/>request</a> <var>request</var>:
```

> @@ -4422,6 +4538,14 @@ steps:
 
  <li><p><a>Upgrade <var>request</var> to a potentially trustworthy URL, if appropriate</a>.
 
+ <li>
+  <p>Optionally, run <a>upgrade an HTTP request</a> algorithm on <var>request</var>, if appropriate</a>.

Spurious trailing `</a>`.

> @@ -2133,6 +2133,18 @@ Unless stated otherwise, it is false.
 
 <p class=note>This flag is for exclusive use by HTML's render-blocking mechanism. [[!HTML]]
 
+<p>A <a for=/>request</a> has an associated boolean <dfn export for=request>is HTTPS upgrade</dfn>.
+Unless stated otherwise, it is false.
+
+<p class=note>This flag is for exclusive use by HTTPS Upgrading algorithm.
+
+<p>A <a for=/>request</a> has an associated <dfn export for=request>HTTPS upgrade fallback URL</dfn>
+which is a <a for=/>URL</a>. It is a copy of the <a for=/>request</a>'s original
+<a for=request>URL</a> before <a for=/>request</a> is <a href="#https-upgrades-upgrade">optionally
+upgraded to HTTPS</a>. Unless stated otherwise, it is unset.

This is weird. I think it should be null or a URL and by default it's null all as the algorithm below seems to explicitly initialize it.

> @@ -5232,6 +5348,10 @@ these steps:
   <a>CORS check</a>, as <var>request</var>'s <a for=request>client</a> and the service worker can
   have different embedder policies.
 
+ <li>If <var>request</var>'s <code>https-upgrade</code> flag is set, set <var>response</var> and
+  <var>internalResponse</var> to the result of running
+  <a>HTTPS upgrade fallback</a> given <var>request</var> and <var>response</var>.

It doesn't look like this is done?

> @@ -4422,6 +4538,14 @@ steps:
 
  <li><p><a>Upgrade <var>request</var> to a potentially trustworthy URL, if appropriate</a>.
 
+ <li>
+  <p>Optionally, run <a>upgrade an HTTP request</a> algorithm on <var>request</var>, if appropriate</a>.

Also, "if appropriate" is rather unclear. Can't we just run this unconditionally and leave all the optionally and other implementation-defined determinations to "upgrade an HTTP request"?

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

Message ID: <whatwg/fetch/pull/1655/review/1637335691@github.com>