From f195f959d33f2c3e4d5eaea514a2273b33636fab Mon Sep 17 00:00:00 2001
From: Matt Menke <[email protected]>
Date: Wed, 03 Jun 2026 12:49:07 -0700
Subject: [PATCH] Fix up Content-Disposition header parsing.

In particular, make existing HttpContentDisposition constructor use
HttpUtil::ValuesIterator() to only look at the first value, in the
case it's passed a comma-delimited list.

Also add an overloaded constructor that takes an HttpResponseHeaders
object, picks out the first one, and only parses that, and label it
as the preferred constructor.

This CL migrates both callsites that were incorrectly calling the old
constructor with the full normalized Content-Disposition header with
the new overload, and adds a base::Feature to use the old parsing logic.

If no issues are encountered in the next couple months, we can remove
the base::Feature and migrate most callers to the new API, though
this CL does update the old API to have correct behavior, it's
still easier to get wrong.

Bug: 517466133
Change-Id: I242c7993e8145565238ac6f804142dc4d161c3c7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7887937
Commit-Queue: mmenke <[email protected]>
Reviewed-by: Xinghui Lu <[email protected]>
Reviewed-by: Kenichi Ishibashi <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1641123}
---

diff --git a/components/download/internal/common/download_utils.cc b/components/download/internal/common/download_utils.cc
index 3b55aa2..4af5c2c2 100644
--- a/components/download/internal/common/download_utils.cc
+++ b/components/download/internal/common/download_utils.cc
@@ -884,12 +884,9 @@
   if (!response_head.headers) {
     return false;
   }
-  std::string disposition =
-      response_head.headers->GetNormalizedHeader("content-disposition")
-          .value_or(std::string());
-  return !disposition.empty() &&
-         net::HttpContentDisposition(disposition, std::string())
-             .is_attachment();
+  return net::HttpContentDisposition(*response_head.headers,
+                                     /*referrer_charset=*/std::string())
+      .is_attachment();
 }
 
 }  // namespace download
diff --git a/content/browser/loader/download_utils_impl.cc b/content/browser/loader/download_utils_impl.cc
index 8fd76cb..fad2264 100644
--- a/content/browser/loader/download_utils_impl.cc
+++ b/content/browser/loader/download_utils_impl.cc
@@ -44,11 +44,8 @@
                   const net::HttpResponseHeaders* headers,
                   const std::string& mime_type) {
   if (headers) {
-    std::string disposition =
-        headers->GetNormalizedHeader("content-disposition")
-            .value_or(std::string());
-    if (!disposition.empty() &&
-        net::HttpContentDisposition(disposition, std::string())
+    if (net::HttpContentDisposition(*headers,
+                                    /*referrer_charset=*/std::string())
             .is_attachment()) {
       return true;
     }
diff --git a/net/base/features.cc b/net/base/features.cc
index 74217b11..65a43cd9 100644
--- a/net/base/features.cc
+++ b/net/base/features.cc
@@ -130,6 +130,9 @@
         &kNetworkQualityEstimator,
         "EffectiveConnectionTypeRecomputationInterval", base::Seconds(10)};
 
+BASE_FEATURE(kOnlyParseFirstContentDisposition,
+             base::FEATURE_ENABLED_BY_DEFAULT);
+
 BASE_FEATURE(kSplitCacheByIncludeCredentials,
              base::FEATURE_DISABLED_BY_DEFAULT);
 
diff --git a/net/base/features.h b/net/base/features.h
index efef7db0..209748f 100644
--- a/net/base/features.h
+++ b/net/base/features.h
@@ -193,6 +193,15 @@
 NET_EXPORT extern const base::FeatureParam<base::TimeDelta>
     kEffectiveConnectionTypeRecomputationInterval;
 
+// When disabled, HttpContentDisposition incorrectly handles multiple
+// comma-delimited Content-Disposition lines, treating them all as a single
+// Content-Disposition string.
+//
+// This is a temporary escape valve in case the fix for
+// https://crbug.com/517466133 causes issues.
+// TODO(crbug.com/519218483): Remove this in late Q3/Q4 2026.
+NET_EXPORT BASE_DECLARE_FEATURE(kOnlyParseFirstContentDisposition);
+
 // Splits cache entries by the request's includeCredentials.
 NET_EXPORT BASE_DECLARE_FEATURE(kSplitCacheByIncludeCredentials);
 
diff --git a/net/http/http_content_disposition.cc b/net/http/http_content_disposition.cc
index b540420b..bf6488a 100644
--- a/net/http/http_content_disposition.cc
+++ b/net/http/http_content_disposition.cc
@@ -9,12 +9,15 @@
 
 #include "base/base64.h"
 #include "base/check_op.h"
+#include "base/feature_list.h"
 #include "base/strings/escape.h"
 #include "base/strings/string_tokenizer.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
+#include "net/base/features.h"
 #include "net/base/net_string_util.h"
+#include "net/http/http_response_headers.h"
 #include "net/http/http_util.h"
 
 namespace net {
@@ -335,9 +338,36 @@
 } // namespace
 
 HttpContentDisposition::HttpContentDisposition(
-    const std::string& header,
+    const HttpResponseHeaders& headers,
     const std::string& referrer_charset) {
-  Parse(header, referrer_charset);
+  if (!base::FeatureList::IsEnabled(
+          features::kOnlyParseFirstContentDisposition)) {
+    std::optional<std::string> header =
+        headers.GetNormalizedHeader("Content-Disposition");
+    if (header) {
+      Parse(*header, referrer_charset);
+    }
+    return;
+  }
+  std::optional<std::string_view> header =
+      headers.EnumerateHeader(/*iter=*/nullptr, "Content-Disposition");
+  if (header) {
+    Parse(*header, referrer_charset);
+  }
+}
+
+HttpContentDisposition::HttpContentDisposition(
+    std::string_view header,
+    const std::string& referrer_charset) {
+  if (!base::FeatureList::IsEnabled(
+          features::kOnlyParseFirstContentDisposition)) {
+    Parse(header, referrer_charset);
+    return;
+  }
+  HttpUtil::ValuesIterator it(header, ',', /*ignore_empty_values=*/false);
+  if (it.GetNext()) {
+    Parse(it.value(), referrer_charset);
+  }
 }
 
 HttpContentDisposition::~HttpContentDisposition() = default;
@@ -393,7 +423,7 @@
 //                      | ext-token "=" ext-value
 //  ext-token           = <the characters in token, followed by "*">
 //
-void HttpContentDisposition::Parse(const std::string& header,
+void HttpContentDisposition::Parse(std::string_view header,
                                    const std::string& referrer_charset) {
   DCHECK(type_ == INLINE);
   DCHECK(filename_.empty());
diff --git a/net/http/http_content_disposition.h b/net/http/http_content_disposition.h
index 7c6cf5f..2e6f3238 100644
--- a/net/http/http_content_disposition.h
+++ b/net/http/http_content_disposition.h
@@ -12,6 +12,8 @@
 
 namespace net {
 
+class HttpResponseHeaders;
+
 class NET_EXPORT HttpContentDisposition {
  public:
   enum Type {
@@ -52,7 +54,35 @@
     HAS_SINGLE_QUOTED_FILENAME = 1 << 7,
   };
 
-  HttpContentDisposition(const std::string& header,
+  // NOTE: Until features::kOnlyParseFirstContentDisposition is removed, new
+  // consumers should use EnumerateHeader() to get the first Content-Disposition
+  // header and then pass it to the constructor that takes a std::string_view.
+  // Once that's removed, however, this constructor should be used. This comment
+  // should be removed when that happens.
+  // TODO(crbug.com/519218483): Remove this comment in late Q3/Q4 2026, when
+  // the feature is removed.
+  //
+  // Preferred constructor. Uses the first Content-Disposition header in the
+  // response (retrieved using `HttpResponseHeaders::EnumerateHeader`). Only
+  // looks at the first Content-Disposition header. The HTTP code guarantees

diff --git a/net/http/http_content_disposition_unittest.cc b/net/http/http_content_disposition_unittest.cc
index 7f62a04..a93dc595 100644
--- a/net/http/http_content_disposition_unittest.cc
+++ b/net/http/http_content_disposition_unittest.cc
@@ -6,7 +6,11 @@
 
 #include <array>
 
+#include "base/strings/strcat.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/test/scoped_feature_list.h"
+#include "net/base/features.h"
+#include "net/http/http_response_headers.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
@@ -19,9 +23,43 @@
   const wchar_t* expected;
 };
 
-}  // anonymous namespace
+}  // namespace
 
-TEST(HttpContentDispositionTest, Filename) {
+// The parameter indicates whether CreateContentDispositionFromHeaders() should
+// use the constructor that takes a full HttpResponseHeaders or the one that
+// takes the header as a string_view.
+class HttpContentDispositionTest : public testing::TestWithParam<bool> {
+ protected:
+  // Creates an HttpContentDisposition from a single header value.
+  HttpContentDisposition CreateContentDisposition(std::string_view header_value,
+                                                  const std::string& charset) {
+    std::string raw_headers =
+        base::StrCat({"HTTP/1.1 200 OK\r\n",
+                      "Content-Disposition: ", header_value, "\r\n\r\n"});
+    return CreateContentDispositionFromHeaders(raw_headers, charset);
+  }
+
+  // Creates an HttpContentDisposition from a full set of HTTP headers. Allows
+  // there to be no Content-Disposition value, or multiple values, unlike
+  // CreateContentDisposition().
+  HttpContentDisposition CreateContentDispositionFromHeaders(
+      std::string_view raw_headers,
+      const std::string& charset) {
+    auto headers = base::MakeRefCounted<HttpResponseHeaders>(
+        HttpUtil::AssembleRawHeaders(raw_headers));
+    if (GetParam()) {
+      return HttpContentDisposition(*headers, charset);
+    } else {
+      std::optional<std::string> normalized =
+          headers->GetNormalizedHeader("Content-Disposition");
+      return HttpContentDisposition(normalized.value_or(""), charset);
+    }
+  }
+};
+
+INSTANTIATE_TEST_SUITE_P(, HttpContentDispositionTest, testing::Bool());
+
+TEST_P(HttpContentDispositionTest, Filename) {
   const FileNameCDCase tests[] = {
       // Test various forms of C-D header fields emitted by web servers.
       {"inline; filename=\"abcde.pdf\"", "", L"abcde.pdf"},
@@ -29,7 +67,7 @@
       {"  inline   ; filename=\"abcde.pdf\"", "", L"abcde.pdf"},
       {"\t\tinline\t\t; filename=\"abcde.pdf\"", "", L"abcde.pdf"},
       {"attachment; filename=abcde.pdf", "", L"abcde.pdf"},
-      {"attachment; filename=abc,de.pdf", "", L"abc,de.pdf"},
+      {"attachment; filename=abc,de.pdf", "", L"abc"},
       {"filename=abcde.pdf", "", L"abcde.pdf"},
       {"filename= abcde.pdf", "", L"abcde.pdf"},
       {"filename =abcde.pdf", "", L"abcde.pdf"},
@@ -44,7 +82,7 @@
       // Unbalanced quotation mark
       {"filename=\"abcdef.pdf", "", L"abcdef.pdf"},
       // Whitespaces are converted to a space.
-      {"inline; filename=\"abc  \t\nde.pdf\"", "", L"abc    de.pdf"},
+      {"inline; filename=\"abc  \tde.pdf\"", "", L"abc   de.pdf"},
       // %-escaped UTF-8
       {"attachment; filename=\"%EC%98%88%EC%88%A0%20"
        "%EC%98%88%EC%88%A0.jpg\"",
@@ -52,7 +90,7 @@
       {"attachment; filename=\"%F0%90%8C%B0%F0%90%8C%B1"
        "abc.jpg\"",
        "", L"\U00010330\U00010331abc.jpg"},
-      {"attachment; filename=\"%EC%98%88%EC%88%A0 \n"
+      {"attachment; filename=\"%EC%98%88%EC%88%A0 \t"
        "%EC%98%88%EC%88%A0.jpg\"",
        "", L"\xc608\xc220  \xc608\xc220.jpg"},
       // Characters that are not supposed to be displayed should still be
@@ -200,14 +238,15 @@
       {"attachment; foobar=x; filename=\"foo.html\"", "", L"foo.html"},
   };
   for (const auto& test : tests) {
-    HttpContentDisposition header(test.header, test.referrer_charset);
+    HttpContentDisposition header =
+        CreateContentDisposition(test.header, test.referrer_charset);
     EXPECT_EQ(test.expected, base::UTF8ToWide(header.filename()))
         << "Failed on input: " << test.header;
   }
 }
 
 // Test cases from http://greenbytes.de/tech/tc2231/
-TEST(HttpContentDispositionTest, tc2231) {
+TEST_P(HttpContentDispositionTest, tc2231) {
   const struct FileNameCDCase {
     const char* header;
     HttpContentDisposition::Type expected_type;
@@ -354,7 +393,7 @@
       // http://greenbytes.de/tech/tc2231/#attmissingdisposition4
       // Note: tc2231 says we should fail to parse this header.
       {"filename=foo.html, filename=bar.html", HttpContentDisposition::INLINE,
-       L"foo.html, filename=bar.html"},
+       L"foo.html"},
       // http://greenbytes.de/tech/tc2231/#emptydisposition
       // Note: tc2231 says we should fail to parse this header.
       {"; filename=foo.html", HttpContentDisposition::INLINE, L"foo.html"},
@@ -381,7 +420,7 @@
       // http://greenbytes.de/tech/tc2231/#attmultinstances
       // Note: tc2231 says we should fail to parse this header.
       {"attachment; filename=foo.html, attachment; filename=bar.html",
-       HttpContentDisposition::ATTACHMENT, L"foo.html, attachment"},
+       HttpContentDisposition::ATTACHMENT, L"foo.html"},
       // http://greenbytes.de/tech/tc2231/#attmissingdelim
       {"attachment; foo=foo filename=bar", HttpContentDisposition::ATTACHMENT,
        L""},
@@ -412,7 +451,8 @@
       // TODO(abarth): http://greenbytes.de/tech/tc2231/#attrfc2047quoted
   };
   for (const auto& test : tests) {
-    HttpContentDisposition header(test.header, std::string());
+    HttpContentDisposition header =
+        CreateContentDisposition(test.header, std::string());
     EXPECT_EQ(test.expected_type, header.type())
         << "Failed on input: " << test.header;
     EXPECT_EQ(test.expected_filename, base::UTF8ToWide(header.filename()))
@@ -420,7 +460,7 @@
   }
 }
 
-TEST(HttpContentDispositionTest, ParseResult) {
+TEST_P(HttpContentDispositionTest, ParseResult) {
   struct ParseResultTestCase {
     const char* header;
     int expected_flags;
@@ -487,7 +527,8 @@
 
   for (size_t i = 0; i < std::size(kTestCases); ++i) {
     const ParseResultTestCase& test_case = kTestCases[i];
-    HttpContentDisposition content_disposition(test_case.header, "utf-8");
+    HttpContentDisposition content_disposition =
+        CreateContentDisposition(test_case.header, "utf-8");
     int result = content_disposition.parse_result_flags();
 
     SCOPED_TRACE(testing::Message() << "Test case " << i
@@ -496,7 +537,15 @@
   }
 }
 
-TEST(HttpContentDispositionTest, ContainsNul) {
+// Unclear if this test is still useful, as nulls are not generally allowed in
+// headers, though one constructor can take arbitrary strings.
+TEST_P(HttpContentDispositionTest, ContainsNul) {
+  // Can only pass nulls to the constructor that takes a string, since they're
+  // not allowed in HTTP headers.
+  if (GetParam()) {
+    GTEST_SKIP();
+  }
+
   const char kHeader[] = "filename=ab\0c";
   const char kExpectedFilename[] = "ab\0c";
   // Note: both header and expected_filename include the trailing NUL.
@@ -507,4 +556,62 @@
   EXPECT_EQ(expected_filename, content_disposition.filename());
 }
 
+TEST_P(HttpContentDispositionTest, NoContentDisposition) {
+  HttpContentDisposition content_disposition =
+      CreateContentDispositionFromHeaders("HTTP/1.1 200 OK\r\n\r\n", "utf-8");
+  EXPECT_EQ(content_disposition.parse_result_flags(),
+            HttpContentDisposition::INVALID);
+  EXPECT_FALSE(content_disposition.is_attachment());
+  EXPECT_EQ(content_disposition.filename(), "");
+}
+
+TEST_P(HttpContentDispositionTest, MultipleContentDisposition) {
+  // Test multiple Content-Disposition headers, only the first should be used.
+  {
+    std::string headers =
+        "HTTP/1.1 200 OK\r\n"
+        "Content-Disposition: inline; filename=abc.pdf\r\n"
+        "Content-Disposition: attachment; filename=def.pdf\r\n\r\n";
+    HttpContentDisposition content_disposition =
+        CreateContentDispositionFromHeaders(headers, "utf-8");
+    EXPECT_FALSE(content_disposition.is_attachment());
+    EXPECT_EQ(HttpContentDisposition::INLINE, content_disposition.type());
+    EXPECT_EQ("abc.pdf", content_disposition.filename());
+  }
+
+  // Test single Content-Disposition header with multiple values separated by a
+  // comma, which should be treated just as if there were multiple
+  // content-disposition headers.
+  {
+    std::string headers =
+        "HTTP/1.1 200 OK\r\n"
+        "Content-Disposition: inline; filename=abc.pdf, "
+        "attachment; filename=def.pdf\r\n\r\n";
+    HttpContentDisposition content_disposition =
+        CreateContentDispositionFromHeaders(headers, "utf-8");
+    EXPECT_FALSE(content_disposition.is_attachment());
+    EXPECT_EQ(HttpContentDisposition::INLINE, content_disposition.type());
+    EXPECT_EQ("abc.pdf", content_disposition.filename());
+  }
+}
+
+// Tests that disabling `kOnlyParseFirstContentDisposition` correctly
+// reintroduces a bug, in case we have to shut it off temporarily due to
+// breakage.
+TEST_P(HttpContentDispositionTest, OnlyParseFirstContentDispositionDisabled) {
+  std::string content_disposition_string = "attachment, attachment";
+  HttpContentDisposition content_disposition =
+      CreateContentDisposition(content_disposition_string, "utf-8");
+  EXPECT_TRUE(content_disposition.is_attachment());
+  EXPECT_EQ("", content_disposition.filename());
+
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndDisableFeature(
+      features::kOnlyParseFirstContentDisposition);
+  HttpContentDisposition content_disposition2 =
+      CreateContentDisposition(content_disposition_string, "utf-8");
+  EXPECT_FALSE(content_disposition2.is_attachment());
+  EXPECT_EQ("", content_disposition2.filename());
+}
+
 }  // namespace net