feat(apis-explorer): session-scoped credentials and a real log-in flow - #1641
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the API Explorer's authorization flow, migrating credential storage from localStorage to sessionStorage and introducing a secure "Log in" method to mint short-lived Bearer tokens directly against Harper instances. It also refactors proxy URL detection to prevent credentials from reaching the central manager. The review feedback highlights a critical security improvement to make the proxy URL check case-insensitive to prevent bypasses, along with corresponding test updates, and suggests using a more idiomatic Object.keys check instead of a for...in loop when validating security requirements.
|
Cross-origin verified: a locally-run test confirms a minted operation token is accepted on a cross-site request (the instance's CORS allows the CI is green and all bot review threads are resolved. Reviewers: please look hardest at the credential boundary in 🤖 Generated with Claude Code |
Move the explorer's per-entity Authorize state from localStorage to
sessionStorage (cleared on tab close) and scrub the legacy plaintext
localStorage secrets on first load. Add a log-in flow that mints a Bearer
operation token — one click for the current Studio session, or a
username/password fallback POSTed directly to the instance's own operations URL
(never through the Fabric Connect proxy; enforced by isDirectOperationsUrl in the
mint helper, which also fails on redirect so the POST can't be replayed past the
check) — so authenticated "Try it out" requests work across sites where the
session cookie isn't sent.
Restructure the Authorize panel into Documentation + Try it out tabs offering
Log in / Basic / Bearer / Cookie (default Log in), modeled with a UI method
distinct from the wire ApiAuth so Login is a representable default. In-flight
mints are invalidated on any explicit auth change or unmount, and the log-in form
drops the typed password once a token is minted. Auth-required operations
deep-link to the Try-it-out log-in view, and requiresAuth now honors OpenAPI
optional-auth (security: [{}]) and explicit [] overrides.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
isDirectOperationsUrl now matches the Fabric Connect proxy path segments (/HDBInstance/, /Cluster/) case-insensitively, so a differently-cased path can't slip typed credentials or a Bearer token past the direct-URL gate. Addresses a review finding; adds lowercase coverage in the util and mint-helper tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
a3aa3d2 to
9f34635
Compare
|
Rebased onto Verified after rebase: 🤖 Generated with Claude Code |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
I think this all seems like a good approach. There some suggestions for further hardening, but I think this generally correct.
🤖 Reviewed with Codex
Addresses kriszyp's review of the Authorize story: - Centralize per-entity credential cleanup: setUserForIdAndKey now clears the explorer's stored settings on sign-out, so the ClusterHome/ClusterCard disconnect paths (setUserForEntity(entity, null)) can't leave one user's Basic password or Bearer token for the next user on the same entity. - Add a per-entity auth epoch (bumped on sign-out, mirrored to localStorage): an in-flight token mint that resolves after a sign-out is discarded rather than written back, and another tab signing the entity out clears this tab's sessionStorage credential and auth state (restores cross-tab logout propagation lost when the secret moved to per-tab sessionStorage). - Clear credentials when the selected server changes, so a token minted for one origin is never sent to another declared server. - Sync the Basic/Bearer forms when the credential is cleared, and clear the login password once a mint from it succeeds (covering re-auth while already authorized). - Run the legacy-secret scrub at app bootstrap (not only on explorer open) and make it re-runnable so a pre-upgrade tab that rewrites the key is scrubbed again. - Relabel the "Authorized" indicators to "Credential set" — a credential is configured, not verified against the operation's specific security scheme. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up hardening after the first review response surfaced further gaps: - Cross-tab / unmounted-tab clearing: an always-on app-level listener (installExplorerCrossTabCleanup, wired at bootstrap) clears a tab's stored explorer credential when another tab signs the entity out, even while this tab's explorer is unmounted — so a tab that navigated away can't later restore a revoked token. The component listener still resets live state when mounted. - Global logout: signOutAllLocally now broadcasts a '*' invalidation so every tab's explorer clears, covering entities not in this tab's authenticated set. - Server scoping: the credential is sent only when the active server shares the trusted instance origin (originOf(baseURL)), so an implicit active-server change — not just an explicit picker change — can't send a token to another origin. - Proxy-path gate: isDirectOperationsUrl matches the segment on a word boundary (no trailing slash required), rejecting a bare `/HDBInstance` or query-string proxy URL that previously slipped through. - Direct credential mint is bounded by a 30s AbortController timeout. - Legacy scrub no longer runs on every settings read (bootstrap + storage-event driven), and forgetAllEntitySettings backs the global clear. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ath gate Final hardening pass on the auth lifecycle: - Stamp each credential with the server it was authorized against (authServer) and send it only when the active server still matches. Replaces the previous origin comparison, which parsed URLs on every render and silently stripped valid credentials when the base URL was relative (e.g. behind a reverse proxy). Plain string compare, no URL parsing on the render path. - isDirectOperationsUrl now tests the parsed pathname, so a proxy path is rejected bare, cased, concatenated (/HDBInstance123), or with a query string, while a host merely named like the segment stays direct. Fails closed on a relative or unparseable URL. - A global logout now advances every entity's epoch (getExplorerAuthEpoch folds in the '*' slot), so a same-tab full logout invalidates an in-flight mint even though localStorage fires no storage event in the originating tab. - runMint clears the pending status when a sign-out invalidates it, instead of leaving the log-in button stuck on "Authorizing…". - Clearing auth or switching to a logged-out method drops the authServer stamp. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@kriszyp thanks — this was a genuinely valuable review; the first finding was a real cross-user credential leak. All 8 inline findings are fixed and their threads resolved (individual replies on each). Pushed as
Also hardened while in here: the proxy-path gate now tests the parsed pathname (rejects bare/cased/concatenated Full unit suite green (2655) with regression tests for each fix above; 🤖 Generated with Claude Code |
…ed server
- A minted token is stamped with Studio's computed URL for the entity (the
instance the mint client actually talks to), not the spec picker's current
selection. Selecting a foreign declared server now withholds the token instead
of marking it authorized for that origin. Regression test added.
- signOutFromPotentiallyAuthenticatedInstances also clears explorer settings and
bumps the auth epoch; it flags sign-out directly rather than via signOutLocally,
so it was the one remaining sign-out path that skipped the cleanup.
- A mirrored cross-tab invalidation now advances this tab's in-memory epoch, so an
in-flight mint here is invalidated by another tab's sign-out through the epoch
check as well as the attempt guard.
- Abort/network failures from the direct mint surface a stable message ("did not
respond within 30 seconds" / "could not reach the instance") instead of the
browser's opaque abort text.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Pushed
One thing to be upfront about: this last commit has no outside-model review coverage. The run I kicked off for it produced nothing — Gemini hit an account quota (resets in ~5h) and Codex hung and was killed with no budget left to retry — so the Given the last few rounds of findings were in my own patches on a security-sensitive surface, I've stopped self-iterating here — @kriszyp @kylebernhardy @DavidCockerill, the credential-to-server stamping model in 🤖 Generated with Claude Code |
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Nice work, a couple of follow-up suggestions, but I don't consider them blockers.
🤖 Reviewed with Codex
An event-only cross-tab signal was not enough: sessionStorage survives a reload, so a tab that restarted (or had not started) when another tab signed the entity out would never observe the invalidation and could keep using a revoked credential. - The sign-out generation now lives in localStorage (durable, shared) instead of an in-memory map, and every stored credential records the generation it was created under. A credential whose stamp no longer matches is not sent, so the check works by comparison at read time rather than depending on a live event. - pruneStaleEntitySettings strips such credentials (keeping non-secret server and method selections) and runs immediately when the app-level cleanup is installed at bootstrap, covering an invalidation that happened before any listener existed. - The cross-tab listener now re-reads state rather than assuming the event names this entity, so another entity's sign-out no longer clears this one. - The mint's abort timer now covers body consumption: an instance that sends headers and then stalls the body previously left the log-in pending forever. An abort during body read propagates instead of reading as "no token returned". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Builds out the APIs Explorer "Authorize" story so authorization is credible and safe in deployed
environments, where the default cookie flow silently fails.
What changed
localStoragetosessionStorage(cleared on tab close), with a one-time scrub of the legacy plaintextlocalStoragesecrets on first load.authStoresign-out now delegates to a sharedforgetEntitySettings.different site than Studio. The Authorize panel now mints a Bearer operation token — one click
for your current Studio session, or a username/password fallback — sent as an explicit
Authorizationheader, which crosses origins where cookies don't.operations): docs explain Log in / Basic / Bearer / Cookie; Try it out is where you authenticate,
which flips the lock and unlocks authenticated requests. Default method is Log in.
requiresAuthnowhonors OpenAPI optional-auth (
security: [{}]) and explicit[]overrides.For the human reviewer
Read
src/features/instance/apis/APIDocs.tsxandsrc/integrations/api/instance/auth/createInstanceAuthenticationTokens.tsfirst — the credentialboundary is the thing to scrutinize.
Decisions a reviewer might reasonably question:
create_authentication_tokensto the operations client's ownbaseURL— the address Studioalready uses to talk to this instance — and only when it passes
isDirectOperationsUrl(rejectsthe Fabric Connect
/HDBInstance/…//Cluster/…proxy paths). The check is enforced inside themint helper, not just at the call site, so the boundary can't regress. When the only reachable URL
is the proxy, the password fallback is withheld (fail-closed) and one-click session mint still
works. Session mint intentionally uses the existing (possibly proxied) client because it authorizes
as you and discloses no one else's credentials.
short-lived token in sessionStorage, never the password. Basic auth still stores username+password
(session-scoped); the docs say so per method.
sessionStorageis not claimed as an XSS boundary —it's a persistence/lifetime choice.
buildFetchSnippetembeds the realAuthorizationheader in the copyable sample (as the prior Swagger UI did). Kept deliberately: it'sthe user's own credential in their own tab, and redacting would break the copy-paste-run purpose.
Flagging in case exfil-via-clipboard is in scope for this surface.
signOutOfInstance→signOutLocally→forgetEntitySettingsclears per-entity; full logout (clearAuthStateLocally,logoutOnSuccess)calls
clearSessionStorage(). So a review note that "normal disconnect paths bypass cleanup" didnot hold on trace.
redirect: 'error') so a 3xx can't replayit past the direct-URL check, and the log-in form drops the typed password from memory once a token
is minted.
Verification
tsc -b,oxlint,dprint checkclean;vitest run— all2630 tests pass (added coverage for storage/scrub,
isDirectOperationsUrl,requiresAuth, themint helper incl. non-direct refusal + server-error surfacing, and the explorer's login flows,
method-switch mint race, and deep-link). The suite's process exit is non-zero only from a
pre-existing, unrelated undici-WebSocket unhandled error in a Chat test (reproduces without this
diff).
cross-origin request (the instance's CORS allows the
Authorizationheader on preflight), which isthe behavior the cookie path couldn't provide and the core reason for this change.
Process
Planning review (step 6) cleared
chosen-approach-soundafter widening the option set. Step-10cross-model review ran five rounds (codex + gemini + cursor-grok, independent). The Harper-domain
adjudication leg fails on this host (zero-byte log), so outside findings were hand-adjudicated.
Round 1 (BLOCK) → round 2 (CHANGES; blocker + majors fixed) → round 3 found no blockers or majors →
round 4 (delta) covers a gemini bot finding — the credential direct-URL gate was case-sensitive, so
isDirectOperationsUrlnow rejects proxy paths case-insensitively → round 5 is the Final-artifactcheck after rebasing onto
stageto integrate its API-explorer sidebar-resize feature, and foundno blockers or majors in the merge. The
Human-Review-Need: 4floor reflects the high-riskauth/storage surface plus the failed adjudication leg (degraded coverage), not an open blocker.
Review response (kriszyp): all 8 inline findings are fixed and the threads resolved — per-entity
cleanup centralized in
setUserForIdAndKey(closing a cross-user credential leak via theClusterHome/ClusterCard disconnect paths), an auth-epoch that invalidates in-flight mints and
propagates sign-out across tabs (including an always-on listener for unmounted tabs), credentials
stamped to the server they were authorized against, form drafts resynced on Clear, the legacy scrub
moved to bootstrap, and the "Authorized" indicator relabelled "Credential set".
Follow-up rounds fixed three further majors the reviews surfaced in my own hardening: minted tokens
are now stamped with Studio's computed URL for the entity (not the spec picker's selection, which had
recreated a narrower version of the leak the stamping exists to prevent);
signOutFromPotentiallyAuthenticatedInstances— the one sign-out path that flags sign-out directlyrather than via
signOutLocally— also clears explorer state; and a mirrored cross-tab invalidationadvances this tab's in-memory epoch so an in-flight mint is caught by the epoch check too. Each has a
regression test.
A second review round from kriszyp found two more real gaps, both fixed in
a439aad7: an event-onlycross-tab signal couldn't survive a reload (sessionStorage outlives one, so a restarted tab never saw
the invalidation and kept a revoked credential) — the sign-out generation is now durable and every
credential is stamped with the generation it was created under, reconciled at bootstrap; and the mint's
abort timer didn't cover body consumption, so a stalled body left the log-in pending forever.
Coverage gap, stated plainly: the last two commits (
e832fca6,a439aad7) have no outside-modelreview. The run for the first produced nothing — Gemini hit an account quota and Codex hung and was
killed with no budget to retry — and I did not re-run for the second. That is an absent review, not a
clean one. What they do have: the full local gate (2661 unit tests,
tsc,oxlint,dprint) and aregression test per fix — including one that caught a bug in my own first attempt at the abort fix
(a blanket
.catchwas swallowing the abort). Everything up to254c81eewas reviewed bycodex+gemini. Reviewers may reasonably want to look hardest at those two commits.
Known follow-up (not in this PR): the credential indicator reports that a credential is configured,
not that it satisfies the operation's specific security requirement (AND/OR alternatives against
referenced schemes). Happy to fold that in if wanted.
Complexity: moderate — new auth UI surface plus a narrow, well-bounded reach into the operations
client for token minting.
Review-Coverage: authored=unknown; ran=none; rounds=1 @ a439aad
Human-Review-Need: 4 @ a439aad