JavaScript SDK: secure the caching fallback's transmission-key storage (KSM-1265)

examples/javascript/custom-caching-function-support/hello.js

@@ -1,58 +1,22 @@
-const fs = require('fs');
-
 const {
     getSecrets,
     initializeStorage,
     localConfigStorage,
-    postFunction
+    createCachingFunction
 } = require('@keeper-security/secrets-manager-core')
 
-const CACHE_FILENAME = 'cache.dat';
-
-// This is basic example of creating custom caching function
-// ⓘ This will store only last request, however you can use any tool to extend this functionality
-// ⓘ Stale cache entries can cause version mismatches if records are updated from other keepersecurity utils. Prefer fresh reads
-
-const cachingPostFunction = async (url, transmissionKey, payload, allowUnverifiedCertificate) => {
-    try {
-        const response = await postFunction(
-            url,
-            transmissionKey,
-            payload,
-            allowUnverifiedCertificate
-        )
-
-        if (response.statusCode == 200) {
-            fs.writeFileSync(CACHE_FILENAME, Buffer.concat([transmissionKey.key, response.data]))
-        }
-
-        return response
-    } catch (e) {
-        console.error(e)
-        let cachedData
-        try {
-            cachedData = fs.readFileSync(CACHE_FILENAME)
-        } catch {
-        }
-        if (!cachedData) {
-            throw new Error('Cached value does not exist')
-        }
-        console.log('Using cached data')
-        transmissionKey.key = cachedData.slice(0, 32)
-        return {
-            statusCode: 200,
-            data: cachedData.slice(32),
-            headers: []
-        }
-    }
-}
+// This is a basic example of using the SDK's built-in caching function.
+// ⓘ createCachingFunction stores only the last successful request, but you can supply your own
+//   queryFunction to extend this behavior.
+// ⓘ Stale cache entries can cause version mismatches if records are updated from other keepersecurity
+//   utils. createCachingFunction rejects cache entries older than its maxCacheAgeMs (default 24h).
 
 const getKeeperRecords = async () => {
     const storage = localConfigStorage("config.json")
 
     const options = {
         storage,
-        queryFunction: cachingPostFunction
+        queryFunction: createCachingFunction(storage)
     }
 
     // if your Keeper Account is in other region than US, update the hostname accordingly

sdk/javascript/packages/core/CHANGELOG.md

@@ -13,6 +13,16 @@
 - 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-1266 - Fixed `localConfigStorage` treating every config-read failure as "no config yet." A missing file is still a legitimate fresh start, and so now is one left completely empty by a process killed mid-save (a partially-written file is not covered by this - there is no reliable way to distinguish a truncated write from genuine corruption, so it still throws). Permission errors, malformed JSON, invalid UTF-8 byte sequences, and JSON that parses but isn't an object (`null`, a number, an array) now throw a typed `KeeperError` instead of silently starting fresh or misbehaving on first use. A leading UTF-8 BOM (produced by tools like Windows Notepad or PowerShell's `Set-Content`) is stripped and the file is read normally, not treated as corruption. `saveStorage`'s write path now writes to a temporary file and renames it into place atomically, instead of truncating the destination before writing (which could previously leave a 0-byte file on disk after a failed write), and wraps its own failures (e.g. `EACCES`, `ENOSPC`) in the same `KeeperError` guarantee. Node validates config readability eagerly, at construction; the browser `localConfigStorage` (KSM-1332, same release) defers the equivalent check lazily to first storage access, since IndexedDB has no synchronous API to check eagerly against - this timing difference between the two platforms is expected and now documented in-code. The write path now resolves a symlinked config path and writes through the real file. It no longer replaces the symlink. Some deployments manage a "current config" symlink this way. This also covers a symlink whose target does not exist yet, for example a symlink an ops tool creates before the target file exists, including a chain of such symlinks up to 40 hops deep, the same bound the operating system's own resolution enforces. A resolution failure partway through that chain, for example a permissions error on an intermediate link, now fails the save instead of silently writing through the wrong path. A hard-linked config path now goes through the same atomic write as any other file. Only the resolved path gets the update; a second hard-linked name keeps its old content, because the atomic write always creates a new file at the resolved path. Before this fix, a hard-linked config path was written in place, so every hard link saw the update, but that write was not atomic: a failure partway through could corrupt the file with no recovery. An atomic write now needs write and execute permission on the config file's directory, not just the file itself. A directory locked down to file-only write access will fail every save from now on. If a crash happens between opening the temporary file and the rename, the SDK now removes the leftover file automatically on the next read. Before this fix, the file stayed on disk indefinitely. That cleanup resolves the config path the same way the write path does, so it also finds the leftover file when the config path is itself a symlink, including one whose target didn't exist at write time. A failed save no longer leaves the in-memory value ahead of the value on disk. `localConfigStorage` now throws the new `KeeperStorageError`, which extends `KeeperError`. Its `code` field carries the original filesystem error code, for example `EACCES` or `ENOSPC`, when one exists. Concurrent `saveString`/`saveBytes`/`delete` calls on the same `localConfigStorage` instance now run one at a time. Before this fix, two overlapping calls could interleave so that a failed save's rollback erased a different, already-successful call's data.
+- KSM-1265 - **BREAKING (Node only):** Security fix (CWE-312, CWE-345): the Node `cachingPostFunction` stored its AES transmission key in plaintext next to the ciphertext it protected, in a fixed path relative to the process's working directory, and restored it with no integrity check. Replaced it with `createCachingFunction(storage, options?)`:
+  ```
+  // before
+  queryFunction: cachingPostFunction
+  // after
+  queryFunction: createCachingFunction(storage)
+  // with a custom cache path or freshness window
+  queryFunction: createCachingFunction(storage, {cachePath, maxCacheAgeMs})
+  ```
+  The cache is now encrypted with a key derived from the app key already held in the config (so reading the cache requires the config, not just the cache file), authenticated so a tampered or corrupted file is rejected instead of silently trusted, bounded by a configurable freshness window (default 24h), and located at `~/.keeper/ksm-cache.dat` by default instead of the working directory. Usage was limited to the opt-in caching example, which has been updated to use the new function. If you called `cachingPostFunction` directly, delete the old cache file in your working directory after upgrading; it is not removed automatically. `cachePath` and `maxCacheAgeMs` are now named fields on an options object instead of positional arguments, since Node's and the browser's second positional argument meant different things; the browser signature (`createCachingFunction(storage, maxCacheAgeMs?)`) is unchanged and still non-breaking there, since the new `maxCacheAgeMs` parameter is optional and an old-format cached value is simply treated as a cache miss. A symlinked cache directory is rejected outright (throws) on both read and write, on POSIX platforms; Windows has no equivalent `O_NOFOLLOW`/`O_DIRECTORY` protection, so on that platform the cache's protection is the encryption and authentication alone. A symlinked cache file itself is not rejected with an error - the write silently replaces it with a real file instead of following it, and the read still requires the actual content underneath to pass its own integrity check - there is no legitimate externally-managed symlink convention for a path the SDK itself names, unlike the config file's own symlink handling, which is unchanged and intentionally different (see the KSM-1266 entry above). Both the config file and the cache file are now written atomically (to a temporary file, then renamed into place, via one shared primitive), so a write that fails partway through can no longer leave a corrupted or truncated file behind. The cache directory is created at `0700`. On the default path (`~/.keeper`), it's re-hardened to `0700` on every write, matching how the cache file itself already self-heals; on a caller-supplied `cachePath` pointing at a directory that already exists (for example, a file directly inside `$HOME`), its permissions are left alone - the SDK does not narrow permissions on a directory it doesn't own. Known limitation: the very first (bind) call's response is not cached, since caching requires an app key that the bind call itself establishes; every call after that caches normally. In the browser, when the app key is held as a non-extractable `CryptoKey` (`useObjects: true`), caching is a no-op rather than an error, the same graceful degradation already used for a network failure with no prior cache. A network failure served from cache now logs a warning, since the caller is getting a response that may be stale. This package now also declares an `exports` field so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle; a consumer still on TypeScript's legacy `moduleResolution: "node"` continues to see the Node type declarations regardless, unchanged from before.
 - Maintenance: Updated `minimatch`, `@babel/core`, and `handlebars` dev dependencies.
 
 ## 17.5.0

sdk/javascript/packages/core/internal/quicktest.ts

@@ -10,7 +10,7 @@ import {
 import {nodePlatform} from '../src/node/nodePlatform';
 import {connectPlatform} from '../src/platform';
 import {inspect} from 'util';
-import {cachingPostFunction, localConfigStorage} from "../src/node";
+import {createCachingFunction, localConfigStorage} from "../src/node";
 
 process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0'
 
@@ -26,7 +26,7 @@ async function test() {
     await initializeStorage(kvs, oneTimeToken)
     const options: SecretManagerOptions = {
         storage: kvs,
-        // queryFunction: cachingPostFunction
+        // queryFunction: createCachingFunction(kvs)
         allowUnverifiedCertificate: true
     }
     const { records } = await getSecrets(options)

sdk/javascript/packages/core/package.json

@@ -5,6 +5,23 @@
   "browser": "dist/index.es.js",
   "main": "dist/index.cjs.js",
   "types": "dist/node/index.d.ts",
+  "exports": {
+    ".": {
+      "types": {
+        "browser": "./dist/browser/index.d.ts",
+        "default": "./dist/node/index.d.ts"
+      },
+      "node": {
+        "import": "./dist/index.cjs.js",
+        "require": "./dist/index.cjs.js"
+      },
+      "browser": "./dist/index.es.js",
+      "import": "./dist/index.cjs.js",
+      "require": "./dist/index.cjs.js",
+      "default": "./dist/index.cjs.js"
+    },
+    "./package.json": "./package.json"
+  },
   "repository": "https://github.com/Keeper-Security/secrets-manager",
   "author": "sm@keepersecurity.com",
   "license": "MIT",

sdk/javascript/packages/core/src/browser/localConfigStorage.ts

@@ -1,5 +1,8 @@
 import {EncryptedPayload, KeeperHttpResponse, KeyValueStorage, TransmissionKey, platform} from "../platform";
 import {KeeperError} from "../errors";
+import {KEY_APP_KEY, deriveCacheKey, encodeCacheBlob, decodeCacheBlob, DEFAULT_MAX_CACHE_AGE_MS, isRawKeyBytes, concatBytes} from "../cache";
+
+const CACHE_STORAGE_KEY = 'cache'
 
 type Reject = (reason: Error) => void
 
@@ -202,30 +205,64 @@ export const secureStorage = async (dbName: string): Promise<KeyValueStorage> =>
     }
 }
 
-export function createCachingFunction(storage: KeyValueStorage): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload) => Promise<KeeperHttpResponse> {
+// Same cache codec (../cache) as node/localConfigStorage.ts's createCachingFunction; only the
+// storage medium differs (IndexedDB here, a file there). Replaces the old plaintext
+// key-beside-data format (CWE-312, CWE-345) with one encrypted under a key derived from the app
+// key, authenticated, and bounded by a freshness window. An old-format cached value simply fails
+// the version check and is treated as a cache miss, the same graceful degradation the Node fix
+// uses for its old-format files.
+export function createCachingFunction(storage: KeyValueStorage, maxCacheAgeMs: number = DEFAULT_MAX_CACHE_AGE_MS): (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload) => Promise<KeeperHttpResponse> {
 
     return async (url: string, transmissionKey: TransmissionKey, payload: EncryptedPayload): Promise<KeeperHttpResponse> => {
+        let response: KeeperHttpResponse
         try {
-            const response = await platform.post(url, payload.payload, {
+            response = await platform.post(url, payload.payload, {
                 PublicKeyId: transmissionKey.publicKeyId.toString(),
                 TransmissionKey: platform.bytesToBase64(transmissionKey.encryptedKey),
                 Authorization: `Signature ${platform.bytesToBase64(payload.signature)}`
             })
-            if (response.statusCode == 200) {
-                await storage.saveBytes('cache', new Uint8Array([...transmissionKey.key, ...response.data]))
-            }
-            return response
         } catch (e) {
-            const cachedData = await storage.getBytes('cache')
-            if (!cachedData) {
-                throw new Error('Cached value does not exist')
+            const appKey = await storage.getBytes(KEY_APP_KEY)
+            if (!appKey || !isRawKeyBytes(appKey)) {
+                throw new KeeperError('Cached value does not exist')
+            }
+            const raw = await storage.getBytes(CACHE_STORAGE_KEY)
+            if (!raw) {
+                throw new KeeperError('Cached value does not exist')
+            }
+            let cachedData: Uint8Array
+            try {
+                cachedData = await decodeCacheBlob(raw, await deriveCacheKey(appKey), maxCacheAgeMs)
+            } catch (e2: Error | any) {
+                throw new KeeperError(`Cached value is invalid: ${e2.message}`)
             }
+            console.error(`Network request failed (${describeCause(e)}); serving cached response, which may be stale`)
             transmissionKey.key = cachedData.slice(0, 32)
             return {
                 statusCode: 200,
                 data: cachedData.slice(32),
                 headers: []
             }
         }
+        if (response.statusCode == 200) {
+            try {
+                const appKey = await storage.getBytes(KEY_APP_KEY)
+                if (appKey && isRawKeyBytes(appKey)) {
+                    const blob = await encodeCacheBlob(concatBytes(transmissionKey.key, response.data), await deriveCacheKey(appKey))
+                    await storage.saveBytes(CACHE_STORAGE_KEY, blob)
+                } else if (appKey) {
+                    // appKey exists but isn't raw bytes - useObjects: true wraps it as a
+                    // non-extractable CryptoKey, so caching is a deliberate no-op here (matches
+                    // the identical guard in the fallback branch above), not a failure. Logged
+                    // once per call, same as the fallback branch's own log a few lines up, so a
+                    // caller who opted into useObjects: true has some signal that caching isn't
+                    // doing anything for them before their first real outage.
+                    console.error('Caching is a no-op with useObjects: true - the app key is not available as raw bytes')
+                }
+            } catch (e) {
+                console.error(`Failed to update cached response: ${describeCause(e)}`)
+            }
+        }
+        return response
     }
 }