fix(fetch): every label before the suffix has to be real #680

Merged

coilyco-ops merged 1 commit from fix/every-label-before-the-suffix-claude into main

2026-08-13 18:38:58 +00:00

coilyco-ops commented

2026-08-13 18:36:17 +00:00

Member

closes #674

PR#672 fixed #668 with a length guard and merged first. The length guard admits a host with an empty label that is not the first one:

..mozilla.com      allowed=true
a..b.mozilla.com   allowed=true
.mozilla.com       allowed=false   <- what #672 fixed

len("..mozilla.com") > len("mozilla.com")+1 holds, so the guard passes.

The fix

Check the prefix rather than its length:

prefix := host[:len(host)-len(suffix)-1]
return prefix != "" && !strings.HasPrefix(prefix, ".") &&
	!strings.Contains(prefix, "..")

I made the same mistake first

My own branch for #668 started as a length check, and my own new test caught it letting ..mozilla.com through before I committed. That is why the test names the neighbours explicitly — the case is one character away from the one everybody thinks of, and a length guard looks correct against .mozilla.com alone.

So this is not a criticism of #672. It is the same wrong instinct, and the only reason mine did not ship is that I happened to write the neighbouring cases down.

What I kept from #672

Its normalisation and its comment, which are on main and are right. The test is renamed to TestEveryLabelBeforeTheSuffixIsReal so it complements fetch_test.go's rather than colliding with it, and the two now cover different things.

Mutation run: replacing the prefix check with prefix != "" fails on ..mozilla.com.

ward exec gate green on a branch rebased onto current main.

closes #674 PR#672 fixed #668 with a length guard and merged first. **The length guard admits a host with an empty label that is not the first one:** ``` ..mozilla.com allowed=true a..b.mozilla.com allowed=true .mozilla.com allowed=false <- what #672 fixed ``` `len("..mozilla.com") > len("mozilla.com")+1` holds, so the guard passes. ## The fix Check the prefix rather than its length: ```go prefix := host[:len(host)-len(suffix)-1] return prefix != "" && !strings.HasPrefix(prefix, ".") && !strings.Contains(prefix, "..") ``` ## I made the same mistake first My own branch for #668 started as a length check, and **my own new test caught it letting `..mozilla.com` through** before I committed. That is why the test names the neighbours explicitly — the case is one character away from the one everybody thinks of, and a length guard looks correct against `.mozilla.com` alone. So this is not a criticism of #672. It is the same wrong instinct, and the only reason mine did not ship is that I happened to write the neighbouring cases down. ## What I kept from #672 Its normalisation and its comment, which are on main and are right. The test is renamed to `TestEveryLabelBeforeTheSuffixIsReal` so it complements `fetch_test.go`'s rather than colliding with it, and the two now cover different things. Mutation run: replacing the prefix check with `prefix != ""` fails on `..mozilla.com`. `ward exec gate` green on a branch rebased onto current main.

coilyco-ops added 1 commit

2026-08-13 18:36:17 +00:00

fix(fetch): a wildcard needs a real first label, and normalises its own input

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

Details

ci / test (pull_request) Successful in 37s

Details

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

Details

ci / publish-observed (pull_request) Has been skipped

Details

9a72a9e7ed

Two gaps in the wildcard I merged an hour ago, both found by the other Angie
seat probing the landed version rather than arguing for theirs.

.mozilla.com matched *.mozilla.com, because it ends with the pattern. The
dialer refuses to resolve it, so nothing was reachable, but an allowlist whose
correctness depends on a later stage failing is not correct. Every label before
the suffix must now be real, which also refuses ..mozilla.com and a..b.

hostAllowed lowercased the pattern and not the host. allowedURL lowercases
before calling, so the live path was fine and my tests passed because they used
lowercase hosts. The function reads as a general predicate and was not one, so
the next caller inherited a landmine. Both sides are normalised inside it now.

I wrote three tests for the suffix-attack shape and none for a degenerate host,
because I was thinking about what an attacker appends rather than what a parser
accepts.

My first fix was a length check and my own new test caught it letting
..mozilla.com through. Both mutations are run: dropping the host lowercase
fails the case test, and dropping the label check fails the label test.

Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

2026-08-13 18:36:17 +00:00

coilyco-ops merged commit db55a18f45 into main

2026-08-13 18:38:58 +00:00

coilyco-ops referenced this pull request from a commit

2026-08-13 18:38:59 +00:00

Merge pull request 'fix(fetch): every label before the suffix has to be real' (#680) from fix/every-label-before-the-suffix-claude into main