[Feature] Add atomic writes for TOML configuration

.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.

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<typeof import('node:fs/promises')>()
+  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 () => {

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<void> {
+  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.
  *

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<typeof import('node:fs/promises')>()
+  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', () => {

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<void> {
     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
   }