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 51 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.
| // Cache hit: same file, unchanged since the last parse. Return a clone so a | ||
| // mutating caller (the write path) can't corrupt the cached value. | ||
| if (rawCache && rawCache.path === p && rawCache.mtimeMs === stat.mtimeMs && rawCache.size === stat.size) { | ||
| return structuredClone(rawCache.value) |
There was a problem hiding this comment.
Minor efficiency: the cache hit still deep-clones. This cache exists so the per-message proxy resolve skips re-read + re-parse, but every hit still structuredClones the whole map. Only the write path mutates its result; the read paths (resolveAccessJwt/resolveToken) treat it as read-only. Returning the cached value directly for reads (and cloning only in the write path, which already re-reads) would make a fresh-JWT message allocation-free. Low impact given messages are serial, but it partly defeats the cache's purpose.
The 0600 credential store is shared by concurrent hyp processes (a second MCP client, or a verb call beside a running proxy). With one-time-use refresh tokens, the loser of a refresh race got invalid_grant only because the winner had already rotated the row, and was told to re-run 'hyp remote login' despite holding a valid session. resolveAccessJwt now re-reads the store once on invalid_grant: if the stored refresh token changed, it adopts the winner's freshly-minted session (its JWT if still fresh, else a refresh with the rotated token) instead of surfacing re-login guidance. An unchanged token is a real revocation and still surfaces the guidance. Documented in LLP 0046 D5.
The no-browser-with-piped-stdin branch is only reached when useStatic is false, and useStatic already forces the static path whenever --token-file is present, so tokenFile is guaranteed absent here. Drop the dead condition.
Export the 401/403 predicate from client.js and have the stdio proxy's per-message forward call it, so "which statuses mean auth" has one home rather than a copy in the proxy.
Code review — multi-agent (high effort, recall-biased)8 finder angles → verify pass. 10 findings survived (2 candidates refuted: the Correctness1. Loopback: a stray callback with a mismatched/absent 2. Proxy startup probe (presence-only) lets a stranded/revoked OIDC session "start" — 3. Proxy emits a bare "remote returned HTTP 401" with no re-login guidance after a post-refresh rejection — 4. Credentials parse cache can serve stale data on a same-size out-of-band rewrite — 5. 6. Non-TTY stdin routes to static login and never opens the browser — 7. Loopback: a present-but-empty Cleanup / altitude8. 9. 10. Generated by a multi-agent review (8 finders → verifiers). Items 4–7 are PLAUSIBLE (realistic but state-dependent); 1–3 are CONFIRMED. |
- proxy: a 401/403 surviving the refresh+retry now returns the re-login guidance the verb path gives, not a bare "remote returned HTTP 401". - identity: reject a non-date expires_at at parse time so a bad server value can't turn every forwarded message into a fresh refresh that re-stores the same value and never self-corrects. - credentials: drop the parse cache before the invalid_grant re-read so a same-size concurrent token rotation can't hide behind the cache; and stop deep-cloning the whole store on every per-message read (only the in-place write path clones now). - share the 401/403 status check (isAuthStatus) and the invalid_grant -> session-expired mapping (describeRefreshError) instead of copy-pasting them across the proxy and verb attach paths. - loopback: treat a present-but-empty error= callback as an error.
Code review: OIDC client login (high-effort, recall-biased)Reviewed Correctness1. Headless 2. 401/403 on an env/static credential gives unactionable "re-run hyp remote login" - 3. Refreshable OIDC session refuses to start when the cached 4. Token endpoint POST has no timeout, on the proxy per-message hot path - 5. A single verb command can rotate the refresh token twice - Robustness / UX6. Loopback receiver lingers ~5s at exit - 7. "Opened your browser to sign in" prints even when nothing opened - 8. Verb path lacks the Lower severity9. Parse cache can serve a stale credential on a same-size, same-mtime-tick external rewrite - 10. Generated with |
normalizeRecord dropped an explicit oidc record that had a valid refresh token but a missing or empty accessJwt (a partial write, an interrupted refresh, a hand edit), and the proxy presence-probe (resolveToken) likewise treated it as no-credential. Both forced a needless full re-login for a session that resolveAccessJwt could mint a fresh JWT for. Keep an explicit-kind oidc record whenever its refresh token is non-empty, defaulting accessJwt to ''. Inferred (no-kind) records still require the full refresh+access pair so one carrying a static token is not hijacked. resolveToken now reports an oidc record as present even when its cached JWT is empty, so the fail-fast probe does not refuse a refreshable session.
postToken's fetch had no deadline, but the stdio proxy refreshes on the per-message hot path (resolveAccessJwt -> refreshSession), so a hung or slow identity endpoint blocked a forwarded JSON-RPC message indefinitely. The loopback receiver had a timeout; the token call, the one actually on the request path, did not. Wrap the fetch and body read in a 30s AbortController, clearing the timer in finally so a fast response does not keep the event loop alive, and surface an abort as a clear "did not respond within 30s" error.
A 401/403 surviving the refresh+retry was reported as "remote session expired - re-run hyp remote login" regardless of credential kind. For a per-target env override (never read from the store) or a CI static token, re-login cannot fix it. Gate the session-expired wording on isRefreshable(resolved); otherwise point at the credential itself.
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. |
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