test: pin gobwas/glob host-matching contract - #837
Conversation
Three packages compile operator-supplied host patterns identically — glob.Compile(pattern, '.') then Match(host): - listener/skiphost/skiphost.go (bypasses the whole outbound pipeline) - routing/router.go (token-exchange routing) - plugins/tokenbroker/plugin.go (broker routing) None of them owns that behaviour; gobwas/glob does. skiphost.New's doc comment then makes load-bearing claims about it — that "*" matches every single-label host, that "**" is match-all, and that "*.*", "*.svc.cluster.local" and "service-*" are safe to accept precisely because they are not. Those claims decide which patterns the boot-time guard rejects, so a wrong one means an operator pattern silently exempts traffic from enforcement. Nothing pinned those claims to the library, so a glob upgrade could quietly invalidate them. #829 (glob v1.0.0) is exactly that shape: its release notes describe "rewrite glob for a much simpler and _correct_ engine". It turned out to be blocked on an unrelated API break, but the semantics risk was real and unguarded either way. These tests assert the contract against the library directly, so the next bump has to prove it still holds rather than assume it. Deliberately not covered: the empty host. It is the only input whose result differs between v0.2.3 and v1.0.0 ("?" matched "" in v0.2.3 and does not in v1.0.0; a run of three or more stars flipped the other way), both need a degenerate pattern to reach, and MatchPattern returns early on host == "" so skiphost never asks glob about it. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedNext included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mrsabath
left a comment
There was a problem hiding this comment.
Summary
Exemplary test-only PR. It pins the gobwas/glob host-matching contract that three packages silently depend on but none owns.
I verified every load-bearing claim against source on the head branch:
skiphost.New(skiphost.go:75) rejects exactly"*"and"**", and its doc comment (lines 49-64) makes precisely the claims the tests pin — including that*.*,*.svc.cluster.local, andservice-*are safe because they are not match-all.MatchPattern(skiphost.go:118) short-circuits onhost == "", so the "empty host deliberately not covered" call is correct — skiphost never asks glob about the one input where v0.2.3 and v1.0.0 disagree.- The "three packages" framing holds:
routing/router.go:53andplugins/tokenbroker/plugin.go:104both use the identicalglob.Compile(pattern, '.')+Match(host)idiom.
Ran all three tests locally against glob v0.2.3 — pass (CI's Go CI (authlib) agrees). The assertions are genuinely assertive: real messages tied to the security premise, no hidden skips, and the subtle cases (*.* pinned to two labels, ** crossing separators, label-exact suffixes) are all right. := inference keeps it compiling against either glob API.
Turning an unstated, security-relevant assumption into a contract the next dependency bump has to prove — 未雨绸缪 (mend the roof before the rain). LGTM.
Areas reviewed: Go (test), cross-package contract verification, local test execution
Commits: 1, signed-off (DCO passes)
CI status: passing (all checks green, incl. Go CI authlib)
glob v1 is a breaking API change, not just the engine rewrite its release notes describe: Compile went from returning the Glob interface to a concrete *Pattern, and the Glob interface was removed. authlib declares glob.Glob in three packages (listener/skiphost, routing, plugins/tokenbroker), and OPA declares it as well — v1/topdown/glob.go and v1/bundle/bundle.go, still true at 1.20.1 — so glob v1 cannot be adopted here until OPA migrates. That is outside this repo, so rossoctl#829 would be reopened on every glob release with no action available. Applied to the four gomod directories that require glob and have a Dependabot entry. storage/redis does not require it; cmd/authbridge-cpex and cmd/authbridge-praxis require it but have no Dependabot entry. Matching semantics are deliberately not cited as a reason: a v0.2.3-vs-v1.0.0 differential over the repo's real pattern corpus (36 patterns x 29 hosts) differed on two cells only, both on the empty host, which skiphost never passes to glob. The contract is pinned separately in rossoctl#837. Drop this ignore once OPA is on the glob v1 API. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
Summary
Pins the gobwas/glob host-matching behaviour that three packages depend on, so a future glob bump has to prove the contract rather than assume it.
listener/skiphost,routingandplugins/tokenbrokerall compile operator-supplied host patterns the same way —glob.Compile(pattern, '.')thenMatch(host)— and none of them owns that behaviour.skiphost.New's doc comment then makes claims about it that decide which patterns the boot-time guard rejects:*matches every single-label host (which is why it is rejected — every short in-cluster service name is one label)**is the unambiguous match-all (also rejected)*.*,*.svc.cluster.localandservice-*are safe to accept precisely because they are not match-allIf a glob upgrade widened any of those accepted patterns, the guard would be waving through a full enforcement bypass —
skip_hostsskips plugins and session recording. Nothing tied those claims to the library.Why now
#829 (glob 0.2.3 → 1.0.0) is exactly that shape — its release notes describe PR #73 as "v1: rewrite glob for a much simpler and correct engine." That PR turned out to be blocked on an unrelated API break (
glob.Globwas removed, and OPA depends on it too), so it is closed. But the semantics risk was real and unguarded either way, and the next bump will not necessarily announce itself.For the record, the differential I ran while reviewing #829 — 36 patterns × 29 hosts, the repo's real pattern corpus plus adversarial brace/class/escape syntax — found only two differing cells between v0.2.3 and v1.0.0, both on the empty-host input:
?""***""What the tests cover
TestGlobContract_SeparatorSemantics*stays inside one dot-separated label;**crosses separators. 20 cases across the repo's real patterns.TestGlobContract_GuardedPatternsAreMatchAll*and**— that they do match the hosts the listener actually seesTestGlobContract_AcceptedPatternsAreNotMatchAllNewaccepts are not match-allDeliberately not covered: the empty host. It is the only input where the two versions disagree, both cells need a degenerate pattern to reach, and
MatchPatternreturns early onhost == ""so skiphost never asks glob about it. Pinning those cells would pin noise.Test-only change; no production code touched. Written with
:=inference so it compiles against either glob API.Assisted-By: Claude Code