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

[stas-schaller]

Summary

JavaScript SDK: replaces the Node caching fallback (cachingPostFunction) with a version that doesn't leak its transmission key or trust an unauthenticated cache file.

Changes

Fixed

• Security fix (CWE-312, CWE-345): cachingPostFunction stored its AES transmission key in plaintext beside the ciphertext it protected, in a path relative to the process's working directory, and restored it with no integrity check. Replaced it with createCachingFunction(storage, cachePath?, maxCacheAgeMs?), matching the factory shape already used on the browser platform: the cache is now encrypted with a key derived from the app key already held in the config, authenticated so a tampered file is rejected instead of trusted, bounded by a configurable freshness window (default 24h), and located at ~/.keeper/ksm-cache.dat by default instead of the working directory. (KSM-1265)

Testing

cd sdk/javascript/packages/core && npm test -- test/localConfigStorage.test.ts

Security Impact

Closes CWE-312 (cleartext storage of the transmission key) and CWE-345 (accepting a cache with no integrity check) in the opt-in cachingPostFunction reference implementation. The default request path, with no custom queryFunction configured, was never affected. The cache-encryption key is derived from the app key already present in the config, so reading the cache requires holding the config, not just the cache file, and the cache is authenticated (AES-256-GCM) so a modified file is rejected rather than served.

Breaking Changes

cachingPostFunction has been removed and replaced with createCachingFunction(storage, cachePath?, maxCacheAgeMs?), which returns the actual queryFunction rather than being one itself. Migration: replace queryFunction: cachingPostFunction with queryFunction: createCachingFunction(storage). The cache file format is also new (encrypted and versioned) and isn't compatible with a pre-existing plaintext cache.dat; delete any old cache file after upgrading. The only caller in this repo (examples/javascript/custom-caching-function-support) has been updated.

Related Issues

• Jira: KSM-1265

[socket-security]

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	npm/​@​keeper-security/​secrets-manager-core@​17.6.0	N/A	N/A	N/A	N/A	N/A

View full report

[mgallego-keeper]

Review summary

This is a real improvement over the previous plaintext-key-beside-ciphertext design, and the core idea (derive a dedicated cache key from the app key, authenticate with AES-256-GCM, bound by a staleness window) is the right shape. I found one gap in the new crypto design that undercuts its own headline claim, plus a breaking-change/versioning concern and several test gaps. Ranked by severity below.

Security findings

1. Freshness timestamp is unauthenticated; the staleness check can be bypassed (Medium-High)

In writeCacheFile/readCacheFile, the cache file layout is [9-byte header][AES-256-GCM ciphertext], where the header (1 version byte + 8-byte timestamp) is written in cleartext, outside the AEAD boundary (Buffer.concat([header, encrypted]), and only encrypted goes through platform.encryptWithKey). nodePlatform.ts's _encrypt/_decrypt take no AAD parameter, so there is no mechanism, even in principle, to bind the header to the ciphertext.

Verified with a proof of concept: flipping only the 8 timestamp bytes to a date far in the future, leaving the ciphertext untouched, causes a cache entry that is genuinely stale (past maxCacheAgeMs) to be served as fresh, statusCode 200, with the original plaintext intact and no error. As a sanity check, flipping a byte inside the ciphertext still correctly throws (failed integrity check), confirming the ciphertext itself is properly authenticated; only the freshness metadata is not.

This requires local write access to the cache file, the same trust boundary the fix already defends against via the 0600 permission. Given that prerequisite, an attacker can pin an old or rotated set of secrets as "fresh" indefinitely by rewriting 8 unauthenticated bytes, with no need for the cache key at all. Suggested fix: bind the header into the AEAD (pass it as GCM associated data, or prepend it to the plaintext before encryption) rather than storing it out of band.

2. Cache directory permissions are not re-asserted (Low, inconsistent with the file-level fix in this same function)

writeCacheFile creates ~/.keeper via fs.mkdirSync(dir, {recursive: true, mode: 0o700}). Like openSync's mode argument, mkdirSync's mode is only honored at creation time; confirmed this empirically (pre-create a directory at 0755, call this exact mkdirSync, the mode stays 0755). This is the same bug class KSM-1263 fixes for files via chmodSecure re-assertion, but it isn't applied to the directory here. Impact is limited since the cache file itself is independently chmod'd to 0600 right after, so contents stay protected, but a loose directory can still leak the file's existence, size, and mtime to other local users.

3. No symlink protection on the cache file path (Low-Medium, same threat actor as #2)

writeCacheFile/readCacheFile open by path with plain 'w', no O_NOFOLLOW and no lstat pre-check. If an attacker with write access to ~/.keeper (same access level as #2) plants a symlink at ksm-cache.dat pointing elsewhere, this code will open, truncate, write, and chmod 0600 whatever that symlink points to, an arbitrary-file-overwrite primitive rather than just cache poisoning.

4. A failed cache write aborts an otherwise-successful call (Low-Medium, design tradeoff)

In createCachingFunction, the cache write on a successful response (if (response.statusCode == 200) { ...; await writeCacheFile(...) }) is not wrapped in try/catch. If ~/.keeper is unwritable (disk full, permission race, read-only filesystem), a request that already got a valid 200 from the server still throws. The new test "a write failure after a successful response propagates instead of being treated as a fallback trigger" documents this as intentional, but a best-effort cache write probably shouldn't be able to fail an already-successful primary operation.

Breaking change shipped in a minor version bump

cachingPostFunction is removed entirely (not just re-signatured) going from 17.5.0 to 17.6.0, a minor bump. The prior breaking change in this changelog, KSM-574 ("Replace Node.js Buffer with Browser-Compatible Alternative"), shipped as 16.6.3 to 17.0.0, a major bump. A consumer on ^17.5.0 will silently pull this breaking change on their next install.

Browser platform left with the same vulnerability

This PR doesn't touch src/browser/localConfigStorage.ts's createCachingFunction, which still has the pre-fix pattern: raw transmissionKey.key concatenated with response bytes, no dedicated encryption, no integrity check, no staleness bound. The comment claiming the new Node function "match[es] the factory shape already used on the browser platform" is only true about the closure shape, not the security properties, worth a follow-up ticket so it doesn't read as "browser is covered too." Separately, package.json's types field always points at dist/node/index.d.ts regardless of which bundle a consumer's browser field resolves to; a browser consumer's TypeScript would type-check createCachingFunction(storage, cachePath, maxCacheAgeMs) fine, but at runtime get the 1-arg browser implementation that silently ignores the extra arguments.

Not blocking this PR, but worth a heads-up: the same caching pattern (plaintext key beside ciphertext, CWD/env-relative path, no integrity check) exists unfixed in the Java/Kotlin, Python, .NET, and Ruby SDKs in this monorepo.

Test coverage gaps

• No test asserts that writeCacheFile re-chmods a pre-existing, loosely-permissioned cache file to 0600 (the config-file equivalent is tested in the KSM-1263 PR, but the cache-file side of that same claim isn't covered here).
• Every test passes an explicit cachePath inside an already-created temp directory. The real default path (~/.keeper/ksm-cache.dat) and the fs.mkdirSync first-run/directory-creation behavior, including the permission gap in #2, are never exercised.
• No test for either "no appKey in storage" branch: silently skipping the cache write on success, or throwing Cached value does not exist on the fallback path when a cache file exists but there's no app key yet.
• No test pins the default maxCacheAgeMs (24h) value itself; staleness is only tested with an explicit small value.

Minor

• KEY_APP_KEY = 'appKey' is a hand-duplicated copy of a private constant in keeper.ts (currently correct), with nothing but a comment guarding against drift.
• No migration note that old cache.dat files at the pre-fix CWD-relative path are orphaned after upgrading (harmless since the new code safely rejects old-format files, but the stale plaintext key isn't cleaned up).