Merge pull request #6475 from Shopify/10-03-capture_conf_update_errors

isaacroldan · web-flow · commit bdb504d9d326 · 2025-10-09T11:03:49.000Z
Capture Conf update errors
diff --git a/packages/cli-kit/src/public/node/fs.test.ts b/packages/cli-kit/src/public/node/fs.test.ts
@@ -3,6 +3,8 @@ import {
   copyFile,
   mkdir,
   fileHasExecutablePermissions,
+  fileHasWritePermissions,
+  unixFileIsOwnedByCurrentUser,
   writeFile,
   readFile,
   inTemporaryDirectory,
@@ -388,3 +390,101 @@ describe('copyDirectoryContents', () => {
     })
   })
 })
+
+describe('fileHasWritePermissions', () => {
+  test('returns true when file has write permissions', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // Given
+      const filePath = joinPath(tmpDir, 'writable-file.txt')
+      await writeFile(filePath, 'test content')
+
+      // When
+      const result = await fileHasWritePermissions(filePath)
+
+      // Then
+      expect(result).toBe(true)
+    })
+  })
+
+  test('returns false when file does not have write permissions', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // Given
+      const filePath = joinPath(tmpDir, 'readonly-file.txt')
+      await writeFile(filePath, 'test content')
+      // Read-only
+      await chmod(filePath, 0o444)
+
+      // When
+      const result = await fileHasWritePermissions(filePath)
+
+      // Then
+      expect(result).toBe(false)
+    })
+  })
+
+  test('returns false when file does not exist', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // Given
+      const filePath = joinPath(tmpDir, 'nonexistent-file.txt')
+
+      // When
+      const result = await fileHasWritePermissions(filePath)
+
+      // Then
+      expect(result).toBe(false)
+    })
+  })
+
+  test('returns true when directory has write permissions', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // When
+      const result = await fileHasWritePermissions(tmpDir)
+
+      // Then
+      expect(result).toBe(true)
+    })
+  })
+})
+
+describe('unixFileIsOwnedByCurrentUser', () => {
+  test.skipIf(process.platform === 'win32')('returns true when file is owned by current user', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // Given
+      const filePath = joinPath(tmpDir, 'my-file.txt')
+      await writeFile(filePath, 'test content')
+
+      // When
+      const result = await unixFileIsOwnedByCurrentUser(filePath)
+
+      // Then
+      expect(result).toBe(true)
+    })
+  })
+
+  test.skipIf(process.platform === 'win32')('returns false when file does not exist', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // Given
+      const filePath = joinPath(tmpDir, 'nonexistent-file.txt')
+
+      // When
+      const result = await unixFileIsOwnedByCurrentUser(filePath)
+
+      // Then
+      expect(result).toBe(false)
+    })
+  })
+
+  test.runIf(process.platform === 'win32')('returns undefined on Windows', async () => {
+    await inTemporaryDirectory(async (tmpDir) => {
+      // Given
+      const filePath = joinPath(tmpDir, 'my-file.txt')
+      await writeFile(filePath, 'test content')
+
+      // When
+      const result = await unixFileIsOwnedByCurrentUser(filePath)
+
+      // Then
+      expect(result).toBe(undefined)
+    })
+  })
+})
diff --git a/packages/cli-kit/src/public/node/fs.ts b/packages/cli-kit/src/public/node/fs.ts
@@ -28,8 +28,10 @@ import {
   constants as fsConstants,
   existsSync as fsFileExistsSync,
   unlinkSync as fsUnlinkSync,
+  accessSync,
   ReadStream,
   WriteStream,
+  statSync,
 } from 'fs'
 import {
   mkdir as fsMkdir,
@@ -440,6 +442,32 @@ export async function fileHasExecutablePermissions(path: string): Promise<boolea
   }
 }
 
+export function fileHasWritePermissions(path: string): boolean {
+  try {
+    accessSync(path, fsConstants.W_OK)
+    return true
+    // eslint-disable-next-line no-catch-all/no-catch-all
+  } catch {
+    return false
+  }
+}
+
+export function unixFileIsOwnedByCurrentUser(path: string): boolean | undefined {
+  // process.getuid() is only available on Unix-like systems
+  if (process.platform === 'win32' || typeof process.getuid !== 'function') return undefined
+  if (!fileExistsSync(path)) return false
+
+  try {
+    const stats = statSync(path)
+    const currentUid = process.getuid()
+
+    return stats.uid === currentUid
+    // eslint-disable-next-line no-catch-all/no-catch-all
+  } catch {
+    return false
+  }
+}
+
 /**
  * Returns true if a file or directory exists.
  *
diff --git a/packages/cli-kit/src/public/node/local-storage.test.ts b/packages/cli-kit/src/public/node/local-storage.test.ts
@@ -1,6 +1,9 @@
+/* eslint-disable @typescript-eslint/dot-notation */
 import {LocalStorage} from './local-storage.js'
 import {inTemporaryDirectory} from './fs.js'
-import {describe, expect, test} from 'vitest'
+import {AbortError} from './error.js'
+import * as fs from './fs.js'
+import {describe, expect, test, vi} from 'vitest'
 
 interface TestSchema {
   testValue: string
@@ -46,12 +49,106 @@ describe('storage', () => {
       // When
       storage.set('testValue', 'test')
       const got = storage.get('testValue')
-      storage.delete('testValue')
-      const got2 = storage.clear()
+      storage.clear()
+      const got2 = storage.get('testValue')
 
       // Then
       expect(got).toEqual('test')
       expect(got2).toEqual(undefined)
     })
   })
 })
+
+describe('error handling', () => {
+  test('throws AbortError when file lacks write permissions', async () => {
+    await inTemporaryDirectory(async (cwd) => {
+      // Given
+      const storage = new LocalStorage<TestSchema>({cwd})
+      storage.set('testValue', 'test')
+
+      // Mock to simulate no write permissions
+      vi.spyOn(fs, 'fileHasWritePermissions').mockReturnValue(false)
+      vi.spyOn(fs, 'unixFileIsOwnedByCurrentUser').mockReturnValue(true)
+
+      // Mock Config to throw
+      storage['config'].set = vi.fn(() => {
+        throw new Error('EACCES: permission denied')
+      })
+      storage['config'].get = vi.fn(() => {
+        throw new Error('EACCES: permission denied')
+      })
+      storage['config'].delete = vi.fn(() => {
+        throw new Error('EACCES: permission denied')
+      })
+      storage['config'].clear = vi.fn(() => {
+        throw new Error('EACCES: permission denied')
+      })
+
+      // When/Then
+      expect(() => storage.set('testValue', 'updated')).toThrow(AbortError)
+      expect(() => storage.set('testValue', 'updated')).toThrow(/Failed to access local storage/)
+
+      expect(() => storage.get('testValue')).toThrow(AbortError)
+      expect(() => storage.get('testValue')).toThrow(/Failed to access local storage/)
+
+      expect(() => storage.delete('testValue')).toThrow(AbortError)
+      expect(() => storage.delete('testValue')).toThrow(/Failed to access local storage/)
+
+      expect(() => storage.clear()).toThrow(AbortError)
+      expect(() => storage.clear()).toThrow(/Failed to access local storage/)
+    })
+  })
+
+  test('throws AbortError when file is owned by different user', async () => {
+    await inTemporaryDirectory(async (cwd) => {
+      // Given
+      const storage = new LocalStorage<TestSchema>({cwd})
+      storage.set('testValue', 'test')
+
+      // Mock to simulate ownership issue
+      vi.spyOn(fs, 'fileHasWritePermissions').mockReturnValue(true)
+      vi.spyOn(fs, 'unixFileIsOwnedByCurrentUser').mockReturnValue(false)
+
+      // Mock Config to throw
+
+      storage['config'].get = vi.fn(() => {
+        throw new Error('EACCES: permission denied')
+      })
+
+      // When/Then
+      try {
+        storage.get('testValue')
+        expect.fail('Should have thrown')
+      } catch (error) {
+        if (error instanceof AbortError) {
+          const tryMessageStr = JSON.stringify(error.tryMessage)
+          expect(tryMessageStr).toContain('owned by a different user')
+          expect(tryMessageStr).toContain('elevated permissions')
+          expect(tryMessageStr).toContain('remove the Shopify CLI preferences folder')
+        } else {
+          throw error
+        }
+      }
+    })
+  })
+
+  test('throws BugError when error is not permission-related', async () => {
+    await inTemporaryDirectory(async (cwd) => {
+      // Given
+      const storage = new LocalStorage<TestSchema>({cwd})
+
+      // Mock to simulate good permissions
+      vi.spyOn(fs, 'fileHasWritePermissions').mockReturnValue(true)
+      vi.spyOn(fs, 'unixFileIsOwnedByCurrentUser').mockReturnValue(true)
+
+      // Mock Config to throw non-permission error
+
+      storage['config'].set = vi.fn(() => {
+        throw new Error('Invalid JSON format')
+      })
+
+      // When/Then
+      expect(() => storage.set('testValue', 'test')).toThrow(/Unexpected error while accessing local storage/)
+    })
+  })
+})
diff --git a/packages/cli-kit/src/public/node/local-storage.ts b/packages/cli-kit/src/public/node/local-storage.ts
@@ -1,3 +1,7 @@
+import {AbortError, BugError} from './error.js'
+import {fileHasWritePermissions, unixFileIsOwnedByCurrentUser} from './fs.js'
+import {dirname} from './path.js'
+import {TokenItem} from './ui.js'
 import Config from 'conf'
 
 /**
@@ -17,34 +21,108 @@ export class LocalStorage<T extends {[key: string]: any}> {
    *
    * @param key - The key to get.
    * @returns The value.
+   * @throws AbortError if a permission error occurs.
+   * @throws BugError if an unexpected error occurs.
    */
   get<TKey extends keyof T>(key: TKey): T[TKey] | undefined {
-    return this.config.get(key)
+    try {
+      return this.config.get(key)
+      // eslint-disable-next-line no-catch-all/no-catch-all
+    } catch (error) {
+      this.handleError(error, 'get')
+    }
   }
 
   /**
    * Set a value in the local storage.
    *
    * @param key - The key to set.
    * @param value - The value to set.
+   * @throws AbortError if a permission error occurs.
+   * @throws BugError if an unexpected error occurs.
    */
   set<TKey extends keyof T>(key: TKey, value?: T[TKey]): void {
-    this.config.set(key, value)
+    try {
+      this.config.set(key, value)
+      // eslint-disable-next-line no-catch-all/no-catch-all
+    } catch (error) {
+      this.handleError(error, 'set')
+    }
   }
 
   /**
    * Delete a value from the local storage.
    *
    * @param key - The key to delete.
+   * @throws AbortError if a permission error occurs.
+   * @throws BugError if an unexpected error occurs.
    */
   delete<TKey extends keyof T>(key: TKey): void {
-    this.config.delete(key)
+    try {
+      this.config.delete(key)
+      // eslint-disable-next-line no-catch-all/no-catch-all
+    } catch (error) {
+      this.handleError(error, 'delete')
+    }
   }
 
   /**
    * Clear the local storage (delete all values).
+   *
+   * @throws AbortError if a permission error occurs.
+   * @throws BugError if an unexpected error occurs.
    */
   clear(): void {
-    this.config.clear()
+    try {
+      this.config.clear()
+      // eslint-disable-next-line no-catch-all/no-catch-all
+    } catch (error) {
+      this.handleError(error, 'clear')
+    }
+  }
+
+  /**
+   * Handle errors from config operations.
+   * If the error is permission-related, throw an AbortError with helpful hints.
+   * Otherwise, throw a BugError.
+   *
+   * @param error - The error that occurred.
+   * @param operation - The operation that failed.
+   * @throws AbortError if the error is permission-related.
+   * @throws BugError if the error is not permission-related.
+   */
+  private handleError(error: unknown, operation: string): never {
+    if (this.isPermissionError()) {
+      throw new AbortError(`Failed to access local storage (${operation}): ${error}`, this.tryMessage())
+    } else {
+      throw new BugError(
+        `Unexpected error while accessing local storage at ${this.config.path} (${operation}): ${error}`,
+      )
+    }
+  }
+
+  private isPermissionError(): boolean {
+    const canAccessFile = fileHasWritePermissions(this.config.path)
+    const canAccessFolder = fileHasWritePermissions(dirname(this.config.path))
+    const ownsFile = unixFileIsOwnedByCurrentUser(this.config.path)
+
+    return !canAccessFile || !canAccessFolder || ownsFile === false
+  }
+
+  private tryMessage() {
+    const ownsFile = unixFileIsOwnedByCurrentUser(this.config.path)
+    const ownsFolder = unixFileIsOwnedByCurrentUser(dirname(this.config.path))
+
+    const message: TokenItem = [`Check that you have write permissions for`, {filePath: this.config.path}]
+    if (ownsFile === false || ownsFolder === false) {
+      message.push(
+        '- The file is owned by a different user. This typically happens when Shopify CLI was previously run with elevated permissions (e.g., sudo).',
+      )
+    }
+
+    message.push('\n\nTo resolve this, remove the Shopify CLI preferences folder:')
+    message.push({command: `rm -rf ${dirname(this.config.path)}`})
+
+    return message
   }
 }
