From 29bd11561079c3126ac8e6ffb05c8a2053a4f966 Mon Sep 17 00:00:00 2001 From: Nick Hayden Date: Tue, 21 Jul 2026 11:41:43 -0400 Subject: [PATCH 1/4] Write TOML updates atomically --- packages/cli-kit/src/public/node/fs.test.ts | 61 +++++++++++++++++++ packages/cli-kit/src/public/node/fs.ts | 20 ++++++ .../src/public/node/toml/toml-file.test.ts | 28 ++++++++- .../cli-kit/src/public/node/toml/toml-file.ts | 10 +-- 4 files changed, 112 insertions(+), 7 deletions(-) diff --git a/packages/cli-kit/src/public/node/fs.test.ts b/packages/cli-kit/src/public/node/fs.test.ts index d30612b5b96..09054ad923d 100644 --- a/packages/cli-kit/src/public/node/fs.test.ts +++ b/packages/cli-kit/src/public/node/fs.test.ts @@ -22,10 +22,71 @@ import { copyDirectoryContents, symlink, fileRealPath, + readdir, + writeFileAtomically, } from './fs.js' import {joinPath, normalizePath} from './path.js' import * as array from '../common/array.js' import {describe, expect, test, vi} from 'vitest' +import {rename as fsRename, stat} from 'node:fs/promises' + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return {...actual, rename: vi.fn(actual.rename)} +}) + +describe('writeFileAtomically', () => { + test('replaces existing file content', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const path = joinPath(tmpDir, 'test-file') + await writeFile(path, 'original content') + + await writeFileAtomically(path, 'replacement content') + + await expect(readFile(path)).resolves.toBe('replacement content') + }) + }) + + test('removes the temporary sibling after success', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const fileName = 'test-file' + const path = joinPath(tmpDir, fileName) + + await writeFileAtomically(path, 'content') + + const entries = await readdir(tmpDir) + expect(entries.filter((entry) => entry.startsWith(`${fileName}.`) && entry.endsWith('.tmp'))).toStrictEqual([]) + }) + }) + + test.skipIf(process.platform === 'win32')('preserves the mode of an existing file', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const path = joinPath(tmpDir, 'test-file') + await writeFile(path, 'original content') + await chmod(path, 0o754) + const originalMode = (await stat(path)).mode + + await writeFileAtomically(path, 'replacement content') + + expect((await stat(path)).mode).toBe(originalMode) + }) + }) + + test('leaves the original unchanged and removes the temporary sibling when replacement fails', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const fileName = 'test-file' + const path = joinPath(tmpDir, fileName) + await writeFile(path, 'original content') + vi.mocked(fsRename).mockRejectedValueOnce(new Error('replacement failed')) + + await expect(writeFileAtomically(path, 'replacement content')).rejects.toThrow('replacement failed') + + await expect(readFile(path)).resolves.toBe('original content') + const entries = await readdir(tmpDir) + expect(entries.filter((entry) => entry.startsWith(`${fileName}.`) && entry.endsWith('.tmp'))).toStrictEqual([]) + }) + }) +}) describe('inTemporaryDirectory', () => { test('ties the lifecycle of the temporary directory to the lifecycle of the callback', async () => { diff --git a/packages/cli-kit/src/public/node/fs.ts b/packages/cli-kit/src/public/node/fs.ts index 5948c9f6bb6..709d8088cf0 100644 --- a/packages/cli-kit/src/public/node/fs.ts +++ b/packages/cli-kit/src/public/node/fs.ts @@ -54,6 +54,7 @@ import { symlink as fsSymlink, } from 'fs/promises' import {pathToFileURL as pathToFile} from 'url' +import {randomUUID} from 'node:crypto' import * as os from 'os' import type {Pattern, Options as GlobOptions} from 'fast-glob' @@ -229,6 +230,25 @@ export async function writeFile( await fsWriteFile(path, data, options) } +/** + * Atomically writes text content to a file through a sibling temporary file. + * + * @param path - Path to the file to be written. + * @param data - Text content to be written. + */ +export async function writeFileAtomically(path: string, data: string): Promise { + const temporaryPath = `${path}.${process.pid}.${randomUUID()}.tmp` + const existingMode = (await fileExists(path)) ? (await fsStat(path)).mode : undefined + + try { + await writeFile(temporaryPath, data) + if (existingMode !== undefined) await chmod(temporaryPath, existingMode) + await renameFile(temporaryPath, path) + } finally { + await removeFile(temporaryPath) + } +} + /** * Synchronously writes content to file at path. * diff --git a/packages/cli-kit/src/public/node/toml/toml-file.test.ts b/packages/cli-kit/src/public/node/toml/toml-file.test.ts index 9e507c60df0..e9f8515a6b6 100644 --- a/packages/cli-kit/src/public/node/toml/toml-file.test.ts +++ b/packages/cli-kit/src/public/node/toml/toml-file.test.ts @@ -1,8 +1,14 @@ import {TomlFile, TomlFileError} from './toml-file.js' import {shouldReportErrorAsUnexpected} from '../error.js' -import {writeFile, readFile, inTemporaryDirectory} from '../fs.js' +import {writeFile, readFile, inTemporaryDirectory, readdir} from '../fs.js' import {joinPath} from '../path.js' -import {describe, expect, test} from 'vitest' +import {describe, expect, test, vi} from 'vitest' +import {rename as fsRename} from 'node:fs/promises' + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal() + return {...actual, rename: vi.fn(actual.rename)} +}) describe('TomlFile', () => { describe('read', () => { @@ -137,6 +143,24 @@ describe('TomlFile', () => { expect(content.auth.redirect_urls).toStrictEqual(['https://new.com', 'https://other.com']) }) }) + + test('leaves the original unchanged when atomic replacement fails', async () => { + await inTemporaryDirectory(async (dir) => { + const fileName = 'test.toml' + const path = joinPath(dir, fileName) + const originalContent = 'name = "old"\n' + await writeFile(path, originalContent) + const file = await TomlFile.read(path) + vi.mocked(fsRename).mockRejectedValueOnce(new Error('replacement failed')) + + await expect(file.patch({name: 'new'})).rejects.toThrow('replacement failed') + + await expect(readFile(path)).resolves.toBe(originalContent) + expect(file.content).toStrictEqual({name: 'old'}) + const entries = await readdir(dir) + expect(entries.filter((entry) => entry.startsWith(`${fileName}.`) && entry.endsWith('.tmp'))).toStrictEqual([]) + }) + }) }) describe('remove', () => { diff --git a/packages/cli-kit/src/public/node/toml/toml-file.ts b/packages/cli-kit/src/public/node/toml/toml-file.ts index a2ee81b3236..7e9287ae76c 100644 --- a/packages/cli-kit/src/public/node/toml/toml-file.ts +++ b/packages/cli-kit/src/public/node/toml/toml-file.ts @@ -1,6 +1,6 @@ import {JsonMapType, decodeToml, encodeToml} from './codec.js' import {AbortError} from '../error.js' -import {fileExists, readFile, writeFile} from '../fs.js' +import {fileExists, readFile, writeFileAtomically} from '../fs.js' import {updateTomlValues} from '@shopify/toml-patch' type TomlPatchValue = string | number | boolean | undefined | (string | number | boolean)[] @@ -76,7 +76,7 @@ export class TomlFile { const raw = await readFile(this.path) const updated = updateTomlValues(raw, patches) const parsed = this.decode(updated) - await writeFile(this.path, updated) + await writeFileAtomically(this.path, updated) this.content = parsed } @@ -94,7 +94,7 @@ export class TomlFile { const raw = await readFile(this.path) const updated = updateTomlValues(raw, [[keys, undefined]]) const parsed = this.decode(updated) - await writeFile(this.path, updated) + await writeFileAtomically(this.path, updated) this.content = parsed } @@ -111,7 +111,7 @@ export class TomlFile { async replace(content: JsonMapType): Promise { const encoded = encodeToml(content) this.decode(encoded) - await writeFile(this.path, encoded) + await writeFileAtomically(this.path, encoded) this.content = content } @@ -133,7 +133,7 @@ export class TomlFile { const raw = await readFile(this.path) const transformed = transform(raw) const parsed = this.decode(transformed) - await writeFile(this.path, transformed) + await writeFileAtomically(this.path, transformed) this.content = parsed } From 7743769f836c9c64c70e689f6e65e1d8eb5370aa Mon Sep 17 00:00:00 2001 From: Nick Hayden Date: Wed, 22 Jul 2026 11:01:22 -0400 Subject: [PATCH 2/4] Harden atomic writes --- packages/cli-kit/src/public/node/fs.test.ts | 59 ++++++++++++++++++++- packages/cli-kit/src/public/node/fs.ts | 28 ++++++++-- 2 files changed, 81 insertions(+), 6 deletions(-) diff --git a/packages/cli-kit/src/public/node/fs.test.ts b/packages/cli-kit/src/public/node/fs.test.ts index 09054ad923d..8e0f76c383a 100644 --- a/packages/cli-kit/src/public/node/fs.test.ts +++ b/packages/cli-kit/src/public/node/fs.test.ts @@ -28,11 +28,11 @@ import { import {joinPath, normalizePath} from './path.js' import * as array from '../common/array.js' import {describe, expect, test, vi} from 'vitest' -import {rename as fsRename, stat} from 'node:fs/promises' +import {rename as fsRename, stat, lstat, writeFile as fsWriteFile} from 'node:fs/promises' vi.mock('node:fs/promises', async (importOriginal) => { const actual = await importOriginal() - return {...actual, rename: vi.fn(actual.rename)} + return {...actual, rename: vi.fn(actual.rename), writeFile: vi.fn(actual.writeFile)} }) describe('writeFileAtomically', () => { @@ -59,15 +59,40 @@ describe('writeFileAtomically', () => { }) }) + test('passes a restrictive mode when creating the temporary sibling', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const path = joinPath(tmpDir, 'test-file') + vi.mocked(fsWriteFile).mockClear() + + await writeFileAtomically(path, 'content') + + const temporaryWrite = vi.mocked(fsWriteFile).mock.calls.find(([writePath]) => String(writePath).endsWith('.tmp')) + expect(temporaryWrite?.[2]).toMatchObject({encoding: 'utf8', mode: 0o600}) + }) + }) + + test.skipIf(process.platform === 'win32')('uses restrictive permissions for a new file', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const path = joinPath(tmpDir, 'test-file') + + await writeFileAtomically(path, 'content') + + expect((await stat(path)).mode & 0o777).toBe(0o600) + }) + }) + test.skipIf(process.platform === 'win32')('preserves the mode of an existing file', async () => { await inTemporaryDirectory(async (tmpDir) => { const path = joinPath(tmpDir, 'test-file') await writeFile(path, 'original content') await chmod(path, 0o754) const originalMode = (await stat(path)).mode + vi.mocked(fsWriteFile).mockClear() await writeFileAtomically(path, 'replacement content') + const temporaryWrite = vi.mocked(fsWriteFile).mock.calls.find(([writePath]) => String(writePath).endsWith('.tmp')) + expect(temporaryWrite?.[2]).toMatchObject({mode: originalMode & 0o7777}) expect((await stat(path)).mode).toBe(originalMode) }) }) @@ -86,6 +111,36 @@ describe('writeFileAtomically', () => { expect(entries.filter((entry) => entry.startsWith(`${fileName}.`) && entry.endsWith('.tmp'))).toStrictEqual([]) }) }) + + test('writes through an existing file symlink and preserves the link', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const targetPath = joinPath(tmpDir, 'target-file') + const linkPath = joinPath(tmpDir, 'linked-file') + await writeFile(targetPath, 'original content') + await symlink(targetPath, linkPath) + + await writeFileAtomically(linkPath, 'replacement content') + + await expect(readFile(targetPath)).resolves.toBe('replacement content') + expect((await lstat(linkPath)).isSymbolicLink()).toBe(true) + const entries = await readdir(tmpDir) + expect(entries.filter((entry) => entry.endsWith('.tmp'))).toStrictEqual([]) + }) + }) + + test('rejects a dangling symlink without replacing it', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const linkPath = joinPath(tmpDir, 'linked-file') + await symlink(joinPath(tmpDir, 'missing-target'), linkPath) + + await expect(writeFileAtomically(linkPath, 'replacement content')).rejects.toThrow( + `Unable to atomically write ${linkPath}: destination is a dangling symlink`, + ) + + expect((await lstat(linkPath)).isSymbolicLink()).toBe(true) + await expect(readdir(tmpDir)).resolves.toEqual(['linked-file']) + }) + }) }) describe('inTemporaryDirectory', () => { diff --git a/packages/cli-kit/src/public/node/fs.ts b/packages/cli-kit/src/public/node/fs.ts index 709d8088cf0..b05e8b55128 100644 --- a/packages/cli-kit/src/public/node/fs.ts +++ b/packages/cli-kit/src/public/node/fs.ts @@ -212,6 +212,7 @@ export function appendFileSync(path: string, data: string): void { export interface WriteOptions { encoding: BufferEncoding + mode?: number } /** @@ -237,13 +238,32 @@ export async function writeFile( * @param data - Text content to be written. */ export async function writeFileAtomically(path: string, data: string): Promise { - const temporaryPath = `${path}.${process.pid}.${randomUUID()}.tmp` - const existingMode = (await fileExists(path)) ? (await fsStat(path)).mode : undefined + let destinationStats + try { + destinationStats = await fsLstat(path) + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error + } + + let destinationPath = path + if (destinationStats) { + try { + destinationPath = await fileRealPath(path) + } catch (error) { + if (destinationStats.isSymbolicLink() && (error as NodeJS.ErrnoException).code === 'ENOENT') { + throw new Error(`Unable to atomically write ${path}: destination is a dangling symlink.`) + } + throw error + } + } + + const existingMode = destinationStats ? (await fsStat(destinationPath)).mode & 0o7777 : undefined + const temporaryPath = `${destinationPath}.${process.pid}.${randomUUID()}.tmp` try { - await writeFile(temporaryPath, data) + await writeFile(temporaryPath, data, {encoding: 'utf8', mode: existingMode ?? 0o600}) if (existingMode !== undefined) await chmod(temporaryPath, existingMode) - await renameFile(temporaryPath, path) + await renameFile(temporaryPath, destinationPath) } finally { await removeFile(temporaryPath) } From 8ccf3e34a37e77690b043ef058ef3cdb287d0888 Mon Sep 17 00:00:00 2001 From: Nick Hayden Date: Wed, 22 Jul 2026 11:01:22 -0400 Subject: [PATCH 3/4] Add atomic write changeset --- .changeset/theme-airlock-atomic-toml-writes.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/theme-airlock-atomic-toml-writes.md diff --git a/.changeset/theme-airlock-atomic-toml-writes.md b/.changeset/theme-airlock-atomic-toml-writes.md new file mode 100644 index 00000000000..f84973ec8e4 --- /dev/null +++ b/.changeset/theme-airlock-atomic-toml-writes.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +Add atomic text writes and use them for TOML configuration updates. From a9b957f272f5ce41aa27a3591993bb4df3b679a0 Mon Sep 17 00:00:00 2001 From: Nick Hayden Date: Thu, 23 Jul 2026 11:52:38 -0400 Subject: [PATCH 4/4] Harden atomic temporary file handling --- .../theme-airlock-atomic-toml-writes.md | 2 +- packages/cli-kit/src/public/node/fs.test.ts | 31 ++++++++++++++++++- packages/cli-kit/src/public/node/fs.ts | 21 +++++++++++-- 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/.changeset/theme-airlock-atomic-toml-writes.md b/.changeset/theme-airlock-atomic-toml-writes.md index f84973ec8e4..af0f40cacf5 100644 --- a/.changeset/theme-airlock-atomic-toml-writes.md +++ b/.changeset/theme-airlock-atomic-toml-writes.md @@ -2,4 +2,4 @@ '@shopify/cli-kit': minor --- -Add atomic text writes and use them for TOML configuration updates. +Write TOML configuration updates atomically while preserving existing file permissions and creating new files securely. diff --git a/packages/cli-kit/src/public/node/fs.test.ts b/packages/cli-kit/src/public/node/fs.test.ts index 8e0f76c383a..ef3f74d3b4a 100644 --- a/packages/cli-kit/src/public/node/fs.test.ts +++ b/packages/cli-kit/src/public/node/fs.test.ts @@ -28,6 +28,9 @@ import { import {joinPath, normalizePath} from './path.js' import * as array from '../common/array.js' import {describe, expect, test, vi} from 'vitest' +// eslint-disable-next-line @typescript-eslint/ban-ts-comment +// @ts-ignore +import {remove as fsRemove} from 'fs-extra/esm' import {rename as fsRename, stat, lstat, writeFile as fsWriteFile} from 'node:fs/promises' vi.mock('node:fs/promises', async (importOriginal) => { @@ -35,6 +38,11 @@ vi.mock('node:fs/promises', async (importOriginal) => { return {...actual, rename: vi.fn(actual.rename), writeFile: vi.fn(actual.writeFile)} }) +vi.mock('fs-extra/esm', async (importOriginal) => { + const actual = await importOriginal<{remove: typeof fsRemove}>() + return {...actual, remove: vi.fn(actual.remove)} +}) + describe('writeFileAtomically', () => { test('replaces existing file content', async () => { await inTemporaryDirectory(async (tmpDir) => { @@ -92,7 +100,7 @@ describe('writeFileAtomically', () => { await writeFileAtomically(path, 'replacement content') const temporaryWrite = vi.mocked(fsWriteFile).mock.calls.find(([writePath]) => String(writePath).endsWith('.tmp')) - expect(temporaryWrite?.[2]).toMatchObject({mode: originalMode & 0o7777}) + expect(temporaryWrite?.[2]).toMatchObject({encoding: 'utf8', mode: 0o600}) expect((await stat(path)).mode).toBe(originalMode) }) }) @@ -112,6 +120,27 @@ describe('writeFileAtomically', () => { }) }) + test('rejects the cleanup error after a successful replacement', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const path = joinPath(tmpDir, 'test-file') + vi.mocked(fsRemove).mockRejectedValueOnce(new Error('cleanup failed')) + + await expect(writeFileAtomically(path, 'replacement content')).rejects.toThrow('cleanup failed') + await expect(readFile(path)).resolves.toBe('replacement content') + }) + }) + + test('preserves the replacement error when temporary cleanup also fails', async () => { + await inTemporaryDirectory(async (tmpDir) => { + const path = joinPath(tmpDir, 'test-file') + await writeFile(path, 'original content') + vi.mocked(fsRename).mockRejectedValueOnce(new Error('replacement failed')) + vi.mocked(fsRemove).mockRejectedValueOnce(new Error('cleanup failed')) + + await expect(writeFileAtomically(path, 'replacement content')).rejects.toThrow('replacement failed') + }) + }) + test('writes through an existing file symlink and preserves the link', async () => { await inTemporaryDirectory(async (tmpDir) => { const targetPath = joinPath(tmpDir, 'target-file') diff --git a/packages/cli-kit/src/public/node/fs.ts b/packages/cli-kit/src/public/node/fs.ts index b05e8b55128..e5a15817115 100644 --- a/packages/cli-kit/src/public/node/fs.ts +++ b/packages/cli-kit/src/public/node/fs.ts @@ -260,13 +260,30 @@ export async function writeFileAtomically(path: string, data: string): Promise