diff --git a/.changeset/spicy-donkeys-win.md b/.changeset/spicy-donkeys-win.md new file mode 100644 index 0000000..f007ce1 --- /dev/null +++ b/.changeset/spicy-donkeys-win.md @@ -0,0 +1,5 @@ +--- +"layne": minor +--- + +Adds a new suppressor feature to ignore findings with a "// SECURITY: XYZ" comment diff --git a/docs/configuration.md b/docs/configuration.md index bb5c92a..c17d2da 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -228,6 +228,63 @@ Arguments are passed directly via `execFile` — **not** through a shell — so --- +## Finding Suppression + +### Why `// nosemgrep` is disabled + +Layne passes `--disable-nosemgrep` to Semgrep on every scan (set in `$global.semgrep.extraArgs`). This prevents contributors from silencing findings inline without review — adding `// nosemgrep` in a PR would suppress the finding in that exact PR, bypassing the security review gate entirely. + +### The replacement: `// SECURITY: ` + +Place a comment with a non-empty justification on the **same line** as the flagged code, or on the **line immediately above** it: + +```js +// SECURITY: This eval call only runs trusted internal templates, never user input. +eval(internalTemplate); + +const query = `SELECT * FROM users WHERE id = ${id}`; // SECURITY: id is always cast to integer by the ORM layer. +``` + +```yaml +# SECURITY: This token is intentionally committed — it is a public read-only CI token with no write access. +GITHUB_TOKEN: ghp_... +``` + +### The tamper-proof guarantee + +The suppressor reads each file at the **merge-base SHA** of the PR (the three-dot diff base) via `git show`. A `// SECURITY:` comment added in the **current PR** is invisible to the suppressor — it is not in the base. The comment must have been reviewed, approved, and merged in a **previous PR** before it takes effect. + +This means a contributor cannot self-approve a finding by adding the comment in their own PR. + +### Syntax rules + +- Comment style: `//` (JS/TS/Go/Java/C…) or `#` (YAML/shell/Python/Ruby…) +- The colon must be followed by at least one non-whitespace character: `// SECURITY: reason` ✓, `// SECURITY:` ✗ +- Placement: same line as the finding **or** the line immediately above + +### Workflow + +1. Review the finding and decide it is a genuine false positive. +2. Add a `// SECURITY: ` comment explaining why. +3. Submit a **separate PR** for the suppression comment, have it reviewed and merged. +4. From that point forward, any PR that triggers the same finding on that line will have it suppressed automatically. + +### Keeping `--disable-nosemgrep` in per-repo `extraArgs` + +`--disable-nosemgrep` is set in `$global.semgrep.extraArgs` and must be carried into any per-repo `extraArgs` override. If you set per-repo `extraArgs` without including `--disable-nosemgrep`, the flag will be absent for that repo — `extraArgs` fully replaces the default, it does not extend it (see [Replacement, not extension](#per-repo-configuration)). + +```json +{ + "owner/repo": { + "semgrep": { + "extraArgs": ["--config", "p/owasp-top-ten", "--severity", "ERROR", "--disable-nosemgrep"] + } + } +} +``` + +--- + ## Labels Layne can automatically add and remove GitHub labels on a PR based on the scan result. This gives at-a-glance triage context directly in the PR list view without opening each PR. diff --git a/docs/security-architecture.md b/docs/security-architecture.md index 9a5824b..a1a119d 100644 --- a/docs/security-architecture.md +++ b/docs/security-architecture.md @@ -122,6 +122,16 @@ The worker container has no outbound network restrictions by default. Semgrep's --- +## Finding Suppression + +Semgrep's built-in `// nosemgrep` mechanism is disabled via `--disable-nosemgrep` on every scan. This prevents contributors from self-approving a finding inline — adding `// nosemgrep` in a PR would suppress the finding in that exact PR, bypassing the security review gate. + +The replacement is the `// SECURITY: ` comment. It is tamper-proof: the suppressor (`src/suppressor.js`) reads each file at the **merge-base SHA** of the PR via `git show` and checks whether the comment existed there. A `// SECURITY:` comment introduced in the current PR is not present at the merge base, so it has no effect. The comment must have been merged in a previous PR — reviewed and approved — before it suppresses anything. + +The suppressor runs in the worker after `dispatch()` returns findings and before `buildAnnotations()` is called. A suppressed finding is removed from the list entirely and never appears in the GitHub Check Run. + +--- + ## Reporting a Vulnerability in Layne See [SECURITY.md](../SECURITY.md). diff --git a/src/__tests__/suppressor.test.js b/src/__tests__/suppressor.test.js new file mode 100644 index 0000000..993bded --- /dev/null +++ b/src/__tests__/suppressor.test.js @@ -0,0 +1,121 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +const mockExecFile = vi.fn(); +vi.mock('child_process', () => ({ execFile: mockExecFile })); + +const { suppressFindings } = await import('../suppressor.js'); + +// Helper: make execFile resolve with given stdout +function resolveWith(stdout) { + mockExecFile.mockImplementationOnce((_cmd, _args, cb) => cb(null, stdout, '')); +} + +// Helper: make execFile reject (simulates new file / blob unavailable) +function rejectWith(err = new Error('not found')) { + mockExecFile.mockImplementationOnce((_cmd, _args, cb) => cb(err, '', '')); +} + +const BASE = { workspacePath: '/workspace', baseSha: 'base-sha' }; + +const finding = (overrides = {}) => ({ + file: 'src/app.js', + line: 5, + severity: 'high', + message: 'issue', + ruleId: 'r/1', + tool: 'semgrep', + ...overrides, +}); + +describe('suppressFindings()', () => { + beforeEach(() => vi.clearAllMocks()); + + it('returns [] and never calls execFile for an empty findings array', async () => { + const result = await suppressFindings([], BASE); + expect(result).toEqual([]); + expect(mockExecFile).not.toHaveBeenCalled(); + }); + + it('keeps a finding when git show fails (new file)', async () => { + rejectWith(new Error('fatal: path not in tree')); + const f = finding(); + const result = await suppressFindings([f], BASE); + expect(result).toEqual([f]); + }); + + it('keeps a finding when there is no SECURITY: comment at base', async () => { + resolveWith('line1\nline2\nline3\nline4\nline5\nline6\n'); + const f = finding({ line: 5 }); + const result = await suppressFindings([f], BASE); + expect(result).toEqual([f]); + }); + + it('suppresses a finding when // SECURITY: reason is on the same line', async () => { + resolveWith('line1\nline2\nline3\nline4\nconst x = 1; // SECURITY: reviewed by alice\nline6\n'); + const result = await suppressFindings([finding({ line: 5 })], BASE); + expect(result).toEqual([]); + }); + + it('suppresses a finding when // SECURITY: reason is on the line immediately above', async () => { + resolveWith('line1\nline2\nline3\n// SECURITY: approved in issue #42\nconst x = 1;\nline6\n'); + const result = await suppressFindings([finding({ line: 5 })], BASE); + expect(result).toEqual([]); + }); + + it('suppresses a finding with # SECURITY: (YAML/shell comment style)', async () => { + resolveWith('line1\nline2\nline3\nline4\n# SECURITY: checked for injection\nline6\n'); + const result = await suppressFindings([finding({ line: 5 })], BASE); + expect(result).toEqual([]); + }); + + it('does NOT suppress when // SECURITY: has no text after the colon', async () => { + resolveWith('line1\nline2\nline3\nline4\n// SECURITY:\nline6\n'); + const f = finding({ line: 5 }); + const result = await suppressFindings([f], BASE); + expect(result).toEqual([f]); + }); + + it('calls execFile only once for two findings in the same file (cache hit)', async () => { + resolveWith('line1\nline2\nline3\nline4\nline5\n'); + const f1 = finding({ line: 2 }); + const f2 = finding({ line: 4 }); + await suppressFindings([f1, f2], BASE); + expect(mockExecFile).toHaveBeenCalledTimes(1); + }); + + it('does not crash and keeps the finding when finding.line === 1', async () => { + resolveWith('const x = 1;\nline2\n'); + const f = finding({ line: 1 }); + const result = await suppressFindings([f], BASE); + expect(result).toEqual([f]); + }); + + it('returns the correct filtered array for a mixed batch', async () => { + // file A: has a SECURITY comment above line 3 + const fileAContent = 'line1\n// SECURITY: reviewed by bob\nconst y = 2;\nline4\n'; + // file B: no SECURITY comment + const fileBContent = 'line1\nline2\nline3\n'; + + mockExecFile + .mockImplementationOnce((_cmd, _args, cb) => cb(null, fileAContent, '')) + .mockImplementationOnce((_cmd, _args, cb) => cb(null, fileBContent, '')); + + const fA = finding({ file: 'src/a.js', line: 3 }); // should be suppressed + const fB = finding({ file: 'src/b.js', line: 2 }); // should be kept + + const result = await suppressFindings([fA, fB], BASE); + expect(result).toEqual([fB]); + }); + + it('handles git show failure for file A independently from success for file B', async () => { + rejectWith(new Error('not found')); + resolveWith('line1\nline2\nline3\n'); + + const fA = finding({ file: 'src/a.js', line: 2 }); + const fB = finding({ file: 'src/b.js', line: 2 }); + + const result = await suppressFindings([fA, fB], BASE); + // fA kept (git show failed), fB kept (no SECURITY comment) + expect(result).toEqual([fA, fB]); + }); +}); diff --git a/src/__tests__/worker.test.js b/src/__tests__/worker.test.js index 7b2e24a..55b1497 100644 --- a/src/__tests__/worker.test.js +++ b/src/__tests__/worker.test.js @@ -80,12 +80,17 @@ vi.mock('../notifiers/index.js', () => ({ notify: vi.fn().mockResolvedValue(undefined), })); +vi.mock('../suppressor.js', () => ({ + suppressFindings: vi.fn(async (findings) => findings), +})); + const { Worker: MockWorker } = await import('bullmq'); const { getInstallationToken } = await import('../auth.js'); const { startCheckRun, completeCheckRun, ensureLabelsExist, setLabels, getMergeBaseSha } = await import('../github.js'); const { scanTotal, scanDuration, scanTimeoutsTotal, scanRetriesTotal, findingTotal, findingsPerScan } = await import('../metrics.js'); const { createWorkspace, setupRepo, getChangedFiles, checkoutFiles, cleanupWorkspace } = await import('../fetcher.js'); const { dispatch } = await import('../dispatcher.js'); +const { suppressFindings } = await import('../suppressor.js'); const { loadScanConfig } = await import('../config.js'); const { notify } = await import('../notifiers/index.js'); const { redis } = await import('../queue.js'); @@ -186,6 +191,18 @@ describe('processJob()', () => { })); }); + it('calls suppressFindings with dispatch output and mergeBaseSha', async () => { + const rawFindings = [{ file: 'a.js', line: 1, severity: 'high', message: 'x', ruleId: 'r/1', tool: 'semgrep' }]; + dispatch.mockResolvedValueOnce(rawFindings); + + await processJob(baseJob); + + expect(suppressFindings).toHaveBeenCalledWith(rawFindings, { + workspacePath: '/tmp/layne-test-workspace', + baseSha: 'merge-base-sha', + }); + }); + it('completes the check run with the reporter output', async () => { await processJob(baseJob); expect(completeCheckRun).toHaveBeenCalledWith(expect.objectContaining({ diff --git a/src/suppressor.js b/src/suppressor.js new file mode 100644 index 0000000..6b36655 --- /dev/null +++ b/src/suppressor.js @@ -0,0 +1,56 @@ +import { execFile } from 'child_process'; + +const SECURITY_COMMENT_RE = /(?:\/\/|#)\s*SECURITY:\s+\S/; + +function gitShow(workspacePath, baseSha, filePath) { + return new Promise((resolve, reject) => { + execFile('git', ['-C', workspacePath, 'show', `${baseSha}:${filePath}`], + (err, stdout) => { if (err && !stdout) reject(err); else resolve(stdout ?? ''); } + ); + }); +} + +/** + * Suppresses findings that already have a `// SECURITY: ` comment at + * the base SHA — meaning the comment was reviewed and merged in a prior PR. + * New `// SECURITY:` comments added in the current PR are invisible to this + * function (they're not in base), so self-approval is impossible. + */ +export async function suppressFindings(findings, { workspacePath, baseSha }) { + if (findings.length === 0) return []; + + const fileCache = new Map(); + + async function getLines(filePath) { + if (fileCache.has(filePath)) return fileCache.get(filePath); + let lines = null; + try { + const content = await gitShow(workspacePath, baseSha, filePath); + lines = content.split('\n'); + } catch { + // New file or blob unavailable — no suppression possible + } + fileCache.set(filePath, lines); + return lines; + } + + const kept = []; + for (const finding of findings) { + const lines = await getLines(finding.file); + if (lines === null) { + kept.push(finding); + continue; + } + + const sameLine = lines[finding.line - 1] ?? ''; + const lineAbove = lines[finding.line - 2] ?? ''; + + if (SECURITY_COMMENT_RE.test(sameLine) || SECURITY_COMMENT_RE.test(lineAbove)) { + console.log(`[suppressor] suppressed finding ${finding.file}:${finding.line} [${finding.ruleId}] — SECURITY: comment found at base`); + } else { + kept.push(finding); + } + } + + return kept; +} diff --git a/src/worker.js b/src/worker.js index 7fb2fc6..60bdbf5 100644 --- a/src/worker.js +++ b/src/worker.js @@ -7,6 +7,7 @@ import { getInstallationToken } from './auth.js'; import { startCheckRun, completeCheckRun, ensureLabelsExist, setLabels, getMergeBaseSha } from './github.js'; import { createWorkspace, setupRepo, getChangedFiles, checkoutFiles, cleanupWorkspace } from './fetcher.js'; import { dispatch } from './dispatcher.js'; +import { suppressFindings } from './suppressor.js'; import { buildAnnotations } from './reporter.js'; import { loadScanConfig } from './config.js'; import { notify } from './notifiers/index.js'; @@ -160,7 +161,8 @@ async function runScan(job) { const scanConfig = await loadScanConfig({ owner, repo }); - const findings = await dispatch({ workspacePath, baseSha, baseRef, changedFiles, labels, owner, repo }); + const rawFindings = await dispatch({ workspacePath, baseSha, baseRef, changedFiles, labels, owner, repo }); + const findings = await suppressFindings(rawFindings, { workspacePath, baseSha: mergeBaseSha }); console.log(`[worker] ${findings.length} total finding(s) for ${owner}/${repo} PR #${prNumber} across all tools:`); for (const f of findings) { console.log(`[worker] ${f.tool} ${f.severity.toUpperCase()} ${f.file}:${f.line} [${f.ruleId}] ${f.message}`);