fix(ci+installers): harden permission checks and guard completion/profile writes (#1247)

* fix: harden permission checks in root CI

* fix: close permission guard review gaps

* test: address coderabbit installer comments

Author: TabishB

.github/workflows/ci.yml

@@ -60,7 +60,7 @@ jobs:
       - name: Setup Node.js
         uses: actions/setup-node@v4
         with:
-          node-version: '20'
+          node-version: '20.19.0'
           cache: 'pnpm'
 
       - name: Install dependencies
@@ -116,7 +116,7 @@ jobs:
       - name: Setup Node.js
         uses: actions/setup-node@v4
         with:
-          node-version: '20'
+          node-version: '20.19.0'
           cache: 'pnpm'
 
       - name: Print environment diagnostics
@@ -155,7 +155,7 @@ jobs:
       - name: Setup Node.js
         uses: actions/setup-node@v4
         with:
-          node-version: '20'
+          node-version: '20.19.0'
           cache: 'pnpm'
 
       - name: Install dependencies
@@ -277,7 +277,7 @@ jobs:
         if: steps.changed-changesets.outputs.has_changesets == 'true'
         uses: actions/setup-node@v4
         with:
-          node-version: '20'
+          node-version: '20.19.0'
           cache: 'pnpm'
 
       - name: Install dependencies

src/core/completions/installers/bash-installer.ts

@@ -240,6 +240,10 @@ export class BashInstaller {
         console.debug(`Unable to read existing completion file at ${targetPath}: ${error.message}`);
       }
 
+      if (!(await FileSystemUtils.canWriteFile(targetPath))) {
+        throw new Error(`Path is not writable: ${targetPath}`);
+      }
+
       // Ensure the directory exists
       const targetDir = path.dirname(targetPath);
       await fs.mkdir(targetDir, { recursive: true });

src/core/completions/installers/fish-installer.ts

@@ -1,6 +1,7 @@
 import { promises as fs } from 'fs';
 import path from 'path';
 import os from 'os';
+import { FileSystemUtils } from '../../../utils/file-system.js';
 import { InstallationResult } from '../factory.js';
 
 /**
@@ -76,6 +77,10 @@ export class FishInstaller {
         console.debug(`Unable to read existing completion file at ${targetPath}: ${error.message}`);
       }
 
+      if (!(await FileSystemUtils.canWriteFile(targetPath))) {
+        throw new Error(`Path is not writable: ${targetPath}`);
+      }
+
       // Ensure the directory exists
       const targetDir = path.dirname(targetPath);
       await fs.mkdir(targetDir, { recursive: true });
@@ -135,6 +140,11 @@ export class FishInstaller {
         };
       }
 
+      const targetDir = path.dirname(targetPath);
+      if (!(await FileSystemUtils.canWriteFile(targetDir))) {
+        throw new Error(`Path is not writable: ${targetDir}`);
+      }
+
       // Remove the completion script
       await fs.unlink(targetPath);
 

src/core/completions/installers/powershell-installer.ts

@@ -170,9 +170,23 @@ export class PowerShellInstaller {
 
     for (const profilePath of profilePaths) {
       try {
-        // Create profile file if it doesn't exist
         const profileDir = path.dirname(profilePath);
-        await fs.mkdir(profileDir, { recursive: true });
+        let profileExists = false;
+        try {
+          await fs.access(profilePath);
+          profileExists = true;
+        } catch (err: any) {
+          if (err?.code !== 'ENOENT') {
+            throw err;
+          }
+        }
+
+        if (!profileExists) {
+          if (!(await FileSystemUtils.canWriteFile(profilePath))) {
+            throw new Error(`Path is not writable: ${profilePath}`);
+          }
+          await fs.mkdir(profileDir, { recursive: true });
+        }
 
         let profileContent = '';
         let fileEncoding: BufferEncoding = 'utf-8';
@@ -209,6 +223,9 @@ export class PowerShellInstaller {
         ].join('\n');
 
         const newContent = profileContent + openspecBlock;
+        if (!(await FileSystemUtils.canWriteFile(profilePath))) {
+          throw new Error(`Path is not writable: ${profilePath}`);
+        }
         await this.writeProfileFile(profilePath, newContent, fileEncoding, fileBom);
         anyConfigured = true;
       } catch (error) {
@@ -271,6 +288,9 @@ export class PowerShellInstaller {
         // Clean up extra newlines
         const newContent = (beforeBlock.trimEnd() + '\n' + afterBlock.trimStart()).trim() + '\n';
 
+        if (!(await FileSystemUtils.canWriteFile(profilePath))) {
+          throw new Error(`Path is not writable: ${profilePath}`);
+        }
         await this.writeProfileFile(profilePath, newContent, fileEncoding, fileBom);
         anyRemoved = true;
       } catch (error) {
@@ -314,6 +334,10 @@ export class PowerShellInstaller {
         console.debug(`Unable to read existing completion file at ${targetPath}: ${error.message}`);
       }
 
+      if (!(await FileSystemUtils.canWriteFile(targetPath))) {
+        throw new Error(`Path is not writable: ${targetPath}`);
+      }
+
       // Ensure the directory exists
       const targetDir = path.dirname(targetPath);
       await fs.mkdir(targetDir, { recursive: true });
@@ -402,6 +426,11 @@ export class PowerShellInstaller {
         };
       }
 
+      const targetDir = path.dirname(targetPath);
+      if (!(await FileSystemUtils.canWriteFile(targetDir))) {
+        throw new Error(`Path is not writable: ${targetDir}`);
+      }
+
       // Remove the completion script
       await fs.unlink(targetPath);
 

src/core/completions/installers/zsh-installer.ts

@@ -256,6 +256,10 @@ export class ZshInstaller {
         console.debug(`Unable to read existing completion file at ${targetPath}: ${error.message}`);
       }
 
+      if (!(await FileSystemUtils.canWriteFile(targetPath))) {
+        throw new Error(`Path is not writable: ${targetPath}`);
+      }
+
       // Ensure the directory exists
       const targetDir = path.dirname(targetPath);
       await fs.mkdir(targetDir, { recursive: true });

src/core/file-state.ts

@@ -120,7 +120,11 @@ export async function acquireFileLock(
   options: FileLockOptions
 ): Promise<nodeFs.promises.FileHandle> {
   const { lockPath, errorFor } = options;
-  await FileSystemUtils.createDirectory(path.dirname(lockPath));
+  const lockDir = path.dirname(lockPath);
+  await FileSystemUtils.createDirectory(lockDir);
+  if (!(await FileSystemUtils.canWriteFile(lockDir))) {
+    throw errorFor('create-failed', { lockPath, cause: 'EACCES' });
+  }
   const deadline = Date.now() + LOCK_DEADLINE_MS;
 
   while (true) {

src/utils/file-system.ts

@@ -4,6 +4,40 @@ import path from 'path';
 const fs = nodeFs.promises;
 const { constants: fsConstants } = nodeFs;
 
+function hasOwnerGroupOrOtherWriteBit(stats: nodeFs.Stats): boolean {
+  return (stats.mode & 0o222) !== 0;
+}
+
+function hasOwnerGroupOrOtherExecuteBit(stats: nodeFs.Stats): boolean {
+  return (stats.mode & 0o111) !== 0;
+}
+
+async function hasWritableModeAndAccess(targetPath: string): Promise<boolean> {
+  try {
+    const stats = await fs.stat(targetPath);
+
+    // POSIX root can often write despite mode bits, but OpenSpec should respect
+    // explicit read-only file/directory modes when deciding whether an install
+    // path is user-writable. This also keeps permission checks deterministic in
+    // root-run CI containers. On Windows, chmod mode bits are not authoritative,
+    // so rely on fs.access below.
+    if (process.platform !== 'win32' && !hasOwnerGroupOrOtherWriteBit(stats)) {
+      return false;
+    }
+    if (process.platform !== 'win32' && stats.isDirectory() && !hasOwnerGroupOrOtherExecuteBit(stats)) {
+      return false;
+    }
+
+    const accessMode = stats.isDirectory()
+      ? fsConstants.W_OK | fsConstants.X_OK
+      : fsConstants.W_OK;
+    await fs.access(targetPath, accessMode);
+    return true;
+  } catch {
+    return false;
+  }
+}
+
 function isMarkerOnOwnLine(content: string, markerIndex: number, markerLength: number): boolean {
   let leftIndex = markerIndex - 1;
   while (leftIndex >= 0 && content[leftIndex] !== '\n') {
@@ -152,18 +186,15 @@ export class FileSystemUtils {
     try {
       const stats = await fs.stat(filePath);
 
-      if (!stats.isFile()) {
-        return true;
+      if (stats.isDirectory()) {
+        return hasWritableModeAndAccess(filePath);
       }
 
-      // On Windows, stats.mode doesn't reliably indicate write permissions.
-      // Use fs.access with W_OK to check actual write permissions cross-platform.
-      try {
-        await fs.access(filePath, fsConstants.W_OK);
+      if (!stats.isFile()) {
         return true;
-      } catch {
-        return false;
       }
+
+      return hasWritableModeAndAccess(filePath);
     } catch (error: any) {
       if (error.code === 'ENOENT') {
         // File doesn't exist - find first existing parent directory and check its permissions
@@ -175,13 +206,8 @@ export class FileSystemUtils {
           return false;
         }
 
-        // Check if the existing parent directory is writable
-        try {
-          await fs.access(existingDir, fsConstants.W_OK);
-          return true;
-        } catch {
-          return false;
-        }
+        // Check if the existing parent directory is writable.
+        return hasWritableModeAndAccess(existingDir);
       }
 
       console.debug(`Unable to determine write permissions for ${filePath}: ${error.message}`);

test/core/completions/installers/bash-installer.test.ts

@@ -151,6 +151,24 @@ describe('BashInstaller', () => {
       expect(result.message).toContain('Failed to install');
     });
 
+    it.skipIf(process.platform === 'win32')('should return failure when completion directory is not writable', async () => {
+      const targetPath = await installer.getInstallationPath();
+      const targetDir = path.dirname(targetPath);
+      await fs.mkdir(targetDir, { recursive: true });
+      await fs.chmod(targetDir, 0o555);
+
+      let result: Awaited<ReturnType<BashInstaller['install']>> | undefined;
+      try {
+        result = await installer.install(testScript);
+      } finally {
+        await fs.chmod(targetDir, 0o755);
+      }
+
+      expect(result?.success).toBe(false);
+      expect(result?.message).toContain('Failed to install');
+      expect(result?.message).toContain(`Path is not writable: ${targetPath}`);
+    });
+
     it('should detect already-installed completion with identical content', async () => {
       // First installation
       const firstResult = await installer.install(testScript);

test/core/completions/installers/fish-installer.test.ts

@@ -286,6 +286,23 @@ complete -c openspec -a 'init'
       expect(result.message).toBe('Completion script uninstalled successfully');
     });
 
+    it.skipIf(process.platform === 'win32')('should uninstall read-only file when parent directory is writable', async () => {
+      await installer.install(mockCompletionScript);
+      const targetPath = path.join(testHomeDir, '.config', 'fish', 'completions', 'openspec.fish');
+      await fs.chmod(targetPath, 0o444);
+
+      let result: Awaited<ReturnType<FishInstaller['uninstall']>> | undefined;
+      try {
+        result = await installer.uninstall();
+      } finally {
+        await fs.chmod(targetPath, 0o644).catch(() => undefined);
+      }
+
+      const fileExists = await fs.access(targetPath).then(() => true).catch(() => false);
+      expect(result?.success).toBe(true);
+      expect(fileExists).toBe(false);
+    });
+
     // Skip on Windows: fs.chmod() on directories doesn't restrict write access on Windows
     // Windows uses ACLs which Node.js chmod doesn't control
     it.skipIf(process.platform === 'win32')('should return failure on permission error', async () => {

test/core/completions/installers/powershell-installer.test.ts

@@ -11,6 +11,14 @@ describe('PowerShellInstaller', () => {
   let originalPlatform: NodeJS.Platform;
   let originalEnv: NodeJS.ProcessEnv;
 
+  const restoreEnvValue = (key: string, value: string | undefined): void => {
+    if (value === undefined) {
+      delete process.env[key];
+    } else {
+      process.env[key] = value;
+    }
+  };
+
   beforeEach(async () => {
     testHomeDir = path.join(os.tmpdir(), `openspec-powershell-test-${randomUUID()}`);
     await fs.mkdir(testHomeDir, { recursive: true });
@@ -257,6 +265,28 @@ describe('PowerShellInstaller', () => {
 
       expect(result).toBe(false);
     });
+
+    it.skipIf(process.platform === 'win32')('should not create profile directory when parent is not writable', async () => {
+      const originalNoAutoConfig = process.env.OPENSPEC_NO_AUTO_CONFIG;
+      const restrictedHome = path.join(testHomeDir, 'restricted-home');
+      await fs.mkdir(restrictedHome);
+      await fs.chmod(restrictedHome, 0o555);
+      const restrictedInstaller = new PowerShellInstaller(restrictedHome);
+      const profileDir = path.dirname(restrictedInstaller.getProfilePath());
+
+      let result = true;
+      try {
+        delete process.env.OPENSPEC_NO_AUTO_CONFIG;
+        result = await restrictedInstaller.configureProfile(mockScriptPath);
+      } finally {
+        restoreEnvValue('OPENSPEC_NO_AUTO_CONFIG', originalNoAutoConfig);
+        await fs.chmod(restrictedHome, 0o755);
+      }
+
+      const profileDirExists = await fs.access(profileDir).then(() => true).catch(() => false);
+      expect(result).toBe(false);
+      expect(profileDirExists).toBe(false);
+    });
   });
 
   describe('removeProfileConfig', () => {
@@ -767,6 +797,26 @@ Register-ArgumentCompleter -CommandName openspec -ScriptBlock $openspecCompleter
       expect(result.message).toBe('Completion script uninstalled successfully');
     });
 
+    it.skipIf(process.platform === 'win32')('should uninstall read-only completion script when parent directory is writable', async () => {
+      const originalNoAutoConfig = process.env.OPENSPEC_NO_AUTO_CONFIG;
+      const targetPath = installer.getInstallationPath();
+      let result: Awaited<ReturnType<PowerShellInstaller['uninstall']>> | undefined;
+
+      try {
+        delete process.env.OPENSPEC_NO_AUTO_CONFIG;
+        await installer.install(mockCompletionScript);
+        await fs.chmod(targetPath, 0o444);
+        result = await installer.uninstall();
+      } finally {
+        restoreEnvValue('OPENSPEC_NO_AUTO_CONFIG', originalNoAutoConfig);
+        await fs.chmod(targetPath, 0o644).catch(() => undefined);
+      }
+
+      const scriptExists = await fs.access(targetPath).then(() => true).catch(() => false);
+      expect(result?.success).toBe(true);
+      expect(scriptExists).toBe(false);
+    });
+
     it('should handle both script and config removal', async () => {
       delete process.env.OPENSPEC_NO_AUTO_CONFIG;
       await installer.install(mockCompletionScript);