Add UseCounter for set-cookie usage in req Headers

This commit adds a UseCounter that tracks if a site in any way sets a header with the name "set-cookie" on a Headers object with a guard of "request". This is done to verify if it is a web compatible change to add "set-cookie" to the request forbidden header list. See whatwg/fetch#973 (comment) Bug: 1292458 Change-Id: I1edd161d941d6490838b77a2fec008de904793ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3423793 Reviewed-by: Yutaka Hirano <[email protected]> Commit-Queue: Yutaka Hirano <[email protected]> Cr-Commit-Position: refs/heads/main@{#965654} NOKEYCHECK=True GitOrigin-RevId: 4fa1c0bf6242c7bbdf97d25426d0dea7c7aa644c
mjfroman · Feb 1, 2022 · a71fe95 · a71fe95
1 parent acb300a
commit a71fe95
Show file tree

Hide file tree

Showing 6 changed files with 82 additions and 34 deletions.
diff --git a/blink/public/mojom/web_feature/web_feature.mojom b/blink/public/mojom/web_feature/web_feature.mojom
@@ -3470,6 +3470,7 @@ enum WebFeature {
   kClientHintsUAFull = 4149,
   kPrivateNetworkAccessWithinWorker = 4150,
   kClientHintsUAWoW64 = 4151,
+  kFetchSetCookieInRequestGuardedHeaders = 4152,
 
   // Add new features immediately above this line. Don't change assigned
   // numbers of any item, and don't reuse removed slots.

diff --git a/blink/renderer/core/fetch/headers.cc b/blink/renderer/core/fetch/headers.cc
@@ -7,7 +7,10 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_iterator_result_value.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_union_bytestringbytestringrecord_bytestringsequencesequence.h"
 #include "third_party/blink/renderer/core/dom/iterator.h"
+#include "third_party/blink/renderer/core/execution_context/execution_context.h"
+#include "third_party/blink/renderer/core/frame/web_feature.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
+#include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
 #include "third_party/blink/renderer/platform/loader/cors/cors.h"
 #include "third_party/blink/renderer/platform/loader/fetch/fetch_utils.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
@@ -46,17 +49,18 @@ class HeadersIterationSource final
 
 }  // namespace
 
-Headers* Headers::Create(ExceptionState&) {
+Headers* Headers::Create(ScriptState* script_state, ExceptionState&) {
   return MakeGarbageCollected<Headers>();
 }
 
-Headers* Headers::Create(const V8HeadersInit* init,
+Headers* Headers::Create(ScriptState* script_state,
+                         const V8HeadersInit* init,
                          ExceptionState& exception_state) {
   // "The Headers(|init|) constructor, when invoked, must run these steps:"
   // "1. Let |headers| be a new Headers object whose guard is "none".
-  Headers* headers = Create(exception_state);
+  Headers* headers = Create(script_state, exception_state);
   // "2. If |init| is given, fill headers with |init|. Rethrow any exception."
-  headers->FillWith(init, exception_state);
+  headers->FillWith(script_state, init, exception_state);
   // "3. Return |headers|."
   return headers;
 }
@@ -72,7 +76,8 @@ Headers* Headers::Clone() const {
   return headers;
 }
 
-void Headers::append(const String& name,
+void Headers::append(ScriptState* script_state,
+                     const String& name,
                      const String& value,
                      ExceptionState& exception_state) {
   // "To append a name/value (|name|/|value|) pair to a Headers object
@@ -94,6 +99,12 @@ void Headers::append(const String& name,
     exception_state.ThrowTypeError("Headers are immutable");
     return;
   }
+  // UseCounter for usages of "set-cookie" in kRequestGuard'ed Headers.
+  if (guard_ == kRequestGuard && name.LowerASCII() == "set-cookie") {
+    ExecutionContext* execution_context = ExecutionContext::From(script_state);
+    UseCounter::Count(execution_context,
+                      WebFeature::kFetchSetCookieInRequestGuardedHeaders);
+  }
   // "4. Otherwise, if guard is |request| and |name| is a forbidden header
   //     name, return."
   if (guard_ == kRequestGuard && cors::IsForbiddenHeaderName(name))
@@ -133,7 +144,9 @@ void Headers::append(const String& name,
     RemovePrivilegedNoCorsRequestHeaders();
 }
 
-void Headers::remove(const String& name, ExceptionState& exception_state) {
+void Headers::remove(ScriptState* script_state,
+                     const String& name,
+                     ExceptionState& exception_state) {
   // "The delete(|name|) method, when invoked, must run these steps:"
   // "1. If name is not a name, throw a TypeError."
   if (!FetchHeaderList::IsValidHeaderName(name)) {
@@ -145,6 +158,12 @@ void Headers::remove(const String& name, ExceptionState& exception_state) {
     exception_state.ThrowTypeError("Headers are immutable");
     return;
   }
+  // UseCounter for usages of "set-cookie" in kRequestGuard'ed Headers.
+  if (guard_ == kRequestGuard && name.LowerASCII() == "set-cookie") {
+    ExecutionContext* execution_context = ExecutionContext::From(script_state);
+    UseCounter::Count(execution_context,
+                      WebFeature::kFetchSetCookieInRequestGuardedHeaders);
+  }
   // "3. Otherwise, if guard is |request| and |name| is a forbidden header
   //     name, return."
   if (guard_ == kRequestGuard && cors::IsForbiddenHeaderName(name))
@@ -201,7 +220,8 @@ bool Headers::has(const String& name, ExceptionState& exception_state) {
   return header_list_->Has(name);
 }
 
-void Headers::set(const String& name,
+void Headers::set(ScriptState* script_state,
+                  const String& name,
                   const String& value,
                   ExceptionState& exception_state) {
   // "The set(|name|, |value|) method, when invoked, must run these steps:"
@@ -222,6 +242,12 @@ void Headers::set(const String& name,
     exception_state.ThrowTypeError("Headers are immutable");
     return;
   }
+  // UseCounter for usages of "set-cookie" in kRequestGuard'ed Headers.
+  if (guard_ == kRequestGuard && name.LowerASCII() == "set-cookie") {
+    ExecutionContext* execution_context = ExecutionContext::From(script_state);
+    UseCounter::Count(execution_context,
+                      WebFeature::kFetchSetCookieInRequestGuardedHeaders);
+  }
   // "4. Otherwise, if guard is |request| and |name| is a forbidden header
   //     name, return."
   if (guard_ == kRequestGuard && cors::IsForbiddenHeaderName(name))
@@ -249,16 +275,19 @@ void Headers::set(const String& name,
 // This overload is not called directly by Web APIs, but rather by other C++
 // classes. For example, when initializing a Request object it is possible that
 // a Request's Headers must be filled with an existing Headers object.
-void Headers::FillWith(const Headers* object, ExceptionState& exception_state) {
+void Headers::FillWith(ScriptState* script_state,
+                       const Headers* object,
+                       ExceptionState& exception_state) {
   DCHECK_EQ(header_list_->size(), 0U);
   for (const auto& header : object->header_list_->List()) {
-    append(header.first, header.second, exception_state);
+    append(script_state, header.first, header.second, exception_state);
     if (exception_state.HadException())
       return;
   }
 }
 
-void Headers::FillWith(const V8HeadersInit* init,
+void Headers::FillWith(ScriptState* script_state,
+                       const V8HeadersInit* init,
                        ExceptionState& exception_state) {
   DCHECK_EQ(header_list_->size(), 0U);
 
@@ -267,15 +296,18 @@ void Headers::FillWith(const V8HeadersInit* init,
 
   switch (init->GetContentType()) {
     case V8HeadersInit::ContentType::kByteStringByteStringRecord:
-      return FillWith(init->GetAsByteStringByteStringRecord(), exception_state);
+      return FillWith(script_state, init->GetAsByteStringByteStringRecord(),
+                      exception_state);
     case V8HeadersInit::ContentType::kByteStringSequenceSequence:
-      return FillWith(init->GetAsByteStringSequenceSequence(), exception_state);
+      return FillWith(script_state, init->GetAsByteStringSequenceSequence(),
+                      exception_state);
   }
 
   NOTREACHED();
 }
 
-void Headers::FillWith(const Vector<Vector<String>>& object,
+void Headers::FillWith(ScriptState* script_state,
+                       const Vector<Vector<String>>& object,
                        ExceptionState& exception_state) {
   DCHECK(!header_list_->size());
   // "1. If |object| is a sequence, then for each |header| in |object|, run
@@ -289,18 +321,19 @@ void Headers::FillWith(const Vector<Vector<String>>& object,
       exception_state.ThrowTypeError("Invalid value");
       return;
     }
-    append(object[i][0], object[i][1], exception_state);
+    append(script_state, object[i][0], object[i][1], exception_state);
     if (exception_state.HadException())
       return;
   }
 }
 
-void Headers::FillWith(const Vector<std::pair<String, String>>& object,
+void Headers::FillWith(ScriptState* script_state,
+                       const Vector<std::pair<String, String>>& object,
                        ExceptionState& exception_state) {
   DCHECK(!header_list_->size());
 
   for (const auto& item : object) {
-    append(item.first, item.second, exception_state);
+    append(script_state, item.first, item.second, exception_state);
     if (exception_state.HadException())
       return;
   }

diff --git a/blink/renderer/core/fetch/headers.h b/blink/renderer/core/fetch/headers.h
@@ -35,8 +35,10 @@ class CORE_EXPORT Headers final
     kNoneGuard
   };
 
-  static Headers* Create(ExceptionState& exception_state);
-  static Headers* Create(const V8HeadersInit* init,
+  static Headers* Create(ScriptState* script_state,
+                         ExceptionState& exception_state);
+  static Headers* Create(ScriptState* script_state,
+                         const V8HeadersInit* init,
                          ExceptionState& exception_state);
 
   // Shares the FetchHeaderList. Called when creating a Request or Response.
@@ -49,18 +51,26 @@ class CORE_EXPORT Headers final
   Headers* Clone() const;
 
   // Headers.idl implementation.
-  void append(const String& name, const String& value, ExceptionState&);
-  void remove(const String& key, ExceptionState&);
+  void append(ScriptState* script_state,
+              const String& name,
+              const String& value,
+              ExceptionState&);
+  void remove(ScriptState* script_state, const String& key, ExceptionState&);
   String get(const String& key, ExceptionState&);
   bool has(const String& key, ExceptionState&);
-  void set(const String& key, const String& value, ExceptionState&);
+  void set(ScriptState* script_state,
+           const String& key,
+           const String& value,
+           ExceptionState&);
 
   void SetGuard(Guard guard) { guard_ = guard; }
   Guard GetGuard() const { return guard_; }
 
   // These methods should only be called when size() would return 0.
-  void FillWith(const Headers*, ExceptionState&);
-  void FillWith(const V8HeadersInit* init, ExceptionState& exception_state);
+  void FillWith(ScriptState* script_state, const Headers*, ExceptionState&);
+  void FillWith(ScriptState* script_state,
+                const V8HeadersInit* init,
+                ExceptionState& exception_state);
 
   // https://fetch.spec.whatwg.org/#concept-headers-remove-privileged-no-cors-request-headers
   void RemovePrivilegedNoCorsRequestHeaders();
@@ -70,8 +80,12 @@ class CORE_EXPORT Headers final
 
  private:
   // These methods should only be called when size() would return 0.
-  void FillWith(const Vector<Vector<String>>&, ExceptionState&);
-  void FillWith(const Vector<std::pair<String, String>>&, ExceptionState&);
+  void FillWith(ScriptState* script_state,
+                const Vector<Vector<String>>&,
+                ExceptionState&);
+  void FillWith(ScriptState* script_state,
+                const Vector<std::pair<String, String>>&,
+                ExceptionState&);
 
   Member<FetchHeaderList> header_list_;
   Guard guard_;

diff --git a/blink/renderer/core/fetch/headers.idl b/blink/renderer/core/fetch/headers.idl
@@ -11,11 +11,11 @@ typedef (sequence<sequence<ByteString>> or record<ByteString, ByteString>) Heade
 [
     Exposed=(Window,Worker)
 ] interface Headers {
-    [RaisesException] constructor(optional HeadersInit init);
-    [RaisesException] void append(ByteString name, ByteString value);
-    [ImplementedAs=remove, RaisesException] void delete(ByteString key);
+    [CallWith=ScriptState, RaisesException] constructor(optional HeadersInit init);
+    [CallWith=ScriptState, RaisesException] void append(ByteString name, ByteString value);
+    [CallWith=ScriptState, ImplementedAs=remove, RaisesException] void delete(ByteString key);
     [RaisesException] ByteString? get(ByteString key);
     [RaisesException] boolean has(ByteString key);
-    [RaisesException] void set(ByteString key, ByteString value);
+    [CallWith=ScriptState, RaisesException] void set(ByteString key, ByteString value);
     iterable<ByteString, ByteString>;
 };
diff --git a/blink/renderer/core/fetch/request.cc b/blink/renderer/core/fetch/request.cc
@@ -623,10 +623,10 @@ Request* Request::CreateRequestWithRequestOrString(
 
     // "Fill |r|'s Headers object with |headers|. Rethrow any exceptions."
     if (init->hasHeaders()) {
-      r->getHeaders()->FillWith(init->headers(), exception_state);
+      r->getHeaders()->FillWith(script_state, init->headers(), exception_state);
     } else {
       DCHECK(headers);
-      r->getHeaders()->FillWith(headers, exception_state);
+      r->getHeaders()->FillWith(script_state, headers, exception_state);
     }
     if (exception_state.HadException())
       return nullptr;
@@ -676,8 +676,8 @@ Request* Request::CreateRequestWithRequestOrString(
     //   `Content-Type`/|Content-Type| to |this|'s headers object.
     if (!content_type.IsEmpty() &&
         !r->getHeaders()->has(http_names::kContentType, exception_state)) {
-      r->getHeaders()->append(http_names::kContentType, content_type,
-                              exception_state);
+      r->getHeaders()->append(script_state, http_names::kContentType,
+                              content_type, exception_state);
     }
     if (exception_state.HadException())
       return nullptr;

diff --git a/blink/renderer/core/fetch/response.cc b/blink/renderer/core/fetch/response.cc
@@ -274,7 +274,7 @@ Response* Response::Create(ScriptState* script_state,
     r->response_->HeaderList()->ClearList();
     // "2. Fill |r|'s Headers object with |init|'s headers member. Rethrow
     // any exceptions."
-    r->headers_->FillWith(init->headers(), exception_state);
+    r->headers_->FillWith(script_state, init->headers(), exception_state);
     if (exception_state.HadException())
       return nullptr;
   }