Repository navigation
[AI-2027] Open the browser from setup and poll a machine pairing - #605
Merged
Merged
Conversation
`kcap setup <tenant>` now mints a pairing on that server, prints the human code and the fallback URL, opens the browser, and waits for a human to approve before carrying on to sign-in. - The code is printed every time. A browser showing a code is only half a comparison; the half that makes it a defence is the one the machine being paired prints. The fallback URL is printed for the same reason the opener is best-effort - nothing can confirm a browser actually opened - `PairingIdentity` asserts that the account which approved is the account the CLI then authenticates as, and setup stops if it is not. The channel carries approval and never a credential, so nothing else binds the two. Read from the access token's own claims rather than a round trip; the server re-checks the same comparison at /complete and answers 403 - `PairingPoll` is a pure classifier like `ProvisioningPoll`, so every branch is covered without a server. 401 is terminal here rather than transient: there is no token source to refresh, the secret was minted once - A 404 on mint is the availability oracle - a server that does not serve the routes needs no version check and no flag, it just gets today's path. Headless, --no-prompt and the None provider skip it for the same reason - `SystemBrowser` replaces LoopbackBrowser's private opener so there is one Closes #604
Three reviewers over the previous commit. The headline is that the identity
check read claims no token has ever carried, so it would have failed for every
user on every GitHub tenant, every time.
`kapacitor:user_id` and `kapacitor:github_id` are added to the ClaimsPrincipal
by a claims transformation at request time; they never go on the wire. The
tenant mints `github_id` and a `sub` of `github|{n}`, so the check fell through
to `sub`, compared "github|4242" against the "github:4242" the poll returns, and
aborted setup telling the user a different account had approved. The tests
missed it because they synthesised the claim shape rather than copying it - the
fixtures are now taken from the server's own token construction, which is the
part that would have caught it.
Also from the reviews:
- Reading claims without an object guard crashed on a valid JWT whose payload
is not an object: TryGetProperty throws there, and only Format/Json exceptions
were caught. Uses `JsonElementExtensions.Str`, which the repo already requires
- The poll compared the server's `expires_at` against the local clock, so a
machine running fast gave up without polling once, on a live pairing. The
budget is now measured locally from a floor; the server still says 410
- An approval naming no approver, or naming another tenant, returned Failed -
which degrades to "carry on with sign-in", i.e. exactly the skipped identity
check the guard exists to prevent. New `Untrusted` result aborts instead
- /complete discarded its status, including the 403 that IS the server
disagreeing about who approved. It now aborts; everything else stays cosmetic
- /complete ran at sign-in, which invalidates the secret and so closes the
channel before the steps worth reporting on had run. It is now last, and the
identity assertion is what runs at sign-in
- A mint missing `expires_at`/`pairing_id`/`setup_url` landed on Expired, which
aborts setup with a message that misdescribed it; it degrades like the rest
- Interval clamped at both ends, 429 now prints, first poll is immediate
- `IPairingChannel` makes the loop testable; 30 new tests cover the deadline,
the back-off, the guards and the wire shape, none of which had any
- HttpClients are disposed and given a 15s timeout, and the step catches what
neither the client nor the flow can, so a new leg cannot crash setup
- IPairingProgress gains WaitEnded (hosts other than Spectre need it), loses a
dead Notice, and reuses SetupAuthProgress.Indent instead of hardcoding it
Left deliberately: the check lives in SetupCommand rather than
OnboardingFacade's commit boundary. Moving it means threading an expected
identity through LoginAsync, which every caller shares - worth doing when the
Avalonia wizard is real, not on this ticket.
The server-side re-check is a corroboration, not an enforcement boundary:
/complete is anonymous and its comparison only runs when a bearer is sent, so a
build that omits it is not stopped. The prose said otherwise and now doesn't.
Closes #604
George-Payne
requested review from
alexeyzimarev,
nortonandreev,
realtonyyoung,
rsergeenko and
stktung
August 19, 2026 13:44
PR Summary by QodoAdd browser-based machine pairing to interactive setup
AI Description
Diagram
High-Level Assessment
Files changed (20)
|
This repo files issues in GitHub and CI enforces it. The other references went in the review pass; this one described a follow-up ticket and read as an exception rather than an oversight, which is why it survived.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
George-Payne
removed request for
alexeyzimarev,
nortonandreev,
realtonyyoung,
rsergeenko and
stktung
August 19, 2026 14:09
Qodo's five findings. Four are the same omission in different places: the mint response was described everywhere as untrusted input and then largely taken at its word. - `user_code` was not validated, so an empty one rendered a prompt with nothing beside it - removing the code comparison silently, which is the one failure mode worse than refusing. It joins the guard - `setup_url` was checked only for emptiness and then handed to a shell-executed open. A `file://` path or a registered custom scheme would have been launched, and a URL on another host is where a human would be asked to approve somebody else's pairing. Now required to be absolute http/https on the same server the pairing was minted on - The poll budget had a floor but no ceiling, so a far-future `expires_at` kept an interactive command polling for months. Clamped at both ends - Every unrecognised status was "keep waiting", including 400/403/405/422. A request the server permanently refuses was retried for seventeen minutes and then reported as an expiry that never happened. Non-429 4xx is terminal now, bar 408, which is the one that means try again The fifth is comment volume: the same rationale was restated across four or five sites. Each invariant now lives on the type that owns it, and the rest point at it.
George-Payne
requested review from
alexeyzimarev,
nortonandreev,
realtonyyoung,
rsergeenko and
stktung
August 19, 2026 14:29
Review catch. The check reduced the setup URL to its authority before comparing, which throws away the path - and ServerIdentity treats the path as significant precisely because deployments can be path-routed. With `--server-url https://host/tenant-a` that is wrong in both directions: the tenant's own `https://host/tenant-a/setup?p=...` reduces to `https://host` and no longer matches its base, so a legitimate page is refused; and nothing about the comparison distinguishes a neighbour's `https://host/tenant-b/setup` from it, so authority-only matching is not a check worth having either way. Both sides are now canonicalized whole and the target must sit under the base, with a trailing separator so `/tenant-a` cannot match `/tenant-abc`. The query and fragment come off before canonicalizing - ServerIdentity refuses a base carrying a query, and `setup_url` always has one - while the URL that is opened keeps them. Userinfo is refused too: the fallback link is printed for a human to read, and `https://acme.kcap.ai@evil.example.com/` reads as one host and addresses another. Regression tests for a path-routed base accepting its own page, rejecting a neighbour's, rejecting one above the base, rejecting a prefix collision, and rejecting userinfo.
realtonyyoung
approved these changes
Aug 19, 2026
realtonyyoung
left a comment
realtonyyoung
left a comment
Collaborator
There was a problem hiding this comment.
Approved — verified the addressed setup URL validation finding; no additional actionable issues.
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
2 participants
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.
The CLI half of the machine-pairing channel.
kcap setup <tenant>(or--server-url) now mints a pairing on that server, prints the human code beside a fallback link, opens the browser, and waits for someone to approve before going on to sign-in.Auth/BrowserPairingFlow— mint, show, open, poll. The code is printed in the terminal every time: the browser showing a code is only half a comparison, and the half that makes it a defence is the one printed by the machine being paired. The fallback link is equally non-optional, because nothing can confirm a browser actually opened.Auth/PairingIdentity— asserts that the account which approved is the account the CLI then authenticates as, and stops setup if it is not. The channel carries approval and never a credential, so nothing else binds the two. Reads the claims each provider actually mints —github_idplus agithub|{n}subject for GitHub,subverbatim for WorkOS — becausekapacitor:user_idnever goes on the wire; a claims transformation adds it per request. Distinguishes "somebody else approved" from "this build cannot tell", which need different remedies.Auth/PairingPoll— a pure classifier in the shape ofProvisioningPoll, so every branch is covered without a server.401is terminal here rather than transient: there is no token source to refresh on the next tick, the secret was minted once.Auth/PairingClient— degrade-don't-throw onTenantProvisioningClient's convention, behind anIPairingChannelseam so the loop is testable.Auth/SystemBrowser— hoisted out ofLoopbackBrowser's private opener so there is one, and one place saying why it is best-effort.Commands/SetupCommand— Step 1b between server resolution and login; the identity assertion immediately after login;/completelast, because completion invalidates the secret and so closes the channel this machine reports progress on.Availability needs no flag and no version check. The routes exist only when the tenant has
Features:FirstRunSetupon, so a404on mint is the oracle — as are401/403/405, which a gateway can answer on an unmapped anonymous route and which all mean the same thing and have the same remedy. Headless,--no-promptand theNoneprovider skip it for the same reason. In every one of those cases setup behaves exactly as it does today.Reviewed before opening. Three passes (general, architecture, conventions) over the first commit; the second commit is what they found. Worth calling out two:
TryGetPropertythrows there and onlyFormat/Jsonexceptions were caught.Also fixed: the poll compared the server's
expires_atagainst the local clock (a fast clock gave up without polling once, on a live pairing); an approval naming no approver returned a result that degraded to "carry on without the check";/completediscarded the403that is the server disagreeing about who approved; a mint missingexpires_ataborted setup claiming expiry.kcap setupgains no new flags, and--no-promptbehaviour is unchanged.Testing. 30 new tests over the flow, the client wire shape and the identity comparison — the deadline, the back-off, the guards and the header/body casing, none of which had coverage. 1949 pass in
Capacitor.Cli.Core.Tests.Unit; AOT publish is clean of IL2026/IL3050.Deliberately left. The check lives in
SetupCommandrather thanOnboardingFacade's commit boundary. Moving it means threading an expected identity throughLoginAsync, which every caller shares — worth doing when the Avalonia wizard is a real surface, not on this ticket.Closes #604
AI-2027