test(ai): add IPv6 + redirect-canonicalisation localOnlyFetch tests#543
Open
sroussey wants to merge 4 commits into
Open
test(ai): add IPv6 + redirect-canonicalisation localOnlyFetch tests#543sroussey wants to merge 4 commits into
sroussey wants to merge 4 commits into
Conversation
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
…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
Coverage Report
File CoverageNo changed files found. |
4 tasks
sroussey
pushed a commit
that referenced
this pull request
Jun 3, 2026
…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
added a commit
that referenced
this pull request
Jun 4, 2026
…ore (sec) (#544) * 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 * fix(storage): restore ICredentialStore contract on ServerCredentialStore (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. * docs(storage): clarify deleteById JSDoc + fix deleteByid typo (review) 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. * refactor(storage): extract commitDelete to dedupe ServerCredentialStore 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
…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.
…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
sroussey
changed the title
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
1 participant
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.
Status after rebase onto current main
Both original commits of this PR were superseded by changes that landed on main:
localOnlyFetchSSRF hardening is fully on main via fix(ai): close WHATWG canonicalisation bypass in localOnlyFetch (sec) #542 (fix(ai): close WHATWG canonicalisation bypass in localOnlyFetch initial-URL check). The initial-URL gate already validatesextractRawHost(input)against the strict-literal grammar; the redirect path correctly relies onurl.hostname(which equals the WHATWG-canonical serialisation ofnext.href— pinned by the new redirect test below).ServerCredentialStorehardening is fully on main via fix(storage): restore ICredentialStore contract on ServerCredentialStore (sec) #544 (fix(storage): restore ICredentialStore contract on ServerCredentialStore). All sub-fixes are present:SAFE_SEGMENTkey/scope grammar (widened to[A-Za-z0-9._:-]so legitimate keys likeopenai.prodandscope:billinground-trip),orphanReasondiscriminator, metadata-first delete ordering,isPendingguard indelete, triple-failure error message correctness,AggregateErrorindeleteAll, and self-evictiontry/catchinget/has.What remains in this PR after rebase: 4 additional regression tests in
packages/test/src/test/ai-provider-api/localOnlyFetch.test.tsthat main does not yet have:accepts an initial IPv6 loopback literal in brackets (positive case)—[::1]rejects an initial IPv6 with a zone identifier—[::1%25eth0]follows a redirect whose Location uses a hex IPv4 spelling — the canonical form is loopback— pins the documented behaviour that aLocation: http://0x7f.0.0.1/is followed because WHATWG canonicalisesnext.hrefto127.0.0.1, which IS loopbackfollows a redirect to a bracketed IPv6 loopback (positive case)A note on the visible diff
Because the rebased history could not be force-pushed in this environment, the displayed PR diff (vs the stale
base.sha) still shows the original Plan A and Plan B work. The actual file contents on the PR head, however, match main exactly for the three production files (localOnlyFetch.ts,ServerCredentialStore.ts,ServerCredentialStore.test.ts); the only net-new lines vs current main are the four IPv6 / redirect-canonicalisation tests above. Merging this PR will therefore add only those four tests to main.A maintainer with push rights may want to force-push a rebased branch to clean up the diff view before merging.
Test plan
bunx vitest run packages/test/src/test/ai-provider-api/localOnlyFetch.test.ts— 19/19 pass locally on the rebased branch.bunx vitest run packages/storage/src/credentials/ServerCredentialStore.test.ts— 31/31 pass locally on the rebased branch (unchanged from main).Original PR description (for history)
This PR originally consolidated Plan A (
localOnlyFetchSSRF hardening, superseded #542) and Plan B (ServerCredentialStorehardening, superseded #541). Both have since been merged separately as #542 and #544.