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

sdk/javascript/packages/core/CHANGELOG.md

@@ -11,6 +11,7 @@
 - KSM-1332 - Fixed the browser IndexedDB storage hanging forever on a storage failure. `localConfigStorage` and `secureStorage` wired only `onsuccess`, so a failed IndexedDB open, read, write or delete left the promise pending with no error, no rejection and no timeout. All eight wrappers now reject with a typed `KeeperError`, and the blocked-upgrade and missing-object-store paths reject too instead of hanging.
 - KSM-1263 - Fixed config and cache file permissions not being re-applied on every write. `fs.openSync`'s mode argument only takes effect when a file is created, so a config or cache file that already existed with looser permissions kept them; permissions are now explicitly reset to 0600 after every write.
 - KSM-1267 - `getFolders()` now classifies why an undecryptable folder was skipped (`integrity`, `format`, `missing-key`, or `malformed-data`) instead of logging an opaque, unclassified error, and logs one summary line naming every folder UID it had to omit. Added an optional `onDecryptionError` callback to `SecretManagerOptions`, invoked once per skipped folder, so a caller can react to or throw to fail closed on a partial result; existing callers that do not set it see no behavior change. Both the Node and browser platforms' `unwrap()` now reject an unwrapped key of the wrong length immediately (a corrupted-but-plausible 16- or 24-byte result was previously accepted by both platforms and cached, failing later at an unrelated call site with a much harder to diagnose error). The underlying finding (the shared-folder key wrap uses unauthenticated AES-256-CBC, a format fixed server-side that the SDK cannot change unilaterally) was reviewed and confirmed low-impact: a manipulated folder key is still caught by the existing AES-GCM authentication on the record keys inside that folder.
+- KSM-1256 - Fixed a stored `serverPublicKeyId` outside the bundled key table (ids 7-18) permanently blocking all requests. This could happen on a config touched by an SDK build predating KSM-1128's validation. The SDK now falls back to the default key (7) instead of throwing, so the request goes out and the server's rotation hint can steer the client back to a valid id. The pinned custom-key path is unaffected: an out-of-table id there is legitimate and is not reset.
 - Maintenance: Updated `minimatch`, `@babel/core`, and `handlebars` dev dependencies.
 
 ## 17.5.0

sdk/javascript/packages/core/src/keeper.ts

@@ -23,6 +23,10 @@ const MAX_THROTTLE_RETRIES = 5
 // pinned): one legitimate rotation should resolve it, so this only needs to tolerate a little
 // slack, not act as a real retry budget.
 const MAX_KEY_ROTATION_RETRIES = 3
+// Same value as the bundled key table's starting index in initialize() below by design (the
+// server defaults new configs to key 7), but a distinct concept - kept as a separate constant
+// rather than coupling the two.
+const DEFAULT_KEY_NUMBER = 7
 const BASE_THROTTLE_DELAY_SEC = 11 // 1s safety margin over the backend's 10s memcached TTL
 const MAX_THROTTLE_DELAY_SEC = 176 // same ceiling the exponential branch reaches at the last retry (11 * 2**4)
 const CLIENT_ID_HASH_TAG = 'KEEPER_SECRETS_MANAGER_CLIENT_ID' // Tag for hashing the client key to client id
@@ -764,20 +768,28 @@ export const postFunction = async (url: string, transmissionKey: TransmissionKey
 export const generateTransmissionKey = async (storage: KeyValueStorage): Promise<TransmissionKey> => {
     const transmissionKey = platform.getRandomBytes(32)
     const keyNumberString = await storage.getString(KEY_SERVER_PUBLIC_KEY_ID)
-    const keyNumber = keyNumberString ? Number(keyNumberString) : 7
     const customPublicKeyB64 = await storage.getString(KEY_SERVER_PUBLIC_KEY)
     if (customPublicKeyB64) {
+        const keyNumber = keyNumberString ? Number(keyNumberString) : DEFAULT_KEY_NUMBER
+        if (!Number.isInteger(keyNumber) || keyNumber <= 0) {
+            throw new KeeperError(`Stored serverPublicKeyId '${keyNumberString}' is not a positive integer`)
+        }
         const customPublicKey = webSafe64ToBytes(customPublicKeyB64)
         const encryptedKey = await platform.publicEncrypt(transmissionKey, customPublicKey)
         return { publicKeyId: keyNumber, key: transmissionKey, encryptedKey }
     }
-    const keeperPublicKey = keeperPublicKeys[keyNumber]
-    if (!keeperPublicKey) {
-        throw new Error(`Key number ${keyNumber} is not supported`)
-    }
-    const encryptedKey = await platform.publicEncrypt(transmissionKey, keeperPublicKeys[keyNumber])
+    const keyNumber = keyNumberString ? Number(keyNumberString) : DEFAULT_KEY_NUMBER
+    // A stored id outside the bundled table (e.g. an older SDK build persisted an unvalidated
+    // server-suggested key) must not permanently block every request. Fall back to the default
+    // key for this call instead of throwing, so the request still goes out and the server's
+    // rotation hint can steer a future call back to a valid id. This fallback is never persisted:
+    // generateTransmissionKey is a read, and writing here can race a concurrent custom-key pin or
+    // a concurrent rotation write, and can stomp a newer SDK build's valid-but-unrecognized id
+    // back to the default on a config shared across mixed-version instances.
+    const effectiveKeyNumber = keyNumber in keeperPublicKeys ? keyNumber : DEFAULT_KEY_NUMBER
+    const encryptedKey = await platform.publicEncrypt(transmissionKey, keeperPublicKeys[effectiveKeyNumber])
     return {
-        publicKeyId: keyNumber,
+        publicKeyId: effectiveKeyNumber,
         key: transmissionKey,
         encryptedKey: encryptedKey
     }
@@ -792,13 +804,57 @@ const encryptAndSignPayload = async (storage: KeyValueStorage, transmissionKey:
     const signature = await platform.sign(signatureBase, KEY_PRIVATE_KEY, storage)
     return {payload: encryptedPayload, signature}
 }
-const postQuery = async (options: SecretManagerOptions, path: string, payload: AnyPayload): Promise<Uint8Array> => {
-    if (options.serverPublicKey) {
-        await options.storage.saveString(KEY_SERVER_PUBLIC_KEY, options.serverPublicKey)
-    }
-    if (options.serverPublicKeyId) {
-        await options.storage.saveString(KEY_SERVER_PUBLIC_KEY_ID, options.serverPublicKeyId)
+// Shared by every call site below that validates a caller-supplied serverPublicKeyId string. The
+// postQuery rotation branch further down validates an already-parsed JSON number instead, a
+// different input contract, so it stays separate rather than routing through this helper.
+const parseServerKeyId = (serverPublicKeyId: string): number => {
+    const parsedKeyId = Number(serverPublicKeyId)
+    if (!Number.isInteger(parsedKeyId) || parsedKeyId <= 0) {
+        throw new KeeperError(`serverPublicKeyId '${serverPublicKeyId}' must be a positive integer`)
+    }
+    return parsedKeyId
+}
+
+// Two call paths persist a caller-supplied serverPublicKey/serverPublicKeyId pair before a request
+// goes out: postQuery (every endpoint) and fetchAndDecryptSecrets, which persists ahead of
+// prepareGetPayload so IL5 custom-key config survives even if that call fails for an unrelated
+// reason (e.g. a missing client id). Both must apply the same validation, so it lives here once.
+const persistServerPublicKeyOptions = async (storage: KeyValueStorage, options: SecretManagerOptions): Promise<void> => {
+    if (options.serverPublicKey !== undefined) {
+        if (options.serverPublicKeyId !== undefined) {
+            // Format only, no table-membership check: an IL5-style id outside the bundled table
+            // is legitimate here precisely because a custom key is being pinned alongside it.
+            parseServerKeyId(options.serverPublicKeyId)
+        }
+        const [storedKey, storedKeyId] = await Promise.all([
+            storage.getString(KEY_SERVER_PUBLIC_KEY),
+            storage.getString(KEY_SERVER_PUBLIC_KEY_ID)
+        ])
+        if (storedKey !== options.serverPublicKey) {
+            await storage.saveString(KEY_SERVER_PUBLIC_KEY, options.serverPublicKey)
+        }
+        if (options.serverPublicKeyId !== undefined && storedKeyId !== options.serverPublicKeyId) {
+            await storage.saveString(KEY_SERVER_PUBLIC_KEY_ID, options.serverPublicKeyId)
+        }
+    } else if (options.serverPublicKeyId !== undefined) {
+        const parsedKeyId = parseServerKeyId(options.serverPublicKeyId)
+        // hasCustomKey must reflect storage, not this call's options - a key pinned on an earlier
+        // call still makes an out-of-table id legitimate on this one, even when a later call omits
+        // the key and re-sends only the id.
+        const hasCustomKey = (await storage.getString(KEY_SERVER_PUBLIC_KEY)) !== undefined
+        if (!hasCustomKey && !(parsedKeyId in keeperPublicKeys)) {
+            const supported = Object.keys(keeperPublicKeys)
+            throw new KeeperError(`serverPublicKeyId '${options.serverPublicKeyId}' is not supported without a matching serverPublicKey; this SDK version supports key ids ${supported[0]}-${supported[supported.length - 1]}`)
+        }
+        const storedKeyId = await storage.getString(KEY_SERVER_PUBLIC_KEY_ID)
+        if (storedKeyId !== options.serverPublicKeyId) {
+            await storage.saveString(KEY_SERVER_PUBLIC_KEY_ID, options.serverPublicKeyId)
+        }
     }
+}
+
+const postQuery = async (options: SecretManagerOptions, path: string, payload: AnyPayload): Promise<Uint8Array> => {
+    await persistServerPublicKeyOptions(options.storage, options)
     const hostName = await options.storage.getString(KEY_HOSTNAME)
     if (!hostName) {
         throw new Error('hostname is missing from the configuration')
@@ -900,12 +956,7 @@ const decryptRecord = async (record: SecretsManagerResponseRecord, storage?: Key
 
 const fetchAndDecryptSecrets = async (options: SecretManagerOptions, queryOptions?: QueryOptions): Promise<{ secrets: KeeperSecrets, justBound: boolean }> => {
     const storage = options.storage
-    if (options.serverPublicKey) {
-        await storage.saveString(KEY_SERVER_PUBLIC_KEY, options.serverPublicKey)
-    }
-    if (options.serverPublicKeyId) {
-        await storage.saveString(KEY_SERVER_PUBLIC_KEY_ID, options.serverPublicKeyId)
-    }
+    await persistServerPublicKeyOptions(storage, options)
     const payload = await prepareGetPayload(storage, queryOptions)
     const responseData = await postQuery(options, 'get_secret', payload)
     const response = JSON.parse(platform.bytesToString(responseData)) as SecretsManagerResponse
@@ -1144,7 +1195,9 @@ export const initializeStorage = async (
             }
             if (tokenParts.length === 4) {
                 const keyId = tokenParts[2]
-                if (!/^\d+$/.test(keyId)) {
+                try {
+                    parseServerKeyId(keyId)
+                } catch {
                     throw new Error(`IL5 token: serverPublicKeyId '${keyId}' must be a positive integer`)
                 }
                 if (tokenParts[3].length < 80) {

sdk/javascript/packages/core/test/keeper.test.ts

@@ -2,6 +2,7 @@ import {
     KeeperHttpResponse,
     getSecrets,
     getFolders,
+    deleteSecret,
     initializeStorage,
     generateTransmissionKey,
     platform,
@@ -268,6 +269,106 @@ test('IL5 dynamic key - Layer 1: generateTransmissionKey uses serverPublicKey fr
     expect(transmissionKey.key.length).toBe(32)
 })
 
+test('generateTransmissionKey falls back to the default key when the stored key id is outside the bundled table', async () => {
+    const storage = inMemoryStorage({
+        serverPublicKeyId: '99'
+    })
+    platform.getRandomBytes = () => new Uint8Array(32)
+    const transmissionKey = await generateTransmissionKey(storage)
+    expect(transmissionKey.publicKeyId).toBe(7)
+    expect(transmissionKey.key.length).toBe(32)
+})
+
+test('generateTransmissionKey never calls saveString when falling back from a corrupted stored id', async () => {
+    const storage = inMemoryStorage({
+        serverPublicKeyId: '9999'
+    })
+    // The fallback is recomputed in memory on every call; it must never overwrite storage, or a
+    // delayed write can clobber a concurrent custom-key pin or a just-learned rotation id.
+    const saveSpy = jest.spyOn(storage, 'saveString')
+    platform.getRandomBytes = () => new Uint8Array(32)
+    await generateTransmissionKey(storage)
+    expect(saveSpy).not.toHaveBeenCalled()
+})
+
+test('generateTransmissionKey throws when a custom key is pinned but the stored key id is not numeric', async () => {
+    const fakeKey = 'BK9w6TZFxE6nFNbMfIpULCup2a8xc6w2tUTABjxny7yFmxW0dAEojwC6j6zb5nTlmb1dAx8nwo3qF7RPYGmloRM'
+    const storage = inMemoryStorage({
+        serverPublicKey: fakeKey,
+        serverPublicKeyId: 'not-a-number'
+    })
+    platform.getRandomBytes = () => new Uint8Array(32)
+    await expect(generateTransmissionKey(storage)).rejects.toThrow(/serverPublicKeyId/)
+})
+
+test('getSecrets rejects a caller-supplied serverPublicKeyId outside the bundled table when no custom key is pinned', async () => {
+    const storage = inMemoryStorage({})
+    await initializeStorage(storage, FAKE_ONE_TIME_TOKEN, 'fake.keepersecurity.com')
+    const options: SecretManagerOptions = {
+        storage,
+        serverPublicKeyId: '9999',
+        queryFunction: async () => { throw new Error('should not reach the network: invalid id must be rejected before the request goes out') }
+    }
+    await expect(getSecrets(options)).rejects.toThrow(/serverPublicKeyId/)
+    expect(await storage.getString('serverPublicKeyId')).toBeUndefined()
+})
+
+test('getSecrets rejects a non-numeric serverPublicKeyId even when paired with a valid custom serverPublicKey, and persists neither', async () => {
+    const fakeKey = 'BK9w6TZFxE6nFNbMfIpULCup2a8xc6w2tUTABjxny7yFmxW0dAEojwC6j6zb5nTlmb1dAx8nwo3qF7RPYGmloRM'
+    const storage = inMemoryStorage({})
+    await initializeStorage(storage, FAKE_ONE_TIME_TOKEN, 'fake.keepersecurity.com')
+    const options: SecretManagerOptions = {
+        storage,
+        serverPublicKey: fakeKey,
+        serverPublicKeyId: 'garbage',
+        queryFunction: async () => { throw new Error('should not reach the network: invalid id must be rejected before the request goes out') }
+    }
+    await expect(getSecrets(options)).rejects.toThrow(/serverPublicKeyId/)
+    expect(await storage.getString('serverPublicKey')).toBeUndefined()
+    expect(await storage.getString('serverPublicKeyId')).toBeUndefined()
+})
+
+test('getSecrets accepts an out-of-table serverPublicKeyId on a later call when a custom key was already pinned by an earlier call', async () => {
+    const fakeKey = 'BK9w6TZFxE6nFNbMfIpULCup2a8xc6w2tUTABjxny7yFmxW0dAEojwC6j6zb5nTlmb1dAx8nwo3qF7RPYGmloRM'
+    const storage = inMemoryStorage({})
+    await initializeStorage(storage, FAKE_ONE_TIME_TOKEN, 'fake.keepersecurity.com')
+
+    const pinOptions: SecretManagerOptions = {
+        storage,
+        serverPublicKey: fakeKey,
+        serverPublicKeyId: '20',
+        queryFunction: async () => { throw new Error('network not needed - only persistence from this call matters') }
+    }
+    await expect(getSecrets(pinOptions)).rejects.toThrow('network not needed - only persistence from this call matters')
+    expect(await storage.getString('serverPublicKey')).toBe(fakeKey)
+    expect(await storage.getString('serverPublicKeyId')).toBe('20')
+
+    // Key omitted this time - re-sending just the id that was already pinned above must not be
+    // treated as a bare out-of-table id with no custom key to justify it. The call still fails
+    // (network stub), but it must fail there, past validation, not at the table-membership check.
+    const idOnlyOptions: SecretManagerOptions = {
+        storage,
+        serverPublicKeyId: '20',
+        queryFunction: async () => { throw new Error('network not needed - only persistence from this call matters') }
+    }
+    await expect(getSecrets(idOnlyOptions)).rejects.toThrow('network not needed - only persistence from this call matters')
+})
+
+test('deleteSecret persists a caller-supplied serverPublicKey/serverPublicKeyId pair on its own, for callers that never go through fetchAndDecryptSecrets first', 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: async () => { throw new Error('network not needed - persistence already happened by this point') }
+    }
+    await expect(deleteSecret(options, ['some-record-uid'])).rejects.toThrow('network not needed - persistence already happened by this point')
+    expect(await storage.getString('serverPublicKey')).toBe(fakeKey)
+    expect(await storage.getString('serverPublicKeyId')).toBe('20')
+})
+
 test('IL5 dynamic key - Layer 2: initializeStorage saves serverPublicKeyId and serverPublicKey from 4-segment IL5 OTT', async () => {
     const fakeKey = 'BK9w6TZFxE6nFNbMfIpULCup2a8xc6w2tUTABjxny7yFmxW0dAEojwC6j6zb5nTlmb1dAx8nwo3qF7RPYGmloRM'
     const storage = inMemoryStorage({})
@@ -441,6 +542,14 @@ test('IL5 dynamic key - Layer 2: rejects non-integer serverPublicKeyId', async (
     ).rejects.toThrow("IL5 token: serverPublicKeyId 'notanumber' must be a positive integer")
 })
 
+test("IL5 dynamic key - Layer 2: rejects a serverPublicKeyId of '0'", async () => {
+    const fakeKey = 'BK9w6TZFxE6nFNbMfIpULCup2a8xc6w2tUTABjxny7yFmxW0dAEojwC6j6zb5nTlmb1dAx8nwo3qF7RPYGmloRM'
+    const storage = inMemoryStorage({})
+    await expect(
+        initializeStorage(storage, `IL5:ONE_TIME_TOKEN:0:${fakeKey}`)
+    ).rejects.toThrow("IL5 token: serverPublicKeyId '0' must be a positive integer")
+})
+
 test('IL5 dynamic key - Layer 2: rejects malformed (too short) serverPublicKey', async () => {
     const storage = inMemoryStorage({})
     await expect(