Bug 1285036 - Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
☠☠ backed out by b2508db54956 ☠ ☠
authorThomas Wisniewski <wisniewskit@gmail.com>
Wed, 20 Jul 2016 12:22:43 -0400
changeset 331256 6b51b69f723ee6e543d15b4a8f55323fa2d71c45
parent 331255 b3e155c83e23389d01f5deec72274d4572525754
child 331257 6462cd2ea24987384812287c7346e2c397e570ff
push id9858
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 14:37:10 +0000
treeherdermozilla-aurora@203106ef6cb6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs1285036
milestone50.0a1
Bug 1285036 - Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
dom/base/nsContentUtils.cpp
dom/xhr/XMLHttpRequestMainThread.cpp
dom/xhr/XMLHttpRequestMainThread.h
dom/xhr/tests/test_xhr_forbidden_headers.html
testing/web-platform/meta/XMLHttpRequest/setrequestheader-case-insensitive.htm.ini
testing/web-platform/meta/XMLHttpRequest/setrequestheader-header-allowed.htm.ini
--- a/dom/base/nsContentUtils.cpp
+++ b/dom/base/nsContentUtils.cpp
@@ -7062,19 +7062,18 @@ nsContentUtils::IsForbiddenRequestHeader
 
 // static
 bool
 nsContentUtils::IsForbiddenSystemRequestHeader(const nsACString& aHeader)
 {
   static const char *kInvalidHeaders[] = {
     "accept-charset", "accept-encoding", "access-control-request-headers",
     "access-control-request-method", "connection", "content-length",
-    "cookie", "cookie2", "content-transfer-encoding", "date", "dnt",
-    "expect", "host", "keep-alive", "origin", "referer", "te", "trailer",
-    "transfer-encoding", "upgrade", "via"
+    "cookie", "cookie2", "date", "dnt", "expect", "host", "keep-alive",
+    "origin", "referer", "te", "trailer", "transfer-encoding", "upgrade", "via"
   };
   for (uint32_t i = 0; i < ArrayLength(kInvalidHeaders); ++i) {
     if (aHeader.LowerCaseEqualsASCII(kInvalidHeaders[i])) {
       return true;
     }
   }
   return false;
 }
--- a/dom/xhr/XMLHttpRequestMainThread.cpp
+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
@@ -1460,17 +1460,17 @@ XMLHttpRequestMainThread::Open(const nsA
       userpass.Append(':');
       AppendUTF16toUTF8(password.Value(), userpass);
     }
     uri->SetUserPass(userpass);
   }
 
   // Clear our record of previously set headers so future header set
   // operations will merge/override correctly.
-  mAlreadySetHeaders.Clear();
+  mAuthorRequestHeaders.Clear();
 
   // When we are called from JS we can find the load group for the page,
   // and add ourselves to it. This way any pending requests
   // will be automatically aborted if the user leaves the page.
   nsCOMPtr<nsILoadGroup> loadGroup = GetLoadGroup();
 
   nsSecurityFlags secFlags;
   nsLoadFlags loadFlags = nsIRequest::LOAD_BACKGROUND |
@@ -2475,16 +2475,19 @@ XMLHttpRequestMainThread::Send(nsIVarian
   //     an asynchronous call.
 
   // Ignore argument if method is GET, there is no point in trying to
   // upload anything
   nsAutoCString method;
   nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel));
 
   if (httpChannel) {
+    // Spec step 5
+    SetAuthorRequestHeadersOnChannel(httpChannel);
+
     httpChannel->GetRequestMethod(method); // If GET, method name will be uppercase
 
     if (!IsSystemXHR()) {
       nsCOMPtr<nsPIDOMWindowInner> owner = GetOwner();
       nsCOMPtr<nsIDocument> doc = owner ? owner->GetExtantDoc() : nullptr;
       nsContentUtils::SetFetchReferrerURIWithPolicy(mPrincipal, doc,
                                                     httpChannel, mozilla::net::RP_Default);
     }
@@ -2529,25 +2532,24 @@ XMLHttpRequestMainThread::Send(nsIVarian
                         &size_u64, defaultContentType, charset);
     NS_ENSURE_SUCCESS(rv, rv);
 
     // make sure it fits within js MAX_SAFE_INTEGER
     mUploadTotal =
       net::InScriptableRange(size_u64) ? static_cast<int64_t>(size_u64) : -1;
 
     if (postDataStream) {
-      // If no content type header was set by the client, we set it to
-      // application/xml.
+      // If author set no Content-Type, use the default from GetRequestBody().
       nsAutoCString contentType;
-      if (NS_FAILED(httpChannel->
-                      GetRequestHeader(NS_LITERAL_CSTRING("Content-Type"),
-                                       contentType))) {
+
+      GetAuthorRequestHeaderValue("content-type", contentType);
+      if (contentType.IsVoid()) {
         contentType = defaultContentType;
 
-        if (!charset.IsEmpty() && !contentType.IsVoid()) {
+        if (!charset.IsEmpty()) {
           // If we are providing the default content type, then we also need to
           // provide a charset declaration.
           contentType.Append(NS_LITERAL_CSTRING(";charset="));
           contentType.Append(charset);
         }
       }
 
       // We don't want to set a charset for streams.
@@ -2715,18 +2717,36 @@ XMLHttpRequestMainThread::Send(nsIVarian
 
   // Check if we should enabled cross-origin upload listeners.
   if (mUpload && mUpload->HasListeners()) {
     mFlagHadUploadListenersOnSend = true;
   }
 
   // Set up the preflight if needed
   if (!IsSystemXHR()) {
+    nsTArray<nsCString> CORSUnsafeHeaders;
+    const char *kCrossOriginSafeHeaders[] = {
+      "accept", "accept-language", "content-language", "content-type",
+      "last-event-id"
+    };
+    for (RequestHeader& header : mAuthorRequestHeaders) {
+      bool safe = false;
+      for (uint32_t i = 0; i < ArrayLength(kCrossOriginSafeHeaders); ++i) {
+        if (header.name.EqualsASCII(kCrossOriginSafeHeaders[i])) {
+          safe = true;
+          break;
+        }
+      }
+      if (!safe) {
+        CORSUnsafeHeaders.AppendElement(header.name);
+      }
+    }
+
     nsCOMPtr<nsILoadInfo> loadInfo = mChannel->GetLoadInfo();
-    loadInfo->SetCorsPreflightInfo(mCORSUnsafeHeaders,
+    loadInfo->SetCorsPreflightInfo(CORSUnsafeHeaders,
                                    mFlagHadUploadListenersOnSend);
   }
 
   mIsMappedArrayBuffer = false;
   if (mResponseType == XMLHttpRequestResponseType::Arraybuffer &&
       Preferences::GetBool("dom.mapped_arraybuffer.enabled", true)) {
     nsCOMPtr<nsIURI> uri;
     nsAutoCString scheme;
@@ -2842,109 +2862,72 @@ XMLHttpRequestMainThread::Send(nsIVarian
     return NS_ERROR_FAILURE;
   }
 
   return rv;
 }
 
 // http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#dom-xmlhttprequest-setrequestheader
 NS_IMETHODIMP
-XMLHttpRequestMainThread::SetRequestHeader(const nsACString& header,
-                                           const nsACString& value)
+XMLHttpRequestMainThread::SetRequestHeader(const nsACString& aName,
+                                           const nsACString& aValue)
 {
   // Steps 1 and 2
   if (mState != State::opened || mFlagSend) {
     return NS_ERROR_DOM_INVALID_STATE_ERR;
   }
-  NS_ASSERTION(mChannel, "mChannel must be valid if we're OPENED.");
 
   // Step 3
-  // Make sure we don't store an invalid header name in mCORSUnsafeHeaders
-  if (!NS_IsValidHTTPToken(header)) {
+  nsAutoCString value(aValue);
+  static const char kHTTPWhitespace[] = "\n\t\r ";
+  value.Trim(kHTTPWhitespace);
+
+  // Step 4
+  if (!NS_IsValidHTTPToken(aName) || !NS_IsReasonableHTTPHeaderValue(value)) {
     return NS_ERROR_DOM_SYNTAX_ERR;
   }
 
-  if (!mChannel)             // open() initializes mChannel, and open()
-    return NS_ERROR_FAILURE; // must be called before first setRequestHeader()
-
-  nsCOMPtr<nsIHttpChannel> httpChannel = do_QueryInterface(mChannel);
-  if (!httpChannel) {
+  // Step 5
+  bool isPrivilegedCaller = IsSystemXHR();
+  bool isForbiddenHeader = nsContentUtils::IsForbiddenRequestHeader(aName);
+  if (!isPrivilegedCaller && isForbiddenHeader) {
+    NS_WARNING("refusing to set request header");
     return NS_OK;
   }
 
-  // We will merge XHR headers, per the spec (secion 4.6.2) unless:
-  // 1 - The caller is privileged and setting an invalid header,
-  // or
-  // 2 - we have not yet explicitly set that header; this allows web
-  //     content to override default headers the first time they set them.
-  bool mergeHeaders = true;
-
-  if (!IsSystemXHR()) {
-    // Step 5: Check for dangerous headers.
-    // Prevent modification to certain HTTP headers (see bug 302263), unless
-    // the executing script is privileged.
-    if (nsContentUtils::IsForbiddenRequestHeader(header)) {
-      NS_WARNING("refusing to set request header");
-      return NS_OK;
-    }
-
-    // Check for dangerous cross-site headers
-    bool safeHeader = IsSystemXHR();
-    if (!safeHeader) {
-      // Content-Type isn't always safe, but we'll deal with it in Send()
-      const char *kCrossOriginSafeHeaders[] = {
-        "accept", "accept-language", "content-language", "content-type",
-        "last-event-id"
-      };
-      for (uint32_t i = 0; i < ArrayLength(kCrossOriginSafeHeaders); ++i) {
-        if (header.LowerCaseEqualsASCII(kCrossOriginSafeHeaders[i])) {
-          safeHeader = true;
-          break;
-        }
+  // Step 6.1
+  nsAutoCString lowercaseName;
+  nsContentUtils::ASCIIToLower(aName, lowercaseName);
+
+  // Step 6.2
+  bool notAlreadySet = true;
+  for (RequestHeader& header : mAuthorRequestHeaders) {
+    if (header.name.Equals(lowercaseName)) {
+      // Gecko-specific: invalid headers can be set by privileged
+      //                 callers, but will not merge.
+      if (isPrivilegedCaller && isForbiddenHeader) {
+        header.value.Assign(value);
+      } else {
+        header.value.AppendLiteral(", ");
+        header.value.Append(value);
       }
-    }
-
-    if (!safeHeader) {
-      if (!mCORSUnsafeHeaders.Contains(header, nsCaseInsensitiveCStringArrayComparator())) {
-        mCORSUnsafeHeaders.AppendElement(header);
-      }
-    }
-  } else {
-    // Case 1 above
-    if (nsContentUtils::IsForbiddenSystemRequestHeader(header)) {
-      mergeHeaders = false;
+      notAlreadySet = false;
+      break;
     }
   }
 
-  if (!mAlreadySetHeaders.Contains(header)) {
-    // Case 2 above
-    mergeHeaders = false;
-  }
-
-  nsresult rv;
-  if (value.IsEmpty()) {
-    rv = httpChannel->SetEmptyRequestHeader(header);
-  } else {
-    // Merge headers depending on what we decided above.
-    rv = httpChannel->SetRequestHeader(header, value, mergeHeaders);
+  // Step 6.3
+  if (notAlreadySet) {
+    RequestHeader newHeader = {
+      nsCString(lowercaseName), nsCString(value)
+    };
+    mAuthorRequestHeaders.AppendElement(newHeader);
   }
-  if (rv == NS_ERROR_INVALID_ARG) {
-    return NS_ERROR_DOM_SYNTAX_ERR;
-  }
-  if (NS_SUCCEEDED(rv)) {
-    // Remember that we've set this header, so subsequent set operations will merge values.
-    mAlreadySetHeaders.PutEntry(nsCString(header));
-
-    // We'll want to duplicate this header for any replacement channels (eg. on redirect)
-    RequestHeader reqHeader = {
-      nsCString(header), nsCString(value)
-    };
-    mModifiedRequestHeaders.AppendElement(reqHeader);
-  }
-  return rv;
+
+  return NS_OK;
 }
 
 NS_IMETHODIMP
 XMLHttpRequestMainThread::GetTimeout(uint32_t *aTimeout)
 {
   *aTimeout = Timeout();
   return NS_OK;
 }
@@ -3174,37 +3157,55 @@ XMLHttpRequestMainThread::AsyncOnChannel
         mNewRedirectChannel = nullptr;
     }
     return rv;
   }
   OnRedirectVerifyCallback(NS_OK);
   return NS_OK;
 }
 
+void
+XMLHttpRequestMainThread::GetAuthorRequestHeaderValue(const char* aName,
+                                                      nsACString& outValue)
+{
+  for (RequestHeader& header : mAuthorRequestHeaders) {
+    if (header.name.Equals(aName)) {
+      outValue.Assign(header.value);
+      return;
+    }
+  }
+  outValue.SetIsVoid(true);
+}
+
+void
+XMLHttpRequestMainThread::SetAuthorRequestHeadersOnChannel(
+  nsCOMPtr<nsIHttpChannel> aHttpChannel)
+{
+  for (RequestHeader& header : mAuthorRequestHeaders) {
+    if (header.value.IsEmpty()) {
+      aHttpChannel->SetEmptyRequestHeader(header.name);
+    } else {
+      aHttpChannel->SetRequestHeader(header.name, header.value, false);
+    }
+  }
+}
+
 nsresult
 XMLHttpRequestMainThread::OnRedirectVerifyCallback(nsresult result)
 {
   NS_ASSERTION(mRedirectCallback, "mRedirectCallback not set in callback");
   NS_ASSERTION(mNewRedirectChannel, "mNewRedirectChannel not set in callback");
 
   if (NS_SUCCEEDED(result)) {
     mChannel = mNewRedirectChannel;
 
     nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel));
     if (httpChannel) {
       // Ensure all original headers are duplicated for the new channel (bug #553888)
-      for (RequestHeader& requestHeader : mModifiedRequestHeaders) {
-        if (requestHeader.value.IsEmpty()) {
-          httpChannel->SetEmptyRequestHeader(requestHeader.header);
-        } else {
-          httpChannel->SetRequestHeader(requestHeader.header,
-                                        requestHeader.value,
-                                        false);
-        }
-      }
+      SetAuthorRequestHeadersOnChannel(httpChannel);
     }
   } else {
     mErrorLoad = true;
   }
 
   mNewRedirectChannel = nullptr;
 
   mRedirectCallback->OnRedirectVerifyCallback(result);
@@ -3510,19 +3511,18 @@ XMLHttpRequestMainThread::EnsureXPCOMifi
 }
 
 bool
 XMLHttpRequestMainThread::ShouldBlockAuthPrompt()
 {
   // Verify that it's ok to prompt for credentials here, per spec
   // http://xhr.spec.whatwg.org/#the-send%28%29-method
 
-  for (uint32_t i = 0, len = mModifiedRequestHeaders.Length(); i < len; ++i) {
-    if (mModifiedRequestHeaders[i].header.
-          LowerCaseEqualsLiteral("authorization")) {
+  for (RequestHeader& requestHeader : mAuthorRequestHeaders) {
+    if (requestHeader.name.EqualsLiteral("authorization")) {
       return true;
     }
   }
 
   nsCOMPtr<nsIURI> uri;
   nsresult rv = mChannel->GetURI(getter_AddRefs(uri));
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return false;
--- a/dom/xhr/XMLHttpRequestMainThread.h
+++ b/dom/xhr/XMLHttpRequestMainThread.h
@@ -215,20 +215,20 @@ public:
        const Optional<nsAString>& aPassword,
        ErrorResult& aRv) override
   {
     aRv = Open(aMethod, NS_ConvertUTF16toUTF8(aUrl),
                aAsync, aUser, aPassword);
   }
 
   virtual void
-  SetRequestHeader(const nsACString& aHeader, const nsACString& aValue,
+  SetRequestHeader(const nsACString& aName, const nsACString& aValue,
                    ErrorResult& aRv) override
   {
-    aRv = SetRequestHeader(aHeader, aValue);
+    aRv = SetRequestHeader(aName, aValue);
   }
 
   virtual uint32_t
   Timeout() const override
   {
     return mTimeoutMilliseconds;
   }
 
@@ -615,17 +615,16 @@ protected:
                 const Optional<nsAString>& password);
 
   already_AddRefed<nsXMLHttpRequestXPCOMifier> EnsureXPCOMifier();
 
   nsCOMPtr<nsISupports> mContext;
   nsCOMPtr<nsIPrincipal> mPrincipal;
   nsCOMPtr<nsIChannel> mChannel;
   nsCOMPtr<nsIDocument> mResponseXML;
-  nsTArray<nsCString> mCORSUnsafeHeaders;
 
   nsCOMPtr<nsIStreamListener> mXMLParserStreamListener;
 
   // used to implement getAllResponseHeaders()
   class nsHeaderVisitor : public nsIHttpHeaderVisitor
   {
   public:
     NS_DECL_ISUPPORTS
@@ -786,22 +785,23 @@ protected:
   bool mIsMappedArrayBuffer;
 
   void ResetResponse();
 
   bool ShouldBlockAuthPrompt();
 
   struct RequestHeader
   {
-    nsCString header;
+    nsCString name;
     nsCString value;
   };
-  nsTArray<RequestHeader> mModifiedRequestHeaders;
+  nsTArray<RequestHeader> mAuthorRequestHeaders;
 
-  nsTHashtable<nsCStringHashKey> mAlreadySetHeaders;
+  void GetAuthorRequestHeaderValue(const char* aName, nsACString& outValue);
+  void SetAuthorRequestHeadersOnChannel(nsCOMPtr<nsIHttpChannel> aChannel);
 
   // Helper object to manage our XPCOM scriptability bits
   nsXMLHttpRequestXPCOMifier* mXPCOMifier;
 
   static bool sDontWarnAboutSyncXHR;
 };
 
 class MOZ_STACK_CLASS AutoDontWarnAboutSyncXHR
--- a/dom/xhr/tests/test_xhr_forbidden_headers.html
+++ b/dom/xhr/tests/test_xhr_forbidden_headers.html
@@ -23,17 +23,16 @@ var headers = [
   "aCCept-chaRset",
   "acCePt-eNcoDing",
   "aCcEsS-cOnTrOl-ReQuEsT-mEtHoD",
   "aCcEsS-cOnTrOl-ReQuEsT-hEaDeRs",
   "coNnEctIon",
   "coNtEnt-LEngth",
   "CoOKIe",
   "cOOkiE2",
-  "cOntEnt-tRAnsFer-enCoDiNg",
   "DATE",
   "dNT",
   "exPeCt",
   "hOSt",
   "keep-alive",
   "oRiGiN",
   "reFERer",
   "te",
@@ -49,16 +48,17 @@ var headers = [
 var i, request;
 
 function  startTest() {
   // Try setting headers in unprivileged context
   request = new XMLHttpRequest();
   request.open("GET", window.location.href);
   for (i = 0; i < headers.length; i++)
     request.setRequestHeader(headers[i], "test" + i);
+  request.send(); // headers aren't set on the channel until send()
 
   // Read out headers
   var channel = SpecialPowers.wrap(request).channel.QueryInterface(SpecialPowers.Ci.nsIHttpChannel);
   for (i = 0; i < headers.length; i++) {
     // Retrieving Content-Length will throw an exception
     var value = null;
     try {
       value = channel.getRequestHeader(headers[i]);
@@ -68,16 +68,17 @@ function  startTest() {
     isnot(value, "test" + i, "Setting " + headers[i] + " header in unprivileged context");
   }
 
   // Try setting headers in privileged context
   request = new XMLHttpRequest({mozAnon: true, mozSystem: true});
   request.open("GET", window.location.href);
   for (i = 0; i < headers.length; i++)
     request.setRequestHeader(headers[i], "test" + i);
+  request.send(); // headers aren't set on the channel until send()
 
   // Read out headers
   var channel = SpecialPowers.wrap(request).channel.QueryInterface(SpecialPowers.Ci.nsIHttpChannel);
   for (i = 0; i < headers.length; i++) {
     var value = channel.getRequestHeader(headers[i]);
     is(value, "test" + i, "Setting " + headers[i] + " header in privileged context");
   }
 
deleted file mode 100644
--- a/testing/web-platform/meta/XMLHttpRequest/setrequestheader-case-insensitive.htm.ini
+++ /dev/null
@@ -1,5 +0,0 @@
-[setrequestheader-case-insensitive.htm]
-  type: testharness
-  [XMLHttpRequest: setRequestHeader() - headers that differ in case]
-    expected: FAIL
-
deleted file mode 100644
--- a/testing/web-platform/meta/XMLHttpRequest/setrequestheader-header-allowed.htm.ini
+++ /dev/null
@@ -1,14 +0,0 @@
-[setrequestheader-header-allowed.htm]
-  type: testharness
-  [XMLHttpRequest: setRequestHeader() - headers that are allowed (Authorization)]
-    expected: FAIL
-
-  [XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Transfer-Encoding)]
-    expected: FAIL
-
-  [XMLHttpRequest: setRequestHeader() - headers that are allowed (Content-Type)]
-    expected: FAIL
-
-  [XMLHttpRequest: setRequestHeader() - headers that are allowed (User-Agent)]
-    expected: FAIL
-