diff --git a/changelog.d/379-security-findings.security.md b/changelog.d/379-security-findings.security.md new file mode 100644 index 00000000..d5d0d19c --- /dev/null +++ b/changelog.d/379-security-findings.security.md @@ -0,0 +1 @@ +Jobs can report the security findings they saw, and each repository now keeps a ledger of them: a finding that was dismissed or fixed and comes back is shown as regressed on the job and repository pages, a dismissal or fix without a written reason is refused, and a finding missing from a later report is never treated as fixed. diff --git a/docs/api.md b/docs/api.md index c55d1b9e..631b677e 100644 --- a/docs/api.md +++ b/docs/api.md @@ -559,7 +559,10 @@ Headers: `Authorization: Bearer `, `Content-Type: application/js "checks": "CI green", "files": 3, "risk": "low", - "rollback": "revert the commit" + "rollback": "revert the commit", + "findings": [ + { "rule": "js/path-injection", "path": "src/app.ts", "line": 120 } + ] }, "checkpoint": "implemented the parser; next: add the rejection test" } @@ -579,7 +582,13 @@ progress heartbeat: it updates `summary`, `checkpoint` and optionally `branch`, into the goal when a failed job is retried. A `plan` job writes the plan to `summary` and must not create a branch, issue or PR; the super admin can execute it later as an iterate job. `evidence` is stored structurally and shown on the job -detail page; the Fledge automation comment stays compact. With +detail page; the Fledge automation comment stays compact. `evidence.findings` is +the security-finding report: up to 100 `{ rule, path, line, state? }` entries, +with `rule` at most 120 characters, `path` at most 200, a positive integer +`line` and `state` one of `open`/`dismissed`. An entry that breaks any of that, +or repeats an id, is dropped and counted in `evidence.findingsDropped`; the +ledger keeps one row per id and is described in +[security.md](security.md#security-finding-ledger-379). With `repos.stage_mode = 'enforced'`, an iterate callback must also include `scope`, `acceptance`, `testRed`, `testGreen` and Dev/Test attribution, and a review callback must include a Reviewer and `reviewScore`; otherwise the job fails with diff --git a/docs/architecture.md b/docs/architecture.md index 9a33c89f..2c8f69a7 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1902,6 +1902,7 @@ listing, and every row links to the page that already owns it. | `repo_members` | per-repository `maintainer`/`developer`/`viewer` rows with a revocation time (#541) | | `scoped_grants` | explicit capability grants with source, evidence, expiry, revocation and revision (#541) | | `identity_migrations` | one report row per identity backfill version (#541) | +| `security_findings` | one row per `rule + path + line` a job reported, with `open`/`fixed`/`dismissed`/`regressed`, the job that dismissed it and the written reason or evidence; absence from a later report never moves a row (#379) | | `settings` | small key/value flags (e.g. quota notice dedupe); age-based pruning reclaims the per-job plan markers (`plan_expired:`, `plan_reminded:`) on the same window as the `jobs` rows | ## Deployment records diff --git a/docs/security.md b/docs/security.md index d8456e6f..c10d02f2 100644 --- a/docs/security.md +++ b/docs/security.md @@ -792,3 +792,39 @@ at an entry outside the actor's audience is refused as if the entry were absent, before the revision fence, so a hidden key is not a write target or a revision oracle. The store refuses a write whose `expected_revision` is stale instead of overwriting the current body. + +## Security-finding ledger (`#379`) + +A job may report the security findings it saw in `evidence.findings`, and the +repository keeps one row per finding id instead of treating every run as a +fresh snapshot. The id is `rule + " " + path + ":" + line` (`js/path-injection +src/app.ts:120`), the list is bounded to 100 entries per report, and an entry +that breaks the contract (rule over 120 characters, path over 200, a line that +is not a positive integer, an unknown state, a duplicate id) is dropped and +counted rather than silently discarded. The block is explicit that these are +**reported** ids: the ledger does not read GitHub's code-scanning alert store in +this slice, so it is a memory of what jobs said, not a second alert source. + +A finding is `open` the first time a job reports it. Two states are decisions, +and each one needs a written reason: + +- `dismissed` is set by a maintainer on the repository page, with a reason; a + dismissal with no reason is refused by the route and by the store, so it can + never be stored as a silent accept (or as `open`). +- `fixed` is set by a maintainer with evidence (a commit, a link or a + measurement). A fix with no evidence is refused, and a `fixed` row whose + evidence is missing is shown as **unverified** instead of "verified fixed". + +The ledger fails closed in both directions. A finding that no later job reports +is not evidence of a fix: absence never moves a row, so only an explicit, +evidenced decision can close one. And a finding that comes back after a decision +is `regressed` — the row keeps the dismissal reason or the fix evidence, the job +page says `regressed (dismissed in job )` when the decision rests on a +dismissal, and the repository page lists it under `regressed` until someone +decides again. The job page lists the findings whose last report was that job, +which is why the line names the job rather than the full history. + +Scope of this slice: recording and displaying reported ids. Reading the ids from +GitHub's alerts (or the SARIF the CI gate already parses) and reconciling the two +sources is a later slice, as is blocking an automatic merge on a `regressed` +finding. diff --git a/src/app.ts b/src/app.ts index 0c77f5c8..bbb06d3a 100644 --- a/src/app.ts +++ b/src/app.ts @@ -2117,6 +2117,9 @@ export async function buildApp(options: BuildOptions): Promise { queued: store.listQueuedJobs(), prJobs: store.listPrJobsForRepo(repo.id, 10), workItems, + // #379: the repository page lists every state the ledger can hold. These + // are the ids jobs reported, not GitHub's alert store. + securityFindings: store.listSecurityFindings(repo.id), commands, // #359: the page names the deadline and the way out, so an ignored plan // is a visible decision with a date rather than a silent wait. @@ -3027,6 +3030,9 @@ export async function buildApp(options: BuildOptions): Promise { repoLabels, events: store.listJobEvents(job.id, 200), relations: store.listTaskRelations(job.id), + // #379: the findings whose last report was this job, so the page can name + // where a dismissed finding was last seen when it came back. + findings: store.listSecurityFindingsForJob(job.id), postmortem: store.getPostmortemForJob(job.id) ?? null, // #540: the cancel form carries the revision it rendered and a fresh key, // so a double submit replays the same receipt and a stale page conflicts. @@ -3469,6 +3475,42 @@ export async function buildApp(options: BuildOptions): Promise { return reply.redirect(`/repos/${repo.id}?${result.merged.length > 0 ? 'notice' : 'error'}=${encodeURIComponent(message)}#delivery`) }) + // #379: the two operator decisions a reported finding can receive. Both need a + // written reason, refused here and again by the store: a dismissal with no + // reason would read as a silent accept, and a fix with no evidence is exactly + // the claim the ledger exists to keep honest. + // #379: the two written decisions on a finding. Both are maintainer-only and + // both refuse a blank reason/evidence in the route *and* in the store, because + // a row that reads as dismissed or fixed with nothing behind it is exactly the + // silent-accept this ledger exists to prevent. + app.post('/repos/:id/security-findings/:findingId/dismiss', async (request, reply) => { + const { viewer, repo } = ownedRepo(request, String((request.params as Record).id), true) + const t = translator(request.locale ?? 'zh') + if (repo.repo_role !== 'maintainer') return reply.redirect(`/repos/${repo.id}?error=${encodeURIComponent(t('只有 maintainer 可以处理安全发现。', 'Only a maintainer can act on a security finding.'))}#security`) + const finding = String((request.params as Record).findingId) + const reason = (form(request).reason ?? '').trim().slice(0, 500) + if (reason === '') return reply.redirect(`/repos/${repo.id}?error=${encodeURIComponent(t('忽略一条安全发现需要写明理由。', 'Dismissing a security finding needs a written reason.'))}#security`) + if (!store.setSecurityFindingState(repo.id, finding, 'dismissed', reason)) { + return reply.redirect(`/repos/${repo.id}?error=${encodeURIComponent(t('这条安全发现不存在。', 'That security finding does not exist.'))}#security`) + } + store.audit(viewer.user.id, 'security_finding.dismissed', `${repo.id}: ${finding}`.slice(0, 600)) + return reply.redirect(`/repos/${repo.id}?notice=${encodeURIComponent(t('已忽略这条安全发现；如果它再次出现，会记为回归。', 'Dismissed. If the same id appears again it is recorded as a regression.'))}#security`) + }) + + app.post('/repos/:id/security-findings/:findingId/fix', async (request, reply) => { + const { viewer, repo } = ownedRepo(request, String((request.params as Record).id), true) + const t = translator(request.locale ?? 'zh') + if (repo.repo_role !== 'maintainer') return reply.redirect(`/repos/${repo.id}?error=${encodeURIComponent(t('只有 maintainer 可以处理安全发现。', 'Only a maintainer can act on a security finding.'))}#security`) + const finding = String((request.params as Record).findingId) + const evidence = (form(request).evidence ?? '').trim().slice(0, 500) + if (evidence === '') return reply.redirect(`/repos/${repo.id}?error=${encodeURIComponent(t('标记已修复需要写明证据（提交、链接或测量值）。', 'Marking a finding fixed needs evidence: a commit, a link or a measurement.'))}#security`) + if (!store.setSecurityFindingState(repo.id, finding, 'fixed', evidence)) { + return reply.redirect(`/repos/${repo.id}?error=${encodeURIComponent(t('这条安全发现不存在。', 'That security finding does not exist.'))}#security`) + } + store.audit(viewer.user.id, 'security_finding.fixed', `${repo.id}: ${finding}`.slice(0, 600)) + return reply.redirect(`/repos/${repo.id}?notice=${encodeURIComponent(t('已记录修复证据；如果同一个 id 再次出现，会记为回归。', 'Fix evidence recorded. If the same id appears again it is recorded as a regression.'))}#security`) + }) + app.post('/repos/:id/work/:workItemId/scope', async (request, reply) => { const { viewer, repo } = ownedRepo(request, String((request.params as Record).id), true) const item = store.getWorkItem(String((request.params as Record).workItemId)) diff --git a/src/db.ts b/src/db.ts index 2b189e2c..e6f695f3 100644 --- a/src/db.ts +++ b/src/db.ts @@ -1438,6 +1438,26 @@ CREATE TABLE IF NOT EXISTS repo_rule_overrides ( PRIMARY KEY (repo_id, path) ); +-- #379: the per-repository security-finding ledger. The id is rule + path + +-- line, so a finding that comes back is the same row. Only an operator decision +-- may write 'dismissed' or 'fixed', and the CHECK below refuses either without +-- the written reason (a dismissal) or evidence (a fix) it rests on; the report +-- path can only move a row to open/regressed. +CREATE TABLE IF NOT EXISTS security_findings ( + repo_id TEXT NOT NULL REFERENCES repos(id) ON DELETE CASCADE, + id TEXT NOT NULL, + state TEXT NOT NULL CHECK (state IN ('open', 'dismissed', 'fixed', 'regressed')), + first_seen_job TEXT NOT NULL, + last_seen_job TEXT NOT NULL, + reason TEXT, + dismissed_in_job TEXT, + updated_at TEXT NOT NULL, + PRIMARY KEY (repo_id, id), + CHECK (state NOT IN ('dismissed', 'fixed') OR (reason IS NOT NULL AND trim(reason) != '')) +); +CREATE INDEX IF NOT EXISTS security_findings_repo_idx ON security_findings(repo_id, state, updated_at DESC); +CREATE INDEX IF NOT EXISTS security_findings_job_idx ON security_findings(last_seen_job); + -- #384: one postmortem per failed/stalled/cancelled job, plus a ledger row per -- recurrence key. The job_id unique constraint is the idempotency guard: a -- replayed terminal transition cannot create a second write-up for the same job. @@ -1940,6 +1960,31 @@ export interface ReleaseTrackingPatch { lastCheckedAt: string } +// #379: one row per reported finding id, per repository. `first_seen_job` and +// `last_seen_job` are job ids kept as text, not foreign keys: the ledger is the +// record of what a job reported, and it must outlive the job row if one is ever +// pruned. `state` is checked by the table, and a decision state requires the +// written reason it rests on. +export interface SecurityFindingRow { + repo_id: string + id: string + state: string + first_seen_job: string + last_seen_job: string + reason: string | null + dismissed_in_job: string | null + updated_at: string +} + +export interface SecurityFindingWrite { + id: string + state: string + firstSeenJob: string + lastSeenJob: string + reason: string | null + dismissedInJob: string | null +} + export class Store { readonly db: Database @@ -4837,6 +4882,77 @@ export class Store { return this.get('SELECT * FROM job_context_files WHERE job_id = ? AND path = ?', jobId, path) } + // #379: the ledger write is one transaction per report, so a report that names + // several findings is recorded whole or not at all. The state machine that + // decides each row lives in src/lib/securityFindings.ts; this method stores + // the decision it returns. + writeSecurityFindings(repoId: string, rows: SecurityFindingWrite[]): void { + if (rows.length === 0) return + const updatedAt = nowIso() + this.transaction(() => { + for (const row of rows) { + this.run( + `INSERT INTO security_findings (repo_id, id, state, first_seen_job, last_seen_job, reason, dismissed_in_job, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + ON CONFLICT(repo_id, id) DO UPDATE SET + state = excluded.state, + last_seen_job = excluded.last_seen_job, + reason = excluded.reason, + dismissed_in_job = excluded.dismissed_in_job, + updated_at = excluded.updated_at`, + repoId, + row.id, + row.state, + row.firstSeenJob, + row.lastSeenJob, + row.reason, + row.dismissedInJob, + updatedAt, + ) + } + }) + } + + getSecurityFinding(repoId: string, id: string): SecurityFindingRow | undefined { + return this.get('SELECT * FROM security_findings WHERE repo_id = ? AND id = ?', repoId, id) + } + + //: Newest first, so the page lists the findings a maintainer is most likely + //: to act on before the older ones. + listSecurityFindings(repoId: string): SecurityFindingRow[] { + return this.all('SELECT * FROM security_findings WHERE repo_id = ? ORDER BY updated_at DESC, id ASC', repoId) + } + + //: The job page shows the findings whose last report was this job. One table + //: keeps the ledger from recording a second, per-job link table; the page + //: says "last reported by this job" rather than claiming the full history. + listSecurityFindingsForJob(jobId: string): SecurityFindingRow[] { + return this.all('SELECT * FROM security_findings WHERE last_seen_job = ? ORDER BY id ASC', jobId) + } + + //: The only write that may set `dismissed` or `fixed`. A blank reason is + //: refused here as well as at the route, so a caller that forgets the check + //: cannot put the ledger into a state that reads as verified without evidence. + setSecurityFindingState(repoId: string, id: string, state: 'dismissed' | 'fixed', reason: string): boolean { + const clean = reason.trim().slice(0, 500) + if (clean === '') return false + const row = this.getSecurityFinding(repoId, id) + if (row === undefined) return false + // `dismissed_in_job` names the job the finding was last seen in when the + // operator decided, which is what the regression line reads back. + const dismissedInJob = state === 'dismissed' ? row.last_seen_job : row.dismissed_in_job + this.run( + 'UPDATE security_findings SET state = ?, reason = ?, dismissed_in_job = ?, updated_at = ? WHERE repo_id = ? AND id = ?', + state, + clean, + dismissedInJob, + nowIso(), + repoId, + id, + ) + return true + } + listRepoRuleOverrides(repoId: string): RepoRuleOverrideRow[] { return this.all('SELECT * FROM repo_rule_overrides WHERE repo_id = ? ORDER BY path', repoId) } diff --git a/src/jobs/service.ts b/src/jobs/service.ts index fc880ca5..84cc31c8 100644 --- a/src/jobs/service.ts +++ b/src/jobs/service.ts @@ -1,6 +1,6 @@ import { createHash } from 'node:crypto' import type { Config } from '../config.js' -import type { JobRow, RepoRow, Store, UserRow } from '../db.js' +import type { JobRow, RepoRow, SecurityFindingWrite, Store, UserRow } from '../db.js' import type { Mailer } from '../email/mailer.js' import { QuotaNotifier } from '../gateway/notifier.js' import { quotaView } from '../gateway/quota.js' @@ -13,6 +13,7 @@ import { redact } from '../lib/secrets.js' import { isValidBranchName } from '../lib/branches.js' import { attributionSentence, parseAttribution, type Attribution } from '../lib/attribution.js' import { parseEvidence, parseEvidenceJson, type Evidence } from '../lib/evidence.js' +import { applyReport, findingId, type FindingState } from '../lib/securityFindings.js' import { classifyFailure, providerRefusalCode, type EnqueueRefusalCode, type FailureCode } from '../lib/failure.js' import { parseModelRun } from '../lib/modelRun.js' import { retryDecision, reviewRetryDecision } from '../lib/retry.js' @@ -1393,6 +1394,25 @@ export class Jobs { finished_at: nowIso(), }) if (!claimed) return false + // #379: the finding ledger is written inside the guarded transition, so a + // replayed callback never reaches this line and the counts it produces are + // the ones the ledger stored. The transition has already committed, so a + // failed ledger write (a locked file, a constraint the report trips) must + // not fail the callback, hold the lease or lose the job result: the rows + // roll back to what they were, which can never make a finding read as + // fixed, and the failure gets a durable row so it cannot vanish. + if (evidence !== null) { + try { + this.recordSecurityFindings(job, evidence) + } catch (error) { + log.warn('security finding ledger write failed', { trace: jobId, error: String(error) }) + try { + this.store.audit(job.user_id, 'security_finding.write_failed', `${job.id}: ${String(error)}`.slice(0, 600)) + } catch (auditError) { + log.warn('security finding ledger failure could not be audited', { trace: jobId, error: String(auditError) }) + } + } + } this.store.releaseJobLease(jobId) this.clearLeaseHeartbeat(jobId) incCounter('fledge_jobs_total', { status: result.status, kind: job.kind }, 1, 'Jobs by terminal status') @@ -1482,6 +1502,49 @@ export class Jobs { return true } + // #379: the finding ledger's only writer, called from the claimed terminal + // transition. It touches only the ids this report names: a finding missing + // from a later report is not evidence of a fix, so absence cannot move a row. + // A dismissal without a reason and a malformed entry are refused here and + // counted, never turned into a clean report. + private recordSecurityFindings(job: JobRow, evidence: Evidence): void { + const findings = evidence.findings ?? [] + const refused = evidence.findingsDismissedRefused ?? 0 + const dropped = evidence.findingsDropped ?? 0 + if (findings.length === 0 && refused === 0 && dropped === 0) return + const writes: SecurityFindingWrite[] = [] + let regressed = 0 + for (const finding of findings) { + const id = findingId(finding) + const previous = this.store.getSecurityFinding(job.repo_id, id) + const decision = applyReport( + previous === undefined + ? undefined + : { state: previous.state as FindingState, firstSeenJob: previous.first_seen_job, dismissedInJob: previous.dismissed_in_job }, + finding, + job.id, + ) + if (decision === null) continue + writes.push({ + id, + state: decision.state, + firstSeenJob: previous?.first_seen_job ?? job.id, + lastSeenJob: decision.lastSeenJob, + reason: previous?.reason ?? null, + dismissedInJob: decision.dismissedInJob, + }) + if (decision.state === 'regressed' && previous?.state !== 'regressed') { + regressed += 1 + this.store.audit(job.user_id, 'security_finding.regressed', `${job.id}: ${id} first=${previous?.first_seen_job ?? job.id}`.slice(0, 600)) + } + } + this.store.writeSecurityFindings(job.repo_id, writes) + if (refused > 0) this.store.audit(job.user_id, 'security_finding.dismissal_refused', `${job.id}: ${refused} dismissed report(s) without a reason`) + const summary = `findings=${findings.length} regressed=${regressed} dismissed_refused=${refused} dropped=${dropped}` + this.store.event('security_findings', { userId: job.user_id, repoId: job.repo_id, jobId: job.id, meta: summary }) + this.store.audit(job.user_id, 'job.security_findings', `${job.id}: ${summary}`) + } + private notePostmortem(jobId: string): void { try { const record = this.store.createPostmortemForJob(jobId) diff --git a/src/lib/evidence.ts b/src/lib/evidence.ts index fcc88bfb..64aa05cc 100644 --- a/src/lib/evidence.ts +++ b/src/lib/evidence.ts @@ -1,3 +1,5 @@ +import { parseFindings, type ReportedFinding } from './securityFindings.js' + export interface Evidence { scope?: string acceptance?: string @@ -9,6 +11,12 @@ export interface Evidence { files?: number risk?: string rollback?: string + //: #379: the security findings this job reports, already bounded and + //: validated. Reported ids only — the ledger behind them is not GitHub's + //: alert store, and an id missing from one report proves nothing. + findings?: ReportedFinding[] + findingsDropped?: number + findingsDismissedRefused?: number } const RISK = new Set(['low', 'medium', 'high', 'unknown']) @@ -33,6 +41,9 @@ export function parseEvidence(value: unknown): Evidence | null { const rollback = text(raw.rollback) const risk = text(raw.risk, 20)?.toLowerCase() const files = typeof raw.files === 'number' && Number.isFinite(raw.files) ? Math.max(0, Math.round(raw.files)) : undefined + // #379: a malformed list is counted, not silently empty: a job whose findings + // were all refused must not look like a job that found nothing. + const parsedFindings = parseFindings(raw.findings) if (scope !== undefined) out.scope = scope if (acceptance !== undefined) out.acceptance = acceptance if (testPlan !== undefined) out.testPlan = testPlan @@ -43,6 +54,9 @@ export function parseEvidence(value: unknown): Evidence | null { if (rollback !== undefined) out.rollback = rollback if (files !== undefined) out.files = files if (risk !== undefined && RISK.has(risk)) out.risk = risk + if (parsedFindings.findings.length > 0) out.findings = parsedFindings.findings + if (parsedFindings.dropped > 0) out.findingsDropped = parsedFindings.dropped + if (parsedFindings.refusedDismissals > 0) out.findingsDismissedRefused = parsedFindings.refusedDismissals return Object.keys(out).length === 0 ? null : out } diff --git a/src/lib/securityFindings.ts b/src/lib/securityFindings.ts new file mode 100644 index 00000000..89de833d --- /dev/null +++ b/src/lib/securityFindings.ts @@ -0,0 +1,129 @@ +// #379: the per-repository ledger of security findings that jobs report. +// +// A finding is identified by the triple rule + path + line, so the same defect +// reported by a later job lands on the same row instead of looking new. The +// ledger keeps three different questions apart: +// +// * what a job reported (this module's `applyReport`), which may only move a +// row that the report names — absence is never evidence of a fix; +// * what an operator decided (`dismissed` needs a written reason, `fixed` +// needs written evidence), which is the only path to those two states; +// * what the reader sees (`fixed` without evidence is displayed as +// unverified, and a finding seen again after a decision is `regressed`). +// +// The module is pure: no clock, no database, no network. The caller passes the +// job id and reads the result back. + +export type FindingState = 'open' | 'dismissed' | 'fixed' | 'regressed' + +export type ReportedState = 'open' | 'dismissed' + +export interface ReportedFinding { + rule: string + path: string + line: number + state: ReportedState +} + +export interface ParsedFindings { + findings: ReportedFinding[] + //: Entries that named no usable finding. The count is kept so a report cannot + //: look empty and clean when its entries were all malformed. + dropped: number + //: Entries that claimed `dismissed`. A dismissal needs a reason an operator + //: writes, so the callback cannot set that state; the count makes the refusal + //: visible instead of turning it into a silent `open`. + refusedDismissals: number +} + +export interface LedgerDecision { + state: FindingState + lastSeenJob: string + dismissedInJob: string | null +} + +export const FINDINGS_MAX = 100 +export const RULE_MAX = 120 +export const PATH_MAX = 200 + +const STATES: ReadonlySet = new Set(['open', 'dismissed']) + +export function findingId(finding: { rule: string; path: string; line: number }): string { + return `${finding.rule} ${finding.path}:${finding.line}` +} + +//: A bounded string, or undefined when the report broke the contract. Unlike a +//: truncated value, a refused one cannot pass a longer string off as its prefix. +function bounded(value: unknown, limit: number): string | undefined { + if (typeof value !== 'string') return undefined + const trimmed = value.trim() + if (trimmed === '' || trimmed.length > limit) return undefined + return trimmed +} + +function parseFinding(value: unknown): ReportedFinding | null { + if (value === null || typeof value !== 'object' || Array.isArray(value)) return null + const raw = value as Record + const rule = bounded(raw.rule, RULE_MAX) + const path = bounded(raw.path, PATH_MAX) + const line = raw.line + const state = raw.state === undefined ? 'open' : raw.state + if (rule === undefined || path === undefined) return null + if (typeof line !== 'number' || !Number.isInteger(line) || line <= 0) return null + if (typeof state !== 'string' || !STATES.has(state)) return null + return { rule, path, line, state: state as ReportedState } +} + +export function parseFindings(value: unknown): ParsedFindings { + if (!Array.isArray(value)) return { findings: [], dropped: 0, refusedDismissals: 0 } + const findings: ReportedFinding[] = [] + const seen = new Set() + let dropped = 0 + let refusedDismissals = 0 + for (const entry of value) { + const parsed = parseFinding(entry) + if (parsed === null) { + dropped += 1 + continue + } + if (parsed.state === 'dismissed') { + // The callback carries no reason, so it may not set the dismissed state. + refusedDismissals += 1 + continue + } + if (findings.length >= FINDINGS_MAX || seen.has(findingId(parsed))) { + dropped += 1 + continue + } + seen.add(findingId(parsed)) + findings.push(parsed) + } + return { findings, dropped, refusedDismissals } +} + +//: What one report does to a row. `undefined` means "this entry may not move +//: the ledger". An id the report does not name is not passed here at all, which +//: is the fail-closed rule: a missing finding is never a fix. +export function applyReport( + previous: { state: FindingState; firstSeenJob: string; dismissedInJob: string | null } | undefined, + reported: ReportedFinding, + jobId: string, +): LedgerDecision | null { + // A dismissal without a reason is not a state this ledger can hold. + if (reported.state !== 'open') return null + if (previous === undefined) return { state: 'open', lastSeenJob: jobId, dismissedInJob: null } + if (previous.state === 'dismissed') { + // The page names the job the finding was dismissed in; the first sighting + // is the fallback for a row dismissed before that field was written. + return { state: 'regressed', lastSeenJob: jobId, dismissedInJob: previous.dismissedInJob ?? previous.firstSeenJob } + } + if (previous.state === 'fixed') return { state: 'regressed', lastSeenJob: jobId, dismissedInJob: null } + return { state: previous.state, lastSeenJob: jobId, dismissedInJob: previous.dismissedInJob } +} + +//: A fixed row is only "verified" when the evidence the fix rested on is still +//: recorded. The store refuses to write one without it; the view calls the row +//: unverified rather than trusting the state alone. +export function isVerifiedFixed(row: { state: string; reason: string | null }): boolean { + return row.state === 'fixed' && row.reason !== null && row.reason.trim() !== '' +} diff --git a/src/views/pages.ts b/src/views/pages.ts index 2e40188c..8f24d204 100644 --- a/src/views/pages.ts +++ b/src/views/pages.ts @@ -1,6 +1,7 @@ import type { Config } from '../config.js' import type { CostReport } from '../metrics/cost.js' -import type { AlertView, AuditRow, DeploymentRollbackRow, DeploymentRow, JobEventRow, JobRow, ModelProviderRow, NotificationRow, OwnedSearch, PostmortemRecurrenceRow, PostmortemRow, ProviderModelRow, ReleaseRow, RepoInstanceRow, RepoRow, ReviewTrendWindow, SessionRow, UserRow, ApiTokenRow, WorkflowFailureRow, WorkItemRow, TaskRelationRow } from '../db.js' +import type { AlertView, AuditRow, DeploymentRollbackRow, DeploymentRow, JobEventRow, JobRow, ModelProviderRow, NotificationRow, OwnedSearch, PostmortemRecurrenceRow, PostmortemRow, ProviderModelRow, ReleaseRow, RepoInstanceRow, RepoRow, ReviewTrendWindow, SecurityFindingRow, SessionRow, UserRow, ApiTokenRow, WorkflowFailureRow, WorkItemRow, TaskRelationRow } from '../db.js' +import { isVerifiedFixed } from '../lib/securityFindings.js' import { assetUrl } from '../lib/assetVersion.js' import { redactCommand, type DriftAssessment, type EnvironmentCurrent, type ReleaseSnapshot } from '../lib/deployments.js' import { agreementContent } from '../users/agreements.js' @@ -58,6 +59,76 @@ function csrf(viewer: Viewer): string { return `` } +// #379: the ledger's state labels. A fixed row is only called verified when it +// still carries the evidence the fix rested on; the store refuses to write one +// without it, and this helper is the second guard for an older or hand-built +// row. A regressed row names the job the finding was dismissed in. +function findingStateText(t: Translate, row: { state: string; reason: string | null; dismissed_in_job: string | null }): string { + if (row.state === 'open') return t('未修复', 'Open') + if (row.state === 'dismissed') return t('已忽略', 'Dismissed') + if (row.state === 'regressed') { + return row.dismissed_in_job === null + ? t('回归（此前已修复）', 'Regressed (was fixed)') + : t(`回归（在任务 ${row.dismissed_in_job} 被忽略）`, `Regressed (dismissed in job ${row.dismissed_in_job})`) + } + return isVerifiedFixed(row) + ? t(`已修复 — ${row.reason ?? ''}`, `Fixed — ${row.reason ?? ''}`) + : t('已修复（没有记录证据，未核实）', 'Fixed (no evidence recorded)') +} + +// #379: the repository card lists the four states and offers the two written +// decisions. The forms are maintainer-only, matching the routes; the listing +// itself is readable by whoever can see the repository. +function securityFindingsCard(t: Translate, viewer: Viewer, repo: RepoRow, rows: SecurityFindingRow[]): string { + const count = (state: string): number => rows.filter((row) => row.state === state).length + const summary = `Open ${count('open')} · Fixed ${count('fixed')} · Regressed ${count('regressed')} · Dismissed ${count('dismissed')}` + const actions = (row: SecurityFindingRow): string => { + if (repo.repo_role !== 'maintainer') return '' + const base = `/repos/${escapeHtml(repo.id)}/security-findings/${escapeHtml(encodeURIComponent(row.id))}` + const hidden = csrf(viewer) + return `
+
+ ${hidden} + + +
+
+ ${hidden} + + +
+
` + } + const list = rows.length === 0 + ? `

${escapeHtml(t('还没有任务上报过安全发现。', 'No job has reported a security finding yet.'))}

` + : `
${rows.map((row) => { + const reason = row.reason === null || row.reason === '' || row.state === 'open' + ? '' + : `${escapeHtml(row.reason)}` + return `
${escapeHtml(row.id)} + ${escapeHtml(findingStateText(t, row))} · ${escapeHtml(t('首次', 'first'))} ${escapeHtml(row.first_seen_job)} · ${escapeHtml(t('最近', 'last'))} ${escapeHtml(row.last_seen_job)} + ${reason}${actions(row)}
` + }).join('')}
` + return `
+

${escapeHtml(t('安全发现', 'Security findings'))}

+

${escapeHtml(t('这些是任务上报的 id，不是 GitHub 的告警库；某个 id 在后续任务里没出现，不能当作已修复。', "These are ids reported by jobs, not GitHub's alert store; an id that a later job does not report is not evidence of a fix."))}

+

${escapeHtml(summary)}

+ ${list} +
` +} + +// #379: the job page names what this job last reported and where a dismissed +// finding was dismissed, so a regression is readable without leaving the job. +function jobSecurityFindingsCard(t: Translate, rows: SecurityFindingRow[]): string { + if (rows.length === 0) return '' + return `
+

${escapeHtml(t('安全发现', 'Security findings'))}

+

${escapeHtml(t('本任务最近上报的 id，不是 GitHub 的告警库。', "The ids this job last reported — not GitHub's alert store."))}

+
${rows.map((row) => `
${escapeHtml(row.id)}${escapeHtml(findingStateText(t, row))}
`).join('')}
+
` +} + + // #540: a console write carries the revision it rendered and a key that a double // submit replays. One renderer, so run, plan, review, triage, start, scope, // verify, execute, resume and retry all name the same two fields. @@ -1151,7 +1222,7 @@ export function dashboardPage(input: { return layout({ title: t('仓库', 'Repositories'), viewer, body, config, locale, path: '/dashboard', active: 'dashboard', notice, error }) } -export function repoPage(input: { config: Config; locale: Locale; viewer: Viewer; recentAuthAt?: string | null; repo: RepoRow; instance?: RepoInstanceRow; jobs: JobRow[]; prJobs?: JobRow[]; workItems?: WorkItemRow[]; pendingPlan?: { jobId: string; deadline: string }; workState?: string; reviewTrend?: { week: ReviewTrendWindow; month: ReviewTrendWindow }; commands?: Record; models?: Array; repoModelIds?: string[]; repoDefaultModelId?: string; repoBotModels?: Array<{ role: string; provider_model_id: string }>; repoSpendCny?: number; questions?: InboxQuestion[]; chiefMessages?: ChiefMessage[]; activeJob?: boolean; jobsPage?: number; jobsTotal?: number; queued?: JobRow[]; notice?: string; error?: string; releases?: ReleaseRow[]; releaseRefused?: string; releaseTagged?: string; releaseRetagged?: string; releaseChecked?: string; postmortems?: PostmortemRow[]; recurrences?: PostmortemRecurrenceRow[]; providers?: ModelProviderRow[]; workflowFailures?: WorkflowFailureRow[]; usage?: { repos: number; reposCap: number; instances: number; instancesCap: number }; scheduledNext?: ScheduledNext }): string { +export function repoPage(input: { config: Config; locale: Locale; viewer: Viewer; recentAuthAt?: string | null; repo: RepoRow; instance?: RepoInstanceRow; jobs: JobRow[]; prJobs?: JobRow[]; workItems?: WorkItemRow[]; pendingPlan?: { jobId: string; deadline: string }; workState?: string; reviewTrend?: { week: ReviewTrendWindow; month: ReviewTrendWindow }; commands?: Record; models?: Array; repoModelIds?: string[]; repoDefaultModelId?: string; repoBotModels?: Array<{ role: string; provider_model_id: string }>; repoSpendCny?: number; questions?: InboxQuestion[]; chiefMessages?: ChiefMessage[]; activeJob?: boolean; jobsPage?: number; jobsTotal?: number; queued?: JobRow[]; notice?: string; error?: string; releases?: ReleaseRow[]; releaseRefused?: string; releaseTagged?: string; releaseRetagged?: string; releaseChecked?: string; postmortems?: PostmortemRow[]; recurrences?: PostmortemRecurrenceRow[]; providers?: ModelProviderRow[]; workflowFailures?: WorkflowFailureRow[]; usage?: { repos: number; reposCap: number; instances: number; instancesCap: number }; scheduledNext?: ScheduledNext; securityFindings?: SecurityFindingRow[] }): string { const { config, locale, viewer, repo, instance, jobs, notice, error } = input const ranks = input.queued === undefined ? undefined : queueRanks(input.queued) const workItems = input.workItems ?? [] @@ -1167,6 +1238,7 @@ export function repoPage(input: { config: Config; locale: Locale; viewer: Viewer const repoSpendCny = input.repoSpendCny ?? 0 const questions = input.questions ?? [] const chiefMessages = input.chiefMessages ?? [] + const securityFindings = input.securityFindings ?? [] const jobsPage = input.jobsPage ?? 1 const jobsTotal = input.jobsTotal ?? jobs.length const t = translator(locale) @@ -1696,7 +1768,7 @@ export function repoPage(input: { config: Config; locale: Locale; viewer: Viewer return `
  • ${escapeHtml(release.tag_name)} ${escapeHtml(release.target_sha.slice(0, 12))} · ${escapeHtml(release.status)}${run}${external}${draft}
    ${csrf(viewer)}
    • ` }).join('')}`} ` - const body = `
    ${repoHeader}${repoTabs}${repoSection('overview', `${planBanner}${lifecycleBanner}${triageCard}${appAttribution ? `` : ''}${inboxSection(locale, viewer, questions, `/repos/${repo.id}`)}${chiefCard}${memoryCard}${gateCard}${rulesCard}`)}${repoSection('delivery', `${releaseCard}${workflowFailuresCard}${postmortemCard}${reviewTrendCard}${deliveryCard}${jobsCard}`)}${repoSection('work', `${workBoard}${workCard}`)}${repoSection('team', `${roleCard}${instanceNotice}${instanceCard}`)}${repoSection('settings', settingsCards)}${repoSection('danger', dangerCard)}
    ` + const body = `
    ${repoHeader}${repoTabs}${repoSection('overview', `${planBanner}${lifecycleBanner}${triageCard}${appAttribution ? `` : ''}${inboxSection(locale, viewer, questions, `/repos/${repo.id}`)}${chiefCard}${memoryCard}${gateCard}${rulesCard}${securityFindingsCard(t, viewer, repo, securityFindings)}`)}${repoSection('delivery', `${releaseCard}${workflowFailuresCard}${postmortemCard}${reviewTrendCard}${deliveryCard}${jobsCard}`)}${repoSection('work', `${workBoard}${workCard}`)}${repoSection('team', `${roleCard}${instanceNotice}${instanceCard}`)}${repoSection('settings', settingsCards)}${repoSection('danger', dangerCard)}
    ` return layout({ title: `${upstreamOwner}/${upstreamName}`, viewer, body: `${body}${advanced ? '' : ``}`, config, locale, path: `/repos/${repo.id}`, active: 'dashboard', notice, error }) } @@ -2734,6 +2806,9 @@ export function jobPage(input: { postmortem?: PostmortemRow | null // #639: relations are explicit; the page lists them and offers one write. relations?: TaskRelationRow[] + //: #379: the findings whose last report was this job, plus the job page line + //: that names where a dismissed finding was dismissed. + findings?: SecurityFindingRow[] // #373 slice 1: the "Why this change" block, read from the pull request. Absent // on a surface that has no pull request to read. changeStory?: ChangeStory @@ -2763,6 +2838,7 @@ export function jobPage(input: { const events = input.events ?? [] const postmortem = input.postmortem ?? null const relations = input.relations ?? [] + const findings = input.findings ?? [] const supportMode = input.supportMode === true const t = translator(locale) const advanced = advancedViewer(viewer) @@ -3086,6 +3162,7 @@ export function jobPage(input: { ${supportMode ? '' : workTypeCard} ${supportMode ? '' : triageCard} ${changeStoryCard} + ${jobSecurityFindingsCard(t, findings)}

    ${escapeHtml(t('结果', 'Result'))}

    diff --git a/test/securityFindings.test.ts b/test/securityFindings.test.ts new file mode 100644 index 00000000..2cf20fe5 --- /dev/null +++ b/test/securityFindings.test.ts @@ -0,0 +1,386 @@ +import test from 'node:test' +import assert from 'node:assert/strict' +import { buildApp } from '../src/app.js' +import { Store } from '../src/db.js' +import type { RepoRow, SecurityFindingRow } from '../src/db.js' +import { Jobs } from '../src/jobs/service.js' +import { MockRunner } from '../src/jobs/mock.js' +import { findingId, parseFindings } from '../src/lib/securityFindings.js' +import { encryptSecret, sha256Hex } from '../src/lib/secrets.js' +import { jobPage, repoPage } from '../src/views/pages.js' +import type { Viewer } from '../src/views/layout.js' +import { formBody, makeUser, MemoryMailer, sessionFor, testConfig } from './helpers.js' + +// #379 slice 1: reported security findings are kept per repository, so a fixed +// or dismissed finding that comes back is visible as `regressed` instead of +// looking new. The two properties this suite exists for are negative ones: +// absence of a finding in a later report is never evidence of a fix, and a +// dismissal or a fix without a written reason does not become a state. + +const RULE = 'js/path-injection' +const PATH = 'src/app.ts' +const LINE = 120 +const ID = `${RULE} ${PATH}:${LINE}` + +// #472: a redirect sentence travels in the one-shot flash cookie and is stripped +// from Location before the browser sees it, so a refusal is read from the cookie +// rather than from the URL. Decoded here directly rather than imported, so this +// file fails on the behaviour and not on a missing helper. +function flashedText(response: { headers: Record }): string { + const header = response.headers['set-cookie'] + const lines = header === undefined ? [] : (Array.isArray(header) ? header : [header]).map(String) + const line = lines.find((entry) => entry.startsWith('fledge_flash=')) + if (line === undefined) return '' + const raw = line.slice('fledge_flash='.length).split(';')[0] ?? '' + if (raw === '') return '' + const parsed = JSON.parse(Buffer.from(raw, 'base64url').toString('utf8')) as { e?: { t?: unknown }; n?: { t?: unknown } } + if (typeof parsed.e?.t === 'string') return parsed.e.t + return typeof parsed.n?.t === 'string' ? parsed.n.t : '' +} + +interface Harness { + store: Store + jobs: Jobs + repo: RepoRow + userId: string + config: ReturnType +} + +function harness(): Harness { + const config = testConfig() + const store = new Store(':memory:') + const jobs = new Jobs(store, config, new MockRunner(), new MemoryMailer()) + const user = makeUser(store, { login: 'findings-user', status: 'approved' }) + const repo = store.createRepo({ + id: 'repo_findings', + userId: user.id, + owner: 'o', + name: 'r', + defaultBranch: 'main', + tokenEnc: encryptSecret('gh-token', config.tokenEncryptionKey), + permission: 'admin', + repoRole: 'maintainer', + goal: '', + autoIterate: false, + intervalMinutes: 1440, + }) + return { store, jobs, repo, userId: user.id, config } +} + +function jobRow(h: Harness, id: string, secret: string): void { + h.store.createJob({ + id, + repoId: h.repo.id, + userId: h.userId, + trigger: 'manual', + goal: 'g', + callbackHash: sha256Hex(secret), + platformTokenHash: '', + kind: 'iterate', + baseBranch: 'main', + }) + h.store.updateJob(id, { status: 'running', started_at: new Date().toISOString(), branch: 'fledge/job' }) +} + +async function report(h: Harness, id: string, findings: unknown[]): Promise { + const secret = `secret-${id}` + jobRow(h, id, secret) + const result = await h.jobs.handleCallback(id, secret, { status: 'succeeded', summary: 'done', evidence: { findings } }) + assert.equal(result.ok, true, result.error ?? 'the callback was refused') +} + +function viewer(h: Harness): Viewer { + const user = h.store.userById(h.userId) + assert.ok(user !== undefined) + return { user, csrfToken: 'csrf' } +} + +test('the reported list is bounded at 100 and a malformed entry is dropped and counted', () => { + const valid = Array.from({ length: 100 }, (_, index) => ({ rule: 'js/x', path: `src/f${index}.ts`, line: index + 1 })) + const parsed = parseFindings([ + ...valid, + // The 101st valid entry is over the bound. + { rule: 'js/x', path: 'src/over.ts', line: 1 }, + { rule: 'r'.repeat(121), path: 'src/a.ts', line: 1 }, + { rule: 'js/x', path: 'p'.repeat(201), line: 1 }, + { rule: 'js/x', path: 'src/a.ts', line: 0 }, + { rule: 'js/x', path: 'src/a.ts', line: 1.5 }, + { rule: '', path: 'src/a.ts', line: 1 }, + { rule: 'js/x', path: 'src/a.ts', line: 1, state: 'fixed' }, + null, + ]) + assert.equal(parsed.findings.length, 100, 'the ledger keeps at most 100 findings per report') + assert.equal(parsed.dropped, 8, 'every refused entry is counted, not silently ignored') + const first = parsed.findings[0] + assert.ok(first !== undefined) + assert.equal(first.state, 'open', 'a finding with no state is reported as open') +}) + +test('the id is rule plus path plus line, so one rule on two paths is two findings', () => { + assert.equal(findingId({ rule: RULE, path: PATH, line: LINE }), ID) + assert.notEqual( + findingId({ rule: RULE, path: 'src/a.ts', line: LINE }), + findingId({ rule: RULE, path: 'src/b.ts', line: LINE }), + ) +}) + +test('a reported finding is open, a dismissal needs a reason, and a re-report is regressed', async () => { + const h = harness() + try { + await report(h, 'job-a', [{ rule: RULE, path: PATH, line: LINE }]) + const opened = h.store.getSecurityFinding(h.repo.id, ID) + assert.equal(opened?.state, 'open') + assert.equal(opened?.first_seen_job, 'job-a') + assert.equal(opened?.last_seen_job, 'job-a') + + // The store refuses a dismissal without a written reason, so it can never be + // stored as a silent accept. + assert.equal(h.store.setSecurityFindingState(h.repo.id, ID, 'dismissed', ' '), false) + assert.equal(h.store.getSecurityFinding(h.repo.id, ID)?.state, 'open') + assert.equal(h.store.setSecurityFindingState(h.repo.id, ID, 'dismissed', 'false positive in the fixture'), true) + assert.equal(h.store.getSecurityFinding(h.repo.id, ID)?.state, 'dismissed') + + await report(h, 'job-b', [{ rule: RULE, path: PATH, line: LINE }]) + const regressed = h.store.getSecurityFinding(h.repo.id, ID) + assert.equal(regressed?.state, 'regressed', 'a dismissed finding that comes back is regressed, not fresh') + assert.equal(regressed?.last_seen_job, 'job-b') + assert.equal(regressed?.dismissed_in_job, 'job-a') + + const html = jobPage({ + config: h.config, + locale: 'en', + viewer: viewer(h), + repo: h.repo, + job: h.store.getJob('job-b')!, + findings: h.store.listSecurityFindingsForJob('job-b'), + }) + // The label is capitalised like every other state label; what this asserts is + // the parenthetical, which is the job the dismissal was recorded in. + assert.match(html, /regressed \(dismissed in job job-a\)/i, 'the job page names the job the finding was dismissed in') + } finally { + h.store.close() + } +}) + +test('two findings with the same rule on different paths are two rows', async () => { + const h = harness() + try { + await report(h, 'job-a', [ + { rule: RULE, path: 'src/a.ts', line: 1 }, + { rule: RULE, path: 'src/b.ts', line: 1 }, + ]) + assert.equal(h.store.listSecurityFindings(h.repo.id).length, 2) + assert.equal(h.store.getSecurityFinding(h.repo.id, `${RULE} src/a.ts:1`)?.state, 'open') + assert.equal(h.store.getSecurityFinding(h.repo.id, `${RULE} src/b.ts:1`)?.state, 'open') + } finally { + h.store.close() + } +}) + +test('a replayed callback writes one row and leaves one last_seen value', async () => { + const h = harness() + try { + const secret = 'replay-secret' + jobRow(h, 'job-a', secret) + const payload = { status: 'succeeded' as const, summary: 'done', evidence: { findings: [{ rule: RULE, path: PATH, line: LINE }] } } + assert.equal((await h.jobs.handleCallback('job-a', secret, payload)).ok, true) + const firstSeen = h.store.getSecurityFinding(h.repo.id, ID)?.updated_at + assert.equal((await h.jobs.handleCallback('job-a', secret, payload)).duplicate, true) + assert.equal(h.store.listSecurityFindings(h.repo.id).length, 1) + assert.equal(h.store.getSecurityFinding(h.repo.id, ID)?.updated_at, firstSeen, 'a replay does not touch the row') + } finally { + h.store.close() + } +}) + +test('a finding missing from a later report is not evidence of a fix', async () => { + const h = harness() + try { + await report(h, 'job-a', [{ rule: RULE, path: PATH, line: LINE }]) + await report(h, 'job-b', [{ rule: RULE, path: 'src/other.ts', line: 7 }]) + const untouched = h.store.getSecurityFinding(h.repo.id, ID) + assert.equal(untouched?.state, 'open', 'absence changes nothing: only a reported id may move a row') + assert.equal(untouched?.last_seen_job, 'job-a') + // An empty report is the strongest form of absence and must not claim a fix. + await report(h, 'job-c', []) + assert.equal(h.store.getSecurityFinding(h.repo.id, ID)?.state, 'open') + } finally { + h.store.close() + } +}) + +test('a fix needs evidence, and a fixed finding that comes back is regressed', async () => { + const h = harness() + try { + await report(h, 'job-a', [{ rule: RULE, path: PATH, line: LINE }]) + assert.equal(h.store.setSecurityFindingState(h.repo.id, ID, 'fixed', ''), false, 'a fix without evidence is refused') + assert.equal(h.store.getSecurityFinding(h.repo.id, ID)?.state, 'open') + assert.equal(h.store.setSecurityFindingState(h.repo.id, ID, 'fixed', 'removed the sink in 4a1b2c3'), true) + const fixed = h.store.getSecurityFinding(h.repo.id, ID) + assert.equal(fixed?.state, 'fixed') + assert.equal(fixed?.reason, 'removed the sink in 4a1b2c3') + + await report(h, 'job-b', [{ rule: RULE, path: PATH, line: LINE }]) + const regressed = h.store.getSecurityFinding(h.repo.id, ID) + assert.equal(regressed?.state, 'regressed', 'the ledger refuses to keep claiming fixed once the finding is seen again') + assert.match(regressed?.reason ?? '', /4a1b2c3/, 'the evidence the fix rested on is kept with the regression') + const html = jobPage({ + config: h.config, + locale: 'en', + viewer: viewer(h), + repo: h.repo, + job: h.store.getJob('job-b')!, + findings: h.store.listSecurityFindingsForJob('job-b'), + }) + assert.match(html, /Regressed/) + assert.doesNotMatch(html, /Fixed —/, 'the page does not present the re-opened finding as fixed') + } finally { + h.store.close() + } +}) + +test('a callback that reports a dismissal without a reason does not become a state', async () => { + const h = harness() + try { + await report(h, 'job-a', [{ rule: RULE, path: PATH, line: LINE, state: 'dismissed' }]) + assert.equal(h.store.getSecurityFinding(h.repo.id, ID), undefined, 'refused, not stored as open') + const audit = h.store.listAudit(20).find((row) => row.action === 'security_finding.dismissal_refused') + assert.ok(audit !== undefined, 'the refusal is audited rather than silent') + assert.match(String(audit.detail), /job-a/) + // A recorded dismissal carries its reason; a report never invents one. + await report(h, 'job-b', [{ rule: RULE, path: PATH, line: LINE }]) + const row = h.store.getSecurityFinding(h.repo.id, ID) + assert.equal(row?.state, 'open') + assert.equal(row?.reason, null) + } finally { + h.store.close() + } +}) + +test('a finding the report no longer names leaves a fixed row fixed and its evidence intact', async () => { + const h = harness() + try { + await report(h, 'job-a', [{ rule: RULE, path: PATH, line: LINE }]) + assert.equal(h.store.setSecurityFindingState(h.repo.id, ID, 'fixed', 'removed the sink in 4a1b2c3'), true) + await report(h, 'job-b', []) + const row = h.store.getSecurityFinding(h.repo.id, ID) + assert.equal(row?.state, 'fixed') + assert.equal(row?.reason, 'removed the sink in 4a1b2c3') + } finally { + h.store.close() + } +}) + +test('a ledger write that fails leaves the job finished and says so', async () => { + const h = harness() + const secret = 'secret-job-write-fails' + jobRow(h, 'job-write-fails', secret) + const original = h.store.writeSecurityFindings.bind(h.store) + try { + // The ledger is memory, not the job. A write that fails (a locked file, a + // constraint the report trips) must not leave the terminal transition's + // side effects half done or the callback failing after the row committed — + // and it must not vanish either, so the failure gets a durable row. + Object.defineProperty(h.store, 'writeSecurityFindings', { + value: () => { + throw new Error('database is locked') + }, + configurable: true, + }) + const result = await h.jobs.handleCallback('job-write-fails', secret, { + status: 'succeeded', + summary: 'done', + evidence: { findings: [{ rule: RULE, path: PATH, line: LINE }] }, + }) + assert.equal(result.ok, true, result.error ?? 'the job still finishes') + assert.equal(h.store.getJob('job-write-fails')?.status, 'succeeded') + assert.equal(h.store.getSecurityFinding(h.repo.id, ID), undefined, 'a failed ledger write stores no row') + assert.ok( + h.store.listAudit(50).some((entry) => entry.action === 'security_finding.write_failed'), + 'the failed write is recorded instead of disappearing into a log', + ) + } finally { + Object.defineProperty(h.store, 'writeSecurityFindings', { value: original, configurable: true }) + h.store.close() + } +}) + +test('the dismiss route refuses an empty reason and accepts a written one', async () => { + const h = harness() + const built = await buildApp({ config: h.config, store: h.store, mailer: new MemoryMailer(), runner: new MockRunner(), startScheduler: false }) + try { + await report(h, 'job-a', [{ rule: RULE, path: PATH, line: LINE }]) + const session = sessionFor(built, h.store.userById(h.userId)!) + const url = `/repos/${h.repo.id}/security-findings/${encodeURIComponent(ID)}/dismiss` + const headers = { cookie: session.cookie, 'content-type': 'application/x-www-form-urlencoded', 'accept-language': 'en' } + const refused = await built.app.inject({ method: 'POST', url, headers, payload: formBody({ csrf: session.csrf, reason: ' ' }) }) + assert.equal(refused.statusCode, 302) + assert.match(flashedText(refused), /reason/i) + assert.equal(h.store.getSecurityFinding(h.repo.id, ID)?.state, 'open', 'a refusal writes no state') + + const accepted = await built.app.inject({ method: 'POST', url, headers, payload: formBody({ csrf: session.csrf, reason: 'false positive in the fixture' }) }) + assert.equal(accepted.statusCode, 302) + assert.match(flashedText(accepted), /dismissed/i) + const row = h.store.getSecurityFinding(h.repo.id, ID) + assert.equal(row?.state, 'dismissed') + assert.equal(row?.reason, 'false positive in the fixture') + assert.ok(h.store.listAudit(20).some((entry) => entry.action === 'security_finding.dismissed')) + } finally { + await built.close() + } +}) + +test('the fix route refuses a fix without evidence', async () => { + const h = harness() + const built = await buildApp({ config: h.config, store: h.store, mailer: new MemoryMailer(), runner: new MockRunner(), startScheduler: false }) + try { + await report(h, 'job-a', [{ rule: RULE, path: PATH, line: LINE }]) + const session = sessionFor(built, h.store.userById(h.userId)!) + const url = `/repos/${h.repo.id}/security-findings/${encodeURIComponent(ID)}/fix` + const headers = { cookie: session.cookie, 'content-type': 'application/x-www-form-urlencoded', 'accept-language': 'en' } + const refused = await built.app.inject({ method: 'POST', url, headers, payload: formBody({ csrf: session.csrf, evidence: '' }) }) + assert.equal(refused.statusCode, 302) + assert.match(flashedText(refused), /evidence/i) + assert.equal(h.store.getSecurityFinding(h.repo.id, ID)?.state, 'open', 'a fix without evidence leaves the finding open') + const accepted = await built.app.inject({ method: 'POST', url, headers, payload: formBody({ csrf: session.csrf, evidence: 'removed the sink in 4a1b2c3' }) }) + assert.equal(accepted.statusCode, 302) + assert.equal(h.store.getSecurityFinding(h.repo.id, ID)?.state, 'fixed') + } finally { + await built.close() + } +}) + +test('the repository page lists the four states, and a fixed row with no evidence is not called verified', async () => { + const h = harness() + try { + await report(h, 'job-a', [{ rule: RULE, path: PATH, line: LINE }]) + await report(h, 'job-a2', [{ rule: 'js/x', path: 'src/a.ts', line: 1 }]) + assert.equal(h.store.setSecurityFindingState(h.repo.id, 'js/x src/a.ts:1', 'dismissed', 'not reachable from user input'), true) + await report(h, 'job-b', [{ rule: 'js/x', path: 'src/b.ts', line: 2 }]) + assert.equal(h.store.setSecurityFindingState(h.repo.id, 'js/x src/b.ts:2', 'fixed', 'removed in 4a1b2c3'), true) + await report(h, 'job-c', [{ rule: 'js/x', path: 'src/b.ts', line: 2 }]) + // A row whose evidence is gone cannot be written through the store, so the + // view is handed one directly: it must not call it verified fixed. + const evidenceGone: SecurityFindingRow = { + repo_id: h.repo.id, id: 'js/x src/c.ts:3', state: 'fixed', first_seen_job: 'job-a', + last_seen_job: 'job-a', reason: null, dismissed_in_job: null, updated_at: new Date().toISOString(), + } + const html = repoPage({ + config: h.config, + locale: 'en', + viewer: viewer(h), + repo: h.repo, + jobs: [], + securityFindings: [...h.store.listSecurityFindings(h.repo.id), evidenceGone], + }) + assert.match(html, /Security findings/) + assert.match(html, /Open/) + assert.match(html, /Fixed/) + assert.match(html, /Regressed/) + assert.match(html, /Dismissed/) + assert.match(html, /no evidence recorded/, 'a fixed row without evidence is shown as unverified') + assert.match(html, /reported by jobs, not GitHub/, 'the card says these are reported ids, not the alert store') + } finally { + h.store.close() + } +})