JS SDK GCP KMS Storage: don't recreate the config file on a non-ENOENT fs.access error

[stas-schaller]

Summary

JavaScript SDK GCP KMS Storage's createConfigFileIfMissing() treated any fs.access failure the same as a missing file, so a transient permission or stale-handle error silently overwrote the real on-disk config with an empty one.

Changes

Fixed

• createConfigFileIfMissing() now checks error.code === 'ENOENT' before recreating the config file. Any other fs.access error (EACCES, EPERM, ESTALE, etc.) now propagates instead of triggering a destructive rewrite. (KSM-1370)

Testing

cd sdk/javascript/packages/gcp
npm test

Breaking Changes

None.

Related Issues

• Jira: KSM-1370

[mgallego-keeper]

This is a security review of the KSM-1370 fix. The fix is correct. I want to merge it. Two items block the merge. One larger point also applies: this exact pattern already exists in core.

What is correct

createConfigFileIfMissing() now recreates the config file only when fs.access reports ENOENT. This is the fix that KSM-1370 asked for. The destructive path in the ticket is now closed. Three details are correct:

• It fails closed for a throwable with no code property, because undefined never equals "ENOENT".
• It does not break normal first-run creation. I checked the error codes on a real file system. A missing file under an existing directory reports ENOENT. A missing parent directory also reports ENOENT. Both normal first-run cases still create the config file.
• Hoisting configPath above the try block and deleting the duplicate declaration is a real cleanup.

One point about how the fix is described. A permanent EACCES error fails the write too, not only the probe. So a permanent EACCES never destroyed the config. The real trigger is a transient error. Three examples: ESTALE or EIO on an NFS or EFS volume; EIO from gcsfuse during a consistency window; or a Windows antivirus driver. That driver can fail the attribute read, then let the write succeed a moment later. The two calls are separated by an await. So a transient failure alone is enough to trigger the bug. This makes the bug more likely in practice, not less. The fix is worth shipping.

Empirically confirmed, not just reasoned

• I fetched the full PR-head package. I ran npm ci and the real jest suite. All 33 tests passed.
• I changed throw error; to return;. This is the exact regression where the code swallows the error instead of propagating it. I reran the suite. Both new tests still passed. This confirms the first blocking item below with a real test run, not just by reading the assertion.
• I wrote the replacement test shown below. It calls the public method storage.init() and asserts .rejects.toMatchObject({ code: 'EACCES' }). I ran it against the same broken code. It failed, as expected. I then restored the real fixed source and reran it. It passed. This proves the replacement test catches what the current test misses.
• I ran a real three-way merge (git merge-file --diff3) of this PR against PR #1181 on their shared base, in both merge orders. Both orders produced a real conflict, not a silent bad merge. The conflict sits at the exact write call both PRs edit.

Blocking 1: nothing asserts that the error reaches the caller

KSM-1370's Expected Result field says the failure must surface as an error to the caller. Its Test Instructions field asks the test to go through loadConfig() or saveConfig(). The new test does neither of these things:

await (storage as any).createConfigFileIfMissing().catch(() => undefined);
expect(fs.writeFile).not.toHaveBeenCalled();

.catch(() => undefined) discards the rejection. The only assertion is that no write happened. If you replace throw error; with return;, no write happens either. So this test would still pass on that regression. Propagation is the entire security value of this change. A future regression that swallows the error instead of throwing it would ship silently. The test also calls a private method through (storage as any) instead of a public entry point.

init() reaches loadConfig() once getCryptoKey resolves. So the test below covers both properties at once:

it('init() rejects and writes nothing when fs.access fails with EACCES', async () => {
    const mockClient = mockSessionConfig.getCryptoClient();
    (mockClient.getCryptoKey as jest.Mock).mockResolvedValue([
        { purpose: 'ENCRYPT_DECRYPT', versionTemplate: { algorithm: 'GOOGLE_SYMMETRIC_ENCRYPTION' } },
    ]);
    (fs.access as jest.Mock).mockRejectedValue(
        Object.assign(new Error('EACCES: permission denied'), { code: 'EACCES' }),
    );

    await expect(storage.init()).rejects.toMatchObject({ code: 'EACCES' });
    expect(fs.writeFile).not.toHaveBeenCalled();
});

Two more additions would make the suite defend the general shape, not just one error code. First, run the same case through the save path. For example: await expect(storage.saveString('clientId', 'x')).rejects.toMatchObject({ code: 'EACCES' }). This works because saveConfig() also calls this function. Second, make the test table driven over several codes: EACCES, EPERM, ESTALE, EIO, and EBUSY. As written, a later change to if (error?.code !== "ENOENT" && error?.code !== "EPERM") would reopen the hole for EPERM. The suite would still pass.

Separately, the ENOENT test proves only the plaintext {} write. It does not prove the encrypted write. The mocked encrypt function returns undefined. So encryptBuffer throws at src/utils.ts:199. The second write at line 471 never runs. The .catch() call hides this failure. If someone deleted the encrypted write entirely, this test would still pass. Giving encrypt a resolved value, the way PR #1181's own test does, would let you assert both writes.

Blocking 2: this PR and PR #1181 break each other

PR #1181 targets the same base branch and edits the same function. It routes the writes through a new writeSecureConfigFile() helper, which calls fs.writeFile(path, data, { mode: 0o600 }). I ran a real three-way merge (git merge-file --diff3) of the two PRs against their shared base, in both merge orders. Both orders produce a real git conflict, not a silent bad merge. The conflict is at this exact hunk:

<<<<<<< 1180
      await fs.writeFile(configPath, Buffer.from("{}"));
||||||| base
      const configPath = resolve(this.configFileLocation);
      await fs.writeFile(configPath, Buffer.from("{}"));
=======
      const configPath = resolve(this.configFileLocation);
      await this.writeSecureConfigFile(configPath, Buffer.from("{}"));
>>>>>>> 1181

GitHub reports both PRs as MERGEABLE right now. This is because GitHub diffs each PR against the base branch, not against each other. Whichever PR merges second will hit this conflict with no advance warning. Please rebase before merging.

There is a second problem that will not show up as a merge conflict. Once PR #1181's changes exist in the function, this assertion breaks, even after a correct manual merge. The reason: the assertion sits inside a describe block that this PR owns. Once this PR merges, that block becomes ordinary base content. PR #1181 will then rebase past it with no warning:

expect(fs.writeFile).toHaveBeenCalledWith(expect.any(String), Buffer.from('{}'));

I ran this assertion against a fs.writeFile call that carries #1181's third argument. I used the exact expect version this package pins, 30.2.0. The assertion fails. toHaveBeenCalledWith requires an exact match of the whole argument list:

- Expected
+ Received
  "/abs/test-config.json",
  {"data": [123, 125], "type": "Buffer"},
+ {"mode": 384},

Suggested order: merge this PR first, since it fixes the Critical bug. Then have #1181 rebase and update this assertion to toHaveBeenCalledWith(expect.any(String), Buffer.from('{}'), { mode: 0o600 }). One risk I can rule out: a simple merge resolution does not cause a TypeScript compile error from a duplicate configPath declaration. I checked this against the real compiler, version 5.9.3. The surviving declaration ends up in a nested catch block scope. This is legal variable shadowing, not a redeclaration error. The only real risk is the assertion above. It is a visible jest failure, not a silent gap. Whoever rebases #1181 will see it fail.

Sequencing risk: the release PR does not carry this fix yet

Release PR #1173 targets release/storage/javascript/gcp-kms/v1.0.1 into master. It was opened one day before this PR. Right now it changes only three files: CHANGELOG.md, package.json, and package-lock.json. It contains no source code changes. It will pick up this fix once this PR merges into the release branch. This must happen before #1173 itself merges to master. #1173 is already MERGEABLE. It is blocked only on the one required review that master enforces. Please merge this PR into the release branch before #1173 is approved. Otherwise v1.0.1 ships without the fix, and the team needs a v1.0.2 release instead.

Versioning: this may need more than a PATCH

This change makes init() and saveConfig() throw on a non-ENOENT fs.access failure. Before this change, they swallowed the error and silently reset the config instead. This is a real behaviour change on a path that real callers can reach, even though the old behaviour was the bug. Consider a deployment that tolerated an EACCES error today, because it got a wrongly-reset but working config. That deployment now gets a hard failure after the upgrade. Version 1.0.1 is not yet published. Renaming the branch and package.json to 1.1.0 costs nothing right now. It is also a more honest label for this change. Doing this after #1173 merges would cost much more. Also consider using the changelog's ### Security category for this entry instead of ### Fixed. This is a credential-destruction fix, not an ordinary bug fix.

The bigger point: core already has this pattern

KSM-1370's own ticket pointed at packages/core/src/node/localConfigStorage.ts for the ENOENT idiom. Since then, that file grew into a complete, well-commented solution. It already solves everything this write path still gets wrong:

• isEnoent(), at line 37, does the same check. It types the error as NodeJS.ErrnoException. So it needs no eslint-disable-next-line @typescript-eslint/no-explicit-any comment.
• writeFileAtomic(), at line 124, writes to a temporary file named <path>.<pid>.<random>.tmp. It checks fs.writeSync's return value against the intended byte length. It calls fsync. It sets the temp file's mode to 0600 before the rename, because rename carries the source file's mode, not the destination's. Then it renames the temp file over the target. This rename is atomic on POSIX systems. It replaces the destination file on Windows too.
• resolveWriteTargetPath(), at line 89, walks a dangling symlink chain by hand, up to MAX_SYMLINK_HOPS = 40. Its comment records the design rule. The config path may legitimately be an externally managed symlink. The cache path must never be followed. The comment states this is exactly the arbitrary-file-overwrite problem that KSM-1265's security fix closed.
• chmodSecure(), at line 12, runs again after every write. It uses the same reasoning PR #1181 reached on its own: the mode: option is honored only when the file is first created.

Three consequences for this PR:

1. A truncation race and a symlink write both survive. After the probe reports ENOENT, fs.writeFile(configPath, Buffer.from("{}")) uses the default flag w. This flag means O_CREAT|O_TRUNC. Anything created between the probe and the write gets silently truncated. Also, a dangling symlink at configPath reports ENOENT to fs.access. So the comment "File genuinely does not exist" is not accurate in that case. The write follows the link and creates the link's target. The option { flag: 'wx' } fixes both problems. O_CREAT|O_EXCL rejects with EEXIST instead of truncating. POSIX open(2) also refuses to follow a symlink at the final path component, regardless of its target.
2. Two writes should be one. This branch first writes plaintext {}. It then performs an OAuth token exchange and a KMS call. Only then does it write the encrypted blob. This window lasts tens to hundreds of milliseconds. The file is plaintext on disk for the whole window. A KMS outage during this window leaves a 2-byte plaintext {} config file on disk. Encrypting first and writing only once removes both the window and the race:

   const blob = await encryptBuffer({ /* ... */ }, this.logger);
   await fs.writeFile(configPath, blob, { flag: 'wx', mode: 0o600 });

3. In the longer term, this code should not be written a fourth time. All four cloud backends duplicate this config file logic. This is why KSM-1367 needed four separate child tickets for one defect. Core's helpers are currently module-private. Sharing them needs an export change in core first, not just an import change here. This deserves its own ticket, not a change in this PR.

Non-blocking, in the function you are already editing

• Two bare catch blocks sit eight lines below this fix, in the same shape as the bug. To be fair, they are not a second destructive path. They only run after the probe already reported ENOENT, so there is no live config left to lose. They are still worth deleting. fs.mkdir(dir, { recursive: true }) already succeeds when the directory exists. So the probe and its catch add nothing. The whole block reduces to one line:

  await fs.mkdir(dirname(configPath), { recursive: true });

• The outer catch cannot do what its comment claims. fs.mkdir(process.cwd(), { recursive: true }) is a no-op. Control still falls through to fs.writeFile(configPath, ...) at the same original path. So nothing is ever redirected to the working directory. This code can also make things worse. process.cwd() itself throws ENOENT when the working directory has been deleted. So this handler can replace the real error with a new, unrelated one.
• Line 444 still recomputes dirname(resolve(this.configFileLocation)), even after you hoisted configPath above the try block. It should read dirname(configPath) instead.
• error?.message?.toString() logs the literal text undefined when a throwable has no message. error instanceof Error ? error.message : String(error) gives an exact message instead.
• Typing the catch variable as NodeJS.ErrnoException removes the need for the new no-explicit-any disable comment. It also matches the isEnoent helper in core.
• The new describe block has no afterEach(() => jest.restoreAllMocks()) call. The three regression blocks above it all have one. Also, jest.clearAllMocks() does not clear an implementation set by mockRejectedValue. Nothing leaks today, because this block is last in the file. PR #1181 appends its own block into exactly that slot, so this could leak state next.

Pre-existing, not this PR, and I will file these separately

• loadConfig() treats a zero-length config file as {}. It then re-encrypts that empty config and writes it back, behind only a warn log. A zero-length file is the classic symptom of an interrupted write. Core's own comment on writeFileAtomic names this exact chain as the reason core switched to atomic writes. So KSM-1367 is not fully closed for the GCP package by this PR alone.
• This file has no atomic write, no fsync call, and no rename anywhere. So a SIGKILL signal during a write can produce that zero-length file in the first place.
• saveConfig()'s "no changes detected" early return happens before it calls createConfigFileIfMissing(). So if the file was deleted while the process was running, saveString() still resolves successfully and writes nothing to disk.
• loadConfig() calls saveConfig() from inside its own JSON-detection try block. So a KMS failure during auto-encryption is reported as "may contain JSON format problems". The code then tries to decrypt plaintext JSON, which also fails. An operator sees a message that the config is corrupt, when the real cause was that KMS was unreachable.
• changeKey() restores gcpKeyConfig and cryptoClient on failure. It does not restore keyType, isAsymmetric, or encryptionAlgorithm. getKeyDetails() already overwrote all three by that point; it assigns encryptionAlgorithm before the unsupported-purpose check can throw. I traced a concrete failure case with two asymmetric keys that use different OAEP hash algorithms. A failed key rotation between them leaves the object with the old key and the new key's hash. A later save then wraps a value under the old key, using the new key's hash. I proved this with real RSA keys. The write succeeds. But the resulting file can never be decrypted again. This is a path to permanent credential loss. It deserves its own ticket.
• The environment variable KSM_CONFIG_FILE="" is not treated as unset. The ?? operator only falls back on null and undefined, not on an empty string. resolve("") returns the working directory. fs.access on a directory succeeds. So the storage logs "Config file already exists" for what is really a directory. It then fails later with an empty path in the error message.

Process note, not about your code

This PR targets release/storage/javascript/gcp-kms/v1.0.1, not master. This repo has only one enforcement rule. It is an organisation ruleset scoped to ~DEFAULT_BRANCH, so it applies only to master. Even on master, it requires one review but no status check. Nothing requires a review or a green check before a merge into any release/** branch. So this review is the only real gate for this change.

One more decision is needed outside this PR. KSM-1368 (AWS), KSM-1369 (Azure), and KSM-1371 (Oracle) all describe the same Critical bug in the sibling backends. All three tickets are still in Triage. Their v1.0.1 release PRs, #1174, #1178, and #1179, are all open into master right now. If only the GCP fix ships, three of the four backends still carry the known defect.

[mgallego-keeper]

Closing this PR. Every fix here is already in PR #1181. That PR merged into this same base branch on 2026-09-21, as commit 74e493e.

I checked this directly, not just by reading the diff. I compared this branch's tip against the current release branch tip.

The ENOENT check in createConfigFileIfMissing is present and unchanged in substance. It still checks error.code, and it still re-throws on anything other than ENOENT.

The changelog entry for KSM-1370 is present. It now sits under a combined Security section for version 1.1.0, alongside the KSM-1450 entry.

Test coverage for this fix is now broader than what this PR shipped. This PR tested EACCES only, through the private createConfigFileIfMissing method directly. The merged version tests EACCES, EPERM, ESTALE, EIO, and EBUSY, through the public init() and saveString() methods.

No further action is needed here. The fix is done, tracked under KSM-1370, and already merged.