Skip to content

fix: restrict permissions before writing pulled secrets #341

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
Fermionic-Lyu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: restrict permissions before writing pulled secrets #341

Show file tree
Hide file tree
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
13 changes: 10 additions & 3 deletions src/commands/secrets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { writeFile } from 'node:fs/promises'
import { appendFileSync, existsSync, readFileSync } from 'node:fs'
import { open } from 'node:fs/promises'
import { appendFileSync, constants, existsSync, readFileSync } from 'node:fs'
import { join } from 'node:path'
import { ApiClient, requireProject } from '../api.js'
import { info, printJson, serializeEnv, handleApproval, die } from '../util.js'
Expand Down Expand Up @@ -107,7 +107,14 @@ export async function secrets(
warnCollisions(b.collisions, branchHint(branch, d.linkedBranch))
if (opts.print) { process.stdout.write(serializeEnv(bundle)); return }
const out = opts.output ?? '.env'
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)

@cubic-dev-ai cubic-dev-ai Bot Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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)

await file.truncate(0)
await file.writeFile(serializeEnv(bundle))
} finally {
await file.close()
}
const scope = opts.service ? `${opts.service}, branch ${branch}` : `branch ${branch}`
info(`wrote ${Object.keys(bundle).length} secrets to ${out} (${scope})`)
if (ensureIgnored(process.cwd(), out)) info(` .gitignore += ${out} (credentials must never be committed)`)
Expand Down
37 changes: 37 additions & 0 deletions test/secrets-permissions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { chmodSync, lstatSync, mkdtempSync, readFileSync, rmSync, statSync, symlinkSync, writeFileSync } from 'node:fs'
import { tmpdir } from 'node:os'
import { join } from 'node:path'
import { expect, it } from 'vitest'
import { secrets } from '../src/commands/secrets.js'

it.each([
{ name: 'new file', mode: undefined, linked: false },
{ name: 'existing readable file', mode: 0o644, linked: false },
{ 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

@cubic-dev-ai cubic-dev-ai Bot Oct 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>

const dir = mkdtempSync(join(tmpdir(), 'insta-secrets-permissions-'))
const cwd = process.cwd()
const target = join(dir, 'secrets.env')
const output = linked ? join(dir, '.env') : target
try {
process.chdir(dir)
if (mode !== undefined) {
writeFileSync(target, 'OLD_SECRET=' + 'old-private-value'.repeat(20) + '\n')
chmodSync(target, mode)
}
if (linked) symlinkSync('secrets.env', output)
await secrets({ output }, {
projectId: 'p1', linkedBranch: 'main',
api: { rawRequest: async () => ({ status: 200, body: { secrets: { PASSWORD: 'private-value' } } }) },
})
if (process.platform !== 'win32') expect(statSync(target).mode & 0o777).toBe(mode === 0o200 ? 0o200 : 0o600)
if (linked) expect(lstatSync(output).isSymbolicLink()).toBe(true)
chmodSync(target, 0o600)
expect(readFileSync(target, 'utf8')).toBe('PASSWORD="private-value"\n')
} finally {
process.chdir(cwd)
rmSync(dir, { recursive: true, force: true })
}
})
Loading