fix(txt): chunk long TXT records per RFC 1035 (#32)#1
Open
bindreams wants to merge 8 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes libdns#32 — long TXT records (>255 bytes, e.g. DKIM keys, long SPF, large ACME challenges) fail to round-trip cleanly through the provider.
Cloudflare stores TXT records >255 bytes as multiple RFC 1035 §3.3.14 character-strings, returned literally as
"chunk1" "chunk2". The oldunwrapContent/wrapContenthelpers only handled a single pair of outer quotes, so:GetRecordssurfaced zone-file artifacts (embedded" "between chunks) insidelibdns.TXT.Text.DeleteRecordssilently no-op'd because the match comparator never produced the chunked form on either side.This PR rewrites both helpers to chunk on write and parse multi-segment on read, then compares structurally (on unwrapped content) in
getDNSRecords. Adjacent hardening surfaced during review is included.Reproduction (from the issue)
After this PR:
rr.Dataround-trips byte-identical,DeleteRecordsreturns 1.Commits
test(txt): add failing tests for long-TXT round-trip (#32)— TDD red. Table-driven cases inclient_test.gothat fail against the old implementation, demonstrating both symptoms.fix(txt): chunk long TXT records per RFC 1035 (#32)— rewrites helpers, drops the long-contentcontent.containsURL filter for TXT, collapses the two-pass byte-equality match loop into a single structural comparison on unwrapped content, and tightens the error-handling path ingetDNSRecords(the old TXT branch silently swalloweddoAPIRequesterrors).test(txt): add libdnstest lifecycle test for long-TXT round-trip— env-gated integration test mirroring the issue reproduction against the real Cloudflare API.fix(txt): accept CR/LF as TXT chunk separators; harden tests (#32)— broadens the separator parser to accept all ASCII whitespace, adds tests for CR/LF/CRLF/empty-leading-chunk/invalid-escape, and randomizes the integration test record name so parallel CI runs against the same zone don't collide.refactor: paginate via shared helper; fix nil ResultInfo dereference— extracts pagination into alistDNSRecordshelper used by bothGetRecordsandgetDNSRecords. Two motivations: (a) droppingcontent.containsfor TXT meansgetDNSRecordscan now see >100 matching candidates for a single name (e.g. many parallel ACME challenges) and needs pagination; (b) the existingGetRecordshad a latent nil-pointer bug — it computedlastPage := response.ResultInfo.TotalCount / response.ResultInfo.PerPagebefore checkingresponse.ResultInfo == nil.Test plan
go vet ./...at repo root and inlibdnstest/— clean.go test ./...at repo root — all unit tests pass (credential-free; covers wrap/unwrap helpers, the round-trip property over ASCII/UTF-8/raw-byte/boundary inputs, malformed-input fallback, and separator variants).cd libdnstest && go test -v -run TestCloudflareLongTXTRoundTripwithCLOUDFLARE_API_TOKEN+CLOUDFLARE_TEST_ZONEset — needs to run against a real zone to confirm the wire-format contract.cd libdnstest && go test -v ./...with credentials — full upstreamlibdnstestsuite still passes (no regression on short TXT or other record types).Notes for review
wrapContent/unwrapContentrather than renamed to*TXT*— they're package-private and all three callsites already guard onr.Type == \"TXT\". The doc comments now spell out the TXT-only contract.getDNSRecordsis technically observable toSetRecords: a transient API error during the "does record exist" check used to fall through tocreateRecord(and likely fail with81058 identical record existsor similar), now it surfaces directly. That's correct behavior — the old swallow was hiding real problems — but flagged here in case you want to discuss.getDNSRecordsalready had no content filter (the API doesn't supportcontent.exactfor those types), soDeleteRecordsagainst any of those types would match by name+type only and delete every record at that name. That's a pre-existing bug, not introduced or affected here.