diff --git a/.changeset/theme-airlock-atomic-toml-writes.md b/.changeset/theme-airlock-atomic-toml-writes.md new file mode 100644 index 00000000000..af0f40cacf5 --- /dev/null +++ b/.changeset/theme-airlock-atomic-toml-writes.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': minor +--- + +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 d30612b5b96..ef3f74d3b4a 100644 --- a/packages/cli-kit/src/public/node/fs.test.ts +++ b/packages/cli-kit/src/public/node/fs.test.ts @@ -22,10 +22,155 @@ 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' +// 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) => { + const actual = await 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) => { + 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('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({encoding: 'utf8', mode: 0o600}) + 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([]) + }) + }) + + 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') + 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', () => { 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..e5a15817115 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' @@ -211,6 +212,7 @@ export function appendFileSync(path: string, data: string): void { export interface WriteOptions { encoding: BufferEncoding + mode?: number } /** @@ -229,6 +231,61 @@ 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 { + 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` + + let primaryOperationFailed = false + let cleanupFailed = false + let cleanupError: unknown + try { + await writeFile(temporaryPath, data, {encoding: 'utf8', mode: 0o600}) + if (existingMode !== undefined) await chmod(temporaryPath, existingMode) + await renameFile(temporaryPath, destinationPath) + } catch (error) { + primaryOperationFailed = true + throw error + } finally { + try { + await removeFile(temporaryPath) + // Preserve the primary operation error when cleanup also fails. + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + if (!primaryOperationFailed) { + cleanupFailed = true + cleanupError = error + } + } + } + + if (cleanupFailed) throw cleanupError +} + /** * 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 }