JavaScript SDK: persist serverPublicKeyId and serverPublicKey once instead of re-saving on every call (KSM-1255)

[stas-schaller]

Summary

Fixes options.serverPublicKeyId and options.serverPublicKey being re-saved to storage on every postQuery/fetchAndDecryptSecrets call. For serverPublicKeyId, this undid a completed server key rotation and forced the client to re-rotate on every subsequent request. For serverPublicKey, it was an unnecessary write on every request. Later review rounds found the write-once fix itself could leave the two fields mismatched, and that one input shape could still reopen the original bug.

Changes

Fixed

• A caller pinning only serverPublicKeyId (no serverPublicKey custom key content) had that pin re-clobber storage after a completed rotation, since both postQuery and fetchAndDecryptSecrets re-persisted it unconditionally on every call. The id is now persisted once, on the first call against a fresh config, via a new persistServerPublicKeyIdOnce helper shared by both call sites; rotation owns every update to it after that. This matches how the other KSM SDKs already handle this value (write once at setup, never re-save per call), confirmed across Python, Java, Ruby, and Rust. (KSM-1255)
• serverPublicKey had the same unconditional re-save at both call sites. A new persistServerPublicKeyOnce helper, mirroring persistServerPublicKeyIdOnce, now writes it once on the first call and leaves it alone after. (KSM-1255)
• Closes the unvalidated-persist gap flagged by the companion recovery ticket (KSM-1256): a caller-supplied serverPublicKeyId outside the bundled key table (7-18) is now rejected immediately at the point it's supplied, before anything is persisted or any request is sent, unless a custom key is also pinned; an out-of-table id there is legitimate (e.g. IL5) and is not validated against the bundled table.
• The rejection message now distinguishes an invalid format (e.g. 'abc', '-1', '7.5') from a validly-formatted id outside the bundled table (e.g. '99'), instead of one generic message for both.
• Round 2: persistServerPublicKeyOnce and persistServerPublicKeyIdOnce were two independent write-once gates, so pinning both fields together while storage already had one of them set (a stale id from an earlier rotation, or a partially-seeded config) only wrote the missing field, leaving a mismatched key/id pair. generateTransmissionKey would then encrypt with the new key but report the old id on the wire. A new persistServerPublicKeyOptions dispatcher treats a same-call pair as one atomic bound identity: writes both whenever the incoming pair differs from what's stored, no-op when it already matches. initializeStorage's IL5 4-segment branch now goes through the same dispatcher instead of writing both fields directly, so a second IL5 bind can't leave a mixed pair either. (KSM-1255)
• Round 2: hasCustomKey was derived from the current call's options.serverPublicKey instead of from storage state, so an id-only pin on a call after an earlier key-only pin wrongly validated the id against the bundled table and could throw, rejecting a legitimate pin with zero network calls. hasCustomKey is now derived from storage (is a custom key already persisted, from any prior call), not from the current call's options. (KSM-1255)
• Round 2: the positive-integer format check and the bundled-table membership check were both gated on hasCustomKey, so a custom-key-paired serverPublicKeyId like 'not-a-number' skipped all validation and reached the server as the literal string "NaN". The format check (extracted into a shared parseServerKeyId()) now always runs; only the table-membership check stays gated on hasCustomKey. (KSM-1255)
• Round 3: a stored serverPublicKey: '' round-trips back as undefined through the shared in-memory storage backend's falsy-value read, so the write-once comparison in round 2's dispatcher never matched and every call re-clobbered storage again for this one input shape (an env-var or templating-assembled config commonly produces '' for an unset optional field rather than omitting it entirely) - reopening the exact bug this PR exists to fix. An empty serverPublicKey is now normalized to "not supplied" at the top of the dispatcher before either write-once path runs. serverPublicKeyId is deliberately left alone: it already fails loud on an empty string, before any write, on every path, so there's no matching gap to close. (KSM-1255)
• Round 3: the atomic-pair dispatcher wrote the key before the id. A storage failure between the two writes left a custom key with no id, and generateTransmissionKey silently fell back to the default key while still holding the leftover custom key bytes, producing an opaque server-side decrypt failure instead of a clear client-side rejection. The two writes are now ordered id-then-key, so the same kind of partial failure leaves an id with no key and fails loud on the next call instead ("Key number N is not supported"). This isn't full atomicity (no batched-write primitive exists across KeyValueStorage's backends), just the less harmful of the two partial-failure orderings.
• Round 3: an IL5 token whose key-id segment is '0' now correctly fails at initializeStorage time (matching the already-shipped fix in the sibling PR for the identical case), but the rejection was missing the "IL5 token: " prefix its two sibling checks in the same branch already carry. Wrapped to restore the consistent prefix.

Testing

cd sdk/javascript/packages/core
npm test

14 new/changed tests total across three review rounds in test/keeper.test.ts, all confirmed failing against the pre-fix code for the stated reason before applying each fix.

Round 1:

• id-only serverPublicKeyId pin does not reclobber a completed rotation on a later call failed with 4 requests instead of 3 across two calls.
• caller-supplied serverPublicKeyId outside the bundled table is rejected upfront, not persisted failed by throwing the old deep-internal error instead of the new upfront one.
• an out-of-table serverPublicKeyId alongside a pinned custom key is not validated against the bundled table guards the IL5 custom-key path while adding validation for the id-only case.
• pinned serverPublicKey is not reclobbered on subsequent calls failed with 4 storage writes instead of 1 across two calls.
• caller-supplied serverPublicKeyId in an invalid format is rejected with a format error, not a table-membership error failed against the old message (serverPublicKeyId abc is not supported).
• concurrent calls against fresh storage do not corrupt the persisted serverPublicKeyId is new coverage, not a regression test.

Round 2:

• pinning both fields together rebinds the pair atomically, not split across two independent gates failed by leaving the stale id ('10') in storage instead of the newly-pinned id ('20').
• an id-only pin after an earlier key-only pin uses storage, not this call, to decide whether a custom key applies failed by throwing serverPublicKeyId 20 is not supported instead of accepting the pin.
• a custom-key-paired serverPublicKeyId still gets format-validated, not sent as NaN failed by silently persisting the garbage id instead of throwing.
• postQuery persists serverPublicKey/serverPublicKeyId on its own, for callers that never go through fetchAndDecryptSecrets first closes a gap confirmed by commenting out postQuery's persist call locally: all existing tests still passed, since every one of them reaches postQuery via fetchAndDecryptSecrets, which already persists the values first.

Round 3:

• empty-string serverPublicKey is treated as not supplied, not persisted or reclobbered failed with 4 writes across two calls instead of 0.
• empty-string serverPublicKey paired with an out-of-table id does not bypass table-membership validation failed by reaching the network layer instead of rejecting upfront ('' and the id were both treated as supplied, skipping the id-only branch's table check entirely).
• a storage failure between the id write and the key write fails loud on the next call, not silently failed because the pre-fix key-then-id order left a key with no id, so the follow-up generateTransmissionKey call succeeded silently with the default key instead of throwing.
• IL5 dynamic key - Layer 2: rejects key id '0' with the "IL5 token:" prefix, matching its sibling checks failed against the unprefixed message.
• The existing concurrent calls against fresh storage... test was rewritten to race two different ids instead of the same id twice - the old version passed even with the write-once guard deleted entirely, so it couldn't detect a regression. Confirmed the new version fails under that same experiment.
• Added '' to the existing invalid-serverPublicKeyId-format test's input list, closing the one case that wasn't already covered.

Full suite 127/127 passing after all three rounds; tsc --noEmit clean.

Breaking Changes

None.

Related Issues

• Jira: KSM-1255

[mgallego-keeper]

Code Review: Comprehensive Analysis

I've conducted a thorough code review of this PR, including deep analysis of the key rotation logic, storage patterns, and test coverage. Overall, this is an excellent fix for the serverPublicKeyId reclobber bug. The implementation is clean, well-tested, and aligns with cross-SDK conventions.

However, I found one critical issue and several test gaps that should be addressed.

---

✅ What's Good

1. Core Fix is Correct: The persistServerPublicKeyIdOnce() helper properly implements write-once semantics

   • Write-once check prevents reclobbering after rotation ✓
   • Validation prevents invalid IDs from being persisted ✓
   • IL5 exception (custom key present) allows out-of-table IDs ✓
   • Rotation path still works correctly (direct saveString) ✓

2. Well-Tested: Three new tests verify:

   • Main fix: rotation doesn't get reclobbered (queryCalls === 3, not 4) ✓
   • Validation: out-of-table ID rejected upfront, not persisted ✓
   • IL5 exception: out-of-table ID with custom key is allowed ✓

3. Follows Conventions: Matches the write-once pattern used elsewhere (KEY_OWNER_PUBLIC_KEY, KEY_CLIENT_ID, etc.)

4. Clear Documentation: CHANGELOG entry and code comments explain the bug and fix comprehensively

5. Cross-SDK Alignment: Matches Python, Java, Ruby, and Rust SDK patterns

---

🔴 Critical Issue: serverPublicKey Has the Same Bug

Lines 802-804 (postQuery) and 907-909 (fetchAndDecryptSecrets):

if (options.serverPublicKey) {
    await options.storage.saveString(KEY_SERVER_PUBLIC_KEY, options.serverPublicKey)
}

This is the exact same bug that you're fixing for serverPublicKeyId. It unconditionally saves on every call, causing:

• Unnecessary storage writes on every API request (performance impact)
• Potential interference with any key management that modifies serverPublicKey in storage
• Inconsistency: serverPublicKeyId now uses write-once, but serverPublicKey doesn't

Recommended Fix

Create a parallel helper function:

const persistServerPublicKeyOnce = async (storage: KeyValueStorage, serverPublicKey: string | undefined): Promise<void> => {
    if (!serverPublicKey) {
        return
    }
    const stored = await storage.getString(KEY_SERVER_PUBLIC_KEY)
    if (stored !== undefined) {
        return
    }
    // Optional: validate format (base64, length >= 80)
    await storage.saveString(KEY_SERVER_PUBLIC_KEY, serverPublicKey)
}

Then replace the unconditional saves with:

await persistServerPublicKeyOnce(options.storage, options.serverPublicKey)

Required Test

test('serverPublicKey is not reclobbered on subsequent calls', async () => {
    const fakeKey = 'BK9w6TZFxE6nFNbMfIpULCup2a8xc6w2tUTABjxny7yFmxW0dAEojwC6j6zb5nTlmb1dAx8nwo3qF7RPYGmloRM'
    const storage = inMemoryStorage({})
    await initializeStorage(storage, FAKE_ONE_TIME_TOKEN, 'fake.keepersecurity.com')
    
    const options: SecretManagerOptions = {
        storage,
        serverPublicKey: fakeKey,
        serverPublicKeyId: '20',
        queryFunction: mockSuccessfulQuery
    }
    
    await getSecrets(options)
    expect(await storage.getString('serverPublicKey')).toBe(fakeKey)
    
    // Simulate a key change in storage
    await storage.saveString('serverPublicKey', 'DIFFERENT_KEY')
    
    // Second call should NOT reclobber back to fakeKey
    await getSecrets(options)
    expect(await storage.getString('serverPublicKey')).toBe('DIFFERENT_KEY')
})

---

⚠️ Test Gaps Identified

Gap 1: Standard Key Rotation Flow (High Priority)

Missing: Test for successful automatic key rotation in the common (non-IL5) case.

Suggested Test:

test('standard key rotation from 7 to 8 succeeds and persists', async () => {
    const storage = inMemoryStorage({})
    await initializeStorage(storage, FAKE_ONE_TIME_TOKEN, 'fake.keepersecurity.com')
    // Storage starts with ID 7 (default)
    
    const enc = new TextEncoder()
    const emptyResponse = enc.encode(JSON.stringify({ records: [], folders: [], expiresOn: 0, warnings: [] }))
    let queryCalls = 0
    
    const options: SecretManagerOptions = {
        storage,
        queryFunction: async (_url, tk) => {
            queryCalls++
            // First call uses key 7, server suggests rotation to 8
            if (queryCalls === 1) {
                return { statusCode: 400, data: enc.encode(keyErrorResponse(8)), headers: [] }
            }
            // Second call should use key 8
            expect(tk.publicKeyId).toBe(8)
            return { statusCode: 200, data: await platform.encryptWithKey(emptyResponse, tk.key), headers: [] }
        }
    }
    
    await getSecrets(options)
    expect(queryCalls).toBe(2) // rotation happened
    expect(await storage.getString('serverPublicKeyId')).toBe('8')
    
    // Third call should use key 8 without re-rotation
    await getSecrets(options)
    expect(queryCalls).toBe(3)
    expect(await storage.getString('serverPublicKeyId')).toBe('8')
})

Gap 2: First-Time Binding with Pinned ID (High Priority)

Missing: Test ensuring write-once doesn't break the first-time binding flow.

Suggested Test:

test('first-time binding works with pinned serverPublicKeyId', async () => {
    const storage = inMemoryStorage({})
    await initializeStorage(storage, FAKE_ONE_TIME_TOKEN, 'fake.keepersecurity.com')
    
    // Clear appKey to simulate pre-binding state
    await storage.delete(KEY_APP_KEY)
    
    const options: SecretManagerOptions = {
        storage,
        serverPublicKeyId: '8',
        queryFunction: mockBindingThenSuccessfulQuery
    }
    
    // First call binds
    const secrets1 = await getSecrets(options)
    expect(await storage.getString('serverPublicKeyId')).toBe('8')
    
    // Second call works normally
    const secrets2 = await getSecrets(options)
    expect(secrets2).toBeDefined()
})

Gap 3: Input Validation Edge Cases (Medium Priority)

Missing: Tests for invalid input formats.

Suggested Tests:

• serverPublicKeyId: "abc" → should reject with clear error message
• serverPublicKeyId: "-1" → should reject
• serverPublicKeyId: "7.5" → should reject
• serverPublicKeyId: "0" → should reject (key 0 doesn't exist)

These currently get rejected (good!), but explicit tests would provide confidence and document expected behavior.

---

💡 Minor Improvements

1. Error Message Clarity

The error message could distinguish between format errors and unsupported keys:

// Current
throw new Error(`serverPublicKeyId ${serverPublicKeyId} is not supported`)

// Suggested
const numericId = Number(serverPublicKeyId)
if (isNaN(numericId) || numericId <= 0 || !Number.isInteger(numericId)) {
    throw new Error(`serverPublicKeyId must be a positive integer, got: ${serverPublicKeyId}`)
}
if (!(numericId in keeperPublicKeys)) {
    throw new Error(`serverPublicKeyId ${serverPublicKeyId} is not in the bundled key table (7-18)`)
}

2. JSDoc Comment

Consider adding a JSDoc comment to document the helper:

/**
 * Persists a caller-supplied serverPublicKeyId once, on the first call against a fresh config.
 * Rotation owns every update after that; re-writing the caller's pin on every call would undo
 * a completed rotation and force re-rotation on every subsequent request.
 * 
 * @param storage - Storage interface
 * @param serverPublicKeyId - Optional caller-supplied server public key ID
 * @param hasCustomKey - Whether a custom serverPublicKey is also pinned (IL5 case)
 */

---

📋 Summary of Recommendations

Critical (Must Fix Before Merge)

1. Apply write-once pattern to serverPublicKey (same as serverPublicKeyId fix)
2. Add test for serverPublicKey write-once behavior

High Priority (Should Fix)

3. Add test for standard key rotation flow (ID 7 → 8)
4. Add test for first-time binding with pinned serverPublicKeyId

Medium Priority (Consider)

5. Improve input validation error messages
6. Add edge case tests for invalid inputs
7. Add JSDoc comment to helper function

Low Priority (Optional)

8. Test concurrent calls on fresh storage
9. Expand error handling tests

---

Verification Steps

After addressing the critical fixes:

1. Run test suite: npm test (should be 82+ tests passing)
2. Type checking: tsc --noEmit (should be clean)
3. Manual verification: Test with IL5 deployment and standard deployment

---

Overall Assessment

✅ APPROVE with required changes

• Core fix: Excellent ⭐
• Tests: Good, with identified gaps
• Critical issue: serverPublicKey needs same treatment as serverPublicKeyId
• After addressing serverPublicKey: Ready to merge

This is high-quality work that addresses a real performance bug. The write-once pattern is exactly the right approach, and the tests demonstrate careful thinking about the fix. Once serverPublicKey gets the same treatment, this will be ready to ship.

[mgallego-keeper]

Round 2 Follow-up

Thanks for adding the write-once treatment for serverPublicKey, that closes the gap from the last round. Digging into how the two helpers interact turned up a new regression plus a few gaps worth closing before this merges.

---

Critical: mismatched key/id pairs are now possible

persistServerPublicKeyOnce and persistServerPublicKeyIdOnce are two independent write-once gates instead of one atomic write. If storage already has one of the two fields set (from an earlier call, a prior rotation, or a partially-seeded config) and a caller pins both fields together, only the missing one gets written:

// storage already has serverPublicKeyId='10', no serverPublicKey
await getSecrets({storage, serverPublicKey: K, serverPublicKeyId: '20', ...})
// persistServerPublicKeyOnce writes K (storedKey was undefined)
// persistServerPublicKeyIdOnce sees storedKeyId='10' already set and skips '20'
// storage now holds {serverPublicKey: K, serverPublicKeyId: '10'} -- a pair that was never actually configured together

generateTransmissionKey then encrypts with K but reports publicKeyId: 10 on the wire. The server can't decrypt, returns {"error":"key"}, and the custom-key branch throws a message blaming key id 10, not the real mismatch.

Before this PR, both fields were always written together in one operation, so they couldn't drift apart. Splitting them into independent gates dropped that coupling. Worth deciding explicitly: should supplying both fields in the same call be treated as an atomic rebind (write both regardless of prior state), even though each field alone still gets write-once treatment? Happy to help implement whichever direction you'd prefer.

Critical: hasCustomKey is sourced from the wrong place

hasCustomKey (line 818) is !!options.serverPublicKey from the current call, not "is a custom key already persisted in storage":

await persistServerPublicKeyIdOnce(options.storage, options.serverPublicKeyId, !!options.serverPublicKey)

Call 1 pins serverPublicKey: K (persisted, no id supplied). Call 2 passes only serverPublicKeyId: '20' (the key is already in storage, so this call doesn't repeat it). hasCustomKey evaluates to false even though K is already pinned, so '20' gets validated against the bundled 7-18 table, fails, and throws, rejecting a legitimate pin for an already-configured custom key with zero network calls.

Suggest deriving this from storage state ((await storage.getString(KEY_SERVER_PUBLIC_KEY)) !== undefined || !!serverPublicKey) rather than from this call's options alone.

High: custom-key pin also disables format validation

The format check and the table-membership check are both nested inside if (!hasCustomKey) (lines 818-827). Pinning a custom key disables format validation too, not just table validation:

await getSecrets({storage, serverPublicKey: K, serverPublicKeyId: 'not-a-number', ...})
// throws nothing; 'not-a-number' is persisted verbatim, Number('not-a-number') = NaN,
// and publicKeyId: NaN reaches the server as the literal header "NaN"

Suggest pulling the format check out so it always runs, and only gating the table-membership check on hasCustomKey:

const parsedKeyId = Number(serverPublicKeyId)
if (!Number.isInteger(parsedKeyId) || parsedKeyId <= 0) {
    throw new Error(`serverPublicKeyId '${serverPublicKeyId}' must be a positive integer`)
}
if (!hasCustomKey && !(parsedKeyId in keeperPublicKeys)) {
    const supported = Object.keys(keeperPublicKeys)
    throw new Error(`serverPublicKeyId ${parsedKeyId} is not supported; this SDK version supports key ids ${supported[0]}-${supported[supported.length - 1]}`)
}

---

Test gaps

1. postQuery's own copy of the two persist calls (lines 841-842) has no coverage. Confirmed by commenting out both lines locally and running the suite: all 27 tests in keeper.test.ts still passed. Every test that reaches postQuery goes through getSecrets first, so fetchAndDecryptSecrets's copy of the calls already persisted the values by the time postQuery's copy runs, making it a no-op whenever it's reached. A test that calls a postQuery-only entry point (updateSecret, deleteSecret, createFolder, and similar) directly, with a config that hasn't gone through getSecrets, would close this.
2. Validation only runs on the very first call that persists the id, ever. A garbage serverPublicKeyId supplied on a later call, after a valid one was already persisted, is silently ignored rather than rejected, since the early-return check at line 813 fires before the validation block. Worth a test that pins a valid id first, then asserts a second call with an invalid id still throws (or documents that it intentionally doesn't).
3. A test for the hasCustomKey-sourced-from-wrong-place case above (pin the key on call 1, pin an out-of-table id alone on call 2).
4. A test for the format-check-bypassed-by-custom-key case above.

---

Medium priority

• initializeStorage's IL5 branch bypasses both persist-once helpers entirely (lines 1190-1191): it writes both keys directly via storage.saveString, so calling it a second time with a new 4-segment IL5 token silently overwrites an already-pinned key and id. This contradicts the new doc comment ("nothing else in the SDK ever updates this value once pinned") and the CHANGELOG line ("rotation owns every update to serverPublicKeyId after that"). Either route this through the same write-once helpers, or adjust the comment and CHANGELOG to scope the guarantee to the getSecrets/postQuery path.
• Three independent key-id validity checks now exist (this file's Number() coercion, postQuery's rotation branch typeof check, and initializeStorage's /^\d+$/ regex), with different semantics: a string '7' is accepted by one and rejected by another for what's conceptually the same input. Worth extracting a single shared validator.
• Table-membership error message quotes the coerced value, not the caller's input (line 825): serverPublicKeyId: '0x13' throws an error citing 19, not 0x13, making it harder to trace back to the source.

Low priority

• A numeric serverPublicKeyId of 0 or NaN is falsy in JS, so it silently skips validation entirely (if (!serverPublicKeyId) return), while the string '0' is correctly rejected a few lines later. Only reachable by a caller bypassing the string | undefined type, but worth serverPublicKeyId === undefined instead of a truthiness check for consistency.
• getString/saveString in both persist-once helpers are check-then-act with no lock, so concurrent calls against fresh shared storage with different pins can race to a last-write-wins result. Likely fine to accept as a known limitation given the usage pattern, but worth a one-line comment acknowledging it if so.

---

Once the design question on the first item is settled I'm glad to help implement the fix and the missing tests.

[mgallego-keeper]

Round 3 follow-up

Thanks, hasCustomKey now correctly reads from storage, the custom-key path is format-validated too, and postQuery's own persist call finally has coverage. The atomic-pair dispatcher also correctly closes the single-call repro from last round. Digging into the new dispatcher and the refactor around it turned up a few more issues, one of which reopens this PR's original bug for a specific input shape.

---

Critical: the undefined-check refactor reopens the original bug for empty strings

Switching persistServerPublicKeyOnce/persistServerPublicKeyIdOnce from a truthy check to === undefined correctly fixes the old falsy 0/NaN bypass, but it now disagrees with two other truthy checks on the same data: generateTransmissionKey's if (customPublicKeyB64), and storage's own read path (platform.ts's getValue, if (!obj) return undefined). A saved empty string reads back as undefined:

// fresh storage
await getSecrets({storage, serverPublicKey: '', serverPublicKeyId: '8', queryFunction})
// persistServerPublicKeyOptions writes both. Next call, same options:
await getSecrets({storage, serverPublicKey: '', serverPublicKeyId: '8', queryFunction})
// storage.getString(KEY_SERVER_PUBLIC_KEY) reads the just-written '' back as undefined (platform.ts getValue),
// so storedKey === serverPublicKey is undefined === '', always false, so both keys get rewritten again.
// Every call rewrites both. This is the exact bug this PR exists to fix, reopened for serverPublicKey: ''.

Measured across 3 calls with this shape: 6 writes of each key, not 1. Separately, {serverPublicKey: '', serverPublicKeyId: <out-of-table>} skips table-membership validation entirely, since '' is !== undefined and reads as "a custom key is present" for gating purposes, contradicting the CHANGELOG line claiming an out-of-table id is "rejected immediately... instead of being persisted unvalidated."

serverPublicKey?: string doesn't distinguish "omitted" from "explicitly blank," and env-var or templating-assembled configs (the IL5/gov-cloud audience this feature targets) routinely produce '' for an unset optional field rather than omitting the key entirely. Suggest normalizing at whichever layer is the right owner: either treat '' as equivalent to undefined at the point these options are read, or fix getValue's truthy check so a stored empty string round trips correctly.

High: the "atomic" pair write isn't atomic, and a partial failure is now silent for IL5

persistServerPublicKeyOptions's both fields branch writes the key and id via two separate sequential saveString calls, despite its own doc comment calling this "one atomic bound identity":

await storage.saveString(KEY_SERVER_PUBLIC_KEY, serverPublicKey)
await storage.saveString(KEY_SERVER_PUBLIC_KEY_ID, serverPublicKeyId)

A failure between the two (disk full, IndexedDB failure) leaves a permanently mismatched pair; confirmed this with a storage wrapper that fails only the second write. Worth noting since this file's own CHANGELOG this release already needed a fix for IndexedDB writes hanging or failing (KSM-1332), so a mid-pair storage failure isn't a far-fetched hypothetical here.

This also flipped the failure mode for the IL5 call site specifically. The old direct writes went id-then-key, so a partial write left an id with no key, which generateTransmissionKey rejects loudly ("Key number N is not supported"). The new shared helper writes key-then-id, so the same kind of partial write now leaves a key with no id; generateTransmissionKey silently defaults to key id 7 and encrypts with the real custom key anyway, so the server can only see an opaque decrypt failure. Silent wrong key beats loud rejected key for exactly the deployments this path serves.

If it's worth closing properly rather than reordering again: a single KeyValueStorage.saveStrings(entries) primitive, or storing the pair as one JSON blob under one key, would remove this whole class of ordering question rather than relying on two calls staying coordinated by convention. Not asking for that in this PR, just flagging it as the more durable option if this comes up again.

Medium: the new concurrency test can't detect the regression it's testing for

test('concurrent calls against fresh storage do not corrupt the persisted serverPublicKeyId', async () => {
    ...
    serverPublicKeyId: '8', // identical on both racing calls
    ...
    await Promise.all([getSecrets(options), getSecrets(options)])
    expect(await storage.getString('serverPublicKeyId')).toBe('8')
})

Both racing calls pin the same id, so every possible interleaving converges on '8' regardless of whether any write-once gating exists. Confirmed by deleting persistServerPublicKeyIdOnce's entire write-once guard (if (storedKeyId !== undefined) return), a real regression back to this PR's original bug, and rerunning: this test still passes. Worth a variant where the two concurrent calls pin genuinely different values, so the test can actually fail if the guard regresses.

Medium: cross-call mismatched pairs are still possible

The atomic-pair path only fires when both fields arrive in the same call. Two ways to still land a pair that was never validated together:

// (a) single-field sequence
await getSecrets({storage, serverPublicKeyId: '10', queryFunction}) // in-table, accepted, no custom key yet
await getSecrets({storage, serverPublicKey: K, queryFunction}) // key-only; this call never supplies an id
// storage ends up: {serverPublicKey: K, serverPublicKeyId: '10'}, never supplied together, never cross-checked

// (b) rotation-established id
// call 1: {serverPublicKeyId: '7'} -> server returns {"error":"key","key_id":11} -> postQuery writes '11'
// directly (line ~941), bypassing the dispatcher entirely
// call 2, later: {serverPublicKey: K1} -> persistServerPublicKeyOnce writes K1 without looking at the id
// storage ends up: {serverPublicKey: K1, serverPublicKeyId: '11'}, again never requested together

Both fail loud via the existing if (customKey) throw guard on the next request rather than corrupting silently, so this is bounded, but it's the same class of bug as last round's critical finding, just reached across calls instead of within one.

Medium: an IL5 token with key id '0' used to succeed; now it aborts initialization entirely

Traced across all three commits: pre-PR and through rounds 1 and 2, the IL5 branch had no > 0 check anywhere on this path, so a 4-segment token with key id '0' completed successfully. Round 3 routes this through the shared parseServerKeyId, whose <= 0 check now throws before KEY_CLIENT_ID/KEY_HOSTNAME are ever saved:

await initializeStorage(storage, 'il5:token:0:' + '<80+ char key>', host)
// /^\d+$/.test('0') passes the branch's own pre-check (line ~1230)
// parseServerKeyId('0') then throws "serverPublicKeyId '0' must be a positive integer" (no "IL5 token: " prefix)
// nothing gets persisted, initializeStorage aborts entirely, where every prior version of this code succeeded

Low real-world likelihood since 0 isn't a plausible deliberate id, but it's a genuine behavior regression rather than just a message-wording issue, and no test in this diff exercises an IL5 token with this value.

Low: validator fragmentation, from last round, is still only half fixed

parseServerKeyId is now shared by 2 of the 3 original call sites, but the IL5 branch's own /^\d+$/ regex and postQuery's rotation-branch inline check remain separate. Concrete fallout: Number() coercion accepts hex, octal, binary, and exponential strings, so serverPublicKeyId: '0x63' silently becomes 99, and the resulting error cites 99, a value that shares no digits with what the caller typed. Same root cause as last round's note, still open.

Low: CHANGELOG and doc comments haven't caught up with this commit

CHANGELOG.md's KSM-1255 entry is untouched since round 1: it doesn't mention the atomic-pair dispatcher, the hasCustomKey-from-storage fix, or the always-on format check. "Rotation owns every update to serverPublicKeyId after that" is also no longer fully accurate now that the atomic-pair path can overwrite it independent of rotation.

FYI, pre-existing, not introduced by this round

While reading through initializeStorage's IL5 branch I noticed the persist call happens before the existingClientId mismatch check later in the function, so a second initializeStorage call with a conflicting client token silently overwrites the first client's pinned pair before throwing on the id mismatch. Traced this structurally across all three commits: it predates this PR and isn't something to fix here, just flagging since it sits right next to code this PR modifies.

---

Given this PR's own history so far (round 2 closed round 1's gap and introduced a new one; this round closes round 2's gaps and reopens the original bug for a different input shape), it might be worth a dedicated pass specifically hunting for "which existing truthy or falsy check disagrees with this one" before the next round, rather than another round of point fixes. Happy to help however's useful.

[mgallego-keeper]

Round 4 review

Thank you for the fixes in commit bf8c130. This commit fixes the two problems named in its title. It does not fix the pattern behind them. The same kind of bug appears again, in new and more serious forms. I confirmed these through direct testing on this commit, not just by reading the diff.

Critical: a failed or delayed write between the id and the key can make the SDK silently switch to a Keeper bundled key

persistServerPublicKeyOptions (lines 887 to 908) writes the id first, then the key, as two separate storage calls. The new comment says a failure between the two writes is safe, because generateTransmissionKey will fail loud for the surviving id. That is only true when the surviving id falls outside the bundled table of ids 7 to 18.

When the surviving id is inside that table, for example '8' or '10', generateTransmissionKey (lines 764 to 784) finds a real entry for that id. It returns Keeper's own bundled key for that id, with no error. The wire format looks correct, because it really did use that bundled key. The caller's real pinned key is never used again, and nothing says so.

I confirmed four separate ways to trigger this on the current commit:

1. A storage write failure between the two writes, on a fresh pin. The id write succeeds, the key write throws. The next call, even one that supplies no key or id at all (the SDK's own intended "pin once" pattern), silently uses the bundled key for the surviving id.
2. The same failure, but on a rebind. Storage already holds a real key and id from before. A caller rebinds to a new key and a new id. The new id write succeeds, the new key write fails. The OLD key stays in storage, now paired with the NEW id. generateTransmissionKey finds the old key still present and uses it, while reporting the new id.
3. Pure concurrency, with no storage fault at all. Two calls share the same storage. One call is between its id write and its key write, while pinning a fresh key. A second call reads storage in that exact window and calls generateTransmissionKey. It gets the bundled key for the surviving id. I confirmed this by inspecting the actual key bytes handed to encryption.
4. Two concurrent calls, each pinning a different, complete key and id pair. Their writes can interleave into a third pair that neither caller supplied. getSecrets partly protects itself, because it calls the persist step twice. Callers that call it only once do not: deleteSecret, updateSecret2, deleteFolder, createSecret2, completeTransaction, file upload, and updateFolder.

For IL5 and other government cloud deployments, using only the customer's own key is a hard requirement. This bug can silently break that requirement through an ordinary concurrent request pattern, not just a rare fault.

Critical: two more falsy inputs reopen the "re-saved on every call" bug this commit fixes for empty strings

The new normalization only checks serverPublicKey === ''. It misses two other input shapes that hit the same underlying problem:

• serverPublicKey: null. Three calls with {serverPublicKey: null, serverPublicKeyId: '8'} produce six writes of each field, not one. null is at least as likely as '' as a "no value" input, from a parsed JSON config or a database default.
• A non-canonical numeric id string. parseServerKeyId accepts '007' and stores it as the literal string '007'. A later call that supplies the canonical form, '7', for the same logical id, fails the no-op check at line 881, because it compares the two strings directly. Every such call rewrites both fields again. The same gap applies to hex, exponential, and padded forms, since parseServerKeyId still uses a bare Number() coercion (line 935) rather than a stricter check.

High: an empty string already sitting in a cloud-backed config can permanently block a real key from ever being written

AWSKeyValueStorage.getString and AzureKeyValueStorage.get both return the stored value directly, with no filtering (AwsKeyValueStore.ts lines 43 to 45 and 448 to 450, AzureKeyValueStorage.ts lines 295 to 298). Neither backend collapses a stored empty string to undefined, unlike the in-memory storage this file's own tests use.

If serverPublicKey is ever set to '' in one of these backends, by a hand-edited config, a template default, or any future code path that writes it unconditionally, the stored value reads back as ''. That is !== undefined, so persistServerPublicKeyOnce treats the field as already set. It never writes a real key again for that config, with no error anywhere. The only recovery is a manual edit of the stored config.

The same read feeds hasCustomKey (line 905). A stray stored '' there means the id-only branch believes a real custom key already exists, which lets an out-of-table serverPublicKeyId skip the bundled-table check that is supposed to apply when there is no real key.

I confirmed the read behavior directly on the AWS and Azure backends.

Medium: the conflict with PR 1143 is worse than last round, and has two separate layers

I redid the trial merge against PR 1143's current commit, 39ec0c7, which has not moved since my last review of that PR. Last round, the conflict was a silent duplicate declaration, only visible at tsc --noEmit time. This round, git itself flags real content conflicts at the two call sites in postQuery and fetchAndDecryptSecrets, because both PRs rewrote the same original line into a call to a same-named helper with a different shape.

There is a second layer underneath. After resolving only the two conflicts git shows, tsc --noEmit still fails, with four separate duplicate-declaration errors for parseServerKeyId and persistServerPublicKeyOptions. Whoever merges these two branches needs to know about both layers, not just the one git marks.

Low: serverPublicKeyId: '' now throws, where the old code silently ignored it

This looks like a deliberate choice, given the new comment explaining why the id is not normalized the same way as the key. It is still an undocumented behavior change from the code before this series of fixes, worth a line in the changelog.

Low: the new IL5 error-wrapping loses the original error's type

initializeStorage's IL5 branch (line 1268) now wraps any error from persistServerPublicKeyOptions in a plain new Error, reading e.message directly. This drops instanceof KeeperError for anything checking it downstream, and produces the literal message "IL5 token: undefined" if the underlying throw has no .message property. The file already has an errorMessage() helper built for exactly this case, used elsewhere in the file but not here.

Low priority, code quality only

• persistServerPublicKeyOnce and persistServerPublicKeyIdOnce duplicate the same four-step shape. A future change to one, like this round's empty-string handling, has to be remembered and applied to the other by hand.
• persistServerPublicKeyIdOnce takes a hasCustomKey boolean parameter that its only caller must remember to compute from storage, not from that call's own options. The function needing a warning comment to prevent this mistake is a sign the parameter should not exist. Reading storage directly inside the function would remove the risk.
• The id-only branch reads two independent storage keys one after another (lines 905 and 826), where the pair branch a few lines above already uses Promise.all for the same kind of read.
• fetchAndDecryptSecrets calls persistServerPublicKeyOptions directly, then postQuery calls it again three lines later with the same arguments, on every single call. This was already known and intentional from round 1. On the browser's IndexedDB-backed storage, each of these calls is a full open, transaction, and decrypt cycle, so the real cost is higher than it looks from the source alone.
• postQuery's rotation branch (line 972) still writes serverPublicKeyId directly, bypassing the shared dispatcher. The new comment says the resulting mismatch always fails loud, through the "Server rejected the custom server public key" guard. That guard only fires for one specific server error shape. A generic server failure for the same mismatch would surface as a generic error instead of that diagnostic.

FYI, pre-existing, not introduced by this round

• A 3-segment IL5 token silently drops the key id, with no error. The length check only rejects tokens with more than 4 segments, or handles exactly 4 (lines 1256 to 1259). Exactly 3 segments matches neither case, even though the error message's own wording says "expected 2 or 4." This traces to an older, unrelated commit.
• Last round I flagged that initializeStorage's IL5 branch persists before its existingClientId check, so a second call with a conflicting token overwrites the first pin before throwing. This round shows a worse case of the same issue: if the second call's token happens to derive the SAME client id, the check passes with no throw at all, and the key is replaced with zero error anywhere.

What round 4 did fix

The concurrency test that could not detect its own regression last round is now genuinely fixed. Deleting the write-once guard it protects correctly makes the test fail. The six new tests in this commit are well built for what they claim to test; a dedicated pass tried twelve different broken versions of the code against them, and all twelve were caught. hasCustomKey reading from storage, the always-on format check, and the atomic-pair dispatcher from round 3 are all still correctly in place.

Recommendation

The root cause across every round so far is the same: a check of !== undefined, or a truthy check, on one of these two fields, that does not agree with what a specific storage backend actually does with a specific input. This round confirms that mismatch in both directions, on three different backend families, plus a pure concurrency trigger that needs no fault at all. Another point fix at the specific spot each new report names is unlikely to close this for good. A single audit of every place either field is read or written, checked against what every shipped storage backend actually returns for '', null, and non-canonical numeric strings, would likely close all of the critical and high items above at once.