From 2420cd155b4863b37dc1b48d5d346dcb0d8acc56 Mon Sep 17 00:00:00 2001
From: Wolfgang Beyer <[email protected]>
Date: Thu, 17 Sep 2026 05:05:52 -0700
Subject: [PATCH] [DevTools] Block path traversal in "save as" dialog

Bug: 513735940
Change-Id: I2524ce6870c9c29697d3fc1ad1f4a3fc9df85cff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8422481
Auto-Submit: Wolfgang Beyer <[email protected]>
Commit-Queue: Wolfgang Beyer <[email protected]>
Reviewed-by: Alex Rudenko <[email protected]>
Commit-Queue: Alex Rudenko <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1699701}
---
diff --git a/chrome/browser/devtools/devtools_file_helper.cc b/chrome/browser/devtools/devtools_file_helper.cc
index 3284853..cfdaaca 100644
--- a/chrome/browser/devtools/devtools_file_helper.cc
+++ b/chrome/browser/devtools/devtools_file_helper.cc
@@ -36,6 +36,7 @@
 #include "content/public/browser/download_manager.h"
 #include "content/public/common/content_client.h"
 #include "crypto/obsolete/md5.h"
+#include "net/base/filename_util.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/shell_dialogs/selected_file_info.h"
 #include "url/gurl.h"
@@ -166,30 +167,24 @@
   }
 
   if (initial_path.empty()) {
-    GURL gurl(url);
-    std::string suggested_file_name;
-    if (gurl.is_valid()) {
-      std::string escaped_content = gurl.ExtractFileName();
-      // TODO(crbug.com/40839171): Due to filename encoding on Windows we can't
-      // expect to always be able to convert to UTF8 and back
-      suggested_file_name = url::DecodeUrlEscapeSequences(
-          escaped_content, url::DecodeUrlMode::kUtf8OrIsomorphic);
-    } else {
-      suggested_file_name = url;
-    }
-    // TODO(crbug.com/40839171): Truncate a UTF8 string in a better way
-    if (suggested_file_name.length() > 64) {
-      suggested_file_name.erase(64);
-    }
-    // TODO(crbug.com/40839171): Ensure suggested_file_name is an ASCII string
+    // SECURITY: |url| is page-controlled. Derive the suggested filename via
+    // net::GenerateFileName(), which strips path separators, "..", BiDi
+    // control characters and shell-integrated extensions - the same
+    // sanitization the download stack applies. This prevents %2F / %5C
+    // traversal out of the default Save-As folder and U+202E filename
+    // spoofing.
+    base::FilePath suggested_file_name = net::GenerateFileName(
+        GURL(url), /*content_disposition=*/std::string(),
+        /*referrer_charset=*/std::string(), /*suggested_name=*/std::string(),
+        /*mime_type=*/std::string(), /*default_name=*/"download");
+
     if (!GetLastSavePath().empty()) {
-      initial_path =
-          GetLastSavePath().DirName().AppendASCII(suggested_file_name);
+      initial_path = GetLastSavePath().DirName().Append(suggested_file_name);
     } else {
       base::FilePath download_path =
           DownloadPrefs::FromDownloadManager(profile_->GetDownloadManager())
               ->DownloadPath();
-      initial_path = download_path.AppendASCII(suggested_file_name);
+      initial_path = download_path.Append(suggested_file_name);
     }
   }
 
diff --git a/chrome/browser/devtools/devtools_file_helper_unittest.cc b/chrome/browser/devtools/devtools_file_helper_unittest.cc
index 75b99061..5ca4a97f 100644
--- a/chrome/browser/devtools/devtools_file_helper_unittest.cc
+++ b/chrome/browser/devtools/devtools_file_helper_unittest.cc
@@ -103,6 +103,40 @@
         });
   }
 
+  // Runs Save() for `url` and returns the path that would be pre-filled in the
+  // "Save as" dialog. The selection is canceled, so nothing is written to disk.
+  base::FilePath GetSuggestedSavePath(const std::string& url) {
+    base::FilePath suggested_path;
+    base::test::TestFuture<void> canceled;
+    file_helper()->Save(
+        url, "some text", /* save_as */ true,
+        /* is_base64 */ false,
+        base::BindLambdaForTesting(
+            [&](DevToolsFileHelper::SelectedCallback,
+                DevToolsFileHelper::CanceledCallback canceled_callback,
+                const base::FilePath& initial_path) {
+              suggested_path = initial_path;
+              std::move(canceled_callback).Run();
+            }),
+        base::DoNothing(), canceled.GetCallback());
+    EXPECT_TRUE(canceled.Wait());
+    return suggested_path;
+  }
+
+  // Saves to `path`, which makes it the directory against which subsequent
+  // suggested save paths are resolved. DevToolsFileHelper keeps the last save
+  // path in a process-global, so tests that care about the directory must
+  // prime it explicitly rather than rely on the order tests run in.
+  void PrimeLastSaveDirectory(const base::FilePath& path) {
+    base::test::TestFuture<const std::string&> saved;
+    file_helper()->Save("https://example.com/primer.txt", "primer",
+                        /* save_as */ true,
+                        /* is_base64 */ false,
+                        FakeSelectFileCallback(ui::SelectedFileInfo(path)),
+                        saved.GetCallback(), base::DoNothing());
+    EXPECT_TRUE(saved.Wait());
+  }
+
   void ConnectAutomaticFileSystem(const base::FilePath& path,
                                   const base::Uuid& uuid,
                                   bool already_known,
@@ -253,6 +287,66 @@
   EXPECT_EQ(base::ReadFileToBytes(tf.path()), data);
 }
 
+TEST_F(DevToolsFileHelperTest, SuggestedSavePathUsesLastSaveDirectory) {
+  base::ScopedTempFile tf;
+  ASSERT_TRUE(tf.Create());
+  PrimeLastSaveDirectory(tf.path());
+
+  EXPECT_EQ(GetSuggestedSavePath("https://example.com/script.js"),
+            tf.path().DirName().Append(FILE_PATH_LITERAL("script.js")));
+}
+
+TEST_F(DevToolsFileHelperTest, SuggestedSavePathFallsBackToDefaultName) {
+  base::ScopedTempFile tf;
+  ASSERT_TRUE(tf.Create());
+  PrimeLastSaveDirectory(tf.path());
+  const base::FilePath dir = tf.path().DirName();
+
+  // URLs without a usable file name must fall back to the default name,
+  // rather than turning the raw URL into a file name.
+  for (const char* url : {"https://example.com/", "not a url", ""}) {
+    EXPECT_EQ(GetSuggestedSavePath(url),
+              dir.Append(FILE_PATH_LITERAL("download")))
+        << url;
+  }
+}
+
+// Regression test: `url` is page-controlled, so the file name derived from it
+// must never be able to escape the target directory, no matter how the
+// separators and parent references are escaped.
+TEST_F(DevToolsFileHelperTest, SuggestedSavePathCannotEscapeDirectory) {
+  base::ScopedTempFile tf;
+  ASSERT_TRUE(tf.Create());
+  PrimeLastSaveDirectory(tf.path());
+  const base::FilePath dir = tf.path().DirName();
+
+  for (const char* url : {
+           "https://example.com/a%2F..%2F..%2Fevil.sh",
+           "https://example.com/%2e%2e%2f%2e%2e%2fevil.sh",
+           "https://example.com/a%5C..%5C..%5Cevil.sh",
+           "https://example.com/%2Fetc%2Fpasswd",
+       }) {
+    const base::FilePath suggested = GetSuggestedSavePath(url);
+    EXPECT_FALSE(suggested.ReferencesParent()) << url;
+    EXPECT_EQ(suggested.DirName(), dir) << url;
+  }
+}
+
+// Regression test: the suggested file name must not carry BiDi control
+// characters, which can be used to spoof the extension shown in the dialog.
+TEST_F(DevToolsFileHelperTest, SuggestedSavePathStripsBidiControlCharacters) {
+  base::ScopedTempFile tf;
+  ASSERT_TRUE(tf.Create());
+  PrimeLastSaveDirectory(tf.path());
+
+  const base::FilePath suggested =
+      GetSuggestedSavePath("https://example.com/report%E2%80%AEfdp.exe");
+
+  EXPECT_EQ(suggested.value().find(FILE_PATH_LITERAL("\u202e")),
+            base::FilePath::StringType::npos);
+  EXPECT_EQ(suggested.DirName(), tf.path().DirName());
+}
+
 #if BUILDFLAG(IS_ANDROID)
 TEST_F(DevToolsFileHelperTest, SaveToFileContentUri) {
   base::ScopedTempFile tf;

diff --git a/chrome/browser/devtools/devtools_file_helper_unittest.cc b/chrome/browser/devtools/devtools_file_helper_unittest.cc
index 75b99061..5ca4a97f 100644
--- a/chrome/browser/devtools/devtools_file_helper_unittest.cc
+++ b/chrome/browser/devtools/devtools_file_helper_unittest.cc
@@ -103,6 +103,40 @@
         });
   }
 
+  // Runs Save() for `url` and returns the path that would be pre-filled in the
+  // "Save as" dialog. The selection is canceled, so nothing is written to disk.
+  base::FilePath GetSuggestedSavePath(const std::string& url) {
+    base::FilePath suggested_path;
+    base::test::TestFuture<void> canceled;
+    file_helper()->Save(
+        url, "some text", /* save_as */ true,
+        /* is_base64 */ false,
+        base::BindLambdaForTesting(
+            [&](DevToolsFileHelper::SelectedCallback,
+                DevToolsFileHelper::CanceledCallback canceled_callback,
+                const base::FilePath& initial_path) {
+              suggested_path = initial_path;
+              std::move(canceled_callback).Run();
+            }),
+        base::DoNothing(), canceled.GetCallback());
+    EXPECT_TRUE(canceled.Wait());
+    return suggested_path;
+  }
+
+  // Saves to `path`, which makes it the directory against which subsequent
+  // suggested save paths are resolved. DevToolsFileHelper keeps the last save
+  // path in a process-global, so tests that care about the directory must
+  // prime it explicitly rather than rely on the order tests run in.
+  void PrimeLastSaveDirectory(const base::FilePath& path) {
+    base::test::TestFuture<const std::string&> saved;
+    file_helper()->Save("https://example.com/primer.txt", "primer",
+                        /* save_as */ true,
+                        /* is_base64 */ false,
+                        FakeSelectFileCallback(ui::SelectedFileInfo(path)),
+                        saved.GetCallback(), base::DoNothing());
+    EXPECT_TRUE(saved.Wait());
+  }
+
   void ConnectAutomaticFileSystem(const base::FilePath& path,
                                   const base::Uuid& uuid,
                                   bool already_known,
@@ -253,6 +287,66 @@
   EXPECT_EQ(base::ReadFileToBytes(tf.path()), data);
 }
 
+TEST_F(DevToolsFileHelperTest, SuggestedSavePathUsesLastSaveDirectory) {
+  base::ScopedTempFile tf;
+  ASSERT_TRUE(tf.Create());
+  PrimeLastSaveDirectory(tf.path());
+
+  EXPECT_EQ(GetSuggestedSavePath("https://example.com/script.js"),
+            tf.path().DirName().Append(FILE_PATH_LITERAL("script.js")));
+}
+
+TEST_F(DevToolsFileHelperTest, SuggestedSavePathFallsBackToDefaultName) {
+  base::ScopedTempFile tf;
+  ASSERT_TRUE(tf.Create());
+  PrimeLastSaveDirectory(tf.path());
+  const base::FilePath dir = tf.path().DirName();
+
+  // URLs without a usable file name must fall back to the default name,
+  // rather than turning the raw URL into a file name.
+  for (const char* url : {"https://example.com/", "not a url", ""}) {
+    EXPECT_EQ(GetSuggestedSavePath(url),
+              dir.Append(FILE_PATH_LITERAL("download")))
+        << url;
+  }
+}
+
+// Regression test: `url` is page-controlled, so the file name derived from it
+// must never be able to escape the target directory, no matter how the
+// separators and parent references are escaped.
+TEST_F(DevToolsFileHelperTest, SuggestedSavePathCannotEscapeDirectory) {
+  base::ScopedTempFile tf;
+  ASSERT_TRUE(tf.Create());
+  PrimeLastSaveDirectory(tf.path());
+  const base::FilePath dir = tf.path().DirName();
+
+  for (const char* url : {
+           "https://example.com/a%2F..%2F..%2Fevil.sh",
+           "https://example.com/%2e%2e%2f%2e%2e%2fevil.sh",
+           "https://example.com/a%5C..%5C..%5Cevil.sh",
+           "https://example.com/%2Fetc%2Fpasswd",
+       }) {
+    const base::FilePath suggested = GetSuggestedSavePath(url);
+    EXPECT_FALSE(suggested.ReferencesParent()) << url;
+    EXPECT_EQ(suggested.DirName(), dir) << url;
+  }
+}
+
+// Regression test: the suggested file name must not carry BiDi control
+// characters, which can be used to spoof the extension shown in the dialog.
+TEST_F(DevToolsFileHelperTest, SuggestedSavePathStripsBidiControlCharacters) {
+  base::ScopedTempFile tf;
+  ASSERT_TRUE(tf.Create());
+  PrimeLastSaveDirectory(tf.path());
+
+  const base::FilePath suggested =
+      GetSuggestedSavePath("https://example.com/report%E2%80%AEfdp.exe");
+
+  EXPECT_EQ(suggested.value().find(FILE_PATH_LITERAL("\u202e")),
+            base::FilePath::StringType::npos);
+  EXPECT_EQ(suggested.DirName(), tf.path().DirName());
+}
+
 #if BUILDFLAG(IS_ANDROID)
 TEST_F(DevToolsFileHelperTest, SaveToFileContentUri) {
   base::ScopedTempFile tf;