fix(login): device-flow link re-anchored onto the environment console; prompt names the deployment (#134) - #135
fix(login): device-flow link re-anchored onto the environment console; prompt names the deployment (#134)#135tonychang04 wants to merge 2 commits into
Conversation
…; prompt names the deployment (#134) The staging platform mints its RFC 8628 verification_uri from its own INSTA_CONSOLE_URL, which points at the staging landing site - a host that serves no /device page (the link passes Vercel SSO, then 404s). For a known environment the CLI knows the console host itself, so re-anchor the link's origin there (path + user_code stay the server's; a correctly configured platform is a no-op, custom/self-hosted hosts are never rewritten). Also name the deployment in the prompt ("to log in to STAGING") so a code can't be approved against the wrong environment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…bic P2) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pX2DU2zrJHcF7VJifurYo
jwfing
left a comment
There was a problem hiding this comment.
Summary
No blocking issues found; the device-flow URL rewrite and prompt labeling are scoped to the reported staging-login failure.
Requirements Context
I used the PR description, linked issue #134 (staging insta login --device --env staging prints a landing-host /device URL that 404s and should label STAGING), README environment/authentication docs, and the repo dev notes in .claude/skills/developing-insta-cli/SKILL.md. The documented intent is to keep prod/staging separated, preserve explicit custom INSTA_API_URL / --api-url targets, and improve the device-login prompt without changing command flags.
Findings
Critical
(none)
Suggestion
(none)
Information
src/commands/auth.ts:61-156,src/env.ts:36-47,test/device-login.test.ts:147-210: Software engineering/functionality review found the implementation aligned with the stated requirements. Known envs are inferred from the resolved API URL, only the approval URL origin is rewritten, and tests cover staging wrong-host rewriting, prod no-op behavior, custom-host pass-through, unparseable strings, prompt labels, and pinned console hosts.src/commands/auth.ts:130-156: Security review found no new token/secret exposure or auth weakening. The code prints the existing device approval URL/user code flow and constrains known-env approval links to the pinned console origin.src/commands/auth.ts:130-156: Performance review found no relevant risk; the added work is one URL parse/reconstruction during interactive login, with no new hot-path loops, blocking I/O, dependencies, or network calls.package.json:35-37: Verification note:npm run typecheckcould not run in this checkout becausetscis unavailable (sh: tsc: not found). I did not run Vitest because the review request was explicitly read-only and the suite creates temporary files.
Verdict
Approved per the requested rule: zero Critical findings. This is a bot-review approval verdict, not a GitHub green-check approval.
jwfing
left a comment
There was a problem hiding this comment.
Summary
Tightly scoped, well-tested fix: the device-login approval link is re-anchored onto the environment's own console origin and the prompt now names the deployment — no Critical findings, three non-blocking Suggestions (one real test-coverage gap, mutation-proved below).
Requirements context
No matching spec/plan found — this repo has no /docs/superpowers/ (or any docs/) directory at head 31d3e8a. Assessed against issue #134 and the PR body.
Issue #134 asks for two things, and both are delivered:
- "The staging device-approval page must exist at whatever URL the CLI prints (either add the route to the staging frontend, or point the CLI at the host that serves it)" → this PR takes the second option.
- "the device-flow output should label the environment explicitly ('log in to STAGING')" →
src/commands/auth.ts:152-155.
I independently verified the PR's root-cause chain rather than taking the body's probe log at face value:
src/app/device/page.tsxexists on bothmainandstagingofInsForge/insta-frontend(GitHub contents API, HTTP 200 on each ref) — so the route does ship to staging.insta-frontend@staging.github/workflows/deploy-staging.yml:41setsSTAGING_HOST: console.staging.instacloud.com, and the workflow aliases the staging deployment to it. So the pinnedENVS.staging.consolevalue is the host that actually serves thestagingbranch. (This host assignment landed in insta-frontend#237, which also deliberately reassignedstaging.instacloud.comto the landing/marketing staging site — exactly the PR's diagnosis.)- Issue #134 explicitly left this unverified ("
console.staging.instacloud.com/deviceis behind the same SSO (unverified whether the route exists there)"). The two checks above close that gap; the PR is pointing at the right host.
Both hosts are behind Vercel deployment-protection SSO, so /device cannot be probed anonymously on staging — the branch+workflow evidence is what settles it.
Findings
Critical
(none) — no correctness bug, no security hole, no requirement violation, no regression risk I could construct. npx tsc --noEmit clean and npx vitest run 490/490 across 38 files green in a fresh clone of 31d3e8a.
Suggestion
1. Functionality / test coverage — the production wiring is untested; the whole feature can regress silently
src/commands/auth.ts:61-65, test/device-login.test.ts:142-213
Both new behavioural tests call deviceGrant(post, wait, target) directly. Nothing exercises loginDevice's job of constructing that target from envForApiUrl(api.apiUrl). Because target?: DeviceTarget is optional (auth.ts:141), dropping it at the call site is not a type error either.
Mutation-proved in the workspace — I replaced the call site with the pre-PR form:
const token = await deviceGrant((path, body) => api.request('POST', path, body, { auth: false }))Result: npx tsc --noEmit clean, npx vitest run 490/490 passing. Every user-visible effect of this PR is gone and the gate stays green.
Cheapest fix: make target required on deviceGrant (it is an internal export with exactly one production call site — grep confirms), so the compiler owns the wiring. Or add a loginDevice-level test with a fake ApiClient asserting the target handed to the injected grant.
For contrast, the behaviours that are covered are genuinely load-bearing — I mutation-tested them too:
- deleting
${url.hash}fromauth.ts:135→ 1 failure (the fragment test attest/device-login.test.ts:194-197is real, not decorative); - short-circuiting
deviceVerificationUrlto always returnraw→ 3 failures; - dropping the
to ${where}label fromauth.ts:155→ 2 failures.
2. Functionality — the rewrite silently papers over the platform misconfiguration it exists to survive
src/commands/auth.ts:130-136
When the rewrite fires, the user sees a working link and no one learns the platform is misconfigured. That is the whole point in the moment, but it also means INSTA_CONSOLE_URL can stay wrong indefinitely with zero signal — and the PR's own "Not in this PR" follow-up is the kind of item that only gets done if something keeps complaining.
This is not hypothetical: staging has already been corrected server-side. Live probe today (2026-08-25):
POST https://api.staging.instacloud.com/api/auth/device/code
→ "verification_uri_complete": "https://console.staging.instacloud.com/device?user_code=…"
POST https://api.instacloud.com/api/auth/device/code
→ "verification_uri_complete": "https://console.instacloud.com/device?user_code=…"
So the rewrite is a no-op on both environments as of now — which is the strongest possible argument for making the non-no-op case loud. Suggest a one-line stderr note when url.origin !== consoleOrigin, e.g. note: the staging platform minted an approval link on https://…; using the known console host instead.
The same argument covers the staleness direction: pinning console origins client-side means a published CLI will rewrite a legitimately-changed future console host back to the stale one. ENVS[*].console should join the release checklist alongside api/mcp.
3. Security — arbitrary server-supplied path/query/fragment is grafted onto the trusted origin
src/commands/auth.ts:135
return url.origin === consoleOrigin ? raw : `${consoleOrigin}${url.pathname}${url.search}${url.hash}`Everything except the origin comes from the server, and it is then rendered under a host the user has every reason to trust. The console does carry a redirect parameter — probing https://console.instacloud.com/device?user_code=TEST returns 307 → /signin?next=%2Fdevice%3Fuser_code%3DTEST — so a platform minting …/signin?next=https://evil.example would have the CLI print a link on the genuine console.instacloud.com origin.
Non-blocking, and this is strictly better than the pre-PR behaviour (which printed the hostile origin verbatim): the precondition is a hostile or badly-templated auth server, which already owns the session anyway. But since the CLI now knows the console host, it costs nothing to also pin the path and carry only the code — nothing server-supplied would land on the trusted origin except user_code:
const u = new URL('/device', ENVS[env].console)
const code = url.searchParams.get('user_code')
if (code) u.searchParams.set('user_code', code)Note this changes the "unknown env-null target" path not at all, and would need the verification_uri (non-complete) case to skip the query.
Everything else on the security dimension is clean: no new dependencies; no new user input reaching SQL/shell/HTTP; device_code is still never printed (only user_code, which RFC 8628 §3.3 intends to be displayed, and which was already printed pre-PR at auth.ts:157); no auth check weakened — the poll loop, token validation and error handling at auth.ts:159-188 are untouched. The non-http(s) case is safe by accident and in the right direction: new URL('javascript:…') has origin "null", so it never matches consoleOrigin and gets defused into a harmless https://console…/… path.
Information
- The PR body's stated bug no longer reproduces. Per the probe above, staging now mints the console host, so the "Not in this PR" follow-up (
INSTA_CONSOLE_URL=https://console.staging.instacloud.com) appears to have already been applied. Worth saying so when closing #134, so the issue records the actual root-cause fix (a platform env var) rather than reading as if the CLI change was the cure. - The PR's "5 pre-existing local failures" did not reproduce. In a clean clone +
npm ci, the full suite is 490/490 / 38 files green — consistent with the author's ambient-local-agent-config explanation. No action. - Library-API check. Verified
deviceAuthorization's server-sideverificationUrioption and the optionalverification_uri_complete/intervalresponse fields against the Better Auth v1.6.23 docs. The root-cause account and theverification_uri_complete ?? verification_urifallback atauth.ts:156both match the current API — no stale or invented usage. - Signature shape.
targetas a third positional after a defaultedwaitforces theundefinedplaceholder atauth.ts:63. An options object would read better, but a 3-arg internal helper is well within the repo's existing style. Pure taste. - Conventions are respected.
.jsESM import specifiers,type-only import forEnvName(auth.ts:4), DI-fake tests with no global module mocking, and comments that explain why, matching the density of the surrounding file. AGENTS.md rule 4 (mirror command/flag changes intoskills/insta/cli-reference.md) does not apply — no command or flag surface changed, only output text. Worth a glance only if that reference quotes the device-login output verbatim. - The no-op guard at
auth.ts:135is not what its test proves.test/device-login.test.ts:199-202would still pass ifurl.origin === consoleOrigin ? raw :were deleted, because reassembling an already-correct URL yields the identical string. The ternary does earn its keep on round-trip fidelity (a barehttps://hostwould otherwise gain a trailing/), but that isn't what the test name claims. Minor.
Performance
No performance-relevant changes. Two new URL() parses per insta login --device invocation, on a path that then blocks on a human for up to 15 minutes. No loops, no I/O, no allocation growth, no DB queries.
Verdict
approved — zero Critical findings. (Informational: the green checkmark is a separate human action; this bot only posts COMMENT.) The three Suggestions are all worth a follow-up, and #1 (making target required so the wiring is compiler-enforced) is a genuinely cheap win.
Fixes #134.
Root cause
The CLI never builds the device-approval URL — the platform does, from its own
INSTA_CONSOLE_URL(Better AuthdeviceAuthorization({ verificationUri:${cfg.consoleUrl}/device})in insta-platform). The staging deployment'sINSTA_CONSOLE_URLpoints at the staging landing host instead of the staging console, so staging mints approval links onto a host that serves no/devicepage.Probe evidence (2026-08-24):
POST https://api.staging.instacloud.com/api/auth/device/code→"verification_uri": "https://staging.instacloud.com/device"— the landing host.POST https://api.instacloud.com/api/auth/device/code→"verification_uri": "https://console.instacloud.com/device"— the console host.https://console.instacloud.com/device?user_code=TEST→307 /signin?next=%2Fdevice%3Fuser_code%3DTEST— the route exists on the prod console.https://instacloud.com/device?user_code=TEST→308 → www.instacloud.com→ 404 — the landing site serves no/deviceanywhere, so staging's link can only 404 (behind Vercel SSO, exactly as reported)./deviceapproval page lives in insta-frontend (src/app/device/page.tsx), which is present on itsstagingbranch (deployed toconsole.staging.instacloud.comper that repo's README) — so the staging console does serve the route; the platform just points at the wrong host.The lasting fix is one env var on the staging platform deployment:
INSTA_CONSOLE_URL=https://console.staging.instacloud.com(currently the landing origin). This PR makes the CLI robust to that class of misconfiguration and closes the UX nit.What this PR does
deviceVerificationUrl). Each env inENVSnow carries itsconsoleorigin (probed above). For a known env, only the URL's origin is replaced — path anduser_codequery stay exactly what the server minted, so a correctly configured platform is a no-op (prod already is). Custom/self-hosted--api-urltargets (env === null) are the user's deliberate choice and are never rewritten; an unparseable server string is shown as-is.to log in to STAGING, open this link…for named envs,to log in to https://insta.local:7130, …for custom hosts.Tests
deviceVerificationUrl: wrong-host staging link re-anchored withuser_codepreserved; already-correct prod link untouched; custom host (env null) untouched; unparseable string passed through.deviceGrantoutput: staging run printsto log in to STAGING+ the console-staging link and never the 404 landing link; custom-host run prints the apiUrl label and the server's link verbatim.env table: prod/staging console hosts pinned to the probed working values.Ran
npm run typecheckand the fullvitestsuite against this branch: the two touched files pass 63/63; the suite's only failures (5, insetup-agent/ensure-skills) are identical on pristinemainon the same machine (ambient local agent config), i.e. pre-existing and unrelated.Not in this PR
INSTA_CONSOLE_URLshould still be corrected tohttps://console.staging.instacloud.com(deployment env, not in any repo I can PR). Until then, this CLI fix alone makesinsta login --device --env stagingprint a working link.🤖 Generated with Claude Code
Summary by cubic
Fixes device login by re-anchoring the device-approval link to the environment’s console and naming the target deployment in the prompt. Previously, staging links pointed at the landing host (no /device) and 404’d; now known environments rewrite only the link origin while preserving the server-provided path and user_code.
INSTA_CONSOLE_URL=https://console.staging.instacloud.comon the staging platform.Written for commit 6c282fe. Summary will update on new commits.