Make OutOfProcessInstance::SaveToBuffer use net::GetSuggestedFilename().

It was just using net::UnescapeURLComponent(), which both has some
behaviors not suitable for this purpose (Like deliberately leaving some
characters we can't safely display to the user, like locks and \x01,
escaped).  Also, net::GetSuggestedFilename() is specifically designed
for this purpose, and has some behaviors that are desireable here
(Like replacing characters that shouldn't appear in file names, and
trimming whitespace).

It turns out that most usages of UnescapeURLComponent() are wrong, and
this CL is part of an effort to fix that.

Bug: 849998
Change-Id: Ib98e178e7f7cc002e49986026a6799b7c581caa4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1596362
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#657486}

pdf/BUILD.gn

@@ -150,6 +150,7 @@ if (enable_pdf) {
     sources = [
       "chunk_stream_unittest.cc",
       "document_loader_impl_unittest.cc",
+      "out_of_process_instance_unittest.cc",
       "pdf_transform_unittest.cc",
       "range_set_unittest.cc",
     ]

pdf/out_of_process_instance.cc

@@ -21,6 +21,7 @@
 #include "base/values.h"
 #include "chrome/common/content_restriction.h"
 #include "net/base/escape.h"
+#include "net/base/filename_util.h"
 #include "pdf/accessibility.h"
 #include "pdf/pdf.h"
 #include "pdf/pdf_features.h"
@@ -1459,14 +1460,10 @@ bool OutOfProcessInstance::ShouldSaveEdits() const {
 void OutOfProcessInstance::SaveToBuffer(const std::string& token) {
   engine_->KillFormFocus();
 
-  GURL url(url_);
-  std::string file_name = url.ExtractFileName();
-  file_name = net::UnescapeURLComponent(file_name, net::UnescapeRule::SPACES);
-
   pp::VarDictionary message;
   message.Set(kType, kJSSaveDataType);
   message.Set(kJSToken, pp::Var(token));
-  message.Set(kJSFileName, pp::Var(file_name));
+  message.Set(kJSFileName, pp::Var(GetFileNameFromUrl(url_)));
   // This will be overwritten if the save is successful.
   message.Set(kJSDataToSave, pp::Var(pp::Var::Null()));
   const bool has_unsaved_changes =
@@ -1731,6 +1728,16 @@ void OutOfProcessInstance::RotateCounterclockwise() {
   engine_->RotateCounterclockwise();
 }
 
+std::string OutOfProcessInstance::GetFileNameFromUrl(const std::string& url) {
+  // Generate a file name. Unfortunately, MIME type can't be provided, since it
+  // requires IO.
+  base::string16 file_name = net::GetSuggestedFilename(
+      GURL(url), std::string() /* content_disposition */,
+      std::string() /* referrer_charset */, std::string() /* suggested_name */,
+      std::string() /* mime_type */, std::string() /* default_name */);
+  return base::UTF16ToUTF8(file_name);
+}
+
 void OutOfProcessInstance::PreviewDocumentLoadComplete() {
   if (preview_document_load_state_ != LOAD_STATE_LOADING ||
       preview_pages_info_.empty()) {

pdf/out_of_process_instance.h

@@ -156,6 +156,10 @@ class OutOfProcessInstance : public pp::Instance,
   void RotateClockwise();
   void RotateCounterclockwise();
 
+  // Creates a file name for saving a PDF file, given the source URL. Exposed
+  // for testing.
+  static std::string GetFileNameFromUrl(const std::string& url);
+
  private:
   void ResetRecentlySentFindUpdate(int32_t);
 

pdf/out_of_process_instance_unittest.cc

@@ -0,0 +1,36 @@
+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "pdf/out_of_process_instance.h"
+
+#include <string>
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chrome_pdf {
+
+TEST(OutOfProcessInstanceTest, GetFileNameFromUrl) {
+  EXPECT_EQ("b.pdf",
+            OutOfProcessInstance::GetFileNameFromUrl("https://test/a/b.pdf"));
+
+  // File extensions should be kept as-is.
+  EXPECT_EQ("b.hat",
+            OutOfProcessInstance::GetFileNameFromUrl("https://test/a/b.hat"));
+
+  // Most escaped characters should be unescaped.
+  EXPECT_EQ("a b.pdf", OutOfProcessInstance::GetFileNameFromUrl(
+                           "https://test/%61%20b.pdf"));
+
+  // Escaped file path delimiters and control codes should be replaced by a
+  // placeholder.
+  EXPECT_EQ("a_b_.pdf", OutOfProcessInstance::GetFileNameFromUrl(
+                            "https://test/a%2Fb%01.pdf"));
+
+  // UTF-8 characters, including ones left escaped by UnescapeURLComponent() for
+  // security reasons, are allowed in file paths.
+  EXPECT_EQ("\xF0\x9F\x94\x92", OutOfProcessInstance::GetFileNameFromUrl(
+                                    "https://test/%F0%9F%94%92"));
+}
+
+}  // namespace chrome_pdf