Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating stored headers II #488

Closed
mnot opened this issue Oct 22, 2020 · 4 comments · Fixed by #490
Closed

Updating stored headers II #488

mnot opened this issue Oct 22, 2020 · 4 comments · Fixed by #490

Comments

@mnot
Copy link
Member

mnot commented Oct 22, 2020

Following #165, I updated tests to match the new requirements and the results are not encouraging.

Ignoring implementations that obviously don't support updating stored headers (nuster, Fastly, nginx), the new requirement:

Caches MAY update the following header fields: Content-Encoding, Content-Length, Content-MD5 (Section 14.15 of [RFC2616]), Content-Range, ETag

has the effect of making browsers more conferment, but other implementations less conformant (because they do update those headers).

I think the most rational approach to address this would be to soften and contextualise the requirement, something like:

In some cases, caches (especially in user agents) store processed representations of the received response, rather than the response itself, and updating header fields that affect that processing can result in inconsistent behaviour and security issues. Caches in this situation MAY omit these header fields from updating stored representations on an exceptional basis, but SHOULD limit such omission to those fields necessary to assure integrity of the stored representation.

For example, a browser cache might store a response after removing content-codings; updating that response with a different Content-Encoding header field would be problematic. Likewise, a browser cache might store a post-parse tree representation of HTML, rather than the bytes received in the response; updating the Content-Type header field would not be workable in this case.

Furthermore, some fields are automatically processed and removed by the HTTP implementation; for example, the Content-Range header field. Implementations MAY automatically omit such header fields from updates, even when the processing does not actually occur.

Finally, the Content-Length header field MUST NOT be updated, as this can cause security issues (see [ref]).

Note that the Content-* prefix is not a signal that a header is omitted from update; it is only a convention for content-related fields.

Note that this doesn't mention ETag or Last-Modified, because if they have a different value, the response won't be selected for update.

@mnot mnot added the caching label Oct 22, 2020
@mnot
Copy link
Member Author

mnot commented Oct 22, 2020

pinging folks involved in #165: @annevk @mikewest @MattMenke2 @agrover @martinthomson @MikeBishop @jyasskin

Thoughts re the proposal above?

@mnot mnot self-assigned this Oct 22, 2020
@martinthomson
Copy link
Contributor

I'm happy with that. At least, I can't see a reason that it would fail. It's good for everything I thought of (but that's not an actual analysis, sorry).

@annevk
Copy link
Contributor

annevk commented Oct 22, 2020

That seems reasonable, though I guess it means we have to eventually define this in Fetch for browsers in particular to avoid having to keep running into subtle compatibility issues.

@mnot
Copy link
Member Author

mnot commented Oct 23, 2020

I think it's appropriate to define the browser-specific behaviours in Fetch; the decisions here are tied into the details of how the various caches work. All I'd ask is that it not be too liberal in using this exception; a number of the current headers (e.g., Content-Location) excepted by browsers for no good reason. Happy to help out over there...

BTW, @annevk see also #474 re: constraints on image cache and friends...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants