From c7cecfc63f6dd3c81d05b48a2e83aa6a18402408 Mon Sep 17 00:00:00 2001 From: Marcio Vinicius Spiridigliozzi da Silva Leite Date: Thu, 6 Aug 2026 09:09:28 -0300 Subject: [PATCH] fix: stop an ordinary link popup reloading the service behind it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closing a popup reloaded the service that opened it, whatever the popup was. That is right for sign-in and wrong for everything else. Reloading exists because the popup shares the service's data store: once sign-in finishes the session cookies are already here, and the main view only needs a reload to leave its signed-out page. But a popup is also how an ordinary link opens. Glance at a link from a chat service, close the window, and the service reloads underneath you — scroll position gone, a half-typed message gone, whatever the page held but never sent gone. A steady, visible cost, paid for a case that comes up rarely. Two signals separate them, and either is enough: - The page closed itself. OAuth popups finish by calling window.close(); a link window is closed by the user. This is what carries flows through providers not listed by name, such as a company's own Okta or Keycloak. - The popup was opened at a known sign-in gateway. A service asking the user to sign in again opens straight at its provider, so the first URL is the gateway. This covers a flow the user closes by hand once it is done, which some providers leave to them. The second signal reads the opening URL only, never the rest of the navigation chain. I had it watching the whole chain first, and measured that being wrong: opening an Azure portal link from Teams starts at portal.azure.com and redirects through login.microsoftonline.com for SSO, so the chain marked it a sign-in and reloaded Teams on close — the very bug this commit is about. Reading only the opening URL keeps the re-auth case (which opens at the gateway) and drops the link case. Both are covered by tests, the second named for how it was found. When neither signal holds, the popup was a link, and the service is left alone. The signal is cleared with the rest of the popup state, so a link opened after a sign-in does not inherit its predecessor's reason to reload. The trade this makes is mild and recoverable in the direction it fails: a sign-in that neither starts at a listed gateway nor closes itself leaves the service on its signed-out page until the user hits reload, once. Against reloading on every link close, that is the better way round. The rule is factored into shouldReloadOpener so it can be tested without a live WKWebView. --- Chorus/Views/WebView/WebViewCoordinator.swift | 69 +++++++++++++++++-- ChorusTests/ChorusTests.swift | 47 +++++++++++++ 2 files changed, 110 insertions(+), 6 deletions(-) diff --git a/Chorus/Views/WebView/WebViewCoordinator.swift b/Chorus/Views/WebView/WebViewCoordinator.swift index de21d9b..df6a80e 100644 --- a/Chorus/Views/WebView/WebViewCoordinator.swift +++ b/Chorus/Views/WebView/WebViewCoordinator.swift @@ -13,6 +13,11 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK /// reload it once the sign-in popup closes (see reloadOpenerAfterPopup). private weak var openerWebView: WKWebView? + /// Whether the current popup was *opened at* a known sign-in gateway. Set + /// once, from the URL that opened it — see `shouldReloadOpener` for why the + /// rest of the navigation chain is deliberately not consulted. + private var popupOpenedAtAuthHost = false + /// Fallback URL to load if the WebContent process crashes before any /// navigation has committed (so `webView.reload()` has nothing to retry). var fallbackURL: URL? @@ -438,6 +443,11 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK // needs to reload to leave its signed-out page. openerWebView = webView + // The sign-in signal, taken from the URL that opened the popup and not + // touched again: a service asking the user to sign in again opens + // straight at its provider. See `shouldReloadOpener`. + popupOpenedAtAuthHost = navigationAction.request.url?.host.map(Self.isAuthHost) ?? false + // CRITICAL: Use the configuration passed in — it inherits the parent's data store let popup = WKWebView(frame: .zero, configuration: configuration) popup.navigationDelegate = self @@ -500,14 +510,56 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK @objc private func popupWindowWillClose(_ notification: Notification) { guard let window = notification.object as? NSWindow, window === popupWindow else { return } - reloadOpenerAfterPopup() + // Closed by the user (red button / ⌘W), not by the page. + reloadOpenerAfterPopup(selfClosed: false) cleanupPopup() } - /// After a sign-in / OAuth popup closes, the shared data store already holds - /// the new session cookies, but the service's main web view is still on its - /// signed-out page. Reload it so the user lands on the authenticated app. - private func reloadOpenerAfterPopup() { + /// Whether closing a popup should reload the service that opened it. + /// + /// Reloading exists for sign-in: the popup shares the service's data store, + /// so once sign-in finishes the session cookies are already here and the + /// main view only needs a reload to leave its signed-out page. + /// + /// Reloading *unconditionally* is wrong, because a popup is also how an + /// ordinary link opens. Glance at a link from a chat service, close the + /// window, and the service reloads underneath you — losing scroll position, + /// a half-typed message, and whatever else the page held but never sent. + /// That is a steady, visible cost paid for a case that comes up rarely. + /// + /// Two signals separate them, and either one is enough: + /// + /// - **The page closed itself.** OAuth popups finish by calling + /// `window.close()`; a link window is closed by the user. This is the + /// signal that carries flows through identity providers we don't list, + /// such as a company's own Okta or Keycloak. + /// - **The popup was opened at a known sign-in gateway.** A service asking + /// the user to sign in again opens straight at its provider, so the very + /// first URL is the gateway. This covers a flow the user closes by hand + /// once it is done, which some providers leave to them. + /// + /// The second signal reads the *opening* URL only, never the rest of the + /// navigation chain, and that distinction is the whole point. Plenty of + /// ordinary links pass through a sign-in gateway on their way somewhere + /// else: opening an Azure portal link from Teams starts at + /// `portal.azure.com` and redirects through `login.microsoftonline.com` for + /// SSO. Watching the chain counts that as a sign-in and reloads the service + /// — the exact bug this function exists to fix, measured happening. + /// + /// When neither signal holds, the popup was a link, and the service is left + /// alone. The failure mode this trades into is mild and recoverable: a + /// sign-in that neither starts at a listed gateway nor closes itself leaves + /// the service on its signed-out page until the user hits reload, once. + nonisolated static func shouldReloadOpener(selfClosed: Bool, openedAtAuthHost: Bool) -> Bool { + selfClosed || openedAtAuthHost + } + + /// Reloads the service that opened the popup, when the rule above says to. + private func reloadOpenerAfterPopup(selfClosed: Bool) { + guard Self.shouldReloadOpener( + selfClosed: selfClosed, + openedAtAuthHost: popupOpenedAtAuthHost + ) else { return } guard let opener = openerWebView else { return } if opener.url != nil { opener.reload() @@ -534,11 +586,16 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK // Give the next popup its own crash budget — a prior popup's tally must // not shorten the backoff for an unrelated sign-in opened soon after. popupCrashTimestamps = [] + // Likewise the sign-in signal: a link popup opened after a sign-in must + // not inherit its predecessor's reason to reload the service. + popupOpenedAtAuthHost = false } func webViewDidClose(_ webView: WKWebView) { if webView === popupWebView { - reloadOpenerAfterPopup() + // The page called window.close() on itself — the shape an OAuth + // popup takes when it finishes. + reloadOpenerAfterPopup(selfClosed: true) cleanupPopup() } } diff --git a/ChorusTests/ChorusTests.swift b/ChorusTests/ChorusTests.swift index 213e152..a4a24a5 100644 --- a/ChorusTests/ChorusTests.swift +++ b/ChorusTests/ChorusTests.swift @@ -1562,6 +1562,53 @@ final class ChorusTests: XCTestCase { XCTAssertFalse(ServiceInstance(label: "X", url: "https://x.test", forceDarkMode: false).isForceDarkModeEnabled) } + // MARK: - Reloading the opener after a popup closes + + func testUserClosingALinkPopupLeavesTheServiceAlone() { + // The regression this guards: glance at a link opened from a chat + // service, close the window, and the service used to reload underneath + // you — losing scroll position and anything typed but not sent. + XCTAssertFalse(WebViewCoordinator.shouldReloadOpener( + selfClosed: false, openedAtAuthHost: false)) + } + + func testSelfClosingPopupReloadsTheService() { + // An OAuth popup finishes by calling window.close(). This is what + // carries sign-in through providers we don't list by name — a company's + // own Okta or Keycloak. + XCTAssertTrue(WebViewCoordinator.shouldReloadOpener( + selfClosed: true, openedAtAuthHost: false)) + } + + func testHandClosedSignInStillReloadsTheService() { + // A service asking the user to sign in again opens straight at its + // provider, so the opening URL is the gateway. Some providers leave the + // last click to the user, and that flow must still reload. + XCTAssertTrue(WebViewCoordinator.shouldReloadOpener( + selfClosed: false, openedAtAuthHost: true)) + } + + func testLinkThatMerelyRedirectsThroughSSOLeavesTheServiceAlone() { + // Measured on a real machine: opening an Azure portal link from Teams + // starts at portal.azure.com and redirects through + // login.microsoftonline.com for SSO. Judging by the navigation chain + // counted that as a sign-in and reloaded Teams on close. Only the + // opening URL is consulted, so a link like this stays a link. + XCTAssertFalse(WebViewCoordinator.isAuthHost("portal.azure.com")) + XCTAssertFalse(WebViewCoordinator.shouldReloadOpener( + selfClosed: false, openedAtAuthHost: false)) + } + + func testKnownAuthGatewaysAreRecognisedIncludingSubdomains() { + XCTAssertTrue(WebViewCoordinator.isAuthHost("login.microsoftonline.com")) + XCTAssertTrue(WebViewCoordinator.isAuthHost("accounts.google.com")) + // A subdomain of a gateway still counts. + XCTAssertTrue(WebViewCoordinator.isAuthHost("eu.login.microsoftonline.com")) + // An ordinary link target does not. + XCTAssertFalse(WebViewCoordinator.isAuthHost("teams.cloud.microsoft")) + XCTAssertFalse(WebViewCoordinator.isAuthHost("example.com")) + } + // MARK: - Link routing (belongsToService) func testBelongsToServiceKeepsSlackWorkspacesInApp() {