Bug 468426 - improved support for "Vary: Cookie"-headers , r=biesi a=bsmedberg
authorbjarne@runitsoft.com
Fri, 26 Nov 2010 12:13:37 -0500
changeset 58260 63c63bac58ec
parent 58259 91465997ac12
child 58261 7d09a3dfbfcb
push id17223
push usereakhgari@mozilla.com
push dateFri, 26 Nov 2010 17:21:53 +0000
treeherdermozilla-central@0d7dd95a1d9a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbiesi, bsmedberg
bugs468426
milestone2.0b8pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 468426 - improved support for "Vary: Cookie"-headers , r=biesi a=bsmedberg
netwerk/protocol/http/nsHttpChannel.cpp
netwerk/protocol/http/nsHttpChannel.h
netwerk/test/unit/test_bug468426.js
netwerk/test/unit/test_bug510359.js
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -1559,71 +1559,117 @@ nsHttpChannel::ResolveProxy()
         return rv;
 
     return pps->AsyncResolve(mURI, 0, this, getter_AddRefs(mProxyRequest));
 }
 
 PRBool
 nsHttpChannel::ResponseWouldVary()
 {
-    PRBool result = PR_FALSE;
+    nsresult rv;
     nsCAutoString buf, metaKey;
     mCachedResponseHead->GetHeader(nsHttp::Vary, buf);
     if (!buf.IsEmpty()) {
         NS_NAMED_LITERAL_CSTRING(prefix, "request-");
 
         // enumerate the elements of the Vary header...
         char *val = buf.BeginWriting(); // going to munge buf
         char *token = nsCRT::strtok(val, NS_HTTP_HEADER_SEPS, &val);
         while (token) {
+            LOG(("nsHttpChannel::ResponseWouldVary [this=%x] " \
+                 "processing %s\n",
+                 this, token));
             //
             // if "*", then assume response would vary.  technically speaking,
             // "Vary: header, *" is not permitted, but we allow it anyways.
             //
-            // if the response depends on the value of the "Cookie" header, then
-            // bail since we do not store cookies in the cache.  this is done
-            // for the following reasons:
+            // We hash values of cookie-headers for the following reasons:
             //
             //   1- cookies can be very large in size
             //
             //   2- cookies may contain sensitive information.  (for parity with
             //      out policy of not storing Set-cookie headers in the cache
             //      meta data, we likewise do not want to store cookie headers
             //      here.)
             //
-            // this implementation is obviously not fully standards compliant, but
-            // it is perhaps most prudent given the above issues.
-            //
-            if ((*token == '*') || (PL_strcasecmp(token, "cookie") == 0)) {
-                result = PR_TRUE;
-                break;
+            if (*token == '*')
+                return PR_TRUE; // if we encounter this, just get out of here
+
+            // build cache meta data key...
+            metaKey = prefix + nsDependentCString(token);
+
+            // check the last value of the given request header to see if it has
+            // since changed.  if so, then indeed the cached response is invalid.
+            nsXPIDLCString lastVal;
+            mCacheEntry->GetMetaDataElement(metaKey.get(), getter_Copies(lastVal));
+            LOG(("nsHttpChannel::ResponseWouldVary [this=%x] " \
+                    "stored value = %c%s%c\n", this, '"', lastVal.get(), '"'));
+
+            // Look for value of "Cookie" in the request headers
+            nsHttpAtom atom = nsHttp::ResolveAtom(token);
+            const char *newVal = mRequestHead.PeekHeader(atom);
+            if (!lastVal.IsEmpty()) {
+                // value for this header in cache, but no value in request
+                if (!newVal)
+                    return PR_TRUE; // yes - response would vary
+
+                // If this is a cookie-header, stored metadata is not
+                // the value itself but the hash. So we also hash the
+                // outgoing value here in order to compare the hashes
+                nsCAutoString hash;
+                if (atom == nsHttp::Cookie) {
+                    rv = Hash(newVal, hash);
+                    // If hash failed, be conservative (the cached hash
+                    // exists at this point) and claim response would vary
+                    if (NS_FAILED(rv))
+                        return PR_TRUE;
+                    newVal = hash.get();
+
+                    LOG(("nsHttpChannel::ResponseWouldVary [this=%x] " \
+                            "set-cookie value hashed to %s\n",
+                         this, newVal));
+                }
+
+                if (strcmp(newVal, lastVal))
+                    return PR_TRUE; // yes, response would vary
+
+            } else if (newVal) { // old value is empty, but newVal is set
+                return PR_TRUE;
             }
-            else {
-                // build cache meta data key...
-                metaKey = prefix + nsDependentCString(token);
-
-                // check the last value of the given request header to see if it has
-                // since changed.  if so, then indeed the cached response is invalid.
-                nsXPIDLCString lastVal;
-                mCacheEntry->GetMetaDataElement(metaKey.get(), getter_Copies(lastVal));
-                if (lastVal) {
-                    nsHttpAtom atom = nsHttp::ResolveAtom(token);
-                    const char *newVal = mRequestHead.PeekHeader(atom);
-                    if (newVal && (strcmp(newVal, lastVal) != 0)) {
-                        result = PR_TRUE; // yes, response would vary
-                        break;
-                    }
-                }
-                
-                // next token...
-                token = nsCRT::strtok(val, NS_HTTP_HEADER_SEPS, &val);
-            }
+
+            // next token...
+            token = nsCRT::strtok(val, NS_HTTP_HEADER_SEPS, &val);
         }
     }
-    return result;
+    return PR_FALSE;
+}
+
+nsresult
+nsHttpChannel::Hash(const char *buf, nsACString &hash)
+{
+    nsresult rv;
+    if (!mHasher) {
+        mHasher = do_CreateInstance(NS_CRYPTO_HASH_CONTRACTID, &rv);
+        if (NS_FAILED(rv)) {
+            LOG(("nsHttpChannel: Failed to instantiate crypto-hasher"));
+            return rv;
+        }
+    }
+
+    rv = mHasher->Init(nsICryptoHash::SHA1);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+   rv = mHasher->Update(reinterpret_cast<unsigned const char*>(buf),
+                         strlen(buf));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    rv = mHasher->Finish(PR_TRUE, hash);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
 // nsHttpChannel <byte-range>
 //-----------------------------------------------------------------------------
 
 nsresult
 nsHttpChannel::SetupByteRangeRequest(PRUint32 partialLen)
@@ -2930,16 +2976,17 @@ nsHttpChannel::InitOfflineCacheEntry()
 }
 
 
 nsresult
 nsHttpChannel::AddCacheEntryHeaders(nsICacheEntryDescriptor *entry)
 {
     nsresult rv;
 
+    LOG(("nsHttpChannel::AddCacheEntryHeaders [this=%x] begin", this));
     // Store secure data in memory only
     if (mSecurityInfo)
         entry->SetSecurityInfo(mSecurityInfo);
 
     // Store the HTTP request method with the cache entry so we can distinguish
     // for example GET and HEAD responses.
     rv = entry->SetMetaDataElement("request-method",
                                    mRequestHead.Method().get());
@@ -2947,39 +2994,63 @@ nsHttpChannel::AddCacheEntryHeaders(nsIC
 
     // Store the HTTP authorization scheme used if any...
     rv = StoreAuthorizationMetaData(entry);
     if (NS_FAILED(rv)) return rv;
 
     // Iterate over the headers listed in the Vary response header, and
     // store the value of the corresponding request header so we can verify
     // that it has not varied when we try to re-use the cached response at
-    // a later time.  Take care not to store "Cookie" headers though.  We
-    // take care of "Vary: cookie" in ResponseWouldVary.
+    // a later time.  Take care to store "Cookie" headers only as hashes
+    // due to security considerations and the fact that they can be pretty
+    // large (bug 468426). We take care of "Vary: cookie" in ResponseWouldVary.
     //
     // NOTE: if "Vary: accept, cookie", then we will store the "accept" header
     // in the cache.  we could try to avoid needlessly storing the "accept"
     // header in this case, but it doesn't seem worth the extra code to perform
     // the check.
     {
         nsCAutoString buf, metaKey;
         mResponseHead->GetHeader(nsHttp::Vary, buf);
         if (!buf.IsEmpty()) {
             NS_NAMED_LITERAL_CSTRING(prefix, "request-");
            
             char *val = buf.BeginWriting(); // going to munge buf
             char *token = nsCRT::strtok(val, NS_HTTP_HEADER_SEPS, &val);
             while (token) {
-                if ((*token != '*') && (PL_strcasecmp(token, "cookie") != 0)) {
+                LOG(("nsHttpChannel::AddCacheEntryHeaders [this=%x] " \
+                        "processing %s", this, token));
+                if (*token != '*') {
                     nsHttpAtom atom = nsHttp::ResolveAtom(token);
-                    const char *requestVal = mRequestHead.PeekHeader(atom);
-                    if (requestVal) {
+                    const char *val = mRequestHead.PeekHeader(atom);
+                    nsCAutoString hash;
+                    if (val) {
+                        // If cookie-header, store a hash of the value
+                        if (atom == nsHttp::Cookie) {
+                            LOG(("nsHttpChannel::AddCacheEntryHeaders [this=%x] " \
+                                    "cookie-value %s", this, val));
+                            rv = Hash(val, hash);
+                            // If hash failed, store a string not very likely
+                            // to be the result of subsequent hashes
+                            if (NS_FAILED(rv))
+                                val = "<hash failed>";
+                            else
+                                val = hash.get();
+
+                            LOG(("   hashed to %s\n", val));
+                        }
+
                         // build cache meta data key and set meta data element...
                         metaKey = prefix + nsDependentCString(token);
-                        entry->SetMetaDataElement(metaKey.get(), requestVal);
+                        entry->SetMetaDataElement(metaKey.get(), val);
+                    } else {
+                        LOG(("nsHttpChannel::AddCacheEntryHeaders [this=%x] " \
+                                "clearing metadata for %s", this, token));
+                        metaKey = prefix + nsDependentCString(token);
+                        entry->SetMetaDataElement(metaKey.get(), nsnull);
                     }
                 }
                 token = nsCRT::strtok(val, NS_HTTP_HEADER_SEPS, &val);
             }
         }
     }
 
 
--- a/netwerk/protocol/http/nsHttpChannel.h
+++ b/netwerk/protocol/http/nsHttpChannel.h
@@ -58,16 +58,17 @@
 #include "nsIPrompt.h"
 #include "nsIResumableChannel.h"
 #include "nsIProtocolProxyCallback.h"
 #include "nsICancelable.h"
 #include "nsIHttpAuthenticableChannel.h"
 #include "nsITraceableChannel.h"
 #include "nsIHttpChannelAuthProvider.h"
 #include "nsIAsyncVerifyRedirectCallback.h"
+#include "nsICryptoHash.h"
 
 class nsAHttpConnection;
 class AutoRedirectVetoNotifier;
 
 using namespace mozilla::net;
 
 //-----------------------------------------------------------------------------
 // nsHttpChannel
@@ -259,16 +260,22 @@ private:
     /**
      * A function that takes care of reading STS headers and enforcing STS 
      * load rules.  After a secure channel is erected, STS requires the channel
      * to be trusted or any STS header data on the channel is ignored.
      * This is called from ProcessResponse.
      */
     nsresult ProcessSTSHeader();
 
+    /**
+     * Computes and returns a 64 bit encoded string holding a hash of the
+     * input buffer. Input buffer must be a null-terminated string.
+     */
+    nsresult Hash(const char *buf, nsACString &hash);
+
 private:
     nsCOMPtr<nsISupports>             mSecurityInfo;
     nsCOMPtr<nsICancelable>           mProxyRequest;
 
     nsRefPtr<nsInputStreamPump>       mTransactionPump;
     nsRefPtr<nsHttpTransaction>       mTransaction;
 
     PRUint64                          mLogicalOffset;
@@ -337,14 +344,16 @@ private:
     PRUint32                          mFallingBack              : 1;
     PRUint32                          mWaitingForRedirectCallback : 1;
     // True if mRequestTime has been set. In such a case it is safe to update
     // the cache entry's expiration time. Otherwise, it is not(see bug 567360).
     PRUint32                          mRequestTimeInitialized : 1;
 
     nsTArray<nsContinueRedirectionFunc> mRedirectFuncStack;
 
+    nsCOMPtr<nsICryptoHash>        mHasher;
+
     nsresult WaitForRedirectCallback();
     void PushRedirectAsyncFunc(nsContinueRedirectionFunc func);
     void PopRedirectAsyncFunc(nsContinueRedirectionFunc func);
 };
 
 #endif // nsHttpChannel_h__
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit/test_bug468426.js
@@ -0,0 +1,104 @@
+do_load_httpd_js();
+
+var httpserver = new nsHttpServer();
+var index = 0;
+var tests = [
+    // Initial request. Cached variant will have no cookie
+    { url : "/bug468426", server : "0", expected : "0", cookie: null},
+
+    // Cache now contains a variant with no value for cookie. If we don't
+    // set cookie we expect to receive the cached variant
+    { url : "/bug468426", server : "1", expected : "0", cookie: null},
+
+    // Cache still contains a variant with no value for cookie. If we
+    // set a value for cookie we expect a fresh value
+    { url : "/bug468426", server : "2", expected : "2", cookie: "c=2"},
+
+    // Cache now contains a variant with cookie "c=2". If the request
+    // also set cookie "c=2", we expect to receive the cached variant.
+    { url : "/bug468426", server : "3", expected : "2", cookie: "c=2"},
+
+    // Cache still contains a variant with cookie "c=2". When setting
+    // cookie "c=4" in the request we expect a fresh value
+    { url : "/bug468426", server : "4", expected : "4", cookie: "c=4"},
+
+    // Cache now contains a variant with cookie "c=4". When setting
+    // cookie "c=4" in the request we expect the cached variant
+    { url : "/bug468426", server : "5", expected : "4", cookie: "c=4"},
+    
+    // Cache still contains a variant with cookie "c=4". When setting
+    // no cookie in the request we expect a fresh value
+    { url : "/bug468426", server : "6", expected : "6", cookie: null},
+
+];
+
+function getCacheService() {
+    return Components.classes["@mozilla.org/network/cache-service;1"]
+            .getService(Components.interfaces.nsICacheService);
+}
+
+function setupChannel(suffix, value, cookie) {
+    var ios = Components.classes["@mozilla.org/network/io-service;1"]
+            .getService(Ci.nsIIOService);
+    var chan = ios.newChannel("http://localhost:4444" + suffix, "", null);
+    var httpChan = chan.QueryInterface(Components.interfaces.nsIHttpChannel);
+    httpChan.requestMethod = "GET";
+    httpChan.setRequestHeader("x-request", value, false);
+    if (cookie != null)
+        httpChan.setRequestHeader("Cookie", cookie, false);
+    return httpChan;
+}
+
+function triggerNextTest() {
+    var channel = setupChannel(tests[index].url, tests[index].server, tests[index].cookie);
+    channel.asyncOpen(new ChannelListener(checkValueAndTrigger, null), null);
+}
+
+function checkValueAndTrigger(request, data, ctx) {
+    do_check_eq(tests[index].expected, data);
+
+    if (index < tests.length - 1) {
+        index++;
+        // This call happens in onStopRequest from the channel. Opening a new
+        // channel to the same url here is no good idea!  Post it instead...
+        do_timeout(1, "triggerNextTest();");
+    } else {
+        httpserver.stop(do_test_finished);
+    }
+}
+
+function run_test() {
+    httpserver.registerPathHandler("/bug468426", handler);
+    httpserver.start(4444);
+
+    // Clear cache and trigger the first test
+    getCacheService().evictEntries(
+            Components.interfaces.nsICache.STORE_ANYWHERE);
+    triggerNextTest();
+
+    do_test_pending();
+}
+
+function handler(metadata, response) {
+    var body = "unset";
+    try {
+        body = metadata.getHeader("x-request");
+    } catch(e) { }
+    response.setStatusLine(metadata.httpVersion, 200, "Ok");
+    response.setHeader("Content-Type", "text/plain", false);
+    response.setHeader("Last-Modified", getDateString(-1), false);
+    response.setHeader("Vary", "Cookie", false);
+    response.bodyOutputStream.write(body, body.length);
+}
+
+function getDateString(yearDelta) {
+    var months = [ 'Jan', 'Feb', 'Mar', 'Apr', 'May', 'Jun', 'Jul', 'Aug',
+            'Sep', 'Oct', 'Nov', 'Dec' ];
+    var days = [ 'Sun', 'Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat' ];
+
+    var d = new Date();
+    return days[d.getUTCDay()] + ", " + d.getUTCDate() + " "
+            + months[d.getUTCMonth()] + " " + (d.getUTCFullYear() + yearDelta)
+            + " " + d.getUTCHours() + ":" + d.getUTCMinutes() + ":"
+            + d.getUTCSeconds() + " UTC";
+}
--- a/netwerk/test/unit/test_bug510359.js
+++ b/netwerk/test/unit/test_bug510359.js
@@ -14,16 +14,17 @@ function getCacheService() {
 
 function setupChannel(suffix, value) {
     var ios = Components.classes["@mozilla.org/network/io-service;1"]
             .getService(Ci.nsIIOService);
     var chan = ios.newChannel("http://localhost:4444" + suffix, "", null);
     var httpChan = chan.QueryInterface(Components.interfaces.nsIHttpChannel);
     httpChan.requestMethod = "GET";
     httpChan.setRequestHeader("x-request", value, false);
+    httpChan.setRequestHeader("Cookie", "c="+value, false);
     return httpChan;
 }
 
 function triggerNextTest() {
     var channel = setupChannel(tests[index].url, tests[index].server);
     channel.asyncOpen(new ChannelListener(checkValueAndTrigger, null), null);
 }