BUG: SPF parsing detects include loops instead of crashing with a stack overflow

[shuvamk]

Implements direction 1 (visited-set) from #4633, as you asked. Fixes #4633.

The problem

Parse() recursed into every include: and redirect= it resolved without keeping any record of the chain it was already resolving, so a cyclic chain never terminated. All five inputs below are hermetic (a fakeResolver map, no network); each is a test case in this PR. The "this PR" column shows the loop portion of the error — the full string also carries the existing in included SPF: wrapper, and it is that full string the tests assert.

resolver contents	Parse() on main @ fdc38db	this PR
a → v=spf1 include:a ~all	fatal error: stack overflow	SPF include loop: a.example.com -> a.example.com
a → include:b, b → include:a	fatal error: stack overflow	SPF include loop: a.example.com -> b.example.com -> a.example.com
a → v=spf1 redirect=a	fatal error: stack overflow	SPF include loop: a.example.com -> a.example.com
?include:a, a → v=spf1 +include:a ~all	fatal error: stack overflow	SPF include loop: a.example.com -> a.example.com
a → v=spf1 include:A.EXAMPLE.COM ~all, A.EXAMPLE.COM → include:a	fatal error: stack overflow	SPF include loop: a.example.com -> A.EXAMPLE.COM

End to end, using the reproducer from the issue (dnsconfig.js with TXT("@", "v=spf1 include:vendor.example ~all", {flatten: "all"}) and an spfcache.json entry pointing vendor.example at itself):

main @ fdc38db — exit 2, 508 lines of output, 85 spflib.Parse frames, 1.7 s wall:

runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0x7d6be5b84350 stack=[0x7d6be5b84000, 0x7d6c05b84000]
fatal error: stack overflow
...
github.com/DNSControl/dnscontrol/v4/pkg/spflib.Parse(...)
	pkg/spflib/parse.go:49 +0x80
github.com/DNSControl/dnscontrol/v4/pkg/spflib.Parse(...)
	pkg/spflib/parse.go:91 +0x4e8
[repeats]

this PR — exit 1, 4 lines:

2026/08/01 21:21:50 2 Validation errors:
2026/08/01 21:21:50 ERROR: in included SPF: SPF include loop: vendor.example -> vendor.example
2026/08/01 21:21:50 WARNING: problem resolving SPF record: lookup vendor.example on 192.168.1.1:53: no such host
exiting due to validation errors

The WARNING is not from this change: it is the cache's staleness check re-resolving vendor.example, which does not exist. The main binary prints the same line when the cache entry for that domain is made non-cyclic (v=spf1 ip4:192.0.2.0/24 ~all), so it is orthogonal — it simply never got the chance to print while the process was dying.

A Go stack overflow is a runtime.throw, not a panic, so recover() cannot catch it — nothing above Parse() could turn this into an error message. Executed against pristine main: wrapping the Parse call in defer func(){ recover() }() neither recovers nor reaches the statement after it; the test binary dies with fatal error: stack overflow having printed neither log line. And the chain being walked is a third party's data: an operator who marks a TXT record flatten: or split: has dnscontrol resolve whatever the vendor publishes at every preview/push.

Cause

pkg/spflib/parse.go:91 called Parse(subRecord, dnsres) with no depth counter and no visited set. The recursive call has been there since the package was added — 01a2424, 2017-05-25, "Initial DNS Resolvers and SPF scaffolding (#123)". (The issue body credits 823e8bb for this; that commit added the flattener, and git log -S 'IncludeRecord, err = Parse' puts the recursion in #123 four months earlier. My mistake there, corrected here.)

The fix

+17/−1 in pkg/spflib/parse.go. Parse keeps its exact signature and seeds an unexported parse(text, dnsres, chain []string); chain holds the domains currently being resolved, and parse refuses to descend into one already in its own ancestry.

Four properties, in the order they matter:

1. No exported surface changes. Parse(text string, dnsres Resolver) (*SPFRecord, error) is untouched, so pkg/normalize/flatten.go, docs/flattener/js.go and the existing tests need no edits.

2. The chain is scoped to the current path, not global. It is extended on descent and unwound on return. A domain legitimately reached twice through independent branches is not a loop, and that shape parses today:

   Parse("v=spf1 include:a.example.com include:b.example.com ~all")
     a.example.com      -> v=spf1 include:shared.example.com ~all
     b.example.com      -> v=spf1 include:shared.example.com ~all
     shared.example.com -> v=spf1 ip4:192.0.2.0/24 ~all
   => parts=3 lookups=4, no error   (on main AND on this branch)
   

   A single set shared across the whole walk would call that a loop and break a record that works now. TestParseSharedIncludeIsNotALoop pins it, and it passes both with and without the fix.

3. The key is the domain, not SPFPart.Text. Text keeps the qualifier (+include:, ?include:), so matching on it would let a qualified loop through. IncludeDomain is already qualifier-stripped. Covered by the ?include:/+include: case in the table above.

4. The chain comparison is case-insensitive, consistent with 819253a "fix(spf): Be case-insensitive when parsing SPF records (BUGFIX: Be case-insensitive when parsing SPF records #3982)". To be clear about what this does and does not buy: it is not needed to stop the recursion. With d == domain instead of strings.EqualFold, a case-alternating cycle is still caught — one hop later, with a redundant node in the chain (a.example.com -> A.EXAMPLE.COM -> a.example.com rather than a.example.com -> A.EXAMPLE.COM). The chain is built from the literal include:/redirect= operands, which are finite and pairwise distinct under exact matching, so depth is bounded either way. It is here because DNS names are case-insensitive and the resolver cache is keyed on the literal operand, so the exact-match form reports a loop node that is really the same domain twice. The last row of the table pins it: that subtest fails if the comparison is changed to ==. If you would rather have the two-line version that matches the design in spflib.Parse() has no depth or cycle limit: a cyclic include: chain crashes dnscontrol with an unrecoverable stack overflow #4633 exactly, say the word and I will drop it.

No record that parses today can be rejected by this. The guard fires only when a domain appears in its own ancestry, which is precisely the condition under which the old code could not return: cache.GetSPF memoizes each name in entry.resolvedSPF, so re-entering a domain replays an identical descent.

The error is wrapped by the existing in included SPF: %w at each level, so a deep loop reads in included SPF: in included SPF: SPF include loop: .... That repetition is how Parse already reports every nested error (verified: a nested unsupported SPF part produces the same shape on main) and is unchanged here.

This deliberately does not enforce the RFC 7208 §4.6.4 ten-lookup cap. That was direction 2 in the issue; it also bounds legitimate deep-but-finite chains, which is what flattening exists to fix. Cycles only.

Tests

pkg/spflib/parse_test.go, +81, reusing the existing fakeResolver from flatten_test.go — no network:

• TestParseIncludeLoop — 5 subtests: self-loop, two-node loop, redirect= loop, qualified ?include: loop, case-changing loop. Each asserts the complete error string, so the reported chain is pinned exactly, not just matched as a substring.
• TestParseSharedIncludeIsNotALoop — the diamond above, asserting it still parses to 3 parts and 4 lookups.

Verified they fail without the fix. With pkg/spflib/parse.go reverted to main and the tests left in place, each of the 5 loop subtests was run under its own -run filter (the first overflow kills the test binary, so anything after it silently never runs):

$ go test -count=1 ./pkg/spflib/ -run 'TestParseIncludeLoop/a_domain_that_includes_itself' -v
=== RUN   TestParseIncludeLoop
=== RUN   TestParseIncludeLoop/a_domain_that_includes_itself
runtime: goroutine stack exceeds 1000000000-byte limit
runtime: sp=0x206424c60390 stack=[0x206424c60000, 0x206444c60000]
fatal error: stack overflow
...
FAIL	github.com/DNSControl/dnscontrol/v4/pkg/spflib	1.515s
FAIL

All 5 behave identically: go test exits 1, 558 lines of output, 93 spflib.Parse frames. They do not report a test failure — they destroy the test binary, which is the point. TestParseSharedIncludeIsNotALoop passes without the fix, as it must. With the fix restored, all 6 pass.

The case-insensitivity in inChain is pinned separately, since a stack overflow cannot distinguish it: replacing strings.EqualFold(d, domain) with d == domain turns exactly one subtest red and leaves the other four green.

--- FAIL: TestParseIncludeLoop (0.00s)
    --- FAIL: TestParseIncludeLoop/a_loop_that_changes_the_case_of_the_domain (0.00s)
        parse_test.go:211: Parse("v=spf1 include:a.example.com ~all") error = "in included SPF: in included SPF: SPF include loop: a.example.com -> A.EXAMPLE.COM -> a.example.com", want "in included SPF: SPF include loop: a.example.com -> A.EXAMPLE.COM"
FAIL

Local verification

CI does not run for outside contributors until the workflow run is approved, so here is the full gate, run on this branch (go1.26.0, darwin/arm64), with main @ fdc38db as the baseline:

command	main	this branch
go test -count=1 ./...	47 ok / 59 no-test-files / 0 FAIL	identical
golangci-lint run ./...	0 issues	0 issues
staticcheck ./...	0 issues	0 issues
go vet ./pkg/spflib/	clean	clean
the six go-checks commands + git status	—	only the two files above are modified
BIND_DOMAIN=example.com go test ./integrationTest/ -args -provider BIND	—	ok, 0.57s
GOOS=js GOARCH=wasm go build ./docs/flattener/	—	exit 0

Branched from fdc38db6, which is where every number above was measured. main has since moved to 278b9632 (#4635, NAMECHEAP test skips); it touches nothing in pkg/spflib and git merge-tree --write-tree origin/main <branch> exits 0, so I have left the branch unrebased rather than add noise to the diff.

If you'd rather

The ten-lookup cap (direction 2) is still available and would compose with this — it bounds pathological non-cyclic nesting, which cycle detection does not. Happy to add it here or in a follow-up if you want it, and equally happy to change the error wording.

Found and written up with LLM assistance (Claude), same as #4630 and #4634.