Reject uncachable status code #326

nyaxt · 2018-11-02T01:13:16Z

Chromium CL: https://chromium-review.googlesource.com/c/chromium/src/+/1313712

nyaxt · 2018-11-02T01:19:38Z

nyaxt · 2018-11-02T01:28:26Z

I think there is a reasonable set of status codes which we should allow, but not sure where the boundary is.

1xx Informational: IIUC, these are partial responses which doesn't make sense to be captured.
non-cachable responses: Disallowing those seem to align with the request method restrictions. (strawman)

@kinu made a point that redirect is disallowed in the loading spec, so there is also discussion of where to enforce the restriction

kinu · 2018-11-02T02:39:29Z

I'm less sure about the non-cacheable responses, for example for CaheStorage we don't check any of these but specifically throw an error if it's 206 (partial response). And if we take a deeper look getting 203 (non-authoritative) seems a little weird, handling of 206 (partial content) responses would need to be discussed in the loading spec (as well as handling of range requests), and all 3xx are forbidden in the loading spec. Overall I feel it might make better sense to have the part only in the loading spec, at least until we have a clearer thought of what should be valid.

kinu · 2018-11-02T09:01:40Z

Well or maybe we can just start with 200 only. Looking into other status code most of them don't really make sense. (I prefer forbidding it in the loading spec though) /cc @horo-t

mnot · 2018-11-03T06:52:40Z

A status code that is uncacheable by default can still be cached if it has explicit freshness information. I think you need to reference the entire response cacheability algorithm.

nyaxt · 2018-11-05T00:30:24Z

@kinu @horo-t @mnot Thank you for your input!
I'll update the PR shortly to only allow 200 here to be most restrictive, but I'm open to loosen the restriction to the response cacheablility algorithm if we have a use case.

nyaxt · 2018-11-05T00:41:43Z

@kinu Would you mind sharing the background why you'd prefer this to be loading spec?

I'm mixed since we already have :method cacheability validation in the sxg format spec. While I think having it outside format spec makes sense if we consider non-webbrowser consumers in the future (which are not covered by loading.md iiuc).

Another strawman would be to have:

format spec say only cachable responses are signature valid
loading spec to allow only 200

jyasskin · 2018-11-14T23:33:51Z

draft-yasskin-http-origin-signed-responses.md

@@ -838,6 +838,10 @@ signature returns "valid", return "valid". Otherwise, return "invalid".
 1. Let `exchange` be the exchange metadata and headers parsed out of `headers`.
 1. If `exchange`'s request method is not safe (Section 4.2.1 of {{!RFC7231}}) or
   not cacheable (Section 4.2.3 of {{!RFC7231}}), return "invalid".


Let's replace the "or not cacheable" bit with the reference to https://httpwg.org/specs/rfc7234.html#rfc.section.3, since that also talks about methods.

jyasskin · 2018-11-14T23:37:43Z

draft-yasskin-http-origin-signed-responses.md

@@ -838,6 +838,10 @@ signature returns "valid", return "valid". Otherwise, return "invalid".
 1. Let `exchange` be the exchange metadata and headers parsed out of `headers`.
 1. If `exchange`'s request method is not safe (Section 4.2.1 of {{!RFC7231}}) or
   not cacheable (Section 4.2.3 of {{!RFC7231}}), return "invalid".
+1. If `exchange`'s response is not complete, (Section 3.1 of {{!RFC7231}}),


Suggested change

1. If `exchange`'s response is not complete, (Section 3.1 of {{!RFC7231}}),

1. If `exchange`'s response is not complete, (Section 3.1 of {{!RFC7234}}),

jyasskin · 2018-11-14T23:41:21Z

draft-yasskin-http-origin-signed-responses.md

@@ -838,6 +838,10 @@ signature returns "valid", return "valid". Otherwise, return "invalid".
 1. Let `exchange` be the exchange metadata and headers parsed out of `headers`.
 1. If `exchange`'s request method is not safe (Section 4.2.1 of {{!RFC7231}}) or
   not cacheable (Section 4.2.3 of {{!RFC7231}}), return "invalid".
+1. If `exchange`'s response is not complete, (Section 3.1 of {{!RFC7231}}),
+   return "invalid".
+1. If `exchange`'s response is not cachable by a public cache (Section 3 of


I believe "shared cache" is the right term, according to https://httpwg.org/specs/rfc7234.html#rfc.section.1. "public" is a response directive, not a type of cache.

I'd also rather not use the term "cacheable" here since https://httpwg.org/specs/rfc7234.html#rfc.section.3 doesn't define it as a predicate on responses. Instead perhaps:

Suggested change

1. If `exchange`'s response is not cachable by a public cache (Section 3 of

1. If Section 3 of {{RFC7234}} forbids a shared cache from storing `exchange`'s response, return "invalid".

jyasskin · 2018-11-14T23:48:04Z

loading.bs

@@ -733,7 +733,7 @@ returns a failure or an [=exchange=] via the following steps:
    * |headers|[0] contains a `` `host` `` key.
    * |headers|[0][`` `:method` ``] is not `` `GET` ``.
    * |headers|[1] contains any keys starting with `` `:` `` that aren't `` `:status` ``.
-    * |headers|[1][`` `:status` ``] doesn't consist of exactly 3 digits.
+    * |headers|[1][`` `:status` ``] is not `` 200 ``.


I think I agree with having this tighter restriction in the loading spec only, at least for now.

nyaxt

Thank you for your review!

nyaxt · 2018-11-20T05:49:04Z

draft-yasskin-http-origin-signed-responses.md

@@ -838,6 +838,10 @@ signature returns "valid", return "valid". Otherwise, return "invalid".
 1. Let `exchange` be the exchange metadata and headers parsed out of `headers`.
 1. If `exchange`'s request method is not safe (Section 4.2.1 of {{!RFC7231}}) or
   not cacheable (Section 4.2.3 of {{!RFC7231}}), return "invalid".
+1. If `exchange`'s response is not complete, (Section 3.1 of {{!RFC7231}}),


nyaxt · 2018-11-20T05:49:11Z

draft-yasskin-http-origin-signed-responses.md

@@ -838,6 +838,10 @@ signature returns "valid", return "valid". Otherwise, return "invalid".
 1. Let `exchange` be the exchange metadata and headers parsed out of `headers`.
 1. If `exchange`'s request method is not safe (Section 4.2.1 of {{!RFC7231}}) or
   not cacheable (Section 4.2.3 of {{!RFC7231}}), return "invalid".
+1. If `exchange`'s response is not complete, (Section 3.1 of {{!RFC7231}}),
+   return "invalid".
+1. If `exchange`'s response is not cachable by a public cache (Section 3 of


nyaxt · 2018-11-20T05:51:39Z

draft-yasskin-http-origin-signed-responses.md

@@ -838,6 +838,10 @@ signature returns "valid", return "valid". Otherwise, return "invalid".
 1. Let `exchange` be the exchange metadata and headers parsed out of `headers`.
 1. If `exchange`'s request method is not safe (Section 4.2.1 of {{!RFC7231}}) or
   not cacheable (Section 4.2.3 of {{!RFC7231}}), return "invalid".


jyasskin

Thanks!

nyaxt requested a review from jyasskin November 2, 2018 01:13

Require the response to be complete and cachable by a public cache.

652ec78

nyaxt force-pushed the statuscode branch from 8a2ff70 to 652ec78 Compare November 5, 2018 01:50

loading spec: response status must be 200

0341c4b

nyaxt mentioned this pull request Nov 9, 2018

Cache-Control header / past Expires in inner exchange's response. #338

Open

jyasskin requested changes Nov 14, 2018

View reviewed changes

nyaxt force-pushed the statuscode branch 2 times, most recently from 48c9fe2 to ce2da0b Compare November 20, 2018 05:54

nyaxt commented Nov 20, 2018

View reviewed changes

Fix @jyasskin's comments

95b849a

nyaxt force-pushed the statuscode branch from ce2da0b to 95b849a Compare November 20, 2018 05:59

jyasskin approved these changes Nov 20, 2018

View reviewed changes

jyasskin merged commit 827cb70 into WICG:master Nov 20, 2018

jyasskin mentioned this pull request Nov 20, 2018

Define the browser algorithm for determining cacheability of status codes #342

Open

nyaxt mentioned this pull request Dec 13, 2018

Implementation snapshot b3 #350

Closed

10 tasks

nyaxt mentioned this pull request Jan 7, 2019

"is complete" check requires the entire response body. Streaming loading? #365

Closed

twifkak mentioned this pull request Feb 28, 2019

Modify cache headers ampproject/amppackager#251

Closed

toyoshim mentioned this pull request Aug 5, 2019

Allowable HTTP status code for Bundle? #478

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject uncachable status code #326

Reject uncachable status code #326

nyaxt commented Nov 2, 2018 •

edited by pr-preview bot

nyaxt commented Nov 2, 2018

nyaxt commented Nov 2, 2018

kinu commented Nov 2, 2018

kinu commented Nov 2, 2018

mnot commented Nov 3, 2018

nyaxt commented Nov 5, 2018

nyaxt commented Nov 5, 2018

jyasskin Nov 14, 2018

nyaxt Nov 20, 2018

jyasskin Nov 14, 2018

nyaxt Nov 20, 2018

jyasskin Nov 14, 2018

nyaxt Nov 20, 2018

jyasskin Nov 14, 2018

nyaxt left a comment

nyaxt Nov 20, 2018

nyaxt Nov 20, 2018

nyaxt Nov 20, 2018

jyasskin left a comment

	1. If `exchange`'s response is not complete, (Section 3.1 of {{!RFC7231}}),
	1. If `exchange`'s response is not complete, (Section 3.1 of {{!RFC7234}}),

	1. If `exchange`'s response is not cachable by a public cache (Section 3 of
	1. If Section 3 of {{RFC7234}} forbids a shared cache from storing `exchange`'s response, return "invalid".

Reject uncachable status code #326

Reject uncachable status code #326

Conversation

nyaxt commented Nov 2, 2018 • edited by pr-preview bot

nyaxt commented Nov 2, 2018

nyaxt commented Nov 2, 2018

kinu commented Nov 2, 2018

kinu commented Nov 2, 2018

mnot commented Nov 3, 2018

nyaxt commented Nov 5, 2018

nyaxt commented Nov 5, 2018

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

nyaxt 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

jyasskin left a comment

Choose a reason for hiding this comment

nyaxt commented Nov 2, 2018 •

edited by pr-preview bot