Re: [whatwg/fetch] Define Cross-Origin-Resource-Policy response header (#733)

mikewest commented on this pull request.

Thanks! I have a few quick thoughts, only one real issue.

> @@ -2437,6 +2437,64 @@ run these steps:</p>
 </ol>
 
 
+<h3 id=cross-origin-resource-policy-header>`<code>Cross-Origin-Resource-Policy</code>` header</h3>
+
+<p>The
+`<dfn export http-header id=http-cross-origin-resource-policy><code>Cross-Origin-Resource-Policy</code></dfn>`
+response <a for=/>header</a> can be used to require checking a <a for=/>request</a>'s
+<a for=request>current url</a>'s <a for=url>origin</a> against a <a for=/>request</a>'s
+<a for=request>origin</a> when <a for=/>request</a>'s <a for=request>mode</a> is
+"<code>no-cors</code>".
+
+<p>Its <a for=header>value</a> <a>ABNF</a>:
+
+<pre>
+Cross-Origin-Resource-Policy     = %x73.61.6D.65 / %x73.61.6D.65.2D.73.69.74.65 ; "same" / "same-site"; case-sensitive</pre>

Nit: The last semicolon should be a comma (similar to the `origin-or-null` definition).

> +<pre>
+Cross-Origin-Resource-Policy     = %x73.61.6D.65 / %x73.61.6D.65.2D.73.69.74.65 ; "same" / "same-site"; case-sensitive</pre>
+
+<p>To perform a <dfn>cross-origin resource policy check</dfn>, given a <var>request</var> and
+<var>response</var>, run these steps:</p>
+
+<ol>
+ <li><p>If <var>request</var>'s <a for=request>mode</a> is not "<code>no-cors</code>", then return
+ <b>allowed</b>.
+
+ <li>
+  <p>If <var>request</var>'s <a for=request>origin</a> is <a>same origin</a> with
+  <var>request</var>'s <a for=request>current url</a>'s <a for=url>origin</a>, then return
+  <b>allowed</b>.
+
+  <p class=note>A cross-origin response redirecting to a same or same-site resource with the

Nit: This is a little unclear. Is the redirect target same-site with the request's origin, or with the cross-origin response?

> + `<a http-header><code>Cross-Origin-Resource-Policy</code></a>` and <var>response</var>'s
+ <a for=response>header list</a>.
+
+ <li><p>If <var>policy</var> is `<code>same</code>`, then return <b>blocked</b>.
+
+ <li>
+  <p>If <var>policy</var> is `<code>same-site</code>` and neither of the following is true
+
+  <ul class=brief>
+   <li><p><var>request</var>'s <a for=request>origin</a>'s <a for=origin>host</a>
+   <a>is a registrable domain suffix of or is equal to</a> <var>request</var>'s
+   <a for=request>current url</a>'s <a for=url>host</a>
+
+   <li><p><var>request</var>'s <a for=request>current url</a>'s <a for=url>host</a>
+   <a>is a registrable domain suffix of or is equal to</a> <var>request</var>'s
+   <a for=request>origin</a>'s <a for=origin>host</a>

I think this isn't broad enough. The bidirectional `is a registrable domain suffix of or is equal to` comparison would return false for a request whose origin's host is `subdomain.subdomain.example.com` and whose current url is `notsubdomain.example.com`.

Instead, I think we need to calculate the relevant registrable domain (perhaps by pointing to the (not terribly clear) algorithm on https://publicsuffix.org/list/) for both the request's origin and the request's current URL, and compare those.

> + `<a http-header><code>Cross-Origin-Resource-Policy</code></a>` and <var>response</var>'s
+ <a for=response>header list</a>.
+
+ <li><p>If <var>policy</var> is `<code>same</code>`, then return <b>blocked</b>.
+
+ <li>
+  <p>If <var>policy</var> is `<code>same-site</code>` and neither of the following is true
+
+  <ul class=brief>
+   <li><p><var>request</var>'s <a for=request>origin</a>'s <a for=origin>host</a>
+   <a>is a registrable domain suffix of or is equal to</a> <var>request</var>'s
+   <a for=request>current url</a>'s <a for=url>host</a>
+
+   <li><p><var>request</var>'s <a for=request>current url</a>'s <a for=url>host</a>
+   <a>is a registrable domain suffix of or is equal to</a> <var>request</var>'s
+   <a for=request>origin</a>'s <a for=origin>host</a>

(And, actually, looking at that algorithm again, I'm not sure it does what it needs to do for `document.domain` either... It doesn't calculate the registrable domain at all, but the public suffix. I'll file a separate bug.)

> + `<a http-header><code>Cross-Origin-Resource-Policy</code></a>` and <var>response</var>'s
+ <a for=response>header list</a>.
+
+ <li><p>If <var>policy</var> is `<code>same</code>`, then return <b>blocked</b>.
+
+ <li>
+  <p>If <var>policy</var> is `<code>same-site</code>` and neither of the following is true
+
+  <ul class=brief>
+   <li><p><var>request</var>'s <a for=request>origin</a>'s <a for=origin>host</a>
+   <a>is a registrable domain suffix of or is equal to</a> <var>request</var>'s
+   <a for=request>current url</a>'s <a for=url>host</a>
+
+   <li><p><var>request</var>'s <a for=request>current url</a>'s <a for=url>host</a>
+   <a>is a registrable domain suffix of or is equal to</a> <var>request</var>'s
+   <a for=request>origin</a>'s <a for=origin>host</a>

https://github.com/whatwg/html/issues/3711

> +
+ <li>
+  <p>If <var>request</var>'s <a for=request>origin</a> is <a>same origin</a> with
+  <var>request</var>'s <a for=request>current url</a>'s <a for=url>origin</a>, then return
+  <b>allowed</b>.
+
+  <p class=note>A cross-origin response redirecting to a same or same-site resource with the
+  `<a http-header><code>Cross-Origin-Resource-Policy</code></a>` header specified does not affect
+  anything.
+  <!-- We could make this have an effect if we fix https://github.com/whatwg/fetch/pull/594 first,
+       but even then we normally do not let this have any effect for "no-cors" so it would be
+       somewhat inconsistent if it did here, but might still be better... -->
+
+ <li><p>Let <var>policy</var> be the <a>combined value</a> with
+ `<a http-header><code>Cross-Origin-Resource-Policy</code></a>` and <var>response</var>'s
+ <a for=response>header list</a>.

It might be worth adding a note here about the meaning of `same, same` or `same, same-site`, as I believe this means that both will be ignored.

-- 
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/733#pullrequestreview-122937325