Present user with option to proceed anyway on invalid SSL cert error - #3711
Conversation
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a thread-safe in-memory SSL bypass store, detects SSL certificate/secure-connection failures, renders a one-time-token-backed “Proceed Anyway (Unsafe)” button on SSL error pages (with auto-reload), intercepts the bypass action to consume the token and record the host, and uses the store to complete server-trust authentication challenges. ChangesSSL/TLS Error Bypass
Sequence DiagramsequenceDiagram
participant User as User/Browser
participant Nav as NavigationDelegate
participant Store as BrowserSSLErrorBypassStore
participant Auth as AuthChallengeHandler
User->>Nav: Clicks "Proceed Anyway (Unsafe)" (cmux-browser-action://bypass-ssl?token=&url=)
Nav->>Nav: parse `token` and `url`
Nav->>Store: consumePendingToken(token, forHost:)
Store-->>Nav: token valid / token invalid/expired
Nav->>Store: add(host) // when token valid
Nav->>Nav: load(target URL)
Note right of Auth: Later, system presents server-trust challenge
Auth->>Store: check(host)
Store-->>Auth: host found / not found
Auth->>Auth: completeChallenge with URLCredential(trust:) if host found
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 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 |
|
To use Codex here, create a Codex account and connect to github. |
Finally was able to test locally and it works wonderfully... Hopefully we can get it merged. @codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
To use Codex here, create a Codex account and connect to github. |
|
The UI looks great — the warning color on "Proceed Anyway (Unsafe)" clearly communicates the security implications to the user while still making the action accessible. The visual distinction between the safe ("Reload") and unsafe action is well done. I'll kick off a full review of the PR now! ✅ Actions performedReview triggered.
|
Greptile SummaryThis PR adds a "Proceed Anyway (Unsafe)" button to the browser's SSL error interstitial, enabling users to bypass self-signed or corporate-CA certificates that macOS doesn't trust. Bypass tokens are one-time UUIDs tied to the observed leaf-cert SHA-256 and the exact failed
Confidence Score: 4/5The core bypass logic is well-designed and safe; the main concern is that the entire state machine is duplicated into The token, fingerprint, and request-replay machinery in
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant WKWebView
participant NavDelegate as BrowserNavigationDelegate
participant BypassState as BrowserSSLTrustBypassState
participant ErrorPage as BrowserErrorPage
participant MsgHandler as BrowserSSLTrustBypassMessageHandler
User->>WKWebView: Navigate to https://self-signed.internal
WKWebView->>NavDelegate: decidePolicyFor(navigationAction)
NavDelegate->>NavDelegate: recordAttemptedRequest(request)
WKWebView->>NavDelegate: didReceive(challenge: ServerTrust)
NavDelegate->>BypassState: recordObservedServerTrust(trust, scope)
NavDelegate->>WKWebView: .performDefaultHandling (rejects cert)
WKWebView->>NavDelegate: didFailProvisionalNavigation(error)
NavDelegate->>ErrorPage: load(failedURL, retry, sslBypassState)
ErrorPage->>BypassState: createPendingBypassAction(request)
BypassState-->>ErrorPage: "cmux-browser-action://bypass-ssl?token=UUID"
ErrorPage->>WKWebView: loadHTMLString (with data-token button)
NavDelegate->>NavDelegate: "acceptsSSLTrustBypassMessages = true"
User->>WKWebView: Click Proceed Anyway
WKWebView->>MsgHandler: userContentController(didReceive token)
MsgHandler->>NavDelegate: canHandleSSLTrustBypassToken(token)
NavDelegate->>MsgHandler: true
MsgHandler->>NavDelegate: handleSSLTrustBypassToken(token, webView)
NavDelegate->>BypassState: consumePendingBypassToken(token)
BypassState-->>NavDelegate: original URLRequest + registers bypass grant
NavDelegate->>WKWebView: load(originalRequest)
WKWebView->>NavDelegate: didReceive(challenge: ServerTrust)
NavDelegate->>BypassState: isBypassed(scope, fingerprint)?
BypassState-->>NavDelegate: true
NavDelegate->>WKWebView: .useCredential (trust accepted)
WKWebView->>NavDelegate: didCommit then clearAttemptedRequest()
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant WKWebView
participant NavDelegate as BrowserNavigationDelegate
participant BypassState as BrowserSSLTrustBypassState
participant ErrorPage as BrowserErrorPage
participant MsgHandler as BrowserSSLTrustBypassMessageHandler
User->>WKWebView: Navigate to https://self-signed.internal
WKWebView->>NavDelegate: decidePolicyFor(navigationAction)
NavDelegate->>NavDelegate: recordAttemptedRequest(request)
WKWebView->>NavDelegate: didReceive(challenge: ServerTrust)
NavDelegate->>BypassState: recordObservedServerTrust(trust, scope)
NavDelegate->>WKWebView: .performDefaultHandling (rejects cert)
WKWebView->>NavDelegate: didFailProvisionalNavigation(error)
NavDelegate->>ErrorPage: load(failedURL, retry, sslBypassState)
ErrorPage->>BypassState: createPendingBypassAction(request)
BypassState-->>ErrorPage: "cmux-browser-action://bypass-ssl?token=UUID"
ErrorPage->>WKWebView: loadHTMLString (with data-token button)
NavDelegate->>NavDelegate: "acceptsSSLTrustBypassMessages = true"
User->>WKWebView: Click Proceed Anyway
WKWebView->>MsgHandler: userContentController(didReceive token)
MsgHandler->>NavDelegate: canHandleSSLTrustBypassToken(token)
NavDelegate->>MsgHandler: true
MsgHandler->>NavDelegate: handleSSLTrustBypassToken(token, webView)
NavDelegate->>BypassState: consumePendingBypassToken(token)
BypassState-->>NavDelegate: original URLRequest + registers bypass grant
NavDelegate->>WKWebView: load(originalRequest)
WKWebView->>NavDelegate: didReceive(challenge: ServerTrust)
NavDelegate->>BypassState: isBypassed(scope, fingerprint)?
BypassState-->>NavDelegate: true
NavDelegate->>WKWebView: .useCredential (trust accepted)
WKWebView->>NavDelegate: didCommit then clearAttemptedRequest()
Reviews (43): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6388-6390: The current bypass button HTML (bypassButtonHTML) only
round-trips the URL, causing POSTs/headers/body to be lost; change the flow to
serialize and persist the original URLRequest (including HTTP method, headers,
and body) behind a short-lived bypass token instead of passing url=..., update
the button to call "cmux-browser-action://bypass-ssl?token=<token>" (or include
token param alongside escapedURL), and modify the handler for
"cmux-browser-action://bypass-ssl" to: mark the host as bypassed, look up and
deserialize the stored URLRequest for that token, and replay the exact original
request (e.g., via webView.load(request:) or equivalent) so method/headers/body
are preserved; apply the same change to the other similar block referenced
(lines 6448-6457).
- Around line 6448-6459: The bypass-ssl handler currently honors any
cmux-browser-action request; update the logic in the navigationAction handling
block (the if that checks url.scheme == "cmux-browser-action" && url.host ==
"bypass-ssl") to require a one-time nonce/token: when rendering the internal SSL
error page generate and store a pending token in the same store that tracks
pending bypasses (e.g., add a pendingBypassToken in BrowserSSLErrorBypassStore
or the SSL error page renderer), require the incoming query to include url=...
and token=... and only call BrowserSSLErrorBypassStore.shared.addBypass(for:)
and webView.load(...) if the token matches the stored pending token;
consume/clear the token immediately after use and reject
(decisionHandler(.cancel)) if missing, mismatched, expired, or already-consumed.
Ensure token creation happens when the SSL error page is shown and that tokens
are single-use and time-limited.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4a92f4fc-92f8-4d8d-91cf-f4c695e9a470
📒 Files selected for processing (1)
Sources/Panels/BrowserPanel.swift
Greptile SummaryThis PR adds a "Proceed Anyway (Unsafe)" button to the SSL/TLS error page in the in-app browser, backed by a new
Confidence Score: 3/5Not safe to merge as-is: the bypass button injects the failed URL into a JavaScript string without escaping single quotes, which allows JS execution in the WKWebView when navigating to a crafted URL. The SSL error page embeds Sources/Panels/BrowserPanel.swift — both the
|
| Filename | Overview |
|---|---|
| Sources/Panels/BrowserPanel.swift | Adds SSL-bypass UI and a new BrowserSSLErrorBypassStore singleton. Contains a JS string-injection vulnerability in the bypass button's onclick and uses NSLock instead of an actor for the shared bypass store. |
Reviews (2): Last reviewed commit: "Update Sources/Panels/BrowserPanel.swift" | Re-trigger Greptile
… error; function requires a one-time token to prevent sites from being able to spoof user acceptance.
2a936a4 to
2c69776
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/BrowserPanel.swift (1)
6411-6434:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAvoid injecting
failedURLinto inline JavaScript.Line 6433 drops the HTML-escaped URL into a single-quoted JS string. A certificate-failing URL containing
'can break the handler and run attacker-controlled script in this internal page. Precompute the deep link withURLComponentsand inject only the encoded result.🔒 Suggested change
if isSSLError, let failedHost = URL(string: failedURL)?.host { let store = BrowserSSLErrorBypassStore.shared let token = store.createPendingToken(for: failedHost) - let escapedToken = escapeHTML(token) + var bypassComponents = URLComponents() + bypassComponents.scheme = "cmux-browser-action" + bypassComponents.host = "bypass-ssl" + bypassComponents.queryItems = [ + URLQueryItem(name: "token", value: token), + URLQueryItem(name: "url", value: failedURL), + ] + let escapedBypassURL = escapeHTML(bypassComponents.string ?? "") bypassButtonHTML = """ - <button class="bypass" onclick="window.location.href='cmux-browser-action://bypass-ssl?token=\(escapedToken)&url=' + encodeURIComponent('\(escapedURL)')">\(escapedBypassLabel)</button> + <button class="bypass" onclick="window.location.href='\(escapedBypassURL)'">\(escapedBypassLabel)</button> """🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserPanel.swift` around lines 6411 - 6434, The bypass button HTML is vulnerable because failedURL is injected into a single-quoted JS string; instead build the deep link URL server-side using URLComponents (use the existing BrowserSSLErrorBypassStore.shared.createPendingToken(for: failedHost) for token and failedHost), percent‑encode the url parameter via URLComponents/addingPercentEncoding, then pass that fully encoded deep link into bypassButtonHTML (replace the inline "' + encodeURIComponent(...)" approach) and only run escapeHTML on the final deep link string before interpolating it into the button HTML; update the bypassButtonHTML construction in the same block that defines escapedToken and failedHost to use the precomputed encoded deep link.
♻️ Duplicate comments (1)
Sources/Panels/BrowserPanel.swift (1)
6514-6532:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplay the original request after bypass.
Lines 6526-6532 rebuild the retry with
URLRequest(url:), so any SSL-failed POST, custom headers, or body are retried as a plain GET. Persist the originalURLRequestbehind the token andwebView.load(...)that exact request afteraddBypass(for:).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserPanel.swift` around lines 6514 - 6532, The retry currently rebuilds the request as URLRequest(url:) losing method, headers and body; update the flow to persist the original URLRequest when creating/issuing the bypass token and, in BrowserPanel where you call BrowserSSLErrorBypassStore.shared.consumePendingToken(token, for: host) and then addBypass(for:), load the original persisted URLRequest (not a new URLRequest(url:)) via webView.load(originalRequest) so POSTs and custom headers/bodies are replayed intact; modify BrowserSSLErrorBypassStore to accept/store the full URLRequest keyed to the token and provide a retrieval API used here.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6405-6408: The default branch currently assigns
error.localizedDescription to message (in the switch handling page open errors),
which exposes raw upstream/vendor details; replace that assignment with a
generic localized fallback string (e.g., a localized key like
"browser.error.cantOpen.message" or similar) and set isSSLError = false as
before, and if needed log the original error (error or
error.localizedDescription) to the console/logger rather than showing it to the
user; update the code around the default case where title, message, and
isSSLError are set to implement this change.
---
Outside diff comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6411-6434: The bypass button HTML is vulnerable because failedURL
is injected into a single-quoted JS string; instead build the deep link URL
server-side using URLComponents (use the existing
BrowserSSLErrorBypassStore.shared.createPendingToken(for: failedHost) for token
and failedHost), percent‑encode the url parameter via
URLComponents/addingPercentEncoding, then pass that fully encoded deep link into
bypassButtonHTML (replace the inline "' + encodeURIComponent(...)" approach) and
only run escapeHTML on the final deep link string before interpolating it into
the button HTML; update the bypassButtonHTML construction in the same block that
defines escapedToken and failedHost to use the precomputed encoded deep link.
---
Duplicate comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 6514-6532: The retry currently rebuilds the request as
URLRequest(url:) losing method, headers and body; update the flow to persist the
original URLRequest when creating/issuing the bypass token and, in BrowserPanel
where you call BrowserSSLErrorBypassStore.shared.consumePendingToken(token, for:
host) and then addBypass(for:), load the original persisted URLRequest (not a
new URLRequest(url:)) via webView.load(originalRequest) so POSTs and custom
headers/bodies are replayed intact; modify BrowserSSLErrorBypassStore to
accept/store the full URLRequest keyed to the token and provide a retrieval API
used here.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a23d4a5a-39c0-4648-84d9-57aa0abbb22c
📒 Files selected for processing (1)
Sources/Panels/BrowserPanel.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Panels/BrowserPanel.swift">
<violation number="1" location="Sources/Panels/BrowserPanel.swift:6538">
P1: Restrict `targetURL` to `http`/`https` before loading it. The current guard accepts any URL with a host, so a crafted `cmux-browser-action://bypass-ssl?...` request can drive `webView.load` for non-web schemes (for example `file://`) even when token validation fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
|
Manually tested using sites from https://badssl.com/, ready for human review. |
|
@lawrencecchen Can you let me know if there's anything else I need to get this merged? (thx) |
|
@lawrencecchen @austinywang This feature (to allow users to proceed anyway if an SSL cert is invalid) is ready to merge, please let me know if there's anything else I can do to help to get it merged. Thanks. 🙏 |
| if activeSSLTrustBypassReplayRequest != nil { | ||
| clearAttemptedRequest(discardPendingBypasses: true) |
There was a problem hiding this comment.
.other navigation path, and the retried page can then commit with activeSSLTrustBypassReplayRequest == nil. In that state activeErrorPageDisplayURL is left set, so BrowserPanel.publishCommittedURL keeps treating the real page as an error page: the omnibar stays on the failed URL and history/favicon updates stay skipped. The popup delegate has the same state transition, so the successful retry path needs to clear the active error-page state when the real navigation commits, not only when a bypass replay commits.
…ass-action # Conflicts: # .github/swift-file-length-budget.tsv
Stale CodeRabbit change request against a superseded commit; current CodeRabbit check passes and addressed feedback is resolved.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ficate-error-bypass-action
…ass-action # Conflicts: # .github/swift-file-length-budget.tsv # Resources/Localizable.xcstrings # cmux.xcodeproj/project.pbxproj
Resolve conflicts after main advanced (SSL "proceed anyway" #3711, etc.): - BrowserNavigationDelegate.swift (real code merge): * didStartProvisionalNavigation: keep main's lastAttemptedRequest URL fallback plus this branch's print-reset + didClearPDFDocument(). * shouldPerformDownload: keep this branch's subframe download gating (main-frame / user-activation / recorded-intent / insecure-HTTP block) and add main's clearAttemptedRequest() on the path that actually proceeds to .download. * keep this branch's WKWebView.cmuxRunPrintOperation() extension. - cmux.xcodeproj/project.pbxproj: union of both branches' new files (main's SSL/error-page files + this branch's PDF/popup-policy files), re-normalized; no duplicate entries. - .github/swift-file-length-budget.tsv: regenerated from merged tree. Localizable.xcstrings auto-merged. Swift compiles clean (tagged Debug build SUCCEEDED). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This branch was successfully deployed
2 participants
Summary
Similar to #1666; possibly fixes #2035
What changed?
User is presented a "Proceed Anyway" button when a certificate error occurs
Why?
Self-signed certs and certificates signed by corporate CAs that aren't trusted by OS trust stores prevent CMUX's browser from being useful in dev scenarios and corporate environments.
Testing
EDIT: I've been dog-fooding this for over a month and it works exactly as one would expect.
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Adds a “Proceed Anyway” option for invalid SSL/TLS errors. Uses a one‑time token to safely replay the failed HTTPS request, keeps the failed URL visible in tabs, popups, and during recovery, hides error pages from history/search, and only shows Reload for headerless GETs.
New Features
NSURLErrorSecureConnectionFailed. Tokens are sent viacmuxSSLTrustBypass(fallbackcmux-browser-action://bypass-ssl?token=...) and accepted only when the HTTPS scope (host/port) and leaf cert SHA‑256 match; tokens expire after 24h.URLRequestwhen safe: require the same URL (normalized), method/body/headers; reject streamed bodies, bodyless non‑idempotent replays, and bodies >1 MB. Allow bypass after safe redirects using the final failed URL/scope/fingerprint.Refactors
cmuxSSLTrustBypassbridge to the active main‑frame error page and synchronously accept tokens only when a pending token exists (safe Main‑Actor hop).Written for commit d9a0d39. Summary will update on new commits.
Summary by CodeRabbit
New Features
Style