Align CH processing model with implementation and HTML#3774 by yoavweiss · Pull Request #773 · whatwg/fetch

yoavweiss · 2018-06-26T07:10:45Z

This PR tackles a part of #726, by copying the request's client-hints list from the environment settings object's one (as defined in whatwg/html#3774). It also aligns the addition of CH headers to current implementations, and adds the Device-Memory, RTT, Downlink and ECT hints.

Preview | Diff

annevk · 2018-08-13T13:13:08Z

fetch.bs

-<dfn export for=request id=concept-request-client-hints-list>client hints list</dfn>,
-which is a <a lt="client hints list" for=client>client-hints list</a>. Unless stated
-otherwise, it is the empty list.
+<dfn export for=request id=concept-request-client-hints-set>client hints set</dfn>,


Let's preserve the original ID. I don't think it matters much that it doesn't match the text anymore.

annevk · 2018-08-13T13:13:52Z

fetch.bs

-which is a <a lt="client hints list" for=client>client-hints list</a>. Unless stated
-otherwise, it is the empty list.
+<dfn export for=request id=concept-request-client-hints-set>client hints set</dfn>,
+which is a <a lt="client hints set" for=client>client-hints set</a>. Unless stated


A primitive cannot belong to something else. I guess this was originally wrong as well.

When is the hyphen to be used by the way? I'd rather consistently use or not use it.

Removed "for=client".

Agreed regarding consistency. Added hyphen everywhere.

annevk · 2018-08-13T13:14:43Z

fetch.bs



-<h3 id=client-hints-list>Client hints list</h3>
+<h3 id=client-hints-set>Client hints set</h3>


ID preservation please.

annevk · 2018-08-13T13:15:11Z

fetch.bs


-<p>A <dfn export id=concept-client-hints-list for=client>client hints list</dfn> is a
-<a for=/>list</a> of
+<p>A <dfn export id=concept-client-hints-set for=client>client hints set</dfn> is a


I guess we should remove for=client here since it doesn't actually belong to a client, right?

annevk · 2018-08-13T13:15:53Z

fetch.bs

   <a for=request>origin</a> to  <var>request</var>'s
   <a for=request>client</a>'s <a for="environment settings object">origin</a>.

+   <li><p>Set <var>request</var>'s <a for=request>client-hints set</a> to be a copy of the <a


client-hints set won't link due to the hyphen. Also, instead of copy you want https://infra.spec.whatwg.org/#list-clone.

Added a direct link to the definition, but not sure that's the best way to do that. Let me know if there's a better way

annevk · 2018-08-13T13:16:34Z

fetch.bs

-    <p>If <var>request</var> is a <a>subresource request</a>, then:
+    <p>If <var>request</var>'s <a for=request>current url</a>'s
+    <a for=url>origin</a> is <a>same origin</a> with <var>request</var>'s
+    <a for=request>origin</a>, then:


This means that a cross-origin URL inbetween doesn't influence this. I'm not sure that's what we want? Also, can we not obtain this state from something else that has already done the check? Lots of independent same-origin checks seem like a code smell.

This means that a cross-origin URL inbetween doesn't influence this

Do you mean a cross-origin redirection between two same origin requests? or something else?

annevk · 2018-08-13T13:17:28Z

fetch.bs


      <ol>
-       <li>
+        <li>


One space indentation was correct.

annevk · 2018-08-13T13:17:43Z

fetch.bs

+
      <p><a for=list>For each</a> <var>hintName</var> of <var>request</var>'s
-      <a for=client>client hints list</a>:
+      <a for=client>client-hints set</a>:


Hyphen again.

annevk · 2018-08-13T13:18:04Z

fetch.bs

    </ol>
-  </ol>
+
+    <li><p>If <var>request</var> is a <a>subresource request</a>, then:


The indentation is off here as well.

annevk · 2018-08-21T08:05:05Z

@yoavweiss same-origin request -> redirect to cross-origin URL -> redirect to same-origin URL.

annevk · 2018-08-21T08:06:31Z

fetch.bs

   <a for=request>client</a>'s <a for="environment settings object">origin</a>.

+   <li><p>Set <var>request</var>'s <a for=request>client-hints set</a> to be a <a
+   href="https://infra.spec.whatwg.org/#list-clone">clone</a> of the <a


You can use <a for=list>clone</a>.

Also, no newlines inside tags or inline elements (slightly different rules from HTML, sorry).

annevk

A comment message detailing the changes made here would help. It also seems this adds more Client-Hints headers that are not CORS-safelisted. Is that intentional?

annevk · 2018-08-21T10:05:06Z

fetch.bs

+<h3 id=client-hints-list>Client-hints set</h3>

-<p class=note>This section will be integrated into HTTP Client Hints.
+<p class=note>This section will be integrated into HTTP Client-Hints.


Is that still the plan? In any event, this section should have a <p class=XXX> explaining that currently the integration is not well defined.

I'm not sure what further integration is needed, tbh. I'll change to "XXX"

annevk · 2018-08-21T10:06:12Z

fetch.bs

+    <li>
+
+     <p class=note>The following step should be applicable only for same-origin requests. See
+     <a href="https://github.com/whatwg/fetch/issues/800">issue</a> for more details.</p>


This should be class=XXX and should be placed at the end of the step per convention.

yoavweiss · 2018-08-21T10:30:15Z

It also seems this adds more Client-Hints headers that are not CORS-safelisted. Is that intentional?

those are added to the list at #725

yoavweiss · 2018-08-21T10:30:59Z

A comment message detailing the changes made here would help

I'll squash and add a meaningful commit message

annevk · 2018-08-21T11:25:21Z

fetch.bs

+<h3 id=client-hints-list>Client-hints set</h3>

-<p class=note>This section will be integrated into HTTP Client Hints.
+<p class=XXX>This section will be integrated into HTTP Client-Hints.


Sorry for being unclear, this issue marker should indicate that Client Hints integration with Fetch is far from finished, similar to the issue marker you added below. I was thinking it would be good to stress that in multiple places.

Changed to point at #726
Let me know if that works

annevk · 2018-08-21T11:27:38Z

The commit message needs to explain the removal of DPR and Viewport-Width from the first table as well.

yoavweiss · 2018-08-21T13:10:35Z

The commit message needs to explain the removal of DPR and Viewport-Width from the first table as well.

added

annevk · 2018-08-23T10:56:03Z

Having considered this further I'm not sure how to accept this as it has the same problems as #725:

It's unclear what the level of implementer support is.
There are no tests.
Adequate tests cannot be written as the infrastructure is still broken (per Integrate latest Client Hints updates #726 and CH processing, cross-origin redirects and service workers #800).

yoavweiss · 2018-08-23T11:47:15Z

It's unclear what the level of implementer support is.

This is currently implemented by Chromium, but not elsewhere. What's the typical process for integrating such features before they gain wider adoption?

There are no tests.

That is not true. See https://wpt.fyi/results/client-hints?label=experimental

Adequate tests cannot be written as the infrastructure is still broken (per #726 and #800).

Are you suggesting that we need the feature policy (w3c/webappsec-permissions-policy#129) and redirection logic (#800) pieces in place before any of this can land?

annevk · 2018-08-23T11:54:31Z

@yoavweiss typically we don't merge until there's at least two implementers committed to some extent (no need for two implementations).

As for tests, I meant tests covering the problematic bits.

And since the redirection logic is vital to how this ends up being defined, yes. It would be less problematic if that were a simple change, but basically all the existing infrastructure is in the wrong place for it, and we might need new primitives as well depending on how we deal with UA-set vs developer-set headers.

annevk · 2018-08-23T11:55:01Z

https://whatwg.org/working-mode#changes has a more formal description of WHATWG's changes policy.

yoavweiss · 2018-08-23T12:06:13Z

And since the redirection logic is vital to how this ends up being defined, yes. It would be less problematic if that were a simple change, but basically all the existing infrastructure is in the wrong place for it, and we might need new primitives as well depending on how we deal with UA-set vs developer-set headers.

I agree that the redirection logic is critical once we start doing same-origin checks. This PR (as well as current Chromium implementation) does not do that. And indeed, things may move once that logic is settled and lands.

At the same time, I thought it'd be worthwhile to define the current behavior and then iterate on that. (as the current spec definition doesn't match the current implementation)

fetch.bs

eeeps · 2019-01-24T15:52:29Z

fetch.bs

+<a for=/>set</a> of
 <a href=http://httpwg.org/http-extensions/client-hints.html#accept-ch>Client hint tokens</a>, each
 of which is one of `<code>DPR</code>`, `<code>Save-Data</code>`, `<code>Viewport-Width</code>`, or
 `<code>Width</code>`.


Add the other new hints here?

fetch.bs

This change aligns the processing of Client-Hints with the shipped Chromium implementation, by changing the following: * Clone the environment settings object's client-hints set to the request's client-hints set. * Apply CH processing to all requests, rather than only subresource requests. * Add processing of new CH headers, added in whatwg#725 * Remove `DPR` and `Viewport-Width` from headers that are sent by default for navigation requests. It also renames "client-hints list" to "client-hints set", and changes it to be a set, to match related HTML spec changes.

from whatwg#773

yoavweiss · 2020-01-07T10:35:29Z

I've moved most of the processing defined in this PR to https://yoavweiss.github.io/client-hints-infrastructure/
Regarding the rest, it's PRed in #971

I'm therefore closing this PR.

yoavweiss requested a review from annevk June 26, 2018 07:10

igrigorik mentioned this pull request Jun 26, 2018

Integrate latest Client Hints updates #726

Closed

10 tasks

annevk reviewed Aug 13, 2018

View reviewed changes

annevk reviewed Aug 21, 2018

View reviewed changes

yoavweiss mentioned this pull request Aug 21, 2018

CH processing, cross-origin redirects and service workers #800

Closed

annevk reviewed Aug 21, 2018

View reviewed changes

yoavweiss force-pushed the ch_processing branch from dd44296 to ee93d96 Compare August 21, 2018 10:44

annevk reviewed Aug 21, 2018

View reviewed changes

yoavweiss force-pushed the ch_processing branch 2 times, most recently from df666da to bea7cde Compare August 21, 2018 12:47

annevk added the topic: client hints label Aug 23, 2018

This was referenced Sep 30, 2018

Accept-CH-Lifetime privacy concerns httpwg/http-extensions#372

Closed

Check Feature Policy before attaching Client Hints #811

Closed

eeeps reviewed Jan 17, 2019

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

yoavweiss force-pushed the ch_processing branch from 2f8047a to d73c83b Compare January 17, 2019 16:58

Ms2ger reviewed Jan 18, 2019

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

eeeps reviewed Jan 24, 2019

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

eeeps reviewed Jan 24, 2019

View reviewed changes

fetch.bs Outdated Show resolved Hide resolved

yoavweiss mentioned this pull request Feb 4, 2019

Client Hints mozilla/standards-positions#79

Closed

yoavweiss mentioned this pull request Feb 4, 2019

Does Client Hints have multi vendor support? #867

Closed

yoavweiss mentioned this pull request Feb 13, 2019

Client-Hints exposes fingerprint values to additional parties and logging sensitive locations httpwg/http-extensions#767

Closed

yoavweiss changed the title ~~Copy client-hints list from env settings object. Align CH processing~~ Align CH processing model with implementation and HTML#3774 Mar 15, 2019

yoavweiss mentioned this pull request Mar 15, 2019

Scrolling iframed specs whatwg/whatwg.org#241

Closed

yoavweiss mentioned this pull request Mar 23, 2019

Remove Client Hints #884

Merged

yoavweiss and others added 12 commits March 26, 2019 10:13

Check Feature Policy before sending Client Hints

b533da4

Add Feature-Policy biblio reference

5c94e8b

Fix up PR from @eeeps according to IRC discussion

5b7f379

Remove the cross-origin condition

5a74cf7

Remove vars from algo name

3da867b

Trying to fix client-hints set linking

7b72f9a

Removed for from dfn

8a2fed1

Sec prefix, redirect 3P handling, response density

9b53f62

Add hints from Tarun Bansal's PR whatwg#725

7050b3e

Fix up sec processing

8b1d633

s/list/set/

5067615

yoavweiss force-pushed the ch_processing branch from 56c7c7b to 5067615 Compare March 26, 2019 09:24

eeeps added a commit to eeeps/fetch that referenced this pull request Nov 26, 2019

Copy/paste @yoav's text

6ecec04

from whatwg#773

yoavweiss closed this Jan 7, 2020

yoavweiss mentioned this pull request Aug 10, 2020

Client Hints infrastructure #1072

Closed



		<h3 id=client-hints-list>Client hints list</h3>
		<h3 id=client-hints-set>Client hints set</h3>

Conversation

yoavweiss commented Jun 26, 2018 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoavweiss commented Aug 21, 2018

Uh oh!

yoavweiss commented Aug 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

annevk commented Aug 21, 2018

Uh oh!

yoavweiss commented Aug 21, 2018

Uh oh!

annevk commented Aug 23, 2018

Uh oh!

yoavweiss commented Aug 23, 2018

Uh oh!

annevk commented Aug 23, 2018

Uh oh!

annevk commented Aug 23, 2018

yoavweiss commented Jun 26, 2018 •

edited by pr-preview bot

Loading