Multi-tenant OIDC browser login on the client (LLP 0046-0048)#204
Multi-tenant OIDC browser login on the client (LLP 0046-0048)#204platypii wants to merge 58 commits into
Conversation
Integration branch base for the chunk-2 OIDC client work. Carries the decision, design, and plan that the implementation PRs realize.
* remote/oidc: PKCE pair + ephemeral loopback receiver (T1, T2)
Two dependency-free local primitives for the browser login flow:
- pkce.js: createPkcePair() -> { verifier, challenge }, S256 over stdlib
crypto. The client's downstream PKCE leg (LLP 0046 D3).
- loopback.js: startLoopbackReceiver({ state, timeoutMs }) binds a
single-shot 127.0.0.1:0 HTTP listener serving /callback, returns its
redirectUri up front, and resolves { code } on a state-matched
callback (rejecting on mismatch, error=, or timeout). RFC 8252
ephemeral redirect (LLP 0046 D2).
Unit tests cover the SHA-256 challenge derivation, fresh randomness,
and the loopback success / state-mismatch / error / timeout paths.
* remote/oidc: loopback close() rejects pending waitForCode; route post-listen errors
Review follow-ups (PR #197):
- close() before a code arrives now rejects an in-flight (or future)
waitForCode() instead of leaving it permanently pending.
- a post-listen server 'error' now settles a pending waitForCode() and
closes the server (via fail()), not a no-op rejectStart.
- tests: close()-rejects-pending, and a non-/callback probe 404s without
consuming the single shot.
…3, T5) (#198) * remote/oidc: identity client, browser opener, login orchestrator (T3, T5) Completes milestone-1 local primitives, composing chunk 1's PKCE + loopback: - identity_client.js: exchangeCode / refreshSession over an injectable fetch against <origin>/v1/identity/token; a 401 invalid_grant surfaces a typed InvalidGrantError. Response field is access_jwt, expires_at is ISO. No external JWKS on the client. - open_browser.js: platform opener (open / xdg-open / cmd start), detached; returns whether an opener was found (LLP 0046 D8). - oidc_login.js: loginWithBrowser() orchestrates PKCE + a random state, starts the loopback receiver, builds the /login/start URL, opens the browser (or prints the URL), awaits the code, and exchanges it for the session. No persistence; the caller stores it (LLP 0046 D2/D3). - types.d.ts: OidcSession / RefreshedAccess shared interfaces. Unit tests cover the token request bodies + response mapping, the invalid_grant typing, each platform opener, and the full PKCE->loopback->exchange orchestration including --no-browser and loopback cleanup on failure. * remote/oidc: open_browser must not crash on async spawn ENOENT Review follow-ups (PR #198): - spawn delivers a missing-opener failure as an async 'error' event, not a synchronous throw. Without a listener it became an uncaught exception that crashed the CLI on the D8 no-opener path. Attach child.on('error') to swallow it; document the boolean return as best-effort. - oidc_login now always prints the URL as a fallback even when the opener reported success, so a silently-failed launcher never strands the user. - test now models spawn's async 'error' emission (EventEmitter child) and asserts no process crash; keep the synchronous-throw case too.
…(T4) (#203) * remote/oidc: discriminated credential record + session-aware resolve (T4) Extends the 0600 credential store to carry OIDC sessions alongside the existing static tokens (LLP 0046 D4), and adds the attach-path resolver (LLP 0046 D5): - Records gain a kind discriminator. readCredentials normalizes each record; a legacy token-only record (no kind) reads as static, so existing files keep working without a rewrite. writeToken now stamps kind: 'static'. - writeSession(stateDir, target, { refreshToken, accessJwt, expiresAt, org }) writes a kind: 'oidc' record through the same atomic 0600 path. - resolveAccessJwt({ target, env, stateDir, identityBase, now, fetchImpl }): env override wins; a static record returns its token; an oidc record returns a fresh access JWT, refreshing + persisting when the cached one is within a 60s skew of expiry, and propagating a refresh failure (typed invalid_grant). resolveToken stays for the stdio proxy. - removeToken drops either kind (whole-record delete, unchanged). - types.d.ts: RemoteStaticRecord / RemoteOidcRecord / the union. Unit tests cover the round-trip, legacy normalization, env override, fresh vs stale refresh + persistence, and failure propagation. Full suite green. * remote/oidc: normalizeRecord keeps a usable static token on a corrupt record Review follow-up (PR #199): an incomplete oidc shape (refreshToken but no accessJwt) on a hand-edited record no longer returns null and drop a working static `token`; it falls through to the static branch. Added boundary tests: the static fallthrough, a malformed-oidc drop, resolveToken on an oidc record, and a skew-window-boundary refresh.
* remote/oidc: browser mode for `hyp remote login` (T6) runRemoteLogin gains an interactive browser authorization-code mode (LLP 0046 D1), selected by default when no static token is supplied: - Flag parsing: --token-file / stdin keep the static path unchanged (kind: 'static', the headless escape hatch, D8); --org <name> selects an org; --browser forces the flow even with stdin piped; --no-browser prints the URL instead of opening it. - The identity base is derived from the configured target URL's origin as <origin>/v1/identity, so no second URL is configured (D6). - On success the resolved org is printed and the OIDC session is stored via writeSession. - A server-surfaced callback error (access_denied, no_membership, org_selection_required, org_not_permitted) is translated to a clear message; org_selection_required instructs a --org re-run rather than enumerating the user's orgs (D7). A small `deps.login` seam keeps the browser path unit-testable. Tests cover --org + identity-base forwarding, --no-browser, each error mapping, the unconfigured-target refusal, and the unchanged static token-file and piped-stdin paths. * remote/oidc: robust login flag parsing; note --org on the static path Review follow-ups (PR #200): - the target name now skips the value slot of --token-file/--org via a firstPositional() helper, so `login --org acme` (name omitted) is a usage error instead of misreading 'acme' as the target. - a static path with --org now prints a note that --org applies only to the browser flow, instead of silently dropping it. - tests: --browser overriding piped stdin, only-flags usage error, --org-missing-value exit, and the static --org-ignored note.
…T7) (#201) * remote/oidc: silent refresh + one-shot 401 retry on the attach path (T7) The query attach path becomes session-aware (LLP 0046 D5): - remote_verb.js resolves the bearer via resolveAccessJwt (not resolveToken), passing the origin-derived identity base. A stale cached JWT is refreshed and persisted before the call. - client.js tags a 401/403 rejection (authError + status) so the attach path can recognize it. On a live auth error for an oidc/file session, the attach path forces a single refresh and retries once; an env override or static token is surfaced as-is (nothing to refresh). - A refresh that fails invalid_grant surfaces "remote session expired - re-run 'hyp remote login <target>'". - resolveAccessJwt gains forceRefresh for the retry; deriveIdentityBase moves to credentials.js as the shared home for the login command and the attach path (single D6 definition). Tests: a stale stored JWT refreshes + persists pre-call; a mid-flight 401 triggers exactly one refresh + retry; invalid_grant maps to re-login guidance; a static 401 is not retried. Full suite green (1527). * remote/oidc: map invalid_grant on the initial resolve too (not just 401 retry) Review follow-up (PR #201): the initial resolveAccessJwt can itself refresh (and throw invalid_grant) when the stored JWT is already stale, which escaped as an unhandled rejection. Wrap it and map via a shared mapRefreshError helper, exactly like the mid-flight 401 retry path. Added a stale-JWT pre-call-refresh-fails unit test.
* remote/oidc: hermetic smoke + LLP follow-ups (T8) - remote_oidc_login smoke: one in-process server plays both the identity surface (/v1/identity/login/start + /token, signing real per-call tokens) and the MCP endpoint (/mcp, accepting only the current JWT). A scripted opener drives the real loopback redirect. Asserts the user-visible result and the remote-oidc smoke_step telemetry across: browser login -> session stored kind: 'oidc' -> query attaches the JWT -> forced expiry drives a silent refresh + persist -> a revoked refresh row drives the re-login message. - Fix surfaced by the smoke: when the stored JWT is already stale, the initial resolveAccessJwt refreshes pre-call and can throw invalid_grant outside the 401-retry try/catch. remote_verb now maps a refresh failure to the re-login guidance on both the initial-resolve and the mid-flight-401 paths (shared mapRefreshError). Added a unit test for the stale-initial-resolve path. - LLP 0033: its credential-store and attach sections now note the kind discriminator and the silent-refresh + 401-retry behavior, pointing at LLP 0046 D4/D5. - Promote LLP 0046/0047/0048 Draft -> Accepted now that the milestones land. All @refs resolve (ref-check clean). * remote/oidc: smoke convention + cleanup follow-ups Review follow-ups (PR #202): - hoist the inline import('node:http').IncomingMessage type to the top-level @import block (repo hard rule: no inline import() types). - drop runQuery's unused cmd param. - move obs.shutdown() into the finally and force-close keep-alive sockets (closeAllConnections) so the server does not wait on idle timeouts.
Code review: Multi-tenant OIDC browser login (LLP 0046-0048)Reviewed the PR diff (24 files): new Correctness
Cleanup
|
`cmd /c start <url>` treats `&` as a command separator, so the authorize URL (always multi-param: redirect_uri, code_challenge, state, ...) was truncated at the first `&`, opening a PKCE-less URL the server rejects. Spawn `rundll32 url.dll,FileProtocolHandler` directly instead, so the URL reaches the handler verbatim with no shell parsing.
The `hyp mcp --remote` proxy resolved the bearer token once via the non-refreshing resolveToken and reused it for the whole long-lived proxy, so an OIDC session's short-lived access JWT expired and every forwarded message returned a generic HTTP 401 with no refresh and no re-login hint. Resolve a fresh access JWT per forwarded message via resolveAccessJwt (env/static tokens pass through unchanged), refresh once and retry on a live 401/403, and map invalid_grant to "re-run 'hyp remote login'". LLP 0033 now documents that the proxy fallback shares the session-aware path.
refreshSession hard-required `org`, so a refresh grant that re-mints only the access JWT (org is fixed for the refresh token's life) threw a misleading "missing org" error and no near-expiry session could refresh. Treat org as optional on refresh and keep the org already stored. Also share trimSlash between identity_client and the start-URL builder, and correct the resolveToken doc now that the proxy refreshes too.
Any non-TTY stdin (a wrapper, an IDE terminal, /dev/null) routes login to the static path, so an empty stdin failed with a bare "empty token" even though browser sign-in is the default. The error now points at --browser. Also unify positional parsing: the value-flag-aware parser firstPositional becomes positionals(), used by add/remove/login alike, so a future value-taking flag on any subcommand can't misread its value as a name.
Buffer's 'base64url' encoding produces unpadded base64url (RFC 7636) directly, so the hand-rolled +/_/= replace chain was redundant.
|
No blocking correctness issues were identified in the branch. The unit tests, lint, typecheck, and type build pass; the OIDC smoke could not run in this sandbox because localhost binding is denied. |
Code review — OIDC browser login (multi-agent, high effort)Reviewed Correctness1. 2. 3. 4. 5. 6. 7. 8. Altitude / cleanup9. Refresh-once-retry policy and its messaging are duplicated across 10. 🤖 multi-agent review via /code-review |
A request whose target makes `new URL` throw (e.g. `GET //`) raised an uncaught exception inside the loopback request listener, crashing the whole `hyp remote login` process. Any local process or port scanner hitting the OS-assigned loopback port during the login window could trigger it. Wrap the parse in try/catch and answer 400 without settling the flow, so the real callback can still arrive.
`echo $TOKEN | hyp remote login foo --no-browser` routed to the browser flow and never read stdin, so the supplied token was silently dropped. --no-browser is a browser-mode modifier (print the URL) yet a piped token signals static intent. Peek stdin in that case: a non-empty piped value is stored statically (with a note that --no-browser was ignored), while an empty pipe still falls through to the browser flow, preserving the documented non-TTY --no-browser behavior. Extracts persistStaticToken so both static paths share one writer.
On a headless box the opener silently fails but still reports success, so the browser flow blocks until the loopback timeout and then printed only the bare timeout error. The old fast-fail TTY guard that named --token-file is gone now that browser login is the default. On any non-callback failure (timeout, transport), append guidance pointing at --token-file, piped stdin, and --no-browser, so the user is never left stuck without a next step.
refreshSession dropped any refresh_token in the token response and resolveAccessJwt re-stored the old refreshToken, so a server that rotates refresh tokens on use (one-time-use) would 401 on the next silent refresh and force a full re-login every session after the first. Surface a rotated refresh_token through RefreshedAccess and keep the stored one only when the server omits it (stable-token servers are unaffected).
…malize readCredentials drops any record it can't normalize; writeToken/writeSession/ removeToken read through it, so an unrelated login or remove silently deleted a hand-edited or newer-version sibling record from the 0600 store. Route the write path through a raw read that keeps unrecognized records intact. removeToken can now also drop a record that doesn't normalize, instead of reporting it absent.
…e check The fail-fast probe called resolveAccessJwt, so a near-expiry OIDC JWT triggered a network refresh at startup that the first handleMessage then repeated (two refreshes + two writes before any message forwarded), and a transient refresh blip aborted a proxy that would otherwise have started. Use the non-refreshing resolveToken for the presence check; the per-message resolveAccessJwt still does the session-aware refresh.
On a live 401, when the forced refresh returned ok:false (e.g. the record vanished, or no identity endpoint resolved), the proxy skipped the retry block and let the stale 401 fall through to a generic 'remote returned HTTP 401', discarding the refresh reason. The one-shot verb path already surfaces it. Return refreshed.error from the retry block so the actual reason rides back.
The re-login message and the 'refreshable' predicate were duplicated verbatim across proxy.js and remote_verb.js, free to drift. Extract sessionExpiredMessage (beside InvalidGrantError) and isRefreshable (beside the resolver that owns the kind+source distinction), and have both attach paths call them. The retry orchestration stays per-path since the proxy and the one-shot verb return different shapes.
The stdio proxy resolves a token per forwarded message, so resolveAccessJwt re-read and re-parsed the 0600 file on every message even when the cached JWT was nowhere near expiry. Add a single-entry parse cache keyed by path + mtime + size, busted on every write through this module, so a fresh-JWT message skips the read and parse while our own writes and external edits are still picked up.
Review fixes pushed (9 commits, each with tests)Addressed the review findings as standalone commits on this branch:
Finding 8 (403 treated as refreshable): not changed. On a closer look this is intentional - Full suite green (1563 pass), |
Code review: OIDC browser login (LLP 0046-0048)High-effort review of the branch diff against 1.
|
RFC 6749 §5.2 returns HTTP 400 for invalid_grant, but the token client only raised InvalidGrantError on a 401. Against a spec-compliant server a revoked or expired refresh row would surface as a generic error instead of the re-login guidance the attach paths are built to show. Key the typed rejection on the body's error code and accept either status.
The MCP client and the identity client both wrapped res.text() to return '' on an unreadable body, byte-for-byte the same. Extract it to src/core/http_text.js so the two cannot drift.
The proxy fails fast with a clear message when globalThis.fetch is absent; the verb path did not, so a fetchless runtime threw the identity client's lower-level "no fetch" deep inside the attach path with no guidance. Add the same early guard, since both the refresh and the tool call need fetch.
The browser opens /callback over a keep-alive connection. server.close() waits for in-flight sockets to drain, so without Connection: close the idle keep-alive socket held the event loop until Node's keepAliveTimeout (~5s) and hyp remote login hung that long at exit. Set Connection: close on the response so close() finishes promptly while the user still sees the page.
openBrowser's boolean is best-effort: a launcher that exists but fails (no display on a headless box) still returns true. "Opened your browser" then overstated what happened. Say "Opening" instead; the URL is still printed right below as the real fallback.
forceExpiry rewrote the credential file with a raw fs.writeFile at the same byte length. A same-size rewrite landing within one mtime tick is hidden behind the credential module's parse cache, so the next resolve could read the pre-expiry record, skip the refresh, and flake the one-refresh assertion. Write through writeSession, which invalidates that cache.
Code review — OIDC client login (high effort, recall-biased)Reviewed Correctness1. Verb attach path advises re-login for an env/static 401 that re-login can't fix — 2. Cross-target lost update can revert a rotated one-time-use refresh token — 3. A 401 with an empty or non-JSON body loses the 4. Loopback 404/400 responses omit 5. The live-401 retry reads the entry through the parse cache without busting it first — Cleanup6. 7. Duplicated user-facing string — 8. 9. The no-fetch guard is repeated across three call sites with three messages — Generated by |
A browser favicon or probe hitting the loopback port on a non-/callback path was answered without Connection: close, so the idle keep-alive socket held the event loop until Node's keepAliveTimeout (~5s) and hyp remote login hung that long at exit. Match respond()'s keep-alive close on the 404 and 400 paths.
…sh resolver - Wrap writeToken/writeSession/removeToken in a cross-process O_EXCL lock that re-reads the freshest file inside the lock, so two logins for different targets can no longer clobber each other's just-rotated one-time-use refresh token (which forced a needless re-login). A stale lock is stolen by age. - Drop the parse cache before the forced-refresh read so the live-401 retry sees a concurrent rotation, symmetric with the invalid_grant re-read. - Read the per-target env override through one helper in both resolvers. - Add describeAuthRejection, the shared "why is this credential dead" wording for a 401 surviving the refresh + retry, and surface attachWithRefresh's final authFailed so callers can word their guidance accordingly.
…b and proxy The verb path advised "re-run hyp remote login" for any surviving 401, including an env-override token that re-login can never fix, and never gave the session-expired guidance the proxy gives when a freshly-refreshed JWT is still rejected (clock skew, or a revocation independent of the refresh row). Route both attach paths through describeAuthRejection so an expired oidc session, an env override, and a static file token each get the right message and exit code. Share the no-fetch wording through one constant so the three entrypoints cannot drift.
…xpired A revoked refresh row answered with an empty or non-JSON 401 body never became InvalidGrantError, so the user got a generic error instead of re-login guidance. The only credential the token endpoint authenticates is the refresh token, so classify any 401 there as invalid_grant (alongside an explicit invalid_grant at any status), and only fail on a non-JSON body when the response was otherwise ok.
Drop the stdin peek that tried to reconcile --no-browser with a piped token. --no-browser now unconditionally means "browser flow, print the URL"; a piped token without a browser-mode flag still takes the static path, so a token is only ignored when --no-browser is given explicitly. Removes a fragile branch and a duplicated note.
Code review — OIDC client login (LLP 0046-0048)High-effort review (8 finder angles + independent verification). The OIDC core is solid: PKCE (S256), the CSRF Correctness1. 2. Loopback misclassifies provider errors as a CSRF state mismatch ( 3. New lock-timeout throw escapes the static-login and remove paths ( 4. A transient empty token-endpoint body becomes a hard "missing field" error ( 5. 6. Static-token parse cache can serve stale credentials (PLAUSIBLE) ( Cleanup / conventions7. Proxy re-implements the MCP request header set ( 8. Semicolons in new test files violate project style ( Verified findings only; |
…mmit resolveAccessJwt read the stored record and POSTed the refresh grant outside the write lock, taking the lock only for the final writeSession. Two `hyp` processes sharing one HYP_HOME (a verb call beside a proxy, two MCP clients) could therefore both read the same stale oidc record and both spend the same one-time-use refresh row, and a concurrent `hyp remote login` writing a fresh session could be clobbered by a proxy's later write carrying the superseded token. Refresh now runs through an optimistic compare-and-swap: the network call stays outside the lock (a 30s identity call must not wedge the 5s write lock), but the commit re-reads under the lock and persists only when the stored refresh token still matches the one we refreshed from. A mismatch means a sibling rotated first, so we adopt the stored session when usable or loop and refresh with the rotated token instead of overwriting it. The invalid_grant re-read now happens under the lock too, so a loser that refreshed before the winner persisted reliably sees the winner's write rather than forcing a spurious re-login.
The callback handler ran the CSRF state comparison before the error check, so an error redirect that omits `state` (RFC 6749 only requires echoing it when present, and some providers drop it on errors like access_denied) fell into the state branch. The user was told their login hit a "mismatched state" and got the headless static-token hint, instead of the real "login failed: access_denied". Surface an `error` callback first. That branch never reads the authorization code, so the CSRF guard (which exists only to stop an attacker's code being adopted) does not apply, and the error value is already sanitized before it reaches the message, log, and terminal. The state guard still runs ahead of the code path.
…hrow writeToken and removeToken now run inside withCredentialsLock, which throws "timed out acquiring the remote credentials lock" under contention. Their call sites in the static-login (persistStaticToken) and remove (runRemoteRemove) paths were unguarded, so a contended write surfaced a raw uncaught error instead of the friendly `hyp remote login:` / `hyp remote remove:` message that held before the lock existed. The remove path also already mutated config before the throw, leaving a partial removal with no explanation. Wrap both calls. Static login keeps its `hyp remote login:` contract and exit 1; remove reports the lock failure and, since the config edit already landed, tells the user the stored token could not be removed.
…ing-field
safeText returns '' for both an empty body and a mid-body read failure
(a reset connection). On a successful token response that empty string
parsed to {} with parseFailed=false, so it slipped past the non-JSON
guard and reached the field extractors, which threw "identity response
missing 'access_jwt'" - a permanent-looking contract error for what was a
transient network blip during a silent refresh.
Reject an empty body on a 2xx with an explicit "empty response - try
again" error before the field extraction, so the failure reads as
transient.
normalizeRecord accepted a record whose `token` was the empty string as a present static record, so `hyp remote list` reported it as `stored` while resolveToken and resolveAccessJwt both rejected an empty token with the "run hyp remote login" guidance - contradictory signals for the same target. An empty token is no credential, so normalize it to absent; list and the resolvers now agree the target is logged out.
The Streamable-HTTP request header map (content-type, the JSON+SSE accept, bearer, mcp-session-id) was built byte-identically in two places: the in-process client's rpc() and the stdio proxy's forward(). A protocol header change in one would silently miss the other, drifting `hyp <verb> --remote` from `hyp mcp`. Extract mcpRequestHeaders() in client.js (alongside the already-shared isAuthStatus and parseRpcResponse) and call it from both.
Code review — OIDC client login (PR #204)Multi-agent review (8 finder angles + adversarial verify) over the 1. (correctness) Concurrent
|
The OIDC refresh compare-and-swap re-read the stored record under the lock but fell back to the in-flight `from` record when the row was gone (`base = latest || from`), so a concurrent `hyp remote remove` landing while a refresh was in flight got its deletion clobbered: the commit wrote the stale session back. Treat a missing record under the lock as a removal and decline to commit, returning the normal no-token error.
Code review: OIDC browser login (LLP 0046-0048)High-effort multi-agent review of Correctness / UX
Cleanup
Minor: every credential write does two Review method: 8 finder angles (3 correctness, reuse/simplify/efficiency, altitude, conventions) then per-finding verification. Conventions (no-semicolons, no em dashes, @import paths) came back clean. |
…e 401 A rejected authorization code (expired, replayed, or mis-scoped client) went through the same 401 -> InvalidGrantError mapping as a refresh, so a first-time login reported 'refresh token was rejected' for a credential that never had a refresh token. Map an authorization_code 401 to a code-specific message instead.
…e fails In runBrowserLogin the single-use code exchange and writeSession shared one try/catch, so a write failure (most likely a lock timeout under a concurrent hyp process) printed the generic login-failed message plus the headless 'no browser' hint, even though the sign-in succeeded. Split the two phases: a post-exchange write failure now reports a store failure and points at the lock, without implying the browser login failed.
…dance describeAuthRejection's static-token branch told the user to 're-run hyp remote login' with no target name, while the env branch, sessionExpiredMessage, and noTokenError all include it. Thread the target through so the re-login advice is consistent across paths.
Code review: correctness findingsReviewed the OIDC client-login diff for real bugs (not style). The PKCE/state/CSRF/permissions/expiry core is solid. Four issues worth a look, roughly highest-confidence first. 1. Refresh race: a concurrent loser reports a false "session expired" (
|
The optimistic refresh compare-and-swap (refresh outside the lock, commit if the stored token still matched) had a race: a loser that re-read on invalid_grant before the winner committed saw an unchanged token and wrongly forced a re-login. The lock's stale-steal was also unsafe - two contenders could both steal an aged lock and clobber each other, and a slow-but-live holder could be stolen by the 30s age guess. Refresh is now single-flight: the whole read-decide-refresh-commit runs inside one lock hold, so only one process ever calls the token endpoint for a store at a time. A sibling never sees invalid_grant from contention, so invalid_grant under the lock is an unambiguous revocation; the retry loop and the "did the token change" re-read are gone. The lock records the holder's host+pid and is a real mutex: a contender steals a held lock only when that process is confirmed dead, and steals atomically (single-winner rename) so two stealers can't both adopt it; a holder only removes a lock still its own. Age remains a backstop for a cross-host or unparseable lock. Lock wait/stale thresholds raised to cover holding the lock across the bounded token call. Updates LLP 0046 D5 to specify the mechanism, not the racy policy.
…e login The receiver settled (and closed, single-shot) on the first /callback hit carrying error=, a mismatched state, or no code - and the error= branch ran before the state check. The ephemeral loopback port is reachable by any local process and by any page the user is browsing (a no-cors GET still reaches us), so a single `GET /callback?error=x` or `?state=wrong` before the genuine redirect arrived would fail-and-close the listener, leaving the real browser callback with connection-refused: a trivial login denial-of-service. Apply the CSRF state check first and ignore any non-matching-state request (neutral page, never settle), so only a callback bearing our exact state can abort or complete the login. The identity server echoes state on both success and error (LLP 0047), so genuine provider errors still match and surface. Aligns LLP 0047, which already documents error redirects as carrying state.
runRemoteLogin matched these flags with argv.indexOf('--org'), which only
finds the bare ` --org value` form. `hyp remote login prod --org=acme` left
org undefined and let positionals skip --org=acme as a flag, so the browser
flow ran with no org and a multi-org account later failed with
org_selection_required. --token-file=path likewise fell through to the
browser flow instead of reading the file. The rest of the CLI already takes
the equals form (core_commands' --token-file=), so this was an inconsistency
users hit.
Parse both forms via a shared valueFlag helper. The space form still rejects
a following flag-looking token as "expects a value"; the equals form takes
its value verbatim and rejects only an empty one.
Code review: real-bug passReviewed the branch diff (focus on correctness, not style). Most of the PR is solid; the live issues cluster in the new cross-process credential lock ( 1. Age backstop steals a live same-host lock holder, reintroducing the lost-refresh-token race
const abandoned = lockHolderIsDead(ownerText) || Date.now() - st.mtimeMs > LOCK_STALE_MSThe 2.
|
… mutex
The cross-process credential lock (LLP 0046 D5) kept its single-flight
refresh but shipped a crash-recovery mechanism with six defects: a same-host
liveness probe that could not see a suspended-but-alive holder (so the age
backstop stole live locks), a 45s wait shorter than the 90s steal age (so
waiters never reached the steal), an empty-lock wedge on a failed owner-tag
write, and a rename-aside-and-restore steal whose restore gap allowed a double
hold.
Replace all of it with one rule set (LLP 0049 D1): grant only by O_EXCL,
break a lock past one stale age with a plain fs.rm, and release only when a
per-acquisition nonce still matches. Breaking never grants the lock, so a
four-line break is safe where the old steal was not. One LOCK_STALE_MS (60s,
2x the bounded token call) replaces two constants in tension; the {host,pid}
tag, process.kill probe, stealLockIfAbandoned, and LOCK_TIMEOUT_MS are gone,
along with the node:os import.
The single-flight refresh under the lock is unchanged and still necessary for
one-time-use refresh-token rotation. Retargets the one steal test from a
dead-pid probe to an age-stale break.
Code review — correctness pass (multi-agent, high effort)Reviewed the OIDC client login diff ( 1. A genuine
|
1 participant
Implements chunk 2 of the multi-tenant OAuth login program: teaching
hypto run a browser OIDC flow against hypaware-server, store the refresh token in the existing0600credential store, and refresh silently on the query path. Follows the design in LLP 0046 (decision), 0047 (design), and 0048 (plan). Provider-agnostic; works against any hypaware-server with login configured.This branch assembles six independently-reviewed PRs (#197, #198, #203, #200, #201, #202); each carries an independent code review and its review-driven fixes.
What lands
New, dependency-free modules under
src/core/remote/:pkce.js- the client's downstream PKCE leg, S256 over stdlibcrypto(D3).loopback.js- ephemeral single-shot127.0.0.1redirect receiver, RFC 8252 (D2).open_browser.js- platform opener with the print-URL fallback (D8).identity_client.js-exchangeCode/refreshSessionover<origin>/v1/identity/token; typedinvalid_grant.oidc_login.js-loginWithBrowser()orchestrating PKCE + state + loopback + exchange.Extended:
credentials.js- records gain akind: 'static' | 'oidc'discriminator (legacytoken-only reads asstatic, no rewrite);writeSession;resolveAccessJwtwith silent refresh + 60s skew;deriveIdentityBase(D4/D5/D6).remote_commands.js-hyp remote logingains browser mode (default when no static token);--org/--browser/--no-browser; static--token-file/stdin unchanged; callback errors explained (D1/D7/D8).mcp/remote_verb.js+mcp/client.js- attach path usesresolveAccessJwt; a live 401/403 on an oidc session refreshes once and retries;invalid_grantsurfaces re-login guidance (D5).Docs: LLP 0033 updated to reference the
kindrecord and session-aware attach; LLP 0046/0047/0048 promoted Draft → Accepted.Out of scope (tracked elsewhere)
hyperparam.app OIDC provider, login-minted gateway enrollment, DNS-TXT domain claiming (server chunks 3-5); OS keychain; path-prefixed identity hosting (the
identityUrloverride).Verification
npm test: 1539 pass, 1 pre-existing skip.npm run smoke -- remote_oidc_login: green (stub identity+MCP server, scripted loopback, login → attach → silent refresh → revoked-refresh re-login).npm run typecheck,npm run lint: clean. All@refanchors resolve.🤖 Generated with Claude Code