From 159520c26a59b55a96448ca4966b31be5165b977 Mon Sep 17 00:00:00 2001 From: Dan Robertson Date: Mon, 10 Jul 2023 12:03:29 +0000 Subject: [PATCH] Bug 1733981: Allow single range header values without preflight. r=twisniewski,necko-reviewers,kershaw The Range header was added as a safe-listed header as long as the value is in a particular format. Update IsCORSSafelistedRequestHeader implementations to account for this. Differential Revision: https://phabricator.services.mozilla.com/D182861 --- dom/base/nsContentUtils.cpp | 89 ++++++++++++++++++- dom/base/nsContentUtils.h | 34 +++++++ dom/base/test/gtest/TestContentUtils.cpp | 75 ++++++++++++++++ dom/fetch/InternalHeaders.cpp | 14 +-- .../cors-safelisted-request-header.any.js.ini | 18 ---- .../meta/fetch/range/general.any.js.ini | 21 ----- 6 files changed, 204 insertions(+), 47 deletions(-) delete mode 100644 testing/web-platform/meta/cors/cors-safelisted-request-header.any.js.ini diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp index 01d1b57cf349..bbaf8a610f69 100644 --- a/dom/base/nsContentUtils.cpp +++ b/dom/base/nsContentUtils.cpp @@ -212,6 +212,7 @@ #include "mozilla/ipc/ProtocolUtils.h" #include "mozilla/ipc/SharedMemory.h" #include "mozilla/net/UrlClassifierCommon.h" +#include "mozilla/Tokenizer.h" #include "mozilla/widget/IMEData.h" #include "nsAboutProtocolUtils.h" #include "nsAlgorithm.h" @@ -7510,6 +7511,77 @@ bool nsContentUtils::ContainsForbiddenMethod(const nsACString& headerValue) { return hasInsecureMethod; } +Maybe nsContentUtils::ParseSingleRangeRequest( + const nsACString& aHeaderValue, bool aAllowWhitespace) { + // See https://fetch.spec.whatwg.org/#simple-range-header-value + mozilla::Tokenizer p(aHeaderValue); + Maybe rangeStart; + Maybe rangeEnd; + + // Step 2 and 3 + if (!p.CheckWord("bytes")) { + return Nothing(); + } + + // Step 4 + if (aAllowWhitespace) { + p.SkipWhites(); + } + + // Step 5 and 6 + if (!p.CheckChar('=')) { + return Nothing(); + } + + // Step 7 + if (aAllowWhitespace) { + p.SkipWhites(); + } + + // Step 8 and 9 + int32_t res; + if (p.ReadInteger(&res)) { + rangeStart = Some(res); + } + + // Step 10 + if (aAllowWhitespace) { + p.SkipWhites(); + } + + // Step 11 + if (!p.CheckChar('-')) { + return Nothing(); + } + + // Step 13 + if (aAllowWhitespace) { + p.SkipWhites(); + } + + // Step 14 and 15 + if (p.ReadInteger(&res)) { + rangeEnd = Some(res); + } + + // Step 16 + if (!p.CheckEOF()) { + return Nothing(); + } + + // Step 17 + if (!rangeStart && !rangeEnd) { + return Nothing(); + } + + // Step 18 + if (rangeStart && rangeEnd && *rangeStart > *rangeEnd) { + return Nothing(); + } + + return Some(ParsedRange(rangeStart, rangeEnd)); +} + // static bool nsContentUtils::IsCorsUnsafeRequestHeaderValue( const nsACString& aHeaderValue) { @@ -7579,6 +7651,19 @@ bool nsContentUtils::IsAllowedNonCorsLanguage(const nsACString& aHeaderValue) { return true; } +bool nsContentUtils::IsAllowedNonCorsRange(const nsACString& aHeaderValue) { + Maybe parsedRange = ParseSingleRangeRequest(aHeaderValue, false); + if (!parsedRange) { + return false; + } + + if (!parsedRange->Start()) { + return false; + } + + return true; +} + // static bool nsContentUtils::IsCORSSafelistedRequestHeader(const nsACString& aName, const nsACString& aValue) { @@ -7593,7 +7678,9 @@ bool nsContentUtils::IsCORSSafelistedRequestHeader(const nsACString& aName, (aName.LowerCaseEqualsLiteral("content-language") && nsContentUtils::IsAllowedNonCorsLanguage(aValue)) || (aName.LowerCaseEqualsLiteral("content-type") && - nsContentUtils::IsAllowedNonCorsContentType(aValue)); + nsContentUtils::IsAllowedNonCorsContentType(aValue)) || + (aName.LowerCaseEqualsLiteral("range") && + nsContentUtils::IsAllowedNonCorsRange(aValue)); } mozilla::LogModule* nsContentUtils::ResistFingerprintingLog() { diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h index 458e8f6f32a8..f573eb6afc3c 100644 --- a/dom/base/nsContentUtils.h +++ b/dom/base/nsContentUtils.h @@ -2769,6 +2769,34 @@ class nsContentUtils { * Checks whether the header value contains any forbidden method */ static bool ContainsForbiddenMethod(const nsACString& headerValue); + + class ParsedRange { + public: + explicit ParsedRange(mozilla::Maybe aStart, + mozilla::Maybe aEnd) + : mStart(aStart), mEnd(aEnd) {} + + mozilla::Maybe Start() const { return mStart; } + mozilla::Maybe End() const { return mEnd; } + + bool operator==(const ParsedRange& aOther) const { + return Start() == aOther.Start() && End() == aOther.End(); + } + + private: + mozilla::Maybe mStart; + mozilla::Maybe mEnd; + }; + + /** + * Parse a single range request and return a pair containing the resulting + * start and end of the range. + * + * See https://fetch.spec.whatwg.org/#simple-range-header-value + */ + static mozilla::Maybe ParseSingleRangeRequest( + const nsACString& aHeaderValue, bool aAllowWhitespace); + /** * Returns whether a given header has characters that aren't permitted */ @@ -2792,6 +2820,12 @@ class nsContentUtils { */ static bool IsAllowedNonCorsLanguage(const nsACString& aHeaderValue); + /** + * Returns whether a given Range header value is allowed for a non-CORS XHR or + * fetch request. + */ + static bool IsAllowedNonCorsRange(const nsACString& aHeaderValue); + /** * Returns whether a given header and value is a CORS-safelisted request * header per https://fetch.spec.whatwg.org/#cors-safelisted-request-header diff --git a/dom/base/test/gtest/TestContentUtils.cpp b/dom/base/test/gtest/TestContentUtils.cpp index d38cb1bdcacb..2a2d2b46001e 100644 --- a/dom/base/test/gtest/TestContentUtils.cpp +++ b/dom/base/test/gtest/TestContentUtils.cpp @@ -21,6 +21,21 @@ struct IsURIInListMatch { bool firstMatch, secondMatch; }; +std::ostream& operator<<(std::ostream& aStream, + const nsContentUtils::ParsedRange& aParsedRange) { + if (aParsedRange.Start()) { + aStream << *aParsedRange.Start(); + } + + aStream << "-"; + + if (aParsedRange.End()) { + aStream << *aParsedRange.End(); + } + + return aStream; +} + TEST(DOM_Base_ContentUtils, IsURIInList) { nsCOMPtr uri, subURI; @@ -144,3 +159,63 @@ TEST(DOM_Base_ContentUtils, StringifyJSON_Object_UndefinedIsVoidString) ASSERT_TRUE(serializedValue.EqualsLiteral("{\"key1\":\"Hello World!\"}")); } + +TEST(DOM_Base_ContentUtils, ParseSingleRangeHeader) +{ + // Parsing a simple range should succeed + EXPECT_EQ(nsContentUtils::ParseSingleRangeRequest("bytes=0-42"_ns, false), + mozilla::Some(nsContentUtils::ParsedRange(mozilla::Some(0), + mozilla::Some(42)))); + + // Range containing a invalid rangeStart should fail + EXPECT_EQ(nsContentUtils::ParseSingleRangeRequest("bytes= t-200"_ns, true), + mozilla::Nothing()); + + // Range containing whitespace, with allowWhitespace=false should fail. + EXPECT_EQ(nsContentUtils::ParseSingleRangeRequest("bytes= 2-200"_ns, false), + mozilla::Nothing()); + + // Range containing whitespace, with allowWhitespace=true should succeed + EXPECT_EQ( + nsContentUtils::ParseSingleRangeRequest("bytes \t= 2 - 200"_ns, true), + mozilla::Some( + nsContentUtils::ParsedRange(mozilla::Some(2), mozilla::Some(200)))); + + // Range containing invalid whitespace should fail + EXPECT_EQ( + nsContentUtils::ParseSingleRangeRequest("bytes \r= 2 - 200"_ns, true), + mozilla::Nothing()); + + // Range without a rangeStart should succeed + EXPECT_EQ(nsContentUtils::ParseSingleRangeRequest("bytes\t=\t-200"_ns, true), + mozilla::Some(nsContentUtils::ParsedRange(mozilla::Nothing(), + mozilla::Some(200)))); + + // Range without a rangeEnd should succeed + EXPECT_EQ(nsContentUtils::ParseSingleRangeRequest("bytes=55-"_ns, true), + mozilla::Some(nsContentUtils::ParsedRange(mozilla::Some(55), + mozilla::Nothing()))); + + // Range without a rangeStart or rangeEnd should fail + EXPECT_EQ(nsContentUtils::ParseSingleRangeRequest("bytes\t=\t-"_ns, true), + mozilla::Nothing()); + + // Range with extra characters should fail + EXPECT_EQ(nsContentUtils::ParseSingleRangeRequest("bytes=0-42 "_ns, true), + mozilla::Nothing()); + + // Range with rangeStart > rangeEnd should fail + EXPECT_EQ(nsContentUtils::ParseSingleRangeRequest("bytes=42-0 "_ns, true), + mozilla::Nothing()); +} + +TEST(DOM_Base_ContentUtils, IsAllowedNonCorsRange) +{ + EXPECT_EQ(nsContentUtils::IsAllowedNonCorsRange("bytes=-200"_ns), false); + EXPECT_EQ(nsContentUtils::IsAllowedNonCorsRange("bytes= 200-"_ns), false); + EXPECT_EQ(nsContentUtils::IsAllowedNonCorsRange("bytes=201-200"_ns), false); + EXPECT_EQ(nsContentUtils::IsAllowedNonCorsRange("bytes=200-201 "_ns), false); + EXPECT_EQ(nsContentUtils::IsAllowedNonCorsRange("bytes=200-"_ns), true); + EXPECT_EQ(nsContentUtils::IsAllowedNonCorsRange("bytes=200-201"_ns), true); + EXPECT_EQ(nsContentUtils::IsAllowedNonCorsRange("bytes=-200 "_ns), false); +} diff --git a/dom/fetch/InternalHeaders.cpp b/dom/fetch/InternalHeaders.cpp index af24432b62a2..89d4ab88d3af 100644 --- a/dom/fetch/InternalHeaders.cpp +++ b/dom/fetch/InternalHeaders.cpp @@ -148,6 +148,7 @@ bool InternalHeaders::DeleteInternal(const nsCString& aLowerName, } void InternalHeaders::Delete(const nsACString& aName, ErrorResult& aRv) { + // See https://fetch.spec.whatwg.org/#dom-headers-delete nsAutoCString lowerName; ToLowerCase(aName, lowerName); @@ -156,36 +157,33 @@ void InternalHeaders::Delete(const nsACString& aName, ErrorResult& aRv) { return; } - // Step 2 if (IsImmutable(aRv)) { return; } - // Step 3 nsAutoCString value; GetInternal(lowerName, value, aRv); if (IsForbiddenRequestHeader(lowerName, value)) { return; } - // Step 4 + // Step 2 if (mGuard == HeadersGuardEnum::Request_no_cors && !IsNoCorsSafelistedRequestHeaderName(lowerName) && !IsPrivilegedNoCorsRequestHeaderName(lowerName)) { return; } - // Step 5 if (IsForbiddenResponseHeader(lowerName)) { return; } - // Steps 6 and 7 + // Steps 3, 4, and 5 if (!DeleteInternal(lowerName, aRv)) { return; } - // Step 8 + // Step 6 if (mGuard == HeadersGuardEnum::Request_no_cors) { RemovePrivilegedNoCorsRequestHeaders(); } @@ -352,7 +350,9 @@ bool InternalHeaders::IsSimpleHeader(const nsCString& aName, (aName.EqualsIgnoreCase("content-language") && nsContentUtils::IsAllowedNonCorsLanguage(aValue)) || (aName.EqualsIgnoreCase("content-type") && - nsContentUtils::IsAllowedNonCorsContentType(aValue)); + nsContentUtils::IsAllowedNonCorsContentType(aValue)) || + (aName.EqualsIgnoreCase("range") && + nsContentUtils::IsAllowedNonCorsRange(aValue)); } // static diff --git a/testing/web-platform/meta/cors/cors-safelisted-request-header.any.js.ini b/testing/web-platform/meta/cors/cors-safelisted-request-header.any.js.ini deleted file mode 100644 index 35292756e5f5..000000000000 --- a/testing/web-platform/meta/cors/cors-safelisted-request-header.any.js.ini +++ /dev/null @@ -1,18 +0,0 @@ -[cors-safelisted-request-header.any.worker.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [No preflight for {"range":"bytes=100-200"}] - expected: FAIL - - [No preflight for {"range":"bytes=200-"}] - expected: FAIL - - -[cors-safelisted-request-header.any.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [No preflight for {"range":"bytes=100-200"}] - expected: FAIL - - [No preflight for {"range":"bytes=200-"}] - expected: FAIL diff --git a/testing/web-platform/meta/fetch/range/general.any.js.ini b/testing/web-platform/meta/fetch/range/general.any.js.ini index 365411025d7d..ff05c9a64112 100644 --- a/testing/web-platform/meta/fetch/range/general.any.js.ini +++ b/testing/web-platform/meta/fetch/range/general.any.js.ini @@ -1,24 +1,3 @@ -[general.any.worker.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [Cross Origin Fetch with safe range header] - expected: FAIL - - -[general.any.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [Cross Origin Fetch with safe range header] - expected: FAIL - - -[general.any.sharedworker.html] - expected: - if (os == "android") and fission: [OK, TIMEOUT] - [Cross Origin Fetch with safe range header] - expected: FAIL - - [general.any.serviceworker.html] expected: if (os == "android") and fission: [OK, TIMEOUT]