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

[stas-schaller]

Summary

JavaScript SDK: replaces the Node caching fallback (cachingPostFunction), which leaked its transmission key in plaintext and trusted an unauthenticated cache file, with an encrypted, integrity-checked version. Hardens both the config file and the new cache file's writes against symlink and hard-link attacks, sharing one atomic-write primitive between them, and closes a set of directory-handling and error-propagation gaps in the cache path.

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, options?) on Node and createCachingFunction(storage, maxCacheAgeMs?) on browser: the cache is 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, and bounded by a configurable freshness window (default 24h, resistant to backward clock skew). The Node default cache location is ~/.keeper/ksm-cache.dat instead of the working directory. (KSM-1265)
• The config file and the cache file now share one atomic-write primitive (writeFileAtomic): write to a temp file in the same directory, fsync, then rename into place, so a write that fails partway through can never leave a corrupted or truncated file behind. A hard-linked destination is written in place instead (trading atomicity for that one file, so every hard link still sees the update).
• The config file's write path resolves a symlinked configName and writes through the real file, so an externally-managed "current config" symlink convention keeps working. The cache file's write path does the opposite on purpose: it never resolves a symlink at the cache path, since there's no legitimate externally-managed symlink convention for a path the SDK itself names and owns. A symlink planted there is replaced outright by the write, never written through. The cache directory and file also reject a symlink on read.
• 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.
• A stale orphaned temp file left behind by an interrupted cache write is swept on the next read or write of that same cache path, same as the config file. The cache file read path is bounded to a maximum expected size before any decode is attempted, so a misconfigured or maliciously-placed oversized file can't force an unbounded allocation.
• A storage failure while reading the app key - either while serving a stale cache after a network failure, or while writing a fresh cache after a successful response - is treated the same as "no usable app key," not a reason to propagate an unrelated exception in place of the real error. Both call sites guard against a non-raw-bytes app key value the same way, so an unusual KeyValueStorage implementation fails closed (skips caching) instead of throwing a confusing internal error.
• cachePath and maxCacheAgeMs are named fields on an options object on Node, rather than positional arguments, since the second positional argument means something different on the browser signature.
• Byte concatenation for the cached blob on both platforms uses a direct copy instead of a spread-into-array, avoiding a large constant-factor slowdown on big payloads. When caching is a no-op because the app key isn't available as raw bytes (browser's useObjects: true mode), that's now logged, matching the existing log on the stale-cache-served path.

Testing

cd sdk/javascript/packages/core
npm test

Key scenarios covered:

• Round-trip: a successful response is cached, then served from cache when the network fails; a tampered, stale, old-format, or oversized cache file is rejected rather than trusted; a cache entry written during backward clock skew is still treated as stale.
• A symlink at the cache path (file or directory) is replaced by the write, never followed - the symlink's target is confirmed untouched.
• A symlinked config path is written through, preserving the symlink; a hard-linked config path updates every link; a symlink swapped in mid-write, or a hard link swapped into a symlink after the link check passes, is confirmed not to redirect the write.
• A write failure at any point (before or after the temp file is opened, before or after the rename) leaves the previous file intact and throws instead of silently corrupting it.
• A cache directory this call creates is hardened to 0700; a pre-existing directory a caller points cachePath at keeps its own permissions.
• A storage failure serving a stale cache, or writing a fresh one, never breaks an already-successful call or a legitimate fallback.

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

Breaking Changes

cachingPostFunction has been removed and replaced with createCachingFunction(storage, options?), which returns the actual queryFunction rather than being one itself. Migration: replace queryFunction: cachingPostFunction with queryFunction: createCachingFunction(storage). The cache file format is 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. Saving the config file now needs write and execute permission on its directory, not just the file itself (inherent to atomic writes via rename).

Related Issues

• Jira: KSM-1265

[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.

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).

[stas-schaller]

@mgallego-keeper Pushed fixes for the rest of this review.

#1 (unauthenticated timestamp) was already fixed in da82a63a, after your review ran against c0d9bace — the timestamp now sits inside the AEAD plaintext, so GCM covers it.

This push:

• Re-chmods ~/.keeper to 0700 on every write, not just creation (initial python sdk commit #2)
• Refuses to follow a symlink at the cache path, on read and write (Fixed client version #3)
• Swallows a failed cache write after a successful response instead of throwing (generated test data to run unit tests against #4)
• Applies the same fix to browser's createCachingFunction, which had the same plaintext-key issue — non-breaking there; shared crypto logic now lives in src/cache.ts
• Reworded the parity comment, rewrote the CHANGELOG breaking-change note, added a migration note for old cache files

[mgallego-keeper]

Follow-up review

Re-checked today's commit against every point in my original review, plus a fresh pass over the diff. Grouped below by status.

Fixed

• Freshness timestamp now authenticated (was #1, Medium-High): the timestamp moved inside the AEAD boundary in encodeCacheBlob/decodeCacheBlob, and the new "forged freshness timestamp is rejected" test confirms it. Good fix.
• Old cache file migration: now documented in the CHANGELOG ("delete the old cache file... it is not removed automatically").

Fixed, but the fix itself has a gap

1. Directory chmod re-assertion is right in principle, but chmods the wrong thing for a relative path (new, Medium)
writeCacheFile now does fs.chmodSync(path.dirname(cachePath), 0o700) unconditionally. Fine for the default path, but path.dirname('cache.dat') is '.', and the pre-fix example hardcoded exactly that bare filename. Any caller who passes a relative cachePath to keep a similar location on disk gets their current working directory silently chmod'd to 0700, with nothing in the docs warning that cachePath has that side effect.

2. Symlink defense (was #3, Low-Medium) narrows the hole rather than closing it
rejectSymlink only lstats the leaf cachePath. It never checks path.dirname(cachePath), so a symlinked ~/.keeper directory is never caught before mkdirSync/chmodSync operate on whatever it resolves to. Separately, the lstat and the later openSync/readFileSync are two independent syscalls (with an awaited encodeCacheBlob(...) between them on the write side), so it's a check-then-use race rather than a real guarantee. Not asking for O_NOFOLLOW here, just flagging that this is narrower than it looks.

3. Cache-write failure isolation (was #4, Low-Medium) doesn't cover the appKey lookup beside it
writeCacheFile itself is correctly wrapped in try/catch now. But storage.getBytes(KEY_APP_KEY) right before it (node/localConfigStorage.ts:150) is not. If that throws after a successful response, on Node the exception propagates and kills an already-successful call, exactly the failure mode this point was meant to prevent. On browser it's worse: the equivalent lookup (browser/localConfigStorage.ts:224) sits inside the outer try that also wraps platform.post, so a throw there is misrouted into the cache-fallback branch and silently returns stale cached data (or throws "Cached value does not exist") instead of the fresh response that already arrived.

4. Browser fix (my "left with the same vulnerability" comment) breaks under useObjects=true storage (new, High)
Confirmed against browserPlatform.ts:255-258: whenever storage.saveObject exists, unwrap stores appKey as a non-extractable CryptoKey, not raw bytes. createCachingFunction's getBytes(KEY_APP_KEY) returns that CryptoKey unchanged (it only special-cases string values), and deriveCacheKey forwards it into crypto.subtle.importKey('raw', ...), which throws. The write is swallowed by the inner catch, and every fallback read then reports "Cached value is invalid." Caching is a silent no-op for this supported storage mode, and it's untested: browserLocalConfigStorage.test.ts never references useObjects, saveObject, or CryptoKey.

Still open from my original review

• types field: still points at the Node-shaped .d.ts for both bundles. This is more dangerous now than when I first flagged it, since browser's createCachingFunction has a real second parameter now, in a different position than Node's (storage, maxCacheAgeMs vs storage, cachePath, maxCacheAgeMs). A TypeScript consumer bundling for browser can pass a value intending it for maxCacheAgeMs and have it silently land on the wrong parameter or nowhere at all.
• Test coverage: still nothing exercises the real default path (~/.keeper/ksm-cache.dat) or the first-run mkdirSync behavior, and still nothing covers the "no appKey yet" branch. That last one would have caught the next item below.

New issues in today's commit

1. os.homedir() at module load can crash the whole import (High)
const DEFAULT_CACHE_PATH = path.join(os.homedir(), '.keeper', 'ksm-cache.dat') runs at module top level (node/localConfigStorage.ts:8), and node/index.ts re-exports this module unconditionally. In a container running as a UID with no matching /etc/passwd entry and no $HOME, os.homedir() throws, so require('@keeper-security/secrets-manager-core') crashes for every consumer on that platform, whether or not they ever touch caching. Suggest computing this lazily inside the factory's default parameter rather than at module scope.

2. The bind response is never cached, on either platform (High)
keeper.ts only calls platform.unwrap(...) to populate appKey after postQuery (and therefore createCachingFunction) has already returned. So if (appKey) at node/localConfigStorage.ts:151, and the identical check at browser/localConfigStorage.ts:225, is always false on the bind call, and nothing gets written. The old cachingPostFunction cached every 200 response unconditionally, so this is a regression: if the very next call fails offline, the fallback finds nothing cached and throws "Cached value does not exist" where the old code would have served the bind response.

3. writeCacheFile's write is not atomic (Medium)
fs.openSync(cachePath, 'w', ...) truncates before the new blob is written; there's no write-to-temp-then-rename and no check that writeSync wrote every byte. A crash between the truncating open and the write completing destroys a previously good cache instead of leaving it intact, defeating the fallback exactly when a crash or outage makes it most needed.

4. Leftover pre-fix cache file is no longer git-ignored (Low-Medium)
examples/javascript/custom-caching-function-support/.gitignore dropped its cache.dat entry. The CHANGELOG says the old file "is not removed automatically," so anyone who ran the pre-fix example and then does a routine git add . in that directory can now commit a file holding a plaintext transmission key, the exact secret this PR exists to stop leaking.

Not blocking

• Same note as before: the config file (readStorage/saveStorage, same module, untouched by this diff) still has no symlink check at all, unlike the cache file a few lines below it. Worth a follow-up ticket now that the file has two different security postures for a materially similar risk.
• The commit is self-labeled breaking in both the CHANGELOG and its own body text, but the header has no ! and there's no BREAKING CHANGE: footer.
• cache.ts:11's new comment cites KSM-574 by ticket number rather than restating the constraint inline.

[mgallego-keeper]

Deep re-review of this PR at its current head (b38812a1), following up on the round 2 cycle above. Ran the full test suite (107/107 passing) and empirically reproduced the symlink and TOCTOU scenarios below rather than relying on reading the check-then-act code alone.

(Posting as a single review body rather than inline comments: this PR currently shows as conflicting against its base, and the diff GitHub computes for it looked unstable when I checked, so I didn't trust line-anchored comments to land in the right place.)

Requesting changes: the top three findings are regressions or security gaps in this PR's own new hardening logic, not pre-existing issues, and I'd consider the first one a blocker given how common the deployment pattern it breaks is.

Correctness / regressions

sdk/javascript/packages/core/src/node/localConfigStorage.ts:35
rejectSymlink() now throws unconditionally whenever the config file path itself is a symlink. This breaks the standard Kubernetes Secret/ConfigMap volume mount layout, where the mounted file is always a symlink (to ..data/<key>, itself a symlink into a timestamped directory) by design. A pod mounting config.json from a Secret/ConfigMap will get KeeperError('Refusing to follow symlink at ...') on every load, where the SDK previously read the file fine. There is no env var, flag, or allowlist anywhere in this diff to opt out.

sdk/javascript/packages/core/src/node/localConfigStorage.ts:53
saveStorage() still truncates the config file in place (fs.openSync(configName, 'w', ...)) rather than getting the atomic temp-file-then-rename treatment this PR gave writeCacheFile. Combined with readStorage() now throwing KeeperError instead of returning {} on a non-ENOENT read failure, a write that fails partway through permanently bricks the config instead of silently resetting. saveStorage runs on every saveString/saveBytes/delete, so a crash or disk-full event mid-write now needs a human to manually delete the file, where before it silently started fresh.

sdk/javascript/packages/core/src/browser/localConfigStorage.ts:221
createCachingFunction's second positional parameter means something different per platform: cachePath: string on Node, maxCacheAgeMs: number in the browser. Isomorphic code (or a dev porting a Node snippet to a browser bundle) calling createCachingFunction(storage, 60000) will silently bind cachePath = 60000 on Node, throwing a confusing path type error deep inside writeCacheFile/readCacheFile instead of applying the intended cache age override.

Security (TOCTOU)

sdk/javascript/packages/core/src/node/localConfigStorage.ts:52
rejectSymlink(configName) (check) and fs.openSync(configName, 'w', ...) (act, line 53) are separate syscalls with no atomic guard between them. An attacker with write access to the config directory can plant a symlink in the gap right after the check passes, redirecting the write, which contains the plaintext EC private key and app key, to an attacker chosen destination. Confirmed empirically that open('w') follows a symlink planted after an lstat based check passes. Same bug class rejectSymlink was added to close, just not closed all the way here.

sdk/javascript/packages/core/src/node/localConfigStorage.ts:90
Same TOCTOU class, on the cache directory: rejectSymlink(dir) (check) then fs.mkdirSync/fs.chmodSync(dir) (act). Empirically confirmed that mkdirSync(recursive: true) silently accepts an existing symlink to a directory, and chmodSync follows it, so a symlink planted in the gap gets chmod 0700 applied to an attacker chosen real directory instead of throwing, and the cache write lands there.

sdk/javascript/packages/core/src/node/localConfigStorage.ts:120
Same TOCTOU class in readCacheFile: rejectSymlink(dir) then fs.readFileSync(cachePath). Lower impact than the two above: winning this race only lets an attacker feed arbitrary bytes into decodeCacheBlob, which fails the AES-GCM auth tag check and throws. Denial of a cache read, not data exposure, but the same unguarded check-then-act gap.

Other findings

sdk/javascript/packages/core/src/cache.ts:4
KEY_APP_KEY is redeclared here as an unlinked literal copy of keeper.ts's private KEY_APP_KEY constant, kept in sync only by a code comment, not the compiler. If either literal is edited without the other, caching goes cold silently on the success path, but throws KeeperError('Cached value does not exist') on the fallback path, right when the fallback is needed most.

sdk/javascript/packages/core/CHANGELOG.md:13
This entry documents symlink rejection only for the cache file/directory, and never mentions that the identical rejectSymlink check was also added to the main config file's readStorage/saveStorage in this same PR. Given the Kubernetes breaking regression noted above, this should be called out explicitly.

sdk/javascript/packages/core/src/node/localConfigStorage.ts:164
The branch that serves a stale cached response after a network failure logs nothing on either platform, while the adjacent cache write failure branch does log via console.error. Worth logging here too so a caller getting stale data during an outage has some signal it is not fresh.

examples/javascript/custom-caching-function-support/hello.js:15
This example (and hello-secret/hello.js, proxy-support/hello.js) call localConfigStorage() with no try/catch and end in .finally() with no .catch(). readStorage's stricter error handling (throws on non-ENOENT read failures) turns a corrupt or unreadable config.json into an unhandled promise rejection that crashes the process. Lower severity since these scripts already crash for other pre-existing reasons, but worth a .catch() while touching this file.

sdk/javascript/packages/core/test/localConfigStorage.test.ts:223
In the "a relative cachePath with no directory component does not touch the current working directory" test, process.chdir(tmpDir) and fs.chmodSync(tmpDir, 0o755) run before the try/finally that restores the original cwd. If chmodSync throws, process.chdir(originalCwd) never runs, leaving process.cwd() pointing at a directory afterEach is about to delete for every later test in that file. Low likelihood, but easy to fix by moving try up one line.

sdk/javascript/packages/core/src/node/localConfigStorage.ts:41
Cosmetic: KeeperError never sets a .code property, so when rejectSymlink's KeeperError propagates through readStorage's own catch (which checks e.code === 'ENOENT'), it always falls through and gets double-wrapped into a redundant nested message.

Known, already deferred: sdk/javascript/packages/core/src/node/localConfigStorage.ts:179, the very first (bind) response can never be cached on either platform, since the app key is only written to storage after the caching queryFunction has already returned. Not a new finding, just confirming it is still real and still shipping; understood this is deferred to a v18 spike per the CHANGELOG.

Reuse suggestions (non-blocking)

• sdk/javascript/packages/core/src/node/localConfigStorage.ts:156: Node's and browser's createCachingFunction hand-duplicate the identical control-flow skeleton (try network, fall back to appKey-derived decrypt-and-splice on failure, best-effort encrypt-and-write on success), with only the byte-storage I/O actually differing. This PR's own third commit had to independently patch the same bug on both platforms by hand, a sign the duplication already drifts in practice.
• sdk/javascript/packages/core/src/browser/localConfigStorage.ts:233: isRawKeyBytes is hand-checked at two call sites instead of centralized inside cache.ts's deriveCacheKey. This guard was missing once already in round 1 and had to be patched at both sites by hand; a third call site added later is one missed check away from repeating that.

---

Ran the full suite and rebuilt cleanly against b38812a1; none of the correctness/security findings above are caught by the existing tests, since they all require either a race window or an external deployment convention (Kubernetes volume mounts) that nothing in this repo's test suite simulates.

[mgallego-keeper]

Round 4. The rebase and round-3 fix commit (851d5f47) held up well on the previous round's own findings: 10 of 12 are fully fixed, verified empirically this time (real symlinks, real races, real mutation testing against the suite), not just read and trusted. Two small loose ends, neither blocking: the suggested test cwd-restore ordering fix in localConfigStorage.test.ts was not applied, and the KeeperError missing a .code property is now moot rather than fixed (an unrelated part of the same refactor removed the only code path that used to trigger it).

The 18 inline comments below are new: things introduced by 851d5f47's own changes, not caught in rounds 1 through 3. Four of them (marked HIGH) got independent, cross-checked confirmation from multiple angles this round (an empirical attack against the live TOCTOU protections, a primary-source check of Node's own platform constants, and a fresh full-diff pass), which is a stronger signal than usual that they're real rather than a misreading.

Requesting changes given the HIGH items, particularly the Windows gap: the symlink protection this PR adds for the cache file and directory silently does not exist at all on that platform.

Not included in this review, by design: the still-unresolved conflict with PR #1136 over the same functions. Happy to raise that separately once we decide how to sequence the two PRs.

[mgallego-keeper]

HIGH: fs.constants.O_DIRECTORY and O_NOFOLLOW are undefined on Windows (confirmed against Node's own node_constants.cc, libuv's win.h, and the official docs; unlike O_SYNC/O_DIRECT, which do get a Windows fallback, these three have none). O_DIRECTORY | O_NOFOLLOW therefore evaluates to undefined | undefined, which JS coerces to 0, not a crash, just a silent no-op. On win32 this line (and the analogous ones in readCacheFile at lines 178 and 183) becomes a plain fs.openSync(dir, 0): no directory verification, no symlink rejection, and no error to signal it. The package's engines field does not exclude Windows, and test.js.yml only runs ubuntu-latest, so this would ship without any CI signal.

[mgallego-keeper]

HIGH: fs.mkdirSync(dir, {recursive: true}) follows a symlink in any ancestor component of a multi-segment custom cachePath, and the O_NOFOLLOW check a few lines below only ever inspects the final leaf directory, so this needs no race at all. Example: cachePath is /shared/ksm/nested/cache.dat, and an attacker who can write to /shared pre-creates /shared/ksm as a symlink to their own directory before the SDK ever runs. mkdirSync(recursive) creates nested inside the attacker's directory; the leaf itself is not a symlink, so the later open passes cleanly and the whole cache silently relocates.

[mgallego-keeper]

HIGH: this directory check opens with O_DIRECTORY|O_NOFOLLOW then immediately closes the descriptor instead of holding it, so it does not actually pin anything. The real file open at line 183 (and, on the write side in writeCacheFile, the write across the await encodeCacheBlob at lines 166 to 167) re-resolves the path from scratch afterward, leaving a real window to swap the whole directory rather than just a leaf symlink. Confirmed empirically: swapping the directory in that gap on read returns the substituted content silently (bounded by the fact a forged GCM blob still needs the app key, so the realistic impact is a denial of service on the fallback exactly when the network is already down); on write, the encrypted blob lands in the attacker's less-restrictive substitute directory instead of the verified 0700 one.

[mgallego-keeper]

HIGH: this chmodSync runs by path, after the rename, rather than on the file descriptor that is still open a few lines above (closed at line 26). That makes it symlink-followable, unlike the directory hardening 15 lines below in writeCacheFile, which correctly uses fchmodSync on an open fd instead. Confirmed empirically: swapping a symlink into finalPath between the shipped renameSync and this chmodSync forces chmod 0600 onto an unrelated file (644 to 600 observed). Separately, and without needing an attacker at all: if this chmodSync throws for any reason, saveStorage reports the save as failed even though the rename already durably succeeded, misleading the caller about whether their data was persisted. Suggest moving the fchmodSync call to before the rename, on the fd that is already open at line 18, the same pattern used in writeCacheFile; confirmed via mutation testing that dropping this line entirely does not break any of the 162 existing tests, so the fix should be free.

[mgallego-keeper]

MEDIUM: this forces path.dirname(cachePath) to 0700 for any caller-supplied cachePath, not just the SDK's own default directory. If someone configures cachePath to a file inside an existing shared directory (say, directly in $HOME), that directory's permissions get silently narrowed to 0700 on the first successful write, with no opt-out and nothing in the CHANGELOG warning this would happen.

[mgallego-keeper]

LOW, efficiency: new Uint8Array([...transmissionKey.key, ...response.data]) boxes every byte through an intermediate JS array. Benchmarked at roughly 440x slower than a direct copy for a 50MB payload (about 1.8s versus 4ms), synchronously blocking the event loop on every successful cached call. cache.ts's own concatBytes (used one line later, inside encodeCacheBlob) already does this without the intermediate array; reusing it here would fix it.

[mgallego-keeper]

LOW: fs.readFileSync(fd) reads the whole file into memory before any format-version check, decryption, or auth-tag check runs, with no size cap. If something unusually large ends up at the cache path (a misconfigured shared directory, a stale leftover file, or the ancestor-symlink issue above), this forces an unbounded allocation before decodeCacheBlob gets any chance to reject it.

[mgallego-keeper]

LOW, observability: when caching is a no-op because the app key is a non-extractable CryptoKey (useObjects: true), this branch is silent, unlike the network-failure fallback five lines below, which does log when serving a stale cache. A caller who opts into useObjects: true gets no indication anywhere that caching is doing nothing for them until their first real outage.

[mgallego-keeper]

LOW, documentation: this says both the config file and the cache file/directory reject a symlink on write. That is accurate for the directory case (it throws), but the file-level case (saveStorage and writeCacheFile's own write, both via atomicWriteFileSync) never checks for a symlink at all; renameSync just silently replaces whatever is at the destination. Safe, but reject overstates what actually happens there: silent replacement gives no error or log signal, unlike the directory case.

[mgallego-keeper]

LOW, packaging: this pins @keeper-security/secrets-manager-core at 17.6.0, which is not published yet (highest on npm right now is 17.5.0, matching the CHANGELOG's own note that the 17.6.0 section is still unreleased). npm install in this example directory fails until the real release ships. Self-heals at release time, just flagging so it is not forgotten.