fix: restrict permissions before writing pulled secrets

[Fermionic-Lyu]

insta secrets pull writes branch credentials into files readable by other local users under a normal umask, including when overwriting an existing output file.

Create new output with mode 0600. For an existing output, intersect its permissions with owner read/write on the opened file descriptor before truncating or writing credentials. This retains stricter modes and existing symlink/inode behavior while keeping permission changes and writes on the same file.

Closes #240 (also covers the permission defect consolidated from #273).

Validation: reproduced new/existing/symlink output at 0644 before the fix; typecheck and 1,999 tests across 101 files pass. Independent review mutation checks catch missing chmod, widened existing permissions, and missing truncation. Repository has no separate lint/formatter command; diff whitespace and existing TypeScript style checked. Linux and Windows CI required before merge; POSIX mode semantics do not represent Windows ACL enforcement.

First push: 2 files, +47/-3. No command or flag changes.

---

Summary by cubic

Fixes insta secrets pull so pulled secrets are no longer written readable by other local users under a normal umask.

• New output files are created with mode 0600; existing files retain stricter modes by intersecting permissions with owner read/write on the opened descriptor before truncating and writing.
• Preserves existing symlink/inode behavior and adds tests for new, existing, write-only, and symlink output files. Closes [Yun Tianming] src/commands/secrets.ts: secrets pull writes .env at umask default mode #240 (also covers the permission defect consolidated from [Yun Tianming] src/commands/secrets.ts: .env bundle written with umask default mode #273).

^{Written for commit 7ceea42. Summary will update on new commits.}

[agent-zhang-beihai]

Reviewed by Wang Miao

This change makes insta secrets write the pulled .env with owner-only permissions. It opens the file with mode 0o600 and narrows an existing file's mode before truncating it, so the old contents never sit in a file that others can read. The fix works, but it hand-builds something the repository already has: writeFileAtomicSync writes owner-only files, follows symlinks and replaces the file atomically. So I'm requesting changes to use that helper.

Use the repo's writeFileAtomicSync instead of a hand-rolled open/chmod/truncate/write

important · judgement · simplicity · src/commands/secrets.ts:110

src/util.ts:27 already covers what these eight lines do, and covers more:

• It always sets the mode with chmodSync after writing, so the umask can't widen it and a pre-existing mode isn't kept.
• It follows a symlink to its target through resolveThroughSymlink, which also handles dangling links. The new test checks the symlink case.
• It is the helper the repo already uses for other owner-only credential files: the compute store, known_hosts and ~/.ssh/config at compute.ts:955,1227,1452,1511.

One call would replace the whole open/chmod/truncate/writeFile/close block:

writeFileAtomicSync(out, serializeEnv(bundle), { mode: 0o600 })

That call also removes the mode & 0o600 masking. Today that masking keeps a write-only 0o200 file at 0o200, which is a case nobody asked for and which the test now has to special-case.

The helper also closes a gap the hand-rolled version leaves open. Right now truncate(0) runs before the write, so a full disk or a crash in between leaves an empty .env. The helper writes to a temporary file and renames it over the target, so a reader sees either the old file or the new one, never an empty one.

Evidence

read-the-code — src/commands/secrets.ts:96-120, src/util.ts:9-75, src/commands/compute.ts:955,1227,1452,1511 (call sites via grep), test/secrets-permissions.test.ts:1-37. I couldn't run the test locally because node_modules isn't installed.

[Fermionic-Lyu]

Deferred on review scope/behavior choice; PR #341 remains open.

The implementation restricts the opened file descriptor before writing, preserves stricter existing permissions (e.g. 0200), and preserves current inode/symlink behavior. Typecheck and all 1,999 local tests passed. Independent review found no production defect.

Wang Miao requests replacing it with writeFileAtomicSync(..., { mode: 0o600 }). That helper exists and provides atomic replacement, but it is not behavior-equivalent: it replaces the inode and forces 0600, including widening a stricter 0200 output. The original #240 explicitly asks to consider retaining stricter output modes. Atomic replacement could also change hard-link behavior; the current write follows the existing target/inode contract.

The concrete choice is whether secrets output should retain stricter permissions and inode behavior, or adopt the repository's atomic-replacement helper with its fixed-mode contract. This cleanup batch will not settle that disagreement or expand the shared helper. Leaving the issue/PR open, recording the options here, and moving on without another review round or merge.

[agent-zhang-beihai]

Reviewed by Yang Dong

This ensures pulled secrets are written only after the destination is restricted to owner permissions, while preserving symlinks and stricter existing modes. The implementation is correct and appropriately tested, so I would approve it.

No findings.

[cubic-dev-ai]

2 issues found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/commands/secrets.ts">

<violation number="1" location="src/commands/secrets.ts:112">
P2: This unconditionally chmods non-regular output targets such as `/dev/null`, so a common discard destination can fail or have its device permissions changed. Restrict the chmod to regular files or reject non-regular destinations.</violation>
</file>

<file name="test/secrets-permissions.test.ts">

<violation number="1" location="test/secrets-permissions.test.ts:13">
P3: The symlink case is skipped unconditionally on Windows before any setup runs, so the fix's core behavior — rewriting credentials through an existing symlink without replacing it — is never exercised in the Windows CI job. GitHub Actions windows-latest runners run as an admin that can create symlinks; try the symlink and skip only when creation actually fails (e.g., EPERM on non-privileged dev machines), keeping the symlink target and lstat assertions on runners that support it.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: This unconditionally chmods non-regular output targets such as /dev/null, so a common discard destination can fail or have its device permissions changed. Restrict the chmod to regular files or reject non-regular destinations.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/commands/secrets.ts, line 112:

<comment>This unconditionally chmods non-regular output targets such as `/dev/null`, so a common discard destination can fail or have its device permissions changed. Restrict the chmod to regular files or reject non-regular destinations.</comment>

<file context>
@@ -107,7 +107,14 @@ export async function secrets(
-  await writeFile(out, serializeEnv(bundle))
+  const file = await open(out, constants.O_WRONLY | constants.O_CREAT, 0o600)
+  try {
+    await file.chmod((await file.stat()).mode & 0o600)
+    await file.truncate(0)
+    await file.writeFile(serializeEnv(bundle))
</file context>

Suggested change

	await file.chmod((await file.stat()).mode & 0o600)
	const stats = await file.stat()
	if (stats.isFile()) await file.chmod(stats.mode & 0o600)

[cubic-dev-ai]

P3: The symlink case is skipped unconditionally on Windows before any setup runs, so the fix's core behavior — rewriting credentials through an existing symlink without replacing it — is never exercised in the Windows CI job. GitHub Actions windows-latest runners run as an admin that can create symlinks; try the symlink and skip only when creation actually fails (e.g., EPERM on non-privileged dev machines), keeping the symlink target and lstat assertions on runners that support it.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/secrets-permissions.test.ts, line 13:

<comment>The symlink case is skipped unconditionally on Windows before any setup runs, so the fix's core behavior — rewriting credentials through an existing symlink without replacing it — is never exercised in the Windows CI job. GitHub Actions windows-latest runners run as an admin that can create symlinks; try the symlink and skip only when creation actually fails (e.g., EPERM on non-privileged dev machines), keeping the symlink target and lstat assertions on runners that support it.</comment>

<file context>
@@ -0,0 +1,37 @@
+  { name: 'existing write-only file', mode: 0o200, linked: false },
+  { name: 'symlink target', mode: 0o644, linked: true },
+])('writes private secret output: $name', async ({ mode, linked }) => {
+  if (linked && process.platform === 'win32') return
+  const dir = mkdtempSync(join(tmpdir(), 'insta-secrets-permissions-'))
+  const cwd = process.cwd()
</file context>