Repository navigation
Test that the app's /cli-auth forwards to the secure origin when sign-in is on the secure origin - #6958
Conversation
With sign-in on the secure origin, CliAuthRoute.beforeModel sends the browser from the app's /cli-auth to the secure origin's cli-auth mode with the CLI's port and state. Nothing exercised that redirect: the existing specs open the secure origin's link directly, and both pages carry the same data-test-secure-* attributes, so the new spec asserts on page.url(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review read the new spec against CliAuthRoute.beforeModel, the host's CLI page, the secure origin's cli-auth mode and its cliCallback checks, and the URL boxel builds for the app's page. It did not run the matrix suite, so whether the spec passes rests on the author's local run and on reading the waits.
Bottom line: no blocking issues. The spec pins what it says it pins: it waits on the secure origin's origin and then checks mode, port and state by address, so neither the app's own CLI page nor a fall-through to the sign-in mode can satisfy it.
- Non-blocking follow-up: drive the spec with the CLI's own
browserLogin({ hostUrl })and finish the sign-in, so the hand-off a forwarded CLI receives gets end-to-end coverage (see the inline comment on thepage.goto).
CI: Lint and the test-asset build were still running when this was posted, and the matrix shards had not reported yet.
| let port = '53412'; | ||
| let state = 'cli-auth-state-0123456789'; | ||
| await page.goto(`${APP_ORIGIN}/cli-auth?port=${port}&state=${state}`); |
There was a problem hiding this comment.
[Claude Code 🤖] Could this spec open the page with the CLI's own browserLogin instead of a hand-built URL, and carry on through the sign-in to the listener? The forwarded flow ends in a hand-off that no matrix spec completes.
The app's /cli-auth forwards only port and state. So the secure origin's cli-auth page sees no session=secure and takes the profile: { secureSession: false } branch of handProfileSignIn, which hands the listener a ?loginToken= navigation. cli-auth-secure-origin.spec.ts always builds its URL with buildSecureCliAuthUrl, which adds session=secure, so it only completes the POST branch. The secureSession: false branch is covered only by unit tests in packages/secure-origin/tests/sign-in.test.ts. Taking the URL from buildCliAuthUrl also replaces the hard-coded port/state here with what boxel really sends. Its state is 32 hex characters, which cliCallback accepts.
A sketch, reusing the shape of signInForProfile and expectOwnDevice from cli-auth-secure-origin.spec.ts:
let auth = await browserLogin({
matrixUrl: getMatrixTestContext().matrixUrl!,
hostUrl: new URL('/', appURL).href, // no secureOriginUrl: opens the app's /cli-auth
log: () => {},
openBrowserFn: async (link) => {
await page.goto(link);
await page.waitForURL(
(url) => url.origin === new URL(secureOriginURL).origin,
);
// the address assertions this spec already makes
await signInOnSecurePage(page, username, password);
return true;
},
});
expect(auth.secureSession).toBeUndefined();
// then the whoami check expectOwnDevice makesThis is a follow-up, not a blocker. The spec as written does pin the redirect it claims to.
There was a problem hiding this comment.
[Claude Code 🤖] Done in a2fd2c4. The spec now runs the CLI's own browserLogin with only a host URL, so the app's /cli-auth URL, port and state are what boxel really sends. It signs in on the secure origin and waits for the listener to receive the login token. Then it checks that browserLogin returns no secure session, and that the redeemed device is the user's own and answers whoami. Passes locally with the rest of the file.
The spec now takes its URL from browserLogin's buildCliAuthUrl and signs in on the secure origin, so the CLI's loopback listener receives the sign-in. That completes the secure origin's login-token hand-off for a CLI that didn't ask for a secure session, which no matrix spec reached: cli-auth-secure-origin.spec.ts always adds session=secure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The
boxelCLI authorizes a machine by opening the app's/cli-auth?port=…&state=…in a browser.portis where the CLI's loopback listener waits for the sign-in to come back, andstateis a value the CLI checks when it does. When a deployment puts sign-in on the secure origin, the app must never draw a password field of its own. SoCliAuthRoute.beforeModel(packages/host/app/routes/cli-auth.ts) sends the browser straight on to the secure origin'scli-authmode with the sameportandstate, and holds the transition so the app's own CLI page never renders.No test covered that path.
secureCliAuthURLbuilds.cli-auth-secure-origin.spec.tsopens the secure origin's link directly, as a CLI that knows the secure origin does, and never visits the app's/cli-auth.session=secure. So the other way the secure origin can finish a CLI sign-in was never run end to end in a browser: handing the listener a Matrix login token alone, by navigating the browser to it. That is the way a forwarded sign-in finishes, because the app forwards onlyportandstate.The gap was easy to miss because the app's CLI page and the secure origin's draw the same sign-in screen from the secure origin package. Both carry the same
data-test-secure-*attributes under the same "Authorize Boxel CLI" title, so a check written against those attributes passes on either origin.This PR adds one spec to
packages/matrix/tests/sign-in-on-secure-origin.spec.ts, whosesignInOnSecureOriginhelper already turns the mode on in the host's config. It runs the CLI's realbrowserLoginwith only a host URL, asboxel profile adddoes when it doesn't know the deployment's secure origin.browserLoginstarts a real loopback listener and builds the real/cli-authURL, with a real port and state, and the spec hands that URL to a Playwright page. The spec then checks four things:page.url()there hasmode=cli-authand the sameportandstatethe CLI put in the app's URL.browserLoginredeems it for a Matrix device.whoamiwith its access token answers with that user and device.Testing
I ran
sign-in-on-secure-origin.spec.tslocally on the matrix test stack against a host dist built from this branch. All seven tests pass.To show the redirect check can fail, I made
beforeModelreturn before redirecting, rebuilt the host and re-ran the spec. It timed out inpage.waitForURL, waiting for a secure origin the browser never reached.🤖 Generated with Claude Code