fix(ci) bypass errexit on WORK_EMAIL_PATTERN validation in sanitization#102
Merged
Conversation
The Sanitization workflow has been silently skipping every scan rule
after the Personal domain rule on every PR run since the work-email
secret-validation guard was added. Confirmed by inspecting log output
across multiple runs: only the JungleTech, Personal email, and
Personal domain scan groups appear; the Work email, PEM, RSA, Slack,
GitHub PAT, AWS, and literal-credential-assignment scans never run.
Root cause. The shell runs under `bash -e -o pipefail` (GitHub
Actions default). The validation line:
printf '' | grep -E "$WORK_EMAIL_PATTERN" >/dev/null 2>&1
pattern_rc=$?
intends to capture grep's exit code so the malformed-pattern guard
can fire. But `grep -E "<any-valid-pattern>"` against empty stdin
returns rc=1 ("no match"). pipefail surfaces that as the pipeline's
exit code, and errexit aborts the script before `pattern_rc=$?`
runs. Result: the script exits 1 with no further scans.
The originally-malformed-pattern case (rc=2) hit the same trap and
also exited prematurely — the fact that PRs #99/#100 originally
showed "Forbidden token(s) found" hits from the recursive-trap rules
was incidental: those rules ran BEFORE the validation. Anything
after never executed.
Fix. Convert the validation to an ||-list, which errexit explicitly
skips per the bash manual:
pattern_rc=0
printf '' | grep -E "$WORK_EMAIL_PATTERN" >/dev/null 2>&1 || pattern_rc=$?
Default-zero pattern_rc means a successful grep (rc=0, theoretical
on empty input) leaves it at 0; any non-zero exit captures the real
code. The downstream `if [ "$pattern_rc" -eq 2 ]` malformed-pattern
guard works unchanged.
Why this matters. Every scan after the Personal-domain rule —
including critical credential-detection rules for AWS keys, GitHub
PATs, Slack tokens, RSA/PEM keys, and literal password/api-key
assignments — has been a no-op on every PR check. A real credential
leak past those rules would have been masked. Fixing the errexit
bypass restores all rules to working order.
After this merges:
- The Work email rule will run if the WORK_EMAIL_PATTERN secret is
set (it is, as of this session)
- The credential-detection rules will run for the first time since
the guard was added in 9dc3de5
- PRs #99 and #100 (currently blocked) should retrigger CI green
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The Sanitization workflow has been silently skipping every scan rule after the Personal domain rule on every PR run since the work-email secret-validation guard was added (commit
9dc3de5). Discovered while debugging blocked PRs #99 and #100 — after PR #101 fixed the recursive-trap docs issue, the Personal email and Personal domain scans started passing as expected, but the workflow still exited 1 with no further output. Inspection of multiple runs confirms only the JungleTech / Personal email / Personal domain scan groups appear in logs. The Work email, PEM, RSA, Slack, GitHub PAT, AWS, and literal-credential-assignment scans never run.Root cause. The shell runs under
bash -e -o pipefail(GitHub Actions default forshell: bash). The validation line at sanitization.yml:109–110:intends to capture grep's exit code so the malformed-pattern guard can fire. But
grep -E "<any-valid-pattern>"against empty stdin returns rc=1 ("no match"). pipefail surfaces that as the pipeline's exit code, and errexit aborts the script beforepattern_rc=$?runs. Result: the script exits 1 with no further scans.The originally-malformed-pattern case (rc=2) hit the same trap and also exited prematurely — the fact that PRs #99/#100 originally showed "Forbidden token(s) found" hits from the recursive-trap docs was incidental: those rules ran before the validation. Anything after never executed.
Fix. Convert the validation to an
||-list, which errexit explicitly skips per the bash manual:Default-zero
pattern_rcmeans a theoretical successful grep (rc=0 on empty input) leaves it at 0; any non-zero exit captures the real code. The downstreamif [ "$pattern_rc" -eq 2 ]malformed-pattern guard works unchanged.Test Plan
set -e -o pipefail; pattern_rc=0; printf '' | grep -E '@distilledtech\.com' >/dev/null 2>&1 || pattern_rc=$?; echo $pattern_rc→ prints1(no match against empty input — expected, no script abort)set -e -o pipefail; pattern_rc=0; printf '' | grep -E '[invalid' >/dev/null 2>&1 || pattern_rc=$?; echo $pattern_rc→ prints2(parse error, captured correctly)Security implications
This fix has higher stakes than a typical CI tweak. Every credential-detection rule has been a no-op on every PR check since the guard was added. A leak of an AWS access key, a GitHub PAT, a Slack token, or a password literal in committed code would not have been blocked by this workflow during that window.
Mitigations already in place that limited blast radius:
feedback_personal_identity_only.md) means commits weren't accidentally tagged with work-email author identityJungleTech / Personal email / Personal domainscans did run, so prior-client-name and personal-email exposures were still blockedBut the tens of additional rules — covering most actual secret leakage scenarios — were silent. After this merges, those rules become live for the first time. Possible (small) follow-up: run a sweep of recent merged PRs against the now-active rule set to confirm no historical leaks slipped through. Tracked as Phase 4.x follow-up; not blocking this PR.
Out of Scope
WORK_EMAIL_PATTERN). Tracked as a Phase 4.x follow-up; doesn't block this PR.Checklist
~/.claude/projects/c--projects-PinballWizard/memory/is now stale, it has been updated or removed — N/ATODO/FIXME/ commented-out code committed<NoWarn>without a comment — N/APre-push self-audit
Step 0 —
/local-review(qualitative).claude/skills/local-review/SKILL.md§ "When to invoke": doc/CI-only changes may skip.Step 1 — Mechanical checklist
*Optionsproperty has at least one real getter call insrc/— N/A9dc3de5line; the original intent is preserved, the runtime behavior is correctedcatch { }— N/AISourceScraper? — N/Agit log -1 --format='%an <%ae>'shows personal noreply, not work email — confirmed94459922+jkeeley2073@users.noreply.github.com