feat(strix): documented accepted-risk suppression (<= MEDIUM, never HIGH/CRITICAL) by seonghobae · Pull Request #609 · ContextualWisdomLab/.github

seonghobae · 2026-07-21T07:04:41Z

목적

스캔 대상 repo가 설계상 수용된(by-design) 특정 finding을 사유와 함께 문서화해 필수 게이트가 그것 때문에 hard-fail 하지 않게 하되, 진짜 이슈에 대한 강제력은 그대로 유지하는 accepted-risk 억제 기구.

⚠️ 이 변경은 게이트의 pass/fail 코어 판정을 건드립니다 — 세밀한 리뷰를 부탁드립니다. 안전 설계는 아래처럼 보수적으로 잡았습니다.

안전 설계

HIGH·CRITICAL은 절대 억제 불가. 상한 STRIX_MAX_SUPPRESSIBLE_SEVERITY(기본 MEDIUM). VULN-0008이 MEDIUM.
억제 조건 4개 모두 충족 시에만: ① finding severity ≤ 상한 ② 선언 항목의 severity도 상한 이내이고 finding severity 이상 ③ finding 제목이 선언 substring 포함 ④ 사유 비어있지 않음(무사유·오형식 항목은 fail-closed).
모든 억제는 ::warning(제목+사유)로 로깅 → 필수 체크 출력에서 감사 가능.
선언 파일 .security/strix-accepted-risks.txt는 PR diff에 포함 → 추가·완화가 리뷰 대상. PR head(신뢰)에서 읽고 체크아웃 폴백.

구현

finding 차단 3경로 모두에 skip 배선(evaluate_pull_request_findings, has_blocking_vulnerability_reports, has_unmapped_threshold_report) — 한 경로만 고치면 다른 경로가 재차단하므로 일관 처리.
evaluate_pull_request_findings의 콘솔-로그 폴백은 이제 per-finding 리포트가 하나도 없을 때만 severity 재도출 → 억제된 per-finding이 집계 로그에서 되살아나지 않음.

테스트

정적 어서션(상한·매처 정의·HIGH/CRITICAL 불가 문구·3경로 배선 수≥3·폴백 가드).
기능 매처 테스트: 문서화된 MEDIUM은 억제, 제목이 같은 HIGH는 차단 유지, 미선언 MEDIUM은 차단 유지. 로컬 FAILURES=0.

동기 사례

ContextualWisdomLab/gyeot#11 VULN-0008(온디바이스 SQLite at-rest 평문) — README가 "서버 미전송"만 약속하고 기기 내 암호화는 미약속인, 문서화된 on-device-first 설계 트레이드오프.

🤖 Generated with Claude Code

Copilot

Pull request overview

Adds an “accepted risk” suppression mechanism to the Strix quick gate so repositories can document specific by-design findings (capped at <= MEDIUM) and avoid hard-failing required checks, while keeping HIGH/CRITICAL findings unsuppressible and auditable.

Changes:

Introduces STRIX_MAX_SUPPRESSIBLE_SEVERITY and .security/strix-accepted-risks.txt parsing + matching to suppress documented findings (with reason) and emit an audit ::warning.
Wires suppression into all finding-blocking verdict paths and tightens the console-log fallback so aggregate logs don’t resurrect already-evaluated per-finding reports.
Extends the Strix gate self-test harness with assertions for wiring and functional matching behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
scripts/ci/strix_quick_gate.sh	Implements accepted-risk declaration reading, matching, logging, and integrates it into the gate’s verdict paths and fallback behavior.
scripts/ci/test_strix_quick_gate.sh	Adds contract + functional tests ensuring suppression wiring and expected MEDIUM/HIGH behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Addresses #609 review (Copilot): - Glob injection: the declared substring was used in a [[ == *"$s"* ]] pattern, so glob metacharacters (*, ?, [) in .security/strix-accepted-risks .txt could broaden the match. Switch to a fixed-string grep -F so a declared substring matches literally and can never over-match (a lone '*' now matches only titles containing an asterisk, not all findings). - Workflow-command injection: the ::warning message embedded the finding title and repo-supplied reason unescaped. Add escape_workflow_command_ message (%, CR, LF -> %25/%0D/%0A) so untrusted content cannot break the command format or inject additional workflow commands into the Actions log. - Reword the misleading "trusted" comment: PR-head blobs are immutable for the run but remain untrusted PR-controlled data, consistent with how the rest of this script treats them. Functional test extended with a bare-'*' declaration that must not match a non-asterisk title, locking in the literal-match fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

scripts/ci/strix_quick_gate.sh:2046

The ::warning message escapes the (untrusted) finding title and accepted-risk reason, but it interpolates STRIX_ACCEPTED_RISKS_PATH without escaping. If this value ever contains %, CR, or LF (e.g. via environment/config), it could break the workflow command format or inject additional commands. Escaping it keeps the workflow-command hardening consistent.

			local safe_title safe_reason
			safe_title="$(escape_workflow_command_message "$finding_title")"
			safe_reason="$(escape_workflow_command_message "$entry_reason")"
			echo "::warning title=Strix accepted risk::Suppressing documented accepted-risk finding (${entry_severity}) '${safe_title}' — reason: ${safe_reason}. HIGH/CRITICAL findings are never suppressible; see ${STRIX_ACCEPTED_RISKS_PATH}." >&2
			return 0

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

…s auth The OpenCode PR-review model pool authenticates to GitHub Models with `STRIX_GITHUB_MODELS_TOKEN: ${{ secrets.STRIX_GITHUB_MODELS_TOKEN || github.token }}`. When that org secret is set but the PAT lacks the `models` permission, the `||` fallback never fires (a non-empty secret is truthy), so every `github-models/*` candidate fails. Observed on #608 and #609 (rerun 2026-07-22 03:09Z, after the secret was rotated at 01:00Z): all six github-models candidates returned `provider failure metadata: class=authentication-or-permission`, producing MODEL_OUTPUT_UNAVAILABLE and blocking every open PR org-wide behind the required `opencode-review` check. This job already grants `models: read` to GITHUB_TOKEN (permissions block), so `github.token` can call GitHub Models directly. Bind the env var to `github.token` in both the model-pool step and the failed-check diagnosis step, removing the fragile mis-scoped-PAT dependency. GitHub Models rate limits still apply; the lead verdict continues to come from the direct-OpenAI backend (OPENAI_API_KEY), tracked separately. Follow-ups (same textual pattern, different context — not in this PR): - strix.yml:428 uses it inside a "Gate Strix secrets" step; changing the token source there affects gating semantics and needs its own review. - pr-review-autofix.yml:313,449 use it in a job whose permissions lack `models: read`, so that fix must also add the permission. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Addresses #609 review (Copilot): - Glob injection: the declared substring was used in a [[ == *"$s"* ]] pattern, so glob metacharacters (*, ?, [) in .security/strix-accepted-risks .txt could broaden the match. Switch to a fixed-string grep -F so a declared substring matches literally and can never over-match (a lone '*' now matches only titles containing an asterisk, not all findings). - Workflow-command injection: the ::warning message embedded the finding title and repo-supplied reason unescaped. Add escape_workflow_command_ message (%, CR, LF -> %25/%0D/%0A) so untrusted content cannot break the command format or inject additional workflow commands into the Actions log. - Reword the misleading "trusted" comment: PR-head blobs are immutable for the run but remain untrusted PR-controlled data, consistent with how the rest of this script treats them. Functional test extended with a bare-'*' declaration that must not match a non-asterisk title, locking in the literal-match fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

+			local safe_title safe_reason
+			safe_title="$(escape_workflow_command_message "$finding_title")"
+			safe_reason="$(escape_workflow_command_message "$entry_reason")"
+			echo "::warning title=Strix accepted risk::Suppressing documented accepted-risk finding (${entry_severity}) '${safe_title}' — reason: ${safe_reason}. HIGH/CRITICAL findings are never suppressible; see ${STRIX_ACCEPTED_RISKS_PATH}." >&2


…IGH/CRITICAL) Adds a per-finding accepted-risk mechanism so a scanned repository can document a specific, reasoned by-design finding and stop it from hard-failing the required gate — without weakening enforcement for genuine issues. Declaration lives in the scanned repo at .security/strix-accepted-risks.txt, one entry per line: `SEVERITY | title-substring | reason`. A finding is suppressed only when ALL hold: - its severity is at or below STRIX_MAX_SUPPRESSIBLE_SEVERITY (default MEDIUM) — HIGH and CRITICAL are never suppressible; - the entry's own declared severity is within that ceiling and >= the finding severity; - the finding title contains the entry substring; and - the entry carries a non-empty reason. Every suppression is emitted as a ::warning (title + reason) so it is auditable in the required check output, and the declaration is part of the PR diff so adding/loosening it is reviewable. The skip is wired into all three finding-blocking verdict paths (evaluate_pull_request_findings, has_blocking_vulnerability_reports, has_unmapped_threshold_report), and the console-log fallback in evaluate_pull_request_findings now only re-derives severity when NO per-finding reports exist — so accepted per-finding reports are not resurrected from the aggregate log. The declaration is read from the PR head (trusted) with a checkout fallback. Tests: static assertions for the caps/wiring/fallback guard, plus a functional matcher test proving a documented MEDIUM is suppressed while a title-matching HIGH and an undeclared MEDIUM both still block. Motivating case: ContextualWisdomLab/gyeot#11 VULN-0008 (on-device SQLite at-rest plaintext), a documented on-device-first design tradeoff. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Addresses #609 review (Copilot): - Glob injection: the declared substring was used in a [[ == *"$s"* ]] pattern, so glob metacharacters (*, ?, [) in .security/strix-accepted-risks .txt could broaden the match. Switch to a fixed-string grep -F so a declared substring matches literally and can never over-match (a lone '*' now matches only titles containing an asterisk, not all findings). - Workflow-command injection: the ::warning message embedded the finding title and repo-supplied reason unescaped. Add escape_workflow_command_ message (%, CR, LF -> %25/%0D/%0A) so untrusted content cannot break the command format or inject additional workflow commands into the Actions log. - Reword the misleading "trusted" comment: PR-head blobs are immutable for the run but remain untrusted PR-controlled data, consistent with how the rest of this script treats them. Functional test extended with a bare-'*' declaration that must not match a non-asterisk title, locking in the literal-match fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

seonghobae · 2026-08-03T03:15:58Z

Closing this implementation because a PR-head accepted-risk file is the wrong trust boundary for a required security gate. Even with a MEDIUM ceiling, the same change under review could add or broaden its own suppression declaration, making the security verdict depend on attacker-controlled policy. A commercial-grade replacement should bind accepted risks to a protected-base or centrally signed policy object with stable finding identifiers, owner/expiry metadata, independent approval, and an auditable exception lifecycle. Until that design exists, Strix remains fail-closed.

Copilot AI review requested due to automatic review settings July 21, 2026 07:04

Copilot started reviewing on behalf of seonghobae July 21, 2026 07:05 View session