LibWeb/Fetch: Refactor forbidden request-headers

This is a change in the Fetch spec.

See:
- https://github.com/whatwg/fetch/commit/92e6c91
- https://github.com/whatwg/xhr/commit/494431a

5 files changed, 125 insertions, 79 deletions

diff --git a/Userland/Libraries/LibWeb/Fetch/Headers.cpp b/Userland/Libraries/LibWeb/Fetch/Headers.cpp
index 5628023a01..5c097a9412 100644
--- a/Userland/Libraries/LibWeb/Fetch/Headers.cpp
+++ b/Userland/Libraries/LibWeb/Fetch/Headers.cpp
@@ -59,6 +59,7 @@ WebIDL::ExceptionOr<void> Headers::append(DeprecatedString const& name_string, D
 WebIDL::ExceptionOr<void> Headers::delete_(DeprecatedString const& name_string)
 {
     // The delete(name) method steps are:
+    auto& realm = this->realm();
     auto name = name_string.bytes();
 
     // 1. If name is not a header name, then throw a TypeError.
@@ -69,8 +70,10 @@ WebIDL::ExceptionOr<void> Headers::delete_(DeprecatedString const& name_string)
     if (m_guard == Guard::Immutable)
         return WebIDL::SimpleException { WebIDL::SimpleExceptionType::TypeError, "Headers object is immutable"sv };
 
-    // 3. Otherwise, if this’s guard is "request" and name is a forbidden header name, return.
-    if (m_guard == Guard::Request && Infrastructure::is_forbidden_header_name(name))
+    // 3. Otherwise, if this's guard is "request" and (name, ``) is a forbidden request-header, return.
+    // NOTE: Passing a dummy header value to forbidden request-header ought not to have any negative repercussions.
+    auto header = TRY_OR_RETURN_OOM(realm, Infrastructure::Header::from_string_pair(name, ""sv));
+    if (m_guard == Guard::Request && TRY_OR_RETURN_OOM(realm, Infrastructure::is_forbidden_request_header(header)))
         return {};
 
     // 4. Otherwise, if this’s guard is "request-no-cors", name is not a no-CORS-safelisted request-header name, and name is not a privileged no-CORS request-header name, return.
@@ -88,9 +91,9 @@ WebIDL::ExceptionOr<void> Headers::delete_(DeprecatedString const& name_string)
     // 7. Delete name from this’s header list.
     m_header_list->delete_(name);
 
-    // 8. If this’s guard is "request-no-cors", then remove privileged no-CORS request headers from this.
+    // 8. If this’s guard is "request-no-cors", then remove privileged no-CORS request-headers from this.
     if (m_guard == Guard::RequestNoCORS)
-        remove_privileged_no_cors_headers();
+        remove_privileged_no_cors_request_headers();
 
     return {};
 }
@@ -129,14 +132,15 @@ WebIDL::ExceptionOr<bool> Headers::has(DeprecatedString const& name_string)
 WebIDL::ExceptionOr<void> Headers::set(DeprecatedString const& name_string, DeprecatedString const& value_string)
 {
     // The set(name, value) method steps are:
+    auto& realm = this->realm();
     auto name = name_string.bytes();
     auto value = value_string.bytes();
 
     // 1. Normalize value.
-    auto normalized_value = TRY_OR_RETURN_OOM(realm(), Infrastructure::normalize_header_value(value));
+    auto normalized_value = TRY_OR_RETURN_OOM(realm, Infrastructure::normalize_header_value(value));
 
     auto header = Infrastructure::Header {
-        .name = TRY_OR_RETURN_OOM(realm(), ByteBuffer::copy(name)),
+        .name = TRY_OR_RETURN_OOM(realm, ByteBuffer::copy(name)),
         .value = move(normalized_value),
     };
 
@@ -150,8 +154,8 @@ WebIDL::ExceptionOr<void> Headers::set(DeprecatedString const& name_string, Depr
     if (m_guard == Guard::Immutable)
         return WebIDL::SimpleException { WebIDL::SimpleExceptionType::TypeError, "Headers object is immutable"sv };
 
-    // 4. Otherwise, if this’s guard is "request" and name is a forbidden header name, return.
-    if (m_guard == Guard::Request && Infrastructure::is_forbidden_header_name(name))
+    // 4. Otherwise, if this’s guard is "request" and (name, value) is a forbidden request-header, return.
+    if (m_guard == Guard::Request && TRY_OR_RETURN_OOM(realm, Infrastructure::is_forbidden_request_header(header)))
         return {};
 
     // 5. Otherwise, if this’s guard is "request-no-cors" and (name, value) is not a no-CORS-safelisted request-header, return.
@@ -163,11 +167,11 @@ WebIDL::ExceptionOr<void> Headers::set(DeprecatedString const& name_string, Depr
         return {};
 
     // 7. Set (name, value) in this’s header list.
-    TRY_OR_RETURN_OOM(realm(), m_header_list->set(move(header)));
+    TRY_OR_RETURN_OOM(realm, m_header_list->set(move(header)));
 
-    // 8. If this’s guard is "request-no-cors", then remove privileged no-CORS request headers from this.
+    // 8. If this’s guard is "request-no-cors", then remove privileged no-CORS request-headers from this.
     if (m_guard == Guard::RequestNoCORS)
-        remove_privileged_no_cors_headers();
+        remove_privileged_no_cors_request_headers();
 
     return {};
 }
@@ -213,10 +217,11 @@ JS::ThrowCompletionOr<void> Headers::for_each(ForEachCallback callback)
 WebIDL::ExceptionOr<void> Headers::append(Infrastructure::Header header)
 {
     // To append a header (name, value) to a Headers object headers, run these steps:
+    auto& realm = this->realm();
     auto& [name, value] = header;
 
     // 1. Normalize value.
-    value = TRY_OR_RETURN_OOM(realm(), Infrastructure::normalize_header_value(value));
+    value = TRY_OR_RETURN_OOM(realm, Infrastructure::normalize_header_value(value));
 
     // 2. If name is not a header name or value is not a header value, then throw a TypeError.
     if (!Infrastructure::is_header_name(name))
@@ -228,28 +233,28 @@ WebIDL::ExceptionOr<void> Headers::append(Infrastructure::Header header)
     if (m_guard == Guard::Immutable)
         return WebIDL::SimpleException { WebIDL::SimpleExceptionType::TypeError, "Headers object is immutable"sv };
 
-    // 4. Otherwise, if headers’s guard is "request" and name is a forbidden header name, return.
-    if (m_guard == Guard::Request && Infrastructure::is_forbidden_header_name(name))
+    // 4. Otherwise, if headers’s guard is "request" and (name, value) is a forbidden request-header, return.
+    if (m_guard == Guard::Request && TRY_OR_RETURN_OOM(realm, Infrastructure::is_forbidden_request_header(header)))
         return {};
 
     // 5. Otherwise, if headers’s guard is "request-no-cors":
     if (m_guard == Guard::RequestNoCORS) {
         // 1. Let temporaryValue be the result of getting name from headers’s header list.
-        auto temporary_value = TRY_OR_RETURN_OOM(realm(), m_header_list->get(name));
+        auto temporary_value = TRY_OR_RETURN_OOM(realm, m_header_list->get(name));
 
         // 2. If temporaryValue is null, then set temporaryValue to value.
         if (!temporary_value.has_value()) {
-            temporary_value = TRY_OR_RETURN_OOM(realm(), ByteBuffer::copy(value));
+            temporary_value = TRY_OR_RETURN_OOM(realm, ByteBuffer::copy(value));
         }
         // 3. Otherwise, set temporaryValue to temporaryValue, followed by 0x2C 0x20, followed by value.
         else {
-            TRY_OR_RETURN_OOM(realm(), temporary_value->try_append(0x2c));
-            TRY_OR_RETURN_OOM(realm(), temporary_value->try_append(0x20));
-            TRY_OR_RETURN_OOM(realm(), temporary_value->try_append(value));
+            TRY_OR_RETURN_OOM(realm, temporary_value->try_append(0x2c));
+            TRY_OR_RETURN_OOM(realm, temporary_value->try_append(0x20));
+            TRY_OR_RETURN_OOM(realm, temporary_value->try_append(value));
         }
 
         auto temporary_header = Infrastructure::Header {
-            .name = TRY_OR_RETURN_OOM(realm(), ByteBuffer::copy(name)),
+            .name = TRY_OR_RETURN_OOM(realm, ByteBuffer::copy(name)),
             .value = temporary_value.release_value(),
         };
 
@@ -263,11 +268,11 @@ WebIDL::ExceptionOr<void> Headers::append(Infrastructure::Header header)
         return {};
 
     // 7. Append (name, value) to headers’s header list.
-    TRY_OR_RETURN_OOM(realm(), m_header_list->append(move(header)));
+    TRY_OR_RETURN_OOM(realm, m_header_list->append(move(header)));
 
-    // 8. If headers’s guard is "request-no-cors", then remove privileged no-CORS request headers from headers.
+    // 8. If headers’s guard is "request-no-cors", then remove privileged no-CORS request-headers from headers.
     if (m_guard == Guard::RequestNoCORS)
-        remove_privileged_no_cors_headers();
+        remove_privileged_no_cors_request_headers();
 
     return {};
 }
@@ -301,9 +306,9 @@ WebIDL::ExceptionOr<void> Headers::fill(HeadersInit const& object)
 }
 
 // https://fetch.spec.whatwg.org/#concept-headers-remove-privileged-no-cors-request-headers
-void Headers::remove_privileged_no_cors_headers()
+void Headers::remove_privileged_no_cors_request_headers()
 {
-    // To remove privileged no-CORS request headers from a Headers object (headers), run these steps:
+    // To remove privileged no-CORS request-headers from a Headers object (headers), run these steps:
 
     static constexpr Array privileged_no_cors_request_header_names = {
         "Range"sv,
diff --git a/Userland/Libraries/LibWeb/Fetch/Headers.h b/Userland/Libraries/LibWeb/Fetch/Headers.h
index 85cfca10df..909a9c2b26 100644
--- a/Userland/Libraries/LibWeb/Fetch/Headers.h
+++ b/Userland/Libraries/LibWeb/Fetch/Headers.h
@@ -62,7 +62,7 @@ private:
 
     virtual void visit_edges(JS::Cell::Visitor&) override;
 
-    void remove_privileged_no_cors_headers();
+    void remove_privileged_no_cors_request_headers();
 
     // https://fetch.spec.whatwg.org/#concept-headers-header-list
     // A Headers object has an associated header list (a header list), which is initially empty.
diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Headers.cpp b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Headers.cpp
index f54ade2d7a..127e6c548e 100644
--- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Headers.cpp
+++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Headers.cpp
@@ -16,6 +16,7 @@
 #include <LibRegex/Regex.h>
 #include <LibWeb/Fetch/Infrastructure/HTTP.h>
 #include <LibWeb/Fetch/Infrastructure/HTTP/Headers.h>
+#include <LibWeb/Fetch/Infrastructure/HTTP/Methods.h>
 #include <LibWeb/Infra/ByteSequences.h>
 #include <LibWeb/Loader/ResourceLoader.h>
 #include <LibWeb/MimeSniff/MimeType.h>
@@ -90,37 +91,46 @@ ErrorOr<Optional<Vector<DeprecatedString>>> HeaderList::get_decode_and_split(Rea
 {
     // To get, decode, and split a header name name from header list list, run these steps:
 
-    // 1. Let initialValue be the result of getting name from list.
-    auto initial_value = TRY(get(name));
+    // 1. Let value be the result of getting name from list.
+    auto value = TRY(get(name));
 
-    // 2. If initialValue is null, then return null.
-    if (!initial_value.has_value())
+    // 2. If value is null, then return null.
+    if (!value.has_value())
         return Optional<Vector<DeprecatedString>> {};
 
-    // 3. Let input be the result of isomorphic decoding initialValue.
-    auto input = StringView { *initial_value };
+    // 3. Return the result of getting, decoding, and splitting value.
+    return get_decode_and_split_header_value(*value);
+}
+
+// https://fetch.spec.whatwg.org/#header-value-get-decode-and-split
+ErrorOr<Optional<Vector<DeprecatedString>>> get_decode_and_split_header_value(ReadonlyBytes value)
+{
+    // To get, decode, and split a header value value, run these steps:
+
+    // 1. Let input be the result of isomorphic decoding value.
+    auto input = StringView { value };
 
-    // 4. Let position be a position variable for input, initially pointing at the start of input.
+    // 2. Let position be a position variable for input, initially pointing at the start of input.
     auto lexer = GenericLexer { input };
 
-    // 5. Let values be a list of strings, initially empty.
+    // 3. Let values be a list of strings, initially empty.
     Vector<DeprecatedString> values;
 
-    // 6. Let value be the empty string.
-    StringBuilder value_builder;
+    // 4. Let temporaryValue be the empty string.
+    StringBuilder temporary_value_builder;
 
-    // 7. While position is not past the end of input:
+    // 5. While position is not past the end of input:
     while (!lexer.is_eof()) {
-        // 1. Append the result of collecting a sequence of code points that are not U+0022 (") or U+002C (,) from input, given position, to value.
+        // 1. Append the result of collecting a sequence of code points that are not U+0022 (") or U+002C (,) from input, given position, to temporaryValue.
         // NOTE: The result might be the empty string.
-        value_builder.append(lexer.consume_until(is_any_of("\","sv)));
+        temporary_value_builder.append(lexer.consume_until(is_any_of("\","sv)));
 
         // 2. If position is not past the end of input, then:
         if (!lexer.is_eof()) {
             // 1. If the code point at position within input is U+0022 ("), then:
             if (lexer.peek() == '"') {
-                // 1. Append the result of collecting an HTTP quoted string from input, given position, to value.
-                value_builder.append(collect_an_http_quoted_string(lexer));
+                // 1. Append the result of collecting an HTTP quoted string from input, given position, to temporaryValue.
+                temporary_value_builder.append(collect_an_http_quoted_string(lexer));
 
                 // 2. If position is not past the end of input, then continue.
                 if (!lexer.is_eof())
@@ -136,14 +146,14 @@ ErrorOr<Optional<Vector<DeprecatedString>>> HeaderList::get_decode_and_split(Rea
             }
         }
 
-        // 3. Remove all HTTP tab or space from the start and end of value.
-        auto value = value_builder.build().trim(HTTP_TAB_OR_SPACE, TrimMode::Both);
+        // 3. Remove all HTTP tab or space from the start and end of temporaryValue.
+        auto temporary_value = temporary_value_builder.build().trim(HTTP_TAB_OR_SPACE, TrimMode::Both);
 
-        // 4. Append value to values.
-        values.append(move(value));
+        // 4. Append temporaryValue to values.
+        values.append(move(temporary_value));
 
-        // 5. Set value to the empty string.
-        value_builder.clear();
+        // 5. Set temporaryValue to the empty string.
+        temporary_value_builder.clear();
     }
 
     // 8. Return values.
@@ -580,34 +590,63 @@ bool is_no_cors_safelisted_request_header(Header const& header)
 }
 
 // https://fetch.spec.whatwg.org/#forbidden-header-name
-bool is_forbidden_header_name(ReadonlyBytes header_name)
+ErrorOr<bool> is_forbidden_request_header(Header const& header)
 {
-    // A forbidden header name is a header name that is a byte-case-insensitive match for one of
+    // A header (name, value) is forbidden request-header if these steps return true:
+    auto name = StringView { header.name };
+
+    // 1. If name is a byte-case-insensitive match for one of:
     // [...]
-    // or a header name that when byte-lowercased starts with `proxy-` or `sec-`.
-    return StringView { header_name }.is_one_of_ignoring_case(
-               "Accept-Charset"sv,
-               "Accept-Encoding"sv,
-               "Access-Control-Request-Headers"sv,
-               "Access-Control-Request-Method"sv,
-               "Connection"sv,
-               "Content-Length"sv,
-               "Cookie"sv,
-               "Cookie2"sv,
-               "Date"sv,
-               "DNT"sv,
-               "Expect"sv,
-               "Host"sv,
-               "Keep-Alive"sv,
-               "Origin"sv,
-               "Referer"sv,
-               "TE"sv,
-               "Trailer"sv,
-               "Transfer-Encoding"sv,
-               "Upgrade"sv,
-               "Via"sv)
-        || StringView { header_name }.starts_with("proxy-"sv, CaseSensitivity::CaseInsensitive)
-        || StringView { header_name }.starts_with("sec-"sv, CaseSensitivity::CaseInsensitive);
+    // then return true.
+    if (name.is_one_of_ignoring_case(
+            "Accept-Charset"sv,
+            "Accept-Encoding"sv,
+            "Access-Control-Request-Headers"sv,
+            "Access-Control-Request-Method"sv,
+            "Connection"sv,
+            "Content-Length"sv,
+            "Cookie"sv,
+            "Cookie2"sv,
+            "Date"sv,
+            "DNT"sv,
+            "Expect"sv,
+            "Host"sv,
+            "Keep-Alive"sv,
+            "Origin"sv,
+            "Referer"sv,
+            "TE"sv,
+            "Trailer"sv,
+            "Transfer-Encoding"sv,
+            "Upgrade"sv,
+            "Via"sv)) {
+        return true;
+    }
+
+    // 2. If name when byte-lowercased starts with `proxy-` or `sec-`, then return true.
+    if (name.starts_with("proxy-"sv, CaseSensitivity::CaseInsensitive)
+        || name.starts_with("sec-"sv, CaseSensitivity::CaseInsensitive)) {
+        return true;
+    }
+
+    // 3. If name is a byte-case-insensitive match for one of:
+    // - `X-HTTP-Method`
+    // - `X-HTTP-Method-Override`
+    // - `X-Method-Override`
+    // then:
+    if (name.is_one_of_ignoring_case(
+            "X-HTTP-Method"sv,
+            "X-HTTP-Method-Override"sv,
+            "X-Method"sv)) {
+        // 1. Let parsedValues be the result of getting, decoding, and splitting value.
+        auto parsed_values = TRY(get_decode_and_split_header_value(header.value));
+
+        // 2. For each method in parsedValues: if the isomorphic encoding of method is a forbidden method, then return true.
+        if (parsed_values.has_value() && any_of(*parsed_values, [](auto method) { return is_forbidden_method(method.bytes()); }))
+            return true;
+    }
+
+    // 4. Return false.
+    return false;
 }
 
 // https://fetch.spec.whatwg.org/#forbidden-response-header-name
diff --git a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Headers.h b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Headers.h
index fbb935ea01..4cc096fde6 100644
--- a/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Headers.h
+++ b/Userland/Libraries/LibWeb/Fetch/Infrastructure/HTTP/Headers.h
@@ -58,6 +58,7 @@ struct RangeHeaderValue {
     Optional<u64> end;
 };
 
+[[nodiscard]] ErrorOr<Optional<Vector<DeprecatedString>>> get_decode_and_split_header_value(ReadonlyBytes);
 [[nodiscard]] ErrorOr<OrderedHashTable<ByteBuffer>> convert_header_names_to_a_sorted_lowercase_set(Span<ReadonlyBytes>);
 [[nodiscard]] bool is_header_name(ReadonlyBytes);
 [[nodiscard]] bool is_header_value(ReadonlyBytes);
@@ -70,7 +71,7 @@ struct RangeHeaderValue {
 [[nodiscard]] bool is_cors_safelisted_response_header_name(ReadonlyBytes, Span<ReadonlyBytes>);
 [[nodiscard]] bool is_no_cors_safelisted_request_header_name(ReadonlyBytes);
 [[nodiscard]] bool is_no_cors_safelisted_request_header(Header const&);
-[[nodiscard]] bool is_forbidden_header_name(ReadonlyBytes);
+[[nodiscard]] ErrorOr<bool> is_forbidden_request_header(Header const&);
 [[nodiscard]] bool is_forbidden_response_header_name(ReadonlyBytes);
 [[nodiscard]] bool is_request_body_header_name(ReadonlyBytes);
 [[nodiscard]] ErrorOr<Optional<Vector<ByteBuffer>>> extract_header_values(Header const&);
diff --git a/Userland/Libraries/LibWeb/XHR/XMLHttpRequest.cpp b/Userland/Libraries/LibWeb/XHR/XMLHttpRequest.cpp
index 4d9a9f5138..50f11db242 100644
--- a/Userland/Libraries/LibWeb/XHR/XMLHttpRequest.cpp
+++ b/Userland/Libraries/LibWeb/XHR/XMLHttpRequest.cpp
@@ -310,15 +310,16 @@ WebIDL::ExceptionOr<void> XMLHttpRequest::set_request_header(DeprecatedString co
     if (!Fetch::Infrastructure::is_header_value(value))
         return WebIDL::SyntaxError::create(realm, "Header value contains invalid characters.");
 
-    // 5. If name is a forbidden header name, then return.
-    if (Fetch::Infrastructure::is_forbidden_header_name(name))
-        return {};
-
-    // 6. Combine (name, value) in this’s author request headers.
     auto header = Fetch::Infrastructure::Header {
         .name = move(name),
         .value = move(value),
     };
+
+    // 5. If (name, value) is a forbidden request-header, then return.
+    if (TRY_OR_RETURN_OOM(realm, Fetch::Infrastructure::is_forbidden_request_header(header)))
+        return {};
+
+    // 6. Combine (name, value) in this’s author request headers.
     TRY_OR_RETURN_OOM(realm, m_author_request_headers->combine(move(header)));
 
     return {};