gofr-dev / gofr-dev/gofr

RBAC keeps a second copy of the route table; the router has already resolved the request by the time the middleware runs

Open
#3,935 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
20.9k
Forks
1.8k
Avg merge
5d 18h
Merged PRs (30d)
39

Description

### Summary

RBAC maintains a second copy of the route table and re-matches every request against it, even
though the router has already resolved the request by the time the RBAC middleware runs. The two
can silently disagree, and nothing checks them against each other.

This is the structural issue behind #3808 — that bug was one way the two matchers diverged; this is
the reason a divergence is possible at all.

### What is already shared

The matching *engine* is the same. Both the application router (`pkg/gofr/http/router.go:39`) and
RBAC (`pkg/gofr/rbac/config.go:168`) use gorilla/mux with `StrictSlash(false)`, and by the time the
RBAC middleware runs, `Router.ServeHTTP` has already normalised `r.URL.Path`, so both match the same
string with the same semantics.

What is duplicated is the **pattern strings**. `configs/rbac.json` is a hand-maintained second copy
of the route table, and a rule whose path does not correspond to any registered route — a typo like
`/api/user/{id}` against a registered `/api/users/{id}` — matches nothing, produces no error at load
or at request time, and leaves the route it was written to guard unguarded.

### The router's answer is already available and unused

GoFr registers middleware through `mux.Router.Use` (`pkg/gofr/http/router.go:155-161`), so
middleware runs *after* route matching. `mux.CurrentRoute(r).GetPathTemplate()` is therefore
available inside `rbac.Middleware` and returns the exact registered template. Verified against
GoFr's router shape (real routes plus the `PathPrefix("/")` catch-all from `gofr.go:183`):

```
GET /api/users/42 -> template="/api/users/{id:[0-9]+}" methods=[GET]
GET /api/users/abc -> template="/" methods=[] (catch-all)
DELETE /api/users/42 -> template="/" methods=[] (catch-all)
GET /.well-known/health -> template="/.well-known/health" methods=[GET]
```

Two things fall out of this:

1. The router's own resolution is free to read, and is authoritative by definition.
2. `template == "/"` with no methods means **no real route matched**, which cleanly separates
"registered route with no RBAC rule" (an unguarded route — dangerous) from "this path matches
nothing and is a 404 regardless" (harmless). That distinction is not currently available to the
middleware, and it is the signal the deferred fail-closed work needs.

### Proposed work, in increasing order of commitment

**1. Startup consistency check (non-breaking, highest value).** Walk the router's registered
templates and report, at boot:
- RBAC rules matching zero registered routes — a dead rule is an unguarded intent, and is exactly
the #3808 failure shape reached through a typo instead of through the matcher.
- Registered routes covered by no RBAC rule — informational, but it is the input an operator needs
before turning on any default-deny mode.

No mainstream auth middleware validates route coverage at boot (Envoy, Istio, Spring, Oathkeeper all
leave it to external linting), so this would be a genuine differentiator rather than table stakes.

**2. Use the matched route as the observability label.** `pkg/gofr/rbac/middleware.go:98` labels
spans and metrics with the *config's* path, so tracing reports RBAC's view of the request rather than
the router's. Using `CurrentRoute` here makes any disagreement between the two visible in traces
instead of invisible.

**3. Opt-in strict mode keyed on route templates (breaking, later).** Match RBAC rules against the
matched route template rather than the request path, making the two identical by construction.

Note this is **not** a free win and should not be the default: today a broad rule like
`/admin/{path:.*}` deliberately covers many distinct registered routes with one entry, and rules with
explicit methods work correctly today. Under template keying, every config path must equal a
registered template character for character, including the exact regex spelling of each constraint.
That trades "the two can silently disagree" for "the operator must mirror every route's constraint
syntax" — a real ergonomic cost, and a breaking change for configs that currently work.

### Relationship to other issues

- #3808 — the wildcard-method matcher bug; fixed by #3934. That PR consolidated RBAC's *two internal*
matchers into one, but the duplication with the *router* is untouched and is what this issue covers.
- #3763 — `EnableRBAC` fails open on an unusable config. Item 1 here is the natural place to surface
the diagnostics that issue wants at boot.
- The deferred fail-closed follow-up to #3808 depends on the "is this a real registered route?"
signal described above, so that design should not be finalised before this one is settled.

Contributor guide

Open the contributing guide

Research direction

Start by reading pkg/gofr/http/router.go:39,155-161, pkg/gofr/rbac/config.go:168, and pkg/gofr/rbac/middleware.go:98, then inspect configs/rbac.json and the router setup in gofr.go:183. Compare the router’s CurrentRoute result with RBAC’s configured paths and clarify which proposed scope is selected. Done means the chosen consistency, observability, or strict-mode behavior is specified with diagnostics and verification criteria.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
api, authorization, backend, observability
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.