From e93e408653145e906f6990120495639685d23a74 Mon Sep 17 00:00:00 2001 From: Marcio Vinicius Spiridigliozzi da Silva Leite Date: Wed, 5 Aug 2026 16:24:34 -0300 Subject: [PATCH] fix: give a same-service window.open a real window, not nil MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A new-window request whose target belongs to the opening service was collapsed into the opener's web view, and the delegate returned nil. For a link click that is right, and it has to stay that way: Slack's target=_blank workspace links go through this path and should keep loading in place rather than spawning a window each time. For a programmatic window.open() it is wrong. The caller gets a window handle back, and callers test it: const w = window.open(url); if (!w) return; Returning nil reads as "popup blocked", so the page abandons whatever it was starting with no window and no error to show for it. Nothing in the UI says a request was refused, which makes it a hard thing to diagnose from the outside. So narrow the collapse to .linkActivated rather than removing it. A same-service window.open now falls through to a real window, which shares the opener's data store, so a session started in it lands in the right place. The rule moves into shouldLoadNewWindowInPlace so it can be tested without a live WKWebView. Scope, stated plainly: I first reached this while chasing a Teams sign-in failure and believed it was the cause. It was not — logging createWebViewWith showed it was never called during that failure, and the real cause was ITP blocking a third-party cookie, which I have sent separately. This change stands on its own as a correctness fix for window.open; it is not a fix for that bug, and I would rather say so than let the connection be assumed. --- Chorus/Views/WebView/WebViewCoordinator.swift | 43 +++++++++++++++--- ChorusTests/ChorusTests.swift | 45 +++++++++++++++++++ 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/Chorus/Views/WebView/WebViewCoordinator.swift b/Chorus/Views/WebView/WebViewCoordinator.swift index de21d9b..c289f80 100644 --- a/Chorus/Views/WebView/WebViewCoordinator.swift +++ b/Chorus/Views/WebView/WebViewCoordinator.swift @@ -418,13 +418,26 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK windowFeatures: WKWindowFeatures ) -> WKWebView? { // If the new-window request is for the same service — e.g. Slack opening - // a workspace via window.open / target=_blank — load it in the existing - // web view instead of spawning a separate NSWindow. Only genuinely + // a workspace via a target=_blank link — load it in the existing web + // view instead of spawning a separate NSWindow. Only genuinely // cross-service popups (real OAuth sign-in windows to another domain) // fall through and get their own window below. - if let targetHost = navigationAction.request.url?.host, - let openerHost = webView.url?.host, - Self.belongsToService(targetHost, serviceHost: openerHost) { + // + // Restricted to real link clicks. A programmatic `window.open()` hands + // the caller a window handle, and sign-in flows test it: + // + // const w = window.open(url); if (!w) return; + // + // Returning nil there reads as "popup blocked", so the page abandons + // whatever it was starting with no window and no error to show for it. + // Same-service `window.open` therefore falls through to a real window, + // which shares the opener's data store so a session started in it lands + // in the right place. + if Self.shouldLoadNewWindowInPlace( + navigationType: navigationAction.navigationType, + targetHost: navigationAction.request.url?.host, + openerHost: webView.url?.host + ) { webView.load(navigationAction.request) return nil } @@ -956,6 +969,26 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK /// Slack can switch workspaces across *.slack.com in-app — except for /// shared-umbrella domains (see `sharedUmbrellaDomains`) where only the exact /// host matches. Used to decide in-app vs. browser for links and new windows. + /// Whether a new-window request should collapse into the opener's web view + /// instead of getting its own window. + /// + /// Only real link clicks collapse. A programmatic `window.open()` must come + /// back with a window handle: sign-in flows null-check the return value to + /// detect a popup blocker, and a nil answer makes them abandon the flow + /// silently — no window, no error, no request. Factored out so the rule is + /// unit-testable without a live `WKWebView`. + nonisolated static func shouldLoadNewWindowInPlace( + navigationType: WKNavigationType, + targetHost: String?, + openerHost: String? + ) -> Bool { + guard navigationType == .linkActivated, + let targetHost, + let openerHost + else { return false } + return belongsToService(targetHost, serviceHost: openerHost) + } + nonisolated static func belongsToService(_ targetHost: String, serviceHost: String) -> Bool { let target = normalizedHost(targetHost) let service = normalizedHost(serviceHost) diff --git a/ChorusTests/ChorusTests.swift b/ChorusTests/ChorusTests.swift index 213e152..5f99a31 100644 --- a/ChorusTests/ChorusTests.swift +++ b/ChorusTests/ChorusTests.swift @@ -1562,6 +1562,51 @@ final class ChorusTests: XCTestCase { XCTAssertFalse(ServiceInstance(label: "X", url: "https://x.test", forceDarkMode: false).isForceDarkModeEnabled) } + // MARK: - New-window requests (shouldLoadNewWindowInPlace) + + func testClickedSameServiceLinkLoadsInPlace() { + // A target=_blank click inside Slack should reuse the service's own web + // view rather than spawning a window. + XCTAssertTrue(WebViewCoordinator.shouldLoadNewWindowInPlace( + navigationType: .linkActivated, + targetHost: "myteam.slack.com", + openerHost: "app.slack.com" + )) + } + + func testProgrammaticSameServicePopupGetsItsOwnWindow() { + // A page calling window.open() against its own host must still get a + // handle back. Collapsing it in place returned nil, which a caller that + // null-checks the handle reads as a blocked popup — so it gives up + // silently, with no window and no error to show for it. + XCTAssertFalse(WebViewCoordinator.shouldLoadNewWindowInPlace( + navigationType: .other, + targetHost: "teams.cloud.microsoft", + openerHost: "teams.cloud.microsoft" + )) + } + + func testCrossServicePopupGetsItsOwnWindow() { + XCTAssertFalse(WebViewCoordinator.shouldLoadNewWindowInPlace( + navigationType: .linkActivated, + targetHost: "login.microsoftonline.com", + openerHost: "teams.cloud.microsoft" + )) + } + + func testNewWindowWithUnknownHostIsNotCollapsed() { + XCTAssertFalse(WebViewCoordinator.shouldLoadNewWindowInPlace( + navigationType: .linkActivated, + targetHost: nil, + openerHost: "app.slack.com" + )) + XCTAssertFalse(WebViewCoordinator.shouldLoadNewWindowInPlace( + navigationType: .linkActivated, + targetHost: "app.slack.com", + openerHost: nil + )) + } + // MARK: - Link routing (belongsToService) func testBelongsToServiceKeepsSlackWorkspacesInApp() {