feat(rbac): check rules against registered routes at startup

[akshat-kumar-singhal]

Description:

• Refs RBAC keeps a second copy of the route table; the router has already resolved the request by the time the middleware runs #3935 (items 1 and 2; item 3, template-keyed strict matching, is not included). Deny-by-default for uncovered routes is split out as RBAC: opt-in defaultPolicy: deny for routes no rule covers #4389.
• configs/rbac.json is a hand-kept second copy of the route table. A rule with a typo, such as /api/user/{id} when the route is /api/users/{id}, matches nothing, so the route it was written for is served with no role checks, and nothing reports it.
• Startup route check. app.Run() now compares the RBAC rules against the full route table, including GoFr's built-in routes, before any server starts. Paths are compared exactly as written, the way requests are matched: /api/users/ and /api/users are different paths, and so are files and /files.
  • Dead rule (matches no registered route, by path or by method): logged as an error, because the route it was written for is unguarded. A dead public rule is only a warning. This check is lenient on purpose (two path variables always count as overlapping), so a live rule is never reported as dead.
  • Uncovered route (a registered route, per method, that no rule matches): logged as an error; it is served with no role check. A route meant to be open is declared with "public": true. GoFr's own /.well-known/* and /favicon.ico are exempt; /static + /static/{path:.*} and /graphql need rules.
  • Partly covered route (rules match some of its requests but not all): its own error line naming the rules. Examples: {id:[0-9]+} against {id}, or a GET rule against a static directory, which answers every method. (/admin/{rest:.*} against /admin matches no request of the route at all, so it is reported as uncovered, and the rule as dead if nothing else matches it.) This check is strict: a rule covers a route only if it matches every request the route serves.
• GOFR_RBAC_ROUTE_CHECK chooses what a mismatch does: warn (default: log and keep starting), fail (log, release what startup opened, exit 1, like a failed OnStart hook), off (skip the check). An unknown value is logged and treated as warn.
• Trace label. On the rbac.authorize span, http.route is now the route the router matched (via gofrHTTP.RouteTemplate, so it works with both the mux and trie routers). The path of the rule that governed the request moves to a new rbac.rule attribute, so a mismatch between the two is visible. The audit log and ErrorHandler's route argument are unchanged.
• To make the check possible, httpServerSetup() now runs in a new prepareHTTPServer step in Run, after the OnStart hooks (a hook can still register a route) and before the MCP port is claimed and the servers start, instead of inside startHTTPServer. The route list is built in the router walk httpServerSetup already makes.

Example output:

RBAC route check failed: rules match no registered route: DELETE /api/user/{id}. These rules protect nothing; fix each rule's path or methods, or remove it.
RBAC route check failed: routes covered by no rule: GET /api/posts, POST /api/users. They are served without role checks; add a rule for each, with "public": true for a route meant to be open.
RBAC route check failed: routes only partly covered: GET /api/users/{id} (by GET /api/users/{id:[0-9]+}). Requests these rules do not match are served without role checks; add a rule that matches each route in full.

Breaking Changes (if applicable):

• None by default: with GOFR_RBAC_ROUTE_CHECK unset, an app whose rules and routes disagree gains error lines in its startup log and otherwise runs as before.
• Upgrade note: an existing RBAC app that relies on "no rule = open" (including /graphql and /static with no rule) will log these errors on every start. Add "public": true rules for those routes, or set GOFR_RBAC_ROUTE_CHECK=off.
• The http.route attribute on the rbac.authorize span changes from the rule path to the matched route template; the old value is now in rbac.rule. A dashboard or alert that groups this span by http.route gets different series and nothing fails, so group by rbac.rule to keep the old meaning. Upgrade note in docs/advanced-guide/rbac/page.md. A route or a static directory registered at / is reported as /; only GoFr's own 404 catch-all is reported as <unmatched>.

Additional Information:

• No new exported API. GoFr's 404 catch-all is marked once, where httpServerSetup registers it, by naming the route (routemark.MarkCatchAll in the internal package pkg/gofr/internal/routemark). The rbac.authorize span label and the route collector both ask that one mark (routemark.CatchAllAnswered / IsCatchAll). Looking it up is a string compare with no allocation, so a 404 with RBAC on takes 14 allocs on mux and 12 on trie, down from 21 and 19 with the regexp parse. A static directory or a route at / is not marked, so it keeps its own template.
• The check lives in pkg/gofr/internal/rbaccheck: Check(endpoints []rbac.EndpointMapping, routes []Route) Mismatches takes typed routes (Method, Path, BuiltIn) and returns the four lists without logging. prepareHTTPServer writes the log lines and applies GOFR_RBAC_ROUTE_CHECK. The check rebuilds the rules from Config.Endpoints the way the matcher does (no methods means every method, methods are upper-cased, and a repeated method and path replaces the earlier one); TestCheck_AgreesWithMiddleware runs the real loader and middleware and fails if the two disagree.
• Startup cost: every rule and route is split once and each constraint compiled once per check. At 1000 rules × 1000 routes: 210 ms / 385 MB / 4.0M allocs before, 20 ms / 1.3 MB / 10k allocs after (BenchmarkCheck).
• EnableRBAC called more than once installs one middleware per call, and every one enforces its own rules. The check reads all installed configs: a route is reported as uncovered (or partly covered) only when no config covers it in full, and a dead rule is reported for each config that holds it, as config N: METHOD path when there is more than one config (rbaccheck.CheckAll).
• What the check cannot see: requests answered by middleware before routing (a CORS preflight, for example).
• Docs: "Startup Route Check" in docs/advanced-guide/rbac/page.md (the three kinds, the modes, a /static example), GOFR_RBAC_ROUTE_CHECK in docs/references/configs/page.md, and the rbac.rule attribute plus an http.route upgrade note in the telemetry section.

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. The problem in #3935 is real, and moving http.route to the matched template is the right call.

I ran a small app against 167eebb4f with both the default router and GOFR_ROUTER=trie, benchmarked the check, and tried the fixes suggested below. Requesting changes for two blockers and one should-fix. Inline comments point to the exact lines.

🔴 1. The coverage report says "covered" for routes that are served with no role check

patternsMayOverlap feeds both reports. Leaning towards "yes" is safe for dead rules, because a live rule is never called dead. For uncovered routes it is the unsafe direction: a route is reported covered when the rule governs none of its requests.

Rule	Route	Startup check	curl, no role header
GET /api/users/ (trailing slash)	GET /api/users	nothing reported	200
GET files (no leading slash)	GET /files	nothing reported	200
GET /admin/{rest:.*}	GET /admin	nothing reported	200

A literal rule is compared byte for byte, and mux runs with StrictSlash(false). So a trailing /, a missing leading /, or a catch-all without its / match nothing at request time. These are exactly the typos this check is meant to catch. Two cases in Test_patternsMayOverlap currently pin this behaviour as intended.

What's needed: "covered" must mean the rule governs every request the route serves. "Dead" can keep the lenient check; coverage can't share it. As a data point, comparing paths without trimming /, and requiring a segment under a catch-all, flags all 3 routes above, and the rbac tests stay green once those 2 cases are flipped. An unconstrained {id} should also not match an empty segment; otherwise /api/users/ still looks live against /api/users/{id}. How you shape it is your call. A rule that covers only part of a route ({id:[0-9]+} vs {id}) is better reported on its own line than counted as covered.

🔴 2. Please keep CheckRoutes, ErrDeadRules and ErrUncoveredRoutes out of the public API

• A gofr app can't get its own *mux.Router, so users can't call CheckRoutes, and Run() already runs the check.
• The error sentinels exist only so run.go can pick a log line, and rbac already has the logger.
• Once exported, they can't be removed without breaking users.

gofr does need some way into rbac. One narrow framework-internal call, the way rbac.Middleware is used today, that takes plain route data rather than *mux.Router and does its own logging would be enough.

🟡 3. Startup cost: please split each path once

Both passes re-split both paths for every rule–route pair, and variableMayMatch recompiles its regex on every call. Measured on this head:

Routes = rules	This PR	Split once, same result
50	0.75 ms · 1 MB	0.08 ms · 26 KB
200	11.7 ms · 15 MB	0.9 ms · 106 KB
1000	264 ms · 384 MB · 4.0M allocs	21 ms · 0.5 MB · 8k allocs

It is one-off garbage rather than retained memory (the heap peaks around 19 MiB), but it happens on every boot of every pod, and the fix is small. Caching compiled constraints covers configs with many {x:regex} segments.

💡 Suggestion: let the user choose what a mismatch means

The only outcome today is an ERROR log, and the result can't reach user code: Run() returns nothing, and EnableRBAC runs before the routes exist. A mode would give users the choice:

Mode	Behaviour
warn (default)	today's behaviour, so upgrading breaks nothing
fail	refuse to start, through the existing startupFailed → abortStartup path
off	skip the check

Where it lives (rbac.json or an env var) is up to you. Longer term this could become a general startup-validation step: features register checks when enabled, and Run() evaluates them once the route table is final, the way readinessChecks and mcpConfigErr already work. That's a direction for maintainers to decide, not something this PR needs to build.

🟢 Smaller points

• Docs: /static/{path:.*} doesn't cover /static itself. AddStaticFiles registers that as a separate exact route (pkg/gofr/http/router.go:520), so the example needs a second rule.
• Static at /: a static directory served at / is skipped as the catch-all and never checked.
• Router walk: collectRoutes is a fourth walk of the router, after gofr.go:260 and tools.go:60/:138. Worth sharing one.
• Description: please say that the lenient matching applies to the coverage report too, and list what the check can't see: slash variants, a catch-all against its bare prefix, static at /, partially covered routes.
• Upgrade note: existing RBAC apps that rely on "no rule = open", including /graphql and /static, will log ERRORs on every boot with no config change. Worth a release-note line.
• Trace change (http.route → matched template, new rbac.rule): looks right, and TestMiddleware_TraceRouteLabels covers both mux and trie.

[aryanmehrotra]

Trimming makes /api/users/ and files look like /api/users. At request time, though, a literal rule is compared byte for byte and mux runs with StrictSlash(false), so neither ever matches. In my run both routes answered 200 with no role header while the check reported them covered. See 🔴 1 in the review body.

[akshat-kumar-singhal]

Fixed. Paths are no longer trimmed: they are split on / with empty segments kept, so /api/users/ is ["", "api", "users", ""] and files is ["files"], and neither overlaps /api/users or /files. Both are now reported: the rules as dead, the routes as uncovered. Pinned in Test_routeEntry_covers ("trailing slash in the rule", "relative rule path") and in TestConfig_ReportRouteMismatches ("a trailing slash makes a different path", "a relative path matches nothing").

[aryanmehrotra]

/admin/{rest:.*} still needs its /: mux matches /admin/ but not /admin. GET /admin answered 200 with no role header and was reported covered.

[akshat-kumar-singhal]

Fixed. A catch-all now needs a segment at its position on the other side, so /admin/{rest:.*} overlaps /admin/ and /admin/x but not /admin. GET /admin is reported uncovered. Case "catch-all does not cover the path it hangs off" in Test_routeEntry_covers, and "catch-all in the rule needs its slash" in Test_routeEntry_overlaps.

[aryanmehrotra]

Both loops share one lenient check. That's right for dead; for uncovered it needs to mean "this rule governs every request of this route". Both loops also re-split every rule–route pair (🟡 3): 4.0M allocations at 1000×1000, versus 8k when each path is split once.

[akshat-kumar-singhal]

Split into two. overlaps stays lenient and decides only whether a rule is dead. covers is strict: a rule covers a route only if it matches every request of the route. A literal covers only the same literal; {x} covers a non-empty literal or a variable that can't be empty; {x:re} covers a literal it matches or the identical constraint; a catch-all covers the rest only if the route has a segment there. A route that some rule overlaps but none covers gets its own error line, "routes only partly covered: GET /api/users/{id} (by GET /api/users/{id:[0-9]+})". The rule × route loop is now one pass, and each path is split once up front. 1000 × 1000: 22 ms / 1.2 MB / 8k allocs, was 210 ms / 385 MB / 4.0M. Benchmark: BenchmarkConfig_ReportRouteMismatches.

[aryanmehrotra]

This recompiles the constraint for every pair it's called on. Caching the compiled regex per constraint string would avoid it.

[akshat-kumar-singhal]

Done. Constraints are compiled once per distinct string, when the rule or route is parsed, and cached for the life of the check.

[aryanmehrotra]

Please keep this, and ErrDeadRules / ErrUncoveredRoutes (lines 16 and 21), out of the public API (🔴 2). A gofr app can't reach its own *mux.Router, and Run() already runs the check. Once exported, they can't be removed without breaking users.

[akshat-kumar-singhal]

Removed all three. The only new export is (*rbac.Config).ReportRouteMismatches(routes []string) bool, because gofr has to call across the package boundary. It takes plain "METHOD /template" strings, logs its own lines, and returns whether it logged an error. No *mux.Router and no sentinels, so the router can change without touching it. run.go's splitJoined is gone with them.

[aryanmehrotra]

This case and catch-all in the rule absorbs nothing (line 469) pin the fail-open behaviour; both should be false. Maybe also add "/api/{path:.*}" vs "/api/" → true and "api/users" vs "/api/users" → false.

[akshat-kumar-singhal]

Both flipped to false, and your two cases added (/api/{path:.*} vs /api/ → true, api/users vs /api/users → false), plus {id} vs an empty trailing segment → false. The table moved to coverage_test.go as Test_routeEntry_overlaps, next to the code it tests.

[aryanmehrotra]

This rule doesn't cover /static itself: AddStaticFiles registers it as a separate exact route (pkg/gofr/http/router.go:520). The example needs a {"path": "/static", ...} rule too.

[akshat-kumar-singhal]

Right, and the check now says so too: /static/{path:.*} doesn't overlap the exact /static route. The example now has both rules, and the docs explain that a catch-all needs its /.

[akshat-kumar-singhal]

Thanks for the careful repro. Your three routes are now reported under both GOFR_ROUTER=mux and trie. Replies are inline. On the rest:

• warn / fail / off: added as GOFR_RBAC_ROUTE_CHECK. warn is the default, so nothing changes on upgrade. fail releases what startup opened and exits 1 through the same path as a failed OnStart hook. off skips the check, for apps that rely on "no rule = open" on purpose. An unknown value is logged and treated as warn. Documented in the RBAC guide and the configs reference.
• Shared router walk: the route list is now built inside the walk httpServerSetup already makes, so rbac no longer walks the router. I left the two walks in tools.go (List and route) alone: they run on each MCP list/call against the live router, not at startup, so there's no startup walk to merge them into.
• Static at /: PathPrefix("/") is now skipped only when its handler is GoFr's own catch-all. A static directory served at / shows up as * /{path:.*} and is checked.
• PR description: rewritten. The lenient matching now applies to dead rules only, and there's an upgrade note for RBAC apps that rely on "no rule = open", including /graphql and /static.

[aryanmehrotra]

Thanks for the quick turnaround. I re-ran everything against 06e25ceb4, with the default router and with GOFR_ROUTER=trie.

✅ Round 1 is closed

Round-1 item	At 06e25ceb4
Trailing slash, relative path, catch-all against its bare prefix	All three reported, as dead rules and as uncovered routes
CheckRoutes and the error sentinels in the public API	Gone; ReportRouteMismatches takes plain strings
Startup cost	Each path split once, constraints compiled once
warn / fail / off	fail logs the mismatches and exits 1 before any server starts
Docs, static at /, shared router walk, upgrade note	Done

The split into a lenient overlaps and a strict covers is the right shape, and the tests bite: making covers fall back to overlaps fails 12 cases.

🔴 covers still answers "covered" for four shapes it does not model

Same class as round 1, in the new strict half. Each route below was reported covered — the startup log named only my control route — and each answered 200 with no role header, under both routers.

Rule	Route	Unguarded request	Why covers says yes
/api/{rest:.*}/admin	/api/users	GET /api/users	segmentsCover returns at the catch-all and never looks at the rule's segments after it
/items/{a}-{b}	/items/{id}	GET /items/abc	isVariableSegment only checks the first and last byte, so {a}-{b} is read as one free variable
/files/{x}	/files/{p:[^.]+}	GET /files/a/b	A free variable is taken to cover any constraint that rejects ""; this one also matches across /
/orders/{id: [0-9]+}	/orders/{id:[0-9]+}	GET /orders/42	The constraint is trimmed before comparing; mux keeps the space (regexp.go:77-81 at the pinned v1.8.1), so the rule matches nothing

The common cause is that covers has several arms that default to "yes". For this report the safe default is the other one: a rule covers a route only when the code can show it, and anything it cannot show is reported as partly covered. That matters most in fail mode, where a clean start will be read as a guarantee.

As a data point: I tried a small change for each — a catch-all covers only as the rule's last segment; a variable segment is exactly one {...}; no trimming; a free variable covers a constrained one only when the constraint cannot admit / (a short regexp/syntax walk). All four routes are then reported as partly covered, and the existing rbac tests stay green without edits. How you shape it is your call.

Test_routeEntry_covers has no case for any of the four; one each would pin them.

🟢 Smaller points

• Built-in exemption: isBuiltInRoute exempts by path prefix, so a route the application itself registers under /.well-known/ is never checked. Exempting the routes GoFr adds, rather than the prefix, would close that.
• Where the check runs: it sits after the OnStart hooks and the MCP bind, so in fail mode a config-only mistake is found after the datasources are up and the hooks have run. If a hook cannot add a route, the check could run first. Not blocking.
• Log order in fail mode: Registered HTTP server on port is now logged just before startup is abandoned.

[aryanmehrotra]

Returning here ignores the rule's segments after the catch-all. /api/{rest:.*}/admin is reported as covering /api/users, which answered 200 with no role header.

[akshat-kumar-singhal]

Fixed. A catch-all now covers only as the rule's last segment, so /api/{rest:.*}/admin covers nothing and GET /api/users is reported as partly covered. Case "catch-all covers only as the rule's last segment" in Test_routeEntry_covers.

[aryanmehrotra]

{a}-{b} passes this check and is then read as one free variable, so /items/{a}-{b} is reported as covering /items/{id}. GET /items/abc answered 200 with no role header.

[akshat-kumar-singhal]

Fixed. A segment is one variable only when it is exactly one {...}. Any other segment holding variables is read as the regexp mux builds for it ([^/]+\-[^/]+ here), so /items/{a}-{b} no longer covers /items/{id}. It still covers /items/{x}-{y} and /items/x-y, and a free {id} covers it. Cases in both Test_routeEntry_covers and Test_routeEntry_overlaps.

[aryanmehrotra]

[^.]+ rejects "" but matches across /. /files/{x} is reported as covering /files/{p:[^.]+}, and GET /files/a/b answered 200 with no role header.

[akshat-kumar-singhal]

Fixed. A variable is now treated as spanning segments when its pattern names / anywhere: a literal, a character class containing it, or .. That's a short regexp/syntax walk, cached per constraint like the compiled regexp. It can over-report (a / the pattern can never actually produce), which only makes covers stricter and overlaps more lenient. So /files/{x} no longer covers /files/{p:[^.]+}, and /files/{p:[^.]+} as a rule covers /files/a/b.

[aryanmehrotra]

mux does not trim the constraint (regexp.go:77-81 at v1.8.1), so {id: [0-9]+} needs a leading space and matches nothing. Trimming makes it compare equal to {id:[0-9]+}; GET /orders/42 answered 200 with no role header.

[akshat-kumar-singhal]

Fixed. Constraints are no longer trimmed, so {id: [0-9]+} neither covers {id:[0-9]+} nor overlaps /orders/42, and is reported as a dead rule. The docs now say constraints are compared as written.

[akshat-kumar-singhal]

Thanks for another careful pass. All four 🔴 shapes are fixed; replies are inline. Each has a case that fails on 06e25ce. The covers arms that defaulted to "yes" now model the segment the way mux does, and anything they can't show is reported as partly covered. Startup cost stays linear: about 10k allocations at 1000 rules × 1000 routes.

On the 🟢 points:

• Built-in exemption: done. GoFr's own routes are now told apart by where they were registered, not by path. httpServerSetup counts routes on either side of the defaults (health, alive, favicon, OpenAPI/Swagger, GraphQL UI), and those go to ReportRouteMismatches as a separate builtInRoutes argument. They still keep a rule written for them live. An application route under /.well-known/ is now checked; there's a case for it in both TestConfig_ReportRouteMismatches and TestApp_prepareHTTPServer.
• Where the check runs: partly. A hook can add a route, because it's a closure: app.OnStart(func(*gofr.Context) error { app.GET(...); return nil }) registers it before httpServerSetup walks the router. Running the check before the hooks would report that route's rule as dead, and with fail it would refuse an app that has nothing wrong with it. So the check stays after the hooks, but now runs before the MCP bind (hooks → route check → MCP). A refused start no longer claims the MCP port. The docs say when it runs and what has already happened by then.
• Log order: fixed. Registered HTTP server on port is now logged in startHTTPServer, so a refused start doesn't announce a server. Covered by TestRun_RBACRouteCheckFailStopsBeforeServers.

[aryanmehrotra]

The check itself works. I ran a sample app in all five modes and the three lines named the right rules and routes. My concern is the shape of this exported method, because we will have to keep it.

func (c *Config) ReportRouteMismatches(routes, builtInRoutes []string) bool

1. The inputs are formatted strings. rbacRouteCollector.add builds "METHOD /template", and parseEntry splits it again. A typed value removes the format contract from the doc comment.
2. Two []string parameters in a row. A caller can swap routes and builtInRoutes and the code still compiles.
3. The method computes, logs, and returns. The caller cannot get the mismatches without reading log lines. The tests need a Warnf on the mock logger for this reason.
4. The bool drops the detail. The caller cannot tell a dead rule from an uncovered route.
5. A nil Logger hides the result. logRouteMismatches returns early at line 89, but the method still returns true. In fail mode the app then exits with no line that names the mismatch.

Suggested shape:

type Route struct {
    Method, Path string
    BuiltIn      bool
}

type RouteMismatches struct {
    DeadRules, DeadPublicRules, Uncovered, PartlyCovered []string
}

func (c *Config) CheckRoutes(routes []Route) RouteMismatches

prepareHTTPServer then logs the lines and applies warn or fail. The matching logic in coverage.go does not change.

Also, the PR description shows this method with one parameter, and says the check runs after the MCP bind. The code has two parameters, and prepareHTTPServer runs before bindMCPServer. Please update the description.

[aryanmehrotra]

One more point on this method: I do not think it needs to be exported at all.

• prepareHTTPServer in pkg/gofr/run.go is the only caller. The method is exported only because the caller is in a different package.
• The check reads the rule method, the rule path, the public flag, and the logger. All four are available from exported fields: Config.Endpoints (Path, Methods, Public) and Config.Logger. The unexported c.rules is built from c.Endpoints in buildEndpointRules.
• A user enables RBAC with app.EnableRBAC() and Run does the check. A user also has no easy way to build the "METHOD /template" list this method needs.

Suggestion: move the check to an internal package, for example pkg/gofr/internal/rbaccheck, and call it from prepareHTTPServer. pkg/gofr can import it, and users cannot. The typed input and the returned result from my comment above still apply, but they then live in the internal package and add nothing to the public API of rbac.

One cost: the check must repeat two rules from buildEndpointRules (an endpoint with no methods becomes *, and method names become upper case). Please add a test that fails if the check and the matcher disagree on these.

[akshat-kumar-singhal]

Done, both comments. The check is no longer part of the public API.

• Moved to pkg/gofr/internal/rbaccheck. Config.ReportRouteMismatches is gone, and rbac/coverage.go with it. The matching logic moved unchanged.
• Typed input and result, as suggested. Check(endpoints []rbac.EndpointMapping, routes []Route) Mismatches, with Route{Method, Path, BuiltIn} and Mismatches{DeadRules, DeadPublicRules, Uncovered, PartlyCovered}. Built-in routes are a field instead of a second slice, so the two lists can no longer be swapped. Check doesn't log. Mismatches.Failed() reports whether anything other than a dead public rule was found.
• Logging moved to pkg/gofr. checkRBACRoutes writes the lines and prepareHTTPServer applies warn/fail. It logs through a.Logger(), the logger every other startup line goes to, and the stop decision reads the returned struct rather than whether a line was logged. So fail can no longer stop startup without a line naming the mismatch. The tests compare the returned struct instead of log lines, and the mock logger's Warnf is back to what development has. Two TestApp_prepareHTTPServer cases now cover the lines that moved: a dead public rule is a warning and doesn't stop startup under fail, and a partly covered route gets its own error line.
• Repeated matcher rules, and the test. buildRules repeats the two rules you named (no methods means *, method names are upper-cased) and a third: a (method, path) declared twice replaces the earlier declaration, as storeEndpointMapping does. Without that, a public rule followed by a protected one for the same dead path would show up as both a dead public rule and a dead rule. TestCheck_AgreesWithMiddleware loads a config through rbac.LoadPermissions with all three shapes, checks what Check reports, and sends the same requests through rbac.Middleware to confirm each one is guarded. I broke each of the three rules in buildRules in turn, and the test failed every time. splitPatternSegments is copied too, since the resolver's version is unexported.
• PR description fixed: it now describes Check, and says the check runs after the OnStart hooks and before the MCP port is claimed. The benchmark figures are updated too: 20 ms / 1.3 MB / 10k allocs at 1000 × 1000.

[aryanmehrotra]

Two points on the http.route change. I agree with the direction: the matched route is the more useful value, and rbac.rule keeps the old one.

1. A real route at / is reported as <unmatched>.

matchedRouteLabel (line 35) treats the template / as the catch-all. An app route registered at / has the same template. I ran a scratch test with router.Add(http.MethodGet, "/", ok) and a rule for /:

Router	Request	http.route	rbac.rule
mux	GET /	<unmatched>	/
trie	GET /	<unmatched>	/
mux	GET /api/users/42	/api/users/{id}	/api/users/{id}

So the span says no route matched for a request that a registered route served. The check needs to tell the catch-all apart by something other than its template. TestMiddleware_TraceRouteLabels has no case for a route at /; please add one for each router.

2. This changes the value of an existing attribute, so it needs an upgrade note.

The value changes in two cases:

• The rule is broader or narrower than the route. Example from your test: the value was /api/{path:.*} and is now /api/users/{id}.
• No rule matches the request. The value was <unmatched> and is now the route template.

A dashboard or an alert that groups the rbac.authorize span by http.route gets different series after the upgrade, and nothing fails. The PR description lists this under breaking changes, but the docs change only describes the new meaning. Please add an upgrade note in docs/advanced-guide/rbac/page.md that says the old value is now in rbac.rule, and make sure it is in the release notes.

[akshat-kumar-singhal]

1. Route at /. Thanks for the scratch test. Inside the middleware there is nothing to tell the two apart by. GoFr's catch-all is PathPrefix("/") and a route at / is Path("/"), so both have the template /. The trie router passes only that template string through the request context (serveMatched, router.go:245), so mux.CurrentRoute(r) is nil and the route itself isn't available to check.

The two ways to get a label right now each mislabel the other case:

Request	<unmatched> for / (this PR)	Template as-is
GET /nope, served by the catch-all	<unmatched> ✅	/ ❌
GET /, served by an app route at /	<unmatched> ❌	/ ✅

I kept <unmatched>, because a 404 never looks like a matched route that way. GoFr's HTTP span (tracer.go:54) already labels every 404 as /, and metricsPath (metrics.go:285) works around the same ambiguity by falling back to the raw path. All three need one fix, in the router, so I opened #4457 for it: the router records which route matched, and GoFr's catch-all is marked. Once that lands, this label, the HTTP span and the metrics path all use it, and I'll add the / case to TestMiddleware_TraceRouteLabels for each router. Until then, a test for / could only pin the wrong label. Authorization itself is unaffected: a request to a route at / is still checked against its rule, and only the span attribute is off.

2. Upgrade note. Added to docs/advanced-guide/rbac/page.md under Telemetry Data Protection. It says the old value is now in rbac.rule, lists the two cases where http.route changes (with the /api/{path:.*} → /api/users/{id} example), tells readers to move dashboards and alerts to rbac.rule to keep grouping by rule, and notes the / gap above. The PR description keeps it under breaking changes so it reaches the release notes.

[aryanmehrotra]

Thank you for b80630946. It resolves most of my comments:

• The check is now in pkg/gofr/internal/rbaccheck, so rbac has no new exported API.
• Check takes typed routes and returns Mismatches, and the caller does the logging.
• TestCheck_AgreesWithMiddleware covers drift between the check and the matcher.
• The docs have an upgrade note for http.route, and the description matches the code.

The tests for pkg/gofr/internal/rbaccheck, pkg/gofr/rbac, and the RBAC tests of pkg/gofr pass for me at this commit.

One point is still open:

A real route at / is reported as http.route = <unmatched> on the rbac.authorize span, on both routers (pkg/gofr/rbac/middleware.go:35). The docs now describe this as a known limit. I prefer a fix in this PR, because the span says that no route matched for a request that a registered route served. The check needs to tell the catch-all apart by something other than its template. Please also add a test case for a route at / for each router.

[akshat-kumar-singhal]

Thank you for the re-review. Fixed in 8843c8f: a real route at / is now reported as http.route = / on the rbac.authorize span, and <unmatched> is left for GoFr's catch-all only.

The catch-all and a / route share the template, but not the route shape: the catch-all is registered with PathPrefix("/"), a / route with an exact path. The new gofrHTTP.IsPrefixRoute(r) reads that from the matched route's compiled regexp, using the trie's existing isAnchoredBothEnds. To make the route available under the trie router, the trie now stores the matched *mux.Route in the request context instead of its template string. RouteTemplate reads the template from it, so its result is unchanged. A pointer fits in the interface while a string had to be boxed, so this also removes one allocation per request; TestTrieRequestAllocationsDoNotRegress now pins 6 instead of 7.

Tests: TestMiddleware_TraceRouteLabels has a route at / under both mux and trie (both failed before the fix), and TestIsPrefixRoute covers /, the catch-all and a parameter route under each router. The known-gap paragraph is gone from the docs.

The HTTP span's own http.route still reads / for a catch-all request; that is unchanged here, and IsPrefixRoute would make it a small follow-up if you want one.

[aryanmehrotra]

Thank you for 8843c8fd. A real route at / is now labelled / on both routers, and the new test fails if I remove the check.

I re-ran everything at this commit: the tests for rbaccheck, rbac and http pass with -race, and a sample app gave the right lines in warn, fail, off, with an invalid value and with the key unset, on both routers. I also compared the check with the real matcher on 76,176 rule and route pairs. No live rule was reported as dead.

Four points on IsPrefixRoute. I think they have one fix.

1. It is a new public function. gofrHTTP.IsPrefixRoute is exported from pkg/gofr/http (route_context.go:59), and the description says there is no new exported API. Please keep it out of the public API, as we did for CheckRoutes.

2. It parses the route regexp on every call. It runs for every 404 and every request to / when RBAC is on. Measured at this commit:

	ns/op	B/op	allocs/op
IsPrefixRoute, one call	325–438	504	7
A lookup of a route marked at setup	17	0	0

Whole 404 request with RBAC on	This PR	With the lookup
mux	23 allocs, 2.60 KiB	16 allocs, 2.11 KiB
trie	21 allocs, 2.24 KiB	14 allocs, 1.75 KiB

Matched routes are not affected (20 and 19 allocs both ways).

3. A static directory at / is still reported as <unmatched>. It is also a PathPrefix("/") route. With AddStaticFiles("/", dir) and a rule for /{path:.*}:

Router	Request	Response	http.route	rbac.rule
mux	GET /a.txt	200, file served	<unmatched>	/{path:.*}
trie	GET /a.txt	200, file served	<unmatched>	/{path:.*}

The question is "did GoFr's catch-all answer this request", not "is this a prefix route".

4. The same question is answered in two ways. rbacRouteCollector.add uses a $ suffix on the regexp and the handler type. IsPrefixRoute uses a syntax parse. Please choose one and use it in both places.

Suggestion: mark GoFr's catch-all once, where httpServerSetup registers it, and let both the span label and the collector ask "is this the marked route". If the marker and the lookup live in an internal package that http and rbac both import, nothing new is exported. How you shape it is your call. Please add a test case for a static directory at / for each router.

Smaller points

• Description: it still lists the / → <unmatched> known gap and #4457, and it says there is no new exported API. Please update it.
• EnableRBAC called twice: both middlewares are installed, but only the last config is checked. In my run the check reported GET /a as served without role checks while the request returned 401, and a dead rule in the first config was not reported. With fail this would stop an app that is guarded correctly.

[akshat-kumar-singhal]

Thanks, all four IsPrefixRoute points are fixed in e4fc649 with the single mark you suggested.

gofrHTTP.IsPrefixRoute is gone, so nothing new is exported. httpServerSetup now registers the catch-all through routemark.MarkCatchAll (internal package pkg/gofr/internal/routemark), which names the route. The rbac.authorize span label and rbacRouteCollector.add both ask routemark.CatchAllAnswered / IsCatchAll, so the $-suffix plus handler-type test for the catch-all is gone. The $ suffix is still used in the collector, but only to tell a prefix route from an exact one (to add the /{path:.*} tail), which is a different question.

Only the 404 catch-all is marked. A static directory at / is a real route that serves files, so it keeps its template: http.route is / and rbac.rule is /{path:.*}. The new cases for AddStaticFiles("/", dir) on mux and trie are in TestMiddleware_TraceRouteLabels and TestCatchAllAnswered, and I checked they fail if the mark is removed or if every route at / counts as the catch-all.

The lookup is a string compare on the route name: 8 ns, 0 allocs. A whole 404 with RBAC on (BenchmarkMiddleware_Unmatched404, same shape as your table): mux 21 -> 14 allocs, 2473 -> 1968 B; trie 19 -> 12 allocs, 2105 -> 1600 B. My "before" used the old regexp parse, so it is a copy of the old logic, not the exact old code.

The description is updated: the / known gap and #4457 are removed, and the marker is described.

On EnableRBAC called twice: done in ad286b7. The app now keeps every config passed to EnableRBAC, and the check reads all of them.
A route is reported as uncovered only when no installed config has a rule covering it, and as partly covered only when no config covers it in full. A route that one config guards is not reported just because another config does not mention it.
A dead rule is reported for each config that holds it, prefixed with the config number (config 1: GET /amdin/{id}) once there is more than one config. With a single config the output is unchanged.
Tests: TestCheckAll in rbaccheck, and two cases in TestApp_prepareHTTPServer (config A guards /admin and has a typo rule, config B does not mention /admin: /admin is not reported and the typo is; neither config guards /admin: it is reported). Checking only the last config makes the first case fail.

[aryanmehrotra]

Thank you for e4fc649c and ad286b71. Both close my last round. I ran a sample app at ad286b71 with the default router and with GOFR_ROUTER=trie, and read the rbac.authorize span from a Zipkin sink.

✅ Verified live

Scenario	mux	trie	Result
Route at /, every route covered, fail	✅	✅	Starts. GET / → http.route="/". A 404 → <unmatched>.
AddStaticFiles("/", dir)	✅	✅	GET /a.txt → 200, http.route="/", rbac.rule="/{path:.*}". 401 with no role.
Dead rule /ghost	✅	✅	http.route="<unmatched>", rbac.rule="/ghost", dead-rule line.
Two configs, B does not mention /admin	✅	✅	/admin not reported. Request with no role → 401.
Two configs, typo in config 1	✅		config 1: GET /amdin
Two configs, neither guards /admin	✅		Reported as uncovered. Request with no role → 200.
fail / FAIL / off / unset / invalid value	✅	✅	fail exits 1. off is silent. Invalid value warns and acts as warn.
14 malformed requests (//admin, ..%2f, %00, /./admin, bad role header)	✅	✅	401, 403 or 404 only. No 500, no panic.

The rbaccheck, routemark, rbac and http tests pass with -race.

Two points before merge

1. No test covers the real catch-all mark. I removed routemark.MarkCatchAll(...) from gofr.go:264 and ran pkg/gofr three times. Nothing failed. The tests in routemark and http mark their own catch-all, so only the wiring in httpServerSetup marks the real one. Without it, every RBAC app logs:

RBAC route check failed: routes only partly covered: * /{path:.*} (by GET /ping).

With fail, that app does not start. A 404 is also labelled / again. Please add a notLog: "/{path:.*}" case to TestApp_prepareHTTPServer, with a config that covers every route.

2. A static directory at / locks out the health probes. The check asks for a rule that covers /{path:.*} for every method. That rule also matches /.well-known/alive and /.well-known/health:

Request, no role	Response
GET /.well-known/alive	401
GET /a.txt	401

A Kubernetes liveness probe then fails. The check does not report it, because built-in routes are exempt. This adds a public rule, and I tested it:

{"path": "/.well-known/{path:.*}", "methods": ["GET"], "public": true}

With it, both probes return 200 and /a.txt still needs a role. Please add it to the static example in docs/advanced-guide/rbac/page.md.

Smaller points (not blocking)

• commonGaps reads its own output back with strings.Cut(p, " (by ") (check.go:88). A field in Mismatches is safer.
• With more than one config, a partly-covered line names only the first config's rules, and gives no config number.
• The docs do not describe the config N: prefix.
• The branch is behind development.

With 1 and 2 done, this is ready from my side.

[akshat-kumar-singhal]

Thanks for running it live on both routers. Both points are done in ebfaaca5, and the branch is merged up with development (5c484cbf).

1. Catch-all mark. Added the catch-all GoFr registers is not checked as a route to TestApp_prepareHTTPServer: a config that covers every route, notLog: "/{path:.*}". With the mark removed it fails, and with the mark in place it passes.

One thing I could not reproduce: when I remove only the mark (routemark.MarkCatchAll(a.httpServer.router.PathPrefix("/")) → a.httpServer.router.PathPrefix("/")), four existing cases already go red on both mux and GOFR_ROUTER=trie, for example rules that all match a route log nothing and fail keeps starting when nothing is wrong, because they assert no RBAC route check failed line. If I delete the whole catch-all registration instead, everything passes, but then there is no catch-all left to report. Was your run closer to the second? Either way, the new case names the intent directly.

2. Health probes with a static directory at /. The docs now have a paragraph after the static example: the /{path:.*} rule also matches /.well-known/alive and /.well-known/health, the check stays silent because those routes are exempt, and the probes get a 401. It shows your public rule next to the root rule. I also noted that the order of the two rules does not matter, because the resolver picks the more specific rule (compareSpecificity in resolver.go).

Smaller points:

• The docs now describe the multi-config case and the config N: prefix.
• The branch is up to date with development.
• commonGaps reading " (by " back, and the partly-covered line naming only the first config's rules without a config number: agreed on both. They change only how the line is built, so I'd rather do them in a follow-up than grow this PR again. Happy to fold them in here if you prefer.

[aryanmehrotra]

Can you resolve the merge conflicts

[akshat-kumar-singhal]

Merged development in (0472a5d). The conflict was with #4437 in rbac.go and rbac_test.go; both changes are kept. RBAC tests pass under -race and lint reports no new issues.

[aryanmehrotra]

Thank you for ebfaaca5 and the merge at 0472a5dc. Both points from my last round are closed.

Last round (ad286b71)                 Now (0472a5dc)
catch-all mark: no test         ──►   3 tests fail if the mark is removed ✅
root static dir: probes get 401 ──►   docs add the public /.well-known rule ✅
branch behind development       ──►   development merged, no conflict ✅

Verified at 0472a5dc

• The tests for rbaccheck, routemark, rbac and http, and the RBAC tests of pkg/gofr, pass with -race.
• I removed routemark.MarkCatchAll(...) at gofr.go:264. Three TestApp_prepareHTTPServer cases then fail: "rules that all match a route log nothing", "the catch-all GoFr registers is not checked as a route", and "fail keeps starting when nothing is wrong".
• The new docs say the more specific rule wins whatever the order. This is true: pkg/gofr/rbac/resolver.go sorts the rules most-specific-first when it loads them.
• The merge with fix(rbac): log an inheritsFrom role that matches no defined role #4437 keeps both changes in rbac.go and rbac_test.go.

I did not re-run the live mux and trie sample app. The code it ran against has not changed since ad286b71.

Not blocking

• check.go:159 reads its own output back with strings.Cut(p, " (by "). A field in Mismatches is safer.
• With more than one config, a partly-covered line still names no config number.

The approval covers 0472a5dc only.

[aryanmehrotra]

Approved at 0472a5dc. Details are in my comment above.

[akshat-kumar-singhal]

Thanks for the approval and for re-checking the catch-all mark.

On the two non-blocking points:

• check.go:159 reading its own output back. Agreed. PartlyCovered entries are formatted as "METHOD path (by …)", and the merge splits them again on " (by ". Keeping the route in its own field of Mismatches removes that coupling.
• Partly-covered line with more than one config. Agreed. Today it shows the rules from the first config that partly covers the route and does not say which config that is; it should name the config.

Neither changes what the check decides, so I would rather not push to this branch and reset the approval. I will do both in a small follow-up PR once this one merges. If you would prefer them here, say so and I will add them.