Skip to content

Commit 35a013b

Browse files
Brian KrafftCopilot
andcommitted
security: address /8eyes audit findings
- Fix #1 (HIGH): Checkout base branch SHA, not PR branch, preventing self-modification of phase-config.yml or workflow files - Fix #2 (MEDIUM): Add actor allowlist to bypass — only configured users can invoke emergency:bypass - Fix #3 (LOW): Strip HTML comments before matching bypass reason pattern, preventing template placeholder from satisfying check - Bump min bypass reason length from 10 to 30 chars - Add CODEOWNERS requiring admin review for .github/** changes - Update bypass heading to use ⚠️ emoji prefix /8eyes verdicts: Security 0.92, Skeptic 0.90, Verifier 0.85 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 96d1476 commit 35a013b

5 files changed

Lines changed: 57 additions & 26 deletions

File tree

.github/CODEOWNERS

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# Phase Gate Enforcement — Protected Files
2+
# ==========================================
3+
# Changes to enforcement infrastructure require admin review.
4+
# This mitigates the PR-self-modification attack vector where
5+
# a PR modifies phase-config.yml or workflow files to weaken gates.
6+
7+
# All GitHub configuration (workflows, config, templates)
8+
.github/** @Deepfreezechill
9+
10+
# Enforcement documentation
11+
docs/enforcement/** @Deepfreezechill

.github/phase-config.yml

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -74,5 +74,8 @@ phases:
7474
escape_hatch:
7575
label: "emergency:bypass"
7676
require_reason: true
77-
reason_pattern: "## Bypass Reason"
78-
min_reason_length: 10
77+
reason_pattern: "## ⚠️ Bypass Reason"
78+
min_reason_length: 30
79+
# Only these GitHub usernames can invoke emergency bypass
80+
allowed_actors:
81+
- "Deepfreezechill"

.github/pull_request_template.md

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@ Closes #<!-- issue number -->
2424
- [ ] ADR written: `docs/adr/ADR-XXX.md`
2525

2626
<!--
27-
## Bypass Reason
28-
(Delete this section unless using emergency:bypass label)
29-
(Explain why this PR must skip phase gate enforcement)
27+
To use emergency bypass: uncomment the section below, fill in the reason,
28+
and add the `emergency:bypass` label. Reason must be 30+ characters.
29+
30+
## ⚠️ Bypass Reason
31+
(Explain why this PR must skip phase gate enforcement — minimum 30 characters)
3032
-->

.github/workflows/bypass-audit.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,10 @@ jobs:
3535
3636
// Extract bypass reason from PR body
3737
const body = pr.body || '';
38-
const reasonIdx = body.indexOf('## Bypass Reason');
38+
const reasonIdx = body.indexOf('## ⚠️ Bypass Reason');
3939
let reason = '(No reason provided)';
4040
if (reasonIdx !== -1) {
41-
reason = body.slice(reasonIdx + '## Bypass Reason'.length).trim().split('\n')[0];
41+
reason = body.slice(reasonIdx + '## ⚠️ Bypass Reason'.length).trim().split('\n')[0];
4242
}
4343
4444
// Create audit issue

.github/workflows/phase-enforce.yml

Lines changed: 34 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,10 @@ jobs:
3636
name: "Phase Gate Enforcement"
3737
runs-on: ubuntu-latest
3838
steps:
39-
- name: Checkout (for phase-config.yml)
39+
- name: Checkout BASE branch (not PR — prevents self-modification attack)
4040
uses: actions/checkout@v4
41+
with:
42+
ref: ${{ github.event.pull_request.base.sha }}
4143

4244
- name: Install js-yaml
4345
run: npm install js-yaml@4 --no-save --silent 2>/dev/null
@@ -165,26 +167,39 @@ jobs:
165167
const hasBypassLabel = prLabels.includes(bypassLabel);
166168
167169
if (hasBypassLabel) {
168-
const reasonPattern = escapeHatch.reason_pattern || '## Bypass Reason';
169-
const reasonIdx = (pr.body || '').indexOf(reasonPattern);
170-
if (reasonIdx === -1) {
171-
addCheck('Emergency bypass', false, `Label \`${bypassLabel}\` present but no "${reasonPattern}" section in PR body. Both keys required.`);
172-
// Don't short-circuit — fall through to fail
170+
// KEY 1: Actor must be in allowlist
171+
const allowedActors = escapeHatch.allowed_actors || [];
172+
const actor = context.actor;
173+
if (allowedActors.length > 0 && !allowedActors.includes(actor)) {
174+
addCheck('Emergency bypass', false, `Actor @${actor} is not authorized to invoke bypass. Allowed: ${allowedActors.join(', ')}`);
175+
// Fall through to normal enforcement (deny)
173176
} else {
174-
const reasonText = (pr.body || '').slice(reasonIdx + reasonPattern.length).trim().split('\n')[0];
175-
if (!reasonText || reasonText.length < 10) {
176-
addCheck('Emergency bypass', false, 'Bypass reason too short (min 10 chars). Explain WHY this bypass is necessary.');
177+
// KEY 2: Reason pattern must be present (not inside HTML comments)
178+
const reasonPattern = escapeHatch.reason_pattern || '## ⚠️ Bypass Reason';
179+
const minLength = escapeHatch.min_reason_length || 30;
180+
// Strip HTML comments before searching to prevent template placeholder match
181+
const bodyNoComments = (pr.body || '').replace(/<!--[\s\S]*?-->/g, '');
182+
const reasonIdx = bodyNoComments.indexOf(reasonPattern);
183+
if (reasonIdx === -1) {
184+
addCheck('Emergency bypass', false, `Label \`${bypassLabel}\` present but no "${reasonPattern}" section in PR body (outside HTML comments). Both keys required.`);
185+
// Don't short-circuit — fall through to fail
177186
} else {
178-
addCheck('Emergency bypass', true, `OVERRIDE ACTIVE: "${reasonText.slice(0, 100)}"`);
179-
180-
// Short-circuit: bypass granted
181-
audit.verdict = 'BYPASS';
182-
audit.bypass_reason = reasonText;
183-
184-
const auditComment = formatAuditComment(audit);
185-
await postAuditComment(auditComment);
186-
core.warning(`⚠️ EMERGENCY BYPASS: ${reasonText.slice(0, 100)}`);
187-
return; // Exit with success
187+
const reasonText = bodyNoComments.slice(reasonIdx + reasonPattern.length).trim().split('\n')[0];
188+
if (!reasonText || reasonText.length < minLength) {
189+
addCheck('Emergency bypass', false, `Bypass reason too short (${(reasonText || '').length}/${minLength} chars). Explain WHY this bypass is necessary.`);
190+
} else {
191+
addCheck('Emergency bypass', true, `OVERRIDE by @${actor}: "${reasonText.slice(0, 100)}"`);
192+
193+
// Short-circuit: bypass granted
194+
audit.verdict = 'BYPASS';
195+
audit.bypass_reason = reasonText;
196+
audit.bypass_actor = actor;
197+
198+
const auditComment = formatAuditComment(audit);
199+
await postAuditComment(auditComment);
200+
core.warning(`⚠️ EMERGENCY BYPASS by @${actor}: ${reasonText.slice(0, 100)}`);
201+
return; // Exit with success
202+
}
188203
}
189204
}
190205
}

0 commit comments

Comments
 (0)