BUG: SPF flattening removes redirect= modifiers that an "all" mechanism ignores by shuvamk · Pull Request #4634 · DNSControl/dnscontrol

shuvamk · 2026-08-01T06:39:24Z

Follow-up to #4630, which you asked for: "Yes, please submit a separate PR that handles redirect. The default should be to remove it."

The problem

Flattening an include splices the child's terms into the parent. If the child's last term is a redirect= whose target is not itself being flattened, that term is carried over verbatim. spfcache.json:

{
  "child.example.net":  { "SPF": "v=spf1 ip4:1.2.3.4 redirect=other.example.org" },
  "qchild.example.net": { "SPF": "v=spf1 ip4:1.2.3.4 ~redirect=other.example.org" },
  "other.example.org":  { "SPF": "v=spf1 ip4:9.9.9.9 -all" }
}

dnscontrol print-ir, five SPF_BUILDER cases, run on main @ fdc38db6 and on this branch:

`SPF_BUILDER`	on `main` today	this PR
`parts: [v=spf1, include:child.example.net, -all]` `flatten: [child.example.net]`	`v=spf1 ip4:1.2.3.4 redirect=other.example.org -all`	`v=spf1 ip4:1.2.3.4 -all`
same, plus `keepIgnoredRedirects: true`	`v=spf1 ip4:1.2.3.4 redirect=other.example.org -all`	`v=spf1 ip4:1.2.3.4 redirect=other.example.org -all`
child uses `~redirect=`	`v=spf1 ip4:1.2.3.4 ~redirect=other.example.org -all`	`v=spf1 ip4:1.2.3.4 -all`
no `all` in the parent	`v=spf1 ip4:1.2.3.4 redirect=other.example.org`	`v=spf1 ip4:1.2.3.4 redirect=other.example.org`
`parts: [v=spf1, ip4:1.2.3.4, -ALL, redirect=other.example.org]` `flatten: [nomatch.example]`	`v=spf1 ip4:1.2.3.4 -ALL redirect=other.example.org`	`v=spf1 ip4:1.2.3.4 -ALL`

Two things are wrong with rows 1 and 3. RFC 7208 §6.1: "Any 'redirect' modifier MUST be ignored if there is an 'all' mechanism anywhere in the record." So the modifier is inert, and it is published to live DNS as bytes in a record that flattening exists to shorten. And DNSControl cannot read back what it just wrote — spflib.Parse rejects a redirect= in any non-final position (pkg/spflib/parse.go:78):

redirect=other.example.org must be last item

The cause

(*SPFRecord).Flatten strips a trailing all from the flattened child and appends everything else unchanged. Nothing checks whether a spliced-in redirect= has landed in a record that has an all. Parse breaks at a lowercase all and rejects redirect= in any non-final position, so it will not normally emit this combination — the exception is an uppercase -ALL, which its case-sensitive check at parse.go:61 walks straight past (residual 2 below).

The fix

Flatten drops redirect= modifiers from a record that contains an all mechanism.

The check runs at every level of the recursion, not only at the top: a redirect= that is already dead inside a nested include must not come back to life when the enclosing record has no all of its own. Pinned by a test.

Parse strips a term's qualifier before deciding what the term is (parse.go:56-58), so it also accepts +redirect=, ~redirect=, -redirect= and ?redirect= — row 3 above. The removal looks past the qualifier the same way isAllMechanism already did; both now share a two-line trimQualifier. Case is not folded, deliberately: Parse matches redirect= case-sensitively and rejects REDIRECT= outright, so folding here would diverge from what the parser accepts.

A redirect= in a record with no all is live and is left alone (row 4).

The opt-out

You also said there should be a way to disable this. Flatten now takes variadic FlattenOption values, so the existing callers in pkg/normalize/flatten.go and docs/flattener/js.go compile unchanged. spflib.KeepIgnoredRedirects() turns the removal off.

It reaches users as a new SPF_BUILDER parameter, keepIgnoredRedirects, default false — i.e. remove, as you asked. It follows the same metadata path as flatten/split/overhead1/txtMaxSize:

SPF_BUILDER({
  parts: ["v=spf1", "include:child.example.net", "-all"],
  flatten: ["child.example.net"],
  keepIgnoredRedirects: true,   // keep the RFC-ignored redirect=
})

On the shape of that option, and an alternative I owe you. The functional-options pattern is introduced by this PR and is used nowhere else in the repo; the idiom in this very file is plain parameters (TXTSplit(pattern string, overhead int, txtMaxSize int)). I used it because it keeps Flatten's signature source-compatible for the //go:build js flattener in docs/flattener/js.go, which I would otherwise have to touch in the same PR. If you would rather have something lighter, the two defensible alternatives are a second exported method (FlattenKeepingIgnoredRedirects, or similar) or simply Flatten(spec string, keepIgnoredRedirects bool) with both call sites updated — that is four lines of machinery instead of fifteen. Say the word and I will switch; I have no attachment to the options pattern.

Two residuals, stated plainly.

Flattening a child whose record ends in a redirect= that you did not also list in flatten already discards that redirect target's authorizations — on main today and after this PR. A conformant receiver ignores the modifier in both cases, so the evaluated result is unchanged; but a non-conformant receiver that honours it would go Pass → Fail for hosts authorised only via the redirect target. keepIgnoredRedirects: true preserves today's exact bytes for anyone who is worried about that.
The removal runs on every Flatten call, not only when flattening relocated a redirect. Row 5 of the table is the case: nothing was flattened at all (flatten: ["nomatch.example"]), and the already-ignored redirect= was still dropped. That is correct per §6.1 — the modifier is ignored because an all is present, regardless of how it got there — but it does mean the change is not confined to records that flattening rewrote.

Tests

TestFlattenIgnoredRedirect (7 cases) and TestFlattenOutputParses (5 qualifier variants) in pkg/spflib/flatten_test.go. Both use the in-test fakeResolver, so no live DNS.

I verified they fail without the fix, in two stages.

Stage 1 — the qualifier handling alone removed, everything else in place:

--- FAIL: TestFlattenIgnoredRedirect/qualified_redirect_is_dropped_when_an_all_mechanism_ignores_it
        got v=spf1 ip4:1.2.3.4 ~redirect=other.example.org -all want v=spf1 ip4:1.2.3.4 -all
--- FAIL: TestFlattenOutputParses/+redirect
        Parse("v=spf1 ip4:1.2.3.4 +redirect=other.example.org -all") returned redirect=other.example.org must be last item
--- FAIL: TestFlattenOutputParses/~redirect
--- FAIL: TestFlattenOutputParses/-redirect
--- FAIL: TestFlattenOutputParses/?redirect

Stage 2 — the whole removal disabled:

--- FAIL: TestFlattenIgnoredRedirect/redirect_of_a_flattened_include_is_dropped_when_an_all_mechanism_ignores_it
--- FAIL: TestFlattenIgnoredRedirect/qualified_redirect_is_dropped_when_an_all_mechanism_ignores_it
--- FAIL: TestFlattenIgnoredRedirect/redirect_ignored_inside_a_nested_include_does_not_become_live_in_the_parent
--- FAIL: TestFlattenOutputParses/{,+,~,-,?}redirect        (all 5)
    --- PASS: .../redirect_is_kept_when_no_all_mechanism_ignores_it
    --- PASS: .../qualified_redirect_is_kept_when_no_all_mechanism_ignores_it
    --- PASS: .../KeepIgnoredRedirects_retains_a_redirect_an_all_mechanism_ignores
    --- PASS: .../redirect_that_matches_the_flatten_spec_is_still_inlined

Restored, all pass. The four cases that pass in both states are deliberate pins, not passengers — they hold the live redirect, the qualified live redirect, the opt-out, and the in-spec redirect against regression. TestFlattenTrailingAll from #4630 also passes unchanged in every state, including its -ALL case.

Local verification

CI workflows sit at action_required for outside contributors, so here is what I ran locally against fdc38db6:

command	`main`	this branch
`go test -count=1 ./...`	47 ok / 59 no-test-files / 0 FAIL	47 ok / 59 no-test-files / 0 FAIL
`golangci-lint run`	0 issues	0 issues
`staticcheck ./...`	0 issues	0 issues
the six `go-checks` commands + `git diff`	—	0 extra files changed
`prettier@3.9.5 --check pkg/js/helpers.js`	clean	clean
`BIND_DOMAIN=example.com go test ./integrationTest/ -args -provider BIND`	—	ok, 0.6s
`GOOS=js GOARCH=wasm go build ./docs/flattener/`	ok	ok

go generate ./... regenerated 3 lines in commands/types/dnscontrol.d.ts; they are committed. (GOOS=js GOARCH=wasm go build ./... fails on main too, inside codeberg.org/miekg/dns and AlecAivazis/survey — pre-existing and unrelated, which is why the row above is scoped to docs/flattener, the package that actually holds the js-tagged caller.)

Diff is 6 files, +178/−10, of which 115 lines are tests and 3 are generated.

…sm ignores Flattening an include splices the child's terms into the parent. When the child's last term is a redirect= that is not itself flattened, that term is carried over verbatim, so a parent that ends in an "all" mechanism produces: v=spf1 ip4:1.2.3.4 redirect=other.example.org -all RFC 7208 Section 6.1: "Any 'redirect' modifier MUST be ignored if there is an 'all' mechanism anywhere in the record." The modifier above is therefore inert, it is published to live DNS as noise, and spflib.Parse rejects it on the way back in with "redirect=other.example.org must be last item", so DNSControl cannot re-read its own output. Flatten now drops redirect= modifiers from a record that has an "all" mechanism. The check runs at every level of the recursion, not only at the top: a redirect that is already dead inside a nested include must not come back to life when the enclosing record has no "all" of its own. Parse strips a term's qualifier before deciding what the term is, so it accepts +redirect=, ~redirect=, -redirect= and ?redirect= as well. The removal has to look past the qualifier the same way, which isAllMechanism already did; both now share trimQualifier. A redirect= in a record with no "all" is live and is left alone. Flatten takes variadic FlattenOption values so the existing callers in pkg/normalize and docs/flattener keep compiling. spflib.KeepIgnoredRedirects() turns the removal off, reachable from the DSL as the SPF_BUILDER parameter keepIgnoredRedirects (default false), following the metadata pattern already used by flatten/split/overhead1/txtMaxSize. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

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 (#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 #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. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Tom Limoncelli <tal@whatexit.org>

Follow-up to #4634. While going through the recently merged pull requests, I noticed that two links on https://docs.dnscontrol.org/language-reference/domain-modifiers/spf_builder don't work. The `txtMaxSize` description shows "[multiple strings][multi-string]" and "[not go higher than 450][record-size]" as plain text, because both reference definitions are on the same line in `SPF_BUILDER.md`, so neither is recognized as a definition. That line dates back to #1809. This GitHub pull request puts each definition on its own line. Reference links with one definition per line render fine on GitBook, as on the "Writing new DNS providers" page. ## AI-attributie Assisted-by: Claude Code:claude-opus-5

shuvamk requested review from TomOnTime and cafferata as code owners August 1, 2026 06:39

shuvamk mentioned this pull request Aug 1, 2026

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

Merged

Merge branch 'main' into fix/spf-strip-ignored-redirect

256fb36

TomOnTime approved these changes Aug 27, 2026

View reviewed changes

Merge branch 'main' into fix/spf-strip-ignored-redirect

40eae59

TomOnTime approved these changes Sep 2, 2026

View reviewed changes

Merge branch 'main' into fix/spf-strip-ignored-redirect

9358ac6

TomOnTime merged commit 34db875 into DNSControl:main Sep 2, 2026
3 checks passed

cafferata mentioned this pull request Sep 19, 2026

docs: fix the reference links in SPF_BUILDER #4914

Merged

Navigation Menu

Repository navigation

BUG: SPF flattening removes redirect= modifiers that an "all" mechanism ignores - #4634

BUG: SPF flattening removes redirect= modifiers that an "all" mechanism ignores#4634
TomOnTime merged 4 commits into
DNSControl:mainfrom
shuvamk:fix/spf-strip-ignored-redirect

shuvamk commented Aug 1, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Conversation

shuvamk commented Aug 1, 2026

The problem

The cause

The fix

The opt-out

Tests

Local verification

Related

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants