mirror of
https://github.com/alkimake/paperclip.git
synced 2026-06-14 01:50:39 +09:00
## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies, including how we ship the public `paperclip` repo itself > - The `PR` and `commitperclip PR Review` workflows are the CI gating layer that decides whether any pull request — human or bot — can be merged to `master` > - Dependabot opens dependency PRs that always carry a `pnpm-lock.yaml` diff and an auto-generated PR body, but our `policy` job hard-fails any non-`chore/refresh-lockfile` lockfile change, and our `commitperclip` quality gate requires a Thinking-Path / What-Changed / Verification / Risks / Model template Dependabot can't produce > - Because `policy` fails first, every downstream lane (`Build`, `Typecheck + Release Registry`, `General tests`, `Verify serialized server`, `Canary Dry Run`, `e2e`, and the required `verify` check) skips and `verify` fails — so we never see whether the upgrade is actually safe > - Socket.dev (PR Alerts + Project Report) and Snyk already run on every dependency PR and are the supply-chain compensating control against malicious upgrades; the missing piece is just letting our own build/test signal run so a human can merge with confidence > - This pull request adds a narrow Dependabot bypass to the two gates that block on lockfile diffs and PR-template prose, while leaving every other policy and security check active > - The benefit is that Dependabot PRs like #7331 will now run the full PR matrix, giving reviewers real evidence to approve or reject — without weakening any check that targets supply-chain or build-correctness risk ## What Changed - `.github/workflows/pr.yml` — extended the existing `chore/refresh-lockfile` bypass on the `policy` job's "Block manual lockfile edits" step to also skip when `github.actor == 'dependabot[bot]'`. Every other policy step (Dockerfile deps stage validation, `no-git-push` enforcement, release-package map check, release bootstrap, manifest-driven `pnpm install --lockfile-only` resolution) keeps running on Dependabot PRs. - `.github/workflows/commitperclip-review.yml` — gated the `Run quality gates` step and the dependent `Fail if quality gates failed` step on `github.event.pull_request.user.login != 'dependabot[bot]'`. `Run security gates` (`check-pr-security.mjs`) stays unconditional so supply-chain visibility into Dependabot lockfile churn is preserved. No changes to `.github/scripts/*.mjs` — keeping the bypass at the workflow level avoids churning unit-tested code. ## Verification - CI on this PR: `policy` should pass and the downstream lanes (`Build`, `Typecheck + Release Registry`, `General tests`, `Verify serialized server`, `Canary Dry Run`, `e2e`, `verify`) should all run normally (this PR isn't from Dependabot, so the bypass condition is false — proves we didn't accidentally widen the exemption). - After merge, ask Dependabot to rebase #7331 (`@dependabot rebase`) and confirm: - `PR / policy` → `success` (lockfile step now `skipped`, other policy steps `success`) - `PR / Build`, `PR / Typecheck + Release Registry`, `PR / General tests (server|workspaces-a|workspaces-b)`, `PR / Verify serialized server (1/4..4/4)`, `PR / Canary Dry Run`, `PR / e2e` → all execute (none `skipped`) - `PR / verify` → `success` once the matrix passes - `commitperclip PR Review / review` → `success` (quality-gates steps `skipped` for Dependabot; security gates ran) - Socket and Snyk checks unchanged - Local sanity-check: `git diff origin/master..HEAD` shows only the two workflow files, 7 added / 2 removed lines. ## Risks - **Auto-merging a poisoned dep.** Mitigated by Socket.dev + Snyk + human merge approval. This change only affects CI gating, not who clicks "Merge". - **Spoofing `github.actor` as `dependabot[bot]`.** GitHub sets `github.actor` from the push actor; spoofing requires a compromised Dependabot install token, which is the same threat model that already lets an attacker push anything to a Dependabot-controlled branch — not a new risk surface. - **Policy "Validate dependency resolution when manifests change" step running `pnpm install --lockfile-only --no-frozen-lockfile` on a Dependabot lockfile.** That step intentionally uses `--lockfile-only`, so it only verifies the manifest resolves and does not push or commit the result. Existing behavior is unchanged. - Low overall: the diff is two workflow-level `if:` conditions in steps that already had bypasses. ## Model Used - Provider: Anthropic Claude (via Claude Code in the Paperclip executor) - Model ID: claude-opus-4-7 - Context window: 200K - Reasoning mode: standard tool-use; no extended thinking required for this change - Capabilities used: file edit, bash, GraphQL/REST API calls - Plan was drafted, approved by board, and split into child issues before implementation; see [PAPA-490](https://paperclip.ing/PAPA/issues/PAPA-490) for the planning thread. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have run tests locally and they pass (this change is workflow-only — no code under test; lint via `yamllint` clean) - [x] I have added or updated tests where applicable (workflow gating; no script changes, no unit-testable surface) - [x] If this change affects the UI, I have included before/after screenshots (no UI changes) - [x] I have updated relevant documentation to reflect my changes (no docs reference these gates) - [x] I have considered and documented any risks above - [x] I will address all Greptile and reviewer comments before requesting merge
360 lines
13 KiB
JavaScript
360 lines
13 KiB
JavaScript
#!/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),
|
|
});
|
|
}
|
|
|
|
export async function findExistingDraftAdvisory(fetchImpl, token, repo, prNumber) {
|
|
const prMarker = `PR #${prNumber}`;
|
|
|
|
for (let page = 1; ; 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;
|
|
}
|
|
}
|
|
|
|
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 ──────────────────────────────────────────────────────────────────────
|
|
|
|
async function main() {
|
|
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
|
|
process.exit(0);
|
|
}
|
|
|
|
if (process.argv[1] === fileURLToPath(import.meta.url)) {
|
|
main().catch(e => { console.error(e.message); process.exit(1); });
|
|
}
|