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

@mikewest commented on this pull request.

A few comments:

> @@ -3225,7 +3238,112 @@ through TLS using ALPN. The protocol cannot be spoofed through HTTP requests in
 </ol>
 </div>
 
+<h3 id=https-upgrades>HTTPS upgrading</h3>
+
+<div algorithm>
+
+<p>User agents may optionally upgrade requests with <a>potentially untrustworthy URL</a>s
+to attempt to fetch them over <a>potentially trustworthy URL</a>s. If an upgraded request
+fails with a network error, it is retried over the original URL.
+
+<p>The HTTPS upgrading algorithm consists of Upgrade and Fallback algorithms, with the following
+steps:
+
+<h4 id=https-upgrades-upgrade>Upgrade algorithm</h4>
+
+<p>Given a request <var>request</var>, this algorithm will rewrite its URL if the request should be
+modified and loaded over <code>"https"</code>, via the following algorithm:

https://fetch.spec.whatwg.org/#cross-origin-resource-policy-internal-check is a good example of the pattern Fetch often uses: `To <dfn>upgrade an HTTP request</dfn> given a [request] <var>request</var> ...`. That gives you a concrete dfn to link to in the algorithm below.

> +  fallback request that shouldn't be upgraded again. Clear the <code>https-upgrade</code> and
+  <code>https-upgrade-fallback-url</code> flags and return <var>request</var>.</li>
+
+  <li><p>Otherwise, upgrade the request by setting the following fields:
+    <ul>
+      <li>Set <code>https-upgrade-fallback-url</code> to request's URL</li>
+      <li>Set <var>request</var>'s URL's scheme to <code>"https"</code></li>
+      <li>Set <code>https-upgrade</code> flag to true</li>
+    </ul>
+  </li>
+</ol>
+
+<h4 id=https-upgrades-fallback>Fallback algorithm</h4>
+
+<p>Runs after the Upgrade algorithm. This algorithm determines whether the upgraded
+request completed without a network error. If not, it retries the request over <code>"http"</code>.

As above, you'll want to structure this as a `<dfn>` and specify the parameters you're passing in. It's a bit difficult to understand which objects you're talking about below without being more specific about things. :)

> +<ol>
+  <li><p>If the <code>https-upgrade</code> flag is not set, return without modifying request.</li>
+
+  <li><p>Otherwise, this is an upgraded request. Let <var>upgradedResponse</var> be the result of
+  fetching the upgraded request.</li>
+
+  <li>
+    <p>If <var>upgradedResponse</var> is a network error, initiate a fallback http load by doing the
+    following:
+
+    <ul>
+      <li><p>Set <var>request</var>'s URL to the value of <var>request</var>'s
+        <code>https-upgrade-fallback-url</code> flag</li>
+
+      <li><p>Set response to the result of running <a>HTTP-redirect fetch</a> given fetchParams and
+        <var>upgradedResponse</var>.</li>

Here, `upgradedResponse` is a network error. This means that "HTTP-redirect Fetch" is not going to have a location to work with and will return early in its [step 4](https://fetch.spec.whatwg.org/#concept-http-redirect-fetch:~:text=If%20locationURL%20is%20null%2C%20then%20return%20response.)

Rather than adjusting the `request`, I think you need to create a synthetic 307 `response` that can be handled by the redirect machinery as usual. That would also suggest that you should perform this work prior to the redirect handling in "HTTP Fetch" (e.g. perhaps just before [step 6](https://fetch.spec.whatwg.org/#cross-origin-resource-policy-header:~:text=If%20internalResponse%E2%80%99s%20status%20is%20a%20redirect%20status%3A)?

Looking at that algorithm again, you'd also need to handle the service worker bits that return a network error in step 3.5 and maybe the check in step 5.

>  
+<div id=example-https-upgrade-slow-https class=example>
+ <p>
+  Optional fast-fallback example: <code>a.com</code> serves <code>http://a.com</code> and loads very
+  slowly over <code>https://a.com</code>.
+  An eligible request to <code>http://a.com</code> will be upgraded to <code>https://a.com</code>.
+  If the upgraded request doesn't return a response for N seconds, the fetch will be canceled.

The N seconds timeout is not defined in this PR. Do you plan to add it?

> +to attempt to fetch them over <a>potentially trustworthy URL</a>s. If an upgraded request
+fails with a network error, it is retried over the original URL.
+
+<p>The HTTPS upgrading algorithm consists of Upgrade and Fallback algorithms, with the following
+steps:
+
+<h4 id=https-upgrades-upgrade>Upgrade algorithm</h4>
+
+<p>Given a request <var>request</var>, this algorithm will rewrite its URL if the request should be
+modified and loaded over <code>"https"</code>, via the following algorithm:
+
+<ol>
+  <li>
+    <p>If one or more of the following conditions are met, return without modifying request:
+    <ul>
+      <li><p><var>request</var> is not a navigation request whose destination is document</li>

I think you can simplify this to:

```suggestion
      <li><p><var>request</var>'s <a for="request">destination</a> is "<code>document</code>"</li>
```

> +
+<p>The HTTPS upgrading algorithm consists of Upgrade and Fallback algorithms, with the following
+steps:
+
+<h4 id=https-upgrades-upgrade>Upgrade algorithm</h4>
+
+<p>Given a request <var>request</var>, this algorithm will rewrite its URL if the request should be
+modified and loaded over <code>"https"</code>, via the following algorithm:
+
+<ol>
+  <li>
+    <p>If one or more of the following conditions are met, return without modifying request:
+    <ul>
+      <li><p><var>request</var> is not a navigation request whose destination is document</li>
+
+      <li><p><var>request</var>'s method is not `<code>GET</code>`</li>

```suggestion
      <li><p><var>request</var>'s <a for="request">method</a> is not "<code>GET</code>"</li>
```

> +steps:
+
+<h4 id=https-upgrades-upgrade>Upgrade algorithm</h4>
+
+<p>Given a request <var>request</var>, this algorithm will rewrite its URL if the request should be
+modified and loaded over <code>"https"</code>, via the following algorithm:
+
+<ol>
+  <li>
+    <p>If one or more of the following conditions are met, return without modifying request:
+    <ul>
+      <li><p><var>request</var> is not a navigation request whose destination is document</li>
+
+      <li><p><var>request</var>'s method is not `<code>GET</code>`</li>
+
+      <li><p><var>request</var>'s URL's scheme is not <code>"http"</code></li>

```suggestion
      <li><p><var>request</var>'s <a for="request">URL</a>'s <a for=url>scheme</a> is not "<code>http</code>"</li>
```

> +<p>Given a request <var>request</var>, this algorithm will rewrite its URL if the request should be
+modified and loaded over <code>"https"</code>, via the following algorithm:
+
+<ol>
+  <li>
+    <p>If one or more of the following conditions are met, return without modifying request:
+    <ul>
+      <li><p><var>request</var> is not a navigation request whose destination is document</li>
+
+      <li><p><var>request</var>'s method is not `<code>GET</code>`</li>
+
+      <li><p><var>request</var>'s URL's scheme is not <code>"http"</code></li>
+
+      <li><p><var>request</var>'s URL's host is non-unique (i.e., contains 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)</li>

"Uniqueness" of a URL isn't a concept that really exists yet. I think it would be best to introduce that concept in URL rather than Fetch, and then reference it from here. I also think you'll want to be a little more exhaustive with the definition. :) You can partially lean on @letitz's work with Private Network Access for some of the IP addresses, but you'll need to specify the TLD matching more robustly (e.g. `.localhost`? `.onion`? other reserved names from https://datatracker.ietf.org/doc/html/rfc6762#appendix-G?)

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

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