desktop-pwas: Disallow . in Win7 launcher filename

This change replaces any '.' characters in a PWA launcher's filename
with '_' on Windows 7 to prevent launchers from having misleading or
potentially unsafe file extensions, such as those in
net::IsShellIntegratedExtension(). This is largely a just-in-case
measure added because a PWA launcher on Windows 7 has no extension.

This change also simplifies the logic for legitimizing a reserved
filename on Windows by prepending '_' rather than appending it.

On Windows, each Progressive Web App (PWA) must have its own launcher
in order for the PWA to be able to register as a handler for file
types. On Windows 7, the filename of this launcher is used as the
PWA's display name in the Open With menu, because Windows 7 lacks an
alternate way to set multiple custom display names for one executable
(and PWA launchers are identical copies of chrome_pwa_launcher.exe).

The launcher filename is based on the PWA's name, with modifications
to ensure the resulting filename is legitimate.

These modifications are as follows:
* Replace illegal characters with '_'

* If filename is reserved on Windows:
  - OLD:
    + Append '_'
    + Replace all '.' with '_'; this was necessary because Windows'
      logic for checking reserved filenames views characters after '.'
      as file extensions, and only the pre-file-extension portion is
      checked for legitimacy (e.g. "nul" is reserved; "nul_" is
      allowed, but "nul.a_" is not).
  - NEW:
    + Prepend '_'

* NEW: On Windows 7, replace all '.' with '_'

Change-Id: Ibb2a183a8f023e5cbd98adb143e95551c49a1218
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2002982
Commit-Queue: Jesse McKenna <jessemckenna@google.com>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734265}

chrome/browser/web_applications/components/web_app_file_handler_registration_win.cc

@@ -32,29 +32,31 @@ namespace {
 // Returns the app-specific-launcher filename to be used for |app_name|.
 base::FilePath GetAppSpecificLauncherFilename(const base::string16& app_name) {
   // Remove any characters that are illegal in Windows filenames.
-  base::FilePath sanitized_app_name =
-      web_app::internals::GetSanitizedFileName(app_name);
-
-  // If |sanitized_app_name| is a reserved filename, append '_' to allow its use
-  // as the launcher filename (e.g. "nul" => "nul_"). If |sanitized_app_name|
-  // starts with a reserved filename followed by '.', replace all '.' characters
-  // with '_' (e.g. "nul.l" => "nul_l"). This is needed because anything after
-  // '.' is interpreted as part of a file extension for the purpose of
-  // identifying reserved filenames, so appending '_' fails to legitimize a
-  // reserved filename when a '.' is present (e.g. "nul.l_" is rejected).
-  if (net::IsReservedNameOnWindows(sanitized_app_name.value())) {
-    base::string16 allowed_filename = sanitized_app_name.value();
-    if (!base::ReplaceChars(allowed_filename, L".", L"_", &allowed_filename))
-      allowed_filename.append(L"_");
-    sanitized_app_name = base::FilePath(allowed_filename);
-  }
-
-  // On Windows 8+, add .exe extension. On Windows 7, where the launcher
-  // filename is used as the app's display name in the Open With menu, omit the
-  // extension.
-  if (base::win::GetVersion() > base::win::Version::WIN7)
-    return sanitized_app_name.AddExtension(L"exe");
-  return sanitized_app_name;
+  base::string16 sanitized_app_name =
+      web_app::internals::GetSanitizedFileName(app_name).value();
+
+  // On Windows 7, where the launcher has no file extension, replace any '.'
+  // characters with '_' to prevent a portion of the filename from being
+  // interpreted as its extension.
+  const bool is_win_7 = base::win::GetVersion() == base::win::Version::WIN7;
+  if (is_win_7)
+    base::ReplaceChars(sanitized_app_name, L".", L"_", &sanitized_app_name);
+
+  // If |sanitized_app_name| is a reserved filename, prepend '_' to allow its
+  // use as the launcher filename (e.g. "nul" => "_nul"). Prepending is
+  // preferred over appending in order to handle filenames containing '.', as
+  // Windows' logic for checking reserved filenames views characters after '.'
+  // as file extensions, and only the pre-file-extension portion is checked for
+  // legitimacy (e.g. "nul_" is allowed, but "nul.a_" is not).
+  if (net::IsReservedNameOnWindows(sanitized_app_name))
+    sanitized_app_name = L"_" + sanitized_app_name;
+
+  // On Windows 8+, add .exe extension. On Windows 7, where an app's display
+  // name in the Open With menu can't be set programmatically, omit the
+  // extension to use the launcher filename as the app's display name.
+  if (!is_win_7)
+    return base::FilePath(sanitized_app_name).AddExtension(L"exe");
+  return base::FilePath(sanitized_app_name);
 }
 
 }  // namespace

chrome/browser/web_applications/components/web_app_file_handler_registration_win_unittest.cc

@@ -226,16 +226,16 @@ TEST_F(WebAppFileHandlerRegistrationWinTest, AppNameWithInvalidChars) {
   EXPECT_EQ(app_specific_launcher_path, registered_app_path);
 }
 
-// Test that an app name that is a reserved filename on Windows has '_' appended
-// to it when used as a filename for its launcher.
+// Test that an app name that is a reserved filename on Windows has '_'
+// prepended to it when used as a filename for its launcher.
 TEST_F(WebAppFileHandlerRegistrationWinTest, AppNameIsReservedFilename) {
   std::set<std::string> file_extensions({"txt"});
   AppId app_id("app_id");
 
-  // "con" is a reserved filename on Windows, so it should have '_' appended.
+  // "con" is a reserved filename on Windows, so it should have '_' prepended.
   std::string app_name("con");
   base::FilePath app_specific_launcher_path =
-      CreateDataDirectoryAndGetLauncherPathForApp(profile(), app_id, "con_");
+      CreateDataDirectoryAndGetLauncherPathForApp(profile(), app_id, "_con");
 
   RegisterFileHandlersWithOs(app_id, app_name, profile(),
                              /*file_extensions*/ {"txt"}, /*mime_types=*/{});
@@ -248,20 +248,19 @@ TEST_F(WebAppFileHandlerRegistrationWinTest, AppNameIsReservedFilename) {
   EXPECT_EQ(app_specific_launcher_path, registered_app_path);
 }
 
-// Test that an app name that consists of a reserved filename on Windows plus
-// '.' has all its '.' characters replaced by '_' when used as a filename for
-// its launcher.
-TEST_F(WebAppFileHandlerRegistrationWinTest, AppNameIsReservedFilenameWithDot) {
+// Test that an app name containing '.' characters has them replaced with '_' on
+// Windows 7 when used as a filename for its launcher.
+TEST_F(WebAppFileHandlerRegistrationWinTest, AppNameContainsDot) {
   std::set<std::string> file_extensions({"txt"});
   AppId app_id("app_id");
 
-  // "con" is a reserved filename on Windows, and appending '_' won't make it
-  // legitimate (because any text after '.' is interpreted as part of a file
-  // extension), so all '.' characters should be replaced with '_'.
-  std::string app_name("con.nect");
+  // "some.app.name" should become "some_app_name" on Windows 7.
+  std::string app_name("some.app.name");
   base::FilePath app_specific_launcher_path =
-      CreateDataDirectoryAndGetLauncherPathForApp(profile(), app_id,
-                                                  "con_nect");
+      CreateDataDirectoryAndGetLauncherPathForApp(
+          profile(), app_id,
+          base::win::GetVersion() > base::win::Version::WIN7 ? "some.app.name"
+                                                             : "some_app_name");
 
   RegisterFileHandlersWithOs(app_id, app_name, profile(),
                              /*file_extensions*/ {"txt"}, /*mime_types=*/{});