feat(apis-explorer): session-scoped credentials and a real log-in flow - #1641
feat(apis-explorer): session-scoped credentials and a real log-in flow#1641dawsontoth wants to merge 2 commits into
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
| this.flagKeyAsSignedOut(id); | ||
| this.updateConnectionIfChanged(id, false, null); | ||
| forgetApiExplorerSettings(id); | ||
| forgetEntitySettings(id); |
There was a problem hiding this comment.
Could we centralize every per-entity disconnect/user transition through this cleanup? Explorer credentials are keyed only by entity, but src/features/cluster/ClusterHome.tsx:84, src/features/cluster/ClusterHome.tsx:308, and src/features/clusters/components/ClusterCard.tsx:117-120 clear users with setUserForEntity(..., null) instead of calling this method. User A can therefore authorize Explorer, disconnect, let user B sign into the same entity, and B’s Explorer will reload A’s stored Basic credentials or Bearer token. Please bind settings to the authenticated identity/auth epoch or make all disconnect and sign-in transitions clear Explorer state, with an A→logout→B regression test.
| (await createInstanceAuthenticationTokens({ instanceClient: operationsParams.instanceClient })).operationToken, | ||
| [operationsParams.instanceClient], | ||
| ); | ||
| const onCredentialMint = useMemo( |
There was a problem hiding this comment.
This mint callback is permanently bound to operationsBaseURL, while ApiExplorer stores the resulting token only by entity and sends it to whichever independently selected activeServer is current. With a multi-server spec, selecting another declared server—or changing servers while minting is pending—attaches this entity’s token to that other origin and may also use the wrong credential for the target. Please bind minted auth to a validated server target and clear/refuse it when the target changes; one safe policy is to restrict one-click minted tokens to the trusted computed Studio server.
| // Server + auth selections persist per entity in sessionStorage (this browser tab only), so | ||
| // credentials never cross entities and are cleared when the tab closes. Ephemeral navigation state | ||
| // (filter, selected endpoint, which pane is open) is deliberately not persisted. | ||
| const [entitySettings, setEntitySettings] = useState(() => readEntitySettings(entityId)); |
There was a problem hiding this comment.
Moving the secret to sessionStorage also removes the previous cross-tab logout propagation: separate top-level tabs have separate sessionStorage areas, and the old storage listener is gone. If two tabs hold Basic/Bearer auth, signing out in one leaves the other tab’s credential and in-memory state usable; cookie logout does not neutralize stored Basic credentials, and operation JWTs may remain valid. Please keep secrets tab-local but broadcast entity/all-auth invalidations through BroadcastChannel or a non-secret localStorage epoch, then clear both storage and component state in every tab.
| ); | ||
| } | ||
|
|
||
| function BasicForm({ auth, onApply }: { auth: ApiAuth; onApply: (username: string, password: string) => void }) { |
There was a problem hiding this comment.
onClearAuth empties the parent credential, but these Basic/Bearer forms initialize their local state only on first mount. Clicking Clear therefore leaves the password/token visible in the mounted input, and submitting immediately restores the supposedly cleared credential. Please synchronize local form state when auth is cleared or key/remount the form on credential changes; add coverage for both Basic and Bearer Clear flows.
| const [loginStatus, setLoginStatus] = useState<'idle' | 'pending' | 'error'>('idle'); | ||
| const [loginError, setLoginError] = useState<string | null>(null); | ||
|
|
||
| const runMint = async (mint: () => Promise<string>) => { |
There was a problem hiding this comment.
The attempt guard is invalidated by UI changes and unmount only, not by external logout/session invalidation. If clearAuthStateLocally() clears sessionStorage while this request is pending, a response arriving before router-driven unmount still passes these checks and line 92 writes the old user’s token back after logout. Please include a shared auth/logout generation or authenticated identity in the mint stamp, re-check it before committing, and test logout between mint start and resolution.
| * write the key back — this runs on every explorer init, but not continuously. | ||
| */ | ||
| function scrubLegacySettings(): void { | ||
| if (legacyScrubbed) { |
There was a problem hiding this comment.
The legacy secret cleanup is both lazy and one-shot. It runs only when readMap() is called, so upgraded users who never reopen Explorer retain the old localStorage password/token indefinitely. Once it does run, legacyScrubbed prevents the “every explorer init” retry described above; a concurrently open pre-upgrade tab can repopulate the key until a full reload. Please run this migration during application bootstrap and retry removal on relevant storage events (or at least remove the one-shot guard), with coverage independent of readEntitySettings().
| * relies on an ambient session cookie that may not be sent cross-site. Branch-only, allocation-free: | ||
| * it runs on the sidebar's lock indicator during ordinary renders. | ||
| */ | ||
| export function isAuthorized(auth: ApiAuth): boolean { |
There was a problem hiding this comment.
This predicate means “an explicit credential is configured,” not that the selected operation is authorized. A Login-minted Bearer token marks a Basic-only operation Authorized; one half of an OpenAPI AND requirement also marks it Authorized; valid cookie/API-key auth is always shown as unauthorized. Please either label this state “Credential configured” or evaluate the operation’s complete security alternatives against their referenced schemes before claiming Authorized.
| const pending = login.status === 'pending'; | ||
|
|
||
| // Don't keep the typed credentials in memory once a token has been minted from them. | ||
| useEffect(() => { |
There was a problem hiding this comment.
This cleanup only runs when the boolean authorized changes. If a login token is already loaded, submitting different username/password credentials replaces it with another non-empty token, so authorized stays true and the submitted password remains in the mounted form. Please clear the credential fields on successful credential mint rather than relying on the generic authorization boolean, and add a regression test that starts already authorized before submitting alternate credentials.
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.
unmount and on any explicit auth change, and the explorer is keyed by
entityId(sign-outnavigation remounts it). A same-entity
signOutLocallywith no navigation and a still-valid JWT isthe residual gap; closing it fully would mean subscribing the presentational tree to
authStore,which I judged over-coupled for this UI. Happy to add it if you'd prefer.
session-storage choice: state is now per-tab, so signing out in one tab doesn't reach another tab's
explorer credentials (the old localStorage design propagated via a
storageevent). Cookies arestill shared, but the explorer's minted token / Basic creds are tab-local. Accepted as the point of
session-scoping; flagging so it's a conscious call.
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.
Remaining minors, left for follow-up / your call: re-authenticating while already authorized doesn't
re-clear the login form (the clear fires on the unauth→auth transition); Basic/Bearer form drafts
persist after Clear (the form is the credential editor by design); and the legacy-secret scrub runs
on first Explorer load rather than app startup (bounded — an already-open old tab can rewrite the key
until the next load).
Complexity: moderate — new auth UI surface plus a narrow, well-bounded reach into the operations
client for token minting.
Review-Coverage: authored=claude; ran=gemini,codex; blocked=domain(exit-1); declined=cursor-grok,cursor-composer; rounds=5 @ 9f34635
Human-Review-Need: 4 @ 9f34635