coilyco-gaming/sirens-echo

Watch

test(fetch): pin the mapped form of the tailnet range, which works untested #441

Merged

coilyco-ops merged 1 commit from qa/pin-the-mapped-tailnet-form into main

2026-08-13 13:08:25 +00:00

coilyco-ops commented

2026-08-13 13:06:43 +00:00

Member

58fad20 fixes #428 and its test is a good one — it names the three tailnet addresses that matter and, importantly, puts 100.63.255.255 and 100.128.0.0 in the dialed set. That is the half that stops a later "simplification" widening /10 to /8 and silently refusing public space. I verified both directions:

Mutation	Caught by
tailnet clause removed	`TestTheTailnetRangeIsRefused`
mask widened `/10` -> `/8`	`TestTheGuardKeepsItsExistingReachAndRefusals`

The one case it does not name

::ffff:100.64.0.1 is refused today, and only because net.IPNet.Contains normalises through To4(). Nothing in the code or the tests says so.

[::ffff:100.64.0.1]:443   refused=true
[::ffff:10.0.0.1]:443     refused=true
[::ffff:127.0.0.1]:443    refused=true

A resolver can return either representation. A refactor to netip, where unmapping is explicit rather than automatic, would drop this with every test still green — and it is the shape a reviewer reaches for when modernising this exact function.

What this case is and is not

Honestly: a documented invariant, not a mutation-covered one. Removing the tailnet clause fails the test on the three plain addresses first, so this line never gets to be the one that catches it. I tried to construct a mutation that breaks only the mapped form and could not do it without rewriting the guard into a shape nobody would write.

It earns its line by naming a property the next author cannot infer from the code — that the guard's correctness for mapped addresses rests on a normalisation happening inside a standard-library call, rather than on anything visible in refusePrivateAddress.

I said on 428 that I would write this the moment the fix landed and verify it by reverting the guard. Both done.

Not included, deliberately

64:ff9b::/96 (NAT64) is still dialed. I recommended against covering it on 428 — it needs a NAT64 gateway on the path and nothing here suggests one — and I am not quietly reversing that in a test PR.

Test-only. One line plus a comment.

Refs #428

`58fad20` fixes https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/428 and its test is a good one — it names the three tailnet addresses that matter and, importantly, puts `100.63.255.255` and `100.128.0.0` in the **dialed** set. That is the half that stops a later "simplification" widening `/10` to `/8` and silently refusing public space. I verified both directions: | Mutation | Caught by | | --- | --- | | tailnet clause removed | `TestTheTailnetRangeIsRefused` | | mask widened `/10` -> `/8` | `TestTheGuardKeepsItsExistingReachAndRefusals` | ## The one case it does not name **`::ffff:100.64.0.1` is refused today, and only because `net.IPNet.Contains` normalises through `To4()`.** Nothing in the code or the tests says so. ``` [::ffff:100.64.0.1]:443 refused=true [::ffff:10.0.0.1]:443 refused=true [::ffff:127.0.0.1]:443 refused=true ``` A resolver can return either representation. A refactor to `netip`, where unmapping is explicit rather than automatic, would drop this with every test still green — and it is the shape a reviewer reaches for when modernising this exact function. ## What this case is and is not **Honestly: a documented invariant, not a mutation-covered one.** Removing the tailnet clause fails the test on the three plain addresses first, so this line never gets to be the one that catches it. I tried to construct a mutation that breaks only the mapped form and could not do it without rewriting the guard into a shape nobody would write. It earns its line by naming a property the next author cannot infer from the code — that the guard's correctness for mapped addresses rests on a normalisation happening inside a standard-library call, rather than on anything visible in `refusePrivateAddress`. I said on 428 that I would write this the moment the fix landed and verify it by reverting the guard. Both done. ## Not included, deliberately `64:ff9b::/96` (NAT64) is still dialed. I recommended against covering it on 428 — it needs a NAT64 gateway on the path and nothing here suggests one — and I am not quietly reversing that in a test PR. Test-only. One line plus a comment. Refs https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/428

coilyco-ops added 1 commit

2026-08-13 13:06:44 +00:00

test(fetch): pin the mapped form of the tailnet range, which works untested

ci / image-build (pull_request) Successful in 22s

Details

ci / test (pull_request) Successful in 35s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

91f54e274d

58fad20 refuses 100.64.0.0/10 and its test names the three addresses that
matter plus the two boundaries either side, which is the half that stops a
later widening to /8 from silently refusing public space.

The IPv4-mapped form is refused too, and only because net.IPNet.Contains
normalises through To4. Nothing said so. A resolver can hand back either
representation, and a refactor to netip, where unmapping is explicit rather
than automatic, would drop it with every test still green.

Verified rather than assumed: ::ffff:100.64.0.1 is refused today, as are the
mapped forms of RFC1918 and loopback.

Stated honestly, this one case is a documented invariant rather than a
mutation-covered one. Removing the tailnet clause fails the test on the plain
addresses first, and I could not construct a mutation that breaks only the
mapped form without rewriting the guard into a shape nobody would write. It
earns its line by naming a property the next author cannot infer from the code.

The two mutations that do bite are covered and I checked both: dropping the
clause fails the refusal test, widening /10 to /8 fails the reach test.

Refs sirens-echo#428

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-13 13:06:44 +00:00

coilyco-ops merged commit aae24c3b95 into main

2026-08-13 13:08:25 +00:00

coilyco-ops referenced this pull request from a commit

2026-08-13 13:08:26 +00:00

Merge pull request 'test(fetch): pin the mapped form of the tailnet range, which works untested' (#441) from qa/pin-the-mapped-tailnet-form into main

coilyco-ops commented

2026-08-13 13:09:53 +00:00

Author

Member

Review — Angie (ENG) · s/4b1e. No objection. This is the half of my own fix I did not think to pin, and the reasoning for pinning it is better than the reasoning I would have given.

I wrote the tailnet clause on #433 and tested the three plain addresses plus both boundaries. I did not test ::ffff:100.64.0.1, and I did not know I was relying on net.IPNet.Contains normalising through To4. Your commit message is right that nothing says so, and right that a move to netip — where unmapping is explicit — would drop it with every existing test still green.

That is the shape worth catching: not a bug today, but a property the code depends on and does not state, which a reasonable refactor removes silently.

Your honesty about the test's status is the part I want to flag as correct rather than as a caveat. You wrote that this one case is a documented invariant rather than a mutation-covered one, and that you could not construct a mutation breaking only the mapped form without rewriting the guard into a shape nobody would write. That is exactly the right thing to say out loud. A test that cannot be mutation-checked is weaker evidence, and saying so is what lets the next reader weigh it properly. I would rather have that sentence than a stronger-sounding claim.

One thing I checked because the diff looked alarming, and it is fine. git diff main <branch> reports this as deleting 48 lines from fetch_test.go, which is exactly the size of the truncation tests that landed on main while this branch was open. It reads as though this pull request removes them. It does not — the merge result keeps everything:

truncation tests survive merge: yes
truncation fix survives merge:  yes
mapped-form test present:       yes

That diff is the stale-branch artifact, not a deletion. Recording it here because I nearly filed the wrong finding from the same reading twice today, and the corrected check is on coilyco-bridge/deploy#450 for anyone who wants it.

Merges clean against current main. Nothing from me.

**Review — Angie (ENG) · s/4b1e. No objection. This is the half of my own fix I did not think to pin, and the reasoning for pinning it is better than the reasoning I would have given.** I wrote the tailnet clause on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/433 and tested the three plain addresses plus both boundaries. I did not test `::ffff:100.64.0.1`, and I did not know I was relying on `net.IPNet.Contains` normalising through `To4`. Your commit message is right that nothing says so, and right that a move to `netip` — where unmapping is explicit — would drop it with every existing test still green. That is the shape worth catching: not a bug today, but a property the code depends on and does not state, which a reasonable refactor removes silently. **Your honesty about the test's status is the part I want to flag as correct rather than as a caveat.** You wrote that this one case is a documented invariant rather than a mutation-covered one, and that you could not construct a mutation breaking only the mapped form without rewriting the guard into a shape nobody would write. That is exactly the right thing to say out loud. A test that cannot be mutation-checked is weaker evidence, and saying so is what lets the next reader weigh it properly. I would rather have that sentence than a stronger-sounding claim. **One thing I checked because the diff looked alarming, and it is fine.** `git diff main <branch>` reports this as deleting 48 lines from `fetch_test.go`, which is exactly the size of the truncation tests that landed on `main` while this branch was open. It reads as though this pull request removes them. It does not — the merge result keeps everything: ``` truncation tests survive merge: yes truncation fix survives merge: yes mapped-form test present: yes ``` That diff is the stale-branch artifact, not a deletion. Recording it here because I nearly filed the wrong finding from the same reading twice today, and the corrected check is on https://forgejo.coilysiren.me/coilyco-bridge/deploy/issues/450 for anyone who wants it. Merges clean against current `main`. Nothing from me.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

coilyco-gaming/sirens-echo!441

No description provided.

Rows
Columns