From bab38ae91b9b32dfbec88d5aabac408d0f3ace0d Mon Sep 17 00:00:00 2001 From: haelyra <49814733+haelyra@users.noreply.github.com> Date: Thu, 13 Aug 2026 16:59:06 -0400 Subject: [PATCH] fix(security): close installer filesystem races Use no-follow file descriptors for legacy Codex snapshots, verification, restoration, and marker cleanup. Quarantine candidate removals and verify inode identity before deletion. Carry the lifecycle runner as a verified artifact so privileged release workflows never dynamically check out and execute an output-selected revision. --- .github/workflows/release.yml | 16 +- .github/workflows/reusable-release.yml | 16 +- scripts/lib/codex-legacy-sync.js | 255 ++++++++++++------ .../release-packed-artifact-workflow.test.js | 7 +- 4 files changed, 181 insertions(+), 113 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ee8946509..32f5fe305 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -16,7 +16,6 @@ jobs: dist_tag: ${{ steps.npm_publish_state.outputs.dist_tag }} package_file: ${{ steps.pack.outputs.package_file }} package_sha256: ${{ steps.pack.outputs.package_sha256 }} - release_commit: ${{ steps.source.outputs.release_commit }} steps: - name: Checkout @@ -25,12 +24,6 @@ jobs: fetch-depth: 0 persist-credentials: false - - name: Pin release source - id: source - run: | - RELEASE_COMMIT=$(git rev-parse HEAD) - echo "release_commit=${RELEASE_COMMIT}" >> "$GITHUB_OUTPUT" - - name: Setup Node.js uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: @@ -121,6 +114,7 @@ jobs: path: | release_body.md ${{ steps.pack.outputs.package_file }} + tests/ci/packed-artifact-lifecycle.js if-no-files-found: error - name: Verify existing npm artifact matches candidate @@ -145,12 +139,6 @@ jobs: runs-on: ${{ matrix.os }} steps: - - name: Checkout lifecycle test - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - with: - ref: ${{ needs.verify.outputs.release_commit }} - persist-credentials: false - - name: Setup Node.js uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: @@ -166,7 +154,7 @@ jobs: env: ECC_RELEASE_PACKAGE: release-artifacts/${{ needs.verify.outputs.package_file }} ECC_RELEASE_SHA256: ${{ needs.verify.outputs.package_sha256 }} - run: node tests/ci/packed-artifact-lifecycle.js + run: node release-artifacts/tests/ci/packed-artifact-lifecycle.js publish: name: Publish Release diff --git a/.github/workflows/reusable-release.yml b/.github/workflows/reusable-release.yml index 81dade65a..a9a7bd6a1 100644 --- a/.github/workflows/reusable-release.yml +++ b/.github/workflows/reusable-release.yml @@ -39,7 +39,6 @@ jobs: dist_tag: ${{ steps.npm_publish_state.outputs.dist_tag }} package_file: ${{ steps.pack.outputs.package_file }} package_sha256: ${{ steps.pack.outputs.package_sha256 }} - release_commit: ${{ steps.source.outputs.release_commit }} steps: - name: Checkout @@ -49,12 +48,6 @@ jobs: ref: refs/tags/${{ inputs.tag }} persist-credentials: false - - name: Pin release source - id: source - run: | - RELEASE_COMMIT=$(git rev-parse HEAD) - echo "release_commit=${RELEASE_COMMIT}" >> "$GITHUB_OUTPUT" - - name: Setup Node.js uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: @@ -138,6 +131,7 @@ jobs: path: | release_body.md ${{ steps.pack.outputs.package_file }} + tests/ci/packed-artifact-lifecycle.js if-no-files-found: error - name: Verify existing npm artifact matches candidate @@ -162,12 +156,6 @@ jobs: runs-on: ${{ matrix.os }} steps: - - name: Checkout lifecycle test - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 - with: - ref: ${{ needs.verify.outputs.release_commit }} - persist-credentials: false - - name: Setup Node.js uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: @@ -183,7 +171,7 @@ jobs: env: ECC_RELEASE_PACKAGE: release-artifacts/${{ needs.verify.outputs.package_file }} ECC_RELEASE_SHA256: ${{ needs.verify.outputs.package_sha256 }} - run: node tests/ci/packed-artifact-lifecycle.js + run: node release-artifacts/tests/ci/packed-artifact-lifecycle.js publish: name: Publish Release diff --git a/scripts/lib/codex-legacy-sync.js b/scripts/lib/codex-legacy-sync.js index cbf5b2750..0efce58a7 100644 --- a/scripts/lib/codex-legacy-sync.js +++ b/scripts/lib/codex-legacy-sync.js @@ -14,8 +14,87 @@ function getStatePath(codexHome) { return path.join(codexHome, 'ecc', 'legacy-sync-state.json'); } -function digestFile(filePath) { - return crypto.createHash('sha256').update(fs.readFileSync(filePath)).digest('hex'); +function openRegularFileNoFollow(filePath, writable = false) { + const noFollow = fs.constants.O_NOFOLLOW || 0; + const flags = (writable ? fs.constants.O_RDWR : fs.constants.O_RDONLY) | noFollow; + let descriptor; + try { + descriptor = fs.openSync(filePath, flags); + } catch (error) { + if (error.code === 'ENOENT') return null; + if (error.code === 'ELOOP') { + throw new Error(`Refusing to manage non-regular legacy sync path: ${filePath}`); + } + throw error; + } + const stat = fs.fstatSync(descriptor); + if (!stat.isFile()) { + fs.closeSync(descriptor); + throw new Error(`Refusing to manage non-regular legacy sync path: ${filePath}`); + } + return { descriptor, stat }; +} + +function readRegularFileNoFollow(filePath, encoding = null) { + const opened = openRegularFileNoFollow(filePath); + if (!opened) return null; + try { + return { + content: fs.readFileSync(opened.descriptor, encoding || undefined), + mode: opened.stat.mode & 0o777, + }; + } finally { + fs.closeSync(opened.descriptor); + } +} + +function replaceOpenedRegularFile(opened, content, mode = null) { + const buffer = Buffer.isBuffer(content) ? content : Buffer.from(content); + fs.ftruncateSync(opened.descriptor, 0); + fs.writeSync(opened.descriptor, buffer, 0, buffer.length, 0); + if (mode) fs.fchmodSync(opened.descriptor, mode); + fs.fsyncSync(opened.descriptor); +} + +function createRegularFileNoFollow(filePath, content, mode = 0o600) { + const noFollow = fs.constants.O_NOFOLLOW || 0; + const flags = fs.constants.O_WRONLY | fs.constants.O_CREAT | fs.constants.O_EXCL | noFollow; + const descriptor = fs.openSync(filePath, flags, mode); + try { + const stat = fs.fstatSync(descriptor); + if (!stat.isFile()) { + throw new Error(`Refusing to create non-regular legacy sync path: ${filePath}`); + } + fs.writeFileSync(descriptor, content); + fs.fchmodSync(descriptor, mode); + fs.fsyncSync(descriptor); + } finally { + fs.closeSync(descriptor); + } +} + +function removeOpenedRegularFile(filePath, opened) { + const quarantineDir = fs.mkdtempSync(path.join(path.dirname(filePath), '.ecc-remove-')); + const quarantinePath = path.join(quarantineDir, path.basename(filePath)); + fs.renameSync(filePath, quarantinePath); + const quarantined = openRegularFileNoFollow(quarantinePath); + const openedStat = fs.fstatSync(opened.descriptor, { bigint: true }); + const quarantinedStat = fs.fstatSync(quarantined.descriptor, { bigint: true }); + fs.closeSync(quarantined.descriptor); + if (quarantinedStat.dev !== openedStat.dev || quarantinedStat.ino !== openedStat.ino) { + try { + fs.linkSync(quarantinePath, filePath); + fs.unlinkSync(quarantinePath); + fs.rmdirSync(quarantineDir); + } catch (_restoreError) { + throw new Error( + `Legacy sync path changed before removal; preserved replacement at ${quarantinePath}` + ); + } + throw new Error(`Legacy sync path changed before removal: ${filePath}`); + } + fs.unlinkSync(quarantinePath); + fs.rmdirSync(quarantineDir); } function atomicWriteJson(filePath, value) { @@ -26,7 +105,18 @@ function atomicWriteJson(filePath, value) { } function readState(statePath) { - const state = JSON.parse(fs.readFileSync(statePath, 'utf8')); + const snapshot = readRegularFileNoFollow(statePath, 'utf8'); + if (!snapshot) throw new Error(`Legacy Codex sync state not found at ${statePath}`); + return parseState(snapshot.content, statePath); +} + +function readStateIfPresent(statePath) { + const snapshot = readRegularFileNoFollow(statePath, 'utf8'); + return snapshot ? parseState(snapshot.content, statePath) : null; +} + +function parseState(content, statePath) { + const state = JSON.parse(content); if (state.schema !== SCHEMA || !Array.isArray(state.paths)) { throw new Error(`Invalid legacy Codex sync state at ${statePath}`); } @@ -68,30 +158,14 @@ function getTrustedRoot(state, filePath) { } function snapshotLegacyPath(filePath) { - let previousContentBase64 = null; - let previousMode = null; - let previousType = 'missing'; - try { - const stat = fs.lstatSync(filePath); - if (stat.isFile()) { - previousType = 'file'; - previousContentBase64 = fs.readFileSync(filePath).toString('base64'); - previousMode = stat.mode & 0o777; - } else { - previousType = stat.isSymbolicLink() ? 'symlink' : 'other'; - } - } catch (error) { - if (error.code !== 'ENOENT') throw error; - } - if (previousType === 'symlink' || previousType === 'other') { - throw new Error(`Refusing to manage non-regular legacy sync path: ${filePath}`); - } + const snapshot = readRegularFileNoFollow(filePath); + const previousType = snapshot ? 'file' : 'missing'; return { path: filePath, installedSha256: null, previousType, - previousContentBase64, - previousMode, + previousContentBase64: snapshot ? snapshot.content.toString('base64') : null, + previousMode: snapshot ? snapshot.mode : null, }; } @@ -102,17 +176,15 @@ function assertInstalledStateUnmodified(state) { if (!trustedRoot || hasUnsafeManagedAncestor(filePath, trustedRoot)) { throw new Error(`Refusing to reuse unsafe legacy Codex ownership path: ${filePath}`); } - let stat = null; - try { - stat = fs.lstatSync(filePath); - } catch (error) { - if (error.code !== 'ENOENT') throw error; - } + const snapshot = readRegularFileNoFollow(filePath); if (!entry.installedSha256) { - if (stat) throw new Error(`Refusing to replace modified legacy Codex artifact: ${filePath}`); + if (snapshot) throw new Error(`Refusing to replace modified legacy Codex artifact: ${filePath}`); continue; } - if (!stat || !stat.isFile() || digestFile(filePath) !== entry.installedSha256) { + const digest = snapshot + ? crypto.createHash('sha256').update(snapshot.content).digest('hex') + : null; + if (digest !== entry.installedSha256) { throw new Error(`Refusing to replace modified legacy Codex artifact: ${filePath}`); } } @@ -124,7 +196,7 @@ function beginLegacySyncState(options) { const configPath = path.join(codexHome, 'config.toml'); const agentsPath = path.join(codexHome, 'AGENTS.md'); const installedHooksPath = options.installedHooksPath ? path.resolve(options.installedHooksPath) : null; - const priorState = fs.existsSync(statePath) ? readState(statePath) : null; + const priorState = readStateIfPresent(statePath); if (priorState && priorState.status !== 'installed') { throw new Error(`Legacy Codex sync state requires recovery before reinstall: ${statePath}`); } @@ -161,15 +233,8 @@ function beginLegacySyncState(options) { for (const [key, filePath] of [['config', configPath], ['agents', agentsPath]]) { if (priorState) break; - if (fs.existsSync(filePath)) { - const stat = fs.lstatSync(filePath); - if (!stat.isFile()) { - throw new Error(`Refusing to snapshot non-regular legacy sync path: ${filePath}`); - } - state.before[key] = fs.readFileSync(filePath, 'utf8'); - } else { - state.before[key] = null; - } + const snapshot = readRegularFileNoFollow(filePath, 'utf8'); + state.before[key] = snapshot ? snapshot.content : null; } atomicWriteJson(statePath, state); return statePath; @@ -210,31 +275,38 @@ function rollbackLegacyCodexSync(options) { retainedPaths.push(filePath); continue; } - let currentStat = null; + let opened = null; try { - currentStat = fs.lstatSync(filePath); - } catch (error) { - if (error.code !== 'ENOENT') throw error; - } - if (currentStat && !currentStat.isFile() && !currentStat.isSymbolicLink()) { + opened = openRegularFileNoFollow(filePath, true); + } catch (_error) { retainedPaths.push(filePath); continue; } if (entry.previousType === 'file' && typeof entry.previousContentBase64 === 'string') { - if (currentStat && currentStat.isSymbolicLink()) { - retainedPaths.push(filePath); - continue; - } + const previousContent = Buffer.from(entry.previousContentBase64, 'base64'); + const previousMode = entry.previousMode || 0o600; fs.mkdirSync(path.dirname(filePath), { recursive: true, mode: 0o700 }); - fs.writeFileSync(filePath, Buffer.from(entry.previousContentBase64, 'base64'), { - mode: entry.previousMode || 0o600, - }); - if (entry.previousMode) fs.chmodSync(filePath, entry.previousMode); + if (opened) { + try { + replaceOpenedRegularFile(opened, previousContent, previousMode); + } finally { + fs.closeSync(opened.descriptor); + } + } else { + createRegularFileNoFollow(filePath, previousContent, previousMode); + } restoredPaths.push(filePath); } else if (entry.previousType === 'missing' || entry.previousType === undefined) { - if (currentStat) fs.rmSync(filePath, { force: true }); + if (opened) { + try { + removeOpenedRegularFile(filePath, opened); + } finally { + fs.closeSync(opened.descriptor); + } + } restoredPaths.push(filePath); } else { + if (opened) fs.closeSync(opened.descriptor); retainedPaths.push(filePath); } } @@ -272,15 +344,21 @@ function finalizeLegacySyncState(options) { delete state.rollbackPaths; delete state.rollbackPreviousHooksPath; delete state.previousInstalledState; - state.paths = state.paths.map(entry => ({ - ...entry, - installedSha256: getTrustedRoot(state, path.resolve(entry.path)) - && !hasUnsafeManagedAncestor(entry.path, getTrustedRoot(state, path.resolve(entry.path))) - && fs.existsSync(entry.path) - && fs.lstatSync(entry.path).isFile() - ? digestFile(entry.path) - : null, - })); + state.paths = state.paths.map(entry => { + const trustedRoot = getTrustedRoot(state, path.resolve(entry.path)); + let installedSha256 = null; + if (trustedRoot && !hasUnsafeManagedAncestor(entry.path, trustedRoot)) { + try { + const snapshot = readRegularFileNoFollow(entry.path); + installedSha256 = snapshot + ? crypto.createHash('sha256').update(snapshot.content).digest('hex') + : null; + } catch (_error) { + installedSha256 = null; + } + } + return { ...entry, installedSha256 }; + }); atomicWriteJson(options.statePath, state); return state; } @@ -374,21 +452,24 @@ function uninstallLegacyCodexSync(options = {}) { const plannedRemovals = []; const removedPaths = []; const agentsPath = path.join(codexHome, 'AGENTS.md'); - const state = fs.existsSync(statePath) ? readState(statePath) : null; + const state = readStateIfPresent(statePath); if (!state) { - if (fs.existsSync(agentsPath)) { - const agentsStat = fs.lstatSync(agentsPath); - if (!agentsStat.isFile()) { - retainedPaths.push(agentsPath); - } else { - const content = fs.readFileSync(agentsPath, 'utf8'); + let openedAgents = null; + try { + openedAgents = openRegularFileNoFollow(agentsPath, !dryRun); + if (openedAgents) { + const content = fs.readFileSync(openedAgents.descriptor, 'utf8'); const stripped = stripMarkerBlock(content); if (stripped !== content) { plannedRemovals.push(`${agentsPath}#ecc-marker-block`); - if (!dryRun) fs.writeFileSync(agentsPath, stripped, 'utf8'); + if (!dryRun) replaceOpenedRegularFile(openedAgents, stripped, openedAgents.stat.mode & 0o777); } } + } catch (_error) { + retainedPaths.push(agentsPath); + } finally { + if (openedAgents) fs.closeSync(openedAgents.descriptor); } retainedPaths.push(...listLegacyCandidates(codexHome)); return { @@ -414,29 +495,41 @@ function uninstallLegacyCodexSync(options = {}) { retainedPaths.push(filePath); continue; } - if (!fs.existsSync(filePath)) continue; - const currentStat = fs.lstatSync(filePath); - const matches = entry.installedSha256 && currentStat.isFile() - ? digestFile(filePath) === entry.installedSha256 + let opened = null; + try { + opened = openRegularFileNoFollow(filePath, !dryRun); + } catch (_error) { + retainedPaths.push(filePath); + continue; + } + if (!opened) continue; + const currentContent = fs.readFileSync(opened.descriptor); + const matches = entry.installedSha256 + ? crypto.createHash('sha256').update(currentContent).digest('hex') === entry.installedSha256 : false; if (!matches) { + fs.closeSync(opened.descriptor); retainedPaths.push(filePath); continue; } plannedRemovals.push(filePath); if (!dryRun) { if (entry.previousType === 'file' && typeof entry.previousContentBase64 === 'string') { - fs.writeFileSync(filePath, Buffer.from(entry.previousContentBase64, 'base64'), { - mode: entry.previousMode || 0o600, - }); + replaceOpenedRegularFile( + opened, + Buffer.from(entry.previousContentBase64, 'base64'), + entry.previousMode || 0o600 + ); } else if (entry.previousType === 'missing' || entry.previousType === undefined) { - fs.rmSync(filePath, { force: true }); + removeOpenedRegularFile(filePath, opened); } else { + fs.closeSync(opened.descriptor); retainedPaths.push(filePath); continue; } removedPaths.push(filePath); } + fs.closeSync(opened.descriptor); } if (state.installedHooksPath) { diff --git a/tests/ci/release-packed-artifact-workflow.test.js b/tests/ci/release-packed-artifact-workflow.test.js index a44c43065..09180218d 100644 --- a/tests/ci/release-packed-artifact-workflow.test.js +++ b/tests/ci/release-packed-artifact-workflow.test.js @@ -58,8 +58,6 @@ for (const workflowPath of workflowPaths) { assert.match(source, /package_sha256:\s*\$\{\{ steps\.pack\.outputs\.package_sha256 \}\}/); assert.match(source, /createHash\(['"]sha256['"]\)/); assert.match(source, /package_sha256=['"]? \+ digest/); - assert.match(source, /release_commit:\s*\$\{\{ steps\.source\.outputs\.release_commit \}\}/); - assert.match(source, /release_commit=\$\{RELEASE_COMMIT\}/); }); test(`${workflowPath} invokes only test files present in the release source`, () => { @@ -80,6 +78,7 @@ for (const workflowPath of workflowPaths) { assert.ok(uploadIndex > packIndex, 'artifact upload must happen after pack and hash'); assert.match(verify, /name:\s*ecc-release-artifacts/); assert.match(verify, /\$\{\{ steps\.pack\.outputs\.package_file \}\}/); + assert.match(verify, /tests\/ci\/packed-artifact-lifecycle\.js/); }); test(`${workflowPath} fails retries when npm already has different bytes`, () => { @@ -102,8 +101,8 @@ for (const workflowPath of workflowPaths) { assert.match(lifecycle, /name:\s*ecc-release-artifacts/); assert.match(lifecycle, /ECC_RELEASE_PACKAGE:\s*release-artifacts\/\$\{\{ needs\.verify\.outputs\.package_file \}\}/); assert.match(lifecycle, /ECC_RELEASE_SHA256:\s*\$\{\{ needs\.verify\.outputs\.package_sha256 \}\}/); - assert.match(lifecycle, /node tests\/ci\/packed-artifact-lifecycle\.js/); - assert.match(lifecycle, /ref:\s*\$\{\{ needs\.verify\.outputs\.release_commit \}\}/); + assert.match(lifecycle, /node release-artifacts\/tests\/ci\/packed-artifact-lifecycle\.js/); + assert.doesNotMatch(lifecycle, /actions\/checkout@/); assert.doesNotMatch(lifecycle, /\bsecrets\s*:/, 'lifecycle job must not receive secrets'); assert.doesNotMatch(lifecycle, /\$\{\{\s*secrets\./, 'lifecycle job must not reference secrets'); });