Fix LinkedIn external-link redirect handoff in browser pane (#2928)#2930
Fix LinkedIn external-link redirect handoff in browser pane (#2928)#2930austinywang wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR implements full URLRequest propagation through browser navigation paths, enabling preservation of HTTP headers, method, and body when opening links in new tabs and popups, while establishing a shared browser context between opener and popup windows. Changes
Sequence Diagram(s)sequenceDiagram
participant WV as WebView
participant ND as NavigationDelegate
participant BP as BrowserPanel
participant WS as Workspace
participant NBS as NewBrowserSurface
WV->>ND: decidePolicyFor navigationAction<br/>(target="_blank" or window.open)
ND->>ND: Extract request from navigationAction
ND->>BP: requestNavigation(request, .newTab)
BP->>BP: Build BrowserNewTabNavigationSeed<br/>from URLRequest
BP->>WS: newBrowserSurface(initialRequest: seed.initialRequest)
WS->>NBS: Create BrowserPanel<br/>with initialRequest
NBS->>NBS: Configure WebView<br/>Navigate via URLRequest<br/>(preserves headers, method, body)
NBS-->>BP: New surface created
BP-->>ND: Navigation handled
ND-->>WV: Cancel original navigation
sequenceDiagram
participant Parent as Parent WebView
participant PND as PopupNavigationDelegate
participant PWC as BrowserPopupWindowController
participant BPC as BrowserPopupBrowserContext
participant Popup as Popup WebView
Parent->>PND: Popup navigation request
PND->>PWC: openInOpenerTab(request)
PWC->>PWC: Check openerPanel exists
PWC->>BPC: Access shared browserContext<br/>(websiteDataStore, processPool)
BPC-->>PWC: Return context
PWC->>Parent: openerPanel.openLinkInNewTab(request)
Parent->>Parent: New tab created with<br/>shared context & full request
Parent-->>PWC: Acknowledged
PWC-->>Popup: Navigation complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryFixes LinkedIn external-link redirect handoff by propagating the full Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/documentation suggestions with no impact on correctness. The core logic — preserving the full URLRequest through the new-tab handoff and unconditionally inheriting the opener's WebKit browsing context — is correct and consistent with how BrowserUIDelegate already handled this. The initialRequest/initialURL priority in BrowserPanel.init is well-ordered (initialRequest wins), avoiding any double navigation. The only findings are a cosmetic dead-branch in two debug log strings and a test scope note. No P0/P1 issues found. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant NavDelegate as BrowserNavigationDelegate
participant Panel as BrowserPanel
participant Workspace
participant NewPanel as New BrowserPanel
User->>NavDelegate: cmd+click / target=_blank
NavDelegate->>NavDelegate: openRequestInNewTab(navigationAction.request)
NavDelegate->>Panel: requestNavigation(request, .newTab)
Panel->>Panel: openLinkInNewTab(request:)
Panel->>Panel: browserNewTabNavigationSeed(from: request)
Note over Panel: Preserves method/headers/body<br/>Resets cachePolicy to .useProtocolCachePolicy
Panel->>Workspace: newBrowserSurface(url: seed.url, initialRequest: seed.initialRequest)
Workspace->>NewPanel: init(initialURL: url, initialRequest: request)
NewPanel->>NewPanel: navigateWithoutInsecureHTTPPrompt(request: initialRequest)
Note over User,NewPanel: Popup path
User->>Panel: window.open() scripted popup
Panel->>Panel: createFloatingPopup(configuration, windowFeatures)
Panel->>Panel: popupBrowserContext (websiteDataStore + processPool)
Panel->>NavDelegate: BrowserPopupWindowController(browserContext: popupBrowserContext)
Note over NavDelegate: configureWebViewConfiguration unconditional<br/>Nested popups inherit same opener context
Reviews (1): Last reviewed commit: "Preserve browser redirect request contex..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| XCTAssertEqual(seed.initialRequest.httpMethod, "POST") | ||
| XCTAssertEqual(seed.initialRequest.httpBody, body) | ||
| XCTAssertEqual( | ||
| seed.initialRequest.value(forHTTPHeaderField: "Referer"), | ||
| "https://www.linkedin.com/feed/" | ||
| ) | ||
| XCTAssertEqual( | ||
| seed.initialRequest.value(forHTTPHeaderField: "X-Cmux-Test"), | ||
| "keep-me" | ||
| ) | ||
| XCTAssertEqual(seed.initialRequest.cachePolicy, .useProtocolCachePolicy) |
There was a problem hiding this comment.
The test constructs a POST request with httpBody and verifies the seed preserves it — but LinkedIn's redirect interstitials are driven by server-side 302/HTML-meta redirects on GET requests, not POSTs. The POST + body path through navigateWithoutInsecureHTTPPrompt and then into a WKWebView.load(_:) call is untested at the WebKit level, and WebKit commonly drops or ignores non-standard headers (including Referer) on programmatic loads. The test proves the struct properties are propagated correctly, but doesn't confirm WebKit actually replays those values — consider adding a note in the test that this verifies the seed struct only, not the end-to-end navigation fidelity.
| if shouldOpenInNewTab, | ||
| let url = navigationAction.request.url { | ||
| navigationAction.request.url != nil { | ||
| #if DEBUG | ||
| dlog("browser.nav.decidePolicy.action kind=openInNewTab url=\(url.absoluteString)") | ||
| dlog( | ||
| "browser.nav.decidePolicy.action kind=openInNewTab url=\(navigationAction.request.url?.absoluteString ?? "nil")" | ||
| ) | ||
| #endif | ||
| openInNewTab?(url) | ||
| openRequestInNewTab(navigationAction.request) | ||
| decisionHandler(.cancel) | ||
| return | ||
| } |
There was a problem hiding this comment.
?? "nil" after nil-check guard
Both the openInNewTab and openInNewTabFromNilTarget branches now guard with navigationAction.request.url != nil but the debug log inside each branch still uses optional-chain + ?? "nil". Since the nil case can never be reached at that point, the fallback string is dead. A forced unwrap (or a local let url = ... rebinding) would make the intent clearer.
| if shouldOpenInNewTab, | |
| let url = navigationAction.request.url { | |
| navigationAction.request.url != nil { | |
| #if DEBUG | |
| dlog("browser.nav.decidePolicy.action kind=openInNewTab url=\(url.absoluteString)") | |
| dlog( | |
| "browser.nav.decidePolicy.action kind=openInNewTab url=\(navigationAction.request.url?.absoluteString ?? "nil")" | |
| ) | |
| #endif | |
| openInNewTab?(url) | |
| openRequestInNewTab(navigationAction.request) | |
| decisionHandler(.cancel) | |
| return | |
| } | |
| dlog( | |
| "browser.nav.decidePolicy.action kind=openInNewTab url=\(navigationAction.request.url!.absoluteString)" | |
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 2792-2795: initialRequest is currently sent directly to
navigateWithoutInsecureHTTPPrompt(...) and bypasses the insecure-HTTP
warning/allowlist flow that initialURL goes through; change the initialRequest
branch to run through the same insecure-HTTP gate used for initialURL (the same
check/prompt/allowlist path) before setting shouldRenderWebView and navigating,
so that callers passing a plain-HTTP request cannot skip the warning. Locate the
branch handling initialRequest/initialURL and replace the direct call to
navigateWithoutInsecureHTTPPrompt(request: initialRequest, ...) with the same
insecure-HTTP handling logic used for initialURL (invoke that
gate/prompt/allowlist path and only call navigateWithoutInsecureHTTPPrompt if
the gate permits).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1da5434-a96c-4a21-96b8-2f749d44eb6f
📒 Files selected for processing (4)
Sources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftSources/Workspace.swiftcmuxTests/GhosttyConfigTests.swift
| if let initialRequest { | ||
| shouldRenderWebView = true | ||
| navigateWithoutInsecureHTTPPrompt(request: initialRequest, recordTypedNavigation: false) | ||
| } else if let url = initialURL { |
There was a problem hiding this comment.
initialRequest bypasses the insecure-HTTP warning path.
Line 2792 now loads initialRequest via navigateWithoutInsecureHTTPPrompt(...), so any caller that passes a blocked plain-HTTP request skips the warning/allowlist flow that initialURL still gets. Route this branch through the same insecure-HTTP gate first so the new API doesn't silently weaken the protection.
⚠️ Proposed fix
- if let initialRequest {
- shouldRenderWebView = true
- navigateWithoutInsecureHTTPPrompt(request: initialRequest, recordTypedNavigation: false)
+ if let initialRequest {
+ shouldRenderWebView = true
+ if let url = initialRequest.url,
+ shouldBlockInsecureHTTPNavigation(to: url) {
+ presentInsecureHTTPAlert(
+ for: initialRequest,
+ intent: .currentTab,
+ recordTypedNavigation: false
+ )
+ } else {
+ navigateWithoutInsecureHTTPPrompt(
+ request: initialRequest,
+ recordTypedNavigation: false
+ )
+ }
} else if let url = initialURL {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Panels/BrowserPanel.swift` around lines 2792 - 2795, initialRequest
is currently sent directly to navigateWithoutInsecureHTTPPrompt(...) and
bypasses the insecure-HTTP warning/allowlist flow that initialURL goes through;
change the initialRequest branch to run through the same insecure-HTTP gate used
for initialURL (the same check/prompt/allowlist path) before setting
shouldRenderWebView and navigating, so that callers passing a plain-HTTP request
cannot skip the warning. Locate the branch handling initialRequest/initialURL
and replace the direct call to navigateWithoutInsecureHTTPPrompt(request:
initialRequest, ...) with the same insecure-HTTP handling logic used for
initialURL (invoke that gate/prompt/allowlist path and only call
navigateWithoutInsecureHTTPPrompt if the gate permits).
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8f5114f. Configure here.
| if let url = initialURL { | ||
| if let initialRequest { | ||
| shouldRenderWebView = true | ||
| navigateWithoutInsecureHTTPPrompt(request: initialRequest, recordTypedNavigation: false) |
There was a problem hiding this comment.
New tab init bypasses insecure HTTP check
Medium Severity
The openLinkInNewTab(url:) overload now delegates to openLinkInNewTab(request:), which always creates a seed containing a non-nil initialRequest. When the new BrowserPanel is initialized with initialRequest, it calls navigateWithoutInsecureHTTPPrompt instead of navigate(to:). The old navigate(to:) path called shouldBlockInsecureHTTPNavigation and showed the insecure HTTP alert; the new path skips that check entirely. This regresses callers that relied on the new panel's init to perform the check — specifically context menu "Open Link in New Tab" (onContextMenuOpenLinkInNewTab) and popup-to-new-tab via openInOpenerTab — since those paths call openLinkInNewTab directly without a prior insecure HTTP guard. A secondary consequence is that insecureHTTPBypassHostOnce is stored but never consumed, leaving a stale one-time bypass for a future navigation.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 8f5114f. Configure here.
1 participant
Summary
Testing
./scripts/reload.sh --tag issue-2928-linkedin-redirectand./scripts/reload.sh --tag issue-2928-linkedin-redirect --launch, but the local Xcode build on this machine stalled before compile tasks began, so the dev app could not be launched for manual verificationCloses #2928
Note
Medium Risk
Touches browser navigation routing and popup/new-tab creation paths; bugs could alter how links open or which headers/cookies are sent during redirects.
Overview
Fixes new-tab and popup retargeting to carry the full original
URLRequest(includingReferer, custom headers, method, and body) so redirect/interstitial flows like LinkedIn’s don’t lose context.This threads an
initialRequestthroughWorkspace.newBrowserSurface/BrowserPanelcreation, updatesBrowserNavigationDelegate+openLinkInNewTab/popup opener handoff to prefer request-based navigation, and introduces an explicitBrowserPopupBrowserContextto ensure popups always reuse the opener’swebsiteDataStoreandprocessPool.Adds regression tests for request-preserving new-tab seeding and popup context inheritance.
Reviewed by Cursor Bugbot for commit 8f5114f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes LinkedIn external-link redirect handoff by preserving the original request when opening in a new tab or popup, and by making popup WebKit session inheritance unconditional. Fixes #2928.
URLRequest(method, body, headers like Referer) when cmd/middle-clicking or retargeting links to a new tab; plumbsinitialRequestthroughWorkspace.newBrowserSurface.websiteDataStoreandprocessPoolso cookies and OAuth flows work consistently.Written for commit 8f5114f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests