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

@yoavweiss commented on this pull request.



> +  <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>Set <var>request</var>'s URL's scheme to <code>"https"</code>
+      <li>Set <code>https-upgrade</code> flag to true
+    </ul>
+  </li>
+</ol>
+
+<h4 id=https-upgrades-fallback>Fallback algorithm</h4>
+
+To <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:
+
+<ol>
+  <li><p>If <var>request</var>'s <code>https-upgrade</code> flag is not set, the <var>request</var>

Similar to the above comment, you want to move the explanation part ("request waas not upgraded", "the upgrade failed", etc) to notes, rather than have it be part of the algorithm

> +</ol>
+
+<h4 id=https-upgrades-fallback>Fallback algorithm</h4>
+
+To <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:
+
+<ol>
+  <li><p>If <var>request</var>'s <code>https-upgrade</code> flag is not set, the <var>request</var>
+  was not upgraded. Return <var>response</var>.
+
+  <li>
+    <p>Otherwise, the request was upgraded.
+
+    <p>If <var>response</var> is a network error, the upgrade failed. Initiate a fallback
+    load by creating a synthetic redirect response <var>fallbackResponse</var> that redirects to

I think you want to be more prescriptive here. E.g. "return a Response object..." with specific creation criteria that would generate the synthetic response you're after.

> @@ -5158,7 +5262,8 @@ these steps:
        <var>response</var>'s <a for=response>URL list</a> has more than one item.
       </ul>
 
-      <p>then return a <a>network error</a>.
+      <p>then return the result of running <a>HTTPS upgrade fallback</a> given <var>request</var>

Should we only fallback in the "error" case above? (for the rest it doesn't seem like a fallback would help)

> @@ -5225,13 +5330,17 @@ these steps:
   <a>cross-origin resource policy check</a> with <var>request</var>'s <a for=request>origin</a>,
   <var>request</var>'s <a for=request>client</a>, <var>request</var>'s
   <a for=request>destination</a>, and <var>internalResponse</var> returns <b>blocked</b>, then
-  return a <a>network error</a>.
+  return the result of running <a>HTTPS upgrade fallback</a> given <var>request</var>

Should we fallback on this CORS error?

>  
   <p class=note>The <a>cross-origin resource policy check</a> runs for responses coming from the
   network and responses coming from the service worker. This is different from the
   <a>CORS check</a>, as <var>request</var>'s <a for=request>client</a> and the service worker can
   have different embedder policies.
 
+ <li>Set <var>response</var> and <var>internalResponse</var> to the result of running

This is a bit confusing. It might be better to externalize the condition from the fallback algorithm or rename it to clarify that it doesn't fallback unconditionally.

> @@ -4405,6 +4523,8 @@ steps:
 
  <li><p><a>Upgrade <var>request</var> to a potentially trustworthy URL, if appropriate</a>.
 
+ <li><p><a href="#https-upgrades-upgrade">Optionally, run HTTPS upgrading algorithm on <var>request</var>, if appropriate</a>.

It's still not clear to me why we need to upgrade in "main fetch", given that any "http" scheme request would go through HTTP Fetch. Can you clarify?

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

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