Repository navigation
fix(kilo-mcp): require the confirmed client for a device pairing - #8534
Conversation
The MCP worker now names the OAuth client (client id, display name and the redirect origin the code is delivered to) when it opens an apps/web device-auth pairing. The approval page shows that client and its redirect origin, and the signed-in user confirms the client before the pairing can be approved; the approval records that confirmation on the request. The worker completes an authorization only when the approved pairing confirms the same client id and redirect origin it named. An approval for anything else closes the request and is answered as denied, so no grant is minted and the client receives no code. Pairings whose requester names no client keep the existing approval contract. Adds the requesting-client columns to device_auth_requests, the optional requesting_client member on POST /api/device-auth/codes, the confirmed_client_id member on POST /api/device-auth/tokens, and requestingClient/clientConfirmed on the poll answer.
iscekic
added
drain
|
bot: Visual evidence from the local dev stack (apps/web with Postgres, the worker under wrangler dev with local bindings, a seeded fake account). The approval page states which app is being connected — its display name, the origin its code is delivered to, and its client id — and the approval stays disabled until the signed-in user confirms that app. The third capture is the approval outcome. These captures come from the same run as the end-to-end checks in the pull request body.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Merge after the migration name in the PR description is corrected Executive SummaryThe incremental change only renumbers the branch's additive Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous Review Summaries (7 snapshots, latest commit a9dceeb)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit a9dceeb)Status: No Issues Found | Recommendation: Merge Executive SummaryThe client-confirmation contract is consistently enforced on both sides — apps/web refuses an approval that does not name the stored client, and the MCP worker denies any approval whose echoed client id or redirect origin differs — and the regenerated Assumptions: Files Reviewed (33 files)
Previous reviewStatus: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe regenerated Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (33 files)
Fix these issues in Kilo Cloud: https://app.kilo.ai/cloud-agent-fork/review/1de889e1-269e-4658-83cf-d80f2bc4b2ad Previous review (commit bd4fbae)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit adds a restart-invariant assertion to the store test, confirming that constructing a second Durable Object over the same SQLite storage leaves the applied Assumptions: Files Reviewed (1 file)
Previous review (commit 6fd40c6)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental commit correctly aligns the MCP worker with apps/web's device-auth contract — the display Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (13 files)
Fix these issues in Kilo Cloud: https://app.kilo.ai/cloud-agent-fork/review/abb48129-d21b-4682-abbd-5090fcbaed55 Previous review (commit c6aa8b7)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit restricts bare-scheme acceptance to non-browser schemes and adds route tests for Assumptions: Files Reviewed (2 files)
Previous review (commit ff90f71)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe new redirect-origin validation rejects paths, queries, userinfo, and prose while accepting the worker's native-scheme shapes, but its bare-scheme shortcut also accepts host-less web schemes such as Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (4 files)
Previous review (commit 3e9af49)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe client-confirmation plumbing is sound on the MCP worker path, but the app identity shown on Assumptions: Overview
Issue Details (click to expand)WARNING
Files Reviewed (26 files)
Fix these issues in Kilo Cloud: https://app.kilo.ai/cloud-agent-fork/review/ae6024a1-e1cb-444b-af59-7aafbdbe6d23 Reviewed by deepseek-flash-latest · Input: 46.9K · Output: 8.8K · Cached: 503.7K Review guidance: REVIEW.md from base branch |
The requesting-app block on the approval page states that its details come from the app that started the request, and the confirmation reads as the user's expectation of that connection. `redirect_origin` must now be an absolute origin — a web origin, or a native app scheme — so the block never presents a path, query, relative value or free prose as the destination.
|
bot: Refreshed captures from the local stack after the review fix. The block now states that its details come from the app that started the request, and the confirmation reads as the user's expectation of that connection. The approval action stays disabled until it is confirmed.
|
A bare scheme is the origin shape of a native app that takes the code without a host. Browser-handled schemes are not origins a code can be delivered to, so `scheme://host` is required for them and the bare form is refused.
|
bot: The proxy selected host repair and retry session for generated-artifact tooling. The pull request body preserves the migration recovery recipe, local verification results, and the pending merge status. The normal commit path remains blocked; no merge-resolution commit has been pushed. |
iscekic
removed
the
merge:human
|
bot: The migration reconciliation is committed and pushed. GitHub reports the branch mergeable, and all completed checks pass. The regenerated migration applied successfully to local PostgreSQL; regression results are recorded in the updated pull request body. The earlier tooling handoff is superseded by the completed publication. |
iscekic
added
merge:blocked:insufficient-e2e
|
bot: Fresh captures from the current head on the local dev stack (apps/web with PostgreSQL, the kilo-mcp worker under The approval page names the app that started the request — its display name, the origin its code is delivered to, and its client id — and the approval action stays disabled until the signed-in user confirms that app. These captures come from the same run as the end-to-end checks in the pull request body.
|
iscekic
added
intent:ok
iscekic
removed
the
merge:human
|
bot: Migration review follow-up: the real-SQLite persistence test now checks the complete recorded migration history across store recreation, alongside the pending authorization and its stored approval display value. The focused store suite passes all 24 tests. This retains the idempotence invariant without fixing the test to one migration count. The local browser flow also reached the approval outcome shown below. This capture contains only the outcome card; the same run completed the organization picker, token exchange, and authenticated MCP account read.
|
Regenerate the branch migration after main added its own migration at the same sequence position.
The branch migration is regenerated one sequence position later because main added its own migration. Only the new columns on the device auth requests table are generated.
iscekic
removed
the
merge:human
|
bot: Approval surface exercised on the local stack at head
|
|
bot: The summary's suggestion is addressed in the pull request body.
No source files changed for this; the head remains |
iscekic
added
merge:blocked:kilobot-not-ready
…ent-confirmation # Conflicts: # packages/db/src/migrations/meta/0299_snapshot.json # packages/db/src/migrations/meta/_journal.json
main added its own migration at the previous position. The four new device auth columns are regenerated one position later from the merged schema.
iscekic
removed
the
merge:human
|
bot: Approval surface exercised on the local stack with an owned fake account.
|
iscekic
added
merge:blocked:kilobot-not-ready
Evidence
Evidence: demonstrated — the branch head was exercised on the local stack with apps/web, PostgreSQL, the MCP worker under
wrangler dev, and Chromium, with an owned fake account. The approval page displayed the requesting app and destination, kept its approval action disabled until that app was confirmed, and reached its success state. The organization picker, the authorization-code exchange, and an authenticated MCP call completed with the paired account. A pairing whose stored approval names a different client than the authorization started for was refused at completion.1ce9d682690491394888e06e8b9346d6d2e908f6, after merging main and moving the branch migration to the next free sequence position:GET /authorize(the consent page names the client), browser approval, organization picker, token exchange answering200, and MCPuser.getMeanswering with the paired fixture account (idof the seeded test user,success: true).disabled: truebefore the confirmation was checked,disabled: falseafter, and the "confirm the requesting app" hint disappeared./api/device-auth/tokens:409 Device authorization client confirmation does not matchwithout a confirmation, the same409when the confirmation names a different client, and200 {"success":true}for the matching client, which records the confirmation time.0300_device_auth_client_detailsapplied to an empty local database (drizzle migrateapplied 301 migrations); the four device-auth columns are present afterwards, andpnpm --filter @kilocode/db testpasses.pnpm --filter kilo-mcp test: 19 files, 528 tests passed. Web Jest forsrc/lib/device-auth,src/app/api/device-auth, andsrc/app/device-auth: 10 suites, 150 tests passed.Limitations: local stack only; no deployed environment, mobile surface, or published CLI was exercised.
Change
The approval page presents the app-provided connection details for explicit confirmation. The web API records the approval, and the worker applies that contract through OAuth completion. Dynamic client registration policy remains unchanged.
Main is merged into this branch. The worker follows the current web pairing response contract, retains the approval display value in its authorization store, and supports continuation of the same request. Browser binding and authenticator enrollment behavior from main is preserved. Generated PostgreSQL migration metadata was regenerated from the merged schema at the next free sequence position after main took the previous one; the worker SQLite artifacts were generated from its schema.
Deployment order: shared database migration, apps/web, then MCP worker. The worker SQLite migration runs on first store access.
Production prerequisites
0300_device_auth_client_detailsbefore deploying apps/web.Proxy answers
The owner's proxy gave these answers in the owner's place.