diff --git a/packages/cli/src/utils/__tests__/credential-output.test.ts b/packages/cli/src/utils/__tests__/credential-output.test.ts index bff969a..a45fc57 100644 --- a/packages/cli/src/utils/__tests__/credential-output.test.ts +++ b/packages/cli/src/utils/__tests__/credential-output.test.ts @@ -47,4 +47,56 @@ describe('writeCredentialFile', () => { const result = await writeCredentialFile(filePath, {}, false); expect(path.isAbsolute(result)).toBe(true); }); + + // The output file holds full card credentials. A pre-existing symbolic + // link at the user's --output-file path lets an attacker on a shared + // filesystem redirect the credential write through the link target. + // Refusing to follow the link closes the cross-user exfiltration vector. + // --force does not authorize symlink replacement: an operator who wants + // to overwrite a regular output file gets that, but a symlink at the + // path is rejected outright in either mode. + + it('refuses to write through a symbolic link without force', async () => { + const filePath = path.join(tmpDir, 'card.json'); + const targetPath = path.join(tmpDir, 'target.json'); + await fs.writeFile(targetPath, '{}'); + await fs.symlink(targetPath, filePath); + + await expect( + writeCredentialFile(filePath, { num: 1 }, false), + ).rejects.toThrow('OUTPUT_FILE_SYMLINK'); + // The symlink target must not have been written. + expect(await fs.readFile(targetPath, 'utf-8')).toBe('{}'); + // The symlink itself must still be in place. + expect((await fs.lstat(filePath)).isSymbolicLink()).toBe(true); + }); + + it('refuses to overwrite a symbolic link even with --force', async () => { + const filePath = path.join(tmpDir, 'card.json'); + const targetPath = path.join(tmpDir, 'target.json'); + await fs.writeFile(targetPath, '{}'); + await fs.symlink(targetPath, filePath); + + // --force is intended to replace an existing regular output file. It + // does not authorize destroying a symlink the operator may not have + // intended to remove, nor writing credentials over its target. + await expect( + writeCredentialFile(filePath, { num: 1 }, true), + ).rejects.toThrow('OUTPUT_FILE_SYMLINK'); + // The symlink target must not have been written. + expect(await fs.readFile(targetPath, 'utf-8')).toBe('{}'); + // The symlink itself must still be in place. + expect((await fs.lstat(filePath)).isSymbolicLink()).toBe(true); + }); + + it('produces a 0o600 file even when force is set', async () => { + // Mode is set at create time via the open() third argument, with + // O_EXCL guaranteeing the file is created here. Validates the mode + // path explicitly because the legacy chmod-after-write step is gone. + const filePath = path.join(tmpDir, 'card.json'); + await fs.writeFile(filePath, 'old', { mode: 0o644 }); + await writeCredentialFile(filePath, { x: 1 }, true); + const stat = await fs.stat(filePath); + expect(stat.mode & 0o777).toBe(0o600); + }); }); diff --git a/packages/cli/src/utils/credential-output.ts b/packages/cli/src/utils/credential-output.ts index 572e7df..22658f9 100644 --- a/packages/cli/src/utils/credential-output.ts +++ b/packages/cli/src/utils/credential-output.ts @@ -1,3 +1,4 @@ +import { constants, type Stats } from 'node:fs'; import fs from 'node:fs/promises'; import path from 'node:path'; @@ -8,20 +9,68 @@ export async function writeCredentialFile( ): Promise { const resolved = path.resolve(filePath); - if (!force) { - try { - await fs.access(resolved); + // Atomically create the credential file so we never write through a + // pre-existing output path. lstat first to inspect what is there: a + // symlink at the final path component is rejected outright (even with + // --force), since --force is meant to replace an existing output file, + // not to authorize writing credentials over or through a symlink. A + // regular file is overwritten only when --force is set. The atomic + // create below uses O_EXCL | O_NOFOLLOW so the race window between the + // precheck and the create is fail-closed. + let existing: Stats | undefined; + try { + existing = await fs.lstat(resolved); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + } + + if (existing?.isSymbolicLink()) { + throw new Error( + `OUTPUT_FILE_SYMLINK: ${resolved} is a symbolic link. Refusing to write credentials through it.`, + ); + } + + if (existing) { + if (!force) { throw new Error( `OUTPUT_FILE_EXISTS: ${resolved} already exists. Use --force to overwrite.`, ); - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; } + await fs.unlink(resolved); } - await fs.writeFile(resolved, JSON.stringify(data, null, 2), { - mode: 0o600, - }); - await fs.chmod(resolved, 0o600); + // O_NOFOLLOW is POSIX-only. On Windows, Node exposes it as 0, so the + // bitwise OR is a no-op there: O_EXCL still prevents overwriting a + // pre-existing entry, but open() does not provide full no-follow + // semantics on Windows. + const noFollowFlag = constants.O_NOFOLLOW ?? 0; + + let handle: fs.FileHandle | undefined; + try { + handle = await fs.open( + resolved, + constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY | noFollowFlag, + 0o600, + ); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code === 'EEXIST') { + throw new Error( + `OUTPUT_FILE_EXISTS: ${resolved} already exists. Use --force to overwrite.`, + ); + } + if (code === 'ELOOP') { + throw new Error( + `OUTPUT_FILE_SYMLINK: ${resolved} is a symbolic link. Refusing to write credentials through it.`, + ); + } + throw err; + } + + try { + await handle.write(JSON.stringify(data, null, 2)); + } finally { + await handle.close(); + } return resolved; }