From f46b3636d6ea358a519be33702851e16d8e4a285 Mon Sep 17 00:00:00 2001
From: Jagadish C K <jagadishck@google.com>
Date: Tue, 07 Jul 2026 01:03:04 -0700
Subject: [PATCH] [data_sharing] Escape access token in preview request URL

PreviewServerProxy::GetSharedDataPreview() built the request query
string by substituting the raw access token into a template and
assigning the result via SetQueryStr(), so reserved characters in the
token were treated as query separators and could spill into additional
parameters. Build the query with net::AppendQueryParameter() instead,
matching how DataSharingServiceImpl already serializes the same
GroupToken fields.

Also fix FieldTrialPreviewServerProxyTest to pass a proper base URL for
the preview_service_base_url param; it previously passed the full
expected URL and only worked because SetQueryStr() overwrote the bogus
query component.

Bug: b:504226770
Change-Id: Ia4720dc90a96172595a0b8a59909f4a8d76d03e3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8016283
Commit-Queue: Jagadish C K (xWF) <jagadishck@google.com>
Reviewed-by: Chad Duffin <chadduffin@google.com>
Reviewed-by: Siddhartha S <ssid@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1657752}
---

diff --git a/components/data_sharing/internal/preview_server_proxy.cc b/components/data_sharing/internal/preview_server_proxy.cc
index 5b453d4..b8a9719d 100644
--- a/components/data_sharing/internal/preview_server_proxy.cc
+++ b/components/data_sharing/internal/preview_server_proxy.cc
@@ -26,6 +26,7 @@
 #include "components/sync/protocol/entity_specifics.pb.h"
 #include "components/sync/protocol/shared_tab_group_data_specifics.pb.h"
 #include "google_apis/common/base_requests.h"
+#include "net/base/url_util.h"
 #include "net/http/http_request_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
@@ -310,20 +311,10 @@
   std::string url_str = GetPreviewServerURLString();
   url_str.append("/").append(shared_entities_preview_path);
   GURL url = GURL(url_str);
-
-  // Query string in the URL to get shared entnties preview. {token} needs to
-  // be replaced by the caller. {pageSize} can be configured through finch.
-  const std::string kQueryString =
-      "accessToken={token}&pageToken=&pageSize={pageSize}";
-  std::string query_str = kQueryString;
-  base::ReplaceFirstSubstringAfterOffset(&query_str, 0, "{token}",
-                                         group_token.access_token);
-  base::ReplaceFirstSubstringAfterOffset(
-      &query_str, 0, "{pageSize}",
-      base::NumberToString(kPreviewDataSize.Get()));
-  GURL::Replacements replacements;
-  replacements.SetQueryStr(query_str);
-  url = url.ReplaceComponents(replacements);
+  url = net::AppendQueryParameter(url, "accessToken", group_token.access_token);
+  url = net::AppendQueryParameter(url, "pageToken", "");
+  url = net::AppendQueryParameter(url, "pageSize",
+                                  base::NumberToString(kPreviewDataSize.Get()));
   auto fetcher = CreateEndpointFetcher(url);
   auto* const fetcher_ptr = fetcher.get();
 
diff --git a/components/data_sharing/internal/preview_server_proxy_unittest.cc b/components/data_sharing/internal/preview_server_proxy_unittest.cc
index 750c588..140b05f 100644
--- a/components/data_sharing/internal/preview_server_proxy_unittest.cc
+++ b/components/data_sharing/internal/preview_server_proxy_unittest.cc
@@ -22,6 +22,7 @@
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "components/sync/base/command_line_switches.h"
 #include "components/sync/base/data_type.h"
+#include "net/base/url_util.h"
 #include "net/http/http_status_code.h"
 #include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -60,6 +61,7 @@
     "collaborations/"
     "cmVzb3VyY2VzLzEyMzQ1NjcvZS8xMTExMTExMTExMTExMTE/dataTypes/-/"
     "sharedEntities:preview?accessToken=abcdefg&pageToken=&pageSize=550";
+const char kFieldTrialServiceBaseUrl[] = "https://test.com";
 const char kExpectedUrlFieldTrial[] =
     "https://test.com/"
     "collaborations/"
@@ -301,6 +303,40 @@
   QueryAndWaitForResponse(syncer::DataType::SHARED_TAB_GROUP_DATA);
 }
 
+TEST_F(PreviewServerProxyTest,
+       TestGetSharedDataPreview_AccessTokenWithReservedChars) {
+  // The access token is opaque to the client and may contain characters that
+  // are reserved in a URL query component. Ensure it is sent as a single
+  // `accessToken` query parameter rather than spilling into additional
+  // parameters.
+  const std::string kToken = "abc&pageSize=1&extra=1";
+  fetcher_->SetFetchResponse(kTabGroupResponse);
+
+  GURL request_url;
+  EXPECT_CALL(*server_proxy_, CreateEndpointFetcher(_))
+      .WillOnce([&](const GURL& url) {
+        request_url = url;
+        return std::move(fetcher_);
+      });
+
+  base::RunLoop run_loop;
+  server_proxy_->GetSharedDataPreview(
+      GroupToken(GroupId(kCollaborationId), kToken),
+      /*data_type=*/std::nullopt,
+      base::BindOnce(
+          [](const DataSharingService::SharedDataPreviewOrFailureOutcome&
+                 result) { ASSERT_TRUE(result.has_value()); })
+          .Then(run_loop.QuitClosure()));
+  run_loop.Run();
+
+  std::string value;
+  ASSERT_TRUE(net::GetValueForKeyInQuery(request_url, "accessToken", &value));
+  EXPECT_EQ(value, kToken);
+  EXPECT_FALSE(net::GetValueForKeyInQuery(request_url, "extra", &value));
+  ASSERT_TRUE(net::GetValueForKeyInQuery(request_url, "pageSize", &value));
+  EXPECT_EQ(value, "550");
+}
+
 TEST_F(PreviewServerProxyTest, TestGetSharedDataPreview_TabWithoutGroup) {
   fetcher_->SetFetchResponse(kTabResponse);
   EXPECT_CALL(*server_proxy_, CreateEndpointFetcher(GURL(kExpectedUrl)))
@@ -493,7 +529,7 @@
  public:
   base::FieldTrialParams GetFieldTrialParams() override {
     base::FieldTrialParams params;
-    params["preview_service_base_url"] = kExpectedUrlFieldTrial;
+    params["preview_service_base_url"] = kFieldTrialServiceBaseUrl;
     return params;
   }
 };

diff --git a/components/data_sharing/internal/preview_server_proxy_unittest.cc b/components/data_sharing/internal/preview_server_proxy_unittest.cc
index 750c588..140b05f 100644
--- a/components/data_sharing/internal/preview_server_proxy_unittest.cc
+++ b/components/data_sharing/internal/preview_server_proxy_unittest.cc
@@ -22,6 +22,7 @@
 #include "components/signin/public/identity_manager/identity_test_environment.h"
 #include "components/sync/base/command_line_switches.h"
 #include "components/sync/base/data_type.h"
+#include "net/base/url_util.h"
 #include "net/http/http_status_code.h"
 #include "services/data_decoder/public/cpp/test_support/in_process_data_decoder.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
@@ -60,6 +61,7 @@
     "collaborations/"
     "cmVzb3VyY2VzLzEyMzQ1NjcvZS8xMTExMTExMTExMTExMTE/dataTypes/-/"
     "sharedEntities:preview?accessToken=abcdefg&pageToken=&pageSize=550";
+const char kFieldTrialServiceBaseUrl[] = "https://test.com";
 const char kExpectedUrlFieldTrial[] =
     "https://test.com/"
     "collaborations/"
@@ -301,6 +303,40 @@
   QueryAndWaitForResponse(syncer::DataType::SHARED_TAB_GROUP_DATA);
 }
 
+TEST_F(PreviewServerProxyTest,
+       TestGetSharedDataPreview_AccessTokenWithReservedChars) {
+  // The access token is opaque to the client and may contain characters that
+  // are reserved in a URL query component. Ensure it is sent as a single
+  // `accessToken` query parameter rather than spilling into additional
+  // parameters.
+  const std::string kToken = "abc&pageSize=1&extra=1";
+  fetcher_->SetFetchResponse(kTabGroupResponse);
+
+  GURL request_url;
+  EXPECT_CALL(*server_proxy_, CreateEndpointFetcher(_))
+      .WillOnce([&](const GURL& url) {
+        request_url = url;
+        return std::move(fetcher_);
+      });
+
+  base::RunLoop run_loop;
+  server_proxy_->GetSharedDataPreview(
+      GroupToken(GroupId(kCollaborationId), kToken),
+      /*data_type=*/std::nullopt,
+      base::BindOnce(
+          [](const DataSharingService::SharedDataPreviewOrFailureOutcome&
+                 result) { ASSERT_TRUE(result.has_value()); })
+          .Then(run_loop.QuitClosure()));
+  run_loop.Run();
+
+  std::string value;
+  ASSERT_TRUE(net::GetValueForKeyInQuery(request_url, "accessToken", &value));
+  EXPECT_EQ(value, kToken);
+  EXPECT_FALSE(net::GetValueForKeyInQuery(request_url, "extra", &value));
+  ASSERT_TRUE(net::GetValueForKeyInQuery(request_url, "pageSize", &value));
+  EXPECT_EQ(value, "550");
+}
+
 TEST_F(PreviewServerProxyTest, TestGetSharedDataPreview_TabWithoutGroup) {
   fetcher_->SetFetchResponse(kTabResponse);
   EXPECT_CALL(*server_proxy_, CreateEndpointFetcher(GURL(kExpectedUrl)))
@@ -493,7 +529,7 @@
  public:
   base::FieldTrialParams GetFieldTrialParams() override {
     base::FieldTrialParams params;
-    params["preview_service_base_url"] = kExpectedUrlFieldTrial;
+    params["preview_service_base_url"] = kFieldTrialServiceBaseUrl;
     return params;
   }
 };