diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index a5d1e4d67c..2126a4e9c6 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -460,6 +460,124 @@ request, so it is NOT enforced - any route it was meant to govern is currently u Treat that line as an open route, not a warning about a typo. +### Startup Route Check + +The RBAC config is a second copy of your route table, written by hand, so the two can drift apart. +When the app starts (inside `app.Run()`, after every route has been registered), GoFr compares +them and logs each kind of mismatch below as an **error**. By default the app keeps starting, so +read the startup logs: each line names a route that is not protected the way the config intends. +[`GOFR_RBAC_ROUTE_CHECK`](#choosing-what-a-mismatch-does) can make a mismatch stop startup instead. + +Paths are compared exactly as written, the same way requests are matched: `/api/users/` and +`/api/users` are different paths, and so are `files` and `/files`. + +**A dead rule** is a rule that matches no registered route. It is usually a typo — a rule for +`/api/user/{id}` when the route is `/api/users/{id}` — and it means the route it was written for +is **not protected**. A rule counts as dead when no request could be matched by both the rule and +a registered route. A rule whose method the route does not register (`DELETE` on a route that only +has `GET`) is dead too. A dead rule marked `"public": true` cannot leave a route unprotected, so it +is reported only as a warning. + +``` +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. +``` + +**An uncovered route** is a registered route, for a given method, that no rule matches, so it would +be served without role checks (see [Unmatched Routes Behavior](#unmatched-routes-behavior)). To +keep a route open on purpose, such as `/login` or a webhook, give it a rule with `"public": true`. +The routes GoFr adds itself — health, alive, `/favicon.ico`, and the OpenAPI, Swagger and GraphQL +Playground pages under `/.well-known/` — are exempt. A route your application registers under +`/.well-known/` is not, and needs a rule like any other. Other routes GoFr registers for you do +need a rule: the `./static` directory (served at `/static` and everything under `/static/` +when it exists) and `/graphql` when GraphQL is enabled. A catch-all needs the `/` in front of it: +a rule for `/admin/{rest:.*}` matches `/admin/users` but not `/admin`, so the route `/admin` needs +a rule of its own. + +``` +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. +``` + +**A partly covered route** is one that rules match for some of its requests but not for all of +them. The requests no rule matches are served without role checks. Common causes: + +- a constraint narrower than the route: a rule for `/api/users/{id:[0-9]+}` does not govern + `/api/users/abc`, which the route `/api/users/{id}` serves; +- a method list narrower than the route: a static directory answers every method, so a rule for + `["GET"]` leaves `HEAD /static/app.js` unchecked. Use `["*"]` for static files. + +``` +RBAC route check failed: routes only partly covered: * /static/{path:.*} (by GET /static/{path:.*}). +Requests these rules do not match are served without role checks; add a rule that matches each +route in full. +``` + +A rule set that opens the static directory and a login route: + +```json +{"path": "/login", "methods": ["POST"], "public": true}, +{"path": "/static", "methods": ["*"], "public": true}, +{"path": "/static/{path:.*}", "methods": ["*"], "public": true} +``` + +A static directory served at the root, with `app.AddStaticFiles("/", dir)`, is the route +`* /{path:.*}`, and the rule that covers it also matches GoFr's health routes, +`/.well-known/alive` and `/.well-known/health`. The check does not report this, because those +routes are exempt, but a request to them without a role gets a 401, so a liveness probe fails. Open +them with a public rule; the more specific rule wins, whatever order the two are written in: + +```json +{"path": "/.well-known/{path:.*}", "methods": ["GET"], "public": true}, +{"path": "/{path:.*}", "methods": ["*"], "requiredPermissions": ["files:read"]} +``` + +The dead-rule check is lenient on purpose: two path variables with different constraints — +`{id:[0-9]+}` in the rule and `{id:[a-z]+}` in the route — are treated as matching, so a rule like +that is not reported as dead. The coverage check is strict: a rule covers a route only when the +check can show that it matches every request the route serves, so the same pair is reported as +partly covered. A rule the check cannot read in full is reported as partly covering its route +rather than trusted, which includes: + +- a catch-all followed by more segments, such as `/api/{rest:.*}/admin`: it covers nothing, as the + check does not work out what the catch-all leaves for the segments after it; +- a constraint that can match `/`, such as `{p:[^.]+}`: like `{path:.*}`, it can span several + segments, so a single-segment `{x}` does not cover it; +- a segment holding more than one variable, such as `{a}-{b}`: it is read as the pattern mux builds + from it, not as one free variable. + +Constraints are compared as written, the way mux reads them: `{id: [0-9]+}`, with a space, only +matches IDs that start with a space, so it neither matches nor covers `{id:[0-9]+}`. + +The check sees the route table, not the middleware in front of it: a request answered before +routing (a CORS preflight, for example) never reaches RBAC either way. + +When `EnableRBAC` is called more than once, every config enforces its own rules on each request, so +the check looks at all of them. A route is reported as uncovered or partly covered only when no +config covers it in full. A dead rule is reported for each config that holds it, prefixed with the +config's number in the order `EnableRBAC` was called: + +``` +RBAC route check failed: rules match no registered route: config 1: GET /amdin/{id}. These rules +protect nothing; fix each rule's path or methods, or remove it. +``` + +#### Choosing what a mismatch does + +Set `GOFR_RBAC_ROUTE_CHECK` in your config: + +| Value | Behavior | +|---|---| +| `warn` (default) | Log each mismatch as an error and keep starting. | +| `fail` | Log each mismatch, release what startup opened, and exit with status 1. A dead public rule is only a warning and does not stop startup. | +| `off` | Skip the check. For apps that leave routes without a rule on purpose and rely on [Unmatched Routes Behavior](#unmatched-routes-behavior). | + +Any other value is logged as an error and treated as `warn`. + +The check runs after the [`OnStart` hooks](/docs/advanced-guide/startup-hooks), because a hook can +still register a route, and before the MCP port is claimed or any server starts. With `fail`, the +datasources are already connected and the hooks have run by the time a mismatch stops startup. + ## JWT-Based RBAC For production/public APIs, use JWT-based role extraction: @@ -799,10 +917,11 @@ In this configuration: - `GET /api/posts` → **Not in RBAC config** → Allowed to proceed (may return 404 if route doesn't exist) - `GET /health` → **Not in RBAC config** → Allowed to proceed (will work if route exists) -This design allows you to: -- Gradually add RBAC protection to specific endpoints -- Keep some routes unprotected (not in RBAC config) -- Let the router handle 404s for non-existent routes +At startup, the app in this example logs an error for `POST /api/users` and `GET /api/posts`: +they are registered but covered by no rule (see [Startup Route Check](#startup-route-check)). +Give every registered route a rule, and a route meant to be open one with `"public": true`. +The runtime behavior above still applies to a request no rule matches, for example one to a route +the check reported as only partly covered, or to any route when `GOFR_RBAC_ROUTE_CHECK=off`. ## Security and Privacy @@ -811,7 +930,11 @@ This design allows you to: RBAC middleware implements industry-standard security practices to protect sensitive data: **Traces (OpenTelemetry):** -- ✅ HTTP method and route patterns included +- ✅ HTTP method and route patterns included: `http.route` is the route the router matched, and + `rbac.rule` is the path of the RBAC rule that governed the request (`` when none did). + When the two disagree, the rule is broader or narrower than the route. `http.route` is + `` when no route matched, that is, when GoFr's catch-all answered the request. An + application route registered at `/` is reported as `/`. - ✅ Authorization status (allowed/denied) included - ❌ Roles excluded (privacy protection - roles are PII) - ❌ Error messages sanitized (prevent information leakage) @@ -835,6 +958,21 @@ RBAC middleware implements industry-standard security practices to protect sensi - ❌ No authorization tokens, headers, or request bodies logged - ❌ No user IDs or personal information logged +#### Upgrade note: `http.route` on the `rbac.authorize` span + +Earlier releases set `http.route` on the `rbac.authorize` span to the path of the RBAC rule that +governed the request, or `` when no rule did. It is now the route the router matched, the +same value as on the request's HTTP span, and the old value moved to the new `rbac.rule` attribute. + +The value of `http.route` changes in two cases: + +- **The rule is broader or narrower than the route.** A request to `GET /api/users/42` governed by + a rule for `/api/{path:.*}` was reported as `/api/{path:.*}` and is now `/api/users/{id}`. +- **No rule matches the request.** It was reported as `` and is now the route template. + +Nothing fails when the value changes, so check every dashboard, alert or query that groups the +`rbac.authorize` span by `http.route`: to keep grouping by rule, group by `rbac.rule` instead. + ### What's Never Logged RBAC middleware never logs: diff --git a/docs/references/configs/page.md b/docs/references/configs/page.md index f32e71f9e3..3fd1e9e7a0 100644 --- a/docs/references/configs/page.md +++ b/docs/references/configs/page.md @@ -205,6 +205,12 @@ This document lists all the configuration options supported by the GoFr framewor --- +- GOFR_RBAC_ROUTE_CHECK +- What the startup check does when the RBAC config and the registered routes disagree — see [Startup Route Check](/docs/advanced-guide/rbac#startup-route-check). `warn` logs each mismatch as an error and keeps starting, `fail` logs them and stops startup with exit status 1, `off` skips the check. Any other value is logged as an error and treated as `warn`. +- warn + +--- + - LOG_DISABLE_PROBES - Disable log probes for health checks - false diff --git a/pkg/gofr/gofr.go b/pkg/gofr/gofr.go index a587d43f34..849dc8caba 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -19,9 +19,12 @@ import ( "gofr.dev/pkg/gofr/config" "gofr.dev/pkg/gofr/container" gofrHTTP "gofr.dev/pkg/gofr/http" + "gofr.dev/pkg/gofr/internal/rbaccheck" + "gofr.dev/pkg/gofr/internal/routemark" "gofr.dev/pkg/gofr/logging" "gofr.dev/pkg/gofr/metrics" "gofr.dev/pkg/gofr/migration" + "gofr.dev/pkg/gofr/rbac" "gofr.dev/pkg/gofr/service" "gofr.dev/pkg/gofr/traces/exporters" ) @@ -80,6 +83,15 @@ type App struct { // same point it reports a port it could not claim. See bindMCPServer. mcpConfigErr error + // rbacConfigs holds every RBAC config installed by EnableRBAC, in order, kept so Run can check + // them against the complete route table. Each call installs a middleware that enforces its own + // config, so all of them are checked. See prepareHTTPServer. + rbacConfigs []*rbac.Config + + // rbacRoutes is the route table as the RBAC route check reads it. httpServerSetup fills it from + // the walk it already makes, and only when rbacConfigs is set. + rbacRoutes []rbaccheck.Route + // exit is os.Exit, indirected so a test can observe the status a failed startup or a failed // command reports without taking the test binary down with it. Nil means os.Exit, which is what // every real app uses. @@ -226,7 +238,11 @@ func (a *App) httpServerSetup() { } } + routes := newRBACRouteCollector(len(a.rbacConfigs) > 0) + // Register default routes - these are only added when HTTP server is actually starting + routes.startBuiltIns(&a.httpServer.router.Router) + a.add(http.MethodGet, service.HealthPath, a.healthHandler) a.add(http.MethodGet, service.AlivePath, liveHandler) a.add(http.MethodGet, "/favicon.ico", faviconHandler) @@ -239,6 +255,8 @@ func (a *App) httpServerSetup() { a.add(http.MethodGet, "/.well-known/graphql/ui", playgroundHandler) } + routes.endBuiltIns(&a.httpServer.router.Router) + for dirName, endpoint := range a.httpServer.staticFiles { a.httpServer.router.AddStaticFiles(a.Logger(), endpoint, dirName) } @@ -247,11 +265,9 @@ func (a *App) httpServerSetup() { a.logReadiness() - if a.container.Logger != nil { - a.container.Logger.Infof("Registered HTTP server on port: %d", a.httpServer.port) - } - - a.httpServer.router.PathPrefix("/").Handler(handler{ + // The one catch-all GoFr registers: marked so the RBAC trace label and route check can tell it + // from a route or static directory at "/", which share its template. + routemark.MarkCatchAll(a.httpServer.router.PathPrefix("/")).Handler(handler{ function: catchAllHandler, container: a.container, }) @@ -266,9 +282,13 @@ func (a *App) httpServerSetup() { } } + routes.add(route, met) + return nil }) + a.rbacRoutes = routes.routes + *a.httpServer.router.RegisteredRoutes = registeredMethods } diff --git a/pkg/gofr/http/alloc_guard_test.go b/pkg/gofr/http/alloc_guard_test.go index 3ef14898ce..a016678595 100644 --- a/pkg/gofr/http/alloc_guard_test.go +++ b/pkg/gofr/http/alloc_guard_test.go @@ -20,7 +20,7 @@ import ( // invisible from the outside. So a revert of any of them keeps the suite green // and gives the allocations back silently. This counts them instead. // -// The count it pins is 7, while the commit's table quotes 8 allocs/op. They are +// The count it pinned was 7 (now 6, see wantAllocs), while the commit's table quotes 8 allocs/op. They are // different fixtures, not an inconsistency: this guard drives five middlewares // and a nopWriter, the benchmark three and a real response writer. The guard's // job is to notice change, so it is measured where the signal is cleanest rather @@ -35,7 +35,10 @@ func TestTrieRequestAllocationsDoNotRegress(t *testing.T) { // AllocsPerRun is deterministic for fixed code, so the only thing that moves // this is a Go or dependency upgrade -- worth a human looking at. If it fails // after one, re-measure and update the constant in the same commit. - const wantAllocs = 7 + // + // 6 since the trie stores the matched *mux.Route in the request context instead + // of its template string: a pointer fits in the interface, a string is boxed. + const wantAllocs = 6 r := NewRouter() // Five middlewares, because that is what newHTTPServer installs and because the diff --git a/pkg/gofr/http/route_context.go b/pkg/gofr/http/route_context.go index fc3f869906..ff4d5a36c5 100644 --- a/pkg/gofr/http/route_context.go +++ b/pkg/gofr/http/route_context.go @@ -1,26 +1,18 @@ package http import ( - "context" "net/http" "github.com/gorilla/mux" -) - -// routeTemplateCtxKey is the private context key under which the trie router -// stores the matched route template (e.g. "/users/{id}"). It exists because, -// once the trie router bypasses mux's ServeHTTP, mux.CurrentRoute(r) is no -// longer populated, so the template must be carried some other way. -type routeTemplateCtxKey struct{} -// withRouteTemplate returns r carrying tmpl as the matched route template. An -// empty template is a no-op so unmatched requests do not pay for a context copy. -func withRouteTemplate(r *http.Request, tmpl string) *http.Request { - if tmpl == "" { - return r - } + "gofr.dev/pkg/gofr/internal/routemark" +) - return r.WithContext(context.WithValue(r.Context(), routeTemplateCtxKey{}, tmpl)) +// withRoute returns r carrying route as the matched route. It exists because, once the trie +// router bypasses mux's ServeHTTP, mux.CurrentRoute(r) is no longer populated, so the route must be +// carried some other way. A nil route is a no-op so unmatched requests do not pay for a context copy. +func withRoute(r *http.Request, route *mux.Route) *http.Request { + return routemark.With(r, route) } // RouteTemplate returns the matched route template for r (e.g. "/users/{id}"), @@ -31,11 +23,7 @@ func withRouteTemplate(r *http.Request, tmpl string) *http.Request { // via mux.CurrentRoute. Callers get consistent behavior without knowing which // router is active. func RouteTemplate(r *http.Request) string { - if tmpl, _ := r.Context().Value(routeTemplateCtxKey{}).(string); tmpl != "" { - return tmpl - } - - if route := mux.CurrentRoute(r); route != nil { + if route := routemark.Matched(r); route != nil { if tmpl, err := route.GetPathTemplate(); err == nil { return tmpl } diff --git a/pkg/gofr/http/route_context_test.go b/pkg/gofr/http/route_context_test.go index e187e832a3..40002bfca7 100644 --- a/pkg/gofr/http/route_context_test.go +++ b/pkg/gofr/http/route_context_test.go @@ -4,10 +4,16 @@ import ( "context" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" + "github.com/gorilla/mux" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "gofr.dev/pkg/gofr/internal/routemark" + "gofr.dev/pkg/gofr/logging" ) // TestRouteTemplate_ResolvesUnderBothRouters is the guard for the accessor the @@ -57,12 +63,73 @@ func TestRouteTemplate_EmptyWhenUnmatched(t *testing.T) { assert.Empty(t, RouteTemplate(req), "no template should be reported for an unrouted request") } -// TestWithRouteTemplate_EmptyIsNoOp verifies that recording an empty template -// does not copy the request — unmatched requests must not pay for a context -// allocation. -func TestWithRouteTemplate_EmptyIsNoOp(t *testing.T) { +// TestWithRoute_NilIsNoOp verifies that recording no route does not copy the +// request — unmatched requests must not pay for a context allocation. +func TestWithRoute_NilIsNoOp(t *testing.T) { req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/x", http.NoBody) - assert.Same(t, req, withRouteTemplate(req, ""), "empty template must not copy the request") - assert.NotSame(t, req, withRouteTemplate(req, "/x/{id}"), "a real template must be recorded") + assert.Same(t, req, withRoute(req, nil), "no route must not copy the request") + assert.NotSame(t, req, withRoute(req, mux.NewRouter().Path("/x/{id}")), "a real route must be recorded") +} + +// TestCatchAllAnswered tells GoFr's marked catch-all apart from a route or a static directory +// registered at "/": all of them have the template "/", under both routers. +func TestCatchAllAnswered(t *testing.T) { + ok := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) }) + + testCases := []struct { + desc string + useTrie bool + static bool + path string + want bool + }{ + {"mux: route at /", false, false, "/", false}, + {"trie: route at /", true, false, "/", false}, + {"mux: catch-all", false, false, "/nowhere", true}, + {"trie: catch-all", true, false, "/nowhere", true}, + {"mux: route with a parameter", false, false, "/users/42", false}, + {"trie: route with a parameter", true, false, "/users/42", false}, + {"mux: static directory at /", false, true, "/a.txt", false}, + {"trie: static directory at /", true, true, "/a.txt", false}, + } + + for i, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + var got bool + + r := NewRouter() + r.useTrie = tc.useTrie + r.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + got = routemark.CatchAllAnswered(req) + + next.ServeHTTP(w, req) + }) + }) + r.Add(http.MethodGet, "/", ok) + r.Add(http.MethodGet, "/users/{id}", ok) + + if tc.static { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a"), 0o600)) + r.AddStaticFiles(logging.NewMockLogger(logging.ERROR), "/", dir) + } + + routemark.MarkCatchAll(r.PathPrefix("/")).Handler(ok) + + req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, tc.path, http.NoBody) + rec := httptest.NewRecorder() + r.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, "TEST[%d], Failed.\n%s", i, tc.desc) + assert.Equal(t, tc.want, got, "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } + + t.Run("unrouted request", func(t *testing.T) { + req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/", http.NoBody) + + assert.False(t, routemark.CatchAllAnswered(req)) + }) } diff --git a/pkg/gofr/http/router.go b/pkg/gofr/http/router.go index 9143278858..fefaad766e 100644 --- a/pkg/gofr/http/router.go +++ b/pkg/gofr/http/router.go @@ -240,11 +240,7 @@ func (rou *Router) serveMatched(w http.ResponseWriter, r *http.Request, match *m r = mux.SetURLVars(r, match.Vars) } - if match.Route != nil { - if tmpl, err := match.Route.GetPathTemplate(); err == nil { - r = withRouteTemplate(r, tmpl) - } - } + r = withRoute(r, match.Route) // mux builds its middleware chain inside Match, guarded by MatchErr == nil, so a route that // matches while REPORTING an error is served WITHOUT the chain. A subrouter carrying its own diff --git a/pkg/gofr/internal/rbaccheck/check.go b/pkg/gofr/internal/rbaccheck/check.go new file mode 100644 index 0000000000..5f416265e3 --- /dev/null +++ b/pkg/gofr/internal/rbaccheck/check.go @@ -0,0 +1,267 @@ +// Package rbaccheck compares an RBAC config against the route table an app serves, so that a rule +// guarding nothing, or a route no rule guards, is found at startup rather than by a request served +// without role checks. GoFr runs it from App.Run; it is internal because an app has no route table +// of its own to hand it. +package rbaccheck + +import ( + "fmt" + "sort" + "strings" + + "gofr.dev/pkg/gofr/rbac" +) + +// Route is one route the router serves. +type Route struct { + // Method is the route's method, or "*" for a route registered without a method matcher, such as + // a static-file prefix. + Method string + + // Path is the route's template in mux syntax. A prefix route is given a trailing catch-all + // segment, as "/static/{path:.*}", so that it stands for every path under the prefix. + Path string + + // BuiltIn is set for a route GoFr registered itself. It needs no rule, but keeps a rule written + // for it live. + BuiltIn bool +} + +// Mismatches lists what Check found, each entry as "METHOD path", sorted. +type Mismatches struct { + // DeadRules match no route - usually a typo in the path, or a method the route does not + // register - so the route each was written for is unguarded. + DeadRules []string + + // DeadPublicRules are public rules that match no route. They cannot leave a route unguarded. + DeadPublicRules []string + + // Uncovered routes are matched by no rule, so they are served without role checks. + Uncovered []string + + // PartlyCovered routes are matched by rules for some of their requests but not all of them. + // Each entry names the rules, as "METHOD path (by METHOD rule and METHOD rule)". + PartlyCovered []string +} + +// Failed reports whether any mismatch can leave a request served without role checks: anything but +// a dead public rule. +func (m *Mismatches) Failed() bool { + return len(m.DeadRules)+len(m.Uncovered)+len(m.PartlyCovered) > 0 +} + +// rule is one (method, path) an endpoint declares. +type rule struct { + entry routeEntry + public bool +} + +// Check compares the RBAC endpoints against the complete route table. +// +// Paths are compared as written, the way requests are matched: "/api/users/" and "/api/users" are +// different paths. The dead-rule check is lenient - a rule is live if some request could match it +// and a route - so that a live rule is never reported as dead. The coverage check is strict: a +// rule covers a route only when the check can show that it matches every request the route +// serves, and a route it cannot show that for is reported as partly covered. +func Check(endpoints []rbac.EndpointMapping, routes []Route) Mismatches { + rc := newRouteChecker() + rules := buildRules(rc, endpoints) + + var ( + m Mismatches + live = make([]bool, len(rules)) + ) + + for i := range routes { + entry := rc.newEntry(strings.ToUpper(routes[i].Method), routes[i].Path) + + if routes[i].BuiltIn { + markLiveRules(rules, &entry, live) + + continue + } + + switch covering := classifyRoute(rules, &entry, live); { + case covering == nil: + m.Uncovered = append(m.Uncovered, entry.label) + case len(covering) > 0: + m.PartlyCovered = append(m.PartlyCovered, entry.label+" (by "+strings.Join(sortedUnique(covering), " and ")+")") + } + } + + for i := range rules { + switch { + case live[i]: + case rules[i].public: + m.DeadPublicRules = append(m.DeadPublicRules, rules[i].entry.label) + default: + m.DeadRules = append(m.DeadRules, rules[i].entry.label) + } + } + + m.DeadRules = sortedUnique(m.DeadRules) + m.DeadPublicRules = sortedUnique(m.DeadPublicRules) + m.Uncovered = sortedUnique(m.Uncovered) + m.PartlyCovered = sortedUnique(m.PartlyCovered) + + return m +} + +// CheckAll checks every config an app installed, in the order they were installed. Each installed +// config enforces its own rules on every request, so a route is served without role checks only +// when no config has a rule that covers it: a route is reported as uncovered when every config +// leaves it uncovered, and as partly covered when no config covers it in full. A dead rule is +// reported for each config that holds it, with the config's number once there is more than one. +func CheckAll(configs [][]rbac.EndpointMapping, routes []Route) Mismatches { + results := make([]Mismatches, len(configs)) + for i := range configs { + results[i] = Check(configs[i], routes) + } + + if len(results) == 1 { + return results[0] + } + + var m Mismatches + + for i := range results { + for _, d := range results[i].DeadRules { + m.DeadRules = append(m.DeadRules, fmt.Sprintf("config %d: %s", i+1, d)) + } + + for _, d := range results[i].DeadPublicRules { + m.DeadPublicRules = append(m.DeadPublicRules, fmt.Sprintf("config %d: %s", i+1, d)) + } + } + + m.Uncovered, m.PartlyCovered = commonGaps(results) + m.DeadRules = sortedUnique(m.DeadRules) + m.DeadPublicRules = sortedUnique(m.DeadPublicRules) + + return m +} + +// commonGaps returns the routes no config covers in full: uncovered when every config leaves them +// uncovered, partly covered otherwise. +func commonGaps(results []Mismatches) (uncovered, partlyCovered []string) { + // A route a config lists in neither Uncovered nor PartlyCovered is covered by it in full. + gaps := make(map[string]int) // route -> configs that do not cover it in full + partly := make(map[string]string) // route -> first PartlyCovered entry naming its rules + unc := make(map[string]int) + + for i := range results { + for _, u := range results[i].Uncovered { + gaps[u]++ + unc[u]++ + } + + for _, p := range results[i].PartlyCovered { + route, _, _ := strings.Cut(p, " (by ") + gaps[route]++ + + if _, seen := partly[route]; !seen { + partly[route] = p + } + } + } + + for route, n := range gaps { + switch { + case n < len(results): // some config covers it in full + case unc[route] == len(results): + uncovered = append(uncovered, route) + default: + partlyCovered = append(partlyCovered, partly[route]) + } + } + + return sortedUnique(uncovered), sortedUnique(partlyCovered) +} + +// buildRules reads endpoints the way the rbac matcher does: an endpoint with no methods stands for +// every method, a method is read in upper case, and a (method, path) declared again replaces the +// earlier declaration. TestCheck_AgreesWithMiddleware fails if the two drift apart. +func buildRules(rc *routeChecker, endpoints []rbac.EndpointMapping) []rule { + var ( + rules []rule + index = make(map[string]int) + ) + + for i := range endpoints { + methods := endpoints[i].Methods + if len(methods) == 0 { + methods = []string{anyMethod} + } + + for _, method := range methods { + r := rule{entry: rc.newEntry(strings.ToUpper(method), endpoints[i].Path), public: endpoints[i].Public} + + if at, seen := index[r.entry.label]; seen { + rules[at] = r + + continue + } + + index[r.entry.label] = len(rules) + rules = append(rules, r) + } + } + + return rules +} + +// classifyRoute marks every rule that overlaps route as live, and reports how route is covered: an +// empty non-nil slice when some rule covers it, the labels of the rules that overlap it when none +// does, and nil when no rule overlaps it. +func classifyRoute(rules []rule, route *routeEntry, live []bool) []string { + covered, overlapping := false, []string{} + + for i := range rules { + if !rules[i].entry.overlaps(route) { + continue + } + + live[i] = true + + switch { + case covered: + case rules[i].entry.covers(route): + covered = true + default: + overlapping = append(overlapping, rules[i].entry.label) + } + } + + switch { + case covered: + return []string{} + case len(overlapping) == 0: + return nil + default: + return overlapping + } +} + +// markLiveRules marks every rule that overlaps route as live. +func markLiveRules(rules []rule, route *routeEntry, live []bool) { + for i := range rules { + if !live[i] && rules[i].entry.overlaps(route) { + live[i] = true + } + } +} + +// sortedUnique sorts entries and drops repeats, in place. +func sortedUnique(entries []string) []string { + sort.Strings(entries) + + out := entries[:0] + + for i, entry := range entries { + if i == 0 || entry != entries[i-1] { + out = append(out, entry) + } + } + + return out +} diff --git a/pkg/gofr/internal/rbaccheck/check_test.go b/pkg/gofr/internal/rbaccheck/check_test.go new file mode 100644 index 0000000000..448ccdcab2 --- /dev/null +++ b/pkg/gofr/internal/rbaccheck/check_test.go @@ -0,0 +1,350 @@ +package rbaccheck + +import ( + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "gofr.dev/pkg/gofr/rbac" +) + +// toRoutes turns "METHOD /template" labels into routes, built in when builtIn is set. +func toRoutes(builtIn bool, labels ...string) []Route { + routes := make([]Route, len(labels)) + + for i, label := range labels { + method, path, _ := strings.Cut(label, " ") + routes[i] = Route{Method: method, Path: path, BuiltIn: builtIn} + } + + return routes +} + +// coverageRoutes is a route table shaped like the one GoFr hands the check: one entry per method +// and path, a static directory served at /static, and GoFr's own routes. +func coverageRoutes(extra ...string) []Route { + routes := toRoutes(false, append([]string{ + "GET /api/users", + "POST /api/users", + "GET /api/users/{id}", + "DELETE /api/users/{id}", + "GET /api/posts", + "* /static", + "* /static/{path:.*}", + }, extra...)...) + + return append(routes, toRoutes(true, + "GET /.well-known/health", + "GET /.well-known/alive", + "GET /favicon.ico", + )...) +} + +// guard is a non-public rule requiring one permission. +func guard(path string, methods ...string) rbac.EndpointMapping { + return rbac.EndpointMapping{Path: path, Methods: methods, RequiredPermissions: []string{"p:x"}} +} + +// open is a public rule. +func open(path string, methods ...string) rbac.EndpointMapping { + return rbac.EndpointMapping{Path: path, Methods: methods, Public: true} +} + +// coveringRules guards every non-built-in route in coverageRoutes. +func coveringRules() []rbac.EndpointMapping { + return []rbac.EndpointMapping{ + guard("/api/users", http.MethodGet, http.MethodPost), + guard("/api/users/{id}", http.MethodGet, http.MethodDelete), + guard("/api/posts", http.MethodGet), + guard("/static", "*"), + guard("/static/{path:.*}", "*"), + } +} + +// checkCase is one case of TestCheck. +type checkCase struct { + desc string + endpoints []rbac.EndpointMapping + extraRoutes []string // application routes added to coverageRoutes for this case + want Mismatches +} + +func mismatchCases() []checkCase { + return []checkCase{ + { + desc: "every route covered and every rule live", + endpoints: coveringRules(), + }, + { + desc: "a typo in a path segment is a dead rule", + endpoints: append(coveringRules(), guard("/api/user/{id}", http.MethodDelete)), + want: Mismatches{DeadRules: []string{"DELETE /api/user/{id}"}}, + }, + { + desc: "a method the route does not register is a dead rule", + endpoints: append(coveringRules(), guard("/api/posts", http.MethodDelete)), + want: Mismatches{DeadRules: []string{"DELETE /api/posts"}}, + }, + { + desc: "a wildcard-method rule is live and covers every method", + endpoints: []rbac.EndpointMapping{ + guard("/api/users", "*"), + guard("/api/users/{id}"), + guard("/api/posts", "*"), + guard("/static", "*"), + guard("/static/{path:.*}", "*"), + }, + }, + { + desc: "a trailing slash makes a different path", + endpoints: append(coveringRules()[1:], guard("/api/users/", http.MethodGet, http.MethodPost)), + want: Mismatches{ + DeadRules: []string{"GET /api/users/", "POST /api/users/"}, + Uncovered: []string{"GET /api/users", "POST /api/users"}, + }, + }, + { + desc: "a relative path matches nothing", + endpoints: append(coveringRules()[1:], guard("api/users", http.MethodGet, http.MethodPost)), + want: Mismatches{ + DeadRules: []string{"GET api/users", "POST api/users"}, + Uncovered: []string{"GET /api/users", "POST /api/users"}, + }, + }, + { + desc: "a catch-all does not cover the path it hangs off", + endpoints: append(coveringRules()[:3], guard("/static/{path:.*}", "*")), + want: Mismatches{Uncovered: []string{"* /static"}}, + }, + { + desc: "a constraint narrower than the route covers it only partly", + endpoints: []rbac.EndpointMapping{ + guard("/api/users", http.MethodGet, http.MethodPost), + guard("/api/users/{id:[0-9]+}", http.MethodGet, http.MethodDelete), + guard("/api/posts", http.MethodGet), + guard("/static", "*"), + guard("/static/{path:.*}", "*"), + }, + want: Mismatches{PartlyCovered: []string{ + "DELETE /api/users/{id} (by DELETE /api/users/{id:[0-9]+})", + "GET /api/users/{id} (by GET /api/users/{id:[0-9]+})", + }}, + }, + { + desc: "a method list narrower than a static route covers it only partly", + endpoints: append(coveringRules()[:4], guard("/static/{path:.*}", http.MethodGet, http.MethodHead)), + want: Mismatches{PartlyCovered: []string{ + "* /static/{path:.*} (by GET /static/{path:.*} and HEAD /static/{path:.*})", + }}, + }, + } +} + +// builtInAndPublicCases are the cases of TestCheck about built-in routes, public rules, and more +// than one kind of mismatch at once. +func builtInAndPublicCases() []checkCase { + return []checkCase{ + { + desc: "a rule for a built-in route is live", + endpoints: append(coveringRules(), open("/.well-known/health", http.MethodGet)), + }, + { + desc: "a dead public rule is reported apart from dead guards", + endpoints: append(coveringRules(), open("/healthz", http.MethodGet)), + want: Mismatches{DeadPublicRules: []string{"GET /healthz"}}, + }, + { + desc: "a public rule covers its route", + endpoints: append(coveringRules()[1:], open("/api/users", "*")), + }, + { + desc: "uncovered routes are reported, built-in routes left out", + endpoints: []rbac.EndpointMapping{ + guard("/api/users", http.MethodGet), + guard("/api/users/{id}", "*"), + guard("/static", "*"), + guard("/static/{path:.*}", "*"), + }, + want: Mismatches{Uncovered: []string{"GET /api/posts", "POST /api/users"}}, + }, + { + desc: "an application route under /.well-known is checked", + endpoints: coveringRules(), + extraRoutes: []string{"GET /.well-known/custom"}, + want: Mismatches{Uncovered: []string{"GET /.well-known/custom"}}, + }, + { + desc: "dead rules and uncovered routes are both reported", + endpoints: append(coveringRules()[1:], guard("/api/user", http.MethodGet)), + want: Mismatches{ + DeadRules: []string{"GET /api/user"}, + Uncovered: []string{"GET /api/users", "POST /api/users"}, + }, + }, + { + desc: "a route method is read in upper case", + endpoints: coveringRules(), + extraRoutes: []string{ + "get /api/posts", + }, + }, + } +} + +func TestCheck(t *testing.T) { + for i, tc := range append(mismatchCases(), builtInAndPublicCases()...) { + t.Run(tc.desc, func(t *testing.T) { + got := Check(tc.endpoints, coverageRoutes(tc.extraRoutes...)) + + assert.Equal(t, tc.want, got, "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} + +func TestCheckAll(t *testing.T) { + static := []rbac.EndpointMapping{guard("/static", "*"), guard("/static/{path:.*}", "*")} + numeric := guard("/api/users/{id:[0-9]+}", http.MethodGet) + + testCases := []struct { + desc string + configs [][]rbac.EndpointMapping + want Mismatches + }{ + { + desc: "one config is checked as Check does", + configs: [][]rbac.EndpointMapping{coveringRules()}, + }, + { + desc: "a route one config covers is not reported for the other", + configs: [][]rbac.EndpointMapping{ + coveringRules(), + append([]rbac.EndpointMapping{guard("/api/users", http.MethodGet)}, static...), + }, + want: Mismatches{}, + }, + { + desc: "a route no config covers is uncovered, and a dead rule is numbered by config", + configs: [][]rbac.EndpointMapping{ + append([]rbac.EndpointMapping{guard("/api/users", "*"), guard("/api/users/{id}", "*"), guard("/api/user", http.MethodGet)}, static...), + append([]rbac.EndpointMapping{guard("/api/users", http.MethodGet)}, static...), + }, + want: Mismatches{DeadRules: []string{"config 1: GET /api/user"}, Uncovered: []string{"GET /api/posts"}}, + }, + { + desc: "a route only partly covered by every config is partly covered", + configs: [][]rbac.EndpointMapping{ + append([]rbac.EndpointMapping{guard("/api/users", "*"), guard("/api/posts", "*"), numeric}, static...), + append([]rbac.EndpointMapping{guard("/api/posts", "*"), numeric}, static...), + }, + want: Mismatches{ + Uncovered: []string{"DELETE /api/users/{id}"}, + PartlyCovered: []string{"GET /api/users/{id} (by GET /api/users/{id:[0-9]+})"}, + }, + }, + } + + for i, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + got := CheckAll(tc.configs, coverageRoutes()) + + assert.Equal(t, tc.want, got, "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} + +func TestMismatches_Failed(t *testing.T) { + testCases := []struct { + desc string + mismatches Mismatches + want bool + }{ + {"no mismatch", Mismatches{}, false}, + {"only dead public rules", Mismatches{DeadPublicRules: []string{"GET /healthz"}}, false}, + {"a dead rule", Mismatches{DeadRules: []string{"GET /api/user"}}, true}, + {"an uncovered route", Mismatches{Uncovered: []string{"GET /api/users"}}, true}, + {"a partly covered route", Mismatches{PartlyCovered: []string{"GET /a (by GET /a/{id:[0-9]+})"}}, true}, + } + + for i, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + assert.Equal(t, tc.want, tc.mismatches.Failed(), "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} + +// agreementConfig declares its rules in the shapes Check has to read the way the rbac matcher does: +// a lower-case method, no methods at all, and a (method, path) declared twice, where the second +// declaration replaces the first. +const agreementConfig = `{"roleHeader":"X-User-Role", +"roles":[{"name":"admin","permissions":["p:x"]}], +"endpoints":[ +{"path":"/lower","methods":["get"],"requiredPermissions":["p:x"]}, +{"path":"/any","requiredPermissions":["p:x"]}, +{"path":"/dup","methods":["GET"],"public":true}, +{"path":"/dup","methods":["GET"],"requiredPermissions":["p:x"]}, +{"path":"/gone","methods":["GET"],"public":true}, +{"path":"/gone","methods":["get"],"requiredPermissions":["p:x"]} +]}` + +// TestCheck_AgreesWithMiddleware checks that Check reads the rules the way the rbac middleware +// enforces them. Check rebuilds the rules from Config.Endpoints rather than sharing the matcher's, +// so it repeats how the matcher reads a declaration; if either side changes that alone, the two +// halves of this test disagree. +func TestCheck_AgreesWithMiddleware(t *testing.T) { + path := filepath.Join(t.TempDir(), "rbac.json") + require.NoError(t, os.WriteFile(path, []byte(agreementConfig), 0o600)) + + config, err := rbac.LoadPermissions(path, nil, nil, nil) + require.NoError(t, err) + + // Every rule is guarded once declarations are read as the matcher reads them, and only /gone + // has no route. + got := Check(config.Endpoints, toRoutes(false, "GET /lower", "DELETE /any", "GET /dup")) + assert.Equal(t, Mismatches{DeadRules: []string{"GET /gone"}}, got) + + // The middleware guards every one of those requests: a request with no role never reaches the + // handler. + reached := false + guarded := rbac.Middleware(config)(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { reached = true })) + + for i, request := range []string{"GET /lower", "DELETE /any", "GET /dup", "GET /gone"} { + method, target, _ := strings.Cut(request, " ") + reached = false + + guarded.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(t.Context(), method, target, http.NoBody)) + + assert.False(t, reached, "TEST[%d], Failed.\n%s is served without a role", i, request) + } +} + +// BenchmarkCheck measures the check on an app with n routes, each guarded by its own rule, with a +// constrained variable in every path so the regex cache is exercised. +func BenchmarkCheck(b *testing.B) { + for _, n := range []int{50, 200, 1000} { + b.Run(fmt.Sprintf("routes=%d", n), func(b *testing.B) { + endpoints := make([]rbac.EndpointMapping, 0, n) + routes := make([]Route, 0, n) + + for i := range n { + path := fmt.Sprintf("/api/v1/resource%d/{id:[0-9]+}/items", i) + endpoints = append(endpoints, guard(path, http.MethodGet)) + routes = append(routes, Route{Method: http.MethodGet, Path: path}) + } + + b.ReportAllocs() + + for b.Loop() { + if got := Check(endpoints, routes); got.Failed() { + b.Fatal("every route is covered, so no mismatch is expected") + } + } + }) + } +} diff --git a/pkg/gofr/internal/rbaccheck/match.go b/pkg/gofr/internal/rbaccheck/match.go new file mode 100644 index 0000000000..b01c48d98d --- /dev/null +++ b/pkg/gofr/internal/rbaccheck/match.go @@ -0,0 +1,427 @@ +package rbaccheck + +import ( + "regexp" + "regexp/syntax" + "slices" + "strings" +) + +// anyMethod stands for "every method", on a rule declared with "*" and on a route registered +// without a method matcher, such as a static-file prefix. +const anyMethod = "*" + +// freeVariablePattern is what mux matches a "{name}" variable against in a path. +const freeVariablePattern = "[^/]+" + +// invalidPattern stands for the pattern of a segment mux would refuse. It does not compile, so the +// segment covers nothing. +const invalidPattern = "[" + +// routeChecker holds what one check shares across its comparisons: each distinct constraint is +// compiled once, however many rules and routes carry it. +type routeChecker struct { + constraints map[string]compiledConstraint +} + +// compiledConstraint is what one check knows about a constraint. +type compiledConstraint struct { + // re is the constraint anchored to a whole segment, or nil when it does not compile. + re *regexp.Regexp + + // spans is set when the constraint can match a "/". See admitsSlash. + spans bool +} + +func newRouteChecker() *routeChecker { + return &routeChecker{constraints: make(map[string]compiledConstraint)} +} + +// routeEntry is a rule or a route, split into segments once for every comparison it takes part in. +type routeEntry struct { + // method is the upper-cased method, or anyMethod. + method string + + // label is how the entry is reported: "METHOD path". + label string + + // segments is the path split on "/", empty segments kept. Nil for an empty path, which matches + // nothing. + segments []pathSegment +} + +// pathSegment is one "/"-separated part of a mux path template. +type pathSegment struct { + // text is the segment as written. + text string + + // variable is set for a segment holding any "{...}" variable. + variable bool + + // catchAll is set for a variable that can span several segments: one whose pattern can match a + // "/", as mux applies a variable's pattern to the whole path rather than to one segment. + catchAll bool + + // constraint is what the segment matches. For a segment that is exactly one variable, it is the + // variable's constraint as written - mux does not trim it - and empty for a free variable. For + // any other segment holding variables, such as "{a}-{b}", it is the regexp mux builds for the + // segment. + constraint string + + // re is constraint compiled and anchored, or nil for a free variable or one that does not + // compile. + re *regexp.Regexp +} + +func (rc *routeChecker) newEntry(method, path string) routeEntry { + entry := routeEntry{method: method, label: method + " " + path} + + if path == "" { + return entry + } + + parts := splitPatternSegments(path) + entry.segments = make([]pathSegment, len(parts)) + + for i, part := range parts { + entry.segments[i] = rc.newSegment(part) + } + + return entry +} + +func (rc *routeChecker) newSegment(text string) pathSegment { + seg := pathSegment{text: text} + + if !strings.Contains(text, "{") { + return seg + } + + seg.variable = true + seg.constraint = segmentPattern(text) + + if seg.constraint != "" { + compiled := rc.compile(seg.constraint) + seg.re, seg.catchAll = compiled.re, compiled.re != nil && compiled.spans + } + + return seg +} + +// segmentPattern returns what a segment holding variables matches, the way mux reads its template: +// the constraint as written for a segment that is exactly one "{name:constraint}", empty for one +// "{name}", and otherwise the segment's literal text and each variable's pattern - "[^/]+" for a +// free one - joined into one regexp. A segment mux would refuse, with unbalanced braces or an +// empty name or pattern, gets invalidPattern, which does not compile and so covers nothing. +func segmentPattern(text string) string { + groups := braceGroups(text) + if groups == nil { + return invalidPattern + } + + var ( + pattern strings.Builder + end int + ) + + for _, group := range groups { + name, constraint, hasConstraint := strings.Cut(text[group[0]+1:group[1]], ":") + if name == "" || hasConstraint && constraint == "" { + return invalidPattern + } + + if len(groups) == 1 && group[0] == 0 && group[1] == len(text)-1 { + return constraint + } + + if !hasConstraint { + constraint = freeVariablePattern + } + + pattern.WriteString(regexp.QuoteMeta(text[end:group[0]]) + "(?:" + constraint + ")") + end = group[1] + 1 + } + + pattern.WriteString(regexp.QuoteMeta(text[end:])) + + return pattern.String() +} + +// braceGroups returns the index of the opening and closing brace of each outermost "{...}" in text, +// or nil when its braces do not balance. Braces nest, as in "{id:[0-9]{3}}". +func braceGroups(text string) [][2]int { + var ( + groups [][2]int + depth, start int + ) + + for i, r := range text { + switch r { + case '{': + if depth == 0 { + start = i + } + + depth++ + case '}': + depth-- + + switch { + case depth < 0: + return nil + case depth == 0: + groups = append(groups, [2]int{start, i}) + } + } + } + + if depth != 0 { + return nil + } + + return groups +} + +// admitsSlash reports whether the regexp pattern can match a string containing "/". It looks only +// at which characters the pattern names, so it can answer "yes" for a pattern that never actually +// produces one; that only makes the check stricter. +func admitsSlash(pattern string) bool { + re, err := syntax.Parse(pattern, syntax.Perl) + if err != nil { + return false + } + + return regexpAdmits(re, '/') +} + +// regexpAdmits reports whether re names r anywhere: as a literal, in a character class, or through +// a wildcard. +func regexpAdmits(re *syntax.Regexp, r rune) bool { + if re.Op == syntax.OpAnyChar || re.Op == syntax.OpAnyCharNotNL { + return true + } + + if re.Op == syntax.OpLiteral { + return slices.Contains(re.Rune, r) + } + + if re.Op == syntax.OpCharClass { + // Rune holds the class as inclusive [lo, hi] pairs. + for i := 0; i+1 < len(re.Rune); i += 2 { + if re.Rune[i] <= r && r <= re.Rune[i+1] { + return true + } + } + + return false + } + + for _, sub := range re.Sub { + if regexpAdmits(sub, r) { + return true + } + } + + return false +} + +// compile returns constraint anchored to a whole segment, as mux applies it, and whether it can +// match a "/". +func (rc *routeChecker) compile(constraint string) compiledConstraint { + if compiled, ok := rc.constraints[constraint]; ok { + return compiled + } + + var compiled compiledConstraint + + if re, err := regexp.Compile("^(?:" + constraint + ")$"); err == nil { + compiled = compiledConstraint{re: re, spans: admitsSlash(constraint)} + } + + rc.constraints[constraint] = compiled + + return compiled +} + +// overlaps reports whether some request could be served by route and governed by the rule e. +// +// It errs toward "yes", because a wrong "no" reports a live rule as dead, telling the operator its +// route is unguarded when it is not. Two variables always overlap, even when their constraints +// could never agree, and a constraint that does not compile overlaps anything. +func (e *routeEntry) overlaps(route *routeEntry) bool { + methodsOverlap := e.method == anyMethod || route.method == anyMethod || e.method == route.method + + return methodsOverlap && segmentsOverlap(e.segments, route.segments) +} + +// segmentsOverlap reports whether some request path could match both a and b. +func segmentsOverlap(a, b []pathSegment) bool { + if a == nil || b == nil { + return false + } + + for i := 0; ; i++ { + if i == len(a) || i == len(b) { + return len(a) == len(b) + } + + // A catch-all absorbs the rest of the other path, which may be empty but must be there: the + // "/" before a catch-all is part of the template. + if a[i].catchAll || b[i].catchAll { + return true + } + + if !a[i].mayMatch(&b[i]) { + return false + } + } +} + +// mayMatch reports whether one path segment could match both s and other. +func (s *pathSegment) mayMatch(other *pathSegment) bool { + switch { + case s.variable && other.variable: + return true + case s.variable: + return s.matchesLiteral(other.text, true) + case other.variable: + return other.matchesLiteral(s.text, true) + default: + return s.text == other.text + } +} + +// matchesLiteral reports whether the variable s matches literal. A free variable matches any +// non-empty literal; a constraint that does not compile matches per ifUnknown. +func (s *pathSegment) matchesLiteral(literal string, ifUnknown bool) bool { + switch { + case s.constraint == "": + return literal != "" && !strings.Contains(literal, "/") + case s.re == nil: + return ifUnknown + default: + return s.re.MatchString(literal) + } +} + +// covers reports whether the rule e governs every request route serves. +func (e *routeEntry) covers(route *routeEntry) bool { + methodCovers := e.method == anyMethod || e.method == route.method + + return methodCovers && segmentsCover(e.segments, route.segments) +} + +// segmentsCover reports whether the rule path a matches every request path the route path b does. +func segmentsCover(a, b []pathSegment) bool { + if a == nil || b == nil { + return false + } + + for i := 0; ; i++ { + if i == len(a) || i == len(b) { + return len(a) == len(b) + } + + // A catch-all covers only as the rule's last segment: the rule's segments after it must match + // too, and how much of the path the catch-all leaves them is not modeled. + if a[i].catchAll { + return i == len(a)-1 && a[i].coversTail(b[i:]) + } + + if !a[i].covers(&b[i]) { + return false + } + } +} + +// covers reports whether the rule segment s matches everything the route segment other does. +func (s *pathSegment) covers(other *pathSegment) bool { + switch { + case other.catchAll: + return false + case !other.variable: + return s.text == other.text || s.variable && s.matchesLiteral(other.text, false) + case !s.variable: + return false + case s.constraint == "": + // A free variable matches any non-empty segment, so it covers a constraint that cannot + // match an empty one. One that can match a "/" is a catch-all, turned away above. + return other.constraint == "" || other.re != nil && !other.re.MatchString("") + default: + return s.re != nil && s.constraint == other.constraint + } +} + +// coversTail reports whether the rule catch-all s matches every remainder the route segments tail +// can produce, joined with "/". tail is never empty. +func (s *pathSegment) coversTail(tail []pathSegment) bool { + mayBeEmpty := len(tail) == 1 && tail[0].mayBeEmpty() + + switch { + case s.constraint == ".*": + return true + case s.constraint == ".+": + return !mayBeEmpty + case s.re == nil: + return false + } + + if len(tail) == 1 && tail[0].catchAll { + return s.constraint == tail[0].constraint + } + + literals := make([]string, len(tail)) + + for i := range tail { + if tail[i].variable { + return false + } + + literals[i] = tail[i].text + } + + return s.re.MatchString(strings.Join(literals, "/")) +} + +// mayBeEmpty reports whether the route segment s can match an empty segment. +func (s *pathSegment) mayBeEmpty() bool { + switch { + case !s.variable: + return s.text == "" + case s.constraint == "": + return false + case s.re == nil: + return true + default: + return s.re.MatchString("") + } +} + +// splitPatternSegments splits a path pattern on "/", ignoring separators that sit inside a +// "{name:regex}" constraint - "{path:[a-z/]+}" is one segment, not three. It is the rbac +// resolver's split, repeated here because the resolver's is unexported. +func splitPatternSegments(pattern string) []string { + var ( + segments []string + depth int + start int + ) + + for i, r := range pattern { + switch r { + case '{': + depth++ + case '}': + if depth > 0 { + depth-- + } + case '/': + if depth == 0 { + segments = append(segments, pattern[start:i]) + start = i + 1 + } + } + } + + return append(segments, pattern[start:]) +} diff --git a/pkg/gofr/internal/rbaccheck/match_test.go b/pkg/gofr/internal/rbaccheck/match_test.go new file mode 100644 index 0000000000..43dfa68318 --- /dev/null +++ b/pkg/gofr/internal/rbaccheck/match_test.go @@ -0,0 +1,120 @@ +package rbaccheck + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +// parseEntry parses "METHOD path" the way Check builds an entry from a route. +func parseEntry(rc *routeChecker, s string) routeEntry { + method, path, _ := strings.Cut(s, " ") + + return rc.newEntry(method, path) +} + +func Test_routeEntry_overlaps(t *testing.T) { + testCases := []struct { + desc string + rule string + route string + want bool + }{ + {"identical literals", "GET /api/users", "GET /api/users", true}, + {"different literals", "GET /api/users", "GET /api/posts", false}, + {"typo in a literal segment", "GET /api/user/{id}", "GET /api/users/{id}", false}, + {"free variable against literal", "GET /api/users/{id}", "GET /api/users/me", true}, + {"literal against free variable", "GET /api/users/me", "GET /api/users/{id}", true}, + {"free variable against an empty segment", "GET /api/users/{id}", "GET /api/users/", false}, + {"constraint admits the literal", "GET /api/users/{id:[0-9]+}", "GET /api/users/42", true}, + {"constraint rejects the literal", "GET /api/users/{id:[0-9]+}", "GET /api/users/me", false}, + {"route constraint rejects the literal", "GET /api/users/me", "GET /api/users/{id:[0-9]+}", false}, + {"variable against variable", "GET /api/users/{id:[0-9]+}", "GET /api/users/{id:[a-z]+}", true}, + {"more segments in the rule", "GET /api/users/{id}/posts", "GET /api/users/{id}", false}, + {"fewer segments in the rule", "GET /api", "GET /api/users", false}, + {"catch-all in the rule absorbs the rest", "GET /api/{path:.*}", "GET /api/users/{id}/posts", true}, + {"catch-all in the rule needs its slash", "GET /api/{path:.*}", "GET /api", false}, + {"catch-all in the rule matches an empty tail", "GET /api/{path:.*}", "GET /api/", true}, + {"catch-all in the route absorbs the rest", "GET /static/css/site.css", "GET /static/{path:.*}", true}, + {"catch-all after a different literal", "GET /admin/{path:.*}", "GET /api/users", false}, + {"trailing slash is a different path", "GET /api/users/", "GET /api/users", false}, + {"relative path is a different path", "GET api/users", "GET /api/users", false}, + {"root against root", "GET /", "GET /", true}, + {"root against a path", "GET /", "GET /api", false}, + {"constraint containing a slash spans segments", "GET /files/{path:[a-z/]+}", "GET /files/a/b/c", true}, + {"uncompilable constraint is assumed to overlap", "GET /api/{id:[}", "GET /api/users", true}, + {"space in a constraint is part of it", "GET /orders/{id: [0-9]+}", "GET /orders/42", false}, + {"two variables in one segment admit the literal", "GET /items/{a}-{b}", "GET /items/x-y", true}, + {"two variables in one segment reject the literal", "GET /items/{a}-{b}", "GET /items/abc", false}, + {"route constraint matching a slash spans segments", "GET /files/a/b", "GET /files/{p:[^.]+}", true}, + {"empty rule matches nothing", "GET ", "GET /api", false}, + {"different methods", "GET /api/users", "POST /api/users", false}, + {"wildcard rule method", "* /api/users", "POST /api/users", true}, + {"wildcard route method", "GET /static/{path:.*}", "* /static/{path:.*}", true}, + } + + for i, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + rc := newRouteChecker() + rule, route := parseEntry(rc, tc.rule), parseEntry(rc, tc.route) + + assert.Equal(t, tc.want, rule.overlaps(&route), "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} + +func Test_routeEntry_covers(t *testing.T) { + testCases := []struct { + desc string + rule string + route string + want bool + }{ + {"identical literals", "GET /api/users", "GET /api/users", true}, + {"trailing slash in the rule", "GET /api/users/", "GET /api/users", false}, + {"relative rule path", "GET files", "GET /files", false}, + {"catch-all does not cover the path it hangs off", "GET /admin/{rest:.*}", "GET /admin", false}, + {"catch-all covers a path under it", "GET /admin/{rest:.*}", "GET /admin/users/{id}", true}, + {"catch-all covers an empty tail", "GET /api/{path:.*}", "GET /api/", true}, + {"non-empty catch-all does not cover an empty tail", "GET /api/{path:.+}", "GET /api/", false}, + {"catch-all covers a route catch-all", "GET /static/{p:.*}", "GET /static/{path:.*}", true}, + {"non-empty catch-all does not cover one that may be empty", "GET /static/{p:.+}", "GET /static/{path:.*}", false}, + {"free variable does not cover a route catch-all", "GET /static/{name}", "GET /static/{path:.*}", false}, + {"free variable covers a literal", "GET /api/users/{id}", "GET /api/users/me", true}, + {"free variable does not cover an empty segment", "GET /api/users/{id}", "GET /api/users/", false}, + {"free variable covers a free variable", "GET /api/users/{id}", "GET /api/users/{uid}", true}, + {"free variable covers a non-empty constraint", "GET /api/users/{id}", "GET /api/users/{id:[0-9]+}", true}, + {"free variable does not cover a constraint that admits empty", "GET /api/{id}", "GET /api/{id:[0-9]*}", false}, + {"constraint does not cover a free variable", "GET /api/users/{id:[0-9]+}", "GET /api/users/{id}", false}, + {"identical constraints", "GET /api/users/{id:[0-9]+}", "GET /api/users/{uid:[0-9]+}", true}, + {"different constraints", "GET /api/users/{id:[0-9]+}", "GET /api/users/{id:[a-z]+}", false}, + {"constraint admits the literal", "GET /api/users/{id:[0-9]+}", "GET /api/users/42", true}, + {"literal does not cover a variable", "GET /api/users/me", "GET /api/users/{id}", false}, + {"uncompilable constraint covers nothing", "GET /api/{id:[}", "GET /api/users", false}, + {"slash constraint covers a matching literal tail", "GET /files/{path:[a-z/]+}", "GET /files/a/b", true}, + {"slash constraint does not cover a variable", "GET /files/{path:[a-z/]+}", "GET /files/{name}", false}, + {"catch-all covers only as the rule's last segment", "GET /api/{rest:.*}/admin", "GET /api/users", false}, + {"two variables in one segment are not one free variable", "GET /items/{a}-{b}", "GET /items/{id}", false}, + {"two variables in one segment cover the same segment", "GET /items/{a}-{b}", "GET /items/{x}-{y}", true}, + {"two variables in one segment cover a literal they match", "GET /items/{a}-{b}", "GET /items/x-y", true}, + {"free variable covers two variables in one segment", "GET /items/{id}", "GET /items/{a}-{b}", true}, + {"free variable does not cover a constraint matching a slash", "GET /files/{x}", "GET /files/{p:[^.]+}", false}, + {"constraint matching a slash covers a literal tail", "GET /files/{p:[^.]+}", "GET /files/a/b", true}, + {"space in a constraint is part of it", "GET /orders/{id: [0-9]+}", "GET /orders/{id:[0-9]+}", false}, + {"same method", "GET /api/users", "GET /api/users", true}, + {"different method", "GET /api/users", "POST /api/users", false}, + {"wildcard rule covers any method", "* /api/users", "DELETE /api/users", true}, + {"wildcard rule covers a wildcard route", "* /static/{path:.*}", "* /static/{path:.*}", true}, + {"one method does not cover a wildcard route", "GET /static/{path:.*}", "* /static/{path:.*}", false}, + } + + for i, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + rc := newRouteChecker() + rule, route := parseEntry(rc, tc.rule), parseEntry(rc, tc.route) + + assert.Equal(t, tc.want, rule.covers(&route), "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} diff --git a/pkg/gofr/internal/routemark/routemark.go b/pkg/gofr/internal/routemark/routemark.go new file mode 100644 index 0000000000..a0d10ab0ad --- /dev/null +++ b/pkg/gofr/internal/routemark/routemark.go @@ -0,0 +1,58 @@ +// Package routemark records which route a request matched and which route is GoFr's own 404 +// catch-all, so that the http router, the RBAC middleware and the RBAC route check all ask the +// same question the same way. It is internal so none of this becomes public API. +package routemark + +import ( + "context" + "net/http" + + "github.com/gorilla/mux" +) + +// catchAllName is the name the catch-all route is registered under. mux keeps it in a plain field of +// the route, so asking for it costs a string compare and no allocation, and no registry is needed +// that would have to outlive or be shared between routers. +const catchAllName = "gofr.catch-all" + +// ctxKey is the private context key under which the trie router stores the matched route. It +// exists because, once the trie router bypasses mux's ServeHTTP, mux.CurrentRoute(r) is no longer +// populated. A pointer is stored rather than the template string: it fits in an interface without +// an allocation. +type ctxKey struct{} + +// MarkCatchAll marks route as GoFr's catch-all, the route that answers 404 for every request no +// other route matched, and returns it. Only that route is marked: a route an application registers +// at "/", or a static directory served from "/", has the same template and is a real route. +func MarkCatchAll(route *mux.Route) *mux.Route { + return route.Name(catchAllName) +} + +// IsCatchAll reports whether route is the route MarkCatchAll marked. A nil route is not. +func IsCatchAll(route *mux.Route) bool { + return route != nil && route.GetName() == catchAllName +} + +// With returns r carrying route as the matched route. A nil route is a no-op so unmatched requests +// do not pay for a context copy. +func With(r *http.Request, route *mux.Route) *http.Request { + if route == nil { + return r + } + + return r.WithContext(context.WithValue(r.Context(), ctxKey{}, route)) +} + +// Matched returns the route that matched r under either router, or nil. +func Matched(r *http.Request) *mux.Route { + if route, _ := r.Context().Value(ctxKey{}).(*mux.Route); route != nil { + return route + } + + return mux.CurrentRoute(r) +} + +// CatchAllAnswered reports whether GoFr's catch-all is the route that matched r. +func CatchAllAnswered(r *http.Request) bool { + return IsCatchAll(Matched(r)) +} diff --git a/pkg/gofr/internal/routemark/routemark_test.go b/pkg/gofr/internal/routemark/routemark_test.go new file mode 100644 index 0000000000..bd037c3eb4 --- /dev/null +++ b/pkg/gofr/internal/routemark/routemark_test.go @@ -0,0 +1,40 @@ +package routemark + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/gorilla/mux" + "github.com/stretchr/testify/assert" +) + +func TestIsCatchAll(t *testing.T) { + router := mux.NewRouter() + + testCases := []struct { + desc string + route *mux.Route + want bool + }{ + {"marked route", MarkCatchAll(router.PathPrefix("/")), true}, + {"unmarked prefix route at /", router.PathPrefix("/"), false}, + {"unmarked exact route at /", router.Path("/"), false}, + {"route with another name", router.PathPrefix("/").Name("other"), false}, + {"nil route", nil, false}, + } + + for i, tc := range testCases { + assert.Equal(t, tc.want, IsCatchAll(tc.route), "TEST[%d], Failed.\n%s", i, tc.desc) + } +} + +func TestMatched(t *testing.T) { + route := mux.NewRouter().Path("/x") + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/x", http.NoBody) + + assert.Nil(t, Matched(req), "an unrouted request has no matched route") + assert.False(t, CatchAllAnswered(req), "an unrouted request was not answered by the catch-all") + assert.Same(t, req, With(req, nil), "no route must not copy the request") + assert.Same(t, route, Matched(With(req, route)), "the route put in the context must come back") +} diff --git a/pkg/gofr/rbac.go b/pkg/gofr/rbac.go index 93d455a2c6..8c7739c205 100644 --- a/pkg/gofr/rbac.go +++ b/pkg/gofr/rbac.go @@ -3,10 +3,14 @@ package gofr import ( "errors" "fmt" + "strings" + "github.com/gorilla/mux" "go.opentelemetry.io/otel" "gofr.dev/pkg/gofr/datasource" + "gofr.dev/pkg/gofr/internal/rbaccheck" + "gofr.dev/pkg/gofr/internal/routemark" "gofr.dev/pkg/gofr/rbac" ) @@ -78,6 +82,8 @@ func (a *App) enableRBAC(configPath ...string) error { middlewareFunc := rbac.Middleware(config) a.UseMiddleware(middlewareFunc) + a.rbacConfigs = append(a.rbacConfigs, config) + return nil } @@ -119,3 +125,161 @@ func (l *errorCountingLogger) Errorf(format string, args ...any) { l.errors++ l.Logger.Errorf(format, args...) } + +// rbacRouteCheckKey selects what the startup route check does on a mismatch. See rbacRouteCheckMode. +const rbacRouteCheckKey = "GOFR_RBAC_ROUTE_CHECK" + +// Values of rbacRouteCheckKey. +const ( + rbacRouteCheckWarn = "warn" + rbacRouteCheckFail = "fail" + rbacRouteCheckOff = "off" +) + +// rbacRouteCheckMode reads rbacRouteCheckKey. A value it does not know is logged and read as +// rbacRouteCheckWarn: failing closed on a typo in a diagnostic setting would stop an app that has +// nothing wrong with it, and switching the check off would hide the mismatches it was set to show. +func (a *App) rbacRouteCheckMode() string { + mode := strings.ToLower(strings.TrimSpace(a.Config.GetOrDefault(rbacRouteCheckKey, rbacRouteCheckWarn))) + + switch mode { + case rbacRouteCheckWarn, rbacRouteCheckFail, rbacRouteCheckOff: + return mode + default: + a.Logger().Errorf("invalid %s=%q: use %q, %q or %q. Using %q.", rbacRouteCheckKey, mode, + rbacRouteCheckWarn, rbacRouteCheckFail, rbacRouteCheckOff, rbacRouteCheckWarn) + + return rbacRouteCheckWarn + } +} + +// checkRBACRoutes runs the RBAC route check against the route table httpServerSetup collected, and +// logs one line for each kind of mismatch it finds. It reports whether any mismatch can leave a +// request served without role checks. +func (a *App) checkRBACRoutes() bool { + endpoints := make([][]rbac.EndpointMapping, len(a.rbacConfigs)) + for i, config := range a.rbacConfigs { + endpoints[i] = config.Endpoints + } + + m := rbaccheck.CheckAll(endpoints, a.rbacRoutes) + logger := a.Logger() + + if len(m.DeadPublicRules) > 0 { + logger.Warnf("RBAC: public rules match no registered route: %s", strings.Join(m.DeadPublicRules, ", ")) + } + + if len(m.DeadRules) > 0 { + logger.Errorf("RBAC route check failed: rules match no registered route: %s. These rules protect "+ + "nothing; fix each rule's path or methods, or remove it.", strings.Join(m.DeadRules, ", ")) + } + + if len(m.Uncovered) > 0 { + logger.Errorf("RBAC route check failed: routes covered by no rule: %s. They are served without role "+ + "checks; add a rule for each, with \"public\": true for a route meant to be open.", strings.Join(m.Uncovered, ", ")) + } + + if len(m.PartlyCovered) > 0 { + logger.Errorf("RBAC route check failed: routes only partly covered: %s. Requests these rules do not "+ + "match are served without role checks; add a rule that matches each route in full.", strings.Join(m.PartlyCovered, ", ")) + } + + return m.Failed() +} + +// rbacRouteCollector builds the route table the RBAC route check reads, from the router walk +// httpServerSetup already makes. A disabled collector adds nothing. +type rbacRouteCollector struct { + enabled bool + seen map[rbaccheck.Route]bool + + // builtInFrom and builtInTo bound, as positions in the router walk, the routes GoFr registers + // itself; walked is the position of the next route add sees. The routes are told apart by + // where they were registered rather than by their path, so that an application route under + // /.well-known/ is still checked. + builtInFrom, builtInTo, walked int + + routes []rbaccheck.Route +} + +func newRBACRouteCollector(enabled bool) *rbacRouteCollector { + return &rbacRouteCollector{enabled: enabled, seen: make(map[rbaccheck.Route]bool)} +} + +// startBuiltIns marks the routes registered on router from now on as GoFr's own, until endBuiltIns. +func (c *rbacRouteCollector) startBuiltIns(router *mux.Router) { + if c.enabled { + c.builtInFrom = countRoutes(router) + } +} + +// endBuiltIns ends the run of routes startBuiltIns began. +func (c *rbacRouteCollector) endBuiltIns(router *mux.Router) { + if c.enabled { + c.builtInTo = countRoutes(router) + } +} + +// nextIsBuiltIn reports whether the route at the next walk position is one of GoFr's own, and +// moves past it. +func (c *rbacRouteCollector) nextIsBuiltIn() bool { + position := c.walked + c.walked++ + + return position >= c.builtInFrom && position < c.builtInTo +} + +// countRoutes returns how many routes a walk of router visits. +func countRoutes(router *mux.Router) int { + n := 0 + + _ = router.Walk(func(*mux.Route, *mux.Router, []*mux.Route) error { + n++ + + return nil + }) + + return n +} + +// add records route once per method, "*" for a route with no method matcher, as built in when it +// falls between startBuiltIns and endBuiltIns. A prefix route gets a trailing catch-all segment, so +// that it stands for every path under the prefix. +// GoFr's PathPrefix("/") catch-all is left out: it answers 404 for every path no other route +// serves, and would otherwise make every rule look live. +func (c *rbacRouteCollector) add(route *mux.Route, methods []string) { + if !c.enabled { + return + } + + builtIn := c.nextIsBuiltIn() + + if routemark.IsCatchAll(route) { + return + } + + tmpl, err := route.GetPathTemplate() + if err != nil { + return + } + + // mux anchors a Path regexp with "$" and leaves a PathPrefix one open. + pathRegexp, _ := route.GetPathRegexp() + if !strings.HasSuffix(pathRegexp, "$") { + tmpl = strings.TrimSuffix(tmpl, "/") + "/{path:.*}" + } + + if len(methods) == 0 { + methods = []string{"*"} + } + + for _, method := range methods { + r := rbaccheck.Route{Method: strings.ToUpper(method), Path: tmpl, BuiltIn: builtIn} + if c.seen[r] { + continue + } + + c.seen[r] = true + c.routes = append(c.routes, r) + } +} diff --git a/pkg/gofr/rbac/middleware.go b/pkg/gofr/rbac/middleware.go index f15359cc70..4662faa180 100644 --- a/pkg/gofr/rbac/middleware.go +++ b/pkg/gofr/rbac/middleware.go @@ -15,7 +15,9 @@ import ( "go.opentelemetry.io/otel/trace" "gofr.dev/pkg/gofr/datasource" + gofrHTTP "gofr.dev/pkg/gofr/http" "gofr.dev/pkg/gofr/http/middleware" + "gofr.dev/pkg/gofr/internal/routemark" ) type authMethod int @@ -24,6 +26,22 @@ const userRole authMethod = 4 const unknownRouteLabel = "" +// matchedRouteLabel returns the route template the router matched for r, which is what http.route +// means everywhere else in a trace. The RBAC rule that governed the request can be broader or +// narrower than that route, so it is recorded separately as rbac.rule. +// +// GoFr's catch-all, which serves every request no route matched, is reported as unmatched. An +// application route or a static directory registered at "/" shares its template, so the two are +// told apart by the mark GoFr puts on the catch-all when it registers it. +func matchedRouteLabel(r *http.Request) string { + tmpl := gofrHTTP.RouteTemplate(r) + if tmpl == "" || routemark.CatchAllAnswered(r) { + return unknownRouteLabel + } + + return tmpl +} + // wellKnownPrefix is the path prefix the OAuth middleware passes through without verifying a token. const wellKnownPrefix = "/.well-known" @@ -125,7 +143,8 @@ func Middleware(config *Config) func(handler http.Handler) http.Handler { ctx, span := config.Tracer.Start(r.Context(), "rbac.authorize") span.SetAttributes( attribute.String("http.method", r.Method), - attribute.String("http.route", routeLabel), + attribute.String("http.route", matchedRouteLabel(r)), + attribute.String("rbac.rule", routeLabel), ) r = r.WithContext(ctx) diff --git a/pkg/gofr/rbac/middleware_test.go b/pkg/gofr/rbac/middleware_test.go index 2d665231ae..66a32fceed 100644 --- a/pkg/gofr/rbac/middleware_test.go +++ b/pkg/gofr/rbac/middleware_test.go @@ -7,18 +7,25 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "github.com/golang-jwt/jwt/v5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + sdktrace "go.opentelemetry.io/otel/sdk/trace" + "go.opentelemetry.io/otel/sdk/trace/tracetest" "go.opentelemetry.io/otel/trace/noop" "go.uber.org/mock/gomock" "gofr.dev/pkg/gofr/container" "gofr.dev/pkg/gofr/datasource" + gofrHTTP "gofr.dev/pkg/gofr/http" "gofr.dev/pkg/gofr/http/middleware" + "gofr.dev/pkg/gofr/internal/routemark" + "gofr.dev/pkg/gofr/logging" ) func TestMiddleware_NilConfig(t *testing.T) { @@ -1515,3 +1522,74 @@ func TestExtractionFailure_Error(t *testing.T) { assert.Equal(t, tc.want, tc.failure.Error(), "TEST[%d], Failed.\n%s", i, tc.desc) } } + +func TestMiddleware_TraceRouteLabels(t *testing.T) { + testCases := []struct { + desc string + router string + path string + wantRoute string + wantRule string + static bool // serve a directory from "/", governed by a /{path:.*} rule + }{ + {"mux: route and rule differ", gofrHTTP.MatcherMux, "/api/users/42", "/api/users/{id}", "/api/{path:.*}", false}, + {"trie: route and rule differ", gofrHTTP.MatcherTrie, "/api/users/42", "/api/users/{id}", "/api/{path:.*}", false}, + {"mux: no route and no rule", gofrHTTP.MatcherMux, "/nowhere", unknownRouteLabel, unknownRouteLabel, false}, + {"trie: no route and no rule", gofrHTTP.MatcherTrie, "/nowhere", unknownRouteLabel, unknownRouteLabel, false}, + {"mux: route at / is not the catch-all", gofrHTTP.MatcherMux, "/", "/", unknownRouteLabel, false}, + {"trie: route at / is not the catch-all", gofrHTTP.MatcherTrie, "/", "/", unknownRouteLabel, false}, + {"mux: static directory at / is not the catch-all", gofrHTTP.MatcherMux, "/a.txt", "/", "/{path:.*}", true}, + {"trie: static directory at / is not the catch-all", gofrHTTP.MatcherTrie, "/a.txt", "/", "/{path:.*}", true}, + } + + for i, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + t.Setenv(gofrHTTP.RouterEnvVar, tc.router) + + recorder := tracetest.NewSpanRecorder() + provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(recorder)) + + endpoints := []EndpointMapping{{Path: "/api/{path:.*}", Methods: []string{http.MethodGet}, Public: true}} + if tc.static { + endpoints = append(endpoints, EndpointMapping{Path: "/{path:.*}", Methods: []string{http.MethodGet}, Public: true}) + } + + config := newTestConfig(t, endpoints, nil) + config.Tracer = provider.Tracer("test") + + ok := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) }) + + router := gofrHTTP.NewRouter() + router.Use(Middleware(config)) + router.Add(http.MethodGet, "/", ok) + router.Add(http.MethodGet, "/api/users/{id}", ok) + + if tc.static { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "a.txt"), []byte("a"), 0o600)) + router.AddStaticFiles(logging.NewMockLogger(logging.ERROR), "/", dir) + } + + routemark.MarkCatchAll(router.PathPrefix("/")).Handler(ok) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, tc.path, http.NoBody) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if tc.static { + require.Equal(t, http.StatusOK, rec.Code, "TEST[%d], Failed.\n%s", i, tc.desc) + } + + spans := recorder.Ended() + require.Len(t, spans, 1, "TEST[%d], Failed.\n%s", i, tc.desc) + + attrs := make(map[string]string) + for _, kv := range spans[0].Attributes() { + attrs[string(kv.Key)] = kv.Value.String() + } + + assert.Equal(t, tc.wantRoute, attrs["http.route"], "TEST[%d], Failed.\n%s", i, tc.desc) + assert.Equal(t, tc.wantRule, attrs["rbac.rule"], "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} diff --git a/pkg/gofr/rbac/route_label_bench_test.go b/pkg/gofr/rbac/route_label_bench_test.go new file mode 100644 index 0000000000..b7cff7e363 --- /dev/null +++ b/pkg/gofr/rbac/route_label_bench_test.go @@ -0,0 +1,68 @@ +package rbac + +import ( + "net/http" + "net/http/httptest" + "testing" + + "go.opentelemetry.io/otel/trace/noop" + + gofrHTTP "gofr.dev/pkg/gofr/http" + "gofr.dev/pkg/gofr/internal/routemark" +) + +// BenchmarkMiddleware_Unmatched404 measures a request that only GoFr's catch-all answers, with RBAC +// on, under both routers. The catch-all check runs for every such request, so its cost shows here. +func BenchmarkMiddleware_Unmatched404(b *testing.B) { + for _, router := range []string{gofrHTTP.MatcherMux, gofrHTTP.MatcherTrie} { + b.Run(router, func(b *testing.B) { + b.Setenv(gofrHTTP.RouterEnvVar, router) + + config := newTestConfig(b, []EndpointMapping{ + {Path: "/api/{path:.*}", Methods: []string{http.MethodGet}, Public: true}, + }, nil) + config.Tracer = noop.NewTracerProvider().Tracer("bench") + + notFound := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusNotFound) }) + + r := gofrHTTP.NewRouter() + r.Use(Middleware(config)) + r.Add(http.MethodGet, "/api/users/{id}", notFound) + routemark.MarkCatchAll(r.PathPrefix("/")).Handler(notFound) + + req := httptest.NewRequestWithContext(b.Context(), http.MethodGet, "/nowhere", http.NoBody) + rec := httptest.NewRecorder() + + b.ReportAllocs() + b.ResetTimer() + + for b.Loop() { + r.ServeHTTP(rec, req) + } + }) + } +} + +// BenchmarkCatchAllAnswered measures the lookup the span label does on every request. +func BenchmarkCatchAllAnswered(b *testing.B) { + r := gofrHTTP.NewRouter() + ok := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + + var seen *http.Request + + r.Use(func(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + seen = req + next.ServeHTTP(w, req) + }) + }) + routemark.MarkCatchAll(r.PathPrefix("/")).Handler(ok) + r.ServeHTTP(httptest.NewRecorder(), httptest.NewRequestWithContext(b.Context(), http.MethodGet, "/x", http.NoBody)) + + b.ReportAllocs() + b.ResetTimer() + + for b.Loop() { + _ = routemark.CatchAllAnswered(seen) + } +} diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index 3a8efeedca..434b293d92 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -2,6 +2,7 @@ package gofr import ( "context" + "fmt" "net/http" "os" "path/filepath" @@ -263,3 +264,288 @@ func TestApp_loadRBACConfig(t *testing.T) { }) } } + +// rbacPublicStatic opens the ./static directory New() serves when this package's tests run: the +// directory's own path and every path under it. +const rbacPublicStatic = `{"path":"/static","methods":["*"],"public":true},` + + `{"path":"/static/{path:.*}","methods":["*"],"public":true}` + +// rbacGuardingPing guards the only application route, GET /ping. +const rbacGuardingPing = `{"roleHeader":"X-User-Role",` + + `"roles":[{"name":"admin","permissions":["admin:read"]}],` + + `"endpoints":[{"path":"/ping","methods":["GET"],"requiredPermissions":["admin:read"]},` + rbacPublicStatic + `]}` + +// rbacWithDeadRule adds a rule whose path has a typo, so it matches no registered route. +const rbacWithDeadRule = `{"roleHeader":"X-User-Role",` + + `"roles":[{"name":"admin","permissions":["admin:read"]}],` + + `"endpoints":[{"path":"/ping","methods":["GET"],"requiredPermissions":["admin:read"]},` + + `{"path":"/api/user/{id}","methods":["DELETE"],"requiredPermissions":["admin:read"]},` + rbacPublicStatic + `]}` + +// rbacLeavingPingUncovered has no dead rule, but no rule covers GET /ping either. +const rbacLeavingPingUncovered = `{"roleHeader":"X-User-Role",` + + `"roles":[{"name":"admin","permissions":["admin:read"]}],` + + `"endpoints":[{"path":"/.well-known/alive","methods":["GET"],"public":true},` + rbacPublicStatic + `]}` + +// rbacWithDeadPublicRule adds a public rule whose path matches no registered route. +const rbacWithDeadPublicRule = `{"roleHeader":"X-User-Role",` + + `"roles":[{"name":"admin","permissions":["admin:read"]}],` + + `"endpoints":[{"path":"/ping","methods":["GET"],"requiredPermissions":["admin:read"]},` + + `{"path":"/healthz","methods":["GET"],"public":true},` + rbacPublicStatic + `]}` + +// rbacPartlyCoveringItems guards GET /items/{id} only for numeric ids. +const rbacPartlyCoveringItems = `{"roleHeader":"X-User-Role",` + + `"roles":[{"name":"admin","permissions":["admin:read"]}],` + + `"endpoints":[{"path":"/ping","methods":["GET"],"requiredPermissions":["admin:read"]},` + + `{"path":"/items/{id:[0-9]+}","methods":["GET"],"requiredPermissions":["admin:read"]},` + rbacPublicStatic + `]}` + +// rbacGuardingPingAndAdminWithTypo guards GET /ping and GET /admin, and holds a rule with a typo. +const rbacGuardingPingAndAdminWithTypo = `{"roleHeader":"X-User-Role",` + + `"roles":[{"name":"admin","permissions":["admin:read"]}],` + + `"endpoints":[{"path":"/ping","methods":["GET"],"requiredPermissions":["admin:read"]},` + + `{"path":"/admin","methods":["GET"],"requiredPermissions":["admin:read"]},` + + `{"path":"/amdin/{id}","methods":["GET"],"requiredPermissions":["admin:read"]},` + rbacPublicStatic + `]}` + +// routeCheckFailed starts every ERROR line the route check writes. +const routeCheckFailed = "RBAC route check failed" + +// prepareHTTPServerCase is one case of TestApp_prepareHTTPServer. +type prepareHTTPServerCase struct { + desc string + config string + second string // a second config passed to EnableRBAC after config; empty installs one + mode string // GOFR_RBAC_ROUTE_CHECK; empty leaves it unset + setup func(*App) // extra registration before the check runs + wantLog []string // on stderr, where ERROR lines go + notLog []string // must not appear on stderr + wantWarn []string // on stdout, where WARN lines go + wantOutcome startupOutcome +} + +func prepareHTTPServerCases(t *testing.T) []prepareHTTPServerCase { + t.Helper() + + return []prepareHTTPServerCase{ + { + desc: "rules that all match a route log nothing", + config: rbacGuardingPing, + notLog: []string{routeCheckFailed}, + }, + { + desc: "the catch-all GoFr registers is not checked as a route", + config: rbacGuardingPing, + notLog: []string{"/{path:.*}"}, + }, + { + desc: "a dead rule is logged as an error", + config: rbacWithDeadRule, + wantLog: []string{routeCheckFailed, "DELETE /api/user/{id}"}, + }, + { + desc: "an uncovered route is logged as an error", + config: rbacLeavingPingUncovered, + wantLog: []string{routeCheckFailed, "GET /ping"}, + }, + { + desc: "warn logs a mismatch and keeps starting", + config: rbacLeavingPingUncovered, + mode: "warn", + wantLog: []string{routeCheckFailed, "GET /ping"}, + }, + { + desc: "fail logs a mismatch and stops startup", + config: rbacLeavingPingUncovered, + mode: "fail", + wantLog: []string{routeCheckFailed, "GET /ping", "GOFR_RBAC_ROUTE_CHECK=fail"}, + wantOutcome: startupFailed, + }, + { + desc: "fail keeps starting when nothing is wrong", + config: rbacGuardingPing, + mode: "fail", + notLog: []string{routeCheckFailed}, + }, + { + desc: "off skips the check", + config: rbacLeavingPingUncovered, + mode: "off", + notLog: []string{routeCheckFailed, "GET /ping"}, + }, + { + desc: "an unknown mode is reported and treated as warn", + config: rbacLeavingPingUncovered, + mode: "strict", + wantLog: []string{"invalid GOFR_RBAC_ROUTE_CHECK", "strict", routeCheckFailed, "GET /ping"}, + }, + { + desc: "a static directory at the root is checked", + config: rbacGuardingPing, + setup: func(a *App) { a.AddStaticFiles("/", t.TempDir()) }, + wantLog: []string{routeCheckFailed, "* /{path:.*}"}, + }, + { + desc: "with two configs, a route one of them guards is not reported, and a dead rule in the first is", + config: rbacGuardingPingAndAdminWithTypo, + second: rbacGuardingPing, + setup: func(a *App) { a.GET("/admin", func(*Context) (any, error) { return "ok", nil }) }, + wantLog: []string{routeCheckFailed, "config 1: GET /amdin/{id}"}, + notLog: []string{"routes covered by no rule", "routes only partly covered", "GET /admin", "config 2"}, + }, + { + desc: "with two configs, a route neither guards is reported", + config: rbacGuardingPing, + second: rbacGuardingPing, + setup: func(a *App) { a.GET("/admin", func(*Context) (any, error) { return "ok", nil }) }, + wantLog: []string{routeCheckFailed, "routes covered by no rule: GET /admin"}, + notLog: []string{"rules match no registered route"}, + }, + { + desc: "a dead public rule is a warning that does not stop startup", + config: rbacWithDeadPublicRule, + mode: "fail", + wantWarn: []string{"public rules match no registered route: GET /healthz"}, + notLog: []string{routeCheckFailed}, + }, + { + desc: "a partly covered route is logged as an error", + config: rbacPartlyCoveringItems, + setup: func(a *App) { a.GET("/items/{id}", func(*Context) (any, error) { return "ok", nil }) }, + wantLog: []string{routeCheckFailed, "routes only partly covered", "GET /items/{id} (by GET /items/{id:[0-9]+})"}, + }, + { + desc: "an application route under /.well-known is checked, GoFr's own are not", + config: rbacGuardingPing, + setup: func(a *App) { a.GET("/.well-known/custom", func(*Context) (any, error) { return "ok", nil }) }, + wantLog: []string{routeCheckFailed, "GET /.well-known/custom"}, + notLog: []string{"/.well-known/health", "/.well-known/alive", "favicon.ico"}, + }, + } +} + +func TestApp_prepareHTTPServer(t *testing.T) { + for i, tc := range prepareHTTPServerCases(t) { + t.Run(tc.desc, func(t *testing.T) { + testutil.NewServerConfigs(t) + + if tc.mode != "" { + t.Setenv("GOFR_RBAC_ROUTE_CHECK", tc.mode) + } + + outcome, stdout, stderr := runPrepareHTTPServer(t, []string{tc.config, tc.second}, tc.setup) + + assert.Equal(t, tc.wantOutcome, outcome, "TEST[%d], Failed.\n%s", i, tc.desc) + + for _, want := range tc.wantLog { + assert.Contains(t, stderr, want, "TEST[%d], Failed.\n%s", i, tc.desc) + } + + for _, unwanted := range tc.notLog { + assert.NotContains(t, stderr, unwanted, "TEST[%d], Failed.\n%s", i, tc.desc) + } + + for _, want := range tc.wantWarn { + assert.Contains(t, stdout, want, "TEST[%d], Failed.\n%s", i, tc.desc) + } + }) + } +} + +// runPrepareHTTPServer builds an app serving GET /ping with the RBAC config content, applies setup +// when it is set, and runs prepareHTTPServer, returning its outcome and what it wrote to stdout and +// stderr. +func runPrepareHTTPServer(t *testing.T, configs []string, setup func(*App)) (outcome startupOutcome, stdout, stderr string) { + t.Helper() + + var paths []string + + for i, config := range configs { + if config != "" { + paths = append(paths, writeRBACConfig(t, t.TempDir(), fmt.Sprintf("rbac%d.json", i), config)) + } + } + + stdout = testutil.StdoutOutputForFunc(func() { + stderr = testutil.StderrOutputForFunc(func() { + a := New() + a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + + if setup != nil { + setup(a) + } + + for _, path := range paths { + require.NoError(t, a.EnableRBAC(path)) + } + + outcome = a.prepareHTTPServer() + }) + }) + + return outcome, stdout, stderr +} + +// TestRun_RBACRouteCheckFailExitsNonZero checks that GOFR_RBAC_ROUTE_CHECK=fail reaches the +// process: an orchestrator reads only the exit status, so a refused start that exited 0 would be +// recorded as a success. +func TestRun_RBACRouteCheckFailExitsNonZero(t *testing.T) { + testutil.NewServerConfigs(t) + t.Setenv("GOFR_RBAC_ROUTE_CHECK", "fail") + + path := writeRBACConfig(t, t.TempDir(), "rbac.json", rbacLeavingPingUncovered) + + var codes []int + + _ = testutil.StderrOutputForFunc(func() { + a := New() + a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + require.NoError(t, a.EnableRBAC(path)) + a.exit = func(code int) { codes = append(codes, code) } + + a.Run() + }) + + assert.Equal(t, []int{exitCodeFailure}, codes) +} + +// TestRun_RBACRouteCheckFailStopsBeforeServers checks that a failed route check is the last thing a +// refused start does: the MCP port is not looked at, and no HTTP server is announced. +func TestRun_RBACRouteCheckFailStopsBeforeServers(t *testing.T) { + tests := []struct { + desc string + enableMCP bool // with an MCP_PORT that cannot be served, which fails startup if looked at + }{ + {desc: "no HTTP server is announced"}, + {desc: "the MCP port is not looked at", enableMCP: true}, + } + + for i, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + testutil.NewServerConfigs(t) + t.Setenv("GOFR_RBAC_ROUTE_CHECK", "fail") + t.Setenv("MCP_PORT", "70000") + + path := writeRBACConfig(t, t.TempDir(), "rbac.json", rbacLeavingPingUncovered) + + var stderr string + + stdout := testutil.StdoutOutputForFunc(func() { + stderr = testutil.StderrOutputForFunc(func() { + a := New() + a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + + if tc.enableMCP { + a.EnableMCP() + } + + require.NoError(t, a.EnableRBAC(path)) + a.exit = func(int) {} + + a.Run() + }) + }) + + assert.Contains(t, stderr, routeCheckFailed, "TEST[%d], Failed.\n%s", i, tc.desc) + assert.NotContains(t, stderr, "MCP server cannot start", "TEST[%d], Failed.\n%s", i, tc.desc) + assert.NotContains(t, stdout, "Registered HTTP server on port", "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} diff --git a/pkg/gofr/run.go b/pkg/gofr/run.go index 07b8802666..5f40bc2fd7 100644 --- a/pkg/gofr/run.go +++ b/pkg/gofr/run.go @@ -87,15 +87,25 @@ func (a *App) Run() { ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGINT, syscall.SIGTERM) defer stop() - // Both steps have already released what startup opened by the time they report anything but + // Each step has already released what startup opened by the time it reports anything but // startupOK. What is left is deciding what the abandonment means to the process, which happens // in one place rather than inside each step. See finishAbandonedStartup. + // + // The RBAC route check in prepareHTTPServer runs after the OnStart hooks, because a hook can + // still register a route, and before the MCP port is claimed, so a refused start has one thing + // fewer to release. if outcome := a.handleStartupHooks(ctx); outcome != startupOK { a.finishAbandonedStartup(outcome) return } + if outcome := a.prepareHTTPServer(); outcome != startupOK { + a.finishAbandonedStartup(outcome) + + return + } + if outcome := a.bindMCPServer(ctx); outcome != startupOK { a.finishAbandonedStartup(outcome) @@ -300,6 +310,41 @@ func (a *App) bindMCPServer(ctx context.Context) startupOutcome { return outcome } +// prepareHTTPServer registers GoFr's built-in routes and then checks the RBAC config against the +// complete route table, before any server starts. The application adds its routes after calling +// EnableRBAC, and the built-in ones are added only here, so this is the first point at which a rule +// that matches no route can be told apart from one whose route is still to come. +// +// What a mismatch does is set by GOFR_RBAC_ROUTE_CHECK: logged as an error while the app keeps +// starting (warn, the default), logged and startup abandoned like a failed OnStart hook (fail), or +// not looked for at all (off). +func (a *App) prepareHTTPServer() startupOutcome { + if !a.httpRegistered { + return startupOK + } + + a.httpServerSetup() + + if len(a.rbacConfigs) == 0 { + return startupOK + } + + mode := a.rbacRouteCheckMode() + if mode == rbacRouteCheckOff { + return startupOK + } + + if !a.checkRBACRoutes() || mode != rbacRouteCheckFail { + return startupOK + } + + a.Logger().Errorf("Stopping startup: the RBAC route check failed and %s=%s.", rbacRouteCheckKey, rbacRouteCheckFail) + + a.shutdownAfterFailedStartup() + + return startupFailed +} + // shutdownAfterFailedStartup releases what startup has already opened when the run is abandoned // before any server is up — a failed startup hook, or an MCP port that cannot be claimed. Run // returns normally afterwards, so without this the datasource connections opened by the container @@ -361,7 +406,10 @@ func (a *App) startMetricsServer(wg *sync.WaitGroup) { func (a *App) startHTTPServer(wg *sync.WaitGroup) { if a.httpRegistered { wg.Add(1) - a.httpServerSetup() + + if a.container.Logger != nil { + a.container.Logger.Infof("Registered HTTP server on port: %d", a.httpServer.port) + } go func(s *httpServer) { defer wg.Done()