JavaScript SDK: fall back to the default key when unsupported, and validate caller-supplied server public key ids (KSM-1256)

[stas-schaller]

Summary

A stored serverPublicKeyId outside the bundled key table permanently blocks every request, with no way to recover except by hand-editing the config file. This PR fixes that. It also validates a caller-supplied serverPublicKey/serverPublicKeyId pair before it persists the id. It also fixes a case where a later call that sends only the id wrongly rejects an already-pinned custom key.

Changes

Fixed

• generateTransmissionKey now falls back to the default key (id 7) for one call when the stored serverPublicKeyId is not in the bundled table. Before this fix, it threw an error instead. The SDK never saves this fallback value. A write here could race a concurrent custom-key pin or a concurrent key-rotation write. It could also overwrite a newer SDK build's valid but unrecognized id with 7, on a config shared across mixed-version instances. The pinned custom-key path (options.serverPublicKey) is not affected. An out-of-table id there is a valid, deliberate configuration, and the SDK does not reset it. The fallback still picks only from the bundled Keeper public keys (ids 7-18). It never uses an attacker-controlled or externally supplied key. (KSM-1256)
• The SDK now validates a caller-supplied serverPublicKey/serverPublicKeyId pair before it saves either field. The id must be a positive integer. Before this fix, a valid custom key paired with a bad id saved both values right away. The bad id then permanently blocked every later call, because the custom-key path has no fallback. (KSM-1256)
• When a call sends only a serverPublicKeyId, with no key, the SDK now checks the bundled key table only if no custom key is already stored from an earlier call. Before this fix, the SDK always ran this check. So a later call that re-sent only the id wrongly rejected it. This happened even when the id already matched a custom key stored from an earlier call. One example is a call that re-affirms a rotation hint. (KSM-1256)
• The IL5 4-segment one-time-token's key id segment now rejects '0', as well as non-numeric input. Before this fix, its error message said "must be a positive integer," but its check still accepted zero. (KSM-1256)
• Both save paths above now skip the write when the value already matches storage. Before this fix, they saved the value again on every call. (KSM-1256)

Testing

cd sdk/javascript/packages/core
npm test

New tests in test/keeper.test.ts:

• generateTransmissionKey falls back to the default key when the stored key id is outside the bundled table
• generateTransmissionKey never calls saveString when falling back from a corrupted stored id
• generateTransmissionKey throws when a custom key is pinned but the stored key id is not numeric
• getSecrets rejects a caller-supplied serverPublicKeyId outside the bundled table when no custom key is pinned
• getSecrets rejects a non-numeric serverPublicKeyId even when paired with a valid custom serverPublicKey, and persists neither. This failed on the unfixed code, because the code saved the key before a later call noticed the bad id.
• getSecrets accepts an out-of-table serverPublicKeyId on a later call when a custom key was already pinned by an earlier call. This failed on the unfixed code, which wrongly rejected the call with "not supported without a matching serverPublicKey".
• deleteSecret persists a caller-supplied serverPublicKey/serverPublicKeyId pair on its own, for callers that never go through fetchAndDecryptSecrets first
• IL5 dynamic key - Layer 2: rejects a serverPublicKeyId of '0'. This failed on the unfixed code, which resolved the call instead of rejecting it.

Full suite 121/121 passing, tsc --noEmit clean.

Breaking Changes

None.

Related Issues

• Jira: KSM-1256

[mgallego-keeper]

Re-review at max effort (KSM-1256). The fallback logic itself is correct and well-tested (verified locally: full suite 77/77 passing, clean tsc --noEmit). Left two inline notes on the touched function below. One more finding does not fit as an inline comment since it sits outside this diff's hunk:

keeper.ts:787 (

secrets-manager/sdk/javascript/packages/core/src/keeper.ts

Line 787 in bf35503

await options.storage.saveString(KEY_SERVER_PUBLIC_KEY_ID, options.serverPublicKeyId)

) and the equivalent site at keeper.ts:894: this PR patches the read side (generateTransmissionKey's new fallback), but the write sites that persist serverPublicKeyId into storage never validate it and never write back the corrected id. A corrupted id (e.g. '9999') gets persisted verbatim, and since the fallback now makes every request succeed with key 7 in memory, the server never sends back {"error":"key"}. That means the one write site that does validate (line 840, gated by the checks at lines 830 and 833) never runs, so the bad id stays in storage indefinitely: masked every call, never actually corrected. Might be worth writing effectiveKeyNumber back to storage when it differs from the stored value, so the config self-heals instead of re-deriving the same fallback forever.

[mgallego-keeper]

Pre-existing gap in this same function, flagging since this PR touches it: this branch returns publicKeyId: keyNumber with no validation. keyNumber is Number(keyNumberString) (line 753), so a non-numeric serverPublicKeyId paired with a set serverPublicKey yields publicKeyId: NaN here, which gets sent verbatim as the PublicKeyId header ('NaN') in postFunction. The PR description calls the pinned custom-key path unaffected, but it has the same class of bug this PR just fixed on the other branch.

[mgallego-keeper]

Two smaller notes on this fallback:

1. It silently substitutes key 7 with no logging. Other self-correcting branches in this file log when they compensate for an abnormal condition (the throttle retry at line 815 logs a WARNING). A similar log line here would give an operator a signal that the stored config was corrupted, since right now that's completely silent.
2. keyNumber in keeperPublicKeys duplicates the membership check already at line 833 (suggestedKeyId in keeperPublicKeys), and the default id 7 is now a bare literal in three places (lines 36, 753, 764) with no shared constant, unlike MAX_THROTTLE_RETRIES / MAX_KEY_ROTATION_RETRIES elsewhere in this file. A DEFAULT_KEY_ID = 7 constant would tie these together so a future change to the default cannot update one site and miss another.

[mgallego-keeper]

This fix trades a loud, safe failure (throw, no state change) for several quiet, unsafe ones. Requesting changes: a few of these are regressions that reproduce the exact bug this PR is meant to close, and the two new tests only cover the two happy path scenarios named in the PR description.

All line references below are in sdk/javascript/packages/core/src/keeper.ts unless noted.

Blocking

1. The self heal write itself is unguarded (line 780). await storage.saveString(KEY_SERVER_PUBLIC_KEY_ID, ...) has no try/catch, and nothing wraps the generateTransmissionKey call in postQuery either. If that write rejects (a KMS backed storage that rethrows, browser IndexedDB quota or a blocked upgrade, restrictive filesystem permissions), the rejection propagates straight out of getSecrets/getFolders on every call, reproducing the exact "permanent failure" this PR is fixing, just with a confusing storage error instead of the old "Key number N is not supported" message. There is no test for a failing storage.saveString.

2. Race: a delayed self heal write can clobber a concurrent custom key pin (lines 778-780). If call A reads a corrupted id and is mid flight on its self heal write when call B pins a custom serverPublicKey/serverPublicKeyId, A's write can land after B's and silently revert the id, leaving the custom key bytes in storage paired with the wrong reported publicKeyId. Reproduced locally with a controlled interleaving storage stub.

3. The write side is still unvalidated (around line 780). The fix only patches the read path in generateTransmissionKey. options.serverPublicKeyId supplied directly by a caller is still persisted with no range check at its write sites, so the silent substitution behavior this PR is trying to eliminate is still reachable on a fully patched SDK through that path, not only via a legacy pre KSM-1128 config. Worth checking how this interacts with #1146's persistServerPublicKeyIdOnce: that only validates on a genuinely fresh config, so once any value is already stored, its check is skipped and everything falls through to this PR's self heal path regardless.

4. The custom key branch can return an unvalidated NaN (line 772). If a custom key is pinned but the stored id is non numeric garbage, Number(keyNumberString) is NaN and the custom key branch returns {publicKeyId: NaN, ...} before ever reaching the new fallback logic, so PublicKeyId: "NaN" goes out on the wire. Neither new test exercises the custom key branch.

5. Cross version corruption risk (lines 778-780). Two processes on different SDK builds sharing one persistent config: a newer sibling can legitimately rotate to a key id that the older build's bundled table does not contain yet. The older build now treats that as invalid and silently stomps it back to 7, mutating shared state that the pre fix code left untouched.

6. Lost update race with the key rotation write (line 780 vs. the rotation write a bit further down). On a genuinely async storage backend, the new self heal write can race the pre existing key rotation write from a concurrent request and overwrite a just learned valid rotated id back to 7.

7. Number() parsing now permanently persists a discarded, recoverable id (line 767). A stored id like '12abc' (a valid id plus trailing corruption) parses to NaN and falls back to 7, same as full garbage, but now that fallback gets written back to storage permanently. Before this PR, that case just threw without touching storage. This is a meaningful change in failure semantics, not just an edge case.

Test coverage

Both new tests are the two happy path scenarios named in the PR description. Missing:

• A failing/rejecting storage.saveString during self heal (issue 1)
• Concurrent calls racing the self heal write against a custom key pin or a rotation write (issues 2, 6)
• The custom key branch with a corrupted, non numeric stored id (issue 4)
• A caller supplied out of range serverPublicKeyId on the write path (issue 3)

Non blocking suggestions

• The fallback key 7 is a bare literal duplicated between initialize() and here. A shared constant would avoid a silent break if the table is ever renumbered.
• The self heal write is completely silent; every other correction path in this file logs (the throttle retry warning, the rotation path). A log line would make this visible in production telemetry.
• The fallback is unconditional and unbounded, unlike the rotation path's MAX_KEY_ROTATION_RETRIES cap.
• keyNumber in keeperPublicKeys duplicates the same membership check already used a few lines below (suggestedKeyId in keeperPublicKeys); a shared helper would keep both in sync.
• effectiveKeyNumber plus the !== keyNumber guard re-derives what the ternary already computed; could mutate keyNumber directly instead, matching the existing idiom.
• storage.saveString is awaited before the independent platform.publicEncrypt call; on a network backed storage implementation this serializes two operations that do not depend on each other.

[mgallego-keeper]

Re-review at max effort (KSM-1256), round 3 (commit a48b754). This round removes the self-heal write entirely instead of hardening it, which cleanly closes most of round 2's findings: the unguarded write, the self-heal-vs-custom-key-pin race, the self-heal-vs-rotation-write race, the cross-version stomping risk, the destructive persistence of a malformed id, and the NaN-on-wire case are all gone. Good direction. But the new write-side validation in persistServerPublicKeyOptions only covers one of the two ways a caller can pair a custom key with a bad id, and the gap it leaves reproduces the exact "permanently blocked, no recovery" failure this PR exists to fix, just through a different door. There is also a second, completely independent door into the same failure mode that this round never touched, and one regression from the new validation itself.

Blocking

1. A custom key paired with a bad id in the same call is still persisted unvalidated (persistServerPublicKeyOptions, around line 808-812). When options.serverPublicKey is set, the if (options.serverPublicKeyId) branch saves the id with no check at all, unlike the else if branch a few lines below, which validates. Confirmed by direct reproduction: getSecrets({storage, serverPublicKey: <valid key>, serverPublicKeyId: 'garbage'}) throws from generateTransmissionKey's new guard, but only after the garbage id has already been written to storage paired with the valid key. Every call after that, with or without options, throws the same error forever: the custom key path has no fallback by design, so there is no recovery. This is the identical failure class KSM-1256 targets, reopened through this call's own write path.

2. IL5 bind with id '0' permanently bricks the config (initializeStorage, around line 1168, vs. generateTransmissionKey, around line 770). The IL5 4-segment token path validates the id with /^\d+$/, which accepts '0', but the new guard in generateTransmissionKey's custom-key branch rejects any keyNumber <= 0. Confirmed by direct reproduction: a single initializeStorage call with an IL5 token whose id segment is '0' succeeds, and every getSecrets call after that throws Stored serverPublicKeyId '0' is not a positive integer forever, with no further caller input required. This is a third, independent entry point into the same permanent-brick class, on a code path this round never touched.

3. Re-sending only the id on a later call now wrongly rejects an already-valid pin (persistServerPublicKeyOptions's else if branch, around line 813). The branch decides whether to validate based on whether options.serverPublicKey is present in this call, not on whether a key is already stored from an earlier call. Confirmed by direct reproduction: call 1 with {serverPublicKey: <valid key>, serverPublicKeyId: '20'} succeeds; call 2 on the same storage with only {serverPublicKeyId: '20'} (the key omitted, for example re-sending a rotation hint) throws, even though the matching key is already in storage. This pattern silently worked before round 3 added validation, so it is a new regression, not a pre-existing gap.

Test coverage

• Neither new test exercises the custom-key-plus-bad-id combination from finding 1: generateTransmissionKey throws when a custom key is pinned but the stored key id is not numeric seeds storage directly and calls generateTransmissionKey, bypassing persistServerPublicKeyOptions entirely.
• No test for the IL5-plus-'0' combination from finding 2.
• No test for the key-omitted-on-a-later-call regression from finding 3.
• postQuery is reached directly, without fetchAndDecryptSecrets running first, by nine other exported operations (getFolders, updateSecret, updateSecret2, deleteSecret, deleteFolder, createSecret, createSecret2, createFolder, updateFolder, uploadFile). None of them has a dedicated test exercising persistServerPublicKeyOptions through that path; every existing test for this logic goes through getSecrets.

Non-blocking suggestions

• persistServerPublicKeyOptions has no read-before-write: it re-saves both values on every call even when they already match storage, unlike #1146's analogous helpers, which check first. On the real file-backed storage that means a full config rewrite and a chmod on every call, twice per request since both fetchAndDecryptSecrets and postQuery call it.
• The else if branch's error message says the id "is not supported... this SDK version supports key ids 7-18" even for non-numeric input like 'abc', which was never a parseable number in the first place. Worth a separate message for the format case, the way #1146's equivalent helper already does.
• options.serverPublicKeyId is only truthiness-checked; a caller passing the value 0 or NaN has it silently dropped on every branch, with no error and no persistence.
• Id validation now lives in four places with three different shapes: the two branches here, the key-rotation write further down in postQuery, and the IL5 regex in initializeStorage. That is the structural reason findings 1 through 3 keep showing up in different corners; consolidating into one shared check would close all of the current gaps and most future ones in the same move.
• From round 2, still open: the literal 7 is now hardcoded in 3 places (was 2), and X in keeperPublicKeys is now duplicated 3 times (was 2). effectiveKeyNumber is unchanged.

[mgallego-keeper]

Re-review at max effort (KSM-1256), round 4 (commit 39ec0c7). This round fixes all three round-3 blocking findings cleanly: confirmed by checking out the pre-fix commit (a48b754) in an isolated worktree and running the targeted tests against both, all three fail against a48b754 and all pass against 39ec0c7. Good progress. But the fix's mechanism, switching persistServerPublicKeyOptions's checks from truthy to strict !== undefined, was applied only to that one function; generateTransmissionKey and postQuery's key-error branch still read the same stored values with truthy checks, and that mismatch reopens the same "silently misconfigured, no error" failure class in a new shape. There are also two correctness findings from my round-2/round-3 reviews that I deliberately held back from the posted review at the time to keep it scoped; they are still open and unchanged by this commit, so I'm surfacing them now.

All line references are in sdk/javascript/packages/core/src/keeper.ts unless noted.

Blocking

1. Empty-string serverPublicKey silently downgrades a pinned custom key to the bundled default (persistServerPublicKeyOptions vs. generateTransmissionKey:772 and postQuery:895). This round switched persistServerPublicKeyOptions's checks on options.serverPublicKey from truthy to !== undefined, but generateTransmissionKey's custom-key branch and postQuery's key-error branch still check the same stored value with if (customPublicKeyB64) / if (customKey). Confirmed by tracing all three sites: a caller who calls getSecrets({storage, serverPublicKey: ''}) (for example via a someVar ?? '' default, a common idiom) after already pinning a real key gets that key overwritten with '' in storage, no error thrown, since '' !== undefined. Every later call then reads '' as falsy and silently falls back to bundled key 7, and postQuery's rotation logic additionally starts treating the deployment as keyless.

2. A caller-reused options object silently undoes a server-driven key rotation on every later call (postQuery:909 vs. persistServerPublicKeyOptions). The rotation write at line 909 goes straight to storage, bypassing persistServerPublicKeyOptions, and self-heals correctly within the call that triggers it. But nothing updates the caller's own options object. Confirmed by tracing postQuery's retry loop directly: if a caller builds options = {storage, serverPublicKeyId: '7'} once and calls getSecrets(options) repeatedly (idiomatic usage, since options objects are typically built once and reused across calls), the next call's persistServerPublicKeyOptions sees storage now holding the rotated id and the stale caller value disagreeing, and reverts storage back to the stale value before the request goes out, forcing another reject-then-rotate round trip. This repeats on every subsequent call, forever, after any rotation.

3. Held back from my round-3 review, still open, unchanged by this commit: a custom key pinned with no id defaults the wire's reported publicKeyId to 7 while actually encrypting with the real custom key (generateTransmissionKey:772-780). getSecrets({storage, serverPublicKey: myKey}) with no id, on fresh storage, throws nothing: the wire reports publicKeyId: 7 but the payload is encrypted with myKey's bytes.

4. Held back from my round-3 review, still open, unchanged by this commit: a key-only rotation leaves a stale id paired with the new key (persistServerPublicKeyOptions:836-838). The id write only happens when options.serverPublicKeyId !== undefined; a caller rotating to a new serverPublicKey with no new id leaves the previous key's id in storage, now paired with the new key.

5. hasCustomKey only checks whether any custom key exists in storage, not whether it corresponds to the id being validated (line 844). This closes round 3's regression (options-based check) but the replacement is still coarse: once any custom key has ever been pinned, a later id-only call can inject an arbitrary out-of-table id with zero relationship check, since hasCustomKey stays true regardless of what id is being asserted. Confirmed by direct reproduction: pin {serverPublicKey: keyA, serverPublicKeyId: '20'}, then a later call with only {serverPublicKeyId: '9999'} overwrites '20' with '9999', no error.

6. generateTransmissionKey's custom-key branch still has no fallback for a corrupted stored id, unlike the branch a few lines below that this PR exists to fix (lines 772-777). storage = {serverPublicKey: realKey, serverPublicKeyId: 'garbage'} throws on every single call, forever, with no server round-trip able to correct it. Same permanent-block failure class KSM-1256 targets, just still open for the custom-key case specifically.

7. Empty-string serverPublicKeyId now throws where round 3 silently ignored it (line 839). The id-alone branch's guard also switched from truthy to !== undefined. A caller building serverPublicKeyId: someVar ?? '' used to have it silently skipped; now every call throws serverPublicKeyId '' must be a positive integer.

Test coverage

• No test for the empty-string downgrade in finding 1.
• No test for the rotation-write-undone scenario in finding 2.
• The new "accepts an out-of-table id on a later call when a custom key was already pinned" test only re-sends the same id both times, so it never exercises finding 5's arbitrary-id-injection gap.
• deleteSecret now has a dedicated test for persisting a caller-supplied pair on its own, but it calls initializeStorage first, so it doesn't cover the case where a prepare*Payload precondition throws before persistServerPublicKeyOptions ever runs (relevant to all 9 postQuery-reached operations besides getSecrets, noted in round 3, still not actually covered).

Non-blocking suggestions

• The non-atomic key-then-id write pair (open since round 2) is no longer just theoretical: a randomized-jitter concurrency trial with two callers pinning different pairs produced a torn/mismatched final pair in 24 of 60 trials. persistServerPublicKeyOptions also now runs up to 4x per logical getSecrets call (fetchAndDecryptSecrets calls it, then postQuery calls it again, up to twice more on the first-bind justBound path), which is what widens the window.
• parseServerKeyId's bare Number() coercion is more lenient than the /^\d+$/ regex it replaced on the IL5 token path: a corrupted token segment like '0x10' used to fail loudly and now silently succeeds, reinterpreted as 16.
• The IL5 branch's catch {} around parseServerKeyId discards the thrown KeeperError's type, re-throwing a plain Error, the only "must be a positive integer" site in the file that doesn't preserve instanceof KeeperError.
• From round 3, still open: id validation (Number.isInteger(n) && n > 0) is duplicated across parseServerKeyId, generateTransmissionKey's custom-key branch, and postQuery's rotation branch instead of all three routing through the one helper this round introduced.
• Cross-PR: a fresh trial merge against #1146 (still at 072dab3) now fails tsc --noEmit with a duplicate-declaration error (parseServerKeyId and persistServerPublicKeyOptions are both independently defined, incompatibly, by each PR), not just the two call-site conflicts noted previously. Worth reconciling before either merges, since resolving only the marked hunks won't compile.