Skip to content

fix: discard wrong-family IP instead of returning it from getIPOnline - #319

Merged
TimothyYe merged 1 commit into
TimothyYe:masterfrom
MaxFreedomPollard:fix/ip-wrong-family
Sep 16, 2026
Merged

TimothyYe merged 1 commit into
TimothyYe:masterfrom
MaxFreedomPollard:fix/ip-wrong-family

Conversation

@MaxFreedomPollard

Copy link
Copy Markdown
Contributor

In IPv6 mode, when the configured IP endpoint answers with an IPv4 address, godns logs that it is skipping that address and then returns it anyway, so godns hands the provider an IPv4 literal for the AAAA record, and the handler caches that address as if the update succeeded.

pkg/lib/ip_helper.go:376 parses the response body straight into onlineIP, which is the variable the function returns. The family check just below rejects an answer of the wrong family by logging The online IP (%s) from %s is not IPV6, will skip it. (line 385) and calling continue, but nothing clears onlineIP on the way out. The retry loop is bounded by maxAttempts := len(helper.reqURLs) * 3 at line 342, so once every attempt has been rejected the loop simply ends and line 412 returns the last rejected address. The call at line 277 inside getCurrentIP treats any non-empty result as success and caches it with setCurrentIP, which means the ip == "" branch at line 279, the one that logs get ip online failed. Fallback to get ip from interface if possible. and tries the interface, never runs.

This is reachable without an exotic setup. In IPv6 mode without query_interface the dial protocol stays plain tcp (lines 301 to 308), so a dual stack echo endpoint is frequently reached over IPv4 and answers with the IPv4 address. An ip_type that matches neither family lands in the same place, because utils.IsIPv4 and utils.IsIPv6 both return false and every answer is skipped; the IPv4/IPv6 value asked about in #203 has that shape. #203 is asking for dual stack updates, which this does not add, so I cite it only as evidence that a neither-family ip_type reaches this code in practice. The IPv4 direction is harder to trigger since that dial is forced to tcp4, so I treat it as a symmetry case rather than a reported failure.

The fix keeps the parsed address in a local candidateIP and promotes it to onlineIP only after the family check passes and response.Body.Close() returns without error. Nothing changes when the close succeeds. A close error was already treated as a failed attempt with continue, so an otherwise good answer whose close fails is now discarded like any other failed attempt, which is the consistent behavior. The trailing if onlineIP == "" { log.Error("fail to get online IP") } was unreachable at that point, since candidateIP is already known to be non-empty and family checked, so the assignment takes its place.

TestGetIPOnlineRejectsWrongFamily in pkg/lib/ip_helper_test.go follows the httptest pattern already used across the repo. Each subtest starts a server that always answers with one fixed address, then builds an IPHelper directly with reqURLs pointing at that server and idx: -1, which is what UpdateConfiguration sets. The table covers IPv6 mode given an IPv4 answer, IPv4 mode given an IPv6 answer, an ip_type of IPv4/IPv6 that matches neither family, and both matching-family cases so the success path stays pinned. getIPOnline is an unexported method and the fields it needs are unexported too, so the test lives in package lib and no production change was needed to make it testable.

Environment: macOS 26.2, Go 1.26.4 on darwin/arm64. I did not run golangci-lint locally. gofmt and go vet are clean and the one comment the fix adds ends with a period for godot.

$ go test -race -count=1 ./... | grep -v "no test files"
ok  	github.com/TimothyYe/godns/internal/handler	1.803s
ok  	github.com/TimothyYe/godns/internal/manager	5.369s
ok  	github.com/TimothyYe/godns/internal/provider	2.964s
ok  	github.com/TimothyYe/godns/internal/provider/cloudflare	5.695s
ok  	github.com/TimothyYe/godns/internal/provider/digitalocean	2.267s
ok  	github.com/TimothyYe/godns/internal/provider/hetzner	5.025s
ok  	github.com/TimothyYe/godns/internal/provider/ionos	3.618s
ok  	github.com/TimothyYe/godns/internal/provider/namecom	6.344s
ok  	github.com/TimothyYe/godns/internal/settings	5.300s
ok  	github.com/TimothyYe/godns/internal/utils	5.347s
ok  	github.com/TimothyYe/godns/pkg/lib	5.334s
ok  	github.com/TimothyYe/godns/pkg/resolver	5.698s

$ go vet ./internal/... ./pkg/...
$ gofmt -l internal pkg cmd
(no output from either)

With pkg/lib/ip_helper.go reverted to master and the new test left in place:

--- FAIL: TestGetIPOnlineRejectsWrongFamily (0.02s)
    --- FAIL: TestGetIPOnlineRejectsWrongFamily/IPv6_mode_skips_an_IPv4_answer (0.01s)
        ip_helper_test.go:83: ip_type "IPV6" with endpoint answer "1.2.3.4": getIPOnline() = "1.2.3.4", want ""
    --- FAIL: TestGetIPOnlineRejectsWrongFamily/IPv4_mode_skips_an_IPv6_answer (0.01s)
        ip_helper_test.go:83: ip_type "IPV4" with endpoint answer "2001:db8::1": getIPOnline() = "2001:db8::1", want ""
    --- FAIL: TestGetIPOnlineRejectsWrongFamily/unknown_ip_type_skips_every_answer (0.00s)
        ip_helper_test.go:83: ip_type "IPv4/IPv6" with endpoint answer "2001:db8::1": getIPOnline() = "2001:db8::1", want ""
FAIL
FAIL	github.com/TimothyYe/godns/pkg/lib	5.708s
FAIL

One thing I left alone: UpdateConfiguration at pkg/lib/ip_helper.go:43 rebuilds reqURLs from the settings it is handed but never assigns them to helper.configuration, which stays fixed at whatever pointer reached GetIPHelperInstance first through its sync.Once. A caller that passes a different *settings.Settings gets fresh URLs while ip_type, user_agent, query_interface and the Mikrotik block all keep reading the old value. No caller does that today, but nothing stops one. That one touches singleton lifetime and locking around configuration, so it does not belong in this diff. Happy to send it separately if you want it.

getIPOnline parsed each answer straight into the function's return
variable, so an address rejected by the IPv4/IPv6 family check stayed in
that variable and was returned once the attempt budget ran out. The
caller then cached an address of the wrong family and never reached the
interface fallback, because the return value was not empty. The parsed
address now lives in a local that is promoted to the return value only
after the family check and the body close both succeed.
@TimothyYe
TimothyYe merged commit a49236d into TimothyYe:master Sep 16, 2026
3 checks passed
@TimothyYe

Copy link
Copy Markdown
Owner

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants