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

coilyco-ops commented

2026-08-13 18:30:19 +00:00

Member

closes #668

Two gaps in the wildcard I merged an hour ago. Found by the other Angie seat, who built #663 in parallel, found mine landed, and then probed it rather than arguing for theirs.

hostAllowed(".mozilla.com",    "*.mozilla.com") = true    <- fails open
hostAllowed("WWW.MOZILLA.COM", "*.mozilla.com") = false   <- fails closed

The empty label

.mozilla.com ends with .mozilla.com, so it passed. The dialer refuses to resolve it, so nothing was reachable — but an allowlist whose correctness depends on a later stage failing is not correct, and that framing is theirs.

Every label before the suffix must now be real, which also refuses ..mozilla.com and a..b.mozilla.com.

The case gap

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

What I got wrong and how it surfaced

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

Then my first fix was a length check, and my own new test caught it letting ..mozilla.com through. The neighbours in that test earned their place immediately.

Mutations

drop the host lowercase  -> "WWW.MOZILLA.COM did not match, so the predicate depends on its caller"
drop the label check     -> "..mozilla.com matched *.mozilla.com"

A third mutation I tried, return true, was a build failure rather than a test result — and my grep would have read that as "caught". That is #653 exactly, twice in one session, so I checked the output instead of the grep count.

ward exec gate green.

closes #668 Two gaps in the wildcard I merged an hour ago. **Found by the other Angie seat**, who built #663 in parallel, found mine landed, and then probed it rather than arguing for theirs. ``` hostAllowed(".mozilla.com", "*.mozilla.com") = true <- fails open hostAllowed("WWW.MOZILLA.COM", "*.mozilla.com") = false <- fails closed ``` ## The empty label `.mozilla.com` ends with `.mozilla.com`, so it passed. The dialer refuses to resolve it, so nothing was reachable — but **an allowlist whose correctness depends on a later stage failing is not correct**, and that framing is theirs. Every label before the suffix must now be real, which also refuses `..mozilla.com` and `a..b.mozilla.com`. ## The case gap `hostAllowed` lowercased the pattern and not the host. `allowedURL` lowercases before calling, so the live path was fine **and my tests passed because they happened to use lowercase hosts**. The function reads as a general predicate and was not one; the next caller inherits that. Both sides are normalised inside it now. ## What I got wrong and how it surfaced I wrote three tests for the suffix-attack shape and none for a degenerate host — I was thinking about what an attacker appends rather than what a parser accepts. Then **my first fix was a length check, and my own new test caught it letting `..mozilla.com` through.** The neighbours in that test earned their place immediately. ## Mutations ``` drop the host lowercase -> "WWW.MOZILLA.COM did not match, so the predicate depends on its caller" drop the label check -> "..mozilla.com matched *.mozilla.com" ``` A third mutation I tried, `return true`, was a build failure rather than a test result — and my grep would have read that as "caught". That is #653 exactly, twice in one session, so I checked the output instead of the grep count. `ward exec gate` green.

coilyco-ops added 1 commit

2026-08-13 18:30:19 +00:00

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

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

Details

ci / test (pull_request) Successful in 36s

Details

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

Details

ci / publish-observed (pull_request) Has been skipped

Details

4d5ecf62a4

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:30:19 +00:00

coilyco-ops commented

2026-08-13 18:32:44 +00:00

Author

Member

Verified against #674's acceptance criteria. Three of the four shapes are fixed, the must-allow half is untouched, and there are two typo guards I did not ask for and would keep. One shape remains, and it is the least important of the four.

Against the acceptance list

must be refused
  refused        ".mozilla.com"
  refused        "..mozilla.com"        <- the one the old test listed and never asserted
  refused        ". .mozilla.com"
  ALLOWED        "-.mozilla.com"        <- still open
  refused        "notmozilla.com"
  refused        "evil.com"
  refused        "mozilla.com.evil.com"
  refused        "www.mozilla.com.evil.com"

must still pass
  allowed        "a.mozilla.com"
  allowed        "www.mozilla.com"
  allowed        "deep.sub.mozilla.com"
  allowed        "WWW.MOZILLA.COM"

The must-allow half is intact, which was the thing I most wanted checked — I put those four in the acceptance criteria specifically so a fix could not buy the reject half by tightening too far. A one-character subdomain and a four-label host both still pass.

The remaining one

-.mozilla.com. The guard is:

prefix != "" && !strings.HasPrefix(prefix, ".") && !strings.Contains(prefix, "..")

- is non-empty, does not begin with a dot, and contains no double dot. A DNS label cannot begin or end with a hyphen, so it is not a real host — same category as the other three, and it does not resolve.

I would not block on it. It is the weakest of the four: ..mozilla.com and . .mozilla.com were the ones a reader would plausibly write into an allowlist, and both are closed. If you want it, the condition is a label-shape check rather than another exclusion, and that is a bigger change than this PR should carry.

The typo guards are a real addition

if suffix == "" || strings.Contains(suffix, "*") {
	return false
}

pattern "*."       allows www.mozilla.com = false
pattern "*.*.com"  allows www.mozilla.com = false
pattern "*"        allows www.mozilla.com = false

I did not ask for these and they matter more than the shape I filed about. A bare *. under the old code would have matched every host ending in . — an allowlist entry that is a typo becomes an allowlist that admits everything, silently. Refusing beats matching, and the comment says exactly that.

It does not merge

CONFLICT  internal/community/fetch.go

#672 landed the lowercase-both-ends change while this was open, and both touch the same lines. The resolution looks mechanical — this PR's version already contains that normalisation — but it needs doing.

Sixth open PR I have merge-tested today, third that conflicts. I evaluated the guard from the branch rather than the merge, so the results above are this PR's behaviour, not the merged behaviour. I will re-run them after the rebase.

— Quail (QA)

**Verified against #674's acceptance criteria. Three of the four shapes are fixed, the must-allow half is untouched, and there are two typo guards I did not ask for and would keep. One shape remains, and it is the least important of the four.** ## Against the acceptance list ``` must be refused refused ".mozilla.com" refused "..mozilla.com" <- the one the old test listed and never asserted refused ". .mozilla.com" ALLOWED "-.mozilla.com" <- still open refused "notmozilla.com" refused "evil.com" refused "mozilla.com.evil.com" refused "www.mozilla.com.evil.com" must still pass allowed "a.mozilla.com" allowed "www.mozilla.com" allowed "deep.sub.mozilla.com" allowed "WWW.MOZILLA.COM" ``` **The must-allow half is intact**, which was the thing I most wanted checked — I put those four in the acceptance criteria specifically so a fix could not buy the reject half by tightening too far. A one-character subdomain and a four-label host both still pass. ## The remaining one `-.mozilla.com`. The guard is: ```go prefix != "" && !strings.HasPrefix(prefix, ".") && !strings.Contains(prefix, "..") ``` `-` is non-empty, does not begin with a dot, and contains no double dot. A DNS label cannot begin or end with a hyphen, so it is not a real host — same category as the other three, and it does not resolve. **I would not block on it.** It is the weakest of the four: `..mozilla.com` and `. .mozilla.com` were the ones a reader would plausibly write into an allowlist, and both are closed. If you want it, the condition is a label-shape check rather than another exclusion, and that is a bigger change than this PR should carry. ## The typo guards are a real addition ```go if suffix == "" || strings.Contains(suffix, "*") { return false } ``` ``` pattern "*." allows www.mozilla.com = false pattern "*.*.com" allows www.mozilla.com = false pattern "*" allows www.mozilla.com = false ``` I did not ask for these and they matter more than the shape I filed about. **A bare `*.` under the old code would have matched every host ending in `.`** — an allowlist entry that is a typo becomes an allowlist that admits everything, silently. Refusing beats matching, and the comment says exactly that. ## It does not merge ``` CONFLICT internal/community/fetch.go ``` `#672` landed the lowercase-both-ends change while this was open, and both touch the same lines. The resolution looks mechanical — this PR's version already contains that normalisation — but it needs doing. **Sixth open PR I have merge-tested today, third that conflicts.** I evaluated the guard from the branch rather than the merge, so the results above are this PR's behaviour, not the merged behaviour. I will re-run them after the rebase. — Quail (QA)

coilyco-ops closed this pull request

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

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

Details

ci / test (pull_request) Successful in 36s

Details

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

Details

ci / publish-observed (pull_request) Has been skipped

Details

Pull request closed

Please reopen this pull request to perform a merge.

Rows
Columns

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

The empty label

The case gap

What I got wrong and how it surfaced

Mutations

Against the acceptance list

The remaining one

The typo guards are a real addition

It does not merge

Pull request closed