Bug 72414 – Entity-header extension headers honored on 304 responses.

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Thomas Sepez]

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.

[Thomas Sepez]

Created attachment 115239 [details]
Flunking testcases

[Adam Barth]

Maybe do all X-WebKit-* headers?  Anything we put in that namespace is going to be an entity header.

[Thomas Sepez]

X-Frame-Options as well.

[Thomas Sepez]

Created attachment 115255 [details]
Flunking testcases

Fix cut-n-paste, extend expires to near the end of time.

[Thomas Sepez]

Created attachment 115416 [details]
Flunking testcases

Add XFO header test as well just for completeness.

[Thomas Sepez]

I'm not going to be able to look at this right now.

[Mike West]

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.

[Mike West]

Created attachment 186598 [details]
Patch

[Mike West]

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?

[Mike West]

Created attachment 186604 [details]
Patch

[Mike West]

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. :)

[Alexey Proskuryakov]

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.

[Mike West]

(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.

[Mike West]

Created attachment 186674 [details]
Patch

[Mike West]

(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.

[Alexey Proskuryakov]

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?

[Mike West]

(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.

[Mike West]

Created attachment 186782 [details]
Patch

[Mike West]

Created attachment 186824 [details]
Patch for landing?

[Mike West]

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.

[Alexey Proskuryakov]

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.

[Mike West]

Created attachment 186892 [details]
Patch

[Mike West]

(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.

[Alexey Proskuryakov]

Comment on attachment 186892 [details]
Patch

Looks good to me.

[WebKit Review Bot]

Comment on attachment 186892 [details]
Patch

Clearing flags on attachment: 186892

Committed r142068: <http://trac.webkit.org/changeset/142068>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.