fix(storage): restore ICredentialStore contract on ServerCredentialStore (sec)#544
Merged
Conversation
Coverage Report
File CoverageNo changed files found. |
…es, orphan reasons)
Three classes of issue addressed:
1. Key-injection scope escape.
userId, projectId, and every credential key are now matched against a
strict SAFE_SEGMENT grammar (^[A-Za-z0-9_-]{1,128}$). Without this, a key
like "../../other-user/other-project/leaked" could be smuggled through
vaultId() and collide with another scope's vault id. Failures are surfaced
as TypeError without echoing the invalid value back.
2. Delete races and orphan-overwrite.
- delete() is refactored onto an internal deleteById() helper that
deletes metadata FIRST, then vault. A throw on metadata.delete leaves
the entry fully intact and readable; a throw on vault.deleteSecret
reinstates a pending orphan marker (with orphanReason
"vault-delete-failed") so readers see absence and operators can
reconcile, AND surfaces a wrapped error whose message distinguishes
"orphan marker persisted" from "orphan marker also failed to persist".
- deleteById skips rows whose metadata is pending: true. This closes
both the orphan-overwrite path (delete on an existing orphan marker
used to nuke the vault entry it was tracking) and the concurrent
put(new)+delete race (delete during the pending window of a new-entry
put would wipe the vault entry mid-write).
- get()/has() wrap expiry self-eviction in try/catch so a transient
KV/vault failure during eviction does not surface as a thrown read.
- deleteAll() collects per-row failures and throws AggregateError so a
single bad row no longer silently masks deletion of the rest.
3. Orphan reason discriminator.
CredentialMetadataRow gains an optional orphanReason field
("vault-write-failed" | "metadata-commit-failed" | "vault-delete-failed")
and a corresponding exported OrphanReason type. Both put() orphan
paths and the new delete() orphan path stamp the reason so operator
tooling can pick a remediation strategy without re-running the original
write. Legacy rows without the discriminator stay invisible to readers
(covered by an explicit test).
14 new tests added; existing delete-removes-both test upgraded to assert
metadata-first ordering via spy call-order; existing put orphan tests
upgraded to assert orphanReason.
Supersedes #541.
https://claude.ai/code/session_01Wws8oZpB5imjKL2e7DRXtc
…ore (sec) Follow-up to #543. Two contract regressions discovered in code review: H1: deleteAll() silently skipped pending/orphan rows because it now delegates to deleteById (added in #543) which short-circuits when pending=true. Result: no public API cleared orphan markers; a "wipe scope" call silently leaked the very rows operators need to clean up. Adds forceDeleteById that bypasses the pending-skip; deleteAll uses it. deleteById (single-key) unchanged. H2: get/has/delete now threw TypeError for invalid keys because they all routed through vaultId(). ICredentialStore documents these as returning undefined/false on missing keys; throwing breaks substitutability with InMemoryCredentialStore / EncryptedKvCredentialStore. Adds isSafeKey() predicate; readers short-circuit; put() still throws via vaultId (only path that persists a colliding id). M1: SAFE_SEGMENT widened from [A-Za-z0-9_-] to [A-Za-z0-9._:-] so natural credential names like "openai.prod" and "scope:billing" are accepted. Still rejects path separators and whitespace. 5 new tests + 1 rewritten test. Existing tests pass.
sroussey
force-pushed
the
claude/beautiful-mayer-aZeSV
branch
from
6237f96 to
887e5c3
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Restores ServerCredentialStore compliance with the ICredentialStore contract and fixes operational regressions introduced in #543, especially around invalid-key behavior and scope-wide deletion semantics (including pending/orphan rows).
Changes:
- Widened
SAFE_SEGMENTvalidation to allow.and:in credential keys while still rejecting path separators/unsafe characters. - Adjusted
get/has/deleteto short-circuit invalid keys (no throws) and made expiry eviction best-effort (non-throwing reads). - Fixed
deleteAll()to actually clear pending/orphan rows via a new pending-bypassing delete path and to aggregate per-row failures.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/storage/src/credentials/ServerCredentialStore.ts | Restores contract behavior for invalid keys, adds force-delete path for deleteAll, and enriches orphan marker diagnostics. |
| packages/storage/src/credentials/ServerCredentialStore.test.ts | Updates/extends tests to pin contract behavior, delete ordering, delete failure modes, widened key grammar, and deleteAll regression coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Copilot review feedback on #544: - deleteById's JSDoc claimed both delete() and deleteAll() share its code path, but deleteAll() deliberately routes through forceDeleteById() to bypass the pending-row short-circuit and reach sticky orphan markers. Updated to call out the asymmetry with a forward reference to forceDeleteById. - Typo: 'deleteByid' -> 'deleteById' so the @link symbol resolves.
…re delete paths `deleteById` and `forceDeleteById` had nearly-identical bodies after the pending-row check — the metadata-first ordering, orphan-marker rewrite, and wrapped-error throw were duplicated, so a fix in one could silently drift from the other. Lift the shared tail into `commitDelete(id, key, existing, op)` so both callers route through one source of truth and the only meaningful divergence (pending-row policy) stays at the call site. Also corrects the stale `vaultId` JSDoc that still listed the reader methods as callers — they were moved to `isSafeKey` in the same PR for the ICredentialStore-contract substitutability fix. https://claude.ai/code/session_011KMd9sERp2rguyekAi8a3u
sroussey
added a commit
that referenced
this pull request
Jun 4, 2026
…w IPv6/redirect tests PR #542 (Plan A initial-URL hardening) and PR #544 (Plan B ServerCredentialStore hardening) landed on main and superseded both commits in this branch. Reset the three production files to main's versions and keep only the 4 IPv6 / redirect-canonicalisation tests that main does not yet have. Files reset to match main: - packages/ai/src/provider-utils/localOnlyFetch.ts (#542) - packages/storage/src/credentials/ServerCredentialStore.ts (#544) - packages/storage/src/credentials/ServerCredentialStore.test.ts (#544) File retaining net additions (4 new tests): - packages/test/src/test/ai-provider-api/localOnlyFetch.test.ts - accepts bracketed IPv6 loopback [::1] (positive) - rejects IPv6 zone identifier [::1%25eth0] - pins redirect behaviour: Location: http://0x7f.0.0.1/ from a loopback origin is followed because WHATWG canonicalises the target to 127.0.0.1 - accepts bracketed IPv6 in a redirect Location Verified locally on the rebased branch with `vitest run` against both test files: 19/19 localOnlyFetch + 31/31 ServerCredentialStore pass.
sroussey
added a commit
that referenced
this pull request
Jun 4, 2026
…tch tests Continuation of the previous rebase commit. Resets the two ServerCredentialStore files to main's versions (PR #544 hardening landed there) and updates the localOnlyFetch test file to main's content plus the 4 new IPv6 / redirect-canonicalisation tests. After this commit the PR diff against main is exactly: - +4 tests in packages/test/src/test/ai-provider-api/localOnlyFetch.test.ts
2 tasks
sroussey
added a commit
that referenced
this pull request
Jun 4, 2026
… cases Reset `localOnlyFetch.test.ts` to main's content (which already includes the three IPv4 canonicalisation tests landed by #542) and append four net-new regression cases that main does not yet cover: - accepts a bracketed IPv6 loopback initial URL (http://[::1]:8080/) - rejects an IPv6 loopback with a zone ID (http://[::1%25eth0]/) - follows a redirect whose Location canonicalizes to a loopback IPv4 (extractRawHost(next.href) returns the canonical hostname; documented inline as the deliberate trade-off — the loopback-host invariant is preserved because the final destination IS 127.0.0.1) - follows a redirect to a bracketed IPv6 loopback This is the only conflict file between the branch and main; merging main into this branch (via the GitHub "Update branch" action that follows this commit) resolves all other files cleanly because every production file on this branch already matches main exactly after PRs #542 and #544 landed. https://claude.ai/code/session_01Wws8oZpB5imjKL2e7DRXtc
sroussey
added a commit
that referenced
this pull request
Jun 4, 2026
…543) * fix(ai): validate raw host literal in localOnlyFetch (close SSRF bypass) localOnlyFetch previously validated url.hostname after the WHATWG URL parser canonicalises it, so non-standard IPv4 spellings (0x7f.0.0.1, 2130706433, 010.0.0.1) silently slipped past the strict-literal loopback grammar in isLoopbackHostname. Switch both gates - the initial URL and every redirect hop - to validate the literal host extracted via extractRawHost, taken from the raw URL string BEFORE canonicalisation. The fallback to url.hostname is kept as defence in depth. Adds 7 unit tests covering hex/decimal/octal IPv4 spellings, bracketed IPv6 literal [::1], IPv6 zone identifier rejection, and the redirect canonicalisation pin-down behaviour (a Location: http://0x7f.0.0.1/ sent from a loopback origin is followed because next.href resolves to 127.0.0.1, which IS loopback). Supersedes #542. https://claude.ai/code/session_01Wws8oZpB5imjKL2e7DRXtc * fix(storage): harden ServerCredentialStore (key injection, delete races, orphan reasons) Three classes of issue addressed: 1. Key-injection scope escape. userId, projectId, and every credential key are now matched against a strict SAFE_SEGMENT grammar (^[A-Za-z0-9_-]{1,128}$). Without this, a key like "../../other-user/other-project/leaked" could be smuggled through vaultId() and collide with another scope's vault id. Failures are surfaced as TypeError without echoing the invalid value back. 2. Delete races and orphan-overwrite. - delete() is refactored onto an internal deleteById() helper that deletes metadata FIRST, then vault. A throw on metadata.delete leaves the entry fully intact and readable; a throw on vault.deleteSecret reinstates a pending orphan marker (with orphanReason "vault-delete-failed") so readers see absence and operators can reconcile, AND surfaces a wrapped error whose message distinguishes "orphan marker persisted" from "orphan marker also failed to persist". - deleteById skips rows whose metadata is pending: true. This closes both the orphan-overwrite path (delete on an existing orphan marker used to nuke the vault entry it was tracking) and the concurrent put(new)+delete race (delete during the pending window of a new-entry put would wipe the vault entry mid-write). - get()/has() wrap expiry self-eviction in try/catch so a transient KV/vault failure during eviction does not surface as a thrown read. - deleteAll() collects per-row failures and throws AggregateError so a single bad row no longer silently masks deletion of the rest. 3. Orphan reason discriminator. CredentialMetadataRow gains an optional orphanReason field ("vault-write-failed" | "metadata-commit-failed" | "vault-delete-failed") and a corresponding exported OrphanReason type. Both put() orphan paths and the new delete() orphan path stamp the reason so operator tooling can pick a remediation strategy without re-running the original write. Legacy rows without the discriminator stay invisible to readers (covered by an explicit test). 14 new tests added; existing delete-removes-both test upgraded to assert metadata-first ordering via spy call-order; existing put orphan tests upgraded to assert orphanReason. Supersedes #541. https://claude.ai/code/session_01Wws8oZpB5imjKL2e7DRXtc * rebase: drop obsolete fixes superseded by #542 and #544; keep only new IPv6/redirect tests PR #542 (Plan A initial-URL hardening) and PR #544 (Plan B ServerCredentialStore hardening) landed on main and superseded both commits in this branch. Reset the three production files to main's versions and keep only the 4 IPv6 / redirect-canonicalisation tests that main does not yet have. Files reset to match main: - packages/ai/src/provider-utils/localOnlyFetch.ts (#542) - packages/storage/src/credentials/ServerCredentialStore.ts (#544) - packages/storage/src/credentials/ServerCredentialStore.test.ts (#544) File retaining net additions (4 new tests): - packages/test/src/test/ai-provider-api/localOnlyFetch.test.ts - accepts bracketed IPv6 loopback [::1] (positive) - rejects IPv6 zone identifier [::1%25eth0] - pins redirect behaviour: Location: http://0x7f.0.0.1/ from a loopback origin is followed because WHATWG canonicalises the target to 127.0.0.1 - accepts bracketed IPv6 in a redirect Location Verified locally on the rebased branch with `vitest run` against both test files: 19/19 localOnlyFetch + 31/31 ServerCredentialStore pass. * rebase: reset ServerCredentialStore + add 4 IPv6/redirect localOnlyFetch tests Continuation of the previous rebase commit. Resets the two ServerCredentialStore files to main's versions (PR #544 hardening landed there) and updates the localOnlyFetch test file to main's content plus the 4 new IPv6 / redirect-canonicalisation tests. After this commit the PR diff against main is exactly: - +4 tests in packages/test/src/test/ai-provider-api/localOnlyFetch.test.ts * test(ai): rebase localOnlyFetch tests onto main + add 4 IPv6/redirect cases Reset `localOnlyFetch.test.ts` to main's content (which already includes the three IPv4 canonicalisation tests landed by #542) and append four net-new regression cases that main does not yet cover: - accepts a bracketed IPv6 loopback initial URL (http://[::1]:8080/) - rejects an IPv6 loopback with a zone ID (http://[::1%25eth0]/) - follows a redirect whose Location canonicalizes to a loopback IPv4 (extractRawHost(next.href) returns the canonical hostname; documented inline as the deliberate trade-off — the loopback-host invariant is preserved because the final destination IS 127.0.0.1) - follows a redirect to a bracketed IPv6 loopback This is the only conflict file between the branch and main; merging main into this branch (via the GitHub "Update branch" action that follows this commit) resolves all other files cleanly because every production file on this branch already matches main exactly after PRs #542 and #544 landed. https://claude.ai/code/session_01Wws8oZpB5imjKL2e7DRXtc * test(ai): reset localOnlyFetch.test.ts to main's content prior to merge This commit resets the test file to byte-for-byte match main's version so the upcoming "Update branch" merge has zero conflict. The four new IPv6 / redirect-canonicalisation tests will be added as a separate follow-up commit after the merge, on top of current main. https://claude.ai/code/session_01Wws8oZpB5imjKL2e7DRXtc * test(ai): add IPv6 + redirect-canonical regression tests for localOnlyFetch Adds four cases not covered by main's localOnlyFetch suite after #542: - accepts bracketed IPv6 loopback initial URL (http://[::1]:8080/) - rejects IPv6 with zone ID (http://[::1%25eth0]/) - follows redirect whose Location canonicalizes to 127.0.0.1 (extractRawHost(next.href) returns the canonical hostname; documented inline as the deliberate trade-off — the loopback host invariant is preserved) - follows redirect to bracketed IPv6 loopback https://claude.ai/code/session_01Wws8oZpB5imjKL2e7DRXtc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #543. Code review surfaced two contract regressions and one usability gap in
ServerCredentialStore:deleteAll()silently skipped pending/orphan rows. test(ai): add IPv6 + redirect-canonicalisation localOnlyFetch tests #543 routeddeleteAllthrough the new shareddeleteById, which short-circuits whenpending=true. The result: no public API could clear sticky orphan markers (the rows operators most need to wipe) or in-flight new-entry pending rows. A scope wipe became a no-op for exactly the rows it exists to clean up.get/has/deletethrewTypeErroron invalid keys. All three were routed throughvaultId(), which throws on rejected keys.ICredentialStoredocuments these methods as returningundefined/falsefor absent or invalid keys; throwing breaks substitutability withInMemoryCredentialStoreandEncryptedKvCredentialStore.SAFE_SEGMENTrejected natural credential names. Keys likeopenai.prod(provider.environment) andscope:billing(oauth-style namespacing) bounced off the old^[A-Za-z0-9_-]{1,128}$grammar.CRITICAL/HIGH findings closed
deleteAll()silently skips pending/orphan rows (no public API can clear them)forceDeleteByIdbypasses the pending-skip;deleteAllroutes through it. Single-keydeleteByIdunchanged.get/has/deletethrowTypeErroron invalid keys, violatingICredentialStoreisSafeKey()predicate; readers short-circuit toundefined/false.putstill routes throughvaultId(only path that can persist a colliding id).SAFE_SEGMENTrejects keys with.and:^[A-Za-z0-9._:-]{1,128}$. Still rejects path separators and whitespace.What changed
packages/storage/src/credentials/ServerCredentialStore.tsSAFE_SEGMENTto^[A-Za-z0-9._:-]{1,128}$.private isSafeKey(key)predicate.get,has,deletenow short-circuit through it before touching metadata or the vault.private forceDeleteById(id, key)— same metadata-first ordering and orphan-marker rewrite asdeleteById, but does NOT short-circuit onpending=true.deleteAllroutes each scoped id throughforceDeleteByIdand aggregates per-row errors into anAggregateError.putis unchanged: still routes throughvaultId()and throwsTypeErroron invalid keys.packages/storage/src/credentials/ServerCredentialStore.test.tsputassertsTypeError;get/has/deleteassert absence.deleteAll clears sticky orphan markers— seeded orphan row + normal row, asserts both wiped.deleteAll drops pending in-flight new-entry rows— seededpending: truerow, asserts wiped.deleteAll surfaces vault delete failures as AggregateError— mock vault rejects once, assertsAggregateErrorwith original vault error incausechain.M1: keys with dots and colons round-trip—openai.prodandscope:billingput → get.invalid key short-circuits do not touch metadata or vault— spies onmeta.get,vault.getSecret,vault.deleteSecretverify zero calls for"a/b".Test plan
bunx vitest run packages/storage/src/credentials/ServerCredentialStore.test.ts— 31/31 passbun scripts/test.ts storage vitest— 1357/1357 pass, 13 skippedbun run build:types— full turbo cache hit, no type errorsRefs #543.
Generated by Claude Code