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

[shuvamk]

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.

1. 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.
2. 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.

Related

Separately filed at your request: #4633, the spflib.Parse() recursion bug. Independent of this change; nothing here touches it.

---

AI-assisted, as with #4630 — the commit carries the Co-Authored-By: Claude Opus 5 trailer.