From a71fe9554715842ecbb17742dff2df971a45925f Mon Sep 17 00:00:00 2001 From: Luca Casonato Date: Tue, 1 Feb 2022 13:22:53 +0000 Subject: [PATCH] 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 https://github.com/whatwg/fetch/issues/973#issuecomment-961727925 Bug: 1292458 Change-Id: I1edd161d941d6490838b77a2fec008de904793ad Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3423793 Reviewed-by: Yutaka Hirano Commit-Queue: Yutaka Hirano Cr-Commit-Position: refs/heads/main@{#965654} NOKEYCHECK=True GitOrigin-RevId: 4fa1c0bf6242c7bbdf97d25426d0dea7c7aa644c --- .../mojom/web_feature/web_feature.mojom | 1 + blink/renderer/core/fetch/headers.cc | 65 ++++++++++++++----- blink/renderer/core/fetch/headers.h | 32 ++++++--- blink/renderer/core/fetch/headers.idl | 8 +-- blink/renderer/core/fetch/request.cc | 8 +-- blink/renderer/core/fetch/response.cc | 2 +- 6 files changed, 82 insertions(+), 34 deletions(-) diff --git a/blink/public/mojom/web_feature/web_feature.mojom b/blink/public/mojom/web_feature/web_feature.mojom index e64e1932c626..b79a6eda759e 100644 --- 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 index 9b6c76500e70..3f4291baad29 100644 --- 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::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>& object, +void Headers::FillWith(ScriptState* script_state, + const Vector>& 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>& 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>& object, +void Headers::FillWith(ScriptState* script_state, + const Vector>& 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 index 361ecb46c36a..03f25698ea85 100644 --- 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>&, ExceptionState&); - void FillWith(const Vector>&, ExceptionState&); + void FillWith(ScriptState* script_state, + const Vector>&, + ExceptionState&); + void FillWith(ScriptState* script_state, + const Vector>&, + ExceptionState&); Member header_list_; Guard guard_; diff --git a/blink/renderer/core/fetch/headers.idl b/blink/renderer/core/fetch/headers.idl index 5b0388d8cc7d..eaaa053a904b 100644 --- a/blink/renderer/core/fetch/headers.idl +++ b/blink/renderer/core/fetch/headers.idl @@ -11,11 +11,11 @@ typedef (sequence> or record) 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; }; diff --git a/blink/renderer/core/fetch/request.cc b/blink/renderer/core/fetch/request.cc index 04ca29e991fe..77870cd39f55 100644 --- 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 index f2433442bece..ce01cb90f4e7 100644 --- 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; }