fix(rbac): make wildcard-method rules reachable and resolve overlapping patterns deterministically (#3808)

[akshat-kumar-singhal]

Description:

Fixes #3808.

An RBAC endpoint declared with "methods": ["*"] never matched any request. Because the middleware
passes through on an unmatched route, the endpoint it was meant to protect served unauthenticated
traffic — a route the operator believes is gated was silently ungated.

Two things beyond the original report turned up while confirming it, both verified against
development:

• Omitting methods entirely has the same effect. It defaults to ["*"] at config.go:296-298,
  so an operator who simply doesn't write the field gets an ungated route.
• The pattern-overlap nondeterminism is per request, not per process. Go re-randomises map order
  on every range, so 50 identical requests in one process split 10×200 / 40×403 — a permissive rule
  intermittently shadows a strict one.

Root Cause Analysis

Symptom. With a config containing {"path": "/admin/{path:.*}", "methods": ["*"], "requiredPermissions": ["admin:read"]}, DELETE /admin/orgs/123 with no role returns 200 and
reaches the handler. The control — the same shape with "methods": ["GET"] — correctly returns 401.
GetEndpointPermission("GET", "/admin/orgs/123") likewise returns ([], false).

Root cause. buildEndpointKey (config.go:324) keys the lookup maps on the declared method,
storing *:/admin/{path:.*}. matchesKey (config.go:471) requires the key to begin with the
request's method via strings.HasPrefix(key, methodUpper+":"). GET: never prefixes *:, so
the entry is unreachable through either map lookup the middleware performs. middleware.go:123 then
treats "no rule matched" as "nothing to enforce" and calls the next handler.

Why it wasn't caught. The package had two matchers. matchEndpoint
(endpoint_matcher.go:33) handles "*" correctly via matchesHTTPMethod and is the one the tests
exercise — but the middleware calls the map-based getEndpointForRequest instead. The correct
implementation was production-dead, and no test covered wildcard methods through the path that
actually runs.

Fix. Both entry points now resolve through a single ordered rule list, built once at load:

• Exact-path lookups stay O(1). The request's own method is probed before *:, so an explicitly
  declared method still wins over a wildcard on the fast path.
• Overlapping patterns resolve most-specific-first rather than by map iteration order: literal
  segment > constrained variable ({id:[0-9]+}) > free variable ({id}) > multi-segment catch-all,
  compared segment by segment, with an explicit method beating ["*"] on a tie. Declaration order
  decides only where two patterns score identically — the docs say where that is rather than
  claiming order-independence the scoring does not deliver.
• matchesKey, findEndpointByPattern, checkPatternMatch and checkExactMatch are removed. The
  new rule type calls the existing matchesHTTPMethod / matchesEndpointPattern primitives rather
  than reimplementing them — a second copy of that logic is what caused this bug.
• A constraint mux cannot parse ({id:[}) passes the brace-balance check, loads, and then silently
  never matches — the same failure shape reached a different way. That is now logged at error
  level naming the pattern and stating the endpoint is not enforced. It does not fail the load:
  GoFr's position is to log and stay up on a config defect (Add AddOptionWithValidation interface to support error reporting in service options #2378), and closing the hole properly
  needs the fail-closed default scoped into RBAC keeps a second copy of the route table; the router has already resolved the request by the time the middleware runs #3935.

Net −200/+80 lines in the package; the fix mostly deletes the duplicate matcher.

Prevention. Table-driven tests cover "*", omitted and empty methods, custom verbs
(PROPFIND), lower-case declarations, most-specific-wins in both declaration orders, explicit method
over wildcard, and the 401/403/200 enforcement matrix through the middleware. The overlap tests run
100 iterations per case; they fail reliably against the old map-range code.

On "*" and unknown methods

["*"] deliberately covers methods GoFr does not know about (WebDAV verbs, for example). The
rejected alternative was expanding "*" at load into one key per known method, which keeps O(1)
lookup but lets custom verbs slip past, recreating this bug class. Because an entry states a
requirement rather than a grant, covering an unrecognised verb tightens enforcement rather than
relaxing it.

On the overlap ordering

Three orderings were considered. First-declared-wins (Envoy, Spring) is order-sensitive — reordering
the JSON silently changes who is authorized. Union-of-matching-rules (the Kubernetes model) does not
transfer: in K8s a matching rule grants permissions so unioning is the strict direction, whereas a
GoFr entry imposes a requirement, making union the most permissive option and letting a broad weak
entry cancel a narrow strict one. Most-specific-wins is the only one under which a narrow entry can
genuinely override a broad one, which is what an operator writing both intends. Happy to switch if
maintainers prefer a different rule — it's isolated to compareSpecificity.

What this PR does not change

endpoint == nil still passes through. Every normative source (Saltzer & Schroeder's fail-safe
defaults, OWASP ASVS V4.1.5, OWASP API5:2023) says an unmatched request should be denied, but the
middleware sees every route including ones intentionally absent from the config
(/.well-known/health, /alive, /graphql/ui), so a blanket flip would break working apps. That
belongs in its own change — an opt-in defaultPolicy with a warn-then-enforce bridge, the way Envoy
(shadow_rules), OPA (dry-run) and Kubernetes PSA (audit/warn/enforce) ship equivalent flips — and
it overlaps #3763, which owns EnableRBAC. Keeping it separate leaves this PR non-breaking.

Breaking Changes (if applicable):

Requests that are served today will be rejected after this change. This is the point of the fix,
but it is an operational break, not just a security improvement. Any application running a
"methods": ["*"] entry — or one that omits methods, which means the same — has an unenforced
route right now. Once the rule is reachable, traffic through that route starts returning 401/403,
including traffic from callers who never held the required permission and have been getting through
regardless. Operators should audit ["*"] entries against real traffic before upgrading rather than
discovering the change through failed requests.

No API changes — every removed function was unexported and GetEndpointPermission keeps its
signature. Three behaviour changes worth calling out on release:

• Previously-unenforced ["*"] rules are now enforced, as above.
• A config with a pattern mux cannot compile still loads and still never matches, but now logs an
  error line naming it. No behaviour change for a running app beyond the new log.
• Overlapping patterns now resolve deterministically by specificity. Anyone who was relying on the
  previous outcome was relying on map iteration order, which varied per request — but the effective
  permissions for such a config can change.

Additional Information:

No new dependencies. mux.Route.GetError() is existing gorilla/mux API.

The issue's secondary observation (map-range nondeterminism) is fixed here rather than filed
separately, since most-specific-wins is the overlap fix — the two cannot be cleanly separated.

Follow-up filed as #3935. This PR consolidates RBAC's two internal matchers into one, but RBAC
still keeps a second copy of the route table and re-matches every request against it, even though
the router has already resolved the request before the middleware runs —
mux.CurrentRoute(r).GetPathTemplate() is available and unused. That structural duplication is why
a divergence like #3808 was possible at all, and #3935 covers it along with the boot-time coverage
check that the deferred fail-closed work depends on.

Verified locally: go build ./..., go test ./pkg/gofr/rbac/, go test -race -count=2 ./pkg/gofr/rbac/, go test ./pkg/gofr/ -run RBAC, and golangci-lint run clean on all changed
files.

Review round (@aryanmehrotra, 2026-08-20)

• Specificity tie fixed. {id} and {id:[0-9]+} scored equally, so sort.SliceStable fell
  through to declaration order — the exact order-sensitivity this PR rejects first-declared-wins for.
  A fourth level (segConstrained) sits between free variable and literal. Mutation-verified:
  removing the new case fails constrained variable wins over free one.
• Residual ties documented, not claimed away. Two identically-shaped patterns (/{a}/{b} vs
  /{x}/{y}) still fall back to declaration order, and {path:[a-z/]+} still scores as
  single-segment despite spanning segments. Both are now stated in the docs under Where the
  ordering is not decided.
• Load-time rejection reverted to a loud error log, as above.
• matchEndpoint deleted. Its eight single-case tests are one table-driven test against
  config.resolve.
• isCatchAllVariable now documents the consequence of the assumption, not just the assumption.
• Docs: the method-drop-out case in the worked example (GET /admin/orgs/123 needs only
  admin:read, because the narrow rule is ["DELETE"]-only and drops out on method), plus two new
  Troubleshooting entries.

Review round 2 (@Umang01-hash, 2026-08-21)

• /-bearing constraints are scored honestly, not disclosed as a gap. The previous round's
  disclosure was wrong about its own code: pathSpecificity("/x/{p:[a-z/]+}") returned three
  literal segments, not a single-segment variable, because the split ran before the brace check —
  so the loosest pattern in a config scored as the most specific one. Segments now split at brace
  depth zero (which also keeps {id:[0-9]{2,3}} intact), and any constraint containing / scores
  as a catch-all. That test is deliberately over-broad; it only ever moves a pattern down the
  ordering, so a rule can lose to a narrower one but never shadow one.
• A duplicate (method, path) declaration now overwrites in full. Pre-existing, reproduced
  against development first: storeEndpointMapping only ever set publicEndpointsMap[key], so
  a public entry followed by a protected one for the same key served the route unauthenticated
  while reporting the protected endpoint. Both the public flag and the stale permission list are
  now cleared. Rejecting duplicates was the alternative and was rejected for the same reason the
  load-time failure was: it turns a config that loads today into a startup failure.
• An equal-specificity tie prefers the protected rule. Previously a public rule declared before
  an overlapping protected one shadowed it — deterministic, and deterministically open. Declaration
  order now decides only between rules of the same public-ness.
• Ordering tests rewritten around cases that reach the ordered scan. narrow wins over broad
  and literal wins over param both survived a compareSpecificity reduced to len(a) - len(b) —
  the first because the patterns differ in length, the second because a literal request path is
  answered by the exact-key fast path and never reaches the scan. Added same-depth cases
  (/{scope}/orgs/{org_id} against /admin/orgs/{org_id} and against /admin/{section}/{org_id},
  more specific declared last) plus direct table tests on pathSpecificity and
  compareSpecificity, including the antisymmetry sort.SliceStable depends on. Every fix in this
  round was mutation-verified.
• Docs: the ordering section states the protected-beats-public tie-break and the conservative
  catch-all rule; a new section covers declaring the same path twice.

For the release notes, beyond "["*"] rules are now enforced": the level they are now enforced
at may be weaker than the permission name suggests. {"methods": ["*"], "requiredPermissions": ["admin:read"]} gates DELETE at admin:read. Operators should audit the permission, not just the
path.

Overlap policy — raised on #3808 for a maintainer call before this lands, per the review.

Checklist:

• I have formatted my code using goimport and golangci-lint.
• All new code is covered by unit tests.
• This PR does not decrease the overall code coverage.
• I have reviewed the code comments and documentation for clarity.

[aryanmehrotra]

Thanks for this — and for the write-up, the root-cause section made this a lot faster to verify.

TL;DR: the bug is real, I reproduced both halves of it against development before reading your fix, and the fix is the right shape. One real change needed around the ordering guarantee, plus one hunk I'd rather we drop. Everything else is approvable.

What it does

Wildcard-method entries were keyed on the declared method (*:/admin/{path:.*}) while matchesKey gated on the request's method, so the rule was unreachable and the middleware's pass-through-on-no-match turned it into an open route. This consolidates both entry points onto one ordered rule list built at load, and resolves overlaps by specificity instead of map-range order.

I confirmed on development:

CONTROL  GET    /team/anything   -> 401  (["GET"] rule works)
SUBJECT  DELETE /admin/orgs/123  -> 200  reached handler   (["*"] rule never fires)
OMITTED  GET    /nomethod/x      -> 200  reached handler   (no methods field)

and the overlap flapping — 50 identical requests in one process: map[200:42 403:8], different split every run. Your point that it's per-request rather than per-process is correct. On your branch all three return 401 and 400 identical overlapping requests give map[403:200]. Documented mux patterns all still validate, including the UUID one with nested braces.

Bug — the ordering guarantee doesn't hold as documented

pkg/gofr/rbac/resolver.go:99 — pathSpecificity scores a constrained variable and a free one identically, so sort.SliceStable falls through to declaration order:

score {id}        = [3 3 2]
score {id:[0-9]+} = [3 3 2]

declaration order [strict, loose] -> [users:strict]
declaration order [loose, strict] -> [users:loose]

Same config, entries swapped, different permission enforced. That contradicts the line you added to the docs — "independent of the order the entries appear in the config file" — and it's the same property you reject for first-declared-wins in the PR description ("reordering the JSON silently changes who is authorized"). It's a realistic config too: our own docs use both {id:[0-9]+} and {resource}.

Cheapest fix is another level, since a constrained variable genuinely constrains more than a free one:

const (
	segCatchAll    = iota + 1 // {path:.*}
	segVariable               // {id}
	segConstrained            // {id:[0-9]+}
	segLiteral                // users
)

case isCatchAllVariable(segment):
	scores = append(scores, segCatchAll)
case strings.Contains(segment, ":"):
	scores = append(scores, segConstrained)
default:
	scores = append(scores, segVariable)

Whatever residual tie is left after that, lets say so plainly in the docs rather than claim order-independence we don't have.

On process

One thing for next time — #3808 has no maintainer comments on it, so the approach here was chosen and built in a single pass. You did lay out three options in the issue and explain why you picked one, which I appreciate, and the fix itself was never in question. But the overlap policy is exactly the kind of call to settle in the issue before there's a diff riding on it. It's much cheaper to change direction there than here — you've written compareSpecificity, the ordered rule list and a large block of table-driven tests around a decision we hadn't actually made yet.

Lets do that part in the issue before the next round.

Lets keep the current validation behaviour

pkg/gofr/rbac/endpoint_matcher.go:110 — I'd drop this one. Compiling the pattern at load turns a config that boots today into a startup failure, and our position has been to log and stay up rather than hard-fail (#2378). I don't want to change that as part of a bugfix.

Being explicit about what that costs, though, because it isn't free: without the compile check, a pattern mux can't parse still loads and then never matches, so the endpoint it was meant to govern stays unguarded — the same shape as the bug you're fixing here. So lets keep validateMuxPattern non-fatal but make it loud: log the pattern that won't compile at error level and carry on, rather than either aborting or staying silent. Genuinely closing that hole needs the fail-closed default you've already scoped into #3935, and it belongs there rather than widening this PR.

The overlap policy is a separate question and does still want a second maintainer's eyes before it lands — most-specific-wins vs first-declared vs union is framework shape, and it's load-bearing for anyone running RBAC.

Also: this is a breaking change for running apps, and the PR is honest about it. Anyone with a ["*"] entry — or one omitting methods — has an unenforced route right now that starts returning 401/403 on upgrade. That needs to land in the release notes, not just the PR body. Worth adding that the newly-enforced level may be weaker than the permission name suggests: {"methods": ["*"], "requiredPermissions": ["admin:read"]} gates DELETE at admin:read, so operators should audit the permission, not just the path.

Smaller things

pkg/gofr/rbac/endpoint_matcher.go:36 — matchEndpoint is now only reachable from endpoint_matcher_test.go, and it rebuilds and sorts the whole rule list on every call instead of using the prebuilt c.rules, while skipping the exact-key fast path. Your own argument is that a second copy of this logic is what caused the bug; a second entry point with different behaviour keeps that risk alive. Can we delete it and point those tests at config.resolve?

pkg/gofr/rbac/resolver.go:117 — isCatchAllVariable only recognises .* and .+. {path:[a-z/]+} matches /files/a/b/c but scores as single-segment, so a broad rule can outrank a narrower one. The comment documents the assumption; can we document the consequence too?

Docs — two gaps:

• The worked example doesn't cover the case operators will actually get wrong. With your own config, DELETE /admin/orgs/123 requires admin:write but GET /admin/orgs/123 requires admin:read, because the narrow rule is ["DELETE"]-only and drops out on method. Two sentences would save someone assuming they'd protected the path.
• The new load-time rejection isn't mentioned anywhere, including Troubleshooting.

Good call keeping the fail-closed default out of this — that's a documented contract ("Unmatched Routes Behavior") and reversing it needs its own change with a migration path, the way you've scoped it in #3935.

One real change needed (the specificity tie), one to drop (the startup validation); the rest is small. Nice work on the root cause.

[akshat-kumar-singhal]

Thanks — all three land, and the specificity one was a real hole. Addressed in 9e1582c.

Specificity tie. You're right, and it's the property this PR argues for elsewhere, so it had to
go. Added segConstrained between free variable and literal exactly as you laid it out:

segCatchAll < segVariable < segConstrained < segLiteral

Mutation-verified rather than just asserted — removing the new case fails
constrained variable wins over free one with /users/{id}, in both declaration orders.

On the residual ties: I've stopped claiming order-independence and said where the ordering stops
deciding instead. Two cases, both now in the docs under Where the ordering is not decided —
identically-shaped patterns (/{a}/{b} vs /{x}/{y}), and the isCatchAllVariable gap you flagged
separately. Ranking either properly means parsing the constraint, which isn't worth the machinery
for a case the documented .*/.+ forms avoid.

Load-time validation. Reverted to non-fatal, and I agree with the reasoning — #2378 is a
standing position and a bugfix isn't where to reverse it. The check moved to
logUncompilablePattern, which names the pattern at error level and says plainly what it costs:

RBAC: endpoint[2]: invalid mux pattern: "/api/{id:[}": ... This endpoint will never match a
request, so it is NOT enforced - any route it was meant to govern is currently unguarded.

Brace-balance validation keeps its existing fatal behaviour — only the step I added is downgraded.
The tests changed shape with it: the load now succeeds and the assertion is on the log line.

matchEndpoint. Deleted. You're right that a second entry point with different behaviour keeps
the risk alive — and it also skipped the fast path, so it wasn't even testing what runs. The eight
single-case tests are now one table-driven test against config.resolve.

isCatchAllVariable. Consequence documented alongside the assumption, in the code and the docs:
{path:[a-z/]+} spans segments but scores as one, so it can outrank a narrower pattern it fully
contains.

Docs. Both gaps covered. The worked example now calls out the case that actually bites —
DELETE /admin/orgs/123 needs admin:write, but GET /admin/orgs/123 needs only admin:read,
because the narrow rule is ["DELETE"]-only and drops out on method before specificity is
considered. Two Troubleshooting entries added: the invalid mux pattern log line, and "the wrong
permission is being required on a path two entries match".

Release notes. Added your point to the PR body — the newly-enforced level may be weaker than the
permission name suggests, so the audit is of the permission, not just the path.

On process. Fair, and I'd rather have had the ordering question settled before writing
compareSpecificity. I've put the three options on #3808 with the argument for each so the choice
can be made there rather than inside a diff. Happy to hold this PR on that, or to swap the policy —
it's compareSpecificity and nothing else.

go build ./..., go test -race -count=2 ./pkg/gofr/rbac/, go test ./pkg/gofr/ -run RBAC and
golangci-lint run are clean; the three remaining noctx hits in endpoint_matcher_test.go are
pre-existing and untouched.

[aryanmehrotra]

All three addressed — thanks for the quick turnaround, and for taking the process point rather than
arguing it.

I re-verified against 9e1582c rather than reading the diff, including end to end through a running
app with EnableRBAC, real routes and real requests:

The original bug, before and after.

                                        development          9e1582c
DELETE /admin/orgs/123   (no role)      204 handler reached  401
GET    /nomethod/thing   (no role)      200 REACHED-HANDLER  401

Overlapping rules, 40 identical requests as a role holding only the weaker permission.

development:  32 x 200,  8 x 403      (re-randomised per request)
9e1582c:      40 x 403, then 40 x 403 again

The specificity tie I raised. {id} scores [4 4 2], {id:[0-9]+} scores [4 4 3], and both
declaration orders now resolve to the constrained rule. Ranking is what decides it, not ordering.

Non-fatal validation. The app boots and says exactly what it costs:

ERROR  RBAC: endpoint[5]: invalid mux pattern: "/bad/{id:[}": ... This endpoint will never match
       a request, so it is NOT enforced - any route it was meant to govern is currently unguarded.

and the route it names serves unguarded, as documented. That's the behaviour we want.

matchEndpoint is gone, the broad rule still governs paths the narrow one doesn't match, and
go test -race -count=2 ./pkg/gofr/rbac/ is clean.

Approving. On the ordering policy — thanks for putting the three options on #3808; I'll settle it
there. Approval doesn't presume the answer: if we land on something other than most-specific-wins
it's an isolated change to compareSpecificity, and I'd rather this fix stopped sitting on a live
bypass in the meantime.

[aryanmehrotra]

Deliberately keeping this off the approval, since none of it is yours to fix here.

While testing this end to end I turned up three defects that predate the PR and are live on
development — filed together as #3979:

1. matchMuxPattern races and grows the router without bound. It calls NewRoute() on the
   shared config.muxRouter per rule per request, and gorilla/mux appends that route permanently:
   0 -> 114 routes after 100 requests, never shrinking, plus a confirmed DATA RACE under 50
   concurrent goroutines. Our suites miss it because they drive requests sequentially.
2. 533-3,734 allocations to authorize one request, depending on config size — the cost of
   recompiling mux patterns per rule per request, on the hot path of every route.
3. Multi-role JWTs authorize nothing. A ["admin","viewer"] claim becomes the string
   "[admin viewer]", which matches no configured role, so every request gets 403 with nothing
   logged to explain it. That's the default shape from Keycloak, Auth0 and Entra.

1 and 2 are the same fix, and it's the one you already argued for in #3935 — reading
mux.CurrentRoute(r).GetPathTemplate() removes matchMuxPattern and takes the race, the growth and
most of the allocations with it. Your instinct there was right; this is the evidence for it.

Would you like to take #3979 in a separate PR? No obligation — say no and I'll find someone, or pick
it up myself. If you do take it, 3 needs a semantics decision before any code (union the permissions
of every named role, or reject arrays at load with a clear error), so worth settling that on the
issue first — same as we're doing for the ordering policy on #3808.

One correction to something I measured: I earlier had this branch looking slower than development
on the resolver benchmark. That comparison was unfair — my harness declared the matching rule last,
which under the new stable ordering forces a full scan while the old map order averaged half. I'm
not reporting a regression from it; the absolute allocation count in #3979 is the finding.

[Umang01-hash]

Reviewed deep + ran locally, verifying not trusting. The core fix is correct and valuable — I reproduced it both ways: on development, DELETE /admin/{path:.} (methods [""]) returns perms=[] not-public, so the middleware endpoint==nil passthrough serves it unauthenticated; at this head it enforces [admin:read]. No breaking API change (signatures identical, resolver.go internal); build/vet/gofmt/-race clean; the wildcard/most-specific/explicit-beats-wildcard matrix passes.

Approve-leaning COMMENT — three small fixes worth making, then two pre-existing bypasses worth follow-up issues (not this PR's regressions). Inline below.

Also: I could NOT reproduce a fail-closed→fail-open regression for uncompilable patterns vs development — development's validateMuxPattern never mux-compiled ({id:[} loaded there too), so this PR is a net improvement (adds the error log). The fail-closed base only exists on an intermediate commit of this branch, not on development.

Pre-existing (follow-up, non-blocking):

• config.go: a duplicate (method, literal-path) declaration with a public entry before a protected one leaves publicEndpointsMap[key]=true while endpointMap[key] is the protected one, so getExactEndpoint returns isPublic=true → unauthenticated. getExactEndpoint/storeEndpointMapping are byte-identical to development, which probed the same fast path — so pre-existing. Worth clearing the public flag on a non-public overwrite, or rejecting duplicate keys.
• The equal-specificity tie-break resolves by declaration order, so a public rule declared before an overlapping protected one shadows it. Deterministic (better than the old random) and disclosed in the docs — fail-safe hardening would be protected-beats-public on a tie.

[Umang01-hash]

This docstring is factually wrong about its own code. pathSpecificity("/x/{p:[a-z/]+}") actually returns [4,4,4], not a single-segment variable(2): strings.Split(pattern, "/") runs before the brace check, so the '/' inside the constraint shatters it into three pseudo-segments, each failing the brace test and scored literal(4). Net effect is the opposite of what the comment says — the rule scores as maximally specific and can outrank a genuinely narrow protected rule (a fail-open if it's public). Trigger needs a '/'-in-regex constraint (a documented anti-pattern), so not a blocker, but please fix the scoring or at minimum correct the comment.

[akshat-kumar-singhal]

Confirmed by running it, and the numbers match yours: pathSpecificity("/x/{p:[a-z/]+}") returned [4 4 4]. strings.Split ran before the brace check, so the / inside the constraint shattered the segment into fragments that each failed the brace test and scored literal — the loosest pattern in the config scoring as the most specific one.

Fixed the scoring rather than the comment, in two parts:

• splitPatternSegments splits at brace depth zero, so a constraint is never cut in half. It also keeps {id:[0-9]{2,3}} intact, which the naive split broke the same way.
• isCatchAllVariable now also accepts any constraint containing /. {path:[a-z/]+} scores as a catch-all — least specific — so it loses to any narrower entry it overlaps.

The second half is deliberately over-broad: {id:[0-9]+/} is anchored to one segment but is still scored as a catch-all. Deciding it exactly means parsing the regex, and the approximation only ever moves a pattern down the ordering — it can lose to a narrower rule, never shadow one. That asymmetry is why it is safe, and it is now what both the docstring and the docs say instead of the old disclosure.

Mutation-verified: reverting the split fails TestPathSpecificity/constraint admitting a slash spans segments and TestGetEndpointForRequest_SlashConstraintDoesNotOutrankNarrowRule; reverting only the / case in isCatchAllVariable fails the first.

[Umang01-hash]

"so that resolution does not depend on declaration order" over-claims — equal-specificity ties DO fall through sort.SliceStable to declaration order. The docs page discloses the tie-break correctly; just soften this code comment so the two agree.

[akshat-kumar-singhal]

Right — softened. The comment now says the ordering makes a narrower rule govern even when a broader one is declared ahead of it, and that identically-scoring patterns keep declaration order, which agrees with the docs page.

One thing did change underneath it while addressing your last point below: an equal-specificity tie no longer resolves purely by declaration order — a rule that requires permissions now outranks a public one. Declaration order decides only between two rules of the same public-ness, which the comment states.

[Umang01-hash]

This case (and "narrow wins over broad" / "literal wins over param") doesn't actually pin path-specificity ordering — reverting compareSpecificity to still passes 5 of 7 subcases (the constrained-variable-wins cases do cover it). Worth tightening these named cases so they'd go red if ordering breaks.

[akshat-kumar-singhal]

Checked this the way you did, and it is worse than the comment says — with compareSpecificity reduced to return len(a) - len(b), 6 of the 7 subcases still pass. Two distinct reasons:

• narrow wins over broad passes because the patterns differ in length ([4 4 2] vs [4 1]), so length alone picks the right one. Nothing about segment-by-segment comparison is exercised.
• literal wins over param passes because a literal request path never reaches the ordered scan at all — resolve answers /admin/orgs/global from the exact-key fast path. That case documents the fast path, not the ordering.

Both are now backed by cases that do reach the scan: /{scope}/orgs/{org_id} against /admin/orgs/{org_id} (literal beats variable in the same position) and against /admin/{section}/{org_id} (the first differing segment decides). Same depth in each, more specific entry declared last, so a declaration-order fallback gets them wrong. Both go red under the length-only mutation.

Added direct table tests too, since the property is easier to pin at that level than through a request: TestPathSpecificity over each segment kind, and TestCompareSpecificity over each ordering relation plus the antisymmetry sort.SliceStable depends on.

[akshat-kumar-singhal]

Thanks for reproducing it both ways rather than reading it — and for checking the fail-closed→fail-open question against development instead of against the intermediate commit. Agreed on that finding: validateMuxPattern never mux-compiled on development, so {id:[} loaded there too and the error log is a net addition.

The three inline points are addressed (replies on each thread). Short version: the pathSpecificity docstring was wrong about its own code exactly as you measured, so the scoring is fixed rather than the comment — segments now split at brace depth zero and a constraint containing / scores as a catch-all; the over-claiming comment is softened; and the ordering tests are rewritten around cases that actually reach the ordered scan.

Both "pre-existing" items are fixed here rather than deferred. I reproduced each first, and both returned isPublic=true before the change:

• Duplicate (method, path) with a public entry first. storeEndpointMapping wrote endpointMap[key] unconditionally but only ever set publicEndpointsMap[key]. The two maps ended up describing different declarations, so getExactEndpoint returned the protected endpoint together with isPublic=true. Fixed at the write: the non-public branch clears the public flag, and the public branch clears any stale permission list, so whichever entry is declared last is the only one that survives. I chose last-wins over rejecting duplicate keys because rejecting turns a config that loads today into a startup failure, which is the same reversal you pushed back on for uncompilable patterns. Last-wins is now stated in the docs rather than being incidental.
• Equal-specificity tie resolving to a public rule. Taking the fail-safe hardening you suggested: a tie now prefers the rule that requires permissions. A tie is a config that expressed no intent either way, and of the two ways to be wrong about an unintended overlap, enforcing is the recoverable one — a 403 is visible and fixable, an unauthenticated read is neither. Declaration order now decides only between two rules of the same public-ness.

Every fix was mutation-verified — reverted in place, suite re-run, named test goes red:

reverted	fails
brace-aware split	TestPathSpecificity/constraint admitting a slash spans segments, SlashConstraintDoesNotOutrankNarrowRule
/ case in isCatchAllVariable	TestPathSpecificity/constraint admitting a slash spans segments
public-flag delete	DuplicateDeclarationLastWins/protected declared last is enforced
public/protected tie-break	ProtectedBeatsPublicOnTie/public declared first
compareSpecificity → len(a)-len(b)	TestCompareSpecificity (3 cases), MostSpecificWins (3 cases)

go build ./..., go test -race -count=2 ./pkg/gofr/rbac/, go test ./pkg/gofr/ -run RBAC, gofmt all clean. golangci-lint on the package reports only 21 pre-existing noctx findings (httptest.NewRequest), identical on development — 4 in endpoint_matcher_test.go, 17 in middleware_test.go. It surfaces 3 at a time because of max-same-issues, which is why they read as fewer than they are. Left alone: a whole-file sweep does not belong in this PR.

Unchanged: endpoint == nil still passes through. These two fixes narrow the fail-open surface where a rule does match; they do not change what happens when none does, which stays scoped to #3935 with the fail-closed default.