[Med] fix(cli): make writeCredentials atomic and enforce 0600 on pre-existing files by katnisscalls99 · Pull Request #712 · profullstack/sh1pt

katnisscalls99 · 2026-06-06T14:47:09Z

Two defects in writeCredentials():

Non-atomic write. fs.writeFile() writes in-place; a kill/OOM mid-write leaves credentials.json truncated. The next readCredentials() hits JSON.parse on garbage and silently returns null (logged out, no explanation). local-vault.ts/writeVault() — which protects the same config dir — already uses the tmp+rename pattern; credentials.ts should match it.
mode: 0o600 only applies on file creation. If the file pre-exists at a looser mode (e.g. 0644 from an older sh1pt version that omitted mode, or from a cp/touch by the user), fs.writeFile() preserves that mode. local-vault.ts/writeVault() explicitly calls chmod() after rename to handle this case; credentials.ts does not.

Fix: mirror writeVault() exactly — write to a .tmp sibling, rename atomically, then chmod(0o600) the destination. Also pass mode: 0o700 to mkdir() so the containing directory is not world-traversable.

Severity: Medium

…ng files Two defects in writeCredentials(): 1. Non-atomic write. fs.writeFile() writes in-place; a kill/OOM mid-write leaves credentials.json truncated. The next readCredentials() hits JSON.parse on garbage and silently returns null (logged out, no explanation). local-vault.ts/writeVault() — which protects the same config dir — already uses the tmp+rename pattern; credentials.ts should match it. 2. mode: 0o600 only applies on file creation. If the file pre-exists at a looser mode (e.g. 0644 from an older sh1pt version that omitted mode, or from a cp/touch by the user), fs.writeFile() preserves that mode. local-vault.ts/writeVault() explicitly calls chmod() after rename to handle this case; credentials.ts does not. Fix: mirror writeVault() exactly — write to a .tmp sibling, rename atomically, then chmod(0o600) the destination. Also pass mode: 0o700 to mkdir() so the containing directory is not world-traversable.

greptile-apps · 2026-06-06T14:49:48Z

Greptile Summary

This PR fixes two defects in writeCredentials() by mirroring the already-correct writeVault() pattern from local-vault.ts: a non-atomic in-place write is replaced with a tmp-file + rename sequence, and an explicit chmod(0o600) is added after rename to handle files that pre-existed with looser permissions.

Atomic write: credentials.json is now written to credentials.json.tmp and renamed into place, preventing a truncated/corrupt file on a mid-write crash.
Permission enforcement: chmod(0o600) is called unconditionally after rename, closing the gap where an older-version or user-touched file could retain a loose mode like 0o644 even after rewrite.
Directory hardening: mkdir() now receives mode: 0o700, preventing the config directory from being world-traversable.

Confidence Score: 4/5

Safe to merge — the core fix is correct and consistent with the existing local-vault.ts pattern, with only minor comment and error-surfacing gaps.

The atomic write and chmod are both functionally correct. The two gaps — a misleading code comment (describing the wrong rename-mode scenario) and silently swallowing chmod failures — don't affect correctness under normal conditions but could confuse future maintainers or hide permission problems on unusual systems.

Only packages/cli/src/credentials.ts changed. The comment at lines 55–57 and the bare .catch(() => {}) on line 58 are the areas worth a second look.

Important Files Changed

Filename	Overview
packages/cli/src/credentials.ts	Adds atomic write (tmp+rename) and post-rename chmod to writeCredentials(), matching the writeVault() pattern in local-vault.ts; also adds mode: 0o700 to mkdir(). The implementation is functionally correct, though the comment explaining why chmod is needed after rename describes the wrong scenario, and chmod errors are silently swallowed.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant writeCredentials
    participant FS as node:fs

    Caller->>writeCredentials: writeCredentials(creds)
    writeCredentials->>FS: "mkdir(configDir, {recursive, mode:0o700})"
    FS-->>writeCredentials: ok
    writeCredentials->>FS: "writeFile(credentials.json.tmp, json, {mode:0o600})"
    FS-->>writeCredentials: ok
    writeCredentials->>FS: rename(credentials.json.tmp → credentials.json)
    note over FS: atomic on same filesystem
    FS-->>writeCredentials: ok
    writeCredentials->>FS: chmod(credentials.json, 0o600)
    note over writeCredentials: error silently swallowed
    FS-->>writeCredentials: ok / EPERM (ignored)
    writeCredentials-->>Caller: void

_{Reviews (1): Last reviewed commit: "fix(cli): make writeCredentials atomic a..." | Re-trigger Greptile}

greptile-apps · 2026-06-06T14:49:51Z

+  // rename(2) preserves the source mode, but if the destination pre-existed
+  // at a looser mode (e.g. 0644 from an older sh1pt build), the resulting
+  // file keeps that loose mode. Explicitly tighten after rename.
+  await fs.chmod(path, 0o600).catch(() => {});


Inaccurate comment about rename semantics

The comment says "if the destination pre-existed at a looser mode, the resulting file keeps that loose mode" — this is incorrect. rename(2) atomically replaces the destination with the source inode, so the result always gets the source (.tmp) file's permissions, not the destination's. The actual scenario chmod guards against is a stale .tmp: if a previous invocation crashed after writeFile but before rename, the .tmp file persists, and because fs.writeFile's mode option is only applied on file creation (not truncation of an existing file), a subsequent run's writeFile call won't tighten that file's mode — so after rename the destination inherits the stale mode. The chmod call itself is correct; only the explanation is wrong. The same inaccuracy was copied from local-vault.ts, so both should be corrected together.

greptile-apps · 2026-06-06T14:49:52Z

+  // rename(2) preserves the source mode, but if the destination pre-existed
+  // at a looser mode (e.g. 0644 from an older sh1pt build), the resulting
+  // file keeps that loose mode. Explicitly tighten after rename.
+  await fs.chmod(path, 0o600).catch(() => {});


Silent chmod failure hides permission errors

.catch(() => {}) silently swallows any chmod error. If chmod fails with EPERM (e.g., the file ends up owned by another user due to a misconfigured setuid wrapper, or an unusual container UID mapping), credentials.json is written successfully but with whatever mode the .tmp file had — potentially world-readable — and the caller receives no error. The same pattern exists in local-vault.ts so this is consistent, but both would benefit from at least logging the failure (e.g., console.warn) so a user running sh1pt login on a hardened system gets some signal rather than silent credential exposure.

github-actions · 2026-06-06T16:09:53Z