feat(web): trusted_domains allowlist skips the terminal link-open confirm - #1758
feat(web): trusted_domains allowlist skips the terminal link-open confirm#1758asheshgoplani wants to merge 1 commit into
Conversation
…firm
Clicking a link in the web terminal always fired xterm's built-in OSC-8
handler, which confirms unconditionally ("This link could potentially be
dangerous"). Against internal tooling — self-hosted GitLab, Gerrit, CI —
every merge-request and build link pays that prompt, several times a minute
during review work, for links the user already trusts.
TerminalPanel now installs its own xterm `linkHandler` (static/app/
terminalLinks.js) that consults two new `[web]` keys:
[web]
trusted_domains = ["gitlab.corp.example", "*.ci.corp.example"]
confirm_link_open = true # default; false accepts the risk globally
A link whose host matches an entry opens directly; everything else keeps the
confirm, so the safety net stays in place for arbitrary links. Matching is on
host only: case-insensitive, port- and path-independent, exact by default,
with `*.base` matching strictly deeper subdomains of `base` (never the bare
base itself). Entries are normalized once in Go
(session.NormalizeTrustedDomains) so a pasted URL, a host:port, or a trailing
dot all reduce to the same comparable host, and unusable entries (`*`,
`*.example`, blanks) are dropped rather than widened. Only http/https links
are ever auto-opened, and the open itself still mirrors xterm's: blank
window, `opener` cleared, then navigate.
Plumbing follows the existing `[web].mutations_enabled` path: config →
web.Config → GET /api/settings → signals hydrated by AppShell → read at
click time, so a settings change needs no terminal rebuild. web.Config takes
`ConfirmLinkOpen *bool` where nil means "confirm", so a Config that omits the
field can never silently disable the prompt. The settings drawer shows both
active values.
Tests: 40 vitest cases on the matcher and the open path, including the
bypass attempts an allowlist has to survive (suffix lookalikes, wildcard
base, host smuggled into userinfo, non-http schemes, popup blocked); a
Playwright spec that boots dedicated web-fixture instances and proves
allowlisted-vs-not and the global opt-out in a real browser against a real
server; Go coverage for config normalization, the /api/settings contract
(including the safe default), and a source pin on the TerminalPanel wiring,
which neither behavioral suite can observe because an OSC-8 link needs a live
tmux pane.
Closes #1682
Committed by Ashesh Goplani
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (16)
✨ 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 |
|
👋 Thanks for the contribution — intake looks complete. Your PR body carries everything the maintainer's validation pipeline reads first: the problem, the reasoning, the human intent behind it, and an AI-disclosure. It will be applied, built, and tested against gate marker read: ai= |
What problem does this solve?
Closes #1682. Clicking a link in the web terminal always fired xterm.js's built-in OSC-8 handler, which confirms unconditionally: "Do you want to navigate to … WARNING: This link could potentially be dangerous". For anyone working against internal tooling (self-hosted GitLab, Gerrit, CI) every merge-request and build link pays that prompt, several times a minute during review work, for links they already trust.
This adds the
[web].trusted_domainsallowlist the issue asked for, plus the explicitconfirm_link_opentoggle:A link whose host matches an entry opens directly. Everything else still confirms, so the safety net stays in place for arbitrary links — the allowlist form the issue thread picked as the default recommendation.
Why this change
The confirm is not agent-deck code: it lives in the vendored
static/vendor/xterm.mjsOscLinkProvider, whose defaultactivatecallback is used whenever the terminal has nolinkHandleroption. So the single link-open call site is thenew Terminal({…})instatic/app/TerminalPanel.js: supplyinglinkHandlerreplaces the built-in handler outright, with no patching of the vendored bundle.The policy itself lives in a new
static/app/terminalLinks.jsand is deliberately conservative, because an allowlist is a security control:*.basematches strictly deeper subdomains ofbaseand never the bare base itself.session.NormalizeTrustedDomains) so a pasted URL, ahost:port, userinfo, or a trailing dot all reduce to the same comparable host. Unusable entries (*,*.,*.example,git.*.corp.example, blanks) are dropped, never widened —*.examplewould otherwise allow every single-label host on the network. The JS mirrors the same normalizer so a hand-edited config and an API-served list behave identically.http/httpsever auto-open.file://gitlab.corp.example/…andjavascript:never match, whatever host they claim.openercleared, then navigate; a blocked popup refuses to navigate rather than hand the target a live opener.Config plumbing follows the existing
[web].mutations_enabledpath — config →web.Config→GET /api/settings→ signals hydrated byAppShell.js→ read at click time, so changing settings needs no terminal rebuild. One deliberate asymmetry:web.Config.ConfirmLinkOpenis a*boolwherenilmeans confirm. A plainboolwould let anyweb.Config{…}built without the field (tests, the e2e fixture, a future caller) silently disable the prompt for every host; the pointer makes the safe value the zero value.GET /api/settingsalways serializestrustedDomainsas an array, nevernull.User impact
[web]config, every link still confirms.[web].trusted_domains; those links now open on the first click.[web].confirm_link_open = falsedrops the prompt entirely for users who accept the risk (documented as the less-preferred option).trusted domains,link confirm) so the active policy is visible without readingconfig.toml.[web]section inskills/agent-deck/references/config-reference.mddocumenting all three keys (mutations_enabledwas previously undocumented there too).Evidence
Playwright e2e (the web gate),
tests/web/e2e/trusted-domains.spec.js— boots dedicatedweb-fixtureinstances on ephemeral ports (same pattern asread-only-mode.spec.js), one with-trusted-domains gitlab.corp.example,*.ci.corp.exampleand one with-confirm-link-open=false, then drives the production handler in a real browser against a real server after real/api/settingshydration:The spec explains in-file why it invokes the
activatecallback xterm itself would call rather than clicking a rendered link cell: an OSC-8 hyperlink only exists in the terminal buffer once a live tmux pane emits one, and the in-memory fixture has no pane. It imports the same module instance the app loaded (identical/static/app/terminalLinks.jsURL → same module record, same hydrated signals), so the decision path under test is the real one.Full
tests/websuite (npm run test:unit+npm run test:e2e), sandboxed HOME/XDG:The 5 failures are all
toHaveScreenshotbaseline mismatches (visual-baselineshome light/dark,keyboard-parityshortcuts overlay). They reproduce identically on a pristineorigin/maincheckout on this machine (verified with the change stashed: same 4 visual-baseline failures, 5 passed) — the committed baselines are generated on Linux CI and this run was macOS. Nothing this PR touches is rendered on the home screen or the shortcuts overlay.Go:
go build ./...andgo vet ./...clean. Go tests were not run on this host — this machine's$HOMEis a live~/.agent-deckdata directory and several packages are not isolated from the real home (the failure mode documented in the repoCLAUDE.md). The new Go coverage is left to CI's sandboxed runners:internal/session/userconfig_web_test.go— defaults when the keys are absent, reading + normalizing the list,confirm_link_opendefault/explicit, and a 17-case table overNormalizeTrustedDomains(case folding, scheme/path/port/userinfo/trailing-dot stripping, IPv6 literal, dedupe, order preservation, and every entry shape that must be dropped).internal/web/handlers_settings_test.go— the/api/settingscontract, including that aConfigwithConfirmLinkOpenunset still reportsconfirmLinkOpen: true.internal/web/issue1682_link_policy_test.go— source pin on theTerminalPanel.jswiring, in the same spirit asTestWebBundle_ChartNotInInitialPayload_RegressionFor1022. Neither behavioral suite can observe whether xterm is actually handed the handler (no OSC-8 link without a live pane; xterm exposes no back-reference from its DOM element to the Terminal instance), and deleting that one option would silently restore the confirm on every link.Bypass cases pinned in
tests/web/unit/terminalLinks.test.js(40 cases):notgitlab.corp.example,gitlab.corp.example.evil.test,evilci.corp.examplevs*.ci.corp.example,ci.corp.exampleagainst its own*.entry,https://gitlab.corp.example@evil.example/,file://andjavascript:with a trusted host, unparseable input, empty/missing list, declined confirm, blocked popup, and the live-signal re-read.AI disclosure
Model(s), if AI helped: claude-opus-4-x
What actually bothered you
From the issue, and it matches the maintainer's own review workflow: every merge-request link in a self-hosted GitLab / Gerrit setup fires the same "this could be dangerous" dialog, so reviewing a stack of MRs from the agent-deck web terminal costs several extra clicks a minute on links that are known-safe internal tooling. The friction is bad enough that the realistic alternative is switching the confirm off wholesale, which is exactly the outcome an allowlist avoids.
Note for @amh1k, who offered to take this on: apologies for the overlap — this was picked up in the same sweep that reopened the issue and I did not want it to sit unlanded a second time. Genuinely happy to hand over follow-ups here (a TUI-side equivalent, or a web toggle that writes the config), and review comments on this diff are very welcome.
Checklist
HOME=$(mktemp -d) XDG_CONFIG_HOME= XDG_DATA_HOME= XDG_CACHE_HOME= go test ./...— not run on this host (its$HOMEis a live agent-deck data dir; see Evidence).go build+go vetclean; Go tests verified by CI.