Stop geolocation watch updates upon permission revocation

Previously, once a site obtained a geolocation watch (either through an already existing grant or through a new permission prompt flow), it kept receiving location updates in the registered watch callback within the same session (i.e. for as long as the user didn't close the tab or performed a navigation). This remained the case, even after the grant was revoked. This CL fixes this behaviour.

(cherry picked from commit 80e3530)

Bug: 1462313
Change-Id: I3151beb1a05bcf7169e952e908b67ae9dbcf7105
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4853341
Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1197210}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4891608
Commit-Queue: Florian Jacky <fjacky@chromium.org>
Owners-Override: Andreea Costinas <acostinas@google.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5993@{#768}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}

chrome/browser/geolocation/geolocation_browsertest.cc

@@ -26,6 +26,8 @@
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/browser/page_specific_content_settings.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/content_settings/core/common/content_settings.h"
+#include "components/network_session_configurator/common/network_switches.h"
 #include "components/permissions/features.h"
 #include "components/permissions/permission_request_manager.h"
 #include "components/permissions/test/permission_request_observer.h"
@@ -172,20 +174,29 @@ class GeolocationBrowserTest : public InProcessBrowserTest {
   void set_html_for_tests(const std::string& html_for_tests) {
     html_for_tests_ = html_for_tests;
   }
+  std::string html_for_tests() { return html_for_tests_; }
   const GURL& iframe_url(size_t i) const { return iframe_urls_[i]; }
   double fake_latitude() const { return fake_latitude_; }
   double fake_longitude() const { return fake_longitude_; }
 
   GURL GetTestURL() const {
     // Return the current test url for the top level page.
-    return embedded_test_server()->GetURL(html_for_tests_);
+    return https_test_server_.GetURL(html_for_tests_);
+  }
+
+  GURL GetTestURLForHostname(std::string hostname) const {
+    // Return the current test url for the top level page.
+    return https_test_server_.GetURL(hostname, html_for_tests_);
   }
 
   content::WebContents* web_contents() {
     return current_browser()->tab_strip_model()->GetActiveWebContents();
   }
 
-  // Initializes the test server and navigates to the initial url.
+  // Initializes the test server and navigates to `target`
+  void Initialize(InitializationOptions options, GURL target);
+
+  // Initializes the test server and navigates to the return value of GetTestUrl
   void Initialize(InitializationOptions options);
 
   // Loads two iframes with different origins: http://127.0.0.1 and
@@ -229,6 +240,9 @@ class GeolocationBrowserTest : public InProcessBrowserTest {
   bool SetPositionAndWaitUntilUpdated(double latitude, double longitude);
 
  protected:
+  // BrowserTestBase:
+  void SetUpCommandLine(base::CommandLine* command_line) override;
+
   // The values used for the position override.
   double fake_latitude_ = 1.23;
   double fake_longitude_ = 4.56;
@@ -237,12 +251,19 @@ class GeolocationBrowserTest : public InProcessBrowserTest {
   // The current Browser as set in Initialize. May be for an incognito profile.
   raw_ptr<Browser, AcrossTasksDanglingUntriaged> current_browser_ = nullptr;
 
+  // The https server used for the tests
+  net::EmbeddedTestServer https_test_server_{
+      net::EmbeddedTestServer::TYPE_HTTPS};
+
  private:
   // Calls watchPosition() in JavaScript and accepts or denies the resulting
   // permission request. Returns the JavaScript response.
   std::string WatchPositionAndRespondToPermissionRequest(
       permissions::PermissionRequestManager::AutoResponseType request_response);
 
+  // The embedded test server handle.
+  net::test_server::EmbeddedTestServerHandle test_server_handle_;
+
   // The path element of a URL referencing the html content for this test.
   std::string html_for_tests_ = "/geolocation/simple.html";
 
@@ -263,24 +284,33 @@ GeolocationBrowserTest::GeolocationBrowserTest()
               fake_longitude_)) {}
 
 void GeolocationBrowserTest::SetUpOnMainThread() {
-  ASSERT_TRUE(embedded_test_server()->Start());
+  current_browser_ = browser();
+  host_resolver()->AddRule("*", "127.0.0.1");
+  https_test_server_.SetSSLConfig(net::EmbeddedTestServer::CERT_TEST_NAMES);
+  https_test_server_.AddDefaultHandlers(GetChromeTestDataDir());
+  ASSERT_TRUE(test_server_handle_ = https_test_server_.StartAndReturnHandle());
 }
 
 void GeolocationBrowserTest::TearDownInProcessBrowserTestFixture() {
   LOG(WARNING) << "TearDownInProcessBrowserTestFixture. Test Finished.";
 }
 
 void GeolocationBrowserTest::Initialize(InitializationOptions options) {
+  Initialize(options, GetTestURL());
+}
+
+void GeolocationBrowserTest::Initialize(InitializationOptions options,
+                                        GURL target) {
   if (options == INITIALIZATION_OFFTHERECORD) {
-    current_browser_ = OpenURLOffTheRecord(browser()->profile(), GetTestURL());
+    current_browser_ = OpenURLOffTheRecord(browser()->profile(), target);
   } else {
     current_browser_ = browser();
     if (options == INITIALIZATION_NEWTAB)
       chrome::NewTab(current_browser_);
   }
   ASSERT_TRUE(current_browser_);
   if (options != INITIALIZATION_OFFTHERECORD)
-    ASSERT_TRUE(ui_test_utils::NavigateToURL(current_browser_, GetTestURL()));
+    ASSERT_TRUE(ui_test_utils::NavigateToURL(current_browser_, target));
 
   // By default the main frame is used for JavaScript execution.
   SetFrameForScriptExecution("");
@@ -310,8 +340,7 @@ void GeolocationBrowserTest::SetFrameForScriptExecution(
 }
 
 HostContentSettingsMap* GeolocationBrowserTest::GetHostContentSettingsMap() {
-  return HostContentSettingsMapFactory::GetForProfile(
-      current_browser()->profile());
+  return HostContentSettingsMapFactory::GetForProfile(browser()->profile());
 }
 
 bool GeolocationBrowserTest::WatchPositionAndGrantPermission() {
@@ -381,6 +410,12 @@ bool GeolocationBrowserTest::SetPositionAndWaitUntilUpdated(double latitude,
              .ExtractString() == "geoposition-updated";
 }
 
+void GeolocationBrowserTest::SetUpCommandLine(base::CommandLine* command_line) {
+  // For using an HTTPS server.
+  base::CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kIgnoreCertificateErrors);
+}
+
 // Tests ----------------------------------------------------------------------
 
 IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest, DisplaysPrompt) {
@@ -607,17 +642,10 @@ class GeolocationPrerenderBrowserTest : public GeolocationBrowserTest {
   ~GeolocationPrerenderBrowserTest() override = default;
 
   void SetUp() override {
-    prerender_helper_.RegisterServerRequestMonitor(embedded_test_server());
+    prerender_helper_.RegisterServerRequestMonitor(&https_test_server_);
     GeolocationBrowserTest::SetUp();
   }
 
-  // GeolocationBrowserTest:
-  void SetUpOnMainThread() override {
-    current_browser_ = browser();
-    host_resolver()->AddRule("*", "127.0.0.1");
-    GeolocationBrowserTest::SetUpOnMainThread();
-  }
-
  protected:
   content::test::PrerenderTestHelper prerender_helper_;
 };
@@ -626,7 +654,7 @@ IN_PROC_BROWSER_TEST_F(GeolocationPrerenderBrowserTest,
                        DeferredBeforePrerenderActivation) {
   // Navigate to an initial page.
   ASSERT_TRUE(ui_test_utils::NavigateToURL(
-      current_browser(), embedded_test_server()->GetURL("/empty.html")));
+      current_browser(), https_test_server_.GetURL("/empty.html")));
 
   // Start a prerender with the geolocation test URL.
   int host_id = prerender_helper_.AddPrerender(GetTestURL());
@@ -656,3 +684,106 @@ IN_PROC_BROWSER_TEST_F(GeolocationPrerenderBrowserTest,
   observer.Wait();
   EXPECT_TRUE(observer.request_shown());
 }
+
+IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest,
+                       GrantToDenyStopsGeolocationWatch) {
+  ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT));
+  ASSERT_TRUE(WatchPositionAndGrantPermission());
+  ExpectPosition(fake_latitude(), fake_longitude());
+
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      GetTestURL(), GetTestURL(), ContentSettingsType::GEOLOCATION,
+      CONTENT_SETTING_BLOCK);
+
+  EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
+  ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
+}
+
+IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest,
+                       GrantToRevokeStopsGeolocationWatch) {
+  ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT));
+  ASSERT_TRUE(WatchPositionAndGrantPermission());
+  ExpectPosition(fake_latitude(), fake_longitude());
+
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      GetTestURL(), GetTestURL(), ContentSettingsType::GEOLOCATION,
+      CONTENT_SETTING_ASK);
+
+  EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
+  ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
+}
+
+IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest,
+                       GrantToDenyToGrantDoesNotRemainBlocked) {
+  // https://crbug.com/1475743
+  ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT));
+  ASSERT_TRUE(WatchPositionAndGrantPermission());
+  ExpectPosition(fake_latitude(), fake_longitude());
+
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      GetTestURL(), GetTestURL(), ContentSettingsType::GEOLOCATION,
+      CONTENT_SETTING_BLOCK);
+
+  EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
+  ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
+
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      GetTestURL(), GetTestURL(), ContentSettingsType::GEOLOCATION,
+      CONTENT_SETTING_ALLOW);
+  ASSERT_TRUE(WatchPositionAndGrantPermission());
+  ExpectPosition(fake_latitude(), fake_longitude());
+}
+
+IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest,
+                       ToggleToDenyDoesNotLeakCrossOrigin) {
+  GURL a_test_gurl = GetTestURLForHostname("a.test");
+  GURL b_test_gurl = GetTestURLForHostname("b.test");
+
+  // Open a.test and allow geolocation.
+  ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT, a_test_gurl));
+  ASSERT_TRUE(WatchPositionAndGrantPermission());
+  ExpectPosition(fake_latitude(), fake_longitude());
+
+  // Toggle to deny on a.test
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      a_test_gurl, a_test_gurl, ContentSettingsType::GEOLOCATION,
+      CONTENT_SETTING_BLOCK);
+  EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
+  ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
+
+  // Navigate to b.test, allow geolocation and verify we can access it.
+  ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT, b_test_gurl));
+  ASSERT_TRUE(WatchPositionAndGrantPermission());
+  ExpectPosition(fake_latitude(), fake_longitude());
+}
+
+IN_PROC_BROWSER_TEST_F(GeolocationBrowserTest,
+                       ToggleToDenyDoesNotOverrideGrantOnOtherOrigin) {
+  GURL a_test_gurl = GetTestURLForHostname("a.test");
+  GURL b_test_gurl = GetTestURLForHostname("b.test");
+
+  // Set up geolocation as allowed on b.test
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      b_test_gurl, b_test_gurl, ContentSettingsType::GEOLOCATION,
+      CONTENT_SETTING_ALLOW);
+
+  // Open a.test and allow geolocation.
+  ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT, a_test_gurl));
+  ASSERT_TRUE(WatchPositionAndGrantPermission());
+  ExpectPosition(fake_latitude(), fake_longitude());
+
+  // Toggle grant to block.
+  GetHostContentSettingsMap()->SetContentSettingDefaultScope(
+      a_test_gurl, a_test_gurl, ContentSettingsType::GEOLOCATION,
+      CONTENT_SETTING_BLOCK);
+
+  EXPECT_TRUE(SetPositionAndWaitUntilUpdated(1, 2));
+  ExpectValueFromScript(GetErrorCodePermissionDenied(), "geoGetLastError()");
+
+  // Navigate to b.test which has geolocation enabled
+  ASSERT_NO_FATAL_FAILURE(Initialize(INITIALIZATION_DEFAULT, b_test_gurl));
+
+  // Ensure no prompt is shown on geolocation access and expect position.
+  WatchPositionAndObservePermissionRequest(/*request_should_display=*/false);
+  ExpectPosition(fake_latitude(), fake_longitude());
+}

chrome/browser/installable/installed_webapp_geolocation_bridge.cc

@@ -96,6 +96,18 @@ void InstalledWebappGeolocationBridge::ClearOverride() {
   StartListeningForUpdates();
 }
 
+void InstalledWebappGeolocationBridge::OnPermissionRevoked() {
+  if (!position_callback_.is_null()) {
+    std::move(position_callback_)
+        .Run(device::mojom::GeopositionResult::NewError(
+            device::mojom::GeopositionError::New(
+                device::mojom::GeopositionErrorCode::kPermissionDenied,
+                /*error_message=*/"User denied Geolocation",
+                /*error_technical=*/"")));
+  }
+  position_callback_.Reset();
+}
+
 void InstalledWebappGeolocationBridge::OnConnectionError() {
   context_->OnConnectionError(this);
 

chrome/browser/installable/installed_webapp_geolocation_bridge.h

@@ -38,6 +38,9 @@ class InstalledWebappGeolocationBridge : public device::mojom::Geolocation {
   void SetOverride(device::mojom::GeopositionResultPtr result);
   void ClearOverride();
 
+  // Handles permission revocations during active use.
+  void OnPermissionRevoked();
+
   // Called by JNI on its thread looper.
   void OnNewLocationAvailable(JNIEnv* env,
                               jdouble latitude,
@@ -53,6 +56,8 @@ class InstalledWebappGeolocationBridge : public device::mojom::Geolocation {
                               jdouble speed);
   void OnNewErrorAvailable(JNIEnv* env, jstring message);
 
+  const GURL& url() { return url_; }
+
  private:
   // device::mojom::Geolocation:
   void SetHighAccuracy(bool high_accuracy) override;

chrome/browser/installable/installed_webapp_geolocation_context.cc

@@ -5,8 +5,9 @@
 #include "chrome/browser/installable/installed_webapp_geolocation_context.h"
 
 #include <utility>
-
+#include "base/containers/cxx20_erase_vector.h"
 #include "chrome/browser/installable/installed_webapp_geolocation_bridge.h"
+#include "url/origin.h"
 
 InstalledWebappGeolocationContext::InstalledWebappGeolocationContext() =
     default;
@@ -25,6 +26,18 @@ void InstalledWebappGeolocationContext::BindGeolocation(
     impls_.back()->StartListeningForUpdates();
 }
 
+void InstalledWebappGeolocationContext::OnPermissionRevoked(
+    const url::Origin& origin) {
+  base::EraseIf(impls_, [&origin](const auto& impl) {
+    if (!origin.IsSameOriginWith(impl->url())) {
+      return false;
+    }
+    // Invoke the position callback with kPermissionDenied before removing.
+    impl->OnPermissionRevoked();
+    return true;
+  });
+}
+
 void InstalledWebappGeolocationContext::OnConnectionError(
     InstalledWebappGeolocationBridge* impl) {
   for (auto it = impls_.begin(); it != impls_.end(); ++it) {

chrome/browser/installable/installed_webapp_geolocation_context.h

@@ -12,6 +12,7 @@
 #include "services/device/public/mojom/geolocation.mojom.h"
 #include "services/device/public/mojom/geolocation_context.mojom.h"
 #include "services/device/public/mojom/geoposition.mojom.h"
+#include "url/origin.h"
 
 class InstalledWebappGeolocationBridge;
 
@@ -32,6 +33,7 @@ class InstalledWebappGeolocationContext
   void BindGeolocation(
       mojo::PendingReceiver<device::mojom::Geolocation> receiver,
       const GURL& requesting_url) override;
+  void OnPermissionRevoked(const url::Origin& origin) override;
   void SetOverride(device::mojom::GeopositionResultPtr result) override;
   void ClearOverride() override;
 

chrome/test/data/geolocation/basic_geolocation.js

@@ -11,6 +11,13 @@ var position_updated = false;
 const callbackStatuses = new ResultQueue();
 const geopositionUpdates = new ResultQueue();
 
+function resetState() {
+  last_position = 0;
+  last_error = 0;
+  watch_id = 0;
+  position_initialized = false;
+  position_updated = false;
+}
 function geoSuccessCallback(position) {
   last_position = position;
 }
@@ -36,18 +43,21 @@ function geoErrorCallbackWithResponse(error) {
   callbackStatuses.push('request-callback-error');
 }
 function geoStart() {
+  resetState();
   watch_id = navigator.geolocation.watchPosition(
       geoSuccessCallback, geoErrorCallback,
       {maximumAge:600000, timeout:100000, enableHighAccuracy:true});
   return watch_id;
 }
 function geoStartWithAsyncResponse() {
+  resetState();
   navigator.geolocation.watchPosition(
     geoSuccessCallbackWithResponse, geoErrorCallbackWithResponse,
       {maximumAge:600000, timeout:100000, enableHighAccuracy:true});
   return callbackStatuses.pop();
 }
 function geoStartWithSyncResponse() {
+  resetState();
   navigator.geolocation.watchPosition(
             geoSuccessCallback, geoErrorCallback,
             {maximumAge:600000, timeout:100000, enableHighAccuracy:true});

chrome/test/data/geolocation/iframe_controller.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-var iframe_hosts = ['http://127.0.0.1', 'http://localhost'];
+var iframe_hosts = ['https://127.0.0.1', 'https://localhost'];
 function getIFrameSrc(iframe_id) {
   var port = location.port;
   var path = location.pathname.substring(0, location.pathname.lastIndexOf('/'));