Repository navigation
Conversation
|
@MisonL is attempting to deploy a commit to the Crafter Station Team on Vercel. A member of the Team first needs to authorize it. |
The page the browser lands on after `petdex login` was two unstyled strings from the 0.1.0 commit: no mark, no type, no viewport, no way out, and the same page for all three refusal reasons. It now renders from `callback-page.ts` using the app's own tokens — the card surface, the brand purple, the CTA's three-layer shadow and the monospace eyebrow all come from `src/app/globals.css` — in the browser's language (en/es/zh), with light and dark from `prefers-color-scheme`. The success page tries to close its tab after five seconds and gives up if the reader touches anything first; both pages offer a button through to the profile. The response is also no longer sent the moment the code arrives. It was sent before the exchange, so a failed exchange reported success in the browser and failure in the terminal. `waitForCallback` now resolves without answering, and `respond(outcome)` writes the page once every step that can still fail has succeeded — including the keychain write, which is why success is answered after `setJson`. Splitting the old `settled` flag was necessary to do this: one flag meant both "the callback was consumed" and "the browser was answered", and those now happen at different times. `respond` is idempotent and never throws, `close` acts as a safety net for a flow that failed without reporting, and the harden headers are limited to four that cannot break a page served over http://127.0.0.1.
Two problems found while working on the callback page. `requestTokens` and `fetchUserInfo` used bare `fetch` with no signal, so a stalled connection left `login()` waiting forever — the callback server's timeout answered the browser, but nothing broke the request the CLI was sitting on. Both now carry a 15s deadline, matching what the OAuth smoke script already does for its authorize probe. `whoami` returned the cached profile without checking that a usable credential still existed beside it. `logout` clears both records through `Promise.allSettled`, so a half-cleared store leaves the profile behind with no tokens, and the command reported "signed in" for a session that would 401 on the next command — the one place the user cannot act on it. It now confirms the token first, which turns a confusing failure later into a prompt to sign in again. Also carries CLI-side localization for the messages these paths emit. Only the copy this flow owns is translated; the rest of the CLI's output is still English. Locale comes from the POSIX environment (LC_ALL, then LC_MESSAGES, then LANG), because a terminal has no URL and no i18n bundle. The website's message files cannot be reused here: the root tsconfig excludes `packages/`, and importing them would pull every UI string into the CLI bundle for a handful of error messages.
…ut page The phase-one deadline closed the listener as it fired. The reader that deadline just expired for is being redirected to this port at that moment, so closing first handed them the browser's own network-error page instead of the timeout page this flow renders for exactly that case. The listener now stays up until a callback is answered or `close()` runs in the caller's `finally`, which already covers the read that never arrives. Verified across the reachable paths: a late valid callback gets the 500 timeout page, a late callback that fails state validation still gets its 400, and the normal path still releases the port on `close()`. Also drops a constant that only restated its single call site's literal.
A request that cannot prove it carries the state this server generated can reach loopback from any page the reader has open, and does not need to read the response to have an effect. `/callback?error=x` and even a bare `/callback` were checked for `error` before `state`, so either could reject the callback promise — cancelling a sign-in the caller knows nothing about, repeatably, without authenticating. Check `state` first and split the refusal in two: - `answerOnly` sends a page to the request that made an unproven claim and touches nothing else: no flag, no deadline, no promise. - `refuseLogin` ends the attempt, and is now reachable only once the state matches, so only the browser the login was actually issued to can decide it. A forged request now leaves the login pending until its own timeout. The existing test for a mismatched state asserted the old behaviour and now asserts that it does not settle the login; the forgery cases cover all five shapes that used to work. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The app URL reaches an `href`, and escaping does not make it safe: `javascript:` and `data:` are valid URLs, so both survive `escapeHtml` intact and clickable. A wrong `PETDEX_URL` — the one input this takes — would put a live script link on the callback page. Resolve the URL and keep the configured value only for http and https, falling back to the production origin otherwise. A misconfigured origin is a configuration error, and the reader should not be handed a link from it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The security headers were a property of whichever branch answered: the two 404s sent a bare Content-Type and the superseded-response 409 sent a partial set. All three now go through shared builders, so nosniff, no-referrer, no-store and X-Frame-Options hold for the whole listener. The HTML answers also gain a Content-Security-Policy of `default-src 'none'`. The page is self-contained by construction, so nothing needs to be allowed in, and an injected element can no longer load a script, an image or a beacon — the exfiltration path a page on loopback would otherwise offer. `'unsafe-inline'` is required for the inline style and scripts, which is also what keeps the `successHtml` / `errorHtml` injection points working unchanged; this is defence in depth, and the escaping of every value that reaches the page remains the actual guarantee. Verified in a headless browser: the page still renders and its own script still fills in the strings, while a probe page under the same policy has its outbound fetch blocked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bound added to `token-exchange.ts` had no direct test: the login-flow suites reach those functions but never assert the signal, so dropping it would have gone unnoticed until someone's network stalled. Assert the signal is present rather than waiting out the 15s deadline. `AbortSignal.timeout` aborting is Node's guarantee, not this module's; what belongs to the module is remembering to pass it, and that is what is checked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment claimed the app policy carries `upgrade-insecure-requests`, which would rewrite this page's loopback origin to https and break it. That is not what happens: loopback is a potentially-trustworthy origin, so browsers exempt it from the directive. Measured, with the directive set and a same-origin subresource requested over http: on `http://127.0.0.1` the subresource loads, and on `http://example.test` resolved to the same address it is upgraded to https and fails. So the directive is a non-issue here and the real reason is the one the comment now gives — the app policy allowlists Clerk, R2 and the analytics hosts, none of which a self-contained page needs. The policy itself is unchanged; only the reasoning was wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects on the failure pages, both found by running the page's own script rather than reading its markup. The detail line was rendered into `<p class="detail" data-copy="resource">`, and the locale script then wrote `resource` over it — a key that is the empty string in all three tables. Every failure page therefore showed an empty bordered rule where the reason belonged, including the provider text the timeout and exchange paths pass in. The paragraph now carries no role, so the script leaves it alone. The error copy for the call to action was written in all three locales and pushed into the table, but the anchor is only emitted on success, so the reader never saw it. It described an action the page does not offer — a retry has to happen in the terminal — so the strings are removed rather than a link invented for them. The failure pages render exactly as before. The tests only ever asserted the raw markup, which is why both survived; the new ones execute the script and check what a reader would actually see. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`new URL(req.url, "http://127.0.0.1")` is the one unguarded call in a handler that wraps its other three writes in try. It throws on request lines that are valid to send but not valid URLs — `GET //` is the cheapest, and `///`, `http://[` and `http://:80/` all do it too. The throw is uncaught, so it takes the whole process down while the user is mid-login: no page, no message, the CLI just exits. Reproduced against the module: a raw `GET //` on the callback port ends in `UNCAUGHT: Invalid URL`. It needs no attacker — a stray request, or any page in the reader's browser that can reach loopback, is enough. An unparseable target is simply not the callback, so it now gets the same 404 a stray path gets. The regression test sends raw request lines over a socket, because fetch normalises the target before it goes out and never reaches the handler with the malformed form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The case makes four raw round-trips before it reaches the real callback, and it ran them against the default 150ms deadline. That deadline is a budget for the reader's browsing, not for the test's own traffic: on a loaded machine the first-phase clock expired mid-case, `waitForCallback()` rejected with `timeout`, and the failure read as a broken server rather than a slow one. Reproduced deterministically by delaying 200ms between the raw requests and the callback — the old code answered 500 and rejected, and now resolves. The loopback round-trips themselves cost about 25ms, which is why it surfaced roughly once in eight runs and not on every one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A connection accepted before the listener closed can have its request parsed after `respond()` has already run — a reader reloading the finished page, or a stale tab. That request matched the callback branch and was held, but `sendOutcome` returns early once `responseSent` is set, so nothing could ever answer it: the tab spun until the process exited, and the socket pinned the event loop in the meantime. Answer it for itself, the way a request with the wrong state is answered. The state check still runs first, so this only ever replies to a caller that already knows the state this server generated. Measured before the fix: a socket opened, then `respond()` called, then a request written to that socket, got no response at all. It now gets a 409. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The angle-bracket escape in `safeJson` had no test that could fail: the three locale tables contain no `<`, so asserting on a rendered page proved nothing — deleting the escape left every assertion green. Export the function and feed it `</script><script>alert(1)</script>`, then assert both that no raw `<` survives and that the result still parses back to the original string. Removing the escape now fails the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both cases asserted `signal instanceof AbortSignal` and `aborted === false`. A plain `new AbortController().signal` satisfies both while never aborting, so swapping `AbortSignal.timeout(n)` for one left the suite green — the assertion could not see the thing it named. Spy on `AbortSignal.timeout` and assert one bounded deadline is requested per call. The controller substitution now fails two cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The page fell back to whatever sans-serif the OS had, so a login landed on something that did not look like Petdex at all. It now carries Geist, the face the website is set in, inlined as base64 woff2 in two @font-face blocks. Inlining is the only option rather than a preference: `auth-server.ts` closes its listener the moment it answers, so the page can never fetch a font file. That is also why the CSP gains `font-src data:` — the one directive the faces need, and one that cannot reach the network. The subsets come from the `geist` npm package (v1.7.2, SIL OFL 1.1), cut to Latin-1 plus the punctuation the page can render, with the weight axis clamped to 400-600 and only the four layout features the Latin text uses. That is 17.2KB and 8.3KB, about 34KB of base64. `scripts/build-callback-fonts.ts` regenerates them; it is not a build step, because it needs Python and the package tarball, neither of which the published CLI should depend on. The stack names Geist first and keeps the system faces behind it, which is what keeps the Chinese copy working: Geist carries no CJK glyphs, so those characters fall through to the system face exactly as they do on the website. Verified in headless Chrome — `document.fonts` reports both faces `loaded`, the Latin text measures differently from `system-ui`, and the CJK text measures identically to it. Also adds `<meta name="color-scheme">` ahead of the stylesheet, so the first frame is painted with the right background instead of flashing the light one. A first attempt at this shipped a page whose fonts were 72 bytes. The generated module wrapped the base64 across adjacent string literals, which JavaScript does not concatenate — only the first line bound, and the rest became dead expressions. The build, the type check and every existing test passed, because nothing asserted the font was actually in the document. There is now a test that decodes each data URI and checks for the woff2 magic; reverting the fix fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The page keys its dark styles off `prefers-color-scheme` rather than the site's `.dark` class, and the comment now says why that is not a shortcut: the class comes from `next-themes` state in the app's origin, which this page never visits, and no channel exists to carry it over. Checked against the products that solve the same problem. GitHub's OAuth consent page ships `data-color-mode="auto"`, which resolves to the media query for a logged-out reader; WorkOS AuthKit, whose hosted page faces the same unreachable-preference case, falls back to `prefers-color-scheme`; AWS CLI moved its SSO page to `light-dark()` in November 2025. No hosted auth product accepts a theme from the authorize URL, and Clerk rejects a `redirect_uri` carrying a query string, so smuggling one through is not an option either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Regenerating the fonts from a clean environment produces a file that differs from the committed one, and the note now says why rather than leaving the next person to rediscover it: woff2's brotli compression is not byte-stable across fontTools or brotli versions, so `head.checkSumAdjustment` — a checksum over the compressed file — moves with it. Measured, not assumed: zeroing that field makes the two files byte-identical, and the glyph sets, cmap, axes and name records already match. The faces render the same; only the checksum differs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test opened a raw socket, saw connect() resolve, and never used it until after respond() — but connect() only completes the TCP handshake: on Linux the server's accept can still be pending, the socket sits in the listen backlog, and closeListening() resets it (ECONNRESET, no request bytes ever read), so the assertion got "error" instead of the 409. On macOS the accept arrives just in time, which is why it passed locally and failed only on ubuntu CI. Send the first line of the request before the login is answered. That reaches an accepted connection — the state the guarantee is about — and leaves the request incomplete, so nothing parses until the second line goes out after respond(). Reproduced on Ubuntu 24.04 + Bun 1.4.2 in Docker (fail before, 3x pass after); the macOS suite still passes 231/231. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MisonL
force-pushed
the
feat/cli-auth-callback-page
branch
from
October 9, 2026 15:00
f301e46 to
b8f4326
Compare
This branch has not been deployed
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.
Summary
When you run
petdex login, the CLI opens your browser to a page on its ownlocalhost port to finish the OAuth dance. This branch rebuilds that page into
something that looks and behaves like part of Petdex — and the rebuild turned
up seven real defects around it, all fixed here.
18 commits · 14 files · +2,872 / −70, all inside
packages/petdex-cli.What you see
type (Geist, inlined as ~34KB of woff2 — inlining is required, the server
closes its socket the instant it answers, so no font file can ever be
fetched), system dark-mode support via
prefers-color-scheme, and all threelanguages (en / es / zh) shipped in one document with a tiny script picking
by browser locale, since the CLI can't know your language.
/my-pets, which is correct for both asigned-in and a not-yet-signed-in reader.
wrote an empty-string key over it. Now the reason actually shows.
Login that no longer hangs or dies
stale tab) used to get no response at all — the socket spun and pinned the
process. It now gets a 409.
reader being redirected at that moment got the browser's raw network-error
page instead of the timeout page. The port now stays open until the flow is
truly done.
GET //, or any page on your machine hitting theloopback port) crashed the whole CLI with an uncaught
Invalid URLmid-login.It's a 404 now.
flow no longer reports a stale session as current.
traffic is no longer racing the reader's deadline.
Hardening of the page itself
reach loopback, and
/callback?error=xused to be able to cancel anin-progress sign-in it knew nothing about. Forgeries now get their own page
and can't touch the login.
http(s):origins — escaping alonewouldn't stop a
javascript:URL.default-src 'none'CSP, and the JSON injection point has a test thatactually drives
</script><script>through it.Tests and docs that check the right thing
satisfied by a signal that never aborts. They now spy on
AbortSignal.timeout.the "fonts were 72 bytes" bug fails the suite.
upgrade-insecure-requests;measurement showed the directive never applied to loopback — the reasoning is
corrected (the memory of that one lives in the repo now).
How to review
Commit by commit (
git log --oneline origin/main..feat/cli-auth-callback-page):each commit message carries the reproduction or measurement behind the change.
Relationship to #819
Independent work, separate PR on purpose: #819 is site/API/deployment fixes,
this is the CLI login page. Both branches are cut from the same
upstream/maintip (
7e327034c) and neither is behind it, so either can merge first. Theytouch three of the same CLI files (
clerk-cli-auth.ts,clerk-cli-auth.test.ts,token-exchange.ts) with different changes, sowhichever merges second needs a trivial rebase — say the word and I'll rebase
whichever one you want in first.
Verification
Run on this branch today:
bun run build+bun run typecheckinpackages/petdex-cli— cleanbun run test— 231 pass / 0 fail across 18 filesPer-commit messages also record measurements taken when each change was made
(headless-browser font and CSP checks, reproduced crashes before fixing).
Those are stated in the commits; today's run only re-verified the suite and
build.
One test needed a platform fix after the first CI run: the late-callback case
opened a raw socket and assumed
connect()meant the server had accepted it —on Linux the accept can still be pending, the listener close resets the socket,
and the assertion got
errorinstead of409(it passed on macOS). Therequest is now sent in two pieces so it provably reaches an accepted
connection. Reproduced on Ubuntu 24.04 + Bun 1.4.2 in Docker — fail before,
3x green after — and verified there with the same commands CI runs
(
bun run build,bun run typecheck,bun test, 231/231).Boundaries — what I did not verify here
committed bytes: woff2's brotli compression isn't byte-stable across
fontTools/brotli versions, so the checksum field moves. Glyphs, cmap, axes
and name records match; the faces render identically. Documented in the final
commit rather than papered over.
paths are covered by the package's own socket-level integration tests.
🤖 Generated with Claude Code