Skip to content

[Med] fix(cli): make writeCredentials atomic and enforce 0600 on pre-existing files #712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
katnisscalls99 wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −3
Open

[Med] fix(cli): make writeCredentials atomic and enforce 0600 on pre-existing files #712

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions packages/cli/src/credentials.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,18 @@ export async function readCredentials(): Promise<Credentials | null> {

export async function writeCredentials(creds: Credentials): Promise<void> {
const path = credentialsPath();
await fs.mkdir(dirname(path), { recursive: true });
// Mode 0600 — only the user can read.
await fs.writeFile(path, JSON.stringify(creds, null, 2), { encoding: 'utf8', mode: 0o600 });
// Mode 0o700 on the directory — match local-vault.ts writeVault pattern.
await fs.mkdir(dirname(path), { recursive: true, mode: 0o700 });
// Atomic write: write to a tmp file then rename, so a crash mid-write
// never leaves credentials.json truncated/corrupt. Mirrors writeVault() in
// local-vault.ts which holds equally sensitive data.
const tmp = `${path}.tmp`;
await fs.writeFile(tmp, JSON.stringify(creds, null, 2) + '\n', { encoding: 'utf8', mode: 0o600 });
await fs.rename(tmp, path);
// 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(() => {});
Comment on lines +55 to +58
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Jun 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps Bot Jun 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

export async function clearCredentials(): Promise<void> {
Expand Down