gofr-dev / gofr-dev/gofr

RBAC: per-request pattern matching races and grows the router without bound; multi-role JWTs authorize nothing

Open
#3,979 0 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

Three defects in the RBAC middleware, all present on `development` and in released versions. None
were introduced by #3934 — they surfaced while reviewing it, and are filed together because the
first two share a root cause and a single fix.

## 1. Per-request pattern matching races, and grows the router without bound

`pkg/gofr/rbac/endpoint_matcher.go:91` builds a route per rule, per request, on the config's shared
router:

```go
route := router.NewRoute().Path(pattern)
```

gorilla/mux `NewRoute` appends to the router it is called on (`mux@v1.8.1/mux.go:279-284`):

```go
func (r *Router) NewRoute() *Route {
route := &Route{routeConf: copyRouteConf(r.routeConf), namedRoutes: r.namedRoutes}
r.routes = append(r.routes, route)
return route
}
```

`config.muxRouter` is created once at load and shared across every request, so that append is both
unsynchronised and permanent.

**Unbounded growth.** Walking the router after driving requests through `rbac.Middleware`:

```
0 routes at start -> 114 routes after 100 requests
```

It never shrinks. A long-lived pod accumulates routes for the whole life of the process.

**Data race.** 50 goroutines x 20 requests through the middleware, under `-race`:

```
WARNING: DATA RACE
mux.(*Router).NewRoute() mux.go:282
rbac.matchMuxPattern() endpoint_matcher.go:91
rbac.matchesKey() config.go:484
rbac.findEndpointByPattern() config.go:439
rbac.getEndpointForRequest() endpoint_matcher.go:210
```

The existing suites do not catch this because they drive requests sequentially; it only appears
under concurrency, which is the only way a server runs it.

**Reproduction:** any RBAC config containing a mux pattern (`{id}`, `{path:.*}`), with concurrent
requests to a path that reaches the pattern scan.

## 2. Authorizing one request costs hundreds to thousands of allocations

Resolving a single pattern path on `development`, `-benchmem`:

| rules in config | ns/op | allocs/op |
| --- | --- | --- |
| 6 | 18,165 | 533 |
| 21 | 58,471 | 1,648 |
| 51 | 127,909 | 3,734 |

This is the cost of recompiling mux patterns per rule per request. RBAC middleware runs on every
request, so this is on the hot path of every route in an application that enables it.

### Both of the above are fixed by the same change

#3935 proposes reading `mux.CurrentRoute(r).GetPathTemplate()` — the router has already resolved the
request by the time middleware runs, so its answer is free and authoritative. That removes
`matchMuxPattern` entirely, and with it the race, the growth and most of the allocations. This issue
is concrete evidence for prioritising that one.

There is also a smaller, independent allocation in the ordered resolver introduced by #3934, worth
folding in whenever this is touched:

```go
func (r *endpointRule) matchesMethod(methodUpper string) bool {
return matchesHTTPMethod(methodUpper, []string{r.method}) // slice per rule, per request
}
```

Comparing `r.method` directly, or storing the one-element slice on the rule at build time, removes
it.

## 3. Multi-role JWTs silently authorize nothing

`extractRoleFromJWT` (`pkg/gofr/rbac/middleware.go`) falls back to `fmt.Sprintf("%v", role)` when the
claim is not a string:

```
claim "admin" -> role="admin" matches
claim ["admin","viewer"] -> role="[admin viewer]" matches no configured role
claim ["admin"] -> role="[admin]" matches no configured role
```

An array is the default shape emitted by Keycloak, Auth0 and Entra ID, so an ordinary setup gets
`403` on every request, with no error logged and nothing in the config to explain why.

It fails closed, so this is not an authorization hole — but it makes JWT-based RBAC appear broken to
anyone whose identity provider emits the common shape.

**Decision needed:** should a role array mean "holds all of these" (union of the permissions of each
named role), or should it be rejected explicitly at load with a clear error? Either is better than
formatting it into a string that can never match.

## Suggested sequencing

1 and 2 land together via #3935. 3 is independent and needs a semantics decision first.

Contributor guide

Open the contributing guide

Research direction

Start with pkg/gofr/rbac/endpoint_matcher.go, config.go, and middleware.go, then run the concurrent reproduction under -race and the allocation benchmark described in the issue. Check the proposed #3935 approach for pattern matching, and decide and test whether JWT role arrays represent combined roles or are rejected with a clear load error. Done means the race, route growth, excess allocations, and silent multi-role failure are covered by tests.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
authorization, backend, performance
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.