From 1b7d52d6dfb074f8343d930cf8ac9eaed49bc381 Mon Sep 17 00:00:00 2001
From: Dan Murphy <dmurph@chromium.org>
Date: Tue, 19 May 2026 17:15:13 -0700
Subject: [PATCH] [PWA] Prevent OS shortcut hijacking on Windows

When a PWA title is updated, we update the OS shortcuts. If a shortcut
with the new title already exists:
- If it belongs to another app/profile, we must not overwrite it. We
  create a unique shortcut (e.g. "Title (1).lnk").
- On subsequent updates, we want to reuse this unique shortcut rather
  than generating new ones (like "Title (2).lnk") or toggling.

This CL introduces GetShortcutUpdatePath to handle this. It reuses the
existing unique shortcut path if it is already owned by the app.

TAG=agy
CONV=cfd00edc-0d8f-4d80-b772-4078ce79fa8f

Bug: 514071775, b:514441606
Change-Id: I02c51511ed4e43b64498f33bb0ff179fadaabb98
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7858492
Reviewed-by: David Bienvenu <davidbienvenu@chromium.org>
Auto-Submit: Daniel Murphy <dmurph@chromium.org>
Commit-Queue: Daniel Murphy <dmurph@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1633212}
---

diff --git a/chrome/browser/web_applications/os_integration/web_app_shortcut_win.cc b/chrome/browser/web_applications/os_integration/web_app_shortcut_win.cc
index 8d30580..4e36c68 100644
--- a/chrome/browser/web_applications/os_integration/web_app_shortcut_win.cc
+++ b/chrome/browser/web_applications/os_integration/web_app_shortcut_win.cc
@@ -27,6 +27,7 @@
 #include "base/logging.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "base/win/shortcut.h"
@@ -331,6 +332,40 @@
   return Result::kOk;
 }
 
+// Returns the path to use for updating an existing shortcut.
+// `desired_path` is the ideal path (based on the new title).
+// `profile_path` and `app_id` identify the app.
+// `current_path` is the path of the shortcut currently being updated.
+//
+// If `desired_path` is not taken, or is already taken by this app, we use it.
+// Otherwise, we look for a unique path like "desired_path (N)".
+// To avoid toggling between "desired_path (1)" and "desired_path (2)" on
+// subsequent updates (if "desired_path" remains taken by another app), we
+// reuse `current_path` if it matches one of the unique path candidates.
+base::FilePath GetShortcutUpdatePath(const base::FilePath& desired_path,
+                                     const base::FilePath& profile_path,
+                                     const webapps::AppId& app_id,
+                                     const base::FilePath& current_path) {
+  if (!base::PathExists(desired_path)) {
+    return desired_path;
+  }
+  if (IsAppShortcutForProfile(desired_path, profile_path, app_id)) {
+    return desired_path;
+  }
+
+  for (int i = 1;; ++i) {
+    const base::FilePath candidate =
+        desired_path.InsertBeforeExtensionASCII(base::StringPrintf(" (%d)", i));
+    if (!base::PathExists(candidate)) {
+      return candidate;
+    }
+    if (IsAppShortcutForProfile(candidate, profile_path, app_id) &&
+        candidate == current_path) {
+      return candidate;
+    }
+  }
+}
+
 Result UpdateShortcuts(const base::FilePath& web_app_path,
                        const base::FilePath& profile_path,
                        const std::u16string& old_app_title,
@@ -346,10 +381,21 @@
   const bool title_change = old_app_title != shortcut_info.title;
   Result result = Result::kOk;
   for (const auto& shortcut : all_shortcuts) {
-    const base::FilePath new_shortcut =
+    const base::FilePath desired_shortcut =
         shortcut.DirName()
             .Append(GetSanitizedFileName(shortcut_info.title))
             .AddExtension(installer::kLnkExt);
+
+    const base::FilePath new_shortcut = GetShortcutUpdatePath(
+        desired_shortcut, profile_path, shortcut_info.app_id, shortcut);
+
+    if (new_shortcut.empty()) {
+      DVLOG(1) << "Error finding unique path for shortcut "
+               << shortcut_info.title;
+      result = Result::kError;
+      continue;
+    }
+
     if (title_change) {
       // When the title changes, it is not enough to rename the shortcut file,
       // because it still points to the old icon. Update the icon file before
@@ -364,15 +410,17 @@
                      shortcut.value().c_str(), nullptr);
     }
 
-    base::File::Error error = base::File::Error::FILE_OK;
-    bool success = base::ReplaceFile(shortcut, new_shortcut, &error);
-    if (success) {
-      SHChangeNotify(SHCNE_RENAMEITEM, SHCNF_PATH | SHCNF_FLUSHNOWAIT,
-                     shortcut.value().c_str(), new_shortcut.value().c_str());
-    } else {
-      DVLOG(1) << "Error renaming shortcut " << shortcut_info.title
-               << " error code " << std::hex << error;
-      result = Result::kError;
+    if (shortcut != new_shortcut) {
+      base::File::Error error = base::File::Error::FILE_OK;
+      const bool success = base::ReplaceFile(shortcut, new_shortcut, &error);
+      if (success) {
+        SHChangeNotify(SHCNE_RENAMEITEM, SHCNF_PATH | SHCNF_FLUSHNOWAIT,
+                       shortcut.value().c_str(), new_shortcut.value().c_str());
+      } else {
+        DVLOG(1) << "Error renaming shortcut " << shortcut_info.title
+                 << " error code " << std::hex << error;
+        result = Result::kError;
+      }
     }
   }
 
@@ -412,28 +460,40 @@
   // like PKEY_ItemName on the shortcut does not seem to change the shortcut's
   // properties, as determined by shortcut_properties.py.
   for (const auto& shortcut : pinned_shortcuts) {
-    const base::FilePath new_shortcut =
+    const base::FilePath desired_shortcut =
         shortcut.DirName()
             .Append(GetSanitizedFileName(shortcut_info.title))
             .AddExtension(installer::kLnkExt);
 
+    const base::FilePath new_shortcut = GetShortcutUpdatePath(
+        desired_shortcut, profile_path, shortcut_info.app_id, shortcut);
+
+    if (new_shortcut.empty()) {
+      DVLOG(1) << "Error finding unique path for shortcut "
+               << shortcut_info.title;
+      result = Result::kError;
+      continue;
+    }
+
     if (title_change) {
       UpdateIconFileForShortcut(web_app_path, shortcut, shortcut_info.title);
       SHChangeNotify(SHCNE_UPDATEITEM, SHCNF_PATH | SHCNF_FLUSH,
                      shortcut.value().c_str(), nullptr);
     }
 
-    base::File::Error error = base::File::Error::FILE_OK;
-    bool success = base::ReplaceFile(shortcut, new_shortcut, &error);
-    if (success) {
-      // Tell the Windows shell the shortcut has been renamed. Using SHCNF_FLUSH
-      // also works, but blocking is probably a bad idea.
-      SHChangeNotify(SHCNE_RENAMEITEM, SHCNF_PATH | SHCNF_FLUSHNOWAIT,
-                     shortcut.value().c_str(), new_shortcut.value().c_str());
-    } else {
-      DVLOG(1) << "Error renaming shortcut " << shortcut_info.title
-               << " error code " << std::hex << error;
-      result = Result::kError;
+    if (shortcut != new_shortcut) {
+      base::File::Error error = base::File::Error::FILE_OK;
+      const bool success = base::ReplaceFile(shortcut, new_shortcut, &error);
+      if (success) {
+        // Tell the Windows shell the shortcut has been renamed. Using
+        // SHCNF_FLUSH also works, but blocking is probably a bad idea.
+        SHChangeNotify(SHCNE_RENAMEITEM, SHCNF_PATH | SHCNF_FLUSHNOWAIT,
+                       shortcut.value().c_str(), new_shortcut.value().c_str());
+      } else {
+        DVLOG(1) << "Error renaming shortcut " << shortcut_info.title
+                 << " error code " << std::hex << error;
+        result = Result::kError;
+      }
     }
   }
   // SHCNE_ALLEVENTS prevents the WebApp icon on the taskbar from becoming a
diff --git a/chrome/browser/web_applications/os_integration/web_app_shortcut_win_unittest.cc b/chrome/browser/web_applications/os_integration/web_app_shortcut_win_unittest.cc
index e2c4d7b..b2fac5f1 100644
--- a/chrome/browser/web_applications/os_integration/web_app_shortcut_win_unittest.cc
+++ b/chrome/browser/web_applications/os_integration/web_app_shortcut_win_unittest.cc
@@ -469,4 +469,220 @@
       base::FilePath(FILE_PATH_LITERAL("test\\web\\app\\dir\\_COM1.ico")));
 }
 
+TEST_F(WebAppShortcutWinTest, UpdatePlatformShortcuts_PreventHijacking) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath shortcut_dir = temp_dir.GetPath();
+
+  const base::FilePath profile_path(FILE_PATH_LITERAL("test/profile/web_app"));
+  const base::FilePath::StringType profile_name =
+      profile_path.BaseName().value();
+

diff --git a/chrome/browser/web_applications/os_integration/web_app_shortcut_win_unittest.cc b/chrome/browser/web_applications/os_integration/web_app_shortcut_win_unittest.cc
index e2c4d7b..b2fac5f1 100644
--- a/chrome/browser/web_applications/os_integration/web_app_shortcut_win_unittest.cc
+++ b/chrome/browser/web_applications/os_integration/web_app_shortcut_win_unittest.cc
@@ -469,4 +469,220 @@
       base::FilePath(FILE_PATH_LITERAL("test\\web\\app\\dir\\_COM1.ico")));
 }
 
+TEST_F(WebAppShortcutWinTest, UpdatePlatformShortcuts_PreventHijacking) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath shortcut_dir = temp_dir.GetPath();
+
+  const base::FilePath profile_path(FILE_PATH_LITERAL("test/profile/web_app"));
+  const base::FilePath::StringType profile_name =
+      profile_path.BaseName().value();
+
+  // Create a shortcut for App A (kWebAppId) with name "old title.lnk".
+  const base::FilePath::StringType old_title = FILE_PATH_LITERAL("old title");
+  const base::FilePath old_shortcut_path =
+      GetShortcutPath(shortcut_dir, old_title);
+  ASSERT_TRUE(
+      CreateTestAppShortcut(old_shortcut_path, profile_name, kWebAppId));
+
+  // Create a colliding shortcut for App B (kWebAppId2) with name "new
+  // title.lnk".
+  const base::FilePath::StringType new_title = FILE_PATH_LITERAL("new title");
+  const base::FilePath colliding_shortcut_path =
+      GetShortcutPath(shortcut_dir, new_title);
+  ASSERT_TRUE(
+      CreateTestAppShortcut(colliding_shortcut_path, profile_name, kWebAppId2));
+
+  // Create an icon file for the web app (App A) with old title.
+  const base::FilePath icon_file = GetIconFilePath(shortcut_dir, u"old title");
+  gfx::ImageFamily image_family;
+  image_family.Add(gfx::Image(CreateDefaultApplicationIcon(5)));
+  EXPECT_TRUE(CheckAndSaveIcon(icon_file, image_family,
+                               /*refresh_shell_icon_cache=*/false));
+
+  // Update App A (kWebAppId) with a new title "new title".
+  ShortcutInfo shortcut_info;
+  shortcut_info.title = u"new title";
+  shortcut_info.profile_path = profile_path;
+  shortcut_info.profile_name = base::WideToUTF8(profile_name);
+  shortcut_info.app_id = kWebAppId;
+  shortcut_info.favicon = std::move(image_family);
+
+  UpdatePlatformShortcuts(shortcut_dir, u"old title",
+                          /*user_specified_locations=*/std::nullopt,
+                          shortcut_info);
+
+  // Assert that App B's shortcut is not overwritten (still owned by
+  // kWebAppId2).
+  EXPECT_TRUE(base::PathExists(colliding_shortcut_path));
+  EXPECT_TRUE(IsAppShortcutForProfile(colliding_shortcut_path, profile_path,
+                                      kWebAppId2));
+
+  // Assert that App A's shortcut is renamed to "new title (1).lnk" (owned by
+  // kWebAppId).
+  const base::FilePath unique_shortcut_path =
+      GetShortcutPath(shortcut_dir, FILE_PATH_LITERAL("new title (1)"));
+  EXPECT_TRUE(base::PathExists(unique_shortcut_path));
+  EXPECT_TRUE(
+      IsAppShortcutForProfile(unique_shortcut_path, profile_path, kWebAppId));
+
+  // Assert that App A's old shortcut "old title.lnk" is deleted.
+  EXPECT_FALSE(base::PathExists(old_shortcut_path));
+}
+
+TEST_F(WebAppShortcutWinTest, UpdatePlatformShortcuts_OverwriteOwnApp) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath shortcut_dir = temp_dir.GetPath();
+
+  const base::FilePath profile_path(FILE_PATH_LITERAL("test/profile/web_app"));
+  const base::FilePath::StringType profile_name =
+      profile_path.BaseName().value();
+
+  // Create a shortcut for App A (kWebAppId) with name "old title.lnk".
+  const base::FilePath::StringType old_title = FILE_PATH_LITERAL("old title");
+  const base::FilePath old_shortcut_path =
+      GetShortcutPath(shortcut_dir, old_title);
+  ASSERT_TRUE(
+      CreateTestAppShortcut(old_shortcut_path, profile_name, kWebAppId));
+
+  // Create another shortcut for App A (kWebAppId) with name "new title.lnk".
+  const base::FilePath::StringType new_title = FILE_PATH_LITERAL("new title");
+  const base::FilePath colliding_shortcut_path =
+      GetShortcutPath(shortcut_dir, new_title);
+  ASSERT_TRUE(
+      CreateTestAppShortcut(colliding_shortcut_path, profile_name, kWebAppId));
+
+  // Create an icon file for the web app (App A) with old title.
+  const base::FilePath icon_file = GetIconFilePath(shortcut_dir, u"old title");
+  gfx::ImageFamily image_family;
+  image_family.Add(gfx::Image(CreateDefaultApplicationIcon(5)));
+  EXPECT_TRUE(CheckAndSaveIcon(icon_file, image_family,
+                               /*refresh_shell_icon_cache=*/false));
+
+  // Update App A (kWebAppId) with a new title "new title".
+  ShortcutInfo shortcut_info;
+  shortcut_info.title = u"new title";
+  shortcut_info.profile_path = profile_path;
+  shortcut_info.profile_name = base::WideToUTF8(profile_name);
+  shortcut_info.app_id = kWebAppId;
+  shortcut_info.favicon = std::move(image_family);
+
+  UpdatePlatformShortcuts(shortcut_dir, u"old title",
+                          /*user_specified_locations=*/std::nullopt,
+                          shortcut_info);
+
+  // Assert that "new title.lnk" is overwritten and remains owned by kWebAppId.
+  EXPECT_TRUE(base::PathExists(colliding_shortcut_path));
+  EXPECT_TRUE(IsAppShortcutForProfile(colliding_shortcut_path, profile_path,
+                                      kWebAppId));
+
+  // Assert that no "new title (1).lnk" is created.
+  const base::FilePath unique_shortcut_path =
+      GetShortcutPath(shortcut_dir, FILE_PATH_LITERAL("new title (1)"));
+  EXPECT_FALSE(base::PathExists(unique_shortcut_path));
+
+  // Assert that App A's old shortcut "old title.lnk" is deleted.
+  EXPECT_FALSE(base::PathExists(old_shortcut_path));
+}
+
+TEST_F(WebAppShortcutWinTest, UpdatePlatformShortcuts_RepeatedUpdateAndRename) {
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+  const base::FilePath shortcut_dir = temp_dir.GetPath();
+
+  const base::FilePath profile_path(FILE_PATH_LITERAL("test/profile/web_app"));
+  const base::FilePath::StringType profile_name =
+      profile_path.BaseName().value();
+
+  // Create a shortcut for App A (kWebAppId) with name "old title.lnk".
+  const base::FilePath::StringType old_title = FILE_PATH_LITERAL("old title");
+  const base::FilePath old_shortcut_path =
+      GetShortcutPath(shortcut_dir, old_title);
+  ASSERT_TRUE(
+      CreateTestAppShortcut(old_shortcut_path, profile_name, kWebAppId));
+
+  // Create a colliding shortcut for App B (kWebAppId2) with name "new
+  // title.lnk".
+  const base::FilePath::StringType new_title = FILE_PATH_LITERAL("new title");
+  const base::FilePath colliding_shortcut_path =
+      GetShortcutPath(shortcut_dir, new_title);
+  ASSERT_TRUE(
+      CreateTestAppShortcut(colliding_shortcut_path, profile_name, kWebAppId2));
+
+  // Create an icon file for the web app (App A) with old title.
+  const base::FilePath icon_file = GetIconFilePath(shortcut_dir, u"old title");
+  gfx::ImageFamily image_family;
+  image_family.Add(gfx::Image(CreateDefaultApplicationIcon(5)));
+  EXPECT_TRUE(CheckAndSaveIcon(icon_file, image_family,
+                               /*refresh_shell_icon_cache=*/false));
+
+  // Update App A (kWebAppId) with a new title "new title".
+  ShortcutInfo shortcut_info;
+  shortcut_info.title = u"new title";
+  shortcut_info.profile_path = profile_path;
+  shortcut_info.profile_name = base::WideToUTF8(profile_name);
+  shortcut_info.app_id = kWebAppId;
+  shortcut_info.favicon = std::move(image_family);
+
+  UpdatePlatformShortcuts(shortcut_dir, u"old title",
+                          /*user_specified_locations=*/std::nullopt,
+                          shortcut_info);
+
+  // Assert that App B's shortcut is not overwritten.
+  EXPECT_TRUE(base::PathExists(colliding_shortcut_path));
+  EXPECT_TRUE(IsAppShortcutForProfile(colliding_shortcut_path, profile_path,
+                                      kWebAppId2));
+
+  // Assert that App A's shortcut is renamed to "new title (1).lnk".
+  const base::FilePath unique_shortcut_path =
+      GetShortcutPath(shortcut_dir, FILE_PATH_LITERAL("new title (1)"));
+  EXPECT_TRUE(base::PathExists(unique_shortcut_path));
+  EXPECT_TRUE(
+      IsAppShortcutForProfile(unique_shortcut_path, profile_path, kWebAppId));
+  EXPECT_FALSE(base::PathExists(old_shortcut_path));
+
+  // --- Repeated Update ---
+  // Update App A (kWebAppId) with the same title "new title" again.
+  // We need to re-create the icon family because std::move emptied it.
+  gfx::ImageFamily image_family2;
+  image_family2.Add(gfx::Image(CreateDefaultApplicationIcon(5)));
+  shortcut_info.favicon = std::move(image_family2);
+
+  // Note: old_app_title is now "new title" because that is the current title.
+  UpdatePlatformShortcuts(shortcut_dir, u"new title",
+                          /*user_specified_locations=*/std::nullopt,
+                          shortcut_info);
+
+  // Assert that we still use "new title (1).lnk" and NOT "new title (2).lnk".
+  EXPECT_TRUE(base::PathExists(unique_shortcut_path));
+  EXPECT_TRUE(
+      IsAppShortcutForProfile(unique_shortcut_path, profile_path, kWebAppId));
+
+  const base::FilePath unique_shortcut_path2 =
+      GetShortcutPath(shortcut_dir, FILE_PATH_LITERAL("new title (2)"));
+  EXPECT_FALSE(base::PathExists(unique_shortcut_path2));
+
+  // --- Rename ---
+  // Rename App A to "new new title".
+  shortcut_info.title = u"new new title";
+  gfx::ImageFamily image_family3;
+  image_family3.Add(gfx::Image(CreateDefaultApplicationIcon(5)));
+  shortcut_info.favicon = std::move(image_family3);
+
+  UpdatePlatformShortcuts(shortcut_dir, u"new title",
+                          /*user_specified_locations=*/std::nullopt,
+                          shortcut_info);
+
+  // Assert that "new title (1).lnk" is deleted.
+  EXPECT_FALSE(base::PathExists(unique_shortcut_path));
+
+  // Assert that "new new title.lnk" is created.
+  const base::FilePath final_shortcut_path =
+      GetShortcutPath(shortcut_dir, FILE_PATH_LITERAL("new new title"));
+  EXPECT_TRUE(base::PathExists(final_shortcut_path));
+  EXPECT_TRUE(
+      IsAppShortcutForProfile(final_shortcut_path, profile_path, kWebAppId));
+}
+
 }  // namespace web_app::internals