From 9ee34d65fc78ba81b36ba6a990179781bb912590 Mon Sep 17 00:00:00 2001 From: gonzaloriestra <14979109+gonzaloriestra@users.noreply.github.com> Date: Sat, 30 May 2026 00:41:39 +0000 Subject: [PATCH] [Security] Harden permissions of .env files Updated `writeFile` to support an optional `mode` parameter and explicitly call `chmod` to ensure permissions are applied even if the file already exists. Used `mode: 0o600` for `.env` files in `writeDotEnv` and `pullEnv` to prevent unauthorized access to sensitive API secrets. Added regression tests for `writeFile` permissions. --- .../app/src/cli/services/app/env/pull.test.ts | 2 + packages/app/src/cli/services/app/env/pull.ts | 4 +- packages/cli-kit/src/public/node/dot-env.ts | 2 +- packages/cli-kit/src/public/node/fs.test.ts | 44 +++++++++++++++++++ packages/cli-kit/src/public/node/fs.ts | 4 ++ 5 files changed, 53 insertions(+), 3 deletions(-) diff --git a/packages/app/src/cli/services/app/env/pull.test.ts b/packages/app/src/cli/services/app/env/pull.test.ts index c8fd46e2488..6db1b4df28d 100644 --- a/packages/app/src/cli/services/app/env/pull.test.ts +++ b/packages/app/src/cli/services/app/env/pull.test.ts @@ -35,6 +35,7 @@ describe('env pull', () => { expect(file.writeFile).toHaveBeenCalledWith( filePath, 'SHOPIFY_API_KEY=api-key\nSHOPIFY_API_SECRET=api-secret\nSCOPES=my-scope', + {encoding: 'utf8', mode: 0o600}, ) expect(unstyled(stringifyMessage(result))).toMatchInlineSnapshot(` "Created ${filePath}: @@ -61,6 +62,7 @@ describe('env pull', () => { expect(file.writeFile).toHaveBeenCalledWith( filePath, 'SHOPIFY_API_KEY=api-key\nSHOPIFY_API_SECRET=api-secret\nSCOPES=my-scope', + {encoding: 'utf8', mode: 0o600}, ) expect(unstyled(stringifyMessage(result))).toMatchInlineSnapshot(` "Updated ${filePath} to be: diff --git a/packages/app/src/cli/services/app/env/pull.ts b/packages/app/src/cli/services/app/env/pull.ts index 422749dd02e..014934b3a3c 100644 --- a/packages/app/src/cli/services/app/env/pull.ts +++ b/packages/app/src/cli/services/app/env/pull.ts @@ -31,7 +31,7 @@ export async function pullEnv({app, remoteApp, organization, envFile}: PullEnvOp if (updatedEnvFileContent === envFileContent) { return outputContent`No changes to ${outputToken.path(envFile)}` } else { - await writeFile(envFile, updatedEnvFileContent) + await writeFile(envFile, updatedEnvFileContent, {encoding: 'utf8', mode: 0o600}) const diff = diffLines(envFileContent ?? '', updatedEnvFileContent) return outputContent`Updated ${outputToken.path(envFile)} to be: @@ -46,7 +46,7 @@ ${outputToken.linesDiff(diff)} } else { const newEnvFileContent = patchEnvFile(null, updatedValues) - await writeFile(envFile, newEnvFileContent) + await writeFile(envFile, newEnvFileContent, {encoding: 'utf8', mode: 0o600}) return outputContent`Created ${outputToken.path(envFile)}: diff --git a/packages/cli-kit/src/public/node/dot-env.ts b/packages/cli-kit/src/public/node/dot-env.ts index 49394a1ce72..4549ad4cb32 100644 --- a/packages/cli-kit/src/public/node/dot-env.ts +++ b/packages/cli-kit/src/public/node/dot-env.ts @@ -43,7 +43,7 @@ export async function writeDotEnv(file: DotEnvFile): Promise { .map(([key, value]) => createDotEnvFileLine(key, value)) .join('\n') - await writeFile(file.path, fileContent) + await writeFile(file.path, fileContent, {encoding: 'utf8', mode: 0o600}) } /** diff --git a/packages/cli-kit/src/public/node/fs.test.ts b/packages/cli-kit/src/public/node/fs.test.ts index d30612b5b96..ca1e1529843 100644 --- a/packages/cli-kit/src/public/node/fs.test.ts +++ b/packages/cli-kit/src/public/node/fs.test.ts @@ -26,6 +26,7 @@ import { import {joinPath, normalizePath} from './path.js' import * as array from '../common/array.js' import {describe, expect, test, vi} from 'vitest' +import {statSync} from 'fs' describe('inTemporaryDirectory', () => { test('ties the lifecycle of the temporary directory to the lifecycle of the callback', async () => { @@ -97,6 +98,49 @@ describe('copy', () => { }) }) +describe('writeFile', () => { + test('writes the file and sets the permissions', async () => { + await inTemporaryDirectory(async (tmpDir) => { + // Given + const filePath = joinPath(tmpDir, 'test-file') + const content = 'test-content' + + // When + await writeFile(filePath, content, {encoding: 'utf8', mode: 0o600}) + + // Then + await expect(readFile(filePath)).resolves.toBe(content) + if (process.platform !== 'win32') { + const stats = statSync(filePath) + expect(stats.mode & 0o777).toBe(0o600) + } + }) + }) + + test('sets permissions even if the file already exists', async () => { + await inTemporaryDirectory(async (tmpDir) => { + // Given + const filePath = joinPath(tmpDir, 'test-file') + const content = 'test-content' + await writeFile(filePath, 'old-content') + // Make it world-readable first if possible + if (process.platform !== 'win32') { + await chmod(filePath, 0o644) + } + + // When + await writeFile(filePath, content, {encoding: 'utf8', mode: 0o600}) + + // Then + await expect(readFile(filePath)).resolves.toBe(content) + if (process.platform !== 'win32') { + const stats = statSync(filePath) + expect(stats.mode & 0o777).toBe(0o600) + } + }) + }) +}) + describe('move', () => { test('moves files', async () => { await inTemporaryDirectory(async (tmpDir) => { diff --git a/packages/cli-kit/src/public/node/fs.ts b/packages/cli-kit/src/public/node/fs.ts index 5948c9f6bb6..32e4ab740cc 100644 --- a/packages/cli-kit/src/public/node/fs.ts +++ b/packages/cli-kit/src/public/node/fs.ts @@ -211,6 +211,7 @@ export function appendFileSync(path: string, data: string): void { export interface WriteOptions { encoding: BufferEncoding + mode?: number | string } /** @@ -227,6 +228,9 @@ export async function writeFile( ): Promise { outputDebug(outputContent`Writing some content to file at ${outputToken.path(path)}...`) await fsWriteFile(path, data, options) + if (options.mode) { + await chmod(path, options.mode) + } } /**