Bug 72414 - Entity-header extension headers honored on 304 responses.
Summary: Entity-header extension headers honored on 304 responses.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 71851
  Show dependency treegraph
 
Reported: 2011-11-15 14:00 PST by Thomas Sepez
Modified: 2013-02-06 23:43 PST (History)
8 users (show)

See Also:


Attachments
Flunking testcases (7.34 KB, patch)
2011-11-15 14:03 PST, Thomas Sepez
no flags Details | Formatted Diff | Diff
Flunking testcases (7.34 KB, patch)
2011-11-15 14:55 PST, Thomas Sepez
no flags Details | Formatted Diff | Diff
Flunking testcases (10.50 KB, patch)
2011-11-16 11:34 PST, Thomas Sepez
no flags Details | Formatted Diff | Diff
Patch (18.38 KB, patch)
2013-02-05 04:43 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (17.21 KB, patch)
2013-02-05 05:01 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (16.78 KB, patch)
2013-02-05 12:18 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (19.21 KB, patch)
2013-02-06 01:16 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing? (19.38 KB, patch)
2013-02-06 04:29 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (19.54 KB, patch)
2013-02-06 12:14 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Sepez 2011-11-15 14:00:22 PST
RFC2616 10.3.5 says that headers returned by a 304 Not Modified response should update the fields from the original response.  But there are a class of entity headers which should not be sent by servers when generating a 304 response.  If they should be sent, browsers have adopted a policy of ignoring them and using the field values from the original non-cached request instead.  

FRC2616 7.1 calls out extension headers to be entity headers.  While it is a stretch to imagine that all of these are such, the following describe entities:

X-Content-* headers
X-WebKit-CSP since it is a placeholder for X-Content-Security-Policy
X-XSS-Protection

We've run into at least one proxy that adds an X-Content-Type-Options to its 304 requests that was not present in the orignal response, which is what has prompted thinking about the general problem.  In particular, it would be bad if a 304 response turned off protections set by headers on the original uncached request.

I've got a couple of test cases for X-WebKit-CSP and X-XSS-Protection that I will be adding shortly.
Comment 1 Thomas Sepez 2011-11-15 14:03:11 PST
Created attachment 115239 [details]
Flunking testcases
Comment 2 Adam Barth 2011-11-15 14:10:12 PST
Maybe do all X-WebKit-* headers?  Anything we put in that namespace is going to be an entity header.
Comment 3 Thomas Sepez 2011-11-15 14:12:43 PST
X-Frame-Options as well.
Comment 4 Thomas Sepez 2011-11-15 14:55:11 PST
Created attachment 115255 [details]
Flunking testcases

Fix cut-n-paste, extend expires to near the end of time.
Comment 5 Thomas Sepez 2011-11-16 11:34:55 PST
Created attachment 115416 [details]
Flunking testcases

Add XFO header test as well just for completeness.
Comment 6 Thomas Sepez 2012-04-13 13:05:15 PDT
I'm not going to be able to look at this right now.
Comment 7 Mike West 2013-02-05 03:25:25 PST
I'm trying to address the Chromium side at https://codereview.chromium.org/12224008/, and Jochen's suggested a direction for the rest of WebCore. Let's see how it goes.
Comment 8 Mike West 2013-02-05 04:43:18 PST
Created attachment 186598 [details]
Patch
Comment 9 Mike West 2013-02-05 04:44:38 PST
The attached patch works locally for the Mac port. I've skipped the tests for Chromium for the moment, as it looks like that will require a Chromium-side change that's currently up for review.

Nate: Jochen suggested that you'd be a good reviewer for this change. Would you mind taking a look?
Comment 10 Mike West 2013-02-05 05:01:13 PST
Created attachment 186604 [details]
Patch
Comment 11 Mike West 2013-02-05 05:02:21 PST
Hrm. I ran the tests through the chromium port locally, and they passed, which surprised me because I'd misunderstood the network stack's role here. So, looks like I can run the tests everywhere. Yay. :)
Comment 12 Alexey Proskuryakov 2013-02-05 10:19:32 PST
Comment on attachment 186604 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186604&action=review

> Source/WebCore/loader/cache/CachedResource.cpp:740
>      DEFINE_STATIC_LOCAL(const AtomicString, contentHeaderPrefix, ("content-", AtomicString::ConstructFromLiteral));
> +    DEFINE_STATIC_LOCAL(const AtomicString, xContentHeaderPrefix, ("x-content-", AtomicString::ConstructFromLiteral));
> +    DEFINE_STATIC_LOCAL(const AtomicString, xFrameHeaderPrefix, ("x-frame-", AtomicString::ConstructFromLiteral));
> +    DEFINE_STATIC_LOCAL(const AtomicString, xWebKitHeaderPrefix, ("x-webkit-", AtomicString::ConstructFromLiteral));
> +    DEFINE_STATIC_LOCAL(const AtomicString, xXSSHeaderPrefix, ("x-xss-", AtomicString::ConstructFromLiteral));

What's the benefit of having AtomicStrings here? These constants are only used as prefixes AFAICT.
Comment 13 Mike West 2013-02-05 10:55:00 PST
(In reply to comment #12)
> (From update of attachment 186604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186604&action=review
> 
> > Source/WebCore/loader/cache/CachedResource.cpp:740
> >      DEFINE_STATIC_LOCAL(const AtomicString, contentHeaderPrefix, ("content-", AtomicString::ConstructFromLiteral));
> > +    DEFINE_STATIC_LOCAL(const AtomicString, xContentHeaderPrefix, ("x-content-", AtomicString::ConstructFromLiteral));
> > +    DEFINE_STATIC_LOCAL(const AtomicString, xFrameHeaderPrefix, ("x-frame-", AtomicString::ConstructFromLiteral));
> > +    DEFINE_STATIC_LOCAL(const AtomicString, xWebKitHeaderPrefix, ("x-webkit-", AtomicString::ConstructFromLiteral));
> > +    DEFINE_STATIC_LOCAL(const AtomicString, xXSSHeaderPrefix, ("x-xss-", AtomicString::ConstructFromLiteral));
> 
> What's the benefit of having AtomicStrings here? These constants are only used as prefixes AFAICT.

They're only used as prefixes, correct. AtomicString is probably overkill.

I'm not even sure this is going to be called often enough to worry about defining static locals.
Comment 14 Mike West 2013-02-05 12:18:14 PST
Created attachment 186674 [details]
Patch
Comment 15 Mike West 2013-02-05 12:19:58 PST
(In reply to comment #14)
> Created an attachment (id=186674) [details]
> Patch

This patch drops the static locals in favor of string literals. I don't think it'll end up making a performance difference; if it does, I'll happily put thought into optimizing.
Comment 16 Alexey Proskuryakov 2013-02-05 13:06:48 PST
Comment on attachment 186674 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186674&action=review

Looks good to me.

> Source/WebCore/loader/cache/CachedResource.cpp:741
>          // Don't allow 304 response to update content headers, these can't change but some servers send wrong values.

This comment seems incomplete now.

In fact, I'd be pretty surprised to see this list of prefixes in code without reading this bug first.

Some things to explain would be: (1) entity header fields should not be sent with 304 responses, but when they are, we ignore them, and (2) per RFC 2612 paragraph 7.1, all extension header fields are treated as entity header fields, and (3) why we have specific prefixes instead of just "x-".

Possibly fourth thing to explain, or maybe a flaw with this patch: why are other entity headers not here, namely Expires and Last-Modified?
Comment 17 Mike West 2013-02-06 00:28:48 PST
(In reply to comment #16)
> (From update of attachment 186674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186674&action=review
> 
> Looks good to me.
> 
> > Source/WebCore/loader/cache/CachedResource.cpp:741
> >          // Don't allow 304 response to update content headers, these can't change but some servers send wrong values.
> 
> This comment seems incomplete now.
> 
> In fact, I'd be pretty surprised to see this list of prefixes in code without reading this bug first.
> 
> Some things to explain would be: (1) entity header fields should not be sent with 304 responses, but when they are, we ignore them, and (2) per RFC 2612 paragraph 7.1, all extension header fields are treated as entity header fields, and (3) why we have specific prefixes instead of just "x-".
> 
> Possibly fourth thing to explain, or maybe a flaw with this patch: why are other entity headers not here, namely Expires and Last-Modified?

You're right. I originally wanted to keep this change as small and simple as possible, but it would be better to be complete. I'll change this method to match Chromium's network stack which filters more completely.
Comment 18 Mike West 2013-02-06 01:16:16 PST
Created attachment 186782 [details]
Patch
Comment 19 Mike West 2013-02-06 04:29:20 PST
Created attachment 186824 [details]
Patch for landing?
Comment 20 Mike West 2013-02-06 04:31:53 PST
Comment on attachment 186824 [details]
Patch for landing?

(In reply to comment #19)
> Created an attachment (id=186824) [details]
> Patch for landing?

I've addressed your feedback, and I'm carrying over your r+, ap. I'd appreciate, however, it if you took another look at the patch since it's changed quite a bit. If you like it, cq+ it, please. If not, let me know and I'll take another pass.
Comment 21 Alexey Proskuryakov 2013-02-06 11:13:17 PST
Comment on attachment 186824 [details]
Patch for landing?

View in context: https://bugs.webkit.org/attachment.cgi?id=186824&action=review

> Source/WebCore/ChangeLog:30
> +            In an anonymous namespace, this patch adds two arrays containing the

We don't use anonymous namespaces, because they make debugging harder (can't easily reference symbols in debugger command line), and also as a coding style matter for consistency with existing code.

Constant global variables already have the right linkage in C++.

> Source/WebCore/loader/cache/CachedResource.cpp:65
> +const char* const headersToIgnoreAfterRevalidation[] = {

This is actual meaningful code for this file, I'd put it below WTF forward declaration.

> Source/WebCore/loader/cache/CachedResource.cpp:108
>  template<> struct SequenceMemoryInstrumentationTraits<WebCore::CachedResourceClient*> {

I wonder why we have to forward declare something from WTF. This does not look safe.

> Source/WebCore/loader/cache/CachedResource.cpp:790
> +        // such headers to update the original request. To match Chromium's
> +        // network stack, we'll base this on the list defined by RFC2616 7.1,

I'd phrase this a little differently. It's not a goal to match Chromium's network stack. We do that because we think that its behavior makes sense, not because it's critical to have the same list here and there.
Comment 22 Mike West 2013-02-06 12:14:02 PST
Created attachment 186892 [details]
Patch
Comment 23 Mike West 2013-02-06 12:17:08 PST
(In reply to comment #21)
> (From update of attachment 186824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186824&action=review
> 
> > Source/WebCore/ChangeLog:30
> > +            In an anonymous namespace, this patch adds two arrays containing the
> 
> We don't use anonymous namespaces, because they make debugging harder (can't easily reference symbols in debugger command line), and also as a coding style matter for consistency with existing code.
> 
> Constant global variables already have the right linkage in C++.

Makes sense, thanks.

> > Source/WebCore/loader/cache/CachedResource.cpp:790
> > +        // such headers to update the original request. To match Chromium's
> > +        // network stack, we'll base this on the list defined by RFC2616 7.1,
> 
> I'd phrase this a little differently. It's not a goal to match Chromium's network stack. We do that because we think that its behavior makes sense, not because it's critical to have the same list here and there.

I've adjusted this comment to drop the reference to Chromium. The ChangeLog still refers to Chromium, but only in the context of referring to it as the source of this list.

Thanks for taking another look.
Comment 24 Alexey Proskuryakov 2013-02-06 12:41:56 PST
Comment on attachment 186892 [details]
Patch

Looks good to me.
Comment 25 WebKit Review Bot 2013-02-06 23:42:55 PST
Comment on attachment 186892 [details]
Patch

Clearing flags on attachment: 186892

Committed r142068: <http://trac.webkit.org/changeset/142068>
Comment 26 WebKit Review Bot 2013-02-06 23:43:00 PST
All reviewed patches have been landed.  Closing bug.