Development record
rules.Merge panics with 'assignment to entry in nil map' (rules.go:193) when the base RuleSet's Domains field is nil and the overlay has at least one domain key. cloneRuleSet (rules.go:524) only allocates out.Domains when the source map is already non-nil, so a base like RuleSet{SchemaVersion: 1} (a Validate-accepted, valid zero-ish value) crashes on merge, contradicting Merge's own doc comment ('New domain keys are added') which promises no such precondition. The sibling loader guard.Merge (internal/core/guard/config.go:77-79) gets this right, explicitly allocating out.Entries when nil before the same kind of loop — Merge is simply missing that guard. Not reachable today: RuleSet.Load's only call site passes Merge(Defaults(), over), and Defaults() always has non-nil Domains, so this is a latent exported-API contract defect rather than a live crash — it becomes live the moment any caller merges onto a non-Defaults() base (e.g. a future multi-tier overlay starting from an empty RuleSet). Reproducing test (recover-guarded): internal/core/rules/, TestMergeNilBaseDomainsAddsNewKeys.
rules.Merge panics with 'assignment to entry in nil map' (rules.go:193) when the base RuleSet's Domains field is nil and the overlay has at least one domain key. cloneRuleSet (rules.go:524) only allocates out.Domains when the source map is already non-nil, so a base like RuleSet{SchemaVersion: 1} (a Validate-accepted, valid zero-ish value) crashes on merge, contradicting Merge's own doc comment ('New domain keys are added') which promises no such precondition. The sibling loader guard.Merge (internal/core/guard/config.go:77-79) gets this right, explicitly allocating out.Entries when nil before the same kind of loop — Merge is simply missing that guard. Not reachable today: RuleSet.Load's only call site passes Merge(Defaults(), over), and Defaults() always has non-nil Domains, so this is a latent exported-API contract defect rather than a live crash — it becomes live the moment any caller merges onto a non-Defaults() base (e.g. a future multi-tier overlay starting from an empty RuleSet). Reproducing test (recover-guarded): internal/core/rules/, TestMergeNilBaseDomainsAddsNewKeys.
Frontmatter
| id | iss-187 |
|---|---|
| type | issue |
| lifecycle | resolved |
| severity | minor |
.abcd/work/issues/resolved/iss-187-rules-merge-nil-domains-panic.md
open on GitHub ↗ · commit history ↗
mentions2
- dcn-26080500000000062026-08-05 — bug-hunt loop round 1: iss-184 (critical, guard tokenizer heredoc misparse) fixed at root cause, in two passes after a pre-PR adversarial security review caught the first pass narrowing the hole rather than closing it. `internal/core/guard/tokenize.go`'s `<<` handling already special-cased the literal-digit arithmetic-shift form (`$((1<<20))` is not a heredoc), but `isDelimStart` accepts any identifier-shaped word, so `$((1<<shift))` — an identifier operand — still read as a heredoc delimiter, and `skipHeredocBodies` then silently consumed every remaining line of the command looking for a "shift" terminator line that never comes, dropping later commands (a force-push, an `rm -rf`, anything) from ever reaching command position — a silent allow, not the loud fail-open the hook's own doc comment promises for anything the tokenizer cannot answer confidently. The first pass made `skipHeredocBodies` report whether it actually found each pending heredoc's terminator, turning a miss into `ErrUnparsableCommand`. The security review then constructed the exploit this missed: an attacker supplying a later line that happens to equal the misread "delimiter" (e.g. appending a bare `shift`) still finds a match, so the swallow still succeeds silently — the fix only closed the *unterminated* half of the class, not the *coincidentally-terminated* half, which is the one an attacker actually controls. The real fix is classification, not detection-of-failure: a heredoc delimiter word immediately followed by a bare `(` or `)` with no separator is never a real heredoc — its body and terminator line have to come first, so nothing legitimate places a paren directly against the delimiter word, and `$((expr<<ident))` produces exactly that shape. `<<` in that position is now read as the arithmetic operator at tokenize time, same as the literal-digit case, so the guarded line is reached and matched normally regardless of what any later line contains. The unterminated-body-to-error fix from the first pass is kept as defense in depth for genuinely malformed heredocs (`cat <<EOF` with no closing `EOF` line), a distinct, narrower gap the same swallow also covered — an existing test that pinned that silent-swallow as intended behaviour is updated to assert the corrected `ErrUnparsableCommand` contract. Detectors: `TestArithmeticShiftByIdentifierIsNotAHeredoc`, `TestArithmeticShiftCoincidentalDelimiterStillBlocks` (the adversarial payload the security review demonstrated), and `TestTokenizeRejectsUnterminatedHeredoc` — each watched failing on pre-fix code for the claimed reason and passing after. Three more bugs surfaced by the same round's multi-angle sweep and independently verified with failing tests are captured but not fixed here — iss-185 (critical, scanner leaves a second back-to-back secret token undetected and unredacted, defeating history's fail-closed residual re-scan), iss-186 (minor, a failed source-unlink in a capture ledger transition can permanently strand an issue id across two status directories), iss-187 (minor, `rules.Merge` panics on a nil-Domains base — currently unreachable through any live caller, so a latent exported-API contract defect) — left open for a future round, highest severity first.
- dcn-26080600000000032026-08-06 — iss-187 (bug-hunt loop, round 1): `rules.Merge`