From 1ee24b59e7d9fa17c7da0fab74ce8f21ac1b0998 Mon Sep 17 00:00:00 2001 From: Sebastian Hengst Date: Tue, 13 May 2025 13:57:24 +0000 Subject: [PATCH] Bug 1962454 - Pass origin instead of host/port to Windows notification r=nalexander,win-reviewers,firefox-desktop-core-reviewers ,gstoll This is to match NotificationDB that uses origin. Differential Revision: https://phabricator.services.mozilla.com/D236586 --- .../components/BrowserContentHandler.sys.mjs | 19 ++- browser/components/tests/browser/browser.toml | 3 + .../browser/browser_handle_notification.js | 127 ++++++++++++++++++ .../components/alerts/AlertNotification.cpp | 5 + toolkit/components/alerts/nsAlertsUtils.cpp | 11 ++ toolkit/components/alerts/nsAlertsUtils.h | 6 + .../components/alerts/nsIAlertsService.idl | 6 + widget/windows/ToastNotificationHandler.cpp | 6 +- .../tests/unit/test_windows_alert_service.js | 32 ++--- 9 files changed, 191 insertions(+), 24 deletions(-) create mode 100644 browser/components/tests/browser/browser_handle_notification.js diff --git a/browser/components/BrowserContentHandler.sys.mjs b/browser/components/BrowserContentHandler.sys.mjs index e72feb538c0d..2bf92f41acbf 100644 --- a/browser/components/BrowserContentHandler.sys.mjs +++ b/browser/components/BrowserContentHandler.sys.mjs @@ -1394,11 +1394,16 @@ nsDefaultCommandLineHandler.prototype = { // window to perform the action in. let winForAction; - if ( - !tagWasHandled && - notificationData?.launchUrl && - !opaqueRelaunchData - ) { + // Fall back to launchUrl to not break notifications opened from + // previous builds after browser updates, as such notification would + // still have the old field. + let origin = notificationData?.origin ?? notificationData?.launchUrl; + + if (!tagWasHandled && origin && !opaqueRelaunchData) { + let originPrincipal = + Services.scriptSecurityManager.createContentPrincipalFromOrigin( + origin + ); // Unprivileged Web Notifications contain a launch URL and are // handled slightly differently than privileged notifications with // actions. If the tag was not handled, then the notification was @@ -1406,7 +1411,9 @@ nsDefaultCommandLineHandler.prototype = { // fallback behavior. let { uri, principal } = resolveURIInternal( cmdLine, - notificationData.launchUrl + // TODO(krosylight): We should handle origin suffix to open the + // relevant container. See bug 1945501. + originPrincipal.originNoSuffix ); if (cmdLine.state != Ci.nsICommandLine.STATE_INITIAL_LAUNCH) { // Try to find an existing window and load our URI into the current diff --git a/browser/components/tests/browser/browser.toml b/browser/components/tests/browser/browser.toml index 0cb4fe542d64..a9b8b36883c9 100644 --- a/browser/components/tests/browser/browser.toml +++ b/browser/components/tests/browser/browser.toml @@ -36,6 +36,9 @@ run-if = ["os == 'win'"] ["browser_forced_colors.js"] +["browser_handle_notification.js"] +run-if = ["os == 'win'"] + ["browser_initial_tab_remoteType.js"] https_first_disabled = true diff --git a/browser/components/tests/browser/browser_handle_notification.js b/browser/components/tests/browser/browser_handle_notification.js new file mode 100644 index 000000000000..18114beb4077 --- /dev/null +++ b/browser/components/tests/browser/browser_handle_notification.js @@ -0,0 +1,127 @@ +/* Any copyright is dedicated to the Public Domain. +http://creativecommons.org/publicdomain/zero/1.0/ */ +"use strict"; + +function createCmdLine(tag, action, state) { + return Cu.createCommandLine( + [ + "--notification-windowsTag", + tag, + "--notification-windowsAction", + JSON.stringify(action), + ], + null, + state + ); +} + +function runCmdLine(cmdLine) { + let cmdLineHandler = Cc["@mozilla.org/browser/final-clh;1"].getService( + Ci.nsICommandLineHandler + ); + cmdLineHandler.handle(cmdLine); +} + +function simulateNotificationClickWithExistingWindow(action) { + let cmdLine = createCmdLine( + "dummyTag", + action, + Ci.nsICommandLine.STATE_REMOTE_AUTO + ); + runCmdLine(cmdLine); +} + +function simulateNotificationClickWithNewWindow(action) { + let cmdLine = createCmdLine( + "dummyTag", + action, + Ci.nsICommandLine.STATE_INITIAL_LAUNCH + ); + runCmdLine(cmdLine); +} + +add_task(async function test_basic() { + let newTabPromise = BrowserTestUtils.waitForNewTab( + gBrowser, + "https://example.com/" + ); + + simulateNotificationClickWithExistingWindow({ + action: "", + origin: "https://example.com", + }); + + let newTab = await newTabPromise; + ok(newTab, "New tab should be opened."); + BrowserTestUtils.removeTab(newTab); +}); + +// launchUrl was used pre-140, we can remove it when we are confident enough +// that there's no old notification with launchUrl lying around anymore +add_task(async function test_legacy_launchUrl() { + let newTabPromise = BrowserTestUtils.waitForNewTab( + gBrowser, + "https://example.com/" + ); + + simulateNotificationClickWithExistingWindow({ + action: "", + launchUrl: "https://example.com", + }); + + let newTab = await newTabPromise; + ok(newTab, "New tab should be opened."); + BrowserTestUtils.removeTab(newTab); +}); + +add_task(async function test_invalid_origin_with_path() { + let newTabPromise = BrowserTestUtils.waitForNewTab( + gBrowser, + "https://example.com/" + ); + + simulateNotificationClickWithExistingWindow({ + action: "", + origin: "https://example.com/example/", + }); + + let newTab = await newTabPromise; + ok(newTab, "New tab should be opened."); + BrowserTestUtils.removeTab(newTab); +}); + +add_task(async function test_user_context() { + let newTabPromise = BrowserTestUtils.waitForNewTab( + gBrowser, + "https://example.com/" + ); + + simulateNotificationClickWithExistingWindow({ + action: "", + origin: "https://example.com^userContextId=1", + }); + + let newTab = await newTabPromise; + registerCleanupFunction(() => BrowserTestUtils.removeTab(newTab)); + ok(newTab, "New tab should be opened."); + + // TODO(krosylight): We want to make sure this opens on the right container. + // See bug 1945501. + is(newTab.userContextId, 0, "The default user context ID is used (for now)."); +}); + +add_task(async function test_basic_initial_load() { + let newWinPromise = BrowserTestUtils.waitForNewWindow({ + url: "https://example.com/", + anyWindow: true, + }); + + simulateNotificationClickWithNewWindow({ + action: "", + origin: "https://example.com", + }); + + let newWin = await newWinPromise; + ok(newWin, "New window should be opened."); + BrowserTestUtils.closeWindow(newWin); +}); diff --git a/toolkit/components/alerts/AlertNotification.cpp b/toolkit/components/alerts/AlertNotification.cpp index 98a093d7aea9..bec229fe0e66 100644 --- a/toolkit/components/alerts/AlertNotification.cpp +++ b/toolkit/components/alerts/AlertNotification.cpp @@ -238,6 +238,11 @@ AlertNotification::GetSource(nsAString& aSource) { return NS_OK; } +NS_IMETHODIMP +AlertNotification::GetOrigin(nsACString& aOrigin) { + return nsAlertsUtils::GetOrigin(mPrincipal, aOrigin); +} + NS_IMETHODIMP AlertNotification::GetOpaqueRelaunchData(nsAString& aOpaqueRelaunchData) { aOpaqueRelaunchData = mOpaqueRelaunchData; diff --git a/toolkit/components/alerts/nsAlertsUtils.cpp b/toolkit/components/alerts/nsAlertsUtils.cpp index b63b368c75a3..8dc25137c2d5 100644 --- a/toolkit/components/alerts/nsAlertsUtils.cpp +++ b/toolkit/components/alerts/nsAlertsUtils.cpp @@ -19,6 +19,7 @@ bool nsAlertsUtils::IsActionablePrincipal(nsIPrincipal* aPrincipal) { /* static */ void nsAlertsUtils::GetSourceHostPort(nsIPrincipal* aPrincipal, nsAString& aHostPort) { + aHostPort.Truncate(); if (!IsActionablePrincipal(aPrincipal)) { return; } @@ -28,3 +29,13 @@ void nsAlertsUtils::GetSourceHostPort(nsIPrincipal* aPrincipal, } CopyUTF8toUTF16(hostPort, aHostPort); } + +/* static */ +nsresult nsAlertsUtils::GetOrigin(nsIPrincipal* aPrincipal, + nsACString& aOrigin) { + aOrigin.Truncate(); + if (!IsActionablePrincipal(aPrincipal)) { + return NS_ERROR_NOT_AVAILABLE; + } + return aPrincipal->GetOrigin(aOrigin); +} diff --git a/toolkit/components/alerts/nsAlertsUtils.h b/toolkit/components/alerts/nsAlertsUtils.h index c126769de7f9..e06bdeb48a65 100644 --- a/toolkit/components/alerts/nsAlertsUtils.h +++ b/toolkit/components/alerts/nsAlertsUtils.h @@ -25,5 +25,11 @@ class nsAlertsUtils final { * empty string if |aPrincipal| is not actionable. */ static void GetSourceHostPort(nsIPrincipal* aPrincipal, nsAString& aHostPort); + + /** + * Sets |aOrigin| to the origin from |aPrincipal|, or an error if |aPrincipal| + * is not actionable. + */ + static nsresult GetOrigin(nsIPrincipal* aPrincipal, nsACString& aOrigin); }; #endif /* nsAlertsUtils_h */ diff --git a/toolkit/components/alerts/nsIAlertsService.idl b/toolkit/components/alerts/nsIAlertsService.idl index 68d003816566..3bde689de91a 100644 --- a/toolkit/components/alerts/nsIAlertsService.idl +++ b/toolkit/components/alerts/nsIAlertsService.idl @@ -219,6 +219,12 @@ interface nsIAlertNotification : nsISupports */ readonly attribute AString source; + /** + * The origin of the originating page, or an empty string if the alert is not + * actionable. This corresponds to `nsIPrincipal.origin`. + */ + readonly attribute ACString origin; + /** * On Windows, chrome-privileged notifications -- i.e., those with a * non-actionable principal -- can have `opaqueRelaunchData`. This data will diff --git a/widget/windows/ToastNotificationHandler.cpp b/widget/windows/ToastNotificationHandler.cpp index e4cd609feb33..9325a01fc9ae 100644 --- a/widget/windows/ToastNotificationHandler.cpp +++ b/widget/windows/ToastNotificationHandler.cpp @@ -342,8 +342,10 @@ nsString ToastNotificationHandler::ActionArgsJSONString( w.StringProperty("privilegedName", NS_ConvertUTF16toUTF8(mName)); } } else { - if (!mHostPort.IsEmpty()) { - w.StringProperty("launchUrl", NS_ConvertUTF16toUTF8(mHostPort)); + nsAutoCString origin; + nsresult rv = mAlertNotification->GetOrigin(origin); + if (NS_SUCCEEDED(rv)) { + w.StringProperty("origin", origin); } } diff --git a/widget/windows/tests/unit/test_windows_alert_service.js b/widget/windows/tests/unit/test_windows_alert_service.js index 0ba0d2a4d4a1..5b2acd2ba021 100644 --- a/widget/windows/tests/unit/test_windows_alert_service.js +++ b/widget/windows/tests/unit/test_windows_alert_service.js @@ -195,7 +195,7 @@ function testAlert(when, { serverEnabled, profD, isBackgroundTaskMode } = {}) { ? "" : ``; - let parsedSettingsAction = hostport => { + let parsedSettingsAction = origin => { if (isBackgroundTaskMode) { return []; } @@ -209,8 +209,8 @@ function testAlert(when, { serverEnabled, profD, isBackgroundTaskMode } = {}) { { action: "settings", }, - hostport && { - launchUrl: hostport, + origin && { + origin, } ) ), @@ -219,7 +219,7 @@ function testAlert(when, { serverEnabled, profD, isBackgroundTaskMode } = {}) { ]; }; - let parsedSnoozeAction = hostport => { + let parsedSnoozeAction = (hostport, origin) => { let content = `Disable notifications from ${hostport}`; return [ content, @@ -230,8 +230,8 @@ function testAlert(when, { serverEnabled, profD, isBackgroundTaskMode } = {}) { { action: "snooze", }, - hostport && { - launchUrl: hostport, + origin && { + origin, } ) ), @@ -414,8 +414,8 @@ function testAlert(when, { serverEnabled, profD, isBackgroundTaskMode } = {}) { ); // But content unprivileged alerts can't use `windowsSystemActivationType`. - let launchUrl = "https://example.com/foo/bar.html"; - const principaluri = Services.io.newURI(launchUrl); + let path = "https://example.com/foo/bar.html"; + const principaluri = Services.io.newURI(path); const principal = Services.scriptSecurityManager.createContentPrincipal( principaluri, {} @@ -436,19 +436,19 @@ function testAlert(when, { serverEnabled, profD, isBackgroundTaskMode } = {}) { { launch: parsedArgumentString({ action: "", - launchUrl: principaluri.hostPort, + origin: principal.origin, }), actions: Object.fromEntries( [ - parsedSnoozeAction(principaluri.hostPort), - parsedSettingsAction(principaluri.hostPort), + parsedSnoozeAction(principal.hostPort, principal.origin), + parsedSettingsAction(principal.origin), [ "dismissTitle", { content: "dismissTitle", arguments: parsedArgumentString({ action: "dismiss", - launchUrl: principaluri.hostPort, + origin: principal.origin, }), }, ], @@ -458,7 +458,7 @@ function testAlert(when, { serverEnabled, profD, isBackgroundTaskMode } = {}) { content: "snoozeTitle", arguments: parsedArgumentString({ action: "snooze", - launchUrl: principaluri.hostPort, + origin: principal.origin, }), }, ], @@ -514,12 +514,12 @@ function testAlert(when, { serverEnabled, profD, isBackgroundTaskMode } = {}) { { launch: parsedArgumentString({ action: "", - launchUrl: principaluri.hostPort, + origin: principal.origin, }), actions: Object.fromEntries( [ - parsedSnoozeAction(principaluri.hostPort), - parsedSettingsAction(principaluri.hostPort), + parsedSnoozeAction(principal.hostPort, principal.origin), + parsedSettingsAction(principal.origin), ].filter(x => x.length) ), },