Reject unparseable regexp patterns instead of panicking

[korya]

Problem

A typo in an assertion pattern crashed the tool with a Go stack trace.

$ http-assert --assert-body '[unclosed' https://api.example.com
panic: regexp: Compile(`[unclosed`): error parsing regexp: missing closing ]: `[unclosed`
goroutine 1 [running]: ...
[exit=2]

Three flags carry user-supplied regexps — --assert-body, --assert-header, --assert-redirect — and all three compiled them with regexp.MustCompile. Assertion patterns need heavy shell escaping, so typos are routine; in CI this surfaced as a raw crash with an exit code that appears nowhere in the documented contract.

The RCA found these were the only user input in the program with no validation path. --log-level, --maphost, the environment variables, the method and the URL each already report bad values through die(). These three had nowhere to report to, because the constructors returned an Assertion and nothing else.

Present since the first commit (0faf21c, 2021-12-04); regexp.Compile had never appeared in this repo. A design gap, not a regression.

Solution

Give the three constructors error returns, then make the whole class of crash unexpressible.

Before / after:

$ http-assert --assert-body '[unclosed' https://api.example.com
Error: Invalid value for --assert-body flag: error parsing regexp: missing closing ]: `[unclosed`
$ echo $?
71

Exit 71 matches --log-level and --maphost, not 103 — that is the usage code, and this is a bad value, not bad usage. (A PreRunE error would have produced 103; that is why the fix uses die(71) directly.)

Fixing one flag proves nothing about the other eighteen, so three guardrails follow, at three different layers:

Layer	Guardrail	Effect
Static	forbidigo rejects regexp.MustCompile	the bug cannot be written
Parser	5 fuzz targets	invariants proven, not read
CLI	540 hostile invocations	no flag can crash the binary

The panic probe reads its own flag list

TestE2ENoFlagPanics parses --help at run time rather than hard-coding 19 flags, so a flag added later is probed without anyone remembering. It asserts it found at least 19, so a --help format change fails loudly instead of quietly probing nothing.

Twenty inputs per flag, chosen to break parsers rather than be rejected politely: unterminated classes and groups, trailing escapes, inverted repetition bounds, format specifiers, flag-lookalikes, control whitespace, multi-byte text, a 10,000-character string, 500 unbalanced brackets. Driven through every flag, the six environment variables, and the positional URL.

Only a crash fails it — any exit code the CLI chooses deliberately is correct behaviour.

Fuzzing found a second panic

Written to prove the parsers were safe, the targets failed on their sixth seed:

printPayload(w, []byte("x"), -1)  ->  panic: runtime error: slice bounds out of range [:-1]

Not reachable today — the sole caller passes the constant 256 — so it is latent, and it survived a static review that correctly identified MustCompile as the only reachable panic and wrongly concluded it was the only panic. Now guarded; the function is total.

Each target then ran 20s (~5.2M executions, ~26M total) with no further findings.

Verified in both directions

Neither guardrail is taken on trust:

• Injecting a panic into parseHeaderLine makes the probe report --assert-header = "[unclosed" crashed the process (exit 2).
• Reverting assertions.go to MustCompile makes just pre-commit fail.

An earlier injection into AssertBodyEmpty was not caught — that is how the recorded limitation was found: pflag rejects a non-boolean before it reaches this program, so for boolean flags the probe mostly exercises the flag layer. Documented in the test rather than papered over.

Coverage stays at 100.0%. No visual change — headless CLI.

Other Changes

• .golangci.yml introduced (the repo had none). It adds only forbidigo; the standard set stays enabled, verified by making an unused symbol and the new rule fire together.
• The default linter set is now asserted, not just documented. .golangci.yml adds to the standard set rather than replacing it — confirmed against the docs (linters.default defaults to standard; enable is additive; exclusions.presets defaults to []) and by diffing the enabled linters with and without the config, where the only difference is +forbidigo. But linters.default: none would silently drop all five standard linters and look like success, since fewer linters means fewer findings. A lint-config-check recipe now fails if any expected linter is missing. Its two failure modes report differently on purpose: an unreadable output blames the parser, a missing linter names the linter.
• The rule covers test files too. The one legitimate use — a literal pattern in the probe's --help parser — was rewritten to compile-and-report rather than carrying a suppression, since an exclusion would apply to every future test.
• TestKnownIssue17RegexPanic asserted the panic and now asserts the rejection. That flip is the record of the fix.

Not fixed: there is still no uniform validation point, so a future flag with a fallible value depends on its author noticing. forbidigo narrows that to "not via MustCompile"; closing it properly needs the error-flow restructuring in #55.

Related:

• Closes Panic: invalid regex in --assert-body/--assert-header/--assert-redirect crashes with a stack trace #17
• Extract run() int and push os.Exit to the edge #55

🤖 Generated with Claude Code