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 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, check the write returned the full byte count instead of trusting it, fsync, then rename into place, so a write that fails partway through - or lands short - can never leave a corrupted or truncated file behind. A write failure also removes its own temp file right away instead of waiting for the next orphan sweep. A hard-linked destination now goes through this same atomic write like any other file, instead of being written in place as a special case; only the resolved path gets the update, and a second hard-linked name to the same file keeps its old content.
• 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; this also covers a symlink whose target doesn't exist yet (a dangling, pre-provisioned symlink). This now also covers a chain of dangling symlinks, up to 40 hops deep. The write resolves through the full chain to the real target, not to an intermediate link. A resolution error partway through the chain, for example a permissions error, now fails the save instead of writing silently to the wrong path. 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.
• Node's package.json listed the import condition ahead of any Node-specific one, so a native-ESM Node consumer (import in a "type": "module" package) resolved the browser bundle instead of the Node build, failing at the first storage call with indexedDB is not defined. Node-specific conditions are now nested under an explicit node key, which Node always matches first regardless of ESM or CJS; a CommonJS require consumer was unaffected and still resolves correctly.

Known limitations, documented rather than fixed

• The cache directory is pinned with O_DIRECTORY where the flag exists, to reject a symlink swapped in for the directory itself; Windows has no equivalent flag, so this protection is POSIX-only, an accepted gap on that one platform.
• The directory pin's open-then-close doesn't actually pin anything past that initial check: the real file open that follows re-resolves the path, so a symlink swapped into an ancestor path segment after the directory handle closes isn't caught. Node's fs API has no openat()-style relative-to-fd primitive to close this properly.

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, including a chain of two dangling symlinks. A readlinkSync failure partway through resolution now rejects the save instead of replacing the link. A symlink swapped in mid-write is confirmed not to redirect the write. The orphan sweep finds a leftover temp file next to a dangling symlink's target directory. A hard-linked config path only updates the resolved path - a second hard-linked name to the same file keeps its old content, since the atomic write always creates a new file at the resolved path.
• 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 short write is rejected the same way instead of committing truncated content.
• 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.
• A native-ESM Node consumer, in a real child process against the built package, resolves the Node build and can save to a config file; the same check confirmed failing (indexedDB is not defined) against the unfixed exports field.

Full suite 191/191 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). A hard-linked config path no longer stays in sync across all its links; only the resolved path gets the update on each save. A symlinked config path, including a chain of symlinks up to 40 hops deep, is written through, not replaced. A save now throws if it hits a resolution error partway through that chain. Before this fix, that same save silently replaced the symlink instead.

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]

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

[mgallego-keeper]

Round 5, on commit ed74e995. The branch was force-pushed since round 4, so I compared trees rather than commit ranges.

CHANGES_REQUESTED, but narrowly and for one short round, not another deep cycle. The security core of this PR is sound and I verified it directly: the cache blob is AES-256-GCM under HMAC-SHA256(appKey, "KSM-cache-v1"), the freshness timestamp is inside the AEAD, and every tamper, truncation, wrong-key and forged-timestamp attack I ran was rejected. Round 4's stated blocker, the Windows symlink gap, is NOT resolved in code; it is now feature-detected and documented as an accepted POSIX-only limitation, with byte-identical runtime behaviour. I re-grade that code gap from HIGH to medium and accept it, because the blob is encrypted and authenticated so a followed junction leaks no plaintext, and the base had no protection on any platform. What I do block on is three small, mechanical items, only one of which is about the security fix at all: a new exports field that routes every native-ESM Node consumer to the browser bundle, a hard-link write-through on the cache path that the PR's own previous head did not have, and one CHANGELOG sentence that promises Windows users a protection the code does not provide. Each is a few lines to fix.

Where this stands

Not as it stands, because of one item that has nothing to do with caching: the new exports block in sdk/javascript/packages/core/package.json silently hands the browser bundle to Node ESM consumers, and it is invisible to tsc and to the whole test suite. That would ship as a broken package for anyone on "type": "module". Everything else I found is either small, pre-existing, or PR 1132's. If the author fixes the exports block, adds a caller-intent parameter to writeFileAtomic so the cache stops writing through a hard link, and qualifies one CHANGELOG sentence for Windows, this is ready. I would not hold the security fix for anything else on my list. Weighing the two costs directly: shipping the current state means publishing a package that breaks working ESM consumers and a security fix that reopens, on one path, the exact arbitrary-overwrite class it exists to close. Both are worse than a one-day round trip on three mechanical edits.

The security fix itself

Yes. The new format achieves what it claims, and I attacked it rather than reading it. Format, reverse-engineered then confirmed byte for byte: one cleartext version byte 0x02, then a 12-byte random GCM IV, then the ciphertext, then the 16-byte tag; inside the ciphertext sit an 8-byte big-endian millisecond timestamp, the 32-byte transmission key and the response body, so the file is the body length plus 69. The cache key is HMAC-SHA256(appKey, "KSM-cache-v1"), which I recomputed independently with crypto.createHmac and matched byte for byte, and encryption is AES-256-GCM with a fresh IV per write. Attacks run and all rejected: single-byte ciphertext flip, auth-tag flip, IV flip, timestamp-byte flip inside the AEAD, version byte set to 0x01 or 0x03, a fully forged blob under the correct cache key carrying a plus-ten-year timestamp, truncation to 0, 1, 13, 14, 28, 29, 36 and 37 bytes, a blob under a different app key, and the same blob after the app key was rotated. Every one failed closed with a KeeperError and no partial trust. The old pre-fix format planted at the new path is rejected in both sub-cases: a random first byte fails the version check, and the roughly one-in-256 case where that byte is 0x02 fails the tag check instead. Both CWEs are genuinely closed. CWE-312: I confirmed the written file contains no raw transmission key, no raw app key, no derived key and no plaintext body marker, where the base wrote the 32-byte transmission key in the clear immediately before the ciphertext it protected. CWE-345: I removed the GCM authentication enforcement from nodePlatform._decrypt (dropped setAuthTag and the final that validates the tag) and the PR's own test 'rejects a tampered cache file instead of returning a synthetic 200' failed, along with a folder-decryption integrity test. So the authentication property is real and it is guarded by a test. The claim that reading the cache requires the config also holds: a config with a different app key gets 'Unsupported state or unable to authenticate data', and a config with no app key gets 'Cached value does not exist'. Two honest limits on the claim, both non-blocking. The confidentiality property has no test, so a build that stores the plaintext beside the ciphertext passes the whole suite; that is N5. And the freshness window bounds only what the SDK will serve, not what stays on disk, so an older authentic blob restored inside the window is still served, which is N18.

The Windows gap from round 4

Definitive answer: SILENT NO-OP, not a hard throw, for the two flag expressions. The gap is real and unchanged from round 4. Mechanism, confirmed from primary sources plus a run: fs.constants.O_DIRECTORY and O_NOFOLLOW do not exist on win32 (node_constants.cc guards both with a bare ifdef, and Node's own docs list only O_APPEND, O_CREAT, O_EXCL, O_RDONLY, O_RDWR, O_TRUNC, O_WRONLY and UV_FS_O_FILEMAP as available there). So hasDirectorySymlinkProtection is false, cacheDirOpenFlags becomes fs.constants.O_DIRECTORY which is undefined, and cacheFileReadFlags degrades to plain O_RDONLY, which does exist. Node's stringToFlags maps a null or undefined flags argument to O_RDONLY, so the new guard is behaviourally identical to the old undefined|undefined, which is 0. It does not crash, because libuv's fs__open always adds FILE_FLAG_BACKUP_SEMANTICS with the comment that this makes it possible to open a directory. I proved the consequence by replacing the fs module with a copy whose constants object lacks those two keys: the PR's own test 'refuses to write when the cache directory itself is a symlink' fails, readdir of the symlink target shows the cache file, console.error is never called, and the read path also follows a symlink at the cache file. Note the two constants degrade differently, which the comment lumps together, and the comment's rationale is unreliable in two ways: it says O_NOFOLLOW in the hard-link branch is defence in depth 'on top of an already-safe rename' when that branch is the one that skips the rename, and it says no fallback exists although the author implemented an fs.lstatSync fallback on PR 1132 before removing it. WHAT TO RUN ON A WINDOWS MACHINE to settle the remaining unknowns, both of which would make things worse than a no-op. First, whether the cache write works at all on the default path: node -e "const fs=require('fs');fs.mkdirSync('t',{recursive:true});const d=fs.openSync('t');try{fs.fchmodSync(d,0o700);console.log('ok')}catch(e){console.log('threw',e.code,e.message)}". If that throws EPERM, then writeCacheFile fails on every successful response for the default ~/.keeper path, the error is swallowed as 'Failed to update cached response', and caching silently never works on Windows. Second, the junction case end to end: mklink /J at %USERPROFILE%.keeper, drive one successful response, and check whether the blob landed in the junction target; then repeat with mklink for a file symlink at ksm-cache.dat on the read path. Also worth adding a windows-latest leg to test.js.yml, following test.dotnet.powershell.yml which already runs a three-OS matrix, and gating the POSIX-only symlink tests on process.platform, using the pattern already at test/localConfigStorage.test.ts:77.

What is posted where

Three blocking items are inline. Twenty-four non-blocking items are also inline on the lines they concern; they are informational and I am not asking for another deep cycle on them. The three blocking items are the whole ask.

Findings in the shared write primitive, which now lives in this PR (8 items)

These are defects in writeFileAtomic. This PR replaced PR 1132's writeConfigFile with that shared
primitive, so a fix applied only on PR 1132 would be discarded when this PR merges on top. I verified
that by trial merge. I have raised the equivalent items on PR 1132 where they were already visible
there, but the fix has to land here to survive. P7 and P8 carry detail that PR 1132's review does
not have.

P1: writeFileAtomic ignores fs.writeSync's byte count on both branches

The unchecked fs.writeSync is byte-identical at base 7552c61 (lines 86 and 106) and is already the number-one blocking item of PR 1132's round-6 review, id B1, on the same file at line 106. Raising it here would tell the author the same thing twice on two stacked PRs. It is also present on the release tip in both saveStorage and the old cache writer, so neither PR introduced it. The only PR 1133 residue is that round 3 raised the byte-count clause against writeCacheFile, and the effect there is a lost cache entry that fails loudly on the authenticated read, which is better than the release branch's behaviour. Note for the merge: if PR 1133 lands first, B1's guard must travel with it, because PR 1133's diff carries the writeFileAtomic lines.

P2: A write failure leaves a 0600 temp file holding the full plaintext config

The write-failure catch closes the descriptor and rethrows with no unlink at base 7552c61 too, and cleanupOrphanedTempFiles is called only from readStorage there, so a long-lived instance never sweeps. Round 4 filed this as a MEDIUM against this PR, but the code is PR 1132's. What PR 1133 adds is a second caller, and that leftover is ciphertext and is swept by the next cache read or write, so it is benign. The plaintext leak, including a privateKey that mutateAndPersist then rolls back in memory, is entirely on the config path.

P3: The hard-link branch chmods by path after closing the descriptor

chmodSecure(finalPath) after fs.closeSync(fd) is identical at base 7552c61 line 99. Round 4 raised it as a HIGH; the rename branch half is now fixed (chmodSecure(tmpPath) runs before renameSync), and the residual is only on the hard-link branch, which is PR 1132's code. The fix, fs.fchmodSync(fd, 0o600) before fs.closeSync(fd), also closes the memory-versus-disk divergence below, so both should land in one place. Also state plainly there that an attacker with directory write access already has the larger realpathSync overwrite on the config path, so this alone is a small increment.

P4: On the hard-link branch a failure after the data is durable rolls back memory while disk keeps the new value

The write, ftruncate and fsync all complete before chmodSecure and the closeSync that can throw, and mutateAndPersist then restores the pre-write snapshot. The ordering is identical at base 7552c61, and PR 1133 only renamed variables there. The worst variant is a shrinking write where ftruncateSync fails: the file keeps the tail of the old longer JSON, so the next localConfigStorage() throws 'contains malformed JSON'. It belongs with P3 because one change fixes both.

P5: A directory at the destination has nlink >= 2, so writeFileAtomic misclassifies it as a hard link

The stat.nlink > 1 test is at base 7552c61 line 74 and was already posted on PR 1132's round 5 at that line. The outcome is a clean EISDIR either way: I confirmed the rename branch also throws EISDIR on a directory, so an isFile() guard changes only the message. The config path never reaches it, because readStorage throws EISDIR at construction. This is a comment-accuracy nit on PR 1132's line, not a PR 1133 defect.

P6: A writable config file inside a non-writable directory can no longer be saved

writeFileAtomic always creates its temp file next to the destination, which is base behaviour at 7552c61 lines 103 and 104. It was already reported on PR 1132 round 4 and the author disclosed it in the KSM-1266 CHANGELOG entry, so it is a resolved design decision there. On the cache path the same failure is caught and logged, so the request still returns its live response. Do not ask for an in-place fallback: that would reintroduce the truncate-then-write window PR 1132 removed, in exactly the deployment that asked for atomicity.

P7: The hard-link branch loses O_NOFOLLOW on Windows, and NTFS hard links do report nlink above 1

fs.constants.O_RDWR | fs.constants.O_NOFOLLOW evaluates to plain O_RDWR on Windows, and the branch is identical at base 7552c61 line 84. So the Windows half of this belongs with PR 1132's own accepted O_NOFOLLOW gap, which the head comment already cross-references. PR 1133 only widens it to the cache file, where the payload is ciphertext rather than the plaintext config.

P8: The 60-second orphan sweep can delete a live writer's temp file, and the config sweep runs only at construction

cleanupOrphanedTempFiles, its 60000 ms gate, and the readStorage-only call site are all base code at 7552c61 lines 149 and 203. The realistic trigger is not a stall: mtime comes from the file server and Date.now() from the client, so more than 60 seconds of clock skew on a network filesystem makes every fresh temp file look orphaned. The comment claiming the age threshold 'is the check that actually holds up here' should be softened there. PR 1133's own contribution is only that a cache temp file is now in scope, where the loss is a skipped cache refresh plus one log line.

Previously posted findings and their status at this head (13 items)

• STILL_OPEN, documented not fixed, re-graded to medium (round round 4) Windows: O_DIRECTORY and O_NOFOLLOW are undefined, so the cache directory and cache file symlink protection is a silent no-op, with no CI signal (round 4 inline, HIGH, the stated reason for ...
• STILL_OPEN, documented as accepted (round round 4) mkdirSync(recursive) follows a symlink in any existing ancestor segment of a multi-segment cachePath (round 4 inline, HIGH)
• STILL_OPEN, residual window now named in the comment (round round 4) The cache directory descriptor is opened with O_DIRECTORY|O_NOFOLLOW and closed immediately, so it pins nothing across the awaited encodeCacheBlob and the write (round 4 inline, HIGH)
• PARTIALLY_FIXED, rename branch fixed, hard-link branch open (round round 4) chmodSync by path after the rename is symlink-followable, and a chmod failure reports a durable save as failed (round 4 inline, HIGH)
• PARTIALLY_FIXED, Node only (round round 4) The stale-cache warning log has no regression test on either platform (round 4 inline, LOW)
• PARTIALLY_FIXED, cap added but advisory, and write side has none (round round 4) fs.readFileSync(fd) reads the whole file with no size cap before any format or authentication check (round 4 inline, LOW)
• PARTIALLY_FIXED, Node only (round round 4) storage.getBytes(KEY_APP_KEY) in the fallback path has no try/catch, on Node and at browser/localConfigStorage.ts:225 (round 4 inline, MEDIUM)
• PARTIALLY_FIXED, one browser site remains (round round 4) No guard on the caught value having a .message property (round 4 inline, MEDIUM)
• PARTIALLY_FIXED, Node is now a compile error, browser is not (round round 3) createCachingFunction's second positional parameter means different things on the two platforms (round 3)
• STILL_OPEN after two rounds (round round 3 and round 4) The suggested test cwd-restore ordering fix was not applied (round 3, restated as round 4 loose end 1)
• STILL_OPEN (round round 3) Node and browser createCachingFunction hand-duplicate the same control-flow skeleton (round 3 reuse suggestion)
• MOOT, and now better than moot (round round 3) KeeperError missing a .code property (round 3, recorded as moot in round 4)
• STILL_OPEN, deliberately not re-reported (round round 4) The example pins @keeper-security/secrets-manager-core at the unpublished 17.6.0 (round 4 inline, LOW)

Findings I checked this round and dropped, so nobody chases them again (6 items)

• Neutering the cache's AES-GCM authentication check leaves all tests green, so the CWE-345 fix has no ... I settled this myself, against a 2-to-1 lens vote to confirm, because my mutation is the faithful one. I removed the authentication enforcement at its source, in src/node/nodePlatform.ts _decrypt, by dropping ...
• renameSync may follow a reparse point at the cache path on Windows, so the arbitrary-file-overwrite fix may ... Two of three lenses refuted this and the documentation answers it. The junction half is answered outright: MoveFileEx states that with MOVEFILE_REPLACE_EXISTING, if the new name is an existing directory an ...
• cacheFileReadFlags drops O_NOFOLLOW whenever O_DIRECTORY is missing, discarding file-level protection for no ... The harmful case needs a platform that defines O_NOFOLLOW but not O_DIRECTORY, and no Node-supported platform is like that. Both entered POSIX.1-2008 together, so every POSIX target Node supports defines both, ...
• The browser closure should gain allowUnverifiedCertificate now, since the plumbing exists The plumbing does not exist. src/browser/browserPlatform.ts post takes three parameters and calls fetch, which has no equivalent of rejectUnauthorized, so the value cannot change behaviour. Adding the ...
• The orphan sweep can delete a config temp file belonging to a stalled writer, from any cache operation The cross-contamination half is false. cleanupOrphanedTempFiles builds its match prefix from basename of the path it is given, so a call passing cachePath can only ever match cache temp files. I confirmed the ...
• The Windows fallback and the exports types condition need no more than the fixes already proposed, and the ... Three smaller items collapse on inspection. The CACHE_KEY_LABEL of KSM-cache-v1 next to CACHE_FORMAT_VERSION of 0x02 versions two different things, the derivation scheme and the wire format, and nothing in the ...

Candidates for their own tickets (0 items)

Merge order

Merge order should be 1132, 1144, 1133, 1157, then 1136. I re-ran that simulation independently: steps 1 to 4 are clean apart from CHANGELOG.md, and after step 4 the tree builds, tsc is clean and 198 tests pass. Step 5, PR 1136, conflicts in exactly five files: examples/javascript/custom-caching-function-support/hello.js, src/browser/localConfigStorage.ts, src/keeper.ts, src/node/localConfigStorage.ts and test/keeper.test.ts. The real risk is a silent one and it is on the timeout, not the cache. PR 1136 widens queryFunction to five parameters and passes requestTimeoutMs fifth; I confirmed PR 1133's Node closure takes four parameters and the browser closure takes three. TypeScript accepts a narrower function where a wider type is expected, so the assignment compiles clean under strict and the timeout is validated, clamped, then discarded. Correcting MERGE-ORDER-FINDINGS item 5c and its wording twice: first, the effect is not an unbounded hang, because src/deadline.ts sets DEFAULT_REQUEST_TIMEOUT_MS to 30000 and both platform.post implementations call deadlineSignal unconditionally, so a dropped value means the 30 second default applies instead of the caller's; second, PR 1136's own test/cachingFunctions.test.ts DOES guard the forwarding, and once its import is repointed at the new factory it reports TS2554 'Expected 3-4 arguments, but got 5' at lines 45 and 79. That guard lives only in that one file, and that file cannot compile in the merged tree because it imports the removed cachingPostFunction, so a resolver who deletes or skips the suite loses both the guard and the timeout. On the question of whether PR 1133 should stabilise its closure signatures now: no for timeoutMs, yes for nothing else. Do not add a trailing timeoutMs here. At this head platform.post has no timeout parameter and neither src/deadline.ts nor validateTimeoutMs exists, so the parameter could only be accepted and ignored, which would silence the one guard that catches the drop. Also do not add allowUnverifiedCertificate to the browser closure: I checked and src/browser/browserPlatform.ts post takes three parameters and uses fetch, which cannot bypass certificate verification, so it would be dead code advertising a capability the browser build lacks. In PR 1136 that parameter exists only as positional padding so timeoutMs lands in the right slot. PR 1136's rebase therefore owns the whole reconciliation: add timeoutMs to both closures, forward it into platform.post, call validateTimeoutMs before the try, make 'if (e instanceof KeeperError) throw e' the first line of the catch (otherwise a deliberate client-side timeout is swallowed and returned as a synthetic 200 carrying stale cached bytes, which I confirmed in the merged tree), and repair rather than delete test/cachingFunctions.test.ts. Two more merge-hygiene notes. CHANGELOG.md conflicts at steps 2, 3 and 4 but auto-merges with no marker at step 5, so PR 1136's KSM-1209 entry survives verbatim and then contradicts PR 1133's removal announcement a few lines below; four of its claims about the offline-cache helpers become false. Fix it during PR 1136's rebase. And any remaining config-write fix for PR 1132, including its blocking B1, should be applied on PR 1133's branch rather than PR 1132's: PR 1133 renamed writeConfigFile to writeFileAtomic and moved the realpathSync resolution out, so a fix touching those lines either conflicts with mismatched variable names, where taking theirs drops it silently, or auto-merges into the now-shared primitive and starts governing the cache file too.

Test status

I ran both myself at head ed74e99 in a clean checkout of the package npx tsc --noEmit exits 0 with no output. npm test (which runs rollup as pretest, then jest) reports "Test Suites: 18 passed, 18 total" and "Tests: 184 passed, 184 total" in 4.696 s. So the PR body's "183/183" is stale after the force-push; the real count is 184. One methodology note for anyone repeating mutation work: a copied tree is NOT fully green. In an rsync copy the baseline is 4 failed / 180 passed, because test/keeper.test.ts reads ../../../fake_data.json and test/proxy.test.ts needs a live proxy. Any mutant result must be compared against that copy's own baseline, not against 184. Also, test/localConfigStorage.test.ts imports the built bundle through '../', so a src edit is invisible until npx rollup -c --bundleConfigAsCjs runs; test/browserLocalConfigStorage.test.ts imports src directly and needs no rebuild.

[mgallego-keeper]

N1 (low): The cache write path has no size cap, so a response over 10 MB is cached and then always rejected

readCacheFile rejects any file larger than MAX_CACHE_FILE_BYTES, but writeCacheFile applies no bound. Blob overhead is exactly 69 bytes, so a response body over 10485691 bytes produces a file the read path always refuses. The SDK writes and fsyncs that file on every successful call, then refuses it during the outage the cache exists for. Please compare blob.length against MAX_CACHE_FILE_BYTES in writeCacheFile and skip the write with one console.error. The cache recovers once a smaller response arrives, so this is not permanent. Please also state the cap in the CHANGELOG. Note the comment cites MAX_ERROR_BODY_DECODE_BYTES in keeper.ts, which exists only on PR 1144's branch.

[mgallego-keeper]

N2 (low): The read-side size cap is advisory, because readFileSync re-stats the descriptor

The cap reads fs.fstatSync(fd).size and then calls fs.readFileSync(fd), which re-stats the descriptor and reads the fresh size. A file that grows after the check is read in full. This is a residual of round 4's request, not a miss: the cap does cheaply reject a large pre-existing file. Allocating one Buffer of MAX_CACHE_FILE_BYTES + 1 and reading with fs.readSync would bound the allocation regardless. An fstat isFile() check would also reject a FIFO or a device, which the current O_NOFOLLOW open allows.

[mgallego-keeper]

N20 (low): The prescribed migration fails silently for CommonJS JavaScript consumers

cachingPostFunction is now undefined on the imported module, and postQuery does options.queryFunction || postFunction, so a CommonJS consumer who keeps the old wiring loses caching with no error, no warning and no log. An ESM consumer gets a loud SyntaxError naming the missing export, so the silent case is CommonJS only. The unmigrated consumer also keeps the old plaintext cache.dat on disk, so the security fix never reaches them. Keeping an exported cachingPostFunction for one release that throws a message naming createCachingFunction would make the removal loud for both language groups. The cheaper option is one CHANGELOG sentence saying a CommonJS consumer who does not migrate loses caching silently rather than seeing an error, and naming cache.dat as the file to delete.

[mgallego-keeper]

N21 (low): A breaking change ships as a plain fix commit inside a minor version bump

cachingPostFunction is exported on master at 17.5.0 and is removed here, while the version stays 17.6.0, a minor bump. semver.satisfies('17.6.0','^17.3.0') is true, and the four sibling storage packages all pin ^17.3.0, so they accept it with no action. A TypeScript consumer gets TS2305; a CommonJS JavaScript consumer gets a runtime TypeError. All three commits are plain fix(javascript) with no exclamation mark and no BREAKING CHANGE footer, while the repo does use that marker elsewhere and the last breaking JavaScript change took the major bump to 17.0.0. Please put the marker on the squashed commit and add the footer, so the history matches the CHANGELOG's own BREAKING label, and confirm with the release owner that a minor is the accepted number here.

[mgallego-keeper]

N22 (nit): Test cwd-restore ordering fix from round 3 is still not applied

process.chdir(tmpDir) and fs.chmodSync(tmpDir, 0o755) still run before the try whose finally restores the working directory. If the chmod throws, afterEach deletes tmpDir while the process is still inside it. The blast radius is wider than in-file: jest reuses a worker across files and never restores the working directory, so the next test file fails to load at all with ENOENT uv_cwd from inside jest's own setup, which names neither this test nor chdir. Moving try one line up, above the chdir, removes it. The trigger needs an unusual filesystem, so this is a nit, but the fix is one line of re-indentation.

[mgallego-keeper]

N23 (nit): Small claim and comment inaccuracies: test count, and ticket plus review metadata in shipped comments

Two small things. The PR body says 183/183 passing; I measured 18 suites and 184 tests at head, and the tsc claim is correct. And three new comments carry ticket numbers (lines 59, 362 and 384), with line 362 also embedding review-process metadata: 'matching how KSM-1266's own review already decided the identical Windows gap for O_NOFOLLOW'. A future reader of the shipped bundle cannot open a review round. Please state the constraint directly and keep ticket numbers in the commit message, the PR description and the CHANGELOG. Note the release branch already ships six such references, so the bare ticket numbers are optional cleanup; the review-round wording is the part that is clearly out of place. Separately, the comment at line 409 cites MAX_ERROR_BODY_DECODE_BYTES in keeper.ts, which exists only on PR 1144's branch.

[mgallego-keeper]

N24 (low): Windows: fchmodSync on the cache directory's descriptor may throw, which would disable cache writes on the default path

On the default path this fchmodSync runs on every successful response. libuv implements fs.fchmod on Windows as ReOpenFile(handle, FILE_WRITE_ATTRIBUTES, 0, 0), passing 0 for dwFlagsAndAttributes, while Microsoft documents FILE_FLAG_BACKUP_SEMANTICS as the flag that obtains a handle to a directory. Two ReOpenFile implementations I could read also set FILE_NON_DIRECTORY_FILE unconditionally. If it throws, createCachingFunction catches it and logs 'Failed to update cached response', so caching would silently never work on Windows on the default path. I confirmed that consequence by fault injection on POSIX: the request still returns 200, no cache file is created, and the first outage throws 'Cached value does not exist'. I have no Windows host, so please run this there: node -e "const fs=require('fs');fs.mkdirSync('t',{recursive:true});const d=fs.openSync('t');try{fs.fchmodSync(d,0o700);console.log('ok')}catch(e){console.log('threw',e.code,e.message)}". The mode bits are meaningless on Windows anyway, so gating the call on hasDirectorySymlinkProtection would remove the risk at no cost.

[stas-schaller]

@mgallego-keeper Round-5's 3 blocking items, verified locally rather than just re-read, and pushed as d9a34c03.

B1 (exports field routes ESM Node consumers to the browser bundle) — already fixed in d94b9d8b. Reproduced your failure mode from scratch (scratch npm package, "type": "module", symlinked into node_modules, real node process): stripping the node condition from package.json's exports block reproduces your exact SyntaxError: ... does not provide an export named 'platform'. With the current exports block it resolves the Node build and works. Same technique your finding used, and the same one test/exports.test.ts now runs as a regression test.

B2 (cache writes go through the config file's hard-link write-in-place branch) — turns out to be moot rather than needing a preserveInode param: writeFileAtomic no longer has an nlink > 1 branch at all, it was removed entirely by #1266's round-6 fix (29e8919c), which landed here via the rebase. Every write, config or cache, always goes through temp-file-then-rename now, so there's no in-place-write path left to write through. Verified with a real repro: hard-linked a victim file to the cache path, drove a real (network-mocked) cache write through createCachingFunction, and confirmed the victim's inode and content are untouched afterward - the cache path now points at a new inode instead.

B3 (CHANGELOG claims the symlinked-cache-directory rejection is unconditional) — this one was real, fixed now. The PR description already had the POSIX-only qualifier; the CHANGELOG sentence didn't. Reworded to match. Also dropped the review-round reference from the comment above hasDirectorySymlinkProtection while touching that area (your N23).

tsc --noEmit clean, jest 19/19 suites, 188/188 tests, both before and after this push (doc/comment-only change, no functional or test edits).

[mgallego-keeper]

Round 6, on commit ae252558d.

APPROVE. Nothing blocks. All three round-5 blocking items are closed at the current head, and the two that were still open at the head I was handed are closed by a commit the author pushed today (ae25255). Re-pin the review to head ae25255 on base 587b32f before posting: I diffed all 45 blobs in the package and only CHANGELOG.md and src/node/localConfigStorage.ts differ from the pinned worktree, by exactly the two edits in that commit, so every other finding carries unchanged. Line anchors in src/node/localConfigStorage.ts from line 355 onward shift down by one at that head; the anchors below are already re-derived against ae25255.

What the fix got right

The exports fix is the right fix, and the author did it properly. He did not just reorder keys. He added a node condition that terminates the resolution decision for every real Node runtime, and he placed it ahead of browser, which is the part that actually closes B1. I confirmed the outcome independently with a real resolver and with real consumers: a native-ESM import and require both land on dist/index.cjs.js, the platform export is present, and saveString works.

The test is the best part. He wrote test/exports.test.ts to spawn a real child Node process, in its own package with its own type field, because no in-process jest test can exercise the native-ESM import condition. That is the correct diagnosis of why round-5's defect shipped with a green suite. I mutation-tested it: removing the node condition fails it, and repointing node.import at the browser bundle fails it. So it is a genuine regression guard for the exact defect, not a test written to pass. Credit for reaching for a child process rather than the easier assertion.

The newest commit, ae25255, closes B3 in close to the reviewer's own words, and it removes review-round wording from a shipped code comment without being asked a second time. The PR body's test count now reads 188 of 188, which matches my measurement exactly, so N23's test-count claim is closed too.

The rebase was also handled carefully. Every one of PR 1132's fixes arrived: the short-write comparison against dataByteLength, both temp-file unlinks, the dangling-symlink resolution, chmod before rename, and fsync before rename. The author generalised the shared write primitive to accept bytes as well as a string rather than forking it, and he hoisted the symlink resolution into saveStorage so the cache path deliberately never resolves a symlink. That is a better placement than the base branch's, and it is behaviour-preserving. The stacking hazard the previous round warned about, that a fix landing only in PR 1132's writeConfigFile would be discarded when PR 1133's writeFileAtomic merged on top, did not materialise.

Deleting the hard-link write-in-place branch also closed five separate findings at once (B2, P3, P4, P5, P7) and made an existing CHANGELOG claim true (N12). That was PR 1132's change, but accepting it rather than preserving the branch was the right call here.

Where this stands

Yes. Ship it, ideally with the one-token C6 fix. PR 1133 is stacked on PR 1132, whose own round-7 review still carries a blocking item, so PR 1133 cannot merge ahead of it. That gives the author a natural window to land C6 without another full review round for PR 1133. Holding this PR longer costs more than the residuals do. It is the actual CWE-312 and CWE-345 remediation: it replaces a cache file that stored the transmission key and the response body in plaintext with no integrity check and no freshness bound. I re-attacked the new format at this head and could not break it. Of the 20 items below, 6 are medium, 10 are low and 4 are nits, and not one is a security regression against the release branch.

Closure

3 of 3 blocking items closed. B1 is fixed by the fix commit: a node condition ahead of browser, with import and require both pointing at the Node bundle, plus a real child-process ESM test that I mutation-tested and that has teeth. B2 is closed because the rebase deleted the hard-link write-in-place branch entirely, so a cache write can no longer route through it; there is no nlink check anywhere in src, and the round-5 attack now leaves the hard-linked victim with its content, its mode and its inode intact. B3 was still open at the pinned head and is fixed at ae25255, which adds close to the exact POSIX qualifier that was asked for. Of the 24 non-blocking items, 2 are closed (N12 became true when the rebase removed the branch; N23's test-count claim now matches my measured 188 of 188), N23 is otherwise two-thirds closed because the review-round wording is gone from the shipped comment at ae25255, and 21 are byte-for-byte unchanged and still open. Of the 8 items routed to PR 1132, 4 are moot because the hard-link branch is gone (P3, P4, P5, P7), 2 are fixed and carried through the rebase (P1 the short-write guard, P2 the write-failure temp-file cleanup, with one residual that stays on PR 1132), and 2 are unchanged (P6, a resolved design decision there, and P8). Nothing the fix commit or the rebase did broke a previously working behaviour. This round's two real costs are prose accuracy (four items) and the new test's breadth (one item). 20 findings survive verification, 5 were refuted and dropped, and 8 are routed to PR 1132.

The security core

The cache format still holds after the rebase. The attack set was re-derived from a real write and re-run in full at this head, not assumed from the previous round.

Rejected with a clear error and no partial trust: a single ciphertext byte flip, an authentication tag flip, an initialization vector flip, a timestamp byte flip inside the authenticated region (both the high and the low byte), the version byte set to 0x00, 0x01, 0x03 and 0xff, a fully forged blob under the correct derived key carrying a timestamp 365 days in the future, a blob two days old, truncation to 0, 1, 12, 13 (the exact header boundary), 14, 28, 72, 73 and 88 bytes, a blob under a different app key, a blob encrypted under the raw app key instead of the derived key, the same blob after an app-key rotation, an old-format plaintext file planted at the new path, and a file one byte over the size cap. For the over-size case fs.readFileSync is called zero times, so the bound is enforced before any allocation.

Confidentiality holds. The written file contains no transmission key, no app key, no derived key and no plaintext body bytes, and the first byte is the version 0x02. Reading the cache from a new instance still needs the config.

On the filesystem side, no attack redirected a write on either path. A live symlink, a dangling symlink (absolute and relative), a dangling chain, a symlink planted just before the rename, and a directory at the path were all handled correctly on the cache path, and the config path kept its documented and intentionally different symlink behaviour. A real SIGKILL mid-save, both at fsync and at a half-completed write, left the old content intact and re-readable on both paths.

I re-confirmed the two structural claims myself. There is no nlink check anywhere in src, so B2's write-in-place branch cannot be reached. And the release branch's cachingPostFunction has the same url-agnostic single-slot fallback with a plaintext transmission key and no integrity check, which is what this PR replaces.

Three real weaknesses remain, and none of them breaks the format. The entry is not bound to the request (C2), which is pre-existing and verified identical on release. A FIFO at the cache path hangs the read (C4), which is also pre-existing in the code this replaces. The freshness window is plus or minus the configured age rather than zero to it, because of Math.abs, so a blob timestamped 23 hours in the future is accepted under a 24 hour window; forging a future timestamp still needs the derived key, so that is a widened window, not an attack.

Packaging

The new exports block is correct for every real Node runtime and for browser-only toolchains, and wrong for one narrow class.

I resolved the actual block with a real resolver over nine condition sets. Correct: real Node ESM, real Node CommonJS, webpack target node, esbuild platform node, webpack target web, and a node-only set, which falls through the node object to the outer default and lands on the Node bundle. Wrong: the sets {module, import} and {import} both resolve the browser ES bundle. The first is @rollup/plugin-node-resolve at default options with ESM output. On the release-branch package shape those same two sets resolve the Node bundle, so this is a regression that this PR introduces. That is C6, and it is a one-token fix.

Two other consequences are new and undocumented. Every deep subpath now fails: I confirmed ERR_PACKAGE_PATH_NOT_EXPORTED for require and for dynamic import, and TS2307 for a TypeScript deep type import, while the published 17.5.0 has no exports field and ships both dist and src. And a toolchain that sets both node and browser now receives the Node bundle, where the previous head gave it the ES bundle.

The new test does guard the reported defect, and I proved that by mutation rather than by reading: removing the node condition fails it, and repointing node.import fails it. But it does not guard the rest of the block. Repointing the browser condition at the Node bundle, and repointing the browser types condition at the Node declarations, both leave the whole suite green, and no test, build step or continuous integration step exercises either. Neither is reachable from a spawned Node process, because Node always sets the node condition and never sets browser. So the test's scope matches the bug it was written for and stops there. A structural assertion over the parsed exports object would close the gap cheaply.

One earlier claim I withdraw: the browser ES bundle carrying a .js extension in a package with no type field is not a Node 20 hazard. I ran real Node 20.20.1 and it imports the bundle successfully, because module syntax detection has been on by default since Node 20.19.0. Only a deprecated flag reproduces a link-time failure.

Findings in the shared write primitive

These are defects in writeFileAtomic, which this PR shares with PR 1132. I am raising each once, on PR 1132, so the fix lands where it survives the merge. Listed here only so the trail is clear.

• PR1132-1 The dangling-symlink resolution handles only one hop, so a two-link chain writes the config to the middle link and never creates the final ...
• PR1132-2 A dangling config symlink makes the SDK create its target anywhere on the filesystem at mode 0600
• PR1132-3 chmodSecure(tmpPath) before the rename and the rename-failure temp-file unlink both survive deletion with the suite green
• PR1132-4 dataByteLength's byte-versus-character semantics have no test, so replacing Buffer.byteLength with .length breaks every save once one ...
• PR1132-5 fs.closeSync and chmodSecure sit outside the write try, so a close or chmod failure still leaks a temp file holding the full plaintext ...
• PR1132-6 The 60-second orphan sweep can delete a live writer's temp file, and the config sweep runs only at construction
• PR1132-7 The config read blocks forever on a FIFO at the config path
• PR1132-8 The rollup node external list is missing path, so the build prints an unresolved-dependency warning on PR 1132's branch alone

What is posted where

19 non-blocking items are inline on the lines they concern. 1 do not anchor to a changed line, so they are below.

Items with no anchor in this diff

C27 (nit): The rollup node external list was not updated for the new os import, so the build prints an unresolved-dependency warning
sdk/javascript/packages/core/rollup.config.js line 39

The node bundle's external array in sdk/javascript/packages/core/rollup.config.js was not updated, so the build now prints an unresolved-dependency line for os. I see it on every build, including the prepublishOnly build, which runs the same command.

The output is correct: dist/index.cjs.js requires both os and path properly, because rollup externalises a builtin anyway and only warns. Only the log is wrong. The concern is that a benign warning in this class trains the team to ignore it.

The array already hand-lists crypto, constants, https and fs, and the release branch builds with no warning at all, so keeping the list current is this package's own convention. Please add "os". The path line in the same warning comes from the base branch, and I have raised that there. A tidier option is to replace the hand-list with the builtin module list, which also drops "constants", since nothing under src imports it. Adding both names removed the warning in my test and left the emitted bundle unchanged apart from the version timestamp.

Previously posted items still open at this head (24 items)

• N1 and N2 STILL_OPEN The cache write path has no size cap, and the read-side cap is advisory because readFileSync re-stats the descriptor
• N3 STILL_OPEN The browser fallback calls storage.getBytes unguarded, while the Node side has a try and catch with a test
• N4 and carry-over 9 STILL_OPEN The browser factory takes a bare positional maxCacheAgeMs, so the Node options object disables the freshness window there
• N5 and N14 STILL_OPEN No test asserts the cache blob excludes the plaintext, and several guards including the key derivation survive deletion
• N6 STILL_OPEN The forged-timestamp test passes on the pre-fix layout too, so the round-3 blocker fix is revertible with a green suite
• N7 STILL_OPEN The orphan sweep resolves a symlink at the cache path, so it sweeps the wrong directory
• N8 and N16 STILL_OPEN A symlinked cache directory disables cache writes silently, and a cache read failure replaces the original network error
• N9 and N18 STILL_OPEN One machine-wide default cache path, and a rejected cache file is never deleted
• N10 STILL_OPEN One unkeyed cache slot serves every endpoint
• N11 STILL_OPEN The CHANGELOG claim that modern TypeScript resolves the browser type declarations is false
• N13 STILL_OPEN The read-path symlink protection has no test on any platform, and there is no Windows continuous integration
• N15 and carry-over 5 STILL_OPEN The browser stale-cache warning has no regression test, so the line can be deleted with the suite green
• N17 STILL_OPEN Math.abs on the freshness check widens the acceptance band for a fast writer clock
• N19 STILL_OPEN The shared Keeper directory has its permissions narrowed on every default-path write
• N20 STILL_OPEN The prescribed migration fails silently for CommonJS JavaScript consumers, since cachingPostFunction is simply undefined
• N21 STILL_OPEN A breaking change ships as plain fix commits inside a minor version bump
• N22 and carry-over 10 STILL_OPEN The test cwd-restore ordering fix is still not applied, now after three rounds
• N23 PARTIALLY_FIXED Claim and comment inaccuracies: test count, review metadata, ticket numbers, and a cross-PR symbol
• N24 STILL_OPEN Windows: fchmodSync on the cache directory descriptor may throw, which would disable cache writes on the default path
• P6 STILL_OPEN A writable config file inside a non-writable directory can no longer be saved
• P8 STILL_OPEN The 60-second orphan sweep can delete a live writer's temp file, and the config sweep runs only at construction
• carry-over 1 STILL_OPEN Windows: O_DIRECTORY and O_NOFOLLOW are undefined, so the cache symlink protection is a silent no-op with no continuous-integration signal
• carry-overs 2 and 3 STILL_OPEN mkdirSync(recursive) follows an ancestor symlink, and the cache directory descriptor pins nothing across the awaited encode and write
• carry-overs 8 and 11 STILL_OPEN One browser site still interpolates a caught value's .message directly, and the two platform caching functions duplicate the same skeleton

Candidate findings I checked and dropped (7 items)

• On Windows the cache directory open uses undefined flags, so the open itself fails and caching never ... The load-bearing step is wrong. libuv's fs__open sets FILE_FLAG_BACKUP_SEMANTICS unconditionally, precisely so a directory can be opened, so a read-only directory open succeeds on Windows ...
• dist/index.es.js is ESM with a .js extension, so on Node 20 the mis-resolution is a link-time failure ... Refuted by both lenses and by me on a real Node 20. Module syntax detection has been on by default since Node 20.19.0, and Node 20 reached end of life in April 2026, so every Node 20 ...
• The 32-byte transmission-key prefix in the cache blob is unvalidated, so a wrong-length key silently ... The mechanism is real in isolation but unreachable through the SDK, and I verified the decisive step myself. In src/keeper.ts, postQuery calls encryptAndSignPayload at line 812 before it ...
• The new test's header comment claims Jest never resolves through exports conditions, which is false One lens confirmed and one refuted, and the refutation is stronger. The comment is qualified ("the way a real consumer's import and require does") and its operative reason, that a relative ...
• In the browser the cache blob shares storage with the app key, so the CHANGELOG's confidentiality claim ... One lens confirmed and one refuted the core framing. I read the sentence: it names "the cache file" and the Node default path in the same clause, inside an entry that opens with a Node-only ...
• The node condition block has no inner default, so a resolver that activates node but neither import nor ... Refuted by one lens and confirmed by me with a real resolver. The condition sets {node} and {node, module} both resolve dist/index.cjs.js, through the outer default, not the browser bundle. ...
• The review is pinned two commits behind the PR True as an observation, but not a code finding and not something to post. The correct action is to re-pin, which I did: head ae25255 on base 587b32f. I then diffed all 45 blobs and ...

Candidates for their own tickets (4 items)

• Bind the cache entry to the request, and stop serving a cached read response for a mutating action C2 is pre-existing on the release branch, verified by running the same offline updateSecret against cachingPostFunction, and it is also present in the Python ... Existing ticket: none identified
• Give the default cache file a per-configuration name, and treat an authentication failure as a cache miss C9 needs two changes that are wider than a review comment: a naming scheme derived from the client id, and a decision to unlink a rejected file and rethrow the ... Existing ticket: none identified
• Cross-SDK review of the caching post function's request binding The single unkeyed cache slot is present in at least the JavaScript and Python SDKs, and the Ruby SDK's spec asserts a 10 MB cache payload round-trips, which ... Existing ticket: none identified
• Add a Windows job to the JavaScript SDK test workflow Three separate items in this PR family cannot be verified without a Windows host: the O_DIRECTORY and O_NOFOLLOW no-op (carry-over 1), the fchmodSync behaviour ... Existing ticket: none identified

Test status

I ran it myself at the pinned head in the read-only worktree, following the machine-load rule of --maxWorkers=5.

npx tsc --noEmit exits 0.
npx rollup -c --bundleConfigAsCjs exits 0, and it prints an unresolved-dependency warning naming path and os. That warning is C27, and the path half belongs to PR 1132.
npx jest --maxWorkers=5 reports 19 suites passed of 19 and 188 tests passed of 188, in 2.1 seconds.

That matches the PR body's claim of 188 of 188 exactly, so N23's test-count sub-item is closed, and it matches the stated baseline of 19 suites and 188 tests. The delta from the previous head's 18 suites and 184 tests is the new test/exports.test.ts plus three tests that arrived with the rebase.

Treat the number as a weak signal for this PR, and say so in the review. Mutation testing across two angles compiled 48 single-guard mutants, each with a rebuild and a fresh type check so no mutant could look uncaught because it failed to compile, and 26 of them left 188 of 188 green. Those include a full revert of the round-3 authenticated-timestamp fix (C10) and two that break the confidentiality property this PR exists to add (C11). Continuous integration adds nothing here either: .github/workflows/test.js.yml fires only for master and the release branch, and this PR targets PR 1132's branch, so no SDK test runs on it, and the workflow is ubuntu-latest only, so no Windows behaviour in this file is observable.

I made no writes anywhere. Both read-only worktrees report a clean git status, every probe ran in a temporary directory, all gh calls were read-only views and content fetches, and the orphan-process check found nothing.

[mgallego-keeper]

C6 (medium): The generic import condition still points at the browser bundle, so a resolver that activates import without node or browser gets the wrong build

The node condition closes B1 for real Node, and I confirmed that: at this head a native-ESM consumer and require both resolve dist/index.cjs.js, and localConfigStorage(...).saveString works. One residual is left. The outer "import" key still points at ./dist/index.es.js, which rollup.config.js builds from src/browser/index.ts. "import" is a module-format condition, not a platform condition. So a resolver that activates import without node or browser still gets the browser bundle.

I measured this with a real resolver over the actual exports block. The condition sets {module, import} and {import} both resolve dist/index.es.js. {module, import} is @rollup/plugin-node-resolve at default options with ESM output, which is a plausible Node-targeting setup. The bundled application then fails at the first storage call with "ReferenceError: indexedDB is not defined", the same symptom as B1. On the release branch there is no exports field, so the identical consumer resolves dist/index.cjs.js and works. That makes this a regression for that consumer.

Fix: point the outer "import" at ./dist/index.cjs.js, the same target as "require" and "default", or drop "import" and let "default" handle it. I checked the result: {module, import} and {import} flip to the Node bundle, and every browser set keeps the ES bundle, because "browser" is matched before "import". One note for consumers: an ESM-only bundler then receives a CommonJS file, so rollup users also need @rollup/plugin-commonjs for named exports. The release branch put them in the same position through "main", so this is not a new burden.

test/exports.test.ts cannot cover this shape, because it spawns a real Node process and Node always sets the node condition. A resolution assertion over a fixed condition set would cover it.

[mgallego-keeper]

C1 (medium): Round-5 N4 still open: the browser factory takes a positional number, the object form typechecks and silently disables the freshness window, and the correct browser call is a hard type error

Still open from round 5 (N4). src/browser/localConfigStorage.ts is byte-identical to the previous head, so this was not addressed this round.

The browser factory still takes a positional number, and the Node factory takes an options object. Round 5 already reported that the object form makes the staleness comparison NaN-false, so the browser cache never expires. I reproduced that again: with the object form and an entry one year old, the browser served it as a statusCode 200; with the number form the same entry was rejected.

One measurement is new, and it is the strongest argument for fixing this. The correct browser call, createCachingFunction(storage, 1000), is a hard type error under every default setting. moduleResolution node, node10, node16, nodenext and bundler all resolve dist/node/index.d.ts and report TS2559. Only customConditions: ["browser"] reaches dist/browser/index.d.ts. So the compiler rejects the right call and accepts the wrong one. The new exports field also exposes only "." and "./package.json", so importing the browser declarations by subpath now reports TS2307 under bundler and nodenext. That removes the workaround.

A non-finite value has the same effect on both platforms. Number(process.env.MAX_AGE) with the variable unset gives NaN, and on Node "options.maxCacheAgeMs ?? DEFAULT_MAX_CACHE_AGE_MS" does not filter it, because ?? only replaces undefined and null. I measured a ten-year-old entry served as a statusCode 200 in both builds.

Suggested fix: accept (storage, options?: number | {maxCacheAgeMs?: number}) on the browser for one release, and normalise a bare number to {maxCacheAgeMs}. Then, in both factories, reject a maxCacheAgeMs that is not a finite number above zero. That makes the documented browser call expressible under default TypeScript settings, and makes the mistake loud.

Impact, stated plainly: no shipped code passes the object form on the browser, the default 24 hour window works, and the mis-called path keeps this PR's encryption and integrity gains. Freshness falls back to release-branch behaviour rather than below it. The CHANGELOG does state the browser form in the same entry, so the docs are right and the type system is not.

[mgallego-keeper]

C2 (medium): Pre-existing: the cache entry is not bound to the request, so an offline write action reports success although nothing reached the server

One cache slot holds the last successful response for any action, and the fallback serves it for any failed call. Nothing binds the blob to the request: the authenticated plaintext is a timestamp plus the response bytes, and the caching function never reads its url argument.

I ran this against the built Node bundle at this head. After one successful get_secret filled the cache, I took the network down and called updateSecret(). It resolved with no error. The only network attempt went to update_secret and failed. The caching function returned statusCode 200 with the cached get_secret body and restored the cached transmission key, so postQuery decrypted it and keeper.ts discarded it. Nothing left the machine and the caller cannot detect it. createFolder, updateFolder and completeTransaction behave the same way. createSecret, createSecret2 and createFolder are worse than silent: they return payload.recordUid or payload.folderUid, so the caller receives an identifier for an object that was never created.

The read side is wrong in two more ways. A cached filtered get_secret response is served for a later unfiltered getSecrets, so a sync tool sees a truncated record set. One successful update_secret with an empty body also shrinks the cache, after which the next offline getSecrets throws a bare JSON parse error instead of serving secrets.

Attribution, stated plainly: I ran the same scenario against the release branch's cachingPostFunction and updateSecret() resolved there too, with the cached get_secret body served for the update_secret URL. So this is pre-existing and not a regression from this PR. This PR narrows the exposure, because the new 24 hour freshness window bounds it and the fallback logs a warning, where release had neither. The ask is to fix it while the cache format is still unreleased, not a new blocker.

Fix: put the request identity inside the authenticated plaintext next to the timestamp, for example the action path, bump CACHE_FORMAT_VERSION to 0x03, and treat a mismatch as a cache miss. Do not hash the encrypted payload: postQuery generates a fresh transmission key on every attempt, so both fields of EncryptedPayload are randomized per call and a digest would never match. Also skip the cache write when the response body is empty. A narrower acceptable answer is to cache and serve only get_secret and get_folders, and say so in the CHANGELOG's Known limitation paragraph, which today mentions only the bind call. Apply the change in both src/node/localConfigStorage.ts and src/browser/localConfigStorage.ts. No existing test pins the current cross-action behaviour, since every cache test uses one URL. The filtered-versus-full get_secret case is not closed by this, because both share one URL and the query function never sees the request plaintext; that needs a public signature change and belongs in its own ticket.

[mgallego-keeper]

C12 (medium): The 10 MiB cap is enforced on read but not on write, and it is below what the SDK itself writes, so a very large response loses the offline fallback that works on release

readCacheFile refuses any file over MAX_CACHE_FILE_BYTES, but writeCacheFile applies no bound. The constant is referenced only on the read side. Blob overhead is 69 bytes, measured, so a response body over 10485691 bytes writes a file the read path always refuses.

I ran it. An 11534336-byte response produced an 11534405-byte cache file with no log line. The next call, with the network down, threw "exceeds the maximum expected size" instead of serving the cache, and the original connection error text was gone from the message. The file is not removed, so every retry during that outage fails the same way. It recovers once a smaller response arrives.

Two facts argue the cap itself is too low, not just the asymmetry. On the release branch the same 11534336-byte response was cached and served back in full, so this is a behaviour regression for a very large application. The Ruby SDK's own cache spec asserts that a 10 MB cache payload round-trips, which contradicts the comment's claim of generous headroom over any real response.

Fix: either raise the cap to a value the SDK's own responses cannot reach, and say what bounds it, or enforce the same constant in writeCacheFile and skip the write with one log line, so the decision is single and explicit. Please also treat an over-size file on the read side as a cache miss: unlink it and throw the same "Cached value does not exist" error the missing-file branch uses, so the network failure keeps its context. Note the one cap test writes the over-size file directly, so it cannot catch a write-side gap; a round-trip test with a payload over the cap would.

[mgallego-keeper]

C10 (medium): Round-5 N6 still open: the round-3 authenticated-timestamp fix can be reverted with 188 of 188 tests green, because the test asserts only the error type

Still open from round 5 (N6). test/localConfigStorage.test.ts and src/cache.ts are byte-identical to the previous head.

This test does not pin what its name says. It flips one byte and asserts only rejects.toBeInstanceOf(KeeperError), and readCacheFile wraps both the staleness error and the authentication error in KeeperError. I checked by moving the freshness timestamp back into a cleartext header, which reverts the round-3 blocker fix. tsc --noEmit exits 0, the build succeeds, and all 188 tests still pass, including this one. Its message on the reverted layout is "cached value is stale", not an authentication failure, because byte 1 is the most significant byte of a big-endian millisecond timestamp and is always zero, so the flip only makes the date absurd.

On that reverted layout I then edited only the 8 timestamp bytes of an expired entry, leaving ciphertext and tag untouched, and the cache returned statusCode 200 with the stale body. So the property is real and worth a test that fails when it breaks.

Please splice writeTimestamp(Date.now()) over offsets 1 to 8 of an expired blob instead of flipping one byte, and assert the message names the authentication failure. I checked that version: it fails on the cleartext layout and passes on the code as written here.

Two smaller points. The inline comment says byte 1 falls inside the GCM ciphertext; the Node platform's encrypt returns a 12-byte IV first, so byte 1 is the first IV byte. The tag check still fails, so the test stays valid. Separately, test/cache.test.ts has one assertion about DEFAULT_MAX_CACHE_AGE_MS and no unit test for encodeCacheBlob or decodeCacheBlob at all, so this one assertion is the only guard on the wire format.

[mgallego-keeper]

C15 (low): Round-5 N11 still open: the CHANGELOG's only sentence about the exports field is wrong about both bundlers and TypeScript, and now also understates the change

Still open from round 5 (N11), and now also incomplete.

The KSM-1265 entry says the exports field exists so bundlers and modern TypeScript resolve the correct platform-specific type declarations for the browser bundle. Both halves are wrong. I re-checked with the package's own TypeScript 5.9 and traceResolution. moduleResolution node, node10, node16, nodenext and bundler all resolve dist/node/index.d.ts, and node16, nodenext and bundler each report a non-matching browser condition. Only customConditions: ["browser"] reaches dist/browser/index.d.ts. Bundlers do not read the types condition at all; they read the runtime conditions.

The sentence also understates the change, because the field decides runtime resolution and removes subpaths. A native-ESM Node consumer now resolves dist/index.cjs.js, which is what the node condition fixed. A resolver whose conditions are module and import resolves the browser ES bundle at this head and resolved the Node bundle on the release branch. Deep paths inside the package no longer resolve at all.

Please replace the sentence with three plain statements. The browser type declarations are selected only when a consumer opts in with customConditions on TypeScript 5.0 or later; every other setting continues to see the Node type declarations. A Node consumer, ESM or CommonJS, resolves the Node build. Paths inside the package are no longer importable, so import the package root. The entry also does not mention the new node condition at all.

[mgallego-keeper]

C22 (low): The CHANGELOG says a symlinked cache directory throws on read and write, but the write-side rejection never reaches the caller

The KSM-1265 entry now says a symlinked cache directory is rejected outright and throws on both read and write, on POSIX platforms. Thank you for the Windows qualifier. The read half is accurate: readCacheFile's error propagates through postQuery, which awaits the query function with no try and catch, so getSecrets rejects.

The write half does not reach the caller. writeCacheFile does throw on the directory open, but createCachingFunction catches every failure of that write and only logs "Failed to update cached response". I put the cache directory behind a symlink and called the caching function: it returned statusCode 200 with the full response body, nothing landed in the target directory, and that one log line was the only trace. A control with a plain directory wrote the cache file, so the write path really did run. Caching then stays off for the life of the process with no exception anywhere.

Please reword to say the read path throws and the write path skips the cache and logs. The behaviour itself is right: a failed cache write should degrade silently, so only the sentence needs to change.

[mgallego-keeper]

C23 (nit): Comment accuracy in shipped code: two references to the removed hard-link branch, one cross-PR symbol that does not exist, and two bare ticket numbers

Three comment residuals in src/node/localConfigStorage.ts, all shipped in dist/index.cjs.js.

First, and new since the previous head: two comments still reason from writeFileAtomic's hard-link branch, which the rebase removed. Line 346 says that in that branch a flag expression coerces to a plain value, and line 348 argues this Windows gap is bigger than that branch's, because there O_NOFOLLOW was one layer on top of an already-safe rename. writeFileAtomic is now one temporary-file and rename path with no such branch, so neither reference has a target, and the second one carries the accept-this-gap argument. A maintainer looking for the branch finds hard-link prose a few lines above that says the write-in-place alternative was rejected. Note the rebase did update a sibling reference in readStorage, so these two look like a dropped guard rather than a deliberate note.

Suggested self-contained replacement, no reference to the removed branch:
// fs.constants.O_DIRECTORY and O_NOFOLLOW do not exist on Windows. There the guard below leaves
// the directory open with an undefined flags value, which Node treats as a plain read-only open,
// and leaves the cache-file open at plain O_RDONLY. Neither call then verifies that the target is
// not a symlink. This is the only symlink protection the cache directory and cache file have.
// Node's public fs API exposes no openat()-style relative-to-fd primitive, so a check-then-open
// substitute would only add a race. Accepted as a documented POSIX-only limitation.
Only the directory flags fall back to undefined; fs.constants.O_RDONLY exists on Windows and is zero, so the file open becomes a plain read-only open.

Second, line 399 says the cache size cap has the same shape as MAX_ERROR_BODY_DECODE_BYTES in keeper.ts. That symbol does not exist anywhere in src on this branch; it lives on the throttle-truncation branch. Please state the headroom rationale on its own.

Third, two bare ticket numbers remain, at lines 60 and 374. Optional cleanup, since the release branch already ships six such references, but the what and the why read better inline. Thank you for removing the review-round wording in the newest commit.

[mgallego-keeper]

C17 (nit): The new exports test guards the reported defect but asserts a symptom, covers one condition, has no spawnSync timeout and hides the child's stderr

Thank you for this test. It has real teeth: I removed the node condition and repointed node.import at the browser bundle, and it failed both times. Four notes to make it cover more and fail more readably.

Coverage. The test asserts the absence of an indexedDB message, so most of the exports block stays unguarded. Repointing the browser condition at the Node bundle, and repointing the browser types condition at the Node declarations, both leave the suite green, and no test, build step or continuous integration step exercises either. Neither case is reachable from a spawned Node process, because Node always sets the node condition and never sets browser. A structural assertion on the parsed exports object, that is the key order and each target, would cover all of them.

Diagnosability. The stdout assertion runs before the status assertion, and the child's stderr is discarded by a not.toContain assertion. With no built output the failure reads only as an empty received string. Please assert result.status first, or include result.stderr in the failure message.

Robustness. spawnSync has no timeout, and jest's own timeout cannot fire while spawnSync blocks the worker. I measured this: an 8000 millisecond child passed under a 5000 millisecond test timeout, and when I killed a blocked run the child survived as an orphan process. Please pass a timeout.

Platform. fs.symlinkSync with type 'dir' needs elevated rights or developer mode on Windows, where 'junction' does not. This is polish rather than a new problem, since test/localConfigStorage.test.ts already creates ten ungated symlinks and the workflow is Linux only.

One correction to the header comment: Jest's resolver does honour exports conditions for a bare specifier. It applies require, node and default, so a self-referencing require resolves through this exact block. The load-bearing reason is the next clause, that a relative specifier bypasses conditional exports, and that a CommonJS environment cannot exercise the native-ESM import condition. Also, 8 of the other 18 test files import from '../src/...' rather than '../'.

[mgallego-keeper]

C25 (nit): The node condition sits ahead of browser, so a toolchain that sets both conditions now receives the Node CommonJS bundle

Small packaging note on condition order. The node key sits before browser, and conditional exports match in key order. So a toolchain that activates both conditions now resolves dist/index.cjs.js, where the previous head and the release branch both gave it dist/index.es.js. webpack sets both conditions for its electron-renderer and nwjs targets.

For electron-preload and electron-main this is an improvement, since they previously received the browser bundle. The one regressing shape is an electron-renderer build run without Node integration, which then fails on the CommonJS requires; target web is the correct setting there. No package in this repository sets both conditions.

If you want a browser-targeting build that also sets node to keep the ES bundle, list browser before node. Node never sets the browser condition, so real Node consumers and test/exports.test.ts are unaffected. This is a preference, not a defect.

[mgallego-keeper]

Round 7, on commit 36894ce5.

APPROVE. Round 6's sole remaining item, C6, is now fixed and verified. Everything else round 6 found is unchanged, because nothing else changed: the head I was about to review turned out to be the same 5 commits round 6 already reviewed, rebased onto the release branch's later commits (the KSM-1266 symlink-chain fix and the KSM-1395 throttle-status widening), with zero new content of their own. I confirmed this by diffing round 6's exact reviewed head against the current one and tracing every changed file back to base-branch churn, not new work.

What C6 was and what closes it

package.json's exports map pointed its generic import condition at the browser bundle. A bundler resolving with a condition set that includes neither node nor browser, for example @rollup/plugin-node-resolve at its documented defaults, fell through to that key and got a build backed by IndexedDB, which throws ReferenceError: indexedDB is not defined at the first storage call outside an actual browser. This fix commit points import at the same dist/index.cjs.js the node condition already resolves to, a one-line change.

The existing exports.test.ts spawns a real Node process to test the earlier, related B1 fix, but a real Node process always adds Node's own node condition and cannot be made to drop it, so that test could not reach this scenario. This commit adds a small, direct implementation of Node's own package-exports condition-matching algorithm, parametrized by condition set, and runs it against the real exports field read from disk. I mutation tested it myself: reverted only the package.json line with the built dist/ left in place, and confirmed exactly the new test fails, the pre-existing Node-process test and the two other new cases (real Node, real browser bundler) stay green. Not a tautology.

Test status

198 of 198 tests pass. npx rollup -c --bundleConfigAsCjs builds clean and npx tsc --noEmit exits 0, verified myself at this head.

Everything else, unchanged from round 6

19 inline comments from round 6 stand as posted; none of that content changed, since the underlying commits didn't change. The two items round 6 left open as explicit team decisions (the first-call cache miss deferred to a v18 spike, and the CHANGELOG's own BREAKING-in-a-minor labeling) are unaffected either way. The collision with PR 1136 over the same caching-function internals is a merge-order concern, not a defect in this PR, and does not block approving this content.