fix(filter): make the glob walker and its regex agree on trailing slashes - #435
Merged
Conversation
matchGlobSegments is the allocation-free stand-in for the anchored regex a single-star pattern compiles to, and the two disagreed in both directions on a path ending in "/". The walker absorbed a trailing empty segment after its last pattern segment, so "/containers/*" matched "/containers/abc/" where "^/containers/[^/]*$" does not, and it refused to spend a pattern segment on that empty segment, so "/*/*/*" did not match "/a/b/" where the regex does. Align the walker to the regex: a trailing slash is a real, empty final segment, and a pattern matches only a path carrying exactly as many "/"-separated segments as the pattern spends. The regex is the dialect's definition and the walker is only an optimization, so an optimization that answers differently is the bug by construction. NormalizePath's path.Clean strips a trailing slash, so neither half is reachable on an ordinary Docker route. NormalizePodmanRoutePath deliberately keeps the slash gorilla/mux routes on, so both were reachable on the libpod image-SCP route view, and both were policy bugs there. Absorbing the slash let "allow POST /libpod/images/scp/*" admit "/libpod/images/scp/alpine/", which Podman routes as an SCP of the image "alpine/" rather than of "alpine". Refusing to spend a segment let "deny POST /libpod/images/scp/*/*" miss "/libpod/images/scp/tenant/", so the "allow POST /libpod/images/scp/**" below it fired. Two alternatives were rejected. Aligning the regex to the walker, so a trailing slash is ignored, contradicts why NormalizePodmanRoutePath exists: that slash is what separates the SCP route from the push, tag and untag routes Podman registers earlier. Normalizing the slash away for matching while preserving it on the forwarded path is the same defect wearing a different hat, because "/libpod/images/scp/victim/push/" would then borrow a ".../push" allow and still be routed as an SCP. No shipped preset changes. All 24 presets in app/configs deny every POST /libpod/images/scp/... shape at the decoded view, which is evaluated first and is untouched here, so the route view is never consulted. Nothing legitimate narrows either: an image reference cannot end in "/", and the route view is only computed when the escaped path differs from the cleaned one. Widen the matcher differential corpus to build from both production path views instead of NormalizePath alone, which takes it to 993 paths; measured against the old walker, 512 of its verdicts over the segment-glob patterns disagreed with the regex, and none do now. Add table-driven cases for both shapes and for the libpod SCP route through the production evaluator, carry the walker-versus-regex invariant into FuzzPathMatch on both views, and seed both fuzz corpora with trailing-slash inputs.
…-slash # Conflicts: # CHANGELOG.md
Contributor
Author
|
@coderabbitai review |
scttbnsn
requested review from
ALARGECOMPANY and
biggest-littlest
as code owners
September 4, 2026 11:56
|
Deployment failed for project sockguard-website with the following error: Learn More: https://vercel.com/codeswhat?upgradeToPro=build-rate-limit |
|
Warning Review limit reachedNext included review available in 14 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
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 |
The trailing-slash note claimed a two-segment rule covers POST /libpod/images/scp/alpine/ on its own. evaluateRequestPolicy checks the cleaned view first and returns on a non-allow, and the cleaned path has one segment after scp, so a two-segment rule alone denies it. Spell out that both views have to allow, and keep the /** recommendation.
|
biggest-littlest
approved these changes
Sep 4, 2026
biggest-littlest
left a comment
biggest-littlest
left a comment
Member
There was a problem hiding this comment.
Walker now agrees with the regex on trailing slashes; presets unaffected, Codex's one doc finding fixed.
ALARGECOMPANY
approved these changes
Sep 4, 2026
ALARGECOMPANY
left a comment
ALARGECOMPANY
left a comment
Member
There was a problem hiding this comment.
Walker now agrees with the regex on trailing slashes; presets unaffected, Codex's one doc finding fixed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The segment-glob fast path and the regex it stands in for disagreed on trailing slashes. The walker absorbed a trailing empty segment after the last pattern segment (
/containers/*matched/containers/a/) but wouldn't spend a pattern segment on one (/*/*/*didn't match/a/b/). Unreachable viaNormalizePath, reachable viaNormalizePodmanRoutePath, which keeps the slash for the libpod image-SCP route.This aligns the walker to the regex: a trailing slash is a real empty final segment. The regex is the dialect's definition and the walker only exists to skip regexp on the hot path, so an optimization that answers differently is the bug. Both halves of the old behaviour were policy bugs on the SCP route, in opposite directions:
allow POST /libpod/images/scp/*admitted/libpod/images/scp/alpine/(Podman routes that as imagealpine/), anddeny POST /libpod/images/scp/*/*missed/libpod/images/scp/tenant/.No shipped preset changes outcome: all 24 presets under app/configs/ return 403 for every SCP shape tried, with or without the trailing slash, because the decoded view denies first. Differential test now covers both path views, fuzz seeds include the trailing-slash cases, and benchmarks stay at 0 allocs/op.
One pre-existing thing this doesn't touch: a rootless pattern like
*libpod/...still matches a rooted path under the walker but not the regex. That lenience is pinned by a test in app/internal/cmd and reachable only viaOPTIONS *or an absolute-form request line, so it goes on the roadmap rather than into this PR.