coilyco-gaming/sirens-echo

Watch

The empty-first-label guard measures length, so ..mozilla.com passes — and the test that lists it never asserts it #674

New issue

Closed

opened 2026-08-13 18:30:44 +00:00 by coilyco-ops · 1 comment

coilyco-ops commented

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

Member

Filed by Quail (QA) · seat claude. Found reviewing PR #672, which merged. Not a bypass — no attacker-controlled host is reachable — but the guard's stated purpose is unmet for three shapes, and the test that was added to cover it asserts less than it lists.

The security property is fine

I probed the matcher adversarially before anything else, because it decides whether a request leaves the process:

refused  "notmozilla.com"
refused  "evil.com"
refused  "mozilla.com.evil.com"
refused  "www.mozilla.com.evil.com"

Suffix confusion is refused in both forms. That is the one that matters and #672 did not weaken it.

The guard misses three shapes

// The length guard is the empty first label. ".mozilla.com" carries the
// suffix and names no host. See sirens-echo#668.
return len(host) > len(suffix)+1 && strings.HasSuffix(host, "."+suffix)

Measured against *.mozilla.com:

refused  ".mozilla.com"      <- the case the guard was written for
ALLOWED  "..mozilla.com"
ALLOWED  ". .mozilla.com"
ALLOWED  "-.mozilla.com"

The comment names the intent exactly — "the empty first label" — and length is a proxy for it. ..mozilla.com is one character longer than the string the guard rejects, so it clears the bound while having the same empty first label.

None of these resolves, so the worst case is a DNS failure rather than a request going somewhere it should not. The defect is that the allowlist says yes to a host that does not exist, which the comment says it exists to prevent.

The test lists two cases and asserts one

for _, host := range []string{".mozilla.com", "..mozilla.com"} {
	if hostAllowed(host, "*.mozilla.com") && host == ".mozilla.com" {
		t.Errorf(...)
	}
}

The && host == ".mozilla.com" makes the second iteration unconditionally silent. ..mozilla.com is named in the test, exercised, and cannot fail it.

That is worth more than the guard gap. A reader sees two hosts in the loop and concludes both are covered, and the one that is not covered is the one that is broken.

Shape

Check the first label rather than the total length:

if !strings.HasSuffix(host, "."+suffix) {
	return false
}
label := strings.TrimSuffix(host, "."+suffix)
return label != "" && !strings.Contains(label, "..") && strings.TrimSpace(label) != ""

Or more simply, reject any host with an empty label anywhere. That covers all three and matches the comment already in the file.

Fixing the test's condition alone will turn main red, which is correct — it is the guard that is wrong, not the test's list. Whoever takes this should expect the flip.

Also worth knowing, and I would not change it

wildcard=false  apex=false   "www.mozilla.com."

The trailing-dot FQDN form is refused, though it resolves to the allowed host. That is the safe direction and I do not know whether anything upstream can produce that form, so I am recording rather than proposing.

Acceptance

..mozilla.com, . .mozilla.com and -.mozilla.com are refused against *.mozilla.com.
a.mozilla.com, www.mozilla.com, deep.sub.mozilla.com and WWW.MOZILLA.COM still pass.
The test asserts every host it lists.

I will write the rows the moment the guard changes, both directions, and I will include the four must-allow cases so the fix cannot buy the reject half by tightening too far. Unclaimed; hostAllowed is production.

Filed by Quail (QA) · seat `claude`. Found reviewing PR #672, which merged. **Not a bypass** — no attacker-controlled host is reachable — but the guard's stated purpose is unmet for three shapes, and the test that was added to cover it asserts less than it lists. ## The security property is fine I probed the matcher adversarially before anything else, because it decides whether a request leaves the process: ``` refused "notmozilla.com" refused "evil.com" refused "mozilla.com.evil.com" refused "www.mozilla.com.evil.com" ``` Suffix confusion is refused in both forms. That is the one that matters and `#672` did not weaken it. ## The guard misses three shapes ```go // The length guard is the empty first label. ".mozilla.com" carries the // suffix and names no host. See sirens-echo#668. return len(host) > len(suffix)+1 && strings.HasSuffix(host, "."+suffix) ``` Measured against `*.mozilla.com`: ``` refused ".mozilla.com" <- the case the guard was written for ALLOWED "..mozilla.com" ALLOWED ". .mozilla.com" ALLOWED "-.mozilla.com" ``` The comment names the intent exactly — *"the empty first label"* — and length is a proxy for it. `..mozilla.com` is one character longer than the string the guard rejects, so it clears the bound while having the same empty first label. **None of these resolves**, so the worst case is a DNS failure rather than a request going somewhere it should not. The defect is that the allowlist says yes to a host that does not exist, which the comment says it exists to prevent. ## The test lists two cases and asserts one ```go for _, host := range []string{".mozilla.com", "..mozilla.com"} { if hostAllowed(host, "*.mozilla.com") && host == ".mozilla.com" { t.Errorf(...) } } ``` The `&& host == ".mozilla.com"` makes the second iteration unconditionally silent. **`..mozilla.com` is named in the test, exercised, and cannot fail it.** That is worth more than the guard gap. A reader sees two hosts in the loop and concludes both are covered, and the one that is not covered is the one that is broken. ## Shape Check the first label rather than the total length: ```go if !strings.HasSuffix(host, "."+suffix) { return false } label := strings.TrimSuffix(host, "."+suffix) return label != "" && !strings.Contains(label, "..") && strings.TrimSpace(label) != "" ``` Or more simply, reject any host with an empty label anywhere. That covers all three and matches the comment already in the file. **Fixing the test's condition alone will turn `main` red**, which is correct — it is the guard that is wrong, not the test's list. Whoever takes this should expect the flip. ## Also worth knowing, and I would not change it ``` wildcard=false apex=false "www.mozilla.com." ``` The trailing-dot FQDN form is refused, though it resolves to the allowed host. That is the safe direction and I do not know whether anything upstream can produce that form, so I am recording rather than proposing. ## Acceptance - `..mozilla.com`, `. .mozilla.com` and `-.mozilla.com` are refused against `*.mozilla.com`. - `a.mozilla.com`, `www.mozilla.com`, `deep.sub.mozilla.com` and `WWW.MOZILLA.COM` still pass. - The test asserts every host it lists. **I will write the rows the moment the guard changes**, both directions, and I will include the four must-allow cases so the fix cannot buy the reject half by tightening too far. Unclaimed; `hostAllowed` is production.

coilyco-ops referenced this issue

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

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

coilyco-ops referenced this issue from a pull request that will close it,

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

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

coilyco-ops closed this issue

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

coilyco-ops commented

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

Author

Member

Re-measured against merged main. The rows are shipped, and there are nine of them rather than one. Quail (QA, claude seat).

I said I would write the rows the moment the guard changed, both directions, including the four must-allow cases. sirens-echo#680 merged, so here they are: sirens-echo#688, tests only.

The fourth shape is still open, and it has eight neighbours

-.mozilla.com is the one this issue named. Probing the merged guard across label shapes turned up eight more:

-.mozilla.com  -a.mozilla.com  a-.mozilla.com    a hyphen cannot open or close a label
_.mozilla.com  "a b.mozilla.com"                 not hostname characters
a/b.mozilla.com  a:80.mozilla.com                URL delimiters inside a host
*.mozilla.com                                    a star is not a label
aaaa..(64)..a.mozilla.com                        one octet over the label limit

Not reachable today. allowedURL is the only caller and passes parsed.Hostname(), which strips the port and cannot contain a slash. I am not calling this exploitable and I would not raise its priority.

What makes it worth closing is the standard already written into TestTheHostIsNormalisedInside: "hostAllowed reads as a general predicate, so it must not depend on its caller having lowercased. allowedURL does; the next caller might not." The slash and colon rows are that same sentence, one field over.

One rule closes all nine

The commit title on sirens-echo#680 is "every label before the suffix has to be real", and the check under it excludes an empty prefix, a leading dot and a doubled dot. That is three exclusions, not a definition of real. Exclusion lists is how we got here — sirens-echo#663 named one shape, sirens-echo#668 named the next, sirens-echo#674 named four more.

prefix := host[:len(host)-len(suffix)-1]
for _, label := range strings.Split(prefix, ".") {
    if !validHostLabel(label) {
        return false
    }
}
return true

func validHostLabel(label string) bool {
    if len(label) == 0 || len(label) > 63 {
        return false
    }
    if label[0] == '-' || label[len(label)-1] == '-' {
        return false
    }
    for i := 0; i < len(label); i++ {
        c := label[i]
        digit := c >= '0' && c <= '9'
        letter := c >= 'a' && c <= 'z'
        if !digit && !letter && c != '-' {
            return false
        }
    }
    return true
}

The empty-label case falls out of len(label) == 0, so the three existing exclusions become consequences of the rule rather than entries beside it.

Measured, not asserted: dropped into fetch.go, this flips exactly nine rows in sirens-echo#688 and leaves the must-allow half green. xn--a.mozilla.com survives, which is the row I would expect a stricter rule to break, since punycode is how a non-ASCII name reaches the wire.

It runs after the lowercase normalisation, so the letter range does not need an uppercase arm. If that ordering ever changes, this silently starts refusing every capitalised host.

What is not mine

The fix. sirens-echo#688 is tests only and asserts current behaviour, so it goes green on main today and turns red the moment someone closes this — with a message naming this issue and telling them which column to flip. Whoever takes it does not need to read my table to know what is expected of them.

I have not claimed the shapes above are exhaustive. They are what one probe found.

**Re-measured against merged main. The rows are shipped, and there are nine of them rather than one. Quail (QA, `claude` seat).** I said I would write the rows the moment the guard changed, both directions, including the four must-allow cases. sirens-echo#680 merged, so here they are: sirens-echo#688, tests only. ## The fourth shape is still open, and it has eight neighbours `-.mozilla.com` is the one this issue named. Probing the merged guard across label shapes turned up eight more: ``` -.mozilla.com -a.mozilla.com a-.mozilla.com a hyphen cannot open or close a label _.mozilla.com "a b.mozilla.com" not hostname characters a/b.mozilla.com a:80.mozilla.com URL delimiters inside a host *.mozilla.com a star is not a label aaaa..(64)..a.mozilla.com one octet over the label limit ``` **Not reachable today.** `allowedURL` is the only caller and passes `parsed.Hostname()`, which strips the port and cannot contain a slash. I am not calling this exploitable and I would not raise its priority. What makes it worth closing is the standard already written into `TestTheHostIsNormalisedInside`: *"hostAllowed reads as a general predicate, so it must not depend on its caller having lowercased. allowedURL does; the next caller might not."* The slash and colon rows are that same sentence, one field over. ## One rule closes all nine The commit title on sirens-echo#680 is *"every label before the suffix has to be real"*, and the check under it excludes an empty prefix, a leading dot and a doubled dot. That is three exclusions, not a definition of real. Exclusion lists is how we got here — sirens-echo#663 named one shape, sirens-echo#668 named the next, sirens-echo#674 named four more. ```go prefix := host[:len(host)-len(suffix)-1] for _, label := range strings.Split(prefix, ".") { if !validHostLabel(label) { return false } } return true func validHostLabel(label string) bool { if len(label) == 0 || len(label) > 63 { return false } if label[0] == '-' || label[len(label)-1] == '-' { return false } for i := 0; i < len(label); i++ { c := label[i] digit := c >= '0' && c <= '9' letter := c >= 'a' && c <= 'z' if !digit && !letter && c != '-' { return false } } return true } ``` The empty-label case falls out of `len(label) == 0`, so the three existing exclusions become consequences of the rule rather than entries beside it. **Measured, not asserted:** dropped into `fetch.go`, this flips exactly nine rows in sirens-echo#688 and leaves the must-allow half green. `xn--a.mozilla.com` survives, which is the row I would expect a stricter rule to break, since punycode is how a non-ASCII name reaches the wire. It runs after the lowercase normalisation, so the letter range does not need an uppercase arm. If that ordering ever changes, this silently starts refusing every capitalised host. ## What is not mine The fix. sirens-echo#688 is tests only and asserts current behaviour, so it goes green on main today and turns red the moment someone closes this — with a message naming this issue and telling them which column to flip. Whoever takes it does not need to read my table to know what is expected of them. I have not claimed the shapes above are exhaustive. They are what one probe found.

coilyco-ops referenced this issue

2026-08-13 19:07:07 +00:00

Populate web fetch egress allowlist: wikipedia, github, all coilysiren domains, temporal.io, example.com, plus ~12 general-purpose domains #637

coilyco-ops referenced this issue

2026-08-13 19:09:56 +00:00

Populate web fetch egress allowlist: wikipedia, github, all coilysiren domains, temporal.io, example.com, plus ~12 general-purpose domains #637

coilyco-ops referenced this issue

2026-08-13 21:24:30 +00:00

Populate web fetch egress allowlist: wikipedia, github, all coilysiren domains, temporal.io, example.com, plus ~12 general-purpose domains #637