Repository navigation
feat(jetbrains): add MCP OAuth sign-in - #14667
Merged
Merged
Conversation
Expose remote MCP authentication across Settings, Marketplace, and the prompt toolbar so servers that require OAuth can be connected without hand-editing configuration. Preserve custom OAuth client settings and surface browser, timeout, and server errors to make sign-in recoverable.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge No new issues in the incremental diff No memory-leak or resource findings in the changed code: the restored Files Reviewed (1 file)
Fix these issues in Kilo Cloud Previous Review Summaries (12 snapshots, latest commit a9bd0d4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a9bd0d4)Status: No Issues Found | Recommendation: Merge Incremental review of
No memory-leak regressions identified (the overlay now retains less, not more, and no new listeners/resources are held). Files Reviewed (23 files)
Previous review (commit 216437d)Status: No Issues Found | Recommendation: Merge Incremental review of Files Reviewed (1 file)
Previous review (commit 86df0dc)Status: No Issues Found | Recommendation: Merge The branch merged Files Reviewed (32 files)
The classifier removal is safe: the CLI classifies HTTP 401/403 and OAuth failures centrally in Previous review (commit 7643cb9)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files, incremental since f01d37c)
Carried forward: none. The prior unresolved findings are on files outside this incremental diff. Previous review (commit f01d37c)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files, incremental since a73754f)
Carried forward: none. The prior Previous review (commit a73754f)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files, incremental since ad1568f)
Carried forward: none. The Fix these issues in Kilo Cloud Previous review (commit ad1568f)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (fix commit ad1568f, 35 files)
The fix commit was reviewed in full. The Fix these issues in Kilo Cloud Previous review (commit 3e81774)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files, incremental since bbc1281)
Carried forward after re-verification: the Fix these issues in Kilo Cloud Previous review (commit bbc1281)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (2 files, incremental since de1bcd1)
Resolved since the previous review: the Fix these issues in Kilo Cloud Previous review (commit de1bcd1)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Resolved since the previous review: the Previous review (commit d75cc82)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit cf1b9d6)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (27 files, incremental since 776f81c)
[Snapshot truncated.] Additional previous summary content was truncated to keep this comment within platform limits. Reviewed by deepseek-v4.1-flash · Input: 44.2K · Output: 6.7K · Cached: 375.9K Review guidance: REVIEW.md from base branch |
Keep Marketplace authentication progress visible, treat user cancellation as a quiet outcome, and bound cached MCP authentication state across Agent Manager worktrees.
Contributor
Author
|
Addressed all three review findings in commit |
Reconnect remote MCP servers after clearing credentials so the runtime immediately reports needs_auth and the chat toolbar can offer sign-in before another tool call fails.
Move MCP sign-in into a reusable session issue action menu in the prompt input, keeping the control hidden when no session-scoped recovery actions are available.
Scope OAuth cancellation to individual attempts, keep reset state consistent through reconnect failures, cancel session issue tracking on disposal, and cover the prompt action wiring directly.
Contributor
Author
|
Addressed all five unresolved review findings in commit |
Anchor the session issue trigger at the prompt top-right, group recovery actions by MCP provider without item icons or a popup header, and open MCP Settings with the affected provider pre-filtered.
Normalize legacy unauthorized MCP failures into sign-in-required state and use config overlay set/unset operations so installed MCP servers can be removed from Settings.
Delegate disposeReadyComponent to DraftReadyConfigurableBase so closing Settings releases the retained McpSettingsUi and cancels its in-flight reload.
Carry the runtime failure reason on the MCP status badge tooltip, since the row hides its description line, and log it for every unhealthy server so the cause is diagnosable from the IDE log.
The CLI only reports needs_auth for UnauthorizedError or an OAuth message, so a rejected browser flow, a failed token exchange, or a bare 401 arrived as failed and silently removed sign-in from MCP Settings, the Marketplace install prompt, and the session prompt. Reclassify those OAuth-specific failures as needing authentication so all three surfaces offer sign-in again.
# Conflicts: # packages/kilo-jetbrains/frontend/src/test/kotlin/ai/kilocode/client/settings/marketplace/MarketplaceSettingsUiTest.kt
…tall - MCP settings: the needs-auth status pill now carries the warning glyph used in the session issue indicator, and the row's action set matches the actual connection state. needs_auth shows only Sign In (no Connect, no Reset sign-in, since there is nothing to reset until a session exists). Reset sign-in only appears once a remote server is connected. - Marketplace MCP + companion skill bundles can now be uninstalled consistently from either MCP Settings or Skills Settings: deleting either half confirms and removes the whole bundle (server, owned skills, and the ownership marker), instead of leaving an orphaned half installed. Ownership is read from a marker written next to the installed skill so removal still works after the catalog changes. - Active list renderer: selected-row action cells (and disabled-cell hit tests) now stay visible/interactive while the list is selected but unfocused, instead of only when the list is the focus owner. This is a shared behavior change across every settings list built on ActiveList. Adds/updates focused tests in McpSettingsUiTest, SkillsSettingsUiTest, SettingsListViewTest, KiloMarketplaceRpcApiImplTest, and KiloBundleLocaleTest, plus localized delete-confirmation strings.
…esting - Anchor the 401/oauth auth-failure markers so an unrelated failure that merely embeds a port number, ID, or hostname containing those substrings is not misclassified as needs_auth. 401 now requires an http/status-prefixed match; oauth requires a word boundary. - A tooltip-only badge (an id with no action, like the MCP status pill) no longer blocks single/double-click fall-through to the row's own click/open handler. Only regions that carry an actual action still swallow the click. Addresses PR #14667 review threads on KiloCliDataParser.kt and McpConfigurable.kt/ActiveListView.kt.
… tooltip badges The prior fix (a73754f) keyed click fall-through on ActiveListHit.action, but that field is only the per-cell handler and is null for every cell routed through the panel-level onCell callback (Sign In, Connect, Delete, rename, ...) and for the row menu glyph. That regressed double-clicking any real onCell-routed action into also firing row activation. hit(e, enabled = false) now blocks fall-through when the hit id belongs to a real visible cell (activeListVisibleCells, including the menu glyph when available) or a registered badge region (activeListRegions) — independent of whether that cell/region carries its own action lambda. A tooltip-only badge (id with neither an action nor an onCell binding) still correctly falls through to row activation. Added a regression test that double-clicks a non-primary onCell action cell and asserts it fires the cell action without also invoking onOpen/ activate, per PR review feedback. Addresses PR #14667 review thread on ActiveListView.kt.
Some remote MCP servers (e.g. anaconda) reject an unauthenticated SSE
connection with 403 rather than 401, reaching the plugin as a plain
"SSE error: Non-200 status code (403)" failure. The CLI only reports
needs_auth for UnauthorizedError or an OAuth-mentioning message, so this
fell through to failed and broke sign-in end to end: no post-install
sign-in prompt, and MCP Settings showed "failed" with no Sign In action
instead of the needs-auth badge.
Extend the anchored http/status marker regex from 401-only to 401 or
403 (\b(?:http|status)\D{0,10}40[13]\b), so both transports'
unauthenticated rejections normalize to needs_auth while still
rejecting unrelated failures that merely embed those digits (ports,
durations, server names, 404/500 statuses).
Adds parser-level coverage for the exact SSE 403 message plus the
existing 401/oauth false-positive guards, and a full-HTTP-path test
through KiloAgentBehaviorRpcApiImplTest/MockCliServer proving the CLI
response reaches the frontend DTO as needs_auth.
Add a refreshOnFocus toggle (default true) to SettingsListPanel so every Agent Behavior list page (MCP, Skills, Commands, Agents) and Marketplace/ Rules settings automatically reload when the panel becomes visible again, e.g. navigating away in Settings and back, or reopening the dialog. Wired via a HierarchyListener on SHOWING_CHANGED: the initial mount is ignored (directory-ready/initial load already handles that), and every subsequent re-show triggers reload() when refreshOnFocus() is true. No per-page changes needed since no subclass overrides the default. Adds a regression test that attaches a panel to a host, hides and shows it via removeNotify()/addNotify(), and asserts a reload happens only on the re-show.
The MCP OAuth, marketplace sign-in, and bundled uninstall keys added in this branch were copied verbatim from the English bundle into all 18 localized bundles, so non-English users saw raw English strings. Translate every one of those keys per locale, preserving MessageFormat placeholders and leaving product/technical tokens (MCP, OAuth, Marketplace) untranslated.
…arsing The plugin re-derived "sign-in required" from the CLI's human-readable MCP error text, duplicating rules the CLI now owns. Core has since consolidated the MCP OAuth flow and reports that state structurally, so the plugin can read it instead of guessing. Drop the local marker/regex classifier and take `needs_auth` straight from the CLI status. Ask `auth/authenticate` for `external` handling and open the authorization URL from the `mcp.auth.url` event, so the browser opens on the user's machine rather than on the host in remote development. Cancel through the new `auth/cancel` endpoint so abandoning or timing out an attempt no longer deletes credentials the user already had; `DELETE /auth` stays behind Reset sign-in. Report a missing server distinctly instead of as a generic failure.
The CLI pin bump moved packages/kilo-jetbrains to 7.8.7 without updating the lockfile's workspace entry, so bun install rewrites it on every fresh setup.
brunoagatao
reviewed
Oct 7, 2026
brunoagatao
left a comment
brunoagatao
left a comment
Collaborator
There was a problem hiding this comment.
High (security): No http/https check before the browser opens the authorization URL.
KiloMcpAuthService.kt:223,230 and McpAuthUrlDialog.kt:43 call BrowserUtil.browse(url) on any mcp.auth.url. The URL comes from the remote server's authorization_endpoint, so the server controls it.
Repro: a server whose metadata gives file:///... or smb://... as the endpoint.
Fix: accept only http and https in onAuthUrl and in the dialog. Also apply it to the redirect URI check (McpEditDialog.kt:221 accepts javascript:x).
Medium:
- A Marketplace failure stops the MCP and Skills pages from loading (
McpConfigurable.kt:97,SkillsConfigurable.kt:122). - A cleared OAuth field stays in
kilo.json(McpEditDialog.kt:130). - The client secret goes to the project
kilo.json, stays in the backend cache, andMcpOAuthDto.toStringprints it. - The full authorization URL, with
state, goes to the log (KiloMcpAuthService.kt:235). - Shared list behaviour changed for all lists (
ActiveListRenderer.kt:361,SettingsListPanel.kt:84), and a test was edited to fit.
`pathological diffFull workload finishes quickly and does not block abort` required a 25ms `setInterval` to fire at least once during the diff. The whole workload now finishes in roughly 150ms, so only a handful of 25ms windows ever exist, and a single ordinary synchronous chunk of diff work on a loaded CI box can starve every one of them. CI saw `ticks=0` with the run taking 289ms while the abort endpoint still answered in under 2ms, so the assertion failed without anything being frozen. The assertion also got more fragile the faster diffFull became, which is backwards for a performance regression guard. Record when the timer actually runs and assert the longest stretch without a callback stays under two seconds, including the head and tail windows. That is a sharper bound than the existing five-second total, since it catches a long contiguous stall even when the overall run lands inside the budget, and it no longer depends on the work outliving one timer period. Verified the bound still fails on a real freeze: injecting a 2.5s synchronous block reports a 2566ms stall and fails the test.
Security: the authorization URL and the OAuth redirect URI both come from the remote MCP server, and both reached a browser or the config file unchecked, so a server could hand over `file:///…`, `smb://…`, or `javascript:…`. Add a parsed http(s)-with-host check and apply it where the URL leaves our control: the auth event is dropped before it can open or reach the fallback dialog, the dialog's Open button is disabled and re-checks on click, and redirect URI validation no longer accepts any absolute URI. Also from review: - Stop logging the authorization URL; it carries the OAuth `state` and other single-use parameters, and the server name identifies the flow on its own. - Write cleared OAuth fields as explicit nulls. The config schema deep-merges on PATCH, so an omitted key kept the client id/secret/scope the user had just cleared, and it came back after a restart. - Redact `clientSecret` in `McpOAuthDto.toString`, which otherwise printed it through RPC logging and exception messages, and keep it out of the backend's save overlay, reading it back from the authoritative config instead. - Keep the MCP and Skills pages working when the Marketplace is unreachable. Bundle ownership only decides whether a delete offers to remove a whole install, so it now degrades to the plain delete confirmation instead of failing the page. - Scope the two shared list changes behind opt-in flags so unrelated lists keep their previous behavior: `ActiveListConfig.keepActions` for showing a selected row's actions while focus is elsewhere, and `refreshOnFocus` now defaults to off. MCP, Skills, and Marketplace opt in. This restores the inverted `SettingsListViewTest` assertion rather than leaving the test bent to fit.
Contributor
Author
|
Addressed all six findings in High (security): non-web authorization URLs. Added
Verified the guard is load-bearing: with the Medium, all five:
New coverage: non-web URL rejection in the service, Open enabled/disabled in the dialog, redirect-URI validation, explicit-null OAuth patch, Checks: |
… tick count" This reverts commit 216437d. That flaky-test fix is unrelated to this PR's MCP auth work and is being handled in another PR instead.
pandemicsyn
approved these changes
Oct 7, 2026
This was referenced Oct 7, 2026
This was referenced Oct 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
JetBrains users could install remote MCP servers that require OAuth, but Marketplace installation stopped at
needs_authwithout offering a login flow. The existing MCP Settings action also discarded the CLI's returned status, provided no cancellation or browser fallback, and could not configure servers that require a pre-registered OAuth client.Two further problems were found while finishing this work:
Why This Change Was Made
This uses the CLI's existing generic MCP OAuth endpoints and status model rather than adding provider-specific behavior. Authentication is coordinated in one JetBrains service and reused by Settings, Marketplace, and a reusable session-issues action menu in the prompt. OAuth client settings are carried through the split-mode RPC DTOs so editing a remote server does not silently remove its existing client configuration.
The follow-up work adopts the consolidated core contract that landed in
feat: consolidate MCP OAuth in core, which is the same contract the VS Code extension migrated to. The plugin now reads structured fields instead of parsing prose, which keeps a single owner for the classification rules.No issue is linked; this follows investigation of the Anaconda Marketplace MCP integration.
Changes:
needs_authstraight from the CLI status and delete the local marker/regex classifier, so recoverable auth failures are identified in exactly one place.externalhandling fromauth/authenticateand open the authorization URL from themcp.auth.urlevent, so the plugin owns the browser hand-off instead of the CLI opening a browser next to itself.auth/cancel, so abandoning or timing out an attempt no longer deletes credentials the user already had.DELETE /authstays behind Reset sign-in, which is the only destructive path.User Impact
JetBrains users can complete OAuth sign-in without hand-editing configuration or manually discovering why an MCP server has no tools. Session-scoped problems now have a reusable prompt action menu that is hidden when there are no issues. Servers requiring a pre-registered client ID can be configured in the MCP editor. Local stdio servers and remote servers with
oauth: falseretain their existing behavior.Cancelling a sign-in, or letting one time out, now leaves existing credentials intact; previously both deleted them, so a server that was already working could be knocked back into
needs_auth. The authorization URL now opens through the IDE on the machine the user is sitting at rather than beside the CLI process.Non-English users now see translated strings for the whole MCP sign-in flow.
The CLI's OAuth callback is still a loopback redirect served next to the CLI process, so a fully remote host/client split can still require forwarding that port; this PR does not change the redirect URI.
Screenshots / Video
Anaconda on marketplace
Install details
After install
Sing in warning in chat
Signed in notfication
Anaconda MCP installed
Removing MCP would remove skills as well as part of the marketplace unit
Evidence
./gradlew typecheckfrompackages/kilo-jetbrains/: passed../gradlew testfrompackages/kilo-jetbrains/: passed, 4416 frontend tests plus the shared and backend suites.KiloMcpAuthServiceTest,KiloAgentBehaviorServiceTest,McpSettingsUiTest,MarketplaceSettingsUiTest,KiloBundleLocaleTest,KiloCliDataParserTest,KiloAgentBehaviorRpcApiImplTest.failedstatus is never upgraded,authenticatesendsexternal,auth/cancelis used for cancel and timeout, credentials survive both, and an external auth URL opens in the client browser with the dialog only as a fallback.v7.8.7) contains the consolidated endpoints before adopting them, so the plugin is not calling routes its bundled CLI lacks.MessageFormatapostrophes.bun run script/check-opencode-annotations.ts --worktreeandbun run script/check-md-table-padding.ts: passed.git diff --check: passed.git push: pre-push hooks passed the repository-wide TypeScript typechecks and the JetBrains typecheck. The first attempt failed onWorktreeManager.ts, a file this branch does not touch: the worktree had a stalesimple-git3.36.0 installed against the pinned 4.0.2.bun installresolved it, and the resulting one-line lockfile sync for thekilo-jetbrainsworkspace version is committed separately..changeset/jetbrains-mcp-oauth-sign-in.mdadds a minor JetBrains release note.CI Flake Fixed Here
unit (linux, 2/2)failed onpackages/opencode/test/kilocode/snapshot-freeze-repro.test.ts, which is unrelated to the MCP work: this branch changes no file underpackages/opencode/src. It turned out to be a genuinely bad assertion rather than infrastructure noise, so it is fixed here instead of retried.The test required a 25 ms
setIntervalto fire at least once during the diff, as a proxy for "the event loop stayed responsive". That workload now finishes in roughly 150 ms, so only a handful of 25 ms windows exist at all, and one ordinary synchronous chunk of diff work on a loaded runner can starve every one of them. CI recordedticks=0on a 289 ms run while the abort endpoint still answered in under 2 ms, so nothing was actually frozen. The assertion also became more fragile the fasterdiffFullgot, which is backwards for a performance regression guard.It now records when the timer actually runs and asserts that the longest stretch without a callback stays under two seconds, including the head and tail windows. That is a strictly sharper bound than the existing five-second total, because it catches a long contiguous stall even when the overall run finishes inside the budget.
Evidence it still guards the original bug: injecting a 2.5 s synchronous block makes it report a 2566 ms stall and fail. After the change it passed 10/10 consecutive runs on an idle machine.
Known Flakiness Not Addressed Here
KiloBackendWorkspaceTestandKiloSessionRpcApiImplTestfail a different subset on repeated local--rerun-tasksruns. This is a local environment artifact rather than a defect in this PR, and the JetBrains CI job passes:SSE: failure (HTTP 404)orReceived HTTP_PROXY_AUTH (407) code while not using proxy.MockCliServerbinds an ephemeral port viaServerSocket(0). On a developer machine with a dense localhost port space, the backend's SSE reconnect can reach a port owned by an unrelated local process. One failing run named port 52532;lsofshows that port held by VS Code'sCode Helper, andcurl http://127.0.0.1:52532/global/eventreturns exactly theHTTP 404seen in the log.Fixing it means reworking the mock server's port and lifecycle handling across the whole backend suite. That is unrelated to MCP auth and would not change any CI result, so it is deliberately left out of this PR rather than masked.
Dependency
None. The MCP OAuth endpoints and the
mcp.auth.urlevent are already provided by the pinned CLI release the plugin bundles.