processResponseDone should receive a response #1202

noamr · 2021-03-25T16:30:40Z

Only call processResponseDone for responses that have their
timing info set, which are responses that have passed CORS
and redirects.

Closes #1201

Preview | Diff

noamr · 2021-03-25T18:47:57Z

@annevk

annevk

I think this is problematic as this would distinguish a CORS network error from other network errors. That they are indistinguishable is a desirable security property.

Can we still remove this from implementations?

I checked other network errors, such as a bad port, and we don't seem to be creating RT entries there, but I haven't done exhaustive testing for all possible network errors.

I'm also a bit concerned that somewhere we might do if "response is a network error, return a network error" which would drop this kind of information. I don't think we've audited that as there hasn't been a reason for it thus far.

cc @yoavweiss @npm1 @rniwa @bdekoz @sefeng211

annevk · 2021-04-13T13:13:07Z

fetch.bs

@@ -4019,7 +4027,7 @@ steps:

 <li><p>Set <var>response</var>'s <a for="response">timing info</a> to <var>timingInfo</var>.

- <li><p><a href="https://github.com/w3c/resource-timing/pull/261">Mark resource timing</a> for
+ <li><p><span>Mark resource timing</span> for


You want to use <a> here.

annevk · 2021-04-13T13:15:07Z

fetch.bs

+     <a for=/>fetch timing info</a> with its <a for="fetch timing info">start time</a> and its
+     <a for="fetch timing info">post-redirect start time</a> set to <var>fetchParams</var>'s
+     <a for="fetch params">timing info</a>'s <a for="fetch timing info">start time</a>, and
+     its <a for="fetch timing info">end time</a> set to the <a for=/>coarsened shared current


No newlines inside <a> et al.

annevk · 2021-04-13T13:15:43Z

fetch.bs

-    <a>CORS check</a> for <var>request</var> and <var>response</var> returns failure, then return a
-    <a>network error</a>.
+    <a>CORS check</a> for <var>request</var> and <var>response</var> returns failure, then:
+    <ul>


This should be an ordered list.

noamr · 2021-04-13T15:59:21Z

I think this is problematic as this would distinguish a CORS network error from other network errors. That they are indistinguishable is a desirable security property.

All network errors that come after the CORS check should go through fetch finale and receive the RT entry, which only has a start/end time.
It's true that only CORS-or-after errors would receive an RT entry. Is that still too exposing?
I can remove this bit for now so that we can proceed with the other issues.

noamr · 2021-04-13T17:19:43Z

Added #1215 to address the CORS/network error issue
This PR handles other issues.

npm1 · 2021-04-13T21:16:25Z

I think this is problematic as this would distinguish a CORS network error from other network errors. That they are indistinguishable is a desirable security property.

We expose errors in RT to prevent an attacker from distinguishing HTTP status codes (success vs errors). Would something similar apply here for CORS or not really (distinguishing success vs failed)?

annevk · 2021-04-14T07:11:15Z

I see where the confusion lies. A HTTP status code of 4xx, 5xx, or even 9xx is not a network error. That's a normal response as far as fetch is concerned. A network error is something more fundamental or the result of a security policy, e.g.:

DNS error
TLS error
Broken H/2 framing
CORS error
CSP block
Mixed Content block

Whether a response with a non-2xx HTTP status code results in an "error" depends on the endpoint. For <object> and <script> it does, for <img> it does not. (I.e., unfortunately 2xx vs non-2xx is exposed to some extent.)

I think in principle we could do one of these:

expose timing information for all of these, including network errors (using TAO as a policy and treating network errors as not having TAO).
expose timing information for responses, but not network errors
expose timing information for responses that result in a "load" event on their endpoint

But what we cannot do (in my opinion) is distinguish between network errors.

annevk · 2021-04-14T09:33:06Z

fetch.bs

- <var>timingInfo</var>, <var>originalURL</var>, <var>initiatorType</var>, and <var>global</var>.
- <!-- TODO -->
+ <li><p>
+ <a href="https://w3c.github.io/resource-timing/#dfn-mark-resource-timing">Mark resource timing</a>


Why is this needed, is it not exported? Let's export it first.

Sure, waiting for Resource-Timing to be published to TR (bikeshed vs. Respec stuff).

Removing this bit for now, will update once the refs are resolvable.

annevk · 2021-04-14T09:34:53Z

fetch.bs

+
+ <li><p>Set <var>timingInfo</var>'s <a for="fetch timing info">end time</a> to
+ the <a for=/>coarsened shared current time</a> given <var>fetchParams</var>'s
+ <a for="fetch params">cross-origin isolated capability</a>.


Why do we need to set end time twice?

We don't actually. Removing.

fetch.bs

annevk · 2021-04-14T09:36:31Z

fetch.bs

- <!-- TODO -->
+ <li><p>
+ <a href="https://w3c.github.io/resource-timing/#dfn-mark-resource-timing">Mark resource timing</a>
+ for <var>timingInfo</var>, <var>originalURL</var>, <var>initiatorType</var>, <var>global</var> and


Oxford comma.

annevk · 2021-04-14T09:38:17Z

fetch.bs

- <a for="fetch timing info">start time</a> and
- <a for="fetch timing info">post-redirect start time</a> are <var>timingInfo</var>'s
- <a for="fetch timing info">start time</a>.
+ <li><p>If <var>response</var>'s <a for=response>timing allow passed flag</a> is not set, then


As this <li> contains multiple flow elements the <p> needs to indented on a newline.

annevk · 2021-04-14T09:39:59Z

(I'm still okay with landing this modulo the above comments and leaving the error question to #1215.)

noamr · 2021-04-15T08:03:50Z

I see where the confusion lies. A HTTP status code of 4xx, 5xx, or even 9xx is not a network error. That's a normal response as far as fetch is concerned. A network error is something more fundamental or the result of a security policy, e.g.:

DNS error

TLS error

Broken H/2 framing

CORS error

CSP block

Mixed Content block

Whether a response with a non-2xx HTTP status code results in an "error" depends on the endpoint. For <object> and <script> it does, for <img> it does not. (I.e., unfortunately 2xx vs non-2xx is exposed to some extent.)

I think in principle we could do one of these:

expose timing information for all of these, including network errors (using TAO as a policy and treating network errors as not having TAO).

expose timing information for responses, but not network errors

expose timing information for responses that result in a "load" event on their endpoint

But what we cannot do (in my opinion) is distinguish between network errors.

Let's continue the discussion in #1215.

annevk · 2021-04-15T10:01:26Z

fetch.bs

 <li><p>Set <var>fetchParams</var>'s <a for="fetch params">request</a>'s
 <a for=request>done flag</a>.

+ <li><p>If <var>timingInfo</var> is null, then return.


Why do we have this here and also below in finalize and report timing? It seems this doesn't need to be here and you should get process response done either way?

fetch.bs

annevk · 2021-04-15T10:12:20Z

fetch.bs

+  <p>If <var>response</var>'s <a for=response>timing allow passed flag</a> is not set, then
+  perform the following steps:
+  <ol>
+    <li><p>Set <var>timingInfo</var> to a new <a for=/>fetch timing info</a> whose


Single-space indentation.

annevk · 2021-04-15T10:12:41Z

fetch.bs

@@ -4017,9 +4029,10 @@ steps:

 <li><p>Set <var>response</var>'s <a for="response">timing info</a> to <var>timingInfo</var>.

- <li><p><a href="https://github.com/w3c/resource-timing/pull/261">Mark resource timing</a> for
- <var>timingInfo</var>, <var>originalURL</var>, <var>initiatorType</var>, and <var>global</var>.
- <!-- TODO -->


Let's restore the TODO.

fetch.bs

annevk · 2021-04-20T13:54:29Z

fetch.bs

@@ -1932,10 +1932,6 @@ message as HTTP/2 does not support them.
 <dfn export for=response id=concept-response-cache-state>cache state</dfn> (the empty string or
 "<code>local</code>"). Unlesss stated otherwise, it is the empty string.

-<p class=note>This is intended solely for usage by service workers. [[SW]]
-<!-- If we ever expand the utility of this we need to carefully consider whether filtered responses
-     need to mask it, whether the cache API needs to store it, etc. -->


I think we should keep this including the comment and instead say something like

This is intended for usage by <cite>Service Workers</cite> and <cite>Resource Timing</cite>.

I guess for Resource Timing we also need opaque responses (that have TAO) to leak this information. I'll have a look if that needs more work.

Only call processResponseDone for responses that have their timing info set, which are responses that have passed CORS and redirects. Closes whatwg#1201

Reset all metrics except for start/end time to zero.

annevk · 2021-04-21T10:59:50Z

@noamr I think it would be good if there was a corresponding PR to Resource Timing the commit could link to for it acknowledging the new cacheState argument.

noamr · 2021-04-21T11:43:10Z

@noamr I think it would be good if there was a corresponding PR to Resource Timing the commit could link to for it acknowledging the new cacheState argument.

It’s already there in the editors draft.

annevk · 2021-04-21T12:09:58Z

I found the link above, it's w3c/resource-timing#266.

noamr mentioned this pull request Mar 28, 2021

New Resource Timing issues #1201

Closed

noamr force-pushed the issue-1201 branch from 947d487 to 18a423d Compare April 7, 2021 15:31

annevk reviewed Apr 13, 2021

View reviewed changes

annevk added security/privacy There are security or privacy implications topic: cors labels Apr 13, 2021

annevk mentioned this pull request Apr 13, 2021

Resource Timing integration whatwg/xhr#319

Merged

noamr force-pushed the issue-1201 branch from 18a423d to b2f4a1c Compare April 13, 2021 16:08

This was referenced Apr 13, 2021

Expose transfer size for resource timing #1203

Closed

Clarify which network errors create a resource timing entry #1215

Closed

annevk reviewed Apr 14, 2021

View reviewed changes

noamr mentioned this pull request Apr 14, 2021

Expose transferSize, encodedBodySize, decodedBodySize w3c/resource-timing#266

Merged

annevk reviewed Apr 15, 2021

View reviewed changes

annevk reviewed Apr 20, 2021

View reviewed changes

noamr added 10 commits April 20, 2021 17:34

processResponseDone should receive a response

cdd8cae

Only call processResponseDone for responses that have their timing info set, which are responses that have passed CORS and redirects. Closes whatwg#1201

Nits

8088493

Assign fetch timing to response also for CORS failure

8e6950b

Reset all metrics except for start/end time to zero.

Use real export from resource timing

a7399bf

Remove CORS part

d128656

Send cache-state to resource timing

9f994a8

Use ref for now

7a1c008

Fix PR nits

04d3a9b

More PR nits

638862d

Undo unwanted responseObject changes

ab08fc0

noamr added 4 commits April 20, 2021 17:34

More nits

9b79ae7

Amend

2f668b6

nits

cef771d

Update note

e61e0a8

noamr force-pushed the issue-1201 branch from 542310b to e61e0a8 Compare April 20, 2021 14:48

noamr and others added 3 commits April 21, 2021 13:34

Use real ref for mark

51dd0e3

Fix line length

764e995

nits

b455c91

annevk mentioned this pull request Apr 21, 2021

Clean up cross references/versioning w3c/resource-timing#271

Open

annevk merged commit 5fac9e8 into whatwg:main Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

processResponseDone should receive a response #1202

processResponseDone should receive a response #1202

noamr commented Mar 25, 2021 •

edited by pr-preview bot

Loading

noamr commented Mar 25, 2021

annevk left a comment

annevk Apr 13, 2021

annevk Apr 13, 2021

annevk Apr 13, 2021

noamr commented Apr 13, 2021

noamr commented Apr 13, 2021

npm1 commented Apr 13, 2021

annevk commented Apr 14, 2021

annevk Apr 14, 2021

noamr Apr 14, 2021

noamr Apr 14, 2021

annevk Apr 14, 2021

noamr Apr 14, 2021

annevk Apr 14, 2021

noamr Apr 14, 2021

annevk Apr 14, 2021

noamr Apr 14, 2021

annevk commented Apr 14, 2021

noamr commented Apr 15, 2021

annevk Apr 15, 2021

noamr Apr 15, 2021

annevk Apr 15, 2021

annevk Apr 15, 2021

noamr Apr 20, 2021

annevk Apr 20, 2021

noamr Apr 20, 2021

annevk commented Apr 21, 2021

noamr commented Apr 21, 2021

annevk commented Apr 21, 2021

processResponseDone should receive a response #1202

processResponseDone should receive a response #1202

Conversation

noamr commented Mar 25, 2021 • edited by pr-preview bot Loading

noamr commented Mar 25, 2021

annevk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noamr commented Apr 13, 2021

noamr commented Apr 13, 2021

npm1 commented Apr 13, 2021

annevk commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Apr 14, 2021

noamr commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Apr 21, 2021

noamr commented Apr 21, 2021

annevk commented Apr 21, 2021

noamr commented Mar 25, 2021 •

edited by pr-preview bot

Loading