JavaScript SDK: fix config read errors silently degrading to an empty config (KSM-1266) - #1132
Conversation
stas-schaller
changed the title
mgallego-keeper
left a comment
mgallego-keeper
left a comment
There was a problem hiding this comment.
Review summary
Correct, well-targeted fix for KSM-1266. JSON.parse's SyntaxError has no .code property, so malformed JSON correctly falls through to the KeeperError branch rather than being mistaken for ENOENT; verified this holds. The three new tests (missing file, unreadable file, malformed JSON) match the intended behavior exactly, and the PR body's own disclosure of the behavior change (localConfigStorage can now throw where it previously started fresh silently) is accurate and, in my view, the right tradeoff: masking a permission error or corrupt config as "no config yet" is worse than a loud failure. One robustness note on the new tests below, not blocking.
Notes
The "unreadable config file" test assumes a non-root process
chmodSync(configPath, 0o000) only blocks read access for a non-root user. If this suite is ever run as root (a common default in Docker-based Node images or some CI containers), fs.readFileSync will still succeed despite mode 0, the file will parse as valid empty JSON, and readStorage won't throw at all, so this test would fail to exercise the code path it's meant to cover (and would fail outright, since the toThrow(KeeperError) assertion would not be satisfied). Not a defect in the production code, just a latent assumption in this specific test. Worth a comment noting the non-root assumption, or skipping the test when process.getuid?.() === 0, so a future root-run CI environment doesn't quietly lose this coverage.
Minor style nit
catch (e: Error | any) is unusual TypeScript; Error | any collapses to plain any during type resolution (confirmed this compiles cleanly under strict: true), so it reads as more specific than it actually is. Purely cosmetic, no functional difference from catch (e: any).
132ca09 to
ec90d0e
Compare
ec90d0e to
0f5c227
Compare
mgallego-keeper
left a comment
mgallego-keeper
left a comment
There was a problem hiding this comment.
Automated re-review. Left inline notes on gaps in the new error-classification logic (a process killed mid-save, non-object JSON, and BOM-prefixed files all currently regress to a crash or silent data loss instead of the intended fresh-start-or-typed-error split), a cross-platform consistency gap versus the browser localConfigStorage, and a few smaller test-coverage and simplification cleanups.
One gap doesn't map to a line touched by this diff, so noting it here: saveStorage's own fs.openSync/writeSync/closeSync/chmodSync calls (below the reviewed section) remain completely unwrapped. A save-time EACCES, ENOSPC, or EROFS still surfaces as a raw untyped Node error, inconsistent with the read-time KeeperError guarantee this file now makes. Pre-existing gap, not introduced by this PR, but worth closing in the same pass for consistency with errors.ts's documented advice that callers can check instanceof KeeperError to distinguish SDK errors from unexpected runtime failures.
… empty config (KSM-1266) localConfigStorage's readStorage treated every read failure as "no config yet," including permission errors and malformed JSON. Only a missing file (ENOENT) is treated that way now; everything else throws a KeeperError.
0f5c227 to
73430ef
Compare
…JSON, wrap saveStorage errors (KSM-1266)
There was a problem hiding this comment.
Round 3 re-review
Follow-up commit 97253dd0 (+ fcf00888) genuinely closes all 11 points from the 2026-08-26 review and the 2026-09-01 automated re-review: empty-file self-heal, non-object/array/null JSON rejection, BOM stripping, saveStorage error-wrapping, the e.code type guard, a describeCause helper modeled on the browser sibling, the round-trip assertion on the missing-file test, the chmod-restore cleanup, and the root-uid skip guard are all genuinely in place.
One exception: the 2026-09-01 note that chmod 0o000 also doesn't block reads on Windows, not just root, is still open; only a root-uid guard was added. Worth a process.platform === 'win32' check alongside it, or marking the test explicitly skipped on Windows.
A deeper pass found more beyond those 11, left as inline comments below where they land on a changed line. Three don't map to any line this PR touches, so noting them here:
file-upload, folders, notation, pam-linked-records, totp examples still lack the .catch() fix. The PR description says "every shipped Node example" now handles a construction-time throw; only 3 of 8 do (hello-secret, proxy-support, custom-caching-function-support, plus quicktest.ts). These 5 were added by the more recent #1151 and use the identical unguarded main().finally() pattern. file-upload/hello.js is the worst case: .finally(() => process.exit(0)) masks a corrupt-config throw as a silent, no-output exit 0 instead of a crash.
cachingPostFunction (same file, localConfigStorage.ts around line 118) has the identical unwrapped-fs-error pattern saveStorage just got fixed for. A cache-write failure after a successful HTTP response falls into the same catch used for POST failures, silently returning stale cached data as a fabricated 200.
platform.ts's loadJsonConfig (used by the Azure DevOps pipeline integration) does JSON.parse and hands the result straight to inMemoryStorage, with none of the null/non-object/array validation this PR just added for the file-backed path.
Not blocking overall (the core fix is sound and well-tested for what it covers), but the saveStorage-truncation-plus-self-heal interaction below is a real regression worth a look before merge.
| fs.writeSync(fd, JSON.stringify(storageData, null, 2)) | ||
| } finally { | ||
| fs.closeSync(fd) | ||
| const fd = fs.openSync(configName, 'w', 0o600) |
There was a problem hiding this comment.
Correctness (new regression): saveStorage truncates the file (fs.openSync(configName, 'w', ...)) before writing. If the write itself then fails (the exact class this catch was just added to handle), it leaves a 0-byte file on disk. The empty-file self-heal added a few lines up (raw.length === 0) then silently treats that as a legitimate fresh start on the very next read, permanently discarding the previous config. The caller correctly sees a rejected KeeperError from the failed save, but has no way to know the old config is now gone too. Two fixes that are each correct in isolation combine into a new hole.
| try { | ||
| parsed = JSON.parse(stripBOM(raw)) | ||
| } catch (e) { | ||
| throw new KeeperError(`Unable to read local config ${configName}: ${describeCause(e)}`) |
There was a problem hiding this comment.
Security: describeCause(e) forwards JSON.parse's raw SyntaxError message verbatim. V8's parse-error text can echo a literal snippet of the surrounding corrupted text, including a fragment of an adjacent secret value, into this KeeperError's message. That's more exposed than before this PR: the new example call sites do console.error(e.message) on exactly this error, a logging channel that didn't exist pre-fix (corruption used to be swallowed silently, with no logging at all).
| let raw: string | ||
| try { | ||
| return JSON.parse(fs.readFileSync(configName).toString()) | ||
| raw = fs.readFileSync(configName).toString() |
There was a problem hiding this comment.
Correctness: fs.readFileSync(...).toString() decodes as UTF-8 leniently: an invalid byte sequence becomes U+FFFD rather than throwing. Single-byte corruption landing inside a JSON string value (e.g. mid-privateKey) silently mangles that value and sails through every guard added below with no throw at all, which cuts against the design comment a few lines down ('fail loudly rather than guess').
| } | ||
| chmodSecure(configName) | ||
| } catch (e) { | ||
| throw new KeeperError(`Unable to save local config ${configName}: ${describeCause(e)}`) |
There was a problem hiding this comment.
Robustness: wrapping every saveStorage failure as a message-only KeeperError drops the original .code (EACCES, ENOSPC, EROFS, ...). A caller that wants to branch on failure type (retry on ENOSPC, alert on EACCES) loses that ability versus the raw fs error this replaces; only free-text describeCause(e) wording survives.
| } | ||
| throw new KeeperError(`Unable to read local config ${configName}: ${describeCause(e)}`) | ||
| } | ||
| // A process killed between saveStorage's truncate and completed write (OOM, SIGKILL, |
There was a problem hiding this comment.
Minor: the empty-file self-heal (raw.length === 0) is stricter than the 'Python SDK already treats a zero-length file as a fresh start' comment above implies: Python's equivalent also treats a whitespace-only file (a stray trailing newline, plausible from some editors or shell redirects) as empty. Here that still throws KeeperError permanently. Low severity, but the comment overstates the parity it's claiming.
| // not reliably `instanceof Error` under Jest's test environment, since Node's own bindings throw | ||
| // from a different realm than the one Jest exposes as the global Error. This also guards against | ||
| // a non-Error throw (throw null, throw 'x') ever reaching a property access. | ||
| const describeCause = (cause: unknown): string => { |
There was a problem hiding this comment.
Reuse: this comment frames describeCause as a sibling of the browser implementation's describeCause/idbFailure convention, but it's already diverged: the browser version also prefixes .name when present, this one only ever returns .message. Also, the citation to errorMessage()/classifyCryptoFailure() "elsewhere in this file" actually points to functions that live in keeper.ts, not this file. Not blocking, but worth either matching the browser sibling's output shape or dropping the "sibling" framing.
| - KSM-1332 - Fixed the browser IndexedDB storage hanging forever on a storage failure. `localConfigStorage` and `secureStorage` wired only `onsuccess`, so a failed IndexedDB open, read, write or delete left the promise pending with no error, no rejection and no timeout. All eight wrappers now reject with a typed `KeeperError`, and the blocked-upgrade and missing-object-store paths reject too instead of hanging. | ||
| - KSM-1263 - Fixed config and cache file permissions not being re-applied on every write. `fs.openSync`'s mode argument only takes effect when a file is created, so a config or cache file that already existed with looser permissions kept them; permissions are now explicitly reset to 0600 after every write. | ||
| - KSM-1267 - `getFolders()` now classifies why an undecryptable folder was skipped (`integrity`, `format`, `missing-key`, or `malformed-data`) instead of logging an opaque, unclassified error, and logs one summary line naming every folder UID it had to omit. Added an optional `onDecryptionError` callback to `SecretManagerOptions`, invoked once per skipped folder, so a caller can react to or throw to fail closed on a partial result; existing callers that do not set it see no behavior change. Both the Node and browser platforms' `unwrap()` now reject an unwrapped key of the wrong length immediately (a corrupted-but-plausible 16- or 24-byte result was previously accepted by both platforms and cached, failing later at an unrelated call site with a much harder to diagnose error). The underlying finding (the shared-folder key wrap uses unauthenticated AES-256-CBC, a format fixed server-side that the SDK cannot change unilaterally) was reviewed and confirmed low-impact: a manipulated folder key is still caught by the existing AES-GCM authentication on the record keys inside that folder. | ||
| - KSM-1266 - Fixed `localConfigStorage` treating every config-read failure as "no config yet." A missing file is still a legitimate fresh start, and so now is one left completely empty by a process killed mid-save (a partially-written file is not covered by this - there is no reliable way to distinguish a truncated write from genuine corruption, so it still throws). Permission errors, malformed JSON, a BOM-prefixed file, and JSON that parses but isn't an object (`null`, a number, an array) now throw a typed `KeeperError` instead of silently starting fresh or misbehaving on first use. `saveStorage`'s write path now wraps its own failures (e.g. `EACCES`, `ENOSPC`) in the same `KeeperError` guarantee. Node validates config readability eagerly, at construction; the browser `localConfigStorage` (KSM-1332, same release) defers the equivalent check lazily to first storage access, since IndexedDB has no synchronous API to check eagerly against - this timing difference between the two platforms is expected and now documented in-code. |
There was a problem hiding this comment.
This line says a BOM-prefixed file "now throws a typed KeeperError", the opposite of what actually ships. The code strips the BOM and accepts the file, and this PR's own new test (a BOM-prefixed config file is not treated as corrupt) asserts exactly that. Worth fixing the wording before this goes out in a release.
| expect(await kvs.getString('foo')).toBe('bar') | ||
| }) | ||
|
|
||
| test('a save-time write failure throws KeeperError instead of a raw fs error', async () => { |
There was a problem hiding this comment.
Test coverage: this only mocks fs.openSync to throw, so it never exercises fs.writeSync, the finally { closeSync }, or chmodSecure below it. The PR description specifically calls out "closing the file descriptor on a write failure before rethrowing", but nothing here actually tests that claim; a regression in the fd-cleanup-on-write-failure path would pass this suite untouched.
| // Docker-based Node images) would leave the file readable and this test would not | ||
| // exercise the path it's meant to cover. | ||
| if (typeof process.getuid === 'function' && process.getuid() === 0) { | ||
| return |
There was a problem hiding this comment.
Minor: the root-uid skip is a bare return, so under a root-run CI container this test reports as a normal green pass with zero assertions executed, rather than a visible skip. Worth an explicit test.skip-style marker so the coverage loss stays visible in CI output instead of silent.
|
|
||
| getKeeperRecords().finally() | ||
| getKeeperRecords().catch((e) => { | ||
| console.error(`Failed to load Keeper secrets: ${e.message}`) |
There was a problem hiding this comment.
Minor: this assumes the rejection reason is always Error-like. If anything upstream ever rejects with a non-Error (plain object, string, null), e.message itself throws inside this callback, which becomes a new unhandled rejection since nothing wraps the .catch() callback itself. Same pattern in proxy-support/hello.js and custom-caching-function-support/hello.js.
… stop leaking parse-error text (KSM-1266) saveStorage wrote directly to the config file, so a write failure after the truncate left a 0-byte file that the empty-file self-heal then treated as a legitimate fresh start, silently discarding the previous config. It now writes to a same-directory temp file and renames it into place atomically. readStorage decoded the file with a lenient UTF-8 decode that replaced invalid byte sequences with U+FFFD instead of throwing; it now uses TextDecoder with fatal:true, which also strips a leading BOM per spec, making the separate stripBOM helper redundant. The malformed-JSON error path no longer forwards JSON.parse's raw message, since V8's parse-error text can echo a fragment of the surrounding corrupted config. Also: the empty-file self-heal now treats a whitespace-only file the same as a zero-length one, matching the comment's existing parity claim; the unreadable-file test's root-only skip guard also accounts for Windows and uses test.skip so the skip is visible instead of a silent early return; a new test proves a save-time failure leaves the original config untouched; the CHANGELOG's KSM-1266 entry no longer claims a BOM-prefixed file throws (it doesn't - it's stripped and accepted); and the file-upload, folders, notation, pam-linked-records, and totp examples now catch a construction-time throw from localConfigStorage instead of exiting silently or crashing on an unhandled rejection, matching the other three examples.
mgallego-keeper
left a comment
mgallego-keeper
left a comment
There was a problem hiding this comment.
Round 4 re-review
Follow-up commit f06702dbc fixes 9 of the 13 round-3 points cleanly: whitespace-only self-heal, the corrected CHANGELOG BOM claim, the root-uid skip now using test.skip (and covering Windows too), all 8 shipped examples now guarding e.message with e?.message ?? String(e), the JSON.parse catch no longer forwarding the raw SyntaxError text, and a new test for save-failure-leaves-original-untouched.
Two round-3 points are still open, noted again below since they don't map to a line this PR touches: cachingPostFunction's stale-cache-as-fake-200 pattern, and platform.ts's loadJsonConfig missing the same JSON-shape validation this PR added for the file path.
The headline change this round, switching saveStorage from truncate-in-place to write-temp-file-then-rename for crash-safety, closes the truncation regression from round 3 but opens two new ones, both checked empirically rather than just read (see inline comments): a symlinked or hard-linked config path breaks silently on the next save, and saves now need write permission on the config's directory, not just the file. Neither is hypothetical, both are the kind of external convention a same-repo test suite has no way to exercise. Requesting changes on those two specifically; everything else here is non-blocking.
Three more items don't map to a changed line:
AwsKeyValueStore.ts's createConfigFileIfMissing() (and the identical pattern in the GCP, Oracle, and Azure backends) overwrites the real encrypted config on ANY fs.access failure, not just a missing file. Unrelated to this PR entirely (none of these four files are touched by this branch), but severe enough, and close enough in shape to what this PR just fixed for the Node file path, that it's worth its own ticket rather than going unnoticed. fs.access failing with EACCES/ESTALE/a transient permission blip is treated identically to "file doesn't exist," so the very next line unconditionally writes {} over whatever was there.
platform.ts's loadJsonConfig still hands JSON.parse's result straight to inMemoryStorage with none of the null/non-object/array validation this PR added for the file-backed path. It has a real caller: the Azure DevOps pipeline extension (ksm-azure-devops-secrets-task/index.ts:163,174) passes pipeline-supplied config straight through it. Separately, the "is this a plain object" predicate this PR adds once in localConfigStorage.ts already exists, unfactored, four times in keeper.ts; a shared helper would have made the gap in loadJsonConfig harder to leave open.
cachingPostFunction (same file, untouched by this PR) still truncates cache.dat before writing, the same non-atomic pattern this PR just fixed for the config file. A crash mid-write leaves a zero-byte cache file, which the fallback then treats as valid cached data, since an empty Buffer is truthy in JavaScript, fabricating a 200 response with empty data instead of surfacing the real error.
The core fix is sound for what it targets, but the symlink/hard-link and directory-permission regressions below should be resolved before merge, and the createConfigFileIfMissing finding above deserves its own ticket regardless of what happens here.
| // Rename takes on the source file's mode, not the destination's, so the temp file | ||
| // must already be 0600 before the rename. | ||
| chmodSecure(tmpPath) | ||
| fs.renameSync(tmpPath, configName) |
There was a problem hiding this comment.
Correctness (new regression): fs.renameSync(tmpPath, configName) replaces configName's directory entry outright rather than writing through it. If configName is a symlink (an externally-managed "current config" link) or has a hard link elsewhere, standard POSIX rename semantics mean the symlink is replaced by a disconnected regular file, or the hard-linked path is left permanently pointing at the old inode's content. Confirmed with an inode comparison before and after a save: the old truncate-in-place write preserved the inode, this one doesn't. Worth deciding explicitly whether this SDK intends to support a symlinked config path (some container deployments lean on exactly this convention) rather than breaking it silently.
| fs.writeSync(fd, JSON.stringify(storageData, null, 2)) | ||
| } finally { | ||
| fs.closeSync(fd) | ||
| const fd = fs.openSync(tmpPath, 'w', 0o600) |
There was a problem hiding this comment.
Correctness (new regression): creating tmpPath here needs write and execute permission on the config file's containing directory, where the old fs.openSync(configName, 'w', ...) only needed write permission on the file itself. Confirmed with a scratch-dir repro: directory mode 0500, file mode 0600, the old code and a plain read both succeed, this one throws EACCES. Any deployment following a least-privilege pattern (writable file, locked-down directory) now fails every save after upgrading, where it used to succeed.
| fs.closeSync(fd) | ||
| const fd = fs.openSync(tmpPath, 'w', 0o600) | ||
| try { | ||
| fs.writeSync(fd, JSON.stringify(storageData, null, 2)) |
There was a problem hiding this comment.
Minor: storage (this function's own parameter) is never read in the body; correctness relies entirely on the outer storageData variable being mutated by reference. It shadows the outer storage: KeyValueStorage declared above. Harmless today, but a future edit here that means to reach the outer storage would silently resolve to this any-typed parameter instead and fail only at runtime, not at compile time.
| try { | ||
| fs.writeSync(fd, JSON.stringify(storageData, null, 2)) | ||
| } finally { | ||
| fs.closeSync(fd) |
There was a problem hiding this comment.
Minor: if fs.writeSync above throws and this fs.closeSync(fd) also throws (e.g. EBADF, or a deferred flush error on some network filesystems), finally's semantics mean only the closeSync error reaches the outer catch; the original writeSync failure is gone. An operator would chase a file-descriptor problem when the real cause was, say, a full disk.
| } | ||
| // Rename takes on the source file's mode, not the destination's, so the temp file | ||
| // must already be 0600 before the rename. | ||
| chmodSecure(tmpPath) |
There was a problem hiding this comment.
Robustness: no fs.fsyncSync(fd) between the write and the rename, so the atomicity this block is built around still doesn't survive a true power-loss or kernel-panic style crash, only a killed process. Without it, the filesystem can journal the rename's directory-entry change before the temp file's data pages are actually flushed, which can leave configName pointing at stale or truncated content on unclean-shutdown recovery, the exact scenario the CHANGELOG entry for this change describes guarding against.
| // Best-effort cleanup - the write failure below is the error that matters; a | ||
| // leftover temp file is cosmetic, it's never read back. | ||
| } | ||
| throw new KeeperError(`Unable to save local config ${configName}: ${describeCause(e)}`) |
There was a problem hiding this comment.
Correctness: by the time this throws, the caller's in-memory value has already been committed; saveString/saveBytes/delete all call the storage mutation before saveStorage. Confirmed: force openSync to fail, saveString correctly rejects and the on-disk file is correctly untouched, but a subsequent getString on the same live instance still returns the new value that was never persisted. A caller that catches this exactly as intended, then keeps running, is acting on a value that reverts on the next restart.
| // Best-effort cleanup - the write failure below is the error that matters; a | ||
| // leftover temp file is cosmetic, it's never read back. | ||
| } | ||
| throw new KeeperError(`Unable to save local config ${configName}: ${describeCause(e)}`) |
There was a problem hiding this comment.
Robustness: this wraps the original fs error in a message-only KeeperError, so the .code (EACCES, ENOSPC, EROFS, ...) is gone. KeeperError's constructor only takes a string; KeeperCryptoError elsewhere in errors.ts already solves this identical need with a structured failure reason "so callers don't have to string-match." A caller wanting to retry on ENOSPC but alert immediately on EACCES can't, today.
| } | ||
| throw new KeeperError(`Unable to read local config ${configName}: ${describeCause(e)}`) | ||
| } | ||
| // A process killed mid-write by saveStorage (or by some other writer entirely) leaves |
There was a problem hiding this comment.
Minor (doc drift): this comment still attributes the empty-file case to "a process killed mid-write by saveStorage." After this same commit's atomic-rename change, saveStorage can no longer leave configName itself empty; only a fully-written temp file ever gets renamed over it. Worth updating so a future reader doesn't conclude the atomic-write fix is incomplete.
|
|
||
| test().finally() | ||
| test().catch((e) => { | ||
| console.error(`quicktest failed: ${e.message}`) |
There was a problem hiding this comment.
Minor: unlike the 8 shipped examples this same commit fixed to use e?.message ?? String(e), this one still reads e.message unguarded. A non-Error rejection here throws fresh from inside this handler itself, becoming an unhandled rejection that masks the original failure.
| main().catch((e) => { | ||
| console.error(`Failed to run folders example: ${e?.message ?? String(e)}`) | ||
| process.exitCode = 1 | ||
| }).finally() |
There was a problem hiding this comment.
Nit: the trailing .finally() with no callback is inert now that .catch() above does the real work; it's a leftover from the pre-fix main().finally() pattern. Same in notation/hello.js:40, pam-linked-records/hello.js:50, and totp/hello.js:42. file-upload/hello.js shows the alternative: it repurposed its .finally() to call process.exit().
…lize concurrent saves (KSM-1266) The round-4 atomic-write fix (temp file + rename) broke two deployment patterns the old truncate-in-place write never touched: renameSync replaces a symlinked config path instead of writing through it, and it detaches a hard-linked config path into a new inode, leaving the other link frozen at the old content. writeConfigFile now resolves the config path via realpathSync first (preserving a symlink, matching the approach the write-file-atomic package uses) and, for a hard-linked path specifically (detected via nlink > 1, which realpathSync can't help with), writes in place instead of through a temp file, trading away atomicity only for that one file rather than breaking hard-link semantics for every config. Atomic writes now also fsync before renaming, so the fix survives a real power-loss event and not only a killed process; the directory-permission requirement this adds (write+execute, not just file-level write) is inherent to atomic-rename and is documented in the CHANGELOG rather than worked around. A SIGKILL between opening the temp file and the rename now gets swept up on the next read (or immediately, on a write failure that isn't a rename failure) instead of leaving a secrets-bearing temp file on disk indefinitely; the sweep is age-gated (60s) rather than PID-liveness-gated, since PIDs aren't visible across the container/pod boundary this matters most for. saveString/saveBytes/delete now snapshot storageData before mutating and roll back on a failed persist, so a failed save no longer leaves the live instance disagreeing with what's on disk. That snapshot/rollback is now serialized per instance (a promise chain), closing a race where two overlapping calls could interleave such that one call's rollback erased a different, already-successful call's mutation. Also: fs.constants.O_NOFOLLOW is undefined on Windows, so the hard-link write path falls back to an explicit lstatSync symlink check there instead of silently losing that protection; fs.writeSync's return value is no longer discarded, a short write is retried until the full buffer lands instead of fsyncing and renaming truncated content; the temp file open uses O_EXCL; localConfigStorage now throws the new KeeperStorageError (extends KeeperError) with the original fs error code where one exists; and the four examples updated in the previous commit for a construction- time throw had a dead .finally() left over from before that fix.
mgallego-keeper
left a comment
mgallego-keeper
left a comment
There was a problem hiding this comment.
This is round 5 of review on this PR.
The new commit, cd0e2af, fixes round 4's two regressions and the in-memory-versus-disk mismatch bug, for the simple case: one writer, one instance, one clean failure.
Empirical testing, using real symlinks, real hard links, a real named pipe, a real process kill, and two real concurrent processes, found 13 more issues in the new code. Two of them are as severe as the round 4 regressions:
- The hard-link write path, added this round, has no backup of its own. A write or truncate failure partway through permanently corrupts the file, with no way to recover it.
- In that same path, a chmod or close failure after the actual write already succeeded still triggers a full rollback. Disk ends up with the correct value. Memory rolls back to the old value. A later save then silently overwrites the correct disk value with the stale one.
Each finding is posted as its own inline comment below, at the line it applies to.
Two more items are open for a third straight review round, in code this PR does not touch: cachingPostFunction's fake-200-from-empty-cache pattern, and platform.ts's loadJsonConfig missing the same JSON-shape check this PR added elsewhere. Both are being filed as their own JIRA tickets, separate from this PR.
cachingPostFunction (sdk/javascript/packages/core/src/node/localConfigStorage.ts, line 375): it reads a local cache.dat file when a network request fails, and checks the result with the expression if (!cachedData). An empty Buffer is truthy in JavaScript, so this check does not catch an empty file. A write failure can truncate cache.dat to 0 bytes. After that, cachingPostFunction returns a fake 200 response with empty data, instead of throwing its own error.
loadJsonConfig (sdk/javascript/packages/core/src/platform.ts, line 79): it passes a parsed JSON value straight to inMemoryStorage, with no check on its shape. readStorage in localConfigStorage.ts added exactly this kind of check in this same PR, for the file-based config path. loadJsonConfig did not get the same check. There is a confirmed live caller: integration/keeper_secrets_manager_azure_pipeline_extension/ksm-azure-devops-secrets-task/index.ts, lines 163 and 174.
Ticket filing for both is delayed right now by an unrelated JIRA outage. I will follow up with the ticket numbers once they exist.
Requesting changes again, given the two new severe findings above.
| : fs.constants.O_RDWR | ||
| const fd = fs.openSync(resolvedPath, openFlags) | ||
| try { | ||
| const bytesWritten = writeFullySync(fd, data) |
There was a problem hiding this comment.
The hard-link write path has no backup copy. It writes new data directly into the only copy of the file.
A write failure partway through leaves torn data on disk. Part of the file has new content. Part of the file has old content.
This happens if fs.writeSync fails after a partial write. It also happens if fs.ftruncateSync fails right after a full write.
The result is permanent, unrecoverable damage to the config file. No hard link recovers a good copy, because they all point at the same damaged file.
I confirmed this by forcing each failure and reading the corrupted file back.
| } | ||
| throw writeError | ||
| } | ||
| fs.closeSync(fd) |
There was a problem hiding this comment.
In the hard-link write path, fs.closeSync and chmodSecure run after the actual write already succeeded and was flushed to disk.
Neither call is inside a try/catch block here.
If either one fails, saveStorage still throws. The caller's code then rolls the in-memory value back to the old value.
But disk already has the new, correct value at that point. So disk and memory now disagree, in the unsafe direction.
A later successful save writes the stale in-memory value back to disk. This silently overwrites the value that was already correct.
I confirmed this by forcing fs.chmodSync to fail right after a successful write.
| const openFlags = typeof fs.constants.O_NOFOLLOW === 'number' | ||
| ? fs.constants.O_RDWR | fs.constants.O_NOFOLLOW | ||
| : fs.constants.O_RDWR | ||
| const fd = fs.openSync(resolvedPath, openFlags) |
There was a problem hiding this comment.
The hard-link write path has no lock. Two real, concurrent writers can write to the same file at the same time.
This can tear the JSON content into an invalid mix of both writers' data. No crash or failure is needed to trigger this.
The other write path, using a temp file and rename, does not have this problem, since a rename is atomic on POSIX systems.
The commit message says the hard-link path trades away crash safety. It does not mention that it also trades away safety from concurrent writers.
| // applied mutation even though B never failed. Chaining every call onto pendingOperation | ||
| // means each call's entire snapshot-mutate-persist(-rollback) sequence fully finishes before | ||
| // the next one's snapshot is even taken, so this can no longer happen. | ||
| let pendingOperation: Promise<void> = Promise.resolve() |
There was a problem hiding this comment.
The new pendingOperation queue only serializes calls within one localConfigStorage instance, in one process.
Two separate instances, or two separate operating system processes, can still write to the same config file at the same time.
I confirmed this with two real Node.js processes. Both saveString calls succeeded with no error. Only one process's write survived on disk. The other process's update was silently lost.
This is the same multi-pod scenario that this file's own code comment names as a real case, for the orphan-file cleanup logic above. The save path has no matching protection.
| let resolvedPath = configName | ||
| let stat: fs.Stats | undefined | ||
| try { | ||
| resolvedPath = fs.realpathSync(configName) |
There was a problem hiding this comment.
A dangling symlink is a symlink whose target does not exist yet. This is a valid setup, used to pre-provision a config path before its target file exists.
fs.realpathSync throws an ENOENT error for a dangling symlink. The code cannot tell this apart from a plain missing file.
So the code falls back to renaming a new temp file directly onto the symlink's own path. This replaces the symlink with a normal file. It does not write through the symlink.
This is the exact problem this round's fix is meant to solve, for this one specific case.
I confirmed this by checking that the symlink was gone after the first save.
| // into a plausible-looking but wrong parsed value. It also strips a leading BOM per | ||
| // the WHATWG Encoding spec's default ignoreBOM:false for the 'utf-8' label, so no | ||
| // separate BOM-stripping step is needed. | ||
| raw = new TextDecoder('utf-8', {fatal: true}).decode(fs.readFileSync(configName)) |
There was a problem hiding this comment.
A FIFO, also called a named pipe, at the config path makes fs.readFileSync block forever inside readStorage.
This freezes the whole process at construction time, since all file operations here are synchronous.
This round added clear handling for symlinks and hard links at this same config path. It added no handling for a FIFO.
Only a forced process kill recovers from this state. There is no timeout and no error.
| const suffix = '.tmp' | ||
| const now = Date.now() | ||
| for (const entry of entries) { | ||
| if (!entry.startsWith(prefix) || !entry.endsWith(suffix)) { |
There was a problem hiding this comment.
The cleanupOrphanedTempFiles function matches file names using a case-sensitive prefix check.
fs.realpathSync does not change the case of a path, on a case-insensitive file system. macOS and Windows both default to case-insensitive file systems.
So if one process opens the config using one case, for example config.json, and is killed mid-write, it leaves an orphaned temp file with that same case in its name.
A later process that opens the same config using a different case, for example CONFIG.JSON, computes a different-case prefix. It never matches the existing orphaned file.
That orphaned file, which holds a full copy of every secret, is never cleaned up.
| } | ||
| } | ||
|
|
||
| if (stat && stat.nlink > 1) { |
There was a problem hiding this comment.
The check for a hard link is stat.nlink > 1. A plain directory also has nlink of 2 or more, by design.
So pointing the config path at a directory would also enter the hard-link write branch, which opens the path with read-write access.
Today, this never actually happens, since an earlier step, reading the directory as a file, fails first with an EISDIR error.
But that earlier failure is not a defense built into this check itself. If that earlier read step ever changes, this check would not catch a directory on its own.
| // must already be 0600 before the rename. | ||
| chmodSecure(tmpPath) | ||
| try { | ||
| fs.renameSync(tmpPath, resolvedPath) |
There was a problem hiding this comment.
There is no fsync call on the folder that contains the config file, anywhere in writeConfigFile. There is only an fsync call on the file itself.
The commit message says this now survives a real power-loss event. That claim is broader than what the code actually guarantees.
If power is lost right after the rename call returns, but before the folder's own change reaches the disk, the rename can be lost or left in an inconsistent state on some file systems.
This same limitation exists in the write-file-atomic package on npm, which this code is modeled on. It is a shared limitation, not a new one. But the claim's wording overstates the guarantee.
| // narrower, race-prone check-then-open rather than one atomic syscall, but a real | ||
| // attacker with no timing control at all is still caught, where a silent no-op would | ||
| // have caught nothing. | ||
| if (typeof fs.constants.O_NOFOLLOW !== 'number' && fs.lstatSync(resolvedPath).isSymbolicLink()) { |
There was a problem hiding this comment.
This round added a fallback check for Windows, since Windows has no O_NOFOLLOW flag. The check uses fs.lstatSync to detect a symlink before opening the file.
This fallback branch has no test coverage. The test suite only runs on ubuntu-latest, where O_NOFOLLOW always exists.
I tried to force this branch in a test, by overriding fs.constants.O_NOFOLLOW. This threw an error, because fs.constants is not configurable.
So this specific security-relevant code has never actually run, in this project's test history.
|
Follow-up: filed the two carry-over items as their own tickets, now that JIRA is available again. KSM-1384: cachingPostFunction returns a fake success from an empty cache file. Both link to KSM-1266. |
…n scope (KSM-1266)
2 participants
Summary
JavaScript SDK: fixes
localConfigStoragetreating every config-read failure as "no config yet," and hardens the config file's write path against symlink/hard-link mishandling, concurrent-save races, and partial writes.Changes
Fixed
readStoragecaught every exception from reading or parsing the config file and returned an empty config, masking permission errors and malformed JSON as a fresh start. Only a missing file (ENOENT) is treated that way now; everything else throwsKeeperStorageError(extendsKeeperError), carrying the original filesystem error code (e.g.EACCES,ENOSPC) when one exists. (KSM-1266)null, a number, a string, an array), and a leading UTF-8 BOM, are handled explicitly instead of crashing on first use or being mistaken for corruption.saveStoragewrites to a temporary file and renames it into place atomically, instead of truncating the destination before writing, and fsyncs before the rename so this survives real power loss, not only a killed process.realpathSyncand writes through the real file instead of replacing the symlink, matching how some deployments manage a "current config" symlink externally. A hard-linked config path is written in place instead (detected vianlink > 1), so every hard link still sees the update, trading away atomicity only for that one file. The hard-link write path usesO_NOFOLLOWto reject a symlink swapped in after thenlinkcheck, on platforms where the flag exists; Windows has no equivalent flag, so this protection is currently POSIX-only, the same accepted gap already tracked forO_DIRECTORYon the caching-fallback branch (KSM-1265).saveString/saveBytes/deletesnapshot the in-memory config before mutating and roll back on a failed persist, so a failed save no longer leaves the live instance disagreeing with what's on disk. Concurrent calls on the same instance are now serialized, closing a race where one call's rollback could otherwise erase a different, already-successful call's mutation.Testing
New tests include a real child-process
SIGKILLbetween the temp-file open and rename, a symlink swapped mid-write, a hard-linked config swapped into a symlink, and concurrent saves where one fails and rolls back.Breaking Changes
localConfigStorage(configName)can throwKeeperStorageError(extendsKeeperError) for a config file that exists but is unreadable, malformed, or a non-object JSON value, where it previously started fresh silently. Saving now requires write and execute permission on the config file's directory, not just the file itself (inherent to atomic writes via rename). A hard-linked config path keeps its pre-existing (non-atomic) crash-safety; a symlinked config path is written through rather than replaced. The four shipped Node examples and the internal dev script handle the construction-time throw.Related Issues