fix(approval): match static-rule hosts case-insensitively - #41
Conversation
Host names are case-insensitive (RFC 4343), but staticURLMatches compared the URL and the rule pattern byte for byte. A rule written in lower case did not match a request that varied the case of the host, even though both reach the same server. An allow rule for *.example.com missed a request to API.Example.com, which then fell through to the LLM judge instead of matching the rule. Fold the scheme and host of both the URL and the pattern to lower case before matching. Only the authority is folded, so paths and query strings stay case-sensitive. This covers prefix, exact and glob rules, and the eval runner's MatchesStaticRules.
|
| Filename | Overview |
|---|---|
| internal/approval/manager.go | Adds lowerAuthority to fold scheme and host to lowercase before stripDefaultPort, then before pattern matching — correctly orders operations so uppercase-scheme patterns have their default ports stripped |
| internal/approval/hostcase_test.go | New test file with 8 table-driven cases covering prefix/exact/glob rules for both allow and deny actions, plus an upper-case pattern against a lower-case URL, and a negative case confirming path case is preserved |
| internal/approval/manager_llm_test.go | Adds 6 new rows to TestStaticURLMatches, including two specifically testing that an uppercase scheme with a redundant default port is normalised correctly — directly covering the ordering dependency between lowerAuthority and stripDefaultPort |
Reviews (3): Last reviewed commit: "fix(approval): fold case before strippin..." | Re-trigger Greptile
stripDefaultPort matches the scheme with a case-sensitive HasPrefix, so a pattern authored as "HTTPS://api.example.com:443/v1" kept its redundant port while the request URL had it stripped, leaving the two strings different and the rule unmatched. Run lowerAuthority first so the scheme is already lower case by the time stripDefaultPort inspects it.
|
Good catch, confirmed. Swapped the order so the case fold runs first, and added table cases for HTTPS://host:443 and HTTP://host:80 so the ordering is pinned. |
bjhaid
left a comment
bjhaid
left a comment
There was a problem hiding this comment.
LGTM. it would be nice if we could do all of this work on load of StaticRule in a non-exported field normalizedPattern rather than on every request, probably a micro optimization but it is something that should count in the long run.
Happy to merge as it is and you can follow up if you want to address my concerns or I can find time to do it when I have some bandwidth.
2 participants
Problem
Host names are case-insensitive (RFC 4343), but
staticURLMatchescompares the URL and the rule pattern byte for byte. A rule written in lower case does not match a request that varies the case of the host, even though both reach the same server.The effect shows up most often on allow rules. A rule for
*.example.comdoes not matchhttps://API.Example.com/v1/users, so traffic the operator already approved falls through to the LLM judge. That is a judge call on every such request, or a denial when the fallback is deny.Deny rules have the same gap. It matters most when a broad allow rule is paired with a host-based deny carve-out, since a static allow returns before the judge runs.
Fix
Lower-case the scheme and host of both the URL and the pattern before matching. Only the authority is folded, so paths and query strings keep their case. Userinfo ahead of
@is left alone.Doing this inside
staticURLMatchescovers prefix, exact and glob rules, andMatchesStaticRulesin the eval runner picks it up as well.Tests
TestStaticRuleHostCaseInsensitivecovers upper and mixed case hosts against prefix, exact and glob rules for both actions, including a pattern authored in upper case matched against a lower-case request. It also asserts that a path differing only in case still does not match, so the fold stays limited to the authority.Four rows added to
TestStaticURLMatchesfor the same behaviour at the matcher level.Ran
go test ./internal/approval/on Go 1.26.1 with the testcontainers Postgres setup. The new cases fail before the change and pass after it, and the rest of the package is unaffected.