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.
Created attachment 115239 [details] Flunking testcases
Maybe do all X-WebKit-* headers? Anything we put in that namespace is going to be an entity header.
X-Frame-Options as well.
Created attachment 115255 [details] Flunking testcases Fix cut-n-paste, extend expires to near the end of time.
Created attachment 115416 [details] Flunking testcases Add XFO header test as well just for completeness.
I'm not going to be able to look at this right now.
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.
Created attachment 186598 [details] Patch
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?
Created attachment 186604 [details] Patch
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 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.
(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.
Created attachment 186674 [details] Patch
(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 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?
(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.
Created attachment 186782 [details] Patch
Created attachment 186824 [details] Patch for landing?
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 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.
Created attachment 186892 [details] Patch
(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 on attachment 186892 [details] Patch Looks good to me.
Comment on attachment 186892 [details] Patch Clearing flags on attachment: 186892 Committed r142068: <http://trac.webkit.org/changeset/142068>
All reviewed patches have been landed. Closing bug.