From c0d4b28b891a30fcabe69210ccc1d3925f85d60b Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Sun, 27 Sep 2026 00:43:56 +0530 Subject: [PATCH 01/11] feat(rbac): check rules against registered routes at startup A rule that matches no registered route (usually a path typo) leaves the route it was written for unguarded. Run now checks the RBAC config against the full route table before any server starts: - a dead rule stops startup under EnableRBACWithError, and is logged at ERROR under the deprecated EnableRBAC; - routes no rule covers are listed in one WARN line. The rbac.authorize span's http.route is now the router-matched template; the governing rule's path moves to a new rbac.rule attribute. Refs #3935 --- docs/advanced-guide/rbac/page.md | 41 ++++- pkg/gofr/gofr.go | 7 + pkg/gofr/rbac.go | 3 + pkg/gofr/rbac/coverage.go | 267 +++++++++++++++++++++++++++++++ pkg/gofr/rbac/coverage_test.go | 157 ++++++++++++++++++ pkg/gofr/rbac/middleware.go | 19 ++- pkg/gofr/rbac/middleware_test.go | 59 ++++++- pkg/gofr/rbac/resolver_test.go | 37 +++++ pkg/gofr/rbac_test.go | 79 +++++++++ pkg/gofr/run.go | 46 +++++- 10 files changed, 711 insertions(+), 4 deletions(-) create mode 100644 pkg/gofr/rbac/coverage.go create mode 100644 pkg/gofr/rbac/coverage_test.go diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index 9f2efd3b42..1c73f12d61 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -302,6 +302,43 @@ 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 reports two kinds of mismatch. + +**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 neither its path nor its method can match any +registered route. A rule whose method the route does not register (`DELETE` on a route that only +has `GET`) is dead too. + +- With `EnableRBACWithError`, a dead rule **stops startup**: the app logs the error, releases + what it opened, and exits with a non-zero status, the same as a failed `OnStart` hook. +- With the deprecated `EnableRBAC`, the app logs the same error and keeps running. +- A dead rule marked `"public": true` never stops startup. It cannot leave a route unprotected, + so it is reported only as a warning. + +``` +RBAC route check failed: RBAC rules match no registered route: DELETE /api/user/{id}. Fix the +rule paths or methods in the RBAC config, or remove the rules. +``` + +**An uncovered route** is a registered route that no rule matches, so it is served without role +checks (see [Unmatched Routes Behavior](#unmatched-routes-behavior)). This is often on purpose, so +it is reported as one warning line and never stops startup. GoFr's own `/.well-known/*` routes and +`/favicon.ico` are left out of it. + +``` +RBAC: 2 registered route(s) are not covered by any rule and are served without role checks: +GET /api/posts, POST /api/users +``` + +The check errs toward "this rule might match". Two path variables with different constraints — +`{id:[0-9]+}` in the rule and `{id:[a-z]+}` in the route — are treated as matching, even though no +request could satisfy both, so a rule like that is not reported as dead. + ## JWT-Based RBAC For production/public APIs, use JWT-based role extraction: @@ -630,7 +667,9 @@ 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. - ✅ Authorization status (allowed/denied) included - ❌ Roles excluded (privacy protection - roles are PII) - ❌ Error messages sanitized (prevent information leakage) diff --git a/pkg/gofr/gofr.go b/pkg/gofr/gofr.go index 898b1fb2c1..c0f0e8031c 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -22,6 +22,7 @@ import ( "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" ) @@ -79,6 +80,12 @@ type App struct { // same point it reports a port it could not claim. See bindMCPServer. mcpConfigErr error + // rbacConfig is the RBAC config installed by EnableRBACWithError, kept so Run can check it + // against the complete route table. rbacStrict is false when it came from the deprecated + // EnableRBAC, which logs a failed check instead of stopping startup. See prepareHTTPServer. + rbacConfig *rbac.Config + rbacStrict bool + // exit is os.Exit, indirected so a test can observe the status a failed startup reports without // taking the test binary down with it. Nil means os.Exit, which is what every real app uses. exit func(int) diff --git a/pkg/gofr/rbac.go b/pkg/gofr/rbac.go index 19e3896dec..75036cdf5d 100644 --- a/pkg/gofr/rbac.go +++ b/pkg/gofr/rbac.go @@ -71,5 +71,8 @@ func (a *App) enableRBAC(configPath ...string) error { middlewareFunc := rbac.Middleware(config) a.UseMiddleware(middlewareFunc) + a.rbacConfig = config + a.rbacStrict = true + return nil } diff --git a/pkg/gofr/rbac/coverage.go b/pkg/gofr/rbac/coverage.go new file mode 100644 index 0000000000..a706946a83 --- /dev/null +++ b/pkg/gofr/rbac/coverage.go @@ -0,0 +1,267 @@ +package rbac + +import ( + "errors" + "fmt" + "regexp" + "sort" + "strings" + + "github.com/gorilla/mux" +) + +// ErrDeadRules is returned by [Config.CheckRoutes] when a rule that requires permissions matches no +// registered route. Such a rule protects nothing, and it is usually a typo that leaves the route it +// was written for unguarded. +var ErrDeadRules = errors.New("RBAC rules match no registered route") + +// 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 = "*" + +// registeredRoute is one (method, path) pair the router serves, as the coverage check sees it. +type registeredRoute struct { + // method is the upper-cased method, or anyMethod for a route with no method matcher. + method string + + // template is the path template. A PathPrefix route carries a trailing catch-all segment, so + // that it overlaps every path under the prefix. + template string + + // label is how the route is reported: "METHOD /path". + label string +} + +// CheckRoutes compares the config against the routes registered on router, which must hold the +// complete route table. +// +// A registered route that no rule covers is served without role checks. That is often intended, +// so the routes are reported in one warning line and are not an error. GoFr's own /.well-known/* +// routes and /favicon.ico are left out of it. +// +// A rule that matches no registered route - usually a typo in its path, or a method the route does +// not register - is returned as an error wrapping [ErrDeadRules] that lists every such rule. A dead +// public rule cannot leave a route unguarded, so it is only warned about. +// +// The PathPrefix("/") catch-all is ignored: it would otherwise make every rule look live. +func (c *Config) CheckRoutes(router *mux.Router) error { + if router == nil { + return nil + } + + dead, uncovered := c.routeCoverage(collectRoutes(router)) + + var deadGuards, deadPublic []string + + for i := range dead { + label := dead[i].method + " " + dead[i].pattern + if dead[i].isPublic { + deadPublic = append(deadPublic, label) + } else { + deadGuards = append(deadGuards, label) + } + } + + uncoveredLabels := make([]string, 0, len(uncovered)) + for i := range uncovered { + uncoveredLabels = append(uncoveredLabels, uncovered[i].label) + } + + sort.Strings(deadGuards) + sort.Strings(deadPublic) + sort.Strings(uncoveredLabels) + + if c.Logger != nil { + if len(uncoveredLabels) > 0 { + c.Logger.Warnf("RBAC: %d registered route(s) are not covered by any rule and are served without role checks: %s", + len(uncoveredLabels), strings.Join(uncoveredLabels, ", ")) + } + + if len(deadPublic) > 0 { + c.Logger.Warnf("RBAC: public rule(s) match no registered route: %s", strings.Join(deadPublic, ", ")) + } + } + + if len(deadGuards) > 0 { + return fmt.Errorf("%w: %s", ErrDeadRules, strings.Join(deadGuards, ", ")) + } + + return nil +} + +// routeCoverage splits the config against routes into the rules that match none of them and the +// routes that no rule matches. +func (c *Config) routeCoverage(routes []registeredRoute) (dead []endpointRule, uncovered []registeredRoute) { + for i := range c.rules { + live := false + + for j := range routes { + if ruleMayMatchRoute(&c.rules[i], &routes[j]) { + live = true + + break + } + } + + if !live { + dead = append(dead, c.rules[i]) + } + } + + for j := range routes { + if isBuiltInRoute(routes[j].template) { + continue + } + + covered := false + + for i := range c.rules { + if ruleMayMatchRoute(&c.rules[i], &routes[j]) { + covered = true + + break + } + } + + if !covered { + uncovered = append(uncovered, routes[j]) + } + } + + return dead, uncovered +} + +// ruleMayMatchRoute reports whether some request could be served by route and governed by rule. +func ruleMayMatchRoute(rule *endpointRule, route *registeredRoute) bool { + methodsMatch := rule.method == anyMethod || route.method == anyMethod || strings.EqualFold(rule.method, route.method) + + return methodsMatch && patternsMayOverlap(rule.pattern, route.template) +} + +// isBuiltInRoute reports whether a template belongs to a route GoFr registers itself, which is left +// out of the uncovered-route report because the application did not write it. +func isBuiltInRoute(template string) bool { + return strings.HasPrefix(template, "/.well-known/") || template == "/favicon.ico" +} + +// collectRoutes lists every (method, path) pair router serves, skipping the PathPrefix("/") +// catch-all and routes with no path. Duplicates collapse to one entry. +func collectRoutes(router *mux.Router) []registeredRoute { + var routes []registeredRoute + + seen := make(map[string]bool) + + _ = router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error { + tmpl, err := route.GetPathTemplate() + if err != nil { + return nil + } + + // mux anchors a Path regexp with "$" and leaves a PathPrefix one open. + pathRegexp, _ := route.GetPathRegexp() + isPrefix := !strings.HasSuffix(pathRegexp, "$") + + if isPrefix && tmpl == "/" { + return nil + } + + template := tmpl + if isPrefix { + template = strings.TrimSuffix(tmpl, "/") + "/{rest:.*}" + } + + methods, err := route.GetMethods() + if err != nil || len(methods) == 0 { + methods = []string{anyMethod} + } + + for _, method := range methods { + method = strings.ToUpper(method) + + label := method + " " + tmpl + if seen[label] { + continue + } + + seen[label] = true + + routes = append(routes, registeredRoute{method: method, template: template, label: label}) + } + + return nil + }) + + return routes +} + +// patternsMayOverlap reports whether a request path could match both the rule pattern and the +// route template. Both use mux syntax. +// +// It errs toward "yes", because a wrong "no" reports a live rule as dead, and that stops startup. +// Two variables always overlap, even when their constraints could never agree, and a constraint +// that does not compile overlaps anything. The cost is that such a rule can go unreported. +func patternsMayOverlap(rule, template string) bool { + if rule == "" || template == "" { + return false + } + + a := splitPatternSegments(strings.Trim(rule, "/")) + b := splitPatternSegments(strings.Trim(template, "/")) + + for i := 0; ; i++ { + // A catch-all absorbs the rest of the other path, including nothing at all. + if isCatchAllAt(a, i) || isCatchAllAt(b, i) { + return true + } + + if i == len(a) || i == len(b) { + return len(a) == len(b) + } + + if !segmentsMayOverlap(a[i], b[i]) { + return false + } + } +} + +// segmentsMayOverlap reports whether one path segment could match both x and y. +func segmentsMayOverlap(x, y string) bool { + xVar, yVar := isVariableSegment(x), isVariableSegment(y) + + switch { + case xVar && yVar: + return true + case xVar: + return variableMayMatch(x, y) + case yVar: + return variableMayMatch(y, x) + default: + return x == y + } +} + +// variableMayMatch reports whether the "{name}" or "{name:regex}" segment could match literal. +func variableMayMatch(variable, literal string) bool { + inner := variable[1 : len(variable)-1] + + idx := strings.Index(inner, ":") + if idx < 0 { + return true + } + + re, err := regexp.Compile("^(?:" + inner[idx+1:] + ")$") + if err != nil { + return true + } + + return re.MatchString(literal) +} + +func isVariableSegment(segment string) bool { + return strings.HasPrefix(segment, "{") && strings.HasSuffix(segment, "}") +} + +// isCatchAllAt reports whether segments has a catch-all variable at index i. +func isCatchAllAt(segments []string, i int) bool { + return i < len(segments) && isVariableSegment(segments[i]) && isCatchAllVariable(segments[i]) +} diff --git a/pkg/gofr/rbac/coverage_test.go b/pkg/gofr/rbac/coverage_test.go new file mode 100644 index 0000000000..4ba2421a33 --- /dev/null +++ b/pkg/gofr/rbac/coverage_test.go @@ -0,0 +1,157 @@ +package rbac + +import ( + "net/http" + "strings" + "testing" + + "github.com/gorilla/mux" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newCoverageRouter builds a router shaped like GoFr's: one route per method and path, the built-in +// well-known routes, a static-file prefix, and the PathPrefix("/") catch-all registered last. +func newCoverageRouter() *mux.Router { + noop := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) + router := mux.NewRouter() + + for _, r := range []struct{ method, path string }{ + {http.MethodGet, "/api/users"}, + {http.MethodPost, "/api/users"}, + {http.MethodGet, "/api/users/{id}"}, + {http.MethodDelete, "/api/users/{id}"}, + {http.MethodGet, "/api/posts"}, + {http.MethodGet, "/.well-known/health"}, + {http.MethodGet, "/.well-known/alive"}, + {http.MethodGet, "/favicon.ico"}, + } { + router.NewRoute().Methods(r.method).Path(r.path).Handler(noop) + } + + router.NewRoute().PathPrefix("/static/").Handler(noop) + router.PathPrefix("/").Handler(noop) + + return router +} + +// guard is a non-public rule requiring one permission. +func guard(path string, methods ...string) EndpointMapping { + return EndpointMapping{Path: path, Methods: methods, RequiredPermissions: []string{"p:x"}} +} + +// coveringRules guards every non-built-in route in newCoverageRouter. +func coveringRules() []EndpointMapping { + return []EndpointMapping{ + guard("/api/users", http.MethodGet, http.MethodPost), + guard("/api/users/{id}", http.MethodGet, http.MethodDelete), + guard("/api/posts", http.MethodGet), + guard("/static/{path:.*}", "*"), + } +} + +func TestConfig_CheckRoutes(t *testing.T) { + testCases := []struct { + desc string + endpoints []EndpointMapping + wantDead []string // "METHOD path" entries expected in the returned error + wantWarn []string // substrings expected in the warning output + notInWarn []string // substrings that must not appear in the warning output + }{ + { + desc: "every route covered and every rule live", + endpoints: coveringRules(), + notInWarn: []string{"GET", "POST", "DELETE"}, + }, + { + desc: "a typo in a path segment is a dead rule", + endpoints: append(coveringRules(), + guard("/api/user/{id}", http.MethodDelete)), + wantDead: []string{"DELETE /api/user/{id}"}, + }, + { + desc: "a method the route does not register is a dead rule", + endpoints: []EndpointMapping{ + guard("/api/posts", http.MethodGet, http.MethodDelete), + }, + wantDead: []string{"DELETE /api/posts"}, + }, + { + desc: "a wildcard-method rule is live when the path matches", + endpoints: []EndpointMapping{ + guard("/api/posts", "*"), + guard("/api/users/{id}"), + }, + }, + { + desc: "a rule under a static prefix is live", + endpoints: []EndpointMapping{ + guard("/static/app.js", http.MethodGet), + }, + }, + { + desc: "the PathPrefix catch-all does not make a rule live", + endpoints: []EndpointMapping{ + guard("/nowhere", http.MethodGet), + }, + wantDead: []string{"GET /nowhere"}, + }, + { + desc: "a rule for a built-in route is live", + endpoints: []EndpointMapping{ + {Path: "/.well-known/health", Methods: []string{http.MethodGet}, Public: true}, + }, + }, + { + desc: "a dead public rule is a warning, not an error", + endpoints: append(coveringRules(), + EndpointMapping{Path: "/helth", Methods: []string{http.MethodGet}, Public: true}), + wantWarn: []string{"GET /helth"}, + }, + { + desc: "uncovered routes are listed in one warning, built-in routes left out", + endpoints: []EndpointMapping{ + guard("/api/users", http.MethodGet), + guard("/api/users/{id}", "*"), + guard("/static/{path:.*}", "*"), + }, + wantWarn: []string{"2 registered route(s)", "POST /api/users", "GET /api/posts"}, + notInWarn: []string{"/.well-known", "/favicon.ico", "GET /api/users,", "/api/users/{id}"}, + }, + { + desc: "an uncovered static prefix is reported", + endpoints: coveringRules()[:3], + wantWarn: []string{"* /static/"}, + }, + } + + for i, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + logger := &mockLogger{} + config := newTestConfig(t, tc.endpoints, nil) + config.Logger = logger + + err := config.CheckRoutes(newCoverageRouter()) + + if len(tc.wantDead) == 0 { + require.NoError(t, err, "TEST[%d], Failed.\n%s", i, tc.desc) + } else { + require.ErrorIs(t, err, ErrDeadRules, "TEST[%d], Failed.\n%s", i, tc.desc) + + for _, dead := range tc.wantDead { + assert.Contains(t, err.Error(), dead, "TEST[%d], Failed.\n%s", i, tc.desc) + } + } + + warnings := strings.Join(logger.warnLogs, "\n") + + for _, want := range tc.wantWarn { + assert.Contains(t, warnings, want, "TEST[%d], Failed.\n%s", i, tc.desc) + } + + for _, unwanted := range tc.notInWarn { + assert.NotContains(t, warnings, unwanted, "TEST[%d], Failed.\n%s", i, tc.desc) + } + }) + } +} diff --git a/pkg/gofr/rbac/middleware.go b/pkg/gofr/rbac/middleware.go index 8e0a910aba..1f27beee78 100644 --- a/pkg/gofr/rbac/middleware.go +++ b/pkg/gofr/rbac/middleware.go @@ -14,6 +14,7 @@ import ( "go.opentelemetry.io/otel/trace" "gofr.dev/pkg/gofr/datasource" + gofrHTTP "gofr.dev/pkg/gofr/http" "gofr.dev/pkg/gofr/http/middleware" ) @@ -23,6 +24,21 @@ 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. +// +// "/" is reported as unmatched: it is the template of GoFr's PathPrefix("/") catch-all, which +// serves every request no route matched. +func matchedRouteLabel(r *http.Request) string { + tmpl := gofrHTTP.RouteTemplate(r) + if tmpl == "" || tmpl == "/" { + return unknownRouteLabel + } + + return tmpl +} + // AuditLog represents a structured log entry for RBAC authorization decisions. // It follows the same pattern as HTTP RequestLog for consistency. type AuditLog struct { @@ -104,7 +120,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 4804b1810a..7803bdc48b 100644 --- a/pkg/gofr/rbac/middleware_test.go +++ b/pkg/gofr/rbac/middleware_test.go @@ -10,9 +10,12 @@ import ( "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" "gofr.dev/pkg/gofr/datasource" + gofrHTTP "gofr.dev/pkg/gofr/http" "gofr.dev/pkg/gofr/http/middleware" ) @@ -860,6 +863,7 @@ func TestHandleAuthError(t *testing.T) { // mockLogger implements the datasource.Logger interface for testing. type mockLogger struct { errorLogs []string + warnLogs []string infoLogs []string // Capture actual log messages logs []string infoArgs []any // Capture structured log arguments (used for both Info and Debug) @@ -891,7 +895,10 @@ func (m *mockLogger) Errorf(format string, args ...any) { func (m *mockLogger) Warn(_ ...any) { m.logs = append(m.logs, "WARN") } -func (m *mockLogger) Warnf(_ string, _ ...any) { m.logs = append(m.logs, "WARNF") } +func (m *mockLogger) Warnf(format string, args ...any) { + m.logs = append(m.logs, "WARNF") + m.warnLogs = append(m.warnLogs, fmt.Sprintf(format, args...)) +} func TestMiddleware_WithTracing(t *testing.T) { t.Run("starts tracing when tracer is available", func(t *testing.T) { @@ -1060,3 +1067,53 @@ func TestSanitizeErrorForTrace(t *testing.T) { assert.NotContains(t, sanitized.Error(), "secret key", "sensitive information should be removed") }) } + +func TestMiddleware_TraceRouteLabels(t *testing.T) { + testCases := []struct { + desc string + router string + path string + wantRoute string + wantRule string + }{ + {"mux: route and rule differ", gofrHTTP.MatcherMux, "/api/users/42", "/api/users/{id}", "/api/{path:.*}"}, + {"trie: route and rule differ", gofrHTTP.MatcherTrie, "/api/users/42", "/api/users/{id}", "/api/{path:.*}"}, + {"mux: no route and no rule", gofrHTTP.MatcherMux, "/nowhere", unknownRouteLabel, unknownRouteLabel}, + {"trie: no route and no rule", gofrHTTP.MatcherTrie, "/nowhere", unknownRouteLabel, unknownRouteLabel}, + } + + 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)) + + config := newTestConfig(t, []EndpointMapping{ + {Path: "/api/{path:.*}", Methods: []string{http.MethodGet}, Public: true}, + }, 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, "/api/users/{id}", ok) + router.PathPrefix("/").Handler(ok) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, tc.path, http.NoBody) + router.ServeHTTP(httptest.NewRecorder(), req) + + 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/resolver_test.go b/pkg/gofr/rbac/resolver_test.go index 41e3e41236..714653b45f 100644 --- a/pkg/gofr/rbac/resolver_test.go +++ b/pkg/gofr/rbac/resolver_test.go @@ -446,3 +446,40 @@ func TestGetEndpointForRequest_SlashConstraintDoesNotOutrankNarrowRule(t *testin assert.Equal(t, "/files/private/{name}", endpoint.Path) } } + +func Test_patternsMayOverlap(t *testing.T) { + testCases := []struct { + desc string + rule string + template string + want bool + }{ + {"identical literals", "/api/users", "/api/users", true}, + {"different literals", "/api/users", "/api/posts", false}, + {"typo in a literal segment", "/api/user/{id}", "/api/users/{id}", false}, + {"free variable against literal", "/api/users/{id}", "/api/users/me", true}, + {"literal against free variable", "/api/users/me", "/api/users/{id}", true}, + {"constraint admits the literal", "/api/users/{id:[0-9]+}", "/api/users/42", true}, + {"constraint rejects the literal", "/api/users/{id:[0-9]+}", "/api/users/me", false}, + {"route constraint rejects the literal", "/api/users/me", "/api/users/{id:[0-9]+}", false}, + {"variable against variable", "/api/users/{id:[0-9]+}", "/api/users/{id:[a-z0-9]+}", true}, + {"more segments in the rule", "/api/users/{id}/posts", "/api/users/{id}", false}, + {"fewer segments in the rule", "/api", "/api/users", false}, + {"catch-all in the rule absorbs the rest", "/api/{path:.*}", "/api/users/{id}/posts", true}, + {"catch-all in the rule absorbs nothing", "/api/{path:.*}", "/api", true}, + {"catch-all in the route absorbs the rest", "/static/css/site.css", "/static/{path:.*}", true}, + {"catch-all after a different literal", "/admin/{path:.*}", "/api/users", false}, + {"trailing slash is ignored", "/api/users/", "/api/users", true}, + {"root against root", "/", "/", true}, + {"root against a path", "/", "/api", false}, + {"constraint containing a slash spans segments", "/files/{path:[a-z/]+}", "/files/a/b/c", true}, + {"uncompilable constraint is assumed to overlap", "/api/{id:[}", "/api/users", true}, + {"empty rule matches nothing", "", "/api", false}, + } + + for i, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + assert.Equal(t, tc.want, patternsMayOverlap(tc.rule, tc.template), "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index 1c0b1110d4..b538fdb8af 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -154,3 +154,82 @@ func TestEnableRBAC(t *testing.T) { }) } } + +// 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"]}]}` + +// 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"]}]}` + +func TestApp_prepareHTTPServer(t *testing.T) { + tests := []struct { + desc string + config string + deprecated bool // EnableRBAC instead of EnableRBACWithError + wantOutcome startupOutcome + wantLog string + }{ + {desc: "rules that all match a route start the app", config: rbacGuardingPing, wantOutcome: startupOK}, + {desc: "a dead rule stops startup under EnableRBACWithError", config: rbacWithDeadRule, + wantOutcome: startupFailed, wantLog: "DELETE /api/user/{id}"}, + {desc: "a dead rule is logged under the deprecated EnableRBAC", config: rbacWithDeadRule, deprecated: true, + wantOutcome: startupOK, wantLog: "DELETE /api/user/{id}"}, + } + + for i, tc := range tests { + t.Run(tc.desc, func(t *testing.T) { + testutil.NewServerConfigs(t) + + path := writeRBACConfig(t, t.TempDir(), "rbac.json", tc.config) + + var outcome startupOutcome + + logs := testutil.StderrOutputForFunc(func() { + a := New() + a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + + if tc.deprecated { + a.EnableRBAC(path) + } else { + require.NoError(t, a.EnableRBACWithError(path)) + } + + outcome = a.prepareHTTPServer() + }) + + assert.Equal(t, tc.wantOutcome, outcome, "TEST[%d], Failed.\n%s", i, tc.desc) + + if tc.wantLog == "" { + assert.NotContains(t, logs, "RBAC route check failed", "TEST[%d], Failed.\n%s", i, tc.desc) + } else { + assert.Contains(t, logs, tc.wantLog, "TEST[%d], Failed.\n%s", i, tc.desc) + } + }) + } +} + +// TestRun_RBACDeadRuleExitsNonZero covers the process-level outcome: a dead rule under +// EnableRBACWithError must be reported to the orchestrator as a failed start, not as a clean exit. +func TestRun_RBACDeadRuleExitsNonZero(t *testing.T) { + testutil.NewServerConfigs(t) + + path := writeRBACConfig(t, t.TempDir(), "rbac.json", rbacWithDeadRule) + + var codes []int + + _ = testutil.StderrOutputForFunc(func() { + app := New() + app.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + require.NoError(t, app.EnableRBACWithError(path)) + app.exit = func(code int) { codes = append(codes, code) } + + app.Run() + }) + + require.Equal(t, []int{exitCodeStartupFailed}, codes, "a dead RBAC rule must stop startup") +} diff --git a/pkg/gofr/run.go b/pkg/gofr/run.go index e5af034568..917b37a79a 100644 --- a/pkg/gofr/run.go +++ b/pkg/gofr/run.go @@ -84,6 +84,12 @@ func (a *App) Run() { return } + if outcome := a.prepareHTTPServer(); outcome != startupOK { + a.finishAbandonedStartup(outcome) + + return + } + timeout, err := getShutdownTimeoutFromConfig(a.Config) if err != nil { a.Logger().Errorf("error parsing value of shutdown timeout from config: %v. Setting default timeout of 30 sec.", err) @@ -282,6 +288,45 @@ 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. +// +// A rule that matches no route stops startup when RBAC was enabled with EnableRBACWithError: the +// caller asked to be told when authorization cannot be enforced as configured, and a typo in a rule +// leaves the route it was written for unguarded. The deprecated EnableRBAC logs and carries on. +func (a *App) prepareHTTPServer() startupOutcome { + if !a.httpRegistered { + return startupOK + } + + a.httpServerSetup() + + if a.rbacConfig == nil { + return startupOK + } + + err := a.rbacConfig.CheckRoutes(&a.httpServer.router.Router) + if err == nil { + return startupOK + } + + if !a.rbacStrict { + a.Logger().Errorf("RBAC route check failed: %v. These rules protect nothing, so a route they were meant "+ + "to guard is served without role checks. Use EnableRBACWithError to stop startup instead.", err) + + return startupOK + } + + a.Logger().Errorf("RBAC route check failed: %v. Fix the rule paths or methods in the RBAC config, "+ + "or remove the rules.", err) + + 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 @@ -343,7 +388,6 @@ func (a *App) startMetricsServer(wg *sync.WaitGroup) { func (a *App) startHTTPServer(wg *sync.WaitGroup) { if a.httpRegistered { wg.Add(1) - a.httpServerSetup() go func(s *httpServer) { defer wg.Done() From 41977662ee97c26821106075abd36a66ca74f38c Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Sun, 27 Sep 2026 00:54:05 +0530 Subject: [PATCH 02/11] fix(rbac): satisfy goconst and typos in the route coverage check --- pkg/gofr/rbac/coverage.go | 5 ++++- pkg/gofr/rbac/coverage_test.go | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/pkg/gofr/rbac/coverage.go b/pkg/gofr/rbac/coverage.go index a706946a83..f6c29acc60 100644 --- a/pkg/gofr/rbac/coverage.go +++ b/pkg/gofr/rbac/coverage.go @@ -19,6 +19,9 @@ var ErrDeadRules = errors.New("RBAC rules match no registered route") // without a method matcher, such as a static-file prefix. const anyMethod = "*" +// faviconPath is the built-in favicon route GoFr registers itself. +const faviconPath = "/favicon.ico" + // registeredRoute is one (method, path) pair the router serves, as the coverage check sees it. type registeredRoute struct { // method is the upper-cased method, or anyMethod for a route with no method matcher. @@ -141,7 +144,7 @@ func ruleMayMatchRoute(rule *endpointRule, route *registeredRoute) bool { // isBuiltInRoute reports whether a template belongs to a route GoFr registers itself, which is left // out of the uncovered-route report because the application did not write it. func isBuiltInRoute(template string) bool { - return strings.HasPrefix(template, "/.well-known/") || template == "/favicon.ico" + return strings.HasPrefix(template, "/.well-known/") || template == faviconPath } // collectRoutes lists every (method, path) pair router serves, skipping the PathPrefix("/") diff --git a/pkg/gofr/rbac/coverage_test.go b/pkg/gofr/rbac/coverage_test.go index 4ba2421a33..4a9d288f99 100644 --- a/pkg/gofr/rbac/coverage_test.go +++ b/pkg/gofr/rbac/coverage_test.go @@ -24,7 +24,7 @@ func newCoverageRouter() *mux.Router { {http.MethodGet, "/api/posts"}, {http.MethodGet, "/.well-known/health"}, {http.MethodGet, "/.well-known/alive"}, - {http.MethodGet, "/favicon.ico"}, + {http.MethodGet, faviconPath}, } { router.NewRoute().Methods(r.method).Path(r.path).Handler(noop) } @@ -105,8 +105,8 @@ func TestConfig_CheckRoutes(t *testing.T) { { desc: "a dead public rule is a warning, not an error", endpoints: append(coveringRules(), - EndpointMapping{Path: "/helth", Methods: []string{http.MethodGet}, Public: true}), - wantWarn: []string{"GET /helth"}, + EndpointMapping{Path: "/healthz", Methods: []string{http.MethodGet}, Public: true}), + wantWarn: []string{"GET /healthz"}, }, { desc: "uncovered routes are listed in one warning, built-in routes left out", @@ -116,7 +116,7 @@ func TestConfig_CheckRoutes(t *testing.T) { guard("/static/{path:.*}", "*"), }, wantWarn: []string{"2 registered route(s)", "POST /api/users", "GET /api/posts"}, - notInWarn: []string{"/.well-known", "/favicon.ico", "GET /api/users,", "/api/users/{id}"}, + notInWarn: []string{"/.well-known", faviconPath, "GET /api/users,", "/api/users/{id}"}, }, { desc: "an uncovered static prefix is reported", From 441fcb6dffe0c0ac65e640f4662780bd400f6121 Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Sun, 27 Sep 2026 01:05:11 +0530 Subject: [PATCH 03/11] feat(rbac): stop startup when a registered route has no rule A route no RBAC rule covers is served without role checks. Under EnableRBACWithError this now stops startup, the same as a rule that matches no route; a route meant to be open is declared with "public": true. The deprecated EnableRBAC logs a WARN line instead. CheckRoutes returns ErrDeadRules and/or ErrUncoveredRoutes joined with errors.Join. GoFr's own /.well-known/* routes and /favicon.ico stay exempt. Refs #3935 --- docs/advanced-guide/rbac/page.md | 49 ++++++++------ pkg/gofr/rbac/coverage.go | 42 +++++++----- pkg/gofr/rbac/coverage_test.go | 109 ++++++++++++++++++------------- pkg/gofr/rbac_test.go | 45 +++++++++---- pkg/gofr/run.go | 32 +++++++-- 5 files changed, 175 insertions(+), 102 deletions(-) diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index 1c73f12d61..cfb65e963e 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -306,38 +306,44 @@ Treat that line as an open route, not a warning about a typo. 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 reports two kinds of mismatch. +them and reports two kinds of mismatch. With `EnableRBACWithError`, either one **stops startup**: +the app logs the error, releases what it opened, and exits with a non-zero status, the same as a +failed `OnStart` hook. With the deprecated `EnableRBAC`, the app logs them and keeps running. **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 neither its path nor its method can match any registered route. A rule whose method the route does not register (`DELETE` on a route that only -has `GET`) is dead too. - -- With `EnableRBACWithError`, a dead rule **stops startup**: the app logs the error, releases - what it opened, and exits with a non-zero status, the same as a failed `OnStart` hook. -- With the deprecated `EnableRBAC`, the app logs the same error and keeps running. -- A dead rule marked `"public": true` never stops startup. It cannot leave a route unprotected, - so it is reported only as a warning. +has `GET`) is dead too. A dead rule marked `"public": true` never stops startup: it cannot leave a +route unprotected, so it is reported only as a warning. ``` -RBAC route check failed: RBAC rules match no registered route: DELETE /api/user/{id}. Fix the -rule paths or methods in the RBAC config, or remove the rules. +RBAC route check failed: RBAC rules match no registered route: DELETE /api/user/{id}. Fix each +rule's path or methods, or remove it. ``` -**An uncovered route** is a registered route that no rule matches, so it is served without role -checks (see [Unmatched Routes Behavior](#unmatched-routes-behavior)). This is often on purpose, so -it is reported as one warning line and never stops startup. GoFr's own `/.well-known/*` routes and -`/favicon.ico` are left out of 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`. +GoFr's own `/.well-known/*` routes and `/favicon.ico` are exempt. Other routes GoFr registers for +you do need a rule: the `./static` directory (served at `/static/` when it exists) and `/graphql` +when GraphQL is enabled. ``` -RBAC: 2 registered route(s) are not covered by any rule and are served without role checks: -GET /api/posts, POST /api/users +RBAC route check failed: registered routes are covered by no RBAC rule: GET /api/posts, +POST /api/users. Add a rule for each, with "public": true for a route meant to be open. +``` + +```json +{"path": "/login", "methods": ["POST"], "public": true}, +{"path": "/static/{path:.*}", "methods": ["*"], "public": true} ``` The check errs toward "this rule might match". Two path variables with different constraints — `{id:[0-9]+}` in the rule and `{id:[a-z]+}` in the route — are treated as matching, even though no -request could satisfy both, so a rule like that is not reported as dead. +request could satisfy both, so a rule like that is not reported as dead. For the same reason a route +counts as covered when a rule matches only some of its requests: a rule for `/api/users/{id:[0-9]+}` +covers the route `/api/users/{id}`, although a request for `/api/users/abc` is not governed by it. ## JWT-Based RBAC @@ -655,10 +661,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 +With `EnableRBACWithError`, the app in this example does not start: `POST /api/users` and +`GET /api/posts` are registered but covered by no rule (see [Startup Route Check](#startup-route-check)). +Every registered route needs a rule, and a route meant to be open gets one with `"public": true`. +The runtime behavior above still applies to a request no rule matches, for example one that only +part of a pattern covers. ## Security and Privacy diff --git a/pkg/gofr/rbac/coverage.go b/pkg/gofr/rbac/coverage.go index f6c29acc60..f70cc2acdb 100644 --- a/pkg/gofr/rbac/coverage.go +++ b/pkg/gofr/rbac/coverage.go @@ -15,6 +15,11 @@ import ( // was written for unguarded. var ErrDeadRules = errors.New("RBAC rules match no registered route") +// ErrUncoveredRoutes is returned by [Config.CheckRoutes] when a registered route is covered by no +// rule. Such a route is served without role checks; one meant to be open is declared with +// "public": true. +var ErrUncoveredRoutes = errors.New("registered routes are covered by no RBAC rule") + // 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 = "*" @@ -38,13 +43,15 @@ type registeredRoute struct { // CheckRoutes compares the config against the routes registered on router, which must hold the // complete route table. // -// A registered route that no rule covers is served without role checks. That is often intended, -// so the routes are reported in one warning line and are not an error. GoFr's own /.well-known/* -// routes and /favicon.ico are left out of it. +// It reports two kinds of mismatch, each listing its entries as "METHOD path", and joins them with +// [errors.Join] when both are present, so the caller can tell them apart with errors.Is: // -// A rule that matches no registered route - usually a typo in its path, or a method the route does -// not register - is returned as an error wrapping [ErrDeadRules] that lists every such rule. A dead -// public rule cannot leave a route unguarded, so it is only warned about. +// - [ErrDeadRules]: a rule matches no registered route - usually a typo in its path, or a method +// the route does not register - so the route it was written for is unguarded. A dead public rule +// cannot leave a route unguarded, so it is only warned about. +// - [ErrUncoveredRoutes]: a registered route is covered by no rule, so it is served without role +// checks. A route meant to be open is declared with "public": true. GoFr's own /.well-known/* +// routes and /favicon.ico are left out. // // The PathPrefix("/") catch-all is ignored: it would otherwise make every rule look live. func (c *Config) CheckRoutes(router *mux.Router) error { @@ -74,22 +81,23 @@ func (c *Config) CheckRoutes(router *mux.Router) error { sort.Strings(deadPublic) sort.Strings(uncoveredLabels) - if c.Logger != nil { - if len(uncoveredLabels) > 0 { - c.Logger.Warnf("RBAC: %d registered route(s) are not covered by any rule and are served without role checks: %s", - len(uncoveredLabels), strings.Join(uncoveredLabels, ", ")) - } - - if len(deadPublic) > 0 { - c.Logger.Warnf("RBAC: public rule(s) match no registered route: %s", strings.Join(deadPublic, ", ")) - } + if c.Logger != nil && len(deadPublic) > 0 { + c.Logger.Warnf("RBAC: public rule(s) match no registered route: %s", strings.Join(deadPublic, ", ")) } + var errs []error + if len(deadGuards) > 0 { - return fmt.Errorf("%w: %s", ErrDeadRules, strings.Join(deadGuards, ", ")) + errs = append(errs, fmt.Errorf("%w: %s. Fix each rule's path or methods, or remove it", + ErrDeadRules, strings.Join(deadGuards, ", "))) + } + + if len(uncoveredLabels) > 0 { + errs = append(errs, fmt.Errorf("%w: %s. Add a rule for each, with \"public\": true for a route meant "+ + "to be open", ErrUncoveredRoutes, strings.Join(uncoveredLabels, ", "))) } - return nil + return errors.Join(errs...) } // routeCoverage splits the config against routes into the rules that match none of them and the diff --git a/pkg/gofr/rbac/coverage_test.go b/pkg/gofr/rbac/coverage_test.go index 4a9d288f99..7c38a9a11f 100644 --- a/pkg/gofr/rbac/coverage_test.go +++ b/pkg/gofr/rbac/coverage_test.go @@ -52,55 +52,49 @@ func coveringRules() []EndpointMapping { func TestConfig_CheckRoutes(t *testing.T) { testCases := []struct { - desc string - endpoints []EndpointMapping - wantDead []string // "METHOD path" entries expected in the returned error - wantWarn []string // substrings expected in the warning output - notInWarn []string // substrings that must not appear in the warning output + desc string + endpoints []EndpointMapping + wantDead []string // "METHOD path" rules expected under ErrDeadRules + wantUncovered []string // "METHOD path" routes expected under ErrUncoveredRoutes + notInErr []string // substrings that must not appear in the error + wantWarn []string // substrings expected in the warning output }{ { desc: "every route covered and every rule live", endpoints: coveringRules(), - notInWarn: []string{"GET", "POST", "DELETE"}, }, { - desc: "a typo in a path segment is a dead rule", - endpoints: append(coveringRules(), - guard("/api/user/{id}", http.MethodDelete)), - wantDead: []string{"DELETE /api/user/{id}"}, + desc: "a typo in a path segment is a dead rule", + endpoints: append(coveringRules(), guard("/api/user/{id}", http.MethodDelete)), + wantDead: []string{"DELETE /api/user/{id}"}, }, { - desc: "a method the route does not register is a dead rule", - endpoints: []EndpointMapping{ - guard("/api/posts", http.MethodGet, http.MethodDelete), - }, - wantDead: []string{"DELETE /api/posts"}, + desc: "a method the route does not register is a dead rule", + endpoints: append(coveringRules(), guard("/api/posts", http.MethodDelete)), + wantDead: []string{"DELETE /api/posts"}, }, { - desc: "a wildcard-method rule is live when the path matches", + desc: "a wildcard-method rule is live and covers every method", endpoints: []EndpointMapping{ - guard("/api/posts", "*"), + guard("/api/users", "*"), guard("/api/users/{id}"), + guard("/api/posts", "*"), + guard("/static/{path:.*}", "*"), }, }, { - desc: "a rule under a static prefix is live", - endpoints: []EndpointMapping{ - guard("/static/app.js", http.MethodGet), - }, + desc: "a rule under a static prefix is live and covers the prefix", + endpoints: append(coveringRules()[:3], guard("/static/app.js", http.MethodGet)), }, { - desc: "the PathPrefix catch-all does not make a rule live", - endpoints: []EndpointMapping{ - guard("/nowhere", http.MethodGet), - }, - wantDead: []string{"GET /nowhere"}, + desc: "the PathPrefix catch-all does not make a rule live", + endpoints: append(coveringRules(), guard("/nowhere", http.MethodGet)), + wantDead: []string{"GET /nowhere"}, }, { desc: "a rule for a built-in route is live", - endpoints: []EndpointMapping{ - {Path: "/.well-known/health", Methods: []string{http.MethodGet}, Public: true}, - }, + endpoints: append(coveringRules(), + EndpointMapping{Path: "/.well-known/health", Methods: []string{http.MethodGet}, Public: true}), }, { desc: "a dead public rule is a warning, not an error", @@ -109,19 +103,30 @@ func TestConfig_CheckRoutes(t *testing.T) { wantWarn: []string{"GET /healthz"}, }, { - desc: "uncovered routes are listed in one warning, built-in routes left out", + desc: "a public rule covers its route", + endpoints: append(coveringRules()[1:], + EndpointMapping{Path: "/api/users", Methods: []string{"*"}, Public: true}), + }, + { + desc: "uncovered routes are an error, built-in routes left out", endpoints: []EndpointMapping{ guard("/api/users", http.MethodGet), guard("/api/users/{id}", "*"), guard("/static/{path:.*}", "*"), }, - wantWarn: []string{"2 registered route(s)", "POST /api/users", "GET /api/posts"}, - notInWarn: []string{"/.well-known", faviconPath, "GET /api/users,", "/api/users/{id}"}, + wantUncovered: []string{"GET /api/posts", "POST /api/users"}, + notInErr: []string{"/.well-known", faviconPath, "GET /api/users,", "/api/users/{id}"}, + }, + { + desc: "an uncovered static prefix is reported", + endpoints: coveringRules()[:3], + wantUncovered: []string{"* /static/"}, }, { - desc: "an uncovered static prefix is reported", - endpoints: coveringRules()[:3], - wantWarn: []string{"* /static/"}, + desc: "dead rules and uncovered routes are both reported", + endpoints: append(coveringRules()[1:], guard("/api/user", http.MethodGet)), + wantDead: []string{"GET /api/user"}, + wantUncovered: []string{"GET /api/users", "POST /api/users"}, }, } @@ -133,14 +138,12 @@ func TestConfig_CheckRoutes(t *testing.T) { err := config.CheckRoutes(newCoverageRouter()) - if len(tc.wantDead) == 0 { - require.NoError(t, err, "TEST[%d], Failed.\n%s", i, tc.desc) - } else { - require.ErrorIs(t, err, ErrDeadRules, "TEST[%d], Failed.\n%s", i, tc.desc) + assertRouteCheckPart(t, err, ErrDeadRules, tc.wantDead, "TEST[%d], Failed.\n%s", i, tc.desc) + assertRouteCheckPart(t, err, ErrUncoveredRoutes, tc.wantUncovered, "TEST[%d], Failed.\n%s", i, tc.desc) - for _, dead := range tc.wantDead { - assert.Contains(t, err.Error(), dead, "TEST[%d], Failed.\n%s", i, tc.desc) - } + for _, unwanted := range tc.notInErr { + require.Error(t, err, "TEST[%d], Failed.\n%s", i, tc.desc) + assert.NotContains(t, err.Error(), unwanted, "TEST[%d], Failed.\n%s", i, tc.desc) } warnings := strings.Join(logger.warnLogs, "\n") @@ -148,10 +151,24 @@ func TestConfig_CheckRoutes(t *testing.T) { for _, want := range tc.wantWarn { assert.Contains(t, warnings, want, "TEST[%d], Failed.\n%s", i, tc.desc) } - - for _, unwanted := range tc.notInWarn { - assert.NotContains(t, warnings, unwanted, "TEST[%d], Failed.\n%s", i, tc.desc) - } }) } } + +// assertRouteCheckPart checks that err carries sentinel and lists every entry of want when want is +// non-empty, and that it does not carry sentinel otherwise. +func assertRouteCheckPart(t *testing.T, err, sentinel error, want []string, msgAndArgs ...any) { + t.Helper() + + if len(want) == 0 { + assert.NotErrorIs(t, err, sentinel, msgAndArgs...) + + return + } + + require.ErrorIs(t, err, sentinel, msgAndArgs...) + + for _, entry := range want { + assert.Contains(t, err.Error(), entry, msgAndArgs...) + } +} diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index b538fdb8af..05d0ca8377 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -155,16 +155,24 @@ func TestEnableRBAC(t *testing.T) { } } +// rbacPublicStatic opens the ./static directory New() serves when this package's tests run. +const rbacPublicStatic = `{"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"]}]}` + `"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"]}]}` + `{"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}]}` func TestApp_prepareHTTPServer(t *testing.T) { tests := []struct { @@ -179,6 +187,10 @@ func TestApp_prepareHTTPServer(t *testing.T) { wantOutcome: startupFailed, wantLog: "DELETE /api/user/{id}"}, {desc: "a dead rule is logged under the deprecated EnableRBAC", config: rbacWithDeadRule, deprecated: true, wantOutcome: startupOK, wantLog: "DELETE /api/user/{id}"}, + {desc: "an uncovered route stops startup under EnableRBACWithError", config: rbacLeavingPingUncovered, + wantOutcome: startupFailed, wantLog: "GET /ping"}, + {desc: "an uncovered route is logged under the deprecated EnableRBAC", config: rbacLeavingPingUncovered, + deprecated: true, wantOutcome: startupOK, wantLog: "GET /ping"}, } for i, tc := range tests { @@ -187,25 +199,34 @@ func TestApp_prepareHTTPServer(t *testing.T) { path := writeRBACConfig(t, t.TempDir(), "rbac.json", tc.config) - var outcome startupOutcome + var ( + outcome startupOutcome + stdout string + ) - logs := testutil.StderrOutputForFunc(func() { - a := New() - a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + // Errors go to stderr and warnings to stdout, so both are captured. + stderr := testutil.StderrOutputForFunc(func() { + stdout = testutil.StdoutOutputForFunc(func() { + a := New() + a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) - if tc.deprecated { - a.EnableRBAC(path) - } else { - require.NoError(t, a.EnableRBACWithError(path)) - } + if tc.deprecated { + a.EnableRBAC(path) + } else { + require.NoError(t, a.EnableRBACWithError(path)) + } - outcome = a.prepareHTTPServer() + outcome = a.prepareHTTPServer() + }) }) + logs := stdout + stderr + assert.Equal(t, tc.wantOutcome, outcome, "TEST[%d], Failed.\n%s", i, tc.desc) if tc.wantLog == "" { assert.NotContains(t, logs, "RBAC route check failed", "TEST[%d], Failed.\n%s", i, tc.desc) + assert.NotContains(t, logs, "covered by no RBAC rule", "TEST[%d], Failed.\n%s", i, tc.desc) } else { assert.Contains(t, logs, tc.wantLog, "TEST[%d], Failed.\n%s", i, tc.desc) } diff --git a/pkg/gofr/run.go b/pkg/gofr/run.go index 917b37a79a..abdbbd896e 100644 --- a/pkg/gofr/run.go +++ b/pkg/gofr/run.go @@ -10,6 +10,8 @@ import ( "sync" "syscall" "time" + + "gofr.dev/pkg/gofr/rbac" ) // telemetryFlushTimeout bounds the metrics and traces flush/shutdown performed @@ -312,21 +314,39 @@ func (a *App) prepareHTTPServer() startupOutcome { return startupOK } - if !a.rbacStrict { - a.Logger().Errorf("RBAC route check failed: %v. These rules protect nothing, so a route they were meant "+ - "to guard is served without role checks. Use EnableRBACWithError to stop startup instead.", err) + // CheckRoutes joins a dead-rule error and an uncovered-route error; each is logged on its own line. + for _, part := range splitJoined(err) { + switch { + case a.rbacStrict: + a.Logger().Errorf("RBAC route check failed: %v.", part) + case errors.Is(part, rbac.ErrUncoveredRoutes): + a.Logger().Warnf("RBAC: %v. They are served without role checks; use EnableRBACWithError "+ + "to stop startup instead.", part) + default: + a.Logger().Errorf("RBAC route check failed: %v. These rules protect nothing; use "+ + "EnableRBACWithError to stop startup instead.", part) + } + } + if !a.rbacStrict { return startupOK } - a.Logger().Errorf("RBAC route check failed: %v. Fix the rule paths or methods in the RBAC config, "+ - "or remove the rules.", err) - a.shutdownAfterFailedStartup() return startupFailed } +// splitJoined returns the errors joined by errors.Join, or err itself when it is not a join. +func splitJoined(err error) []error { + var joined interface{ Unwrap() []error } + if errors.As(err, &joined) { + return joined.Unwrap() + } + + return []error{err} +} + // 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 From d732eb6078809d14262f9178c24a05d3fd4893da Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Tue, 29 Sep 2026 15:25:46 +0530 Subject: [PATCH 04/11] feat(rbac): log route check failures instead of stopping startup EnableRBACWithError is gone (#4387 now makes EnableRBAC return an error), so strictness can no longer be tied to a new, unreleased method: a check that stops startup would stop existing apps that call EnableRBAC. A dead rule and an uncovered route are both logged at error level, and the app keeps starting. --- docs/advanced-guide/rbac/page.md | 20 ++++----- pkg/gofr/gofr.go | 6 +-- pkg/gofr/rbac.go | 1 - pkg/gofr/rbac/coverage.go | 3 +- pkg/gofr/rbac_test.go | 72 ++++++-------------------------- pkg/gofr/run.go | 42 ++++++------------- 6 files changed, 39 insertions(+), 105 deletions(-) diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index cfb65e963e..d7d157fc38 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -306,20 +306,19 @@ Treat that line as an open route, not a warning about a typo. 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 reports two kinds of mismatch. With `EnableRBACWithError`, either one **stops startup**: -the app logs the error, releases what it opened, and exits with a non-zero status, the same as a -failed `OnStart` hook. With the deprecated `EnableRBAC`, the app logs them and keeps running. +them and logs each kind of mismatch below as an **error**. The app keeps starting, so read the +startup logs: each line names a route that is not protected the way the config intends. **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 neither its path nor its method can match any 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` never stops startup: it cannot leave a -route unprotected, so it is reported only as a warning. +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: RBAC rules match no registered route: DELETE /api/user/{id}. Fix each -rule's path or methods, or remove it. +rule's path or methods, or remove it. These rules protect nothing. ``` **An uncovered route** is a registered route, for a given method, that no rule matches, so it would @@ -331,7 +330,8 @@ when GraphQL is enabled. ``` RBAC route check failed: registered routes are covered by no RBAC rule: GET /api/posts, -POST /api/users. Add a rule for each, with "public": true for a route meant to be open. +POST /api/users. Add a rule for each, with "public": true for a route meant to be open. They are +served without role checks. ``` ```json @@ -661,9 +661,9 @@ 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) -With `EnableRBACWithError`, the app in this example does not start: `POST /api/users` and -`GET /api/posts` are registered but covered by no rule (see [Startup Route Check](#startup-route-check)). -Every registered route needs a rule, and a route meant to be open gets one with `"public": true`. +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 that only part of a pattern covers. diff --git a/pkg/gofr/gofr.go b/pkg/gofr/gofr.go index c0f0e8031c..e03a8ba2ad 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -80,11 +80,9 @@ type App struct { // same point it reports a port it could not claim. See bindMCPServer. mcpConfigErr error - // rbacConfig is the RBAC config installed by EnableRBACWithError, kept so Run can check it - // against the complete route table. rbacStrict is false when it came from the deprecated - // EnableRBAC, which logs a failed check instead of stopping startup. See prepareHTTPServer. + // rbacConfig is the RBAC config installed by EnableRBAC, kept so Run can check it against the + // complete route table. See prepareHTTPServer. rbacConfig *rbac.Config - rbacStrict bool // exit is os.Exit, indirected so a test can observe the status a failed startup reports without // taking the test binary down with it. Nil means os.Exit, which is what every real app uses. diff --git a/pkg/gofr/rbac.go b/pkg/gofr/rbac.go index 75036cdf5d..fbdf399ba6 100644 --- a/pkg/gofr/rbac.go +++ b/pkg/gofr/rbac.go @@ -72,7 +72,6 @@ func (a *App) enableRBAC(configPath ...string) error { a.UseMiddleware(middlewareFunc) a.rbacConfig = config - a.rbacStrict = true return nil } diff --git a/pkg/gofr/rbac/coverage.go b/pkg/gofr/rbac/coverage.go index f70cc2acdb..c7fb0068c5 100644 --- a/pkg/gofr/rbac/coverage.go +++ b/pkg/gofr/rbac/coverage.go @@ -208,7 +208,8 @@ func collectRoutes(router *mux.Router) []registeredRoute { // patternsMayOverlap reports whether a request path could match both the rule pattern and the // route template. Both use mux syntax. // -// It errs toward "yes", because a wrong "no" reports a live rule as dead, and that stops startup. +// 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. The cost is that such a rule can go unreported. func patternsMayOverlap(rule, template string) bool { diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index 05d0ca8377..66c90065dd 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -176,21 +176,13 @@ const rbacLeavingPingUncovered = `{"roleHeader":"X-User-Role",` + func TestApp_prepareHTTPServer(t *testing.T) { tests := []struct { - desc string - config string - deprecated bool // EnableRBAC instead of EnableRBACWithError - wantOutcome startupOutcome - wantLog string + desc string + config string + wantLog string // on stderr, where ERROR lines go; empty means no route check failure is logged }{ - {desc: "rules that all match a route start the app", config: rbacGuardingPing, wantOutcome: startupOK}, - {desc: "a dead rule stops startup under EnableRBACWithError", config: rbacWithDeadRule, - wantOutcome: startupFailed, wantLog: "DELETE /api/user/{id}"}, - {desc: "a dead rule is logged under the deprecated EnableRBAC", config: rbacWithDeadRule, deprecated: true, - wantOutcome: startupOK, wantLog: "DELETE /api/user/{id}"}, - {desc: "an uncovered route stops startup under EnableRBACWithError", config: rbacLeavingPingUncovered, - wantOutcome: startupFailed, wantLog: "GET /ping"}, - {desc: "an uncovered route is logged under the deprecated EnableRBAC", config: rbacLeavingPingUncovered, - deprecated: true, wantOutcome: startupOK, wantLog: "GET /ping"}, + {desc: "rules that all match a route log nothing", config: rbacGuardingPing}, + {desc: "a dead rule is logged as an error", config: rbacWithDeadRule, wantLog: "DELETE /api/user/{id}"}, + {desc: "an uncovered route is logged as an error", config: rbacLeavingPingUncovered, wantLog: "GET /ping"}, } for i, tc := range tests { @@ -199,58 +191,20 @@ func TestApp_prepareHTTPServer(t *testing.T) { path := writeRBACConfig(t, t.TempDir(), "rbac.json", tc.config) - var ( - outcome startupOutcome - stdout string - ) - - // Errors go to stderr and warnings to stdout, so both are captured. stderr := testutil.StderrOutputForFunc(func() { - stdout = testutil.StdoutOutputForFunc(func() { - a := New() - a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) - - if tc.deprecated { - a.EnableRBAC(path) - } else { - require.NoError(t, a.EnableRBACWithError(path)) - } + a := New() + a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + require.NoError(t, a.EnableRBAC(path)) - outcome = a.prepareHTTPServer() - }) + a.prepareHTTPServer() }) - logs := stdout + stderr - - assert.Equal(t, tc.wantOutcome, outcome, "TEST[%d], Failed.\n%s", i, tc.desc) - if tc.wantLog == "" { - assert.NotContains(t, logs, "RBAC route check failed", "TEST[%d], Failed.\n%s", i, tc.desc) - assert.NotContains(t, logs, "covered by no RBAC rule", "TEST[%d], Failed.\n%s", i, tc.desc) + assert.NotContains(t, stderr, "RBAC route check failed", "TEST[%d], Failed.\n%s", i, tc.desc) } else { - assert.Contains(t, logs, tc.wantLog, "TEST[%d], Failed.\n%s", i, tc.desc) + assert.Contains(t, stderr, "RBAC route check failed", "TEST[%d], Failed.\n%s", i, tc.desc) + assert.Contains(t, stderr, tc.wantLog, "TEST[%d], Failed.\n%s", i, tc.desc) } }) } } - -// TestRun_RBACDeadRuleExitsNonZero covers the process-level outcome: a dead rule under -// EnableRBACWithError must be reported to the orchestrator as a failed start, not as a clean exit. -func TestRun_RBACDeadRuleExitsNonZero(t *testing.T) { - testutil.NewServerConfigs(t) - - path := writeRBACConfig(t, t.TempDir(), "rbac.json", rbacWithDeadRule) - - var codes []int - - _ = testutil.StderrOutputForFunc(func() { - app := New() - app.GET("/ping", func(*Context) (any, error) { return "pong", nil }) - require.NoError(t, app.EnableRBACWithError(path)) - app.exit = func(code int) { codes = append(codes, code) } - - app.Run() - }) - - require.Equal(t, []int{exitCodeStartupFailed}, codes, "a dead RBAC rule must stop startup") -} diff --git a/pkg/gofr/run.go b/pkg/gofr/run.go index abdbbd896e..7acca31240 100644 --- a/pkg/gofr/run.go +++ b/pkg/gofr/run.go @@ -86,11 +86,7 @@ func (a *App) Run() { return } - if outcome := a.prepareHTTPServer(); outcome != startupOK { - a.finishAbandonedStartup(outcome) - - return - } + a.prepareHTTPServer() timeout, err := getShutdownTimeoutFromConfig(a.Config) if err != nil { @@ -292,49 +288,35 @@ func (a *App) bindMCPServer(ctx context.Context) startupOutcome { // 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 +// 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. // -// A rule that matches no route stops startup when RBAC was enabled with EnableRBACWithError: the -// caller asked to be told when authorization cannot be enforced as configured, and a typo in a rule -// leaves the route it was written for unguarded. The deprecated EnableRBAC logs and carries on. -func (a *App) prepareHTTPServer() startupOutcome { +// A mismatch is logged as an error and the app keeps starting: a rule that matches no route +// protects nothing, and a route that no rule covers is served without role checks. +func (a *App) prepareHTTPServer() { if !a.httpRegistered { - return startupOK + return } a.httpServerSetup() if a.rbacConfig == nil { - return startupOK + return } err := a.rbacConfig.CheckRoutes(&a.httpServer.router.Router) if err == nil { - return startupOK + return } // CheckRoutes joins a dead-rule error and an uncovered-route error; each is logged on its own line. for _, part := range splitJoined(err) { - switch { - case a.rbacStrict: - a.Logger().Errorf("RBAC route check failed: %v.", part) - case errors.Is(part, rbac.ErrUncoveredRoutes): - a.Logger().Warnf("RBAC: %v. They are served without role checks; use EnableRBACWithError "+ - "to stop startup instead.", part) - default: - a.Logger().Errorf("RBAC route check failed: %v. These rules protect nothing; use "+ - "EnableRBACWithError to stop startup instead.", part) + if errors.Is(part, rbac.ErrUncoveredRoutes) { + a.Logger().Errorf("RBAC route check failed: %v. They are served without role checks.", part) + } else { + a.Logger().Errorf("RBAC route check failed: %v. These rules protect nothing.", part) } } - - if !a.rbacStrict { - return startupOK - } - - a.shutdownAfterFailedStartup() - - return startupFailed } // splitJoined returns the errors joined by errors.Join, or err itself when it is not a join. From f7a96e973bfa9656551d01ab14a17d147df64007 Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Wed, 30 Sep 2026 13:07:23 +0530 Subject: [PATCH 05/11] fix(rbac): check route coverage strictly and keep the check out of the public API The route check reported a route as covered when a rule matched only some of its requests: slashes were trimmed before comparing, and a catch-all counted as matching the path it hangs off. Coverage is now strict, with a separate "only partly covered" error line; the dead-rule check stays lenient. CheckRoutes, ErrDeadRules and ErrUncoveredRoutes are gone. The check is now (*rbac.Config).ReportRouteMismatches([]string) bool, fed from the router walk httpServerSetup already makes. Paths are split and constraints compiled once per check (22 ms at 1000x1000, was 210 ms). GOFR_RBAC_ROUTE_CHECK chooses warn (default), fail or off. --- docs/advanced-guide/rbac/page.md | 72 +++-- docs/references/configs/page.md | 6 + pkg/gofr/gofr.go | 10 + pkg/gofr/rbac.go | 80 +++++ pkg/gofr/rbac/coverage.go | 509 ++++++++++++++++++++----------- pkg/gofr/rbac/coverage_test.go | 293 ++++++++++++++---- pkg/gofr/rbac/resolver_test.go | 37 --- pkg/gofr/rbac_test.go | 125 +++++++- pkg/gofr/run.go | 46 ++- 9 files changed, 848 insertions(+), 330 deletions(-) diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index d7d157fc38..aa821569a3 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -306,44 +306,80 @@ Treat that line as an open route, not a warning about a typo. 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**. The app keeps starting, so read the -startup logs: each line names a route that is not protected the way the config intends. +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 neither its path nor its method can match any -registered route. A rule whose method the route does not register (`DELETE` on a route that only +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: RBAC rules match no registered route: DELETE /api/user/{id}. Fix each -rule's path or methods, or remove it. These rules protect nothing. +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`. GoFr's own `/.well-known/*` routes and `/favicon.ico` are exempt. Other routes GoFr registers for -you do need a rule: the `./static` directory (served at `/static/` when it exists) and `/graphql` -when GraphQL is enabled. +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: registered routes are covered by no RBAC rule: GET /api/posts, -POST /api/users. Add a rule for each, with "public": true for a route meant to be open. They are -served without role checks. +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} ``` -The check errs toward "this rule might match". Two path variables with different constraints — -`{id:[0-9]+}` in the rule and `{id:[a-z]+}` in the route — are treated as matching, even though no -request could satisfy both, so a rule like that is not reported as dead. For the same reason a route -counts as covered when a rule matches only some of its requests: a rule for `/api/users/{id:[0-9]+}` -covers the route `/api/users/{id}`, although a request for `/api/users/abc` is not governed by it. +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 it +matches every request the route serves, so the same pair is reported as partly covered. + +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. + +#### 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`. ## JWT-Based RBAC @@ -664,8 +700,8 @@ In this configuration: 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 that only -part of a pattern covers. +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 diff --git a/docs/references/configs/page.md b/docs/references/configs/page.md index e9695bdd63..8128948a91 100644 --- a/docs/references/configs/page.md +++ b/docs/references/configs/page.md @@ -200,6 +200,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 e03a8ba2ad..2a7993b82c 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -84,6 +84,10 @@ type App struct { // complete route table. See prepareHTTPServer. rbacConfig *rbac.Config + // rbacRoutes is the route table as the RBAC route check reads it, "METHOD /template" per entry. + // httpServerSetup fills it from the walk it already makes, and only when rbacConfig is set. + rbacRoutes []string + // exit is os.Exit, indirected so a test can observe the status a failed startup reports without // taking the test binary down with it. Nil means os.Exit, which is what every real app uses. exit func(int) @@ -257,6 +261,8 @@ func (a *App) httpServerSetup() { var registeredMethods []string + routes := newRBACRouteCollector(a.rbacConfig != nil) + _ = a.httpServer.router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error { met, _ := route.GetMethods() for _, method := range met { @@ -265,9 +271,13 @@ func (a *App) httpServerSetup() { } } + routes.add(route, met) + return nil }) + a.rbacRoutes = routes.labels + *a.httpServer.router.RegisteredRoutes = registeredMethods } diff --git a/pkg/gofr/rbac.go b/pkg/gofr/rbac.go index fbdf399ba6..ed2435d2da 100644 --- a/pkg/gofr/rbac.go +++ b/pkg/gofr/rbac.go @@ -3,7 +3,9 @@ package gofr import ( "errors" "fmt" + "strings" + "github.com/gorilla/mux" "go.opentelemetry.io/otel" "gofr.dev/pkg/gofr/rbac" @@ -75,3 +77,81 @@ func (a *App) enableRBAC(configPath ...string) error { return nil } + +// 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 + } +} + +// rbacRouteCollector builds the route list the RBAC route check reads, from the router walk +// httpServerSetup already makes. A disabled collector adds nothing. +type rbacRouteCollector struct { + enabled bool + seen map[string]bool + labels []string +} + +func newRBACRouteCollector(enabled bool) *rbacRouteCollector { + return &rbacRouteCollector{enabled: enabled, seen: make(map[string]bool)} +} + +// add records route as one "METHOD /template" entry per method, "*" for a route with no method +// matcher. 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 + } + + 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, "$") { + if _, isCatchAll := route.GetHandler().(handler); isCatchAll && tmpl == "/" { + return + } + + tmpl = strings.TrimSuffix(tmpl, "/") + "/{path:.*}" + } + + if len(methods) == 0 { + methods = []string{"*"} + } + + for _, method := range methods { + label := strings.ToUpper(method) + " " + tmpl + if c.seen[label] { + continue + } + + c.seen[label] = true + c.labels = append(c.labels, label) + } +} diff --git a/pkg/gofr/rbac/coverage.go b/pkg/gofr/rbac/coverage.go index c7fb0068c5..cf4f16b615 100644 --- a/pkg/gofr/rbac/coverage.go +++ b/pkg/gofr/rbac/coverage.go @@ -1,25 +1,11 @@ package rbac import ( - "errors" - "fmt" "regexp" "sort" "strings" - - "github.com/gorilla/mux" ) -// ErrDeadRules is returned by [Config.CheckRoutes] when a rule that requires permissions matches no -// registered route. Such a rule protects nothing, and it is usually a typo that leaves the route it -// was written for unguarded. -var ErrDeadRules = errors.New("RBAC rules match no registered route") - -// ErrUncoveredRoutes is returned by [Config.CheckRoutes] when a registered route is covered by no -// rule. Such a route is served without role checks; one meant to be open is declared with -// "public": true. -var ErrUncoveredRoutes = errors.New("registered routes are covered by no RBAC rule") - // 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 = "*" @@ -27,253 +13,426 @@ const anyMethod = "*" // faviconPath is the built-in favicon route GoFr registers itself. const faviconPath = "/favicon.ico" -// registeredRoute is one (method, path) pair the router serves, as the coverage check sees it. -type registeredRoute struct { - // method is the upper-cased method, or anyMethod for a route with no method matcher. - method string - - // template is the path template. A PathPrefix route carries a trailing catch-all segment, so - // that it overlaps every path under the prefix. - template string - - // label is how the route is reported: "METHOD /path". - label string -} - -// CheckRoutes compares the config against the routes registered on router, which must hold the -// complete route table. +// ReportRouteMismatches compares the config against routes, the complete route table, and logs +// every mismatch it finds. Each route is "METHOD /template" in mux syntax, with "*" for a route +// registered without a method matcher; a prefix route is passed with a trailing catch-all segment, +// as "/static/{path:.*}". It reports whether any mismatch was logged as an error. // -// It reports two kinds of mismatch, each listing its entries as "METHOD path", and joins them with -// [errors.Join] when both are present, so the caller can tell them apart with errors.Is: +// It logs one line for each kind of mismatch, listing its entries as "METHOD path": // -// - [ErrDeadRules]: a rule matches no registered route - usually a typo in its path, or a method -// the route does not register - so the route it was written for is unguarded. A dead public rule -// cannot leave a route unguarded, so it is only warned about. -// - [ErrUncoveredRoutes]: a registered route is covered by no rule, so it is served without role -// checks. A route meant to be open is declared with "public": true. GoFr's own /.well-known/* -// routes and /favicon.ico are left out. +// - a dead rule matches no route - usually a typo in its path, or a method the route does not +// register - so the route it was written for is unguarded. A dead public rule cannot leave a +// route unguarded, so it is only warned about. +// - an uncovered route is matched by no rule, so it is served without role checks. +// - a partly covered route is matched by rules for some of its requests but not all of them. // -// The PathPrefix("/") catch-all is ignored: it would otherwise make every rule look live. -func (c *Config) CheckRoutes(router *mux.Router) error { - if router == nil { - return nil +// GoFr's own /.well-known/* routes and /favicon.ico need no rule. +// +// 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 it matches every request the route serves. +func (c *Config) ReportRouteMismatches(routes []string) bool { + rc := newRouteChecker() + + rules := make([]routeEntry, len(c.rules)) + for i := range c.rules { + rules[i] = rc.newEntry(c.rules[i].method, c.rules[i].pattern) + } + + parsed := make([]routeEntry, len(routes)) + for j, route := range routes { + parsed[j] = rc.parseEntry(route) } - dead, uncovered := c.routeCoverage(collectRoutes(router)) + var ( + live = make([]bool, len(rules)) + uncovered, partial []string + deadGuards, deadOpen []string + ) - var deadGuards, deadPublic []string + for j := range parsed { + if isBuiltInRoute(parsed[j].rawPath) { + markLiveRules(rules, &parsed[j], live) - for i := range dead { - label := dead[i].method + " " + dead[i].pattern - if dead[i].isPublic { - deadPublic = append(deadPublic, label) - } else { - deadGuards = append(deadGuards, label) + continue + } + + switch covering := classifyRoute(rules, &parsed[j], live); { + case covering == nil: + uncovered = append(uncovered, parsed[j].label) + case len(covering) > 0: + partial = append(partial, parsed[j].label+" (by "+strings.Join(sortedUnique(covering), " and ")+")") } } - uncoveredLabels := make([]string, 0, len(uncovered)) - for i := range uncovered { - uncoveredLabels = append(uncoveredLabels, uncovered[i].label) + for i := range rules { + switch { + case live[i]: + case c.rules[i].isPublic: + deadOpen = append(deadOpen, rules[i].label) + default: + deadGuards = append(deadGuards, rules[i].label) + } } - sort.Strings(deadGuards) - sort.Strings(deadPublic) - sort.Strings(uncoveredLabels) + c.logRouteMismatches(sortedUnique(deadGuards), sortedUnique(deadOpen), sortedUnique(uncovered), sortedUnique(partial)) - if c.Logger != nil && len(deadPublic) > 0 { - c.Logger.Warnf("RBAC: public rule(s) match no registered route: %s", strings.Join(deadPublic, ", ")) + return len(deadGuards)+len(uncovered)+len(partial) > 0 +} + +// logRouteMismatches writes one line for each non-empty kind of mismatch. +func (c *Config) logRouteMismatches(deadGuards, deadOpen, uncovered, partial []string) { + if c.Logger == nil { + return } - var errs []error + if len(deadOpen) > 0 { + c.Logger.Warnf("RBAC: public rules match no registered route: %s", strings.Join(deadOpen, ", ")) + } if len(deadGuards) > 0 { - errs = append(errs, fmt.Errorf("%w: %s. Fix each rule's path or methods, or remove it", - ErrDeadRules, strings.Join(deadGuards, ", "))) + c.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(deadGuards, ", ")) } - if len(uncoveredLabels) > 0 { - errs = append(errs, fmt.Errorf("%w: %s. Add a rule for each, with \"public\": true for a route meant "+ - "to be open", ErrUncoveredRoutes, strings.Join(uncoveredLabels, ", "))) + if len(uncovered) > 0 { + c.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(uncovered, ", ")) } - return errors.Join(errs...) + if len(partial) > 0 { + c.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(partial, ", ")) + } } -// routeCoverage splits the config against routes into the rules that match none of them and the -// routes that no rule matches. -func (c *Config) routeCoverage(routes []registeredRoute) (dead []endpointRule, uncovered []registeredRoute) { - for i := range c.rules { - live := false +// 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 []routeEntry, route *routeEntry, live []bool) []string { + covered, overlapping := false, []string{} - for j := range routes { - if ruleMayMatchRoute(&c.rules[i], &routes[j]) { - live = true - - break - } + for i := range rules { + if !rules[i].overlaps(route) { + continue } - if !live { - dead = append(dead, c.rules[i]) + live[i] = true + + switch { + case covered: + case rules[i].covers(route): + covered = true + default: + overlapping = append(overlapping, rules[i].label) } } - for j := range routes { - if isBuiltInRoute(routes[j].template) { - continue + 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 []routeEntry, route *routeEntry, live []bool) { + for i := range rules { + if !live[i] && rules[i].overlaps(route) { + live[i] = true } + } +} - covered := false +// sortedUnique sorts entries and drops repeats, in place. +func sortedUnique(entries []string) []string { + sort.Strings(entries) - for i := range c.rules { - if ruleMayMatchRoute(&c.rules[i], &routes[j]) { - covered = true + out := entries[:0] - break - } - } - - if !covered { - uncovered = append(uncovered, routes[j]) + for i, entry := range entries { + if i == 0 || entry != entries[i-1] { + out = append(out, entry) } } - return dead, uncovered -} - -// ruleMayMatchRoute reports whether some request could be served by route and governed by rule. -func ruleMayMatchRoute(rule *endpointRule, route *registeredRoute) bool { - methodsMatch := rule.method == anyMethod || route.method == anyMethod || strings.EqualFold(rule.method, route.method) - - return methodsMatch && patternsMayOverlap(rule.pattern, route.template) + return out } // isBuiltInRoute reports whether a template belongs to a route GoFr registers itself, which is left -// out of the uncovered-route report because the application did not write it. +// out of the coverage report because the application did not write it. func isBuiltInRoute(template string) bool { return strings.HasPrefix(template, "/.well-known/") || template == faviconPath } -// collectRoutes lists every (method, path) pair router serves, skipping the PathPrefix("/") -// catch-all and routes with no path. Duplicates collapse to one entry. -func collectRoutes(router *mux.Router) []registeredRoute { - var routes []registeredRoute +// 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]*regexp.Regexp +} - seen := make(map[string]bool) +func newRouteChecker() *routeChecker { + return &routeChecker{constraints: make(map[string]*regexp.Regexp)} +} - _ = router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error { - tmpl, err := route.GetPathTemplate() - if err != nil { - return nil - } +// 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 - // mux anchors a Path regexp with "$" and leaves a PathPrefix one open. - pathRegexp, _ := route.GetPathRegexp() - isPrefix := !strings.HasSuffix(pathRegexp, "$") + // rawPath is the path as written. + rawPath string - if isPrefix && tmpl == "/" { - return nil - } + // label is how the entry is reported: "METHOD path". + label string - template := tmpl - if isPrefix { - template = strings.TrimSuffix(tmpl, "/") + "/{rest:.*}" - } + // segments is rawPath split on "/", empty segments kept. Nil for an empty path, which matches + // nothing. + segments []pathSegment +} - methods, err := route.GetMethods() - if err != nil || len(methods) == 0 { - methods = []string{anyMethod} - } +// pathSegment is one "/"-separated part of a mux path template. +type pathSegment struct { + // text is the segment as written. + text string - for _, method := range methods { - method = strings.ToUpper(method) + // variable is set for a "{name}" or "{name:constraint}" segment. + variable bool - label := method + " " + tmpl - if seen[label] { - continue - } + // catchAll is set for a variable that can span several segments. See isCatchAllVariable. + catchAll bool - seen[label] = true + // constraint is the variable's constraint as written, trimmed; empty for a free variable. + constraint string - routes = append(routes, registeredRoute{method: method, template: template, label: label}) - } + // re is constraint compiled and anchored, or nil for a free variable or one that does not + // compile. + re *regexp.Regexp +} - return nil - }) +// parseEntry parses "METHOD path". A string with no space is a path with anyMethod. +func (rc *routeChecker) parseEntry(s string) routeEntry { + method, path, found := strings.Cut(s, " ") + if !found { + method, path = anyMethod, s + } - return routes + return rc.newEntry(strings.ToUpper(method), path) } -// patternsMayOverlap reports whether a request path could match both the rule pattern and the -// route template. Both use mux syntax. +func (rc *routeChecker) newEntry(method, path string) routeEntry { + entry := routeEntry{method: method, rawPath: path, 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 !isVariableSegment(text) { + return seg + } + + seg.variable = true + seg.catchAll = isCatchAllVariable(text) + + inner := text[1 : len(text)-1] + + idx := strings.Index(inner, ":") + if idx < 0 { + return seg + } + + seg.constraint = strings.TrimSpace(inner[idx+1:]) + seg.re = rc.compile(seg.constraint) + + return seg +} + +// compile returns constraint anchored to a whole segment, as mux applies it, or nil when it does +// not compile. +func (rc *routeChecker) compile(constraint string) *regexp.Regexp { + if re, ok := rc.constraints[constraint]; ok { + return re + } + + re, err := regexp.Compile("^(?:" + constraint + ")$") + if err != nil { + re = nil + } + + rc.constraints[constraint] = re + + return re +} + +func isVariableSegment(segment string) bool { + return strings.HasPrefix(segment, "{") && strings.HasSuffix(segment, "}") +} + +// 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. The cost is that such a rule can go unreported. -func patternsMayOverlap(rule, template string) bool { - if rule == "" || template == "" { +// 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 } - a := splitPatternSegments(strings.Trim(rule, "/")) - b := splitPatternSegments(strings.Trim(template, "/")) - for i := 0; ; i++ { - // A catch-all absorbs the rest of the other path, including nothing at all. - if isCatchAllAt(a, i) || isCatchAllAt(b, 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) } - if !segmentsMayOverlap(a[i], b[i]) { + if a[i].catchAll { + return a[i].coversTail(b[i:]) + } + + if !a[i].covers(&b[i]) { return false } } } -// segmentsMayOverlap reports whether one path segment could match both x and y. -func segmentsMayOverlap(x, y string) bool { - xVar, yVar := isVariableSegment(x), isVariableSegment(y) - +// covers reports whether the rule segment s matches everything the route segment other does. +func (s *pathSegment) covers(other *pathSegment) bool { switch { - case xVar && yVar: - return true - case xVar: - return variableMayMatch(x, y) - case yVar: - return variableMayMatch(y, x) + 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. + return other.constraint == "" || other.re != nil && !other.re.MatchString("") default: - return x == y + return s.re != nil && s.constraint == other.constraint } } -// variableMayMatch reports whether the "{name}" or "{name:regex}" segment could match literal. -func variableMayMatch(variable, literal string) bool { - inner := variable[1 : len(variable)-1] +// 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() - idx := strings.Index(inner, ":") - if idx < 0 { + switch { + case s.constraint == ".*": return true + case s.constraint == ".+": + return !mayBeEmpty + case s.re == nil: + return false } - re, err := regexp.Compile("^(?:" + inner[idx+1:] + ")$") - if err != nil { - return true + if len(tail) == 1 && tail[0].catchAll { + return s.constraint == tail[0].constraint } - return re.MatchString(literal) -} + literals := make([]string, len(tail)) -func isVariableSegment(segment string) bool { - return strings.HasPrefix(segment, "{") && strings.HasSuffix(segment, "}") + for i := range tail { + if tail[i].variable { + return false + } + + literals[i] = tail[i].text + } + + return s.re.MatchString(strings.Join(literals, "/")) } -// isCatchAllAt reports whether segments has a catch-all variable at index i. -func isCatchAllAt(segments []string, i int) bool { - return i < len(segments) && isVariableSegment(segments[i]) && isCatchAllVariable(segments[i]) +// 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("") + } } diff --git a/pkg/gofr/rbac/coverage_test.go b/pkg/gofr/rbac/coverage_test.go index 7c38a9a11f..4da303f2eb 100644 --- a/pkg/gofr/rbac/coverage_test.go +++ b/pkg/gofr/rbac/coverage_test.go @@ -1,38 +1,29 @@ package rbac import ( + "fmt" "net/http" "strings" "testing" - "github.com/gorilla/mux" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -// newCoverageRouter builds a router shaped like GoFr's: one route per method and path, the built-in -// well-known routes, a static-file prefix, and the PathPrefix("/") catch-all registered last. -func newCoverageRouter() *mux.Router { - noop := http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}) - router := mux.NewRouter() - - for _, r := range []struct{ method, path string }{ - {http.MethodGet, "/api/users"}, - {http.MethodPost, "/api/users"}, - {http.MethodGet, "/api/users/{id}"}, - {http.MethodDelete, "/api/users/{id}"}, - {http.MethodGet, "/api/posts"}, - {http.MethodGet, "/.well-known/health"}, - {http.MethodGet, "/.well-known/alive"}, - {http.MethodGet, faviconPath}, - } { - router.NewRoute().Methods(r.method).Path(r.path).Handler(noop) +// coverageRoutes is a route table shaped like the one GoFr hands the check: one entry per method +// and path, the built-in well-known routes, and a static directory served at /static. +func coverageRoutes() []string { + return []string{ + "GET /api/users", + "POST /api/users", + "GET /api/users/{id}", + "DELETE /api/users/{id}", + "GET /api/posts", + "GET /.well-known/health", + "GET /.well-known/alive", + "GET " + faviconPath, + "* /static", + "* /static/{path:.*}", } - - router.NewRoute().PathPrefix("/static/").Handler(noop) - router.PathPrefix("/").Handler(noop) - - return router } // guard is a non-public rule requiring one permission. @@ -40,25 +31,37 @@ func guard(path string, methods ...string) EndpointMapping { return EndpointMapping{Path: path, Methods: methods, RequiredPermissions: []string{"p:x"}} } -// coveringRules guards every non-built-in route in newCoverageRouter. +// coveringRules guards every non-built-in route in coverageRoutes. func coveringRules() []EndpointMapping { return []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:.*}", "*"), } } -func TestConfig_CheckRoutes(t *testing.T) { - testCases := []struct { - desc string - endpoints []EndpointMapping - wantDead []string // "METHOD path" rules expected under ErrDeadRules - wantUncovered []string // "METHOD path" routes expected under ErrUncoveredRoutes - notInErr []string // substrings that must not appear in the error - wantWarn []string // substrings expected in the warning output - }{ +// Markers that tell the three error lines apart. +const ( + deadLine = "rules match no registered route" + uncoveredLine = "routes covered by no rule" + partialLine = "routes only partly covered" +) + +// routeMismatchCase is one case of TestConfig_ReportRouteMismatches. +type routeMismatchCase struct { + desc string + endpoints []EndpointMapping + wantDead []string // entries expected on the dead-rule line + wantUncovered []string // entries expected on the uncovered-route line + wantPartial []string // entries expected on the partly-covered line + notLogged []string // substrings that must appear in no error line + wantWarn []string // substrings expected in the warning output +} + +func routeMismatchCases() []routeMismatchCase { + return []routeMismatchCase{ { desc: "every route covered and every rule live", endpoints: coveringRules(), @@ -79,17 +82,50 @@ func TestConfig_CheckRoutes(t *testing.T) { guard("/api/users", "*"), guard("/api/users/{id}"), guard("/api/posts", "*"), + guard("/static", "*"), guard("/static/{path:.*}", "*"), }, }, { - desc: "a rule under a static prefix is live and covers the prefix", - endpoints: append(coveringRules()[:3], guard("/static/app.js", http.MethodGet)), + desc: "a trailing slash makes a different path", + endpoints: append(coveringRules()[1:], + guard("/api/users/", http.MethodGet, http.MethodPost)), + wantDead: []string{"GET /api/users/", "POST /api/users/"}, + wantUncovered: []string{"GET /api/users", "POST /api/users"}, + }, + { + desc: "a relative path matches nothing", + endpoints: append(coveringRules()[1:], + guard("api/users", http.MethodGet, http.MethodPost)), + wantDead: []string{"GET api/users", "POST api/users"}, + wantUncovered: []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:.*}", "*")), + wantUncovered: []string{"* /static"}, + notLogged: []string{"* /static/{path:.*}"}, + }, + { + desc: "a constraint narrower than the route covers it only partly", + endpoints: []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:.*}", "*"), + }, + wantPartial: []string{ + "GET /api/users/{id} (by GET /api/users/{id:[0-9]+})", + "DELETE /api/users/{id} (by DELETE /api/users/{id:[0-9]+})", + }, }, { - desc: "the PathPrefix catch-all does not make a rule live", - endpoints: append(coveringRules(), guard("/nowhere", http.MethodGet)), - wantDead: []string{"GET /nowhere"}, + desc: "a method list narrower than a static route covers it only partly", + endpoints: append(coveringRules()[:4], + guard("/static/{path:.*}", http.MethodGet, http.MethodHead)), + wantPartial: []string{"* /static/{path:.*} (by GET /static/{path:.*} and HEAD /static/{path:.*})"}, }, { desc: "a rule for a built-in route is live", @@ -112,15 +148,11 @@ func TestConfig_CheckRoutes(t *testing.T) { endpoints: []EndpointMapping{ guard("/api/users", http.MethodGet), guard("/api/users/{id}", "*"), + guard("/static", "*"), guard("/static/{path:.*}", "*"), }, wantUncovered: []string{"GET /api/posts", "POST /api/users"}, - notInErr: []string{"/.well-known", faviconPath, "GET /api/users,", "/api/users/{id}"}, - }, - { - desc: "an uncovered static prefix is reported", - endpoints: coveringRules()[:3], - wantUncovered: []string{"* /static/"}, + notLogged: []string{"/.well-known", faviconPath, "GET /api/users,", "/api/users/{id}"}, }, { desc: "dead rules and uncovered routes are both reported", @@ -129,25 +161,30 @@ func TestConfig_CheckRoutes(t *testing.T) { wantUncovered: []string{"GET /api/users", "POST /api/users"}, }, } +} - for i, tc := range testCases { +func TestConfig_ReportRouteMismatches(t *testing.T) { + for i, tc := range routeMismatchCases() { t.Run(tc.desc, func(t *testing.T) { logger := &mockLogger{} config := newTestConfig(t, tc.endpoints, nil) config.Logger = logger - err := config.CheckRoutes(newCoverageRouter()) + got := config.ReportRouteMismatches(coverageRoutes()) - assertRouteCheckPart(t, err, ErrDeadRules, tc.wantDead, "TEST[%d], Failed.\n%s", i, tc.desc) - assertRouteCheckPart(t, err, ErrUncoveredRoutes, tc.wantUncovered, "TEST[%d], Failed.\n%s", i, tc.desc) + wantMismatch := len(tc.wantDead)+len(tc.wantUncovered)+len(tc.wantPartial) > 0 + assert.Equal(t, wantMismatch, got, "TEST[%d], Failed.\n%s", i, tc.desc) - for _, unwanted := range tc.notInErr { - require.Error(t, err, "TEST[%d], Failed.\n%s", i, tc.desc) - assert.NotContains(t, err.Error(), unwanted, "TEST[%d], Failed.\n%s", i, tc.desc) + assertLogLine(t, logger.errorLogs, deadLine, tc.wantDead, "TEST[%d], Failed.\n%s", i, tc.desc) + assertLogLine(t, logger.errorLogs, uncoveredLine, tc.wantUncovered, "TEST[%d], Failed.\n%s", i, tc.desc) + assertLogLine(t, logger.errorLogs, partialLine, tc.wantPartial, "TEST[%d], Failed.\n%s", i, tc.desc) + + errors := strings.Join(logger.errorLogs, "\n") + for _, unwanted := range tc.notLogged { + assert.NotContains(t, errors, unwanted, "TEST[%d], Failed.\n%s", i, tc.desc) } warnings := strings.Join(logger.warnLogs, "\n") - for _, want := range tc.wantWarn { assert.Contains(t, warnings, want, "TEST[%d], Failed.\n%s", i, tc.desc) } @@ -155,20 +192,158 @@ func TestConfig_CheckRoutes(t *testing.T) { } } -// assertRouteCheckPart checks that err carries sentinel and lists every entry of want when want is -// non-empty, and that it does not carry sentinel otherwise. -func assertRouteCheckPart(t *testing.T, err, sentinel error, want []string, msgAndArgs ...any) { +// TestConfig_ReportRouteMismatches_NilLogger checks that a config with no logger still reports the +// mismatch through its result instead of panicking. +func TestConfig_ReportRouteMismatches_NilLogger(t *testing.T) { + config := newTestConfig(t, coveringRules()[1:], nil) + + assert.True(t, config.ReportRouteMismatches(coverageRoutes())) +} + +// assertLogLine checks that exactly one line carries marker and lists every entry of want when want +// is non-empty, and that no line carries marker otherwise. +func assertLogLine(t *testing.T, lines []string, marker string, want []string, msgAndArgs ...any) { t.Helper() + var matched []string + + for _, line := range lines { + if strings.Contains(line, marker) { + matched = append(matched, line) + } + } + if len(want) == 0 { - assert.NotErrorIs(t, err, sentinel, msgAndArgs...) + assert.Empty(t, matched, msgAndArgs...) return } - require.ErrorIs(t, err, sentinel, msgAndArgs...) + if !assert.Len(t, matched, 1, msgAndArgs...) { + return + } for _, entry := range want { - assert.Contains(t, err.Error(), entry, msgAndArgs...) + assert.Contains(t, matched[0], entry, msgAndArgs...) + } +} + +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}, + {"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 := rc.parseEntry(tc.rule), rc.parseEntry(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}, + {"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 := rc.parseEntry(tc.rule), rc.parseEntry(tc.route) + + assert.Equal(t, tc.want, rule.covers(&route), "TEST[%d], Failed.\n%s", i, tc.desc) + }) + } +} + +// BenchmarkConfig_ReportRouteMismatches 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 BenchmarkConfig_ReportRouteMismatches(b *testing.B) { + for _, n := range []int{50, 200, 1000} { + b.Run(fmt.Sprintf("routes=%d", n), func(b *testing.B) { + endpoints := make([]EndpointMapping, 0, n) + routes := make([]string, 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, "GET "+path) + } + + config := newTestConfig(b, endpoints, nil) + + b.ReportAllocs() + + for b.Loop() { + if config.ReportRouteMismatches(routes) { + b.Fatal("every route is covered, so no mismatch is expected") + } + } + }) } } diff --git a/pkg/gofr/rbac/resolver_test.go b/pkg/gofr/rbac/resolver_test.go index 714653b45f..41e3e41236 100644 --- a/pkg/gofr/rbac/resolver_test.go +++ b/pkg/gofr/rbac/resolver_test.go @@ -446,40 +446,3 @@ func TestGetEndpointForRequest_SlashConstraintDoesNotOutrankNarrowRule(t *testin assert.Equal(t, "/files/private/{name}", endpoint.Path) } } - -func Test_patternsMayOverlap(t *testing.T) { - testCases := []struct { - desc string - rule string - template string - want bool - }{ - {"identical literals", "/api/users", "/api/users", true}, - {"different literals", "/api/users", "/api/posts", false}, - {"typo in a literal segment", "/api/user/{id}", "/api/users/{id}", false}, - {"free variable against literal", "/api/users/{id}", "/api/users/me", true}, - {"literal against free variable", "/api/users/me", "/api/users/{id}", true}, - {"constraint admits the literal", "/api/users/{id:[0-9]+}", "/api/users/42", true}, - {"constraint rejects the literal", "/api/users/{id:[0-9]+}", "/api/users/me", false}, - {"route constraint rejects the literal", "/api/users/me", "/api/users/{id:[0-9]+}", false}, - {"variable against variable", "/api/users/{id:[0-9]+}", "/api/users/{id:[a-z0-9]+}", true}, - {"more segments in the rule", "/api/users/{id}/posts", "/api/users/{id}", false}, - {"fewer segments in the rule", "/api", "/api/users", false}, - {"catch-all in the rule absorbs the rest", "/api/{path:.*}", "/api/users/{id}/posts", true}, - {"catch-all in the rule absorbs nothing", "/api/{path:.*}", "/api", true}, - {"catch-all in the route absorbs the rest", "/static/css/site.css", "/static/{path:.*}", true}, - {"catch-all after a different literal", "/admin/{path:.*}", "/api/users", false}, - {"trailing slash is ignored", "/api/users/", "/api/users", true}, - {"root against root", "/", "/", true}, - {"root against a path", "/", "/api", false}, - {"constraint containing a slash spans segments", "/files/{path:[a-z/]+}", "/files/a/b/c", true}, - {"uncompilable constraint is assumed to overlap", "/api/{id:[}", "/api/users", true}, - {"empty rule matches nothing", "", "/api", false}, - } - - for i, tc := range testCases { - t.Run(tc.desc, func(t *testing.T) { - assert.Equal(t, tc.want, patternsMayOverlap(tc.rule, tc.template), "TEST[%d], Failed.\n%s", i, tc.desc) - }) - } -} diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index 66c90065dd..38c8f6ed18 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -155,8 +155,10 @@ func TestEnableRBAC(t *testing.T) { } } -// rbacPublicStatic opens the ./static directory New() serves when this package's tests run. -const rbacPublicStatic = `{"path":"/static/{path:.*}","methods":["*"],"public":true}` +// 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",` + @@ -172,39 +174,132 @@ const rbacWithDeadRule = `{"roleHeader":"X-User-Role",` + // 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}]}` + `"endpoints":[{"path":"/.well-known/alive","methods":["GET"],"public":true},` + rbacPublicStatic + `]}` + +// routeCheckFailed starts every ERROR line the route check writes. +const routeCheckFailed = "RBAC route check failed" func TestApp_prepareHTTPServer(t *testing.T) { tests := []struct { - desc string - config string - wantLog string // on stderr, where ERROR lines go; empty means no route check failure is logged + desc string + config string + 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 + wantOutcome startupOutcome }{ - {desc: "rules that all match a route log nothing", config: rbacGuardingPing}, - {desc: "a dead rule is logged as an error", config: rbacWithDeadRule, wantLog: "DELETE /api/user/{id}"}, - {desc: "an uncovered route is logged as an error", config: rbacLeavingPingUncovered, wantLog: "GET /ping"}, + { + desc: "rules that all match a route log nothing", + config: rbacGuardingPing, + notLog: []string{routeCheckFailed}, + }, + { + 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:.*}"}, + }, } for i, tc := range tests { t.Run(tc.desc, func(t *testing.T) { testutil.NewServerConfigs(t) + if tc.mode != "" { + t.Setenv("GOFR_RBAC_ROUTE_CHECK", tc.mode) + } + path := writeRBACConfig(t, t.TempDir(), "rbac.json", tc.config) + var outcome startupOutcome + stderr := testutil.StderrOutputForFunc(func() { a := New() a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + + if tc.setup != nil { + tc.setup(a) + } + require.NoError(t, a.EnableRBAC(path)) - a.prepareHTTPServer() + outcome = a.prepareHTTPServer() }) - if tc.wantLog == "" { - assert.NotContains(t, stderr, "RBAC route check failed", "TEST[%d], Failed.\n%s", i, tc.desc) - } else { - assert.Contains(t, stderr, "RBAC route check failed", "TEST[%d], Failed.\n%s", i, tc.desc) - assert.Contains(t, stderr, tc.wantLog, "TEST[%d], Failed.\n%s", i, tc.desc) + 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) } }) } } + +// 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{exitCodeStartupFailed}, codes) +} diff --git a/pkg/gofr/run.go b/pkg/gofr/run.go index 7acca31240..396674ac97 100644 --- a/pkg/gofr/run.go +++ b/pkg/gofr/run.go @@ -10,8 +10,6 @@ import ( "sync" "syscall" "time" - - "gofr.dev/pkg/gofr/rbac" ) // telemetryFlushTimeout bounds the metrics and traces flush/shutdown performed @@ -86,7 +84,11 @@ func (a *App) Run() { return } - a.prepareHTTPServer() + if outcome := a.prepareHTTPServer(); outcome != startupOK { + a.finishAbandonedStartup(outcome) + + return + } timeout, err := getShutdownTimeoutFromConfig(a.Config) if err != nil { @@ -291,42 +293,34 @@ func (a *App) bindMCPServer(ctx context.Context) startupOutcome { // 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. // -// A mismatch is logged as an error and the app keeps starting: a rule that matches no route -// protects nothing, and a route that no rule covers is served without role checks. -func (a *App) prepareHTTPServer() { +// 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 + return startupOK } a.httpServerSetup() if a.rbacConfig == nil { - return + return startupOK } - err := a.rbacConfig.CheckRoutes(&a.httpServer.router.Router) - if err == nil { - return + mode := a.rbacRouteCheckMode() + if mode == rbacRouteCheckOff { + return startupOK } - // CheckRoutes joins a dead-rule error and an uncovered-route error; each is logged on its own line. - for _, part := range splitJoined(err) { - if errors.Is(part, rbac.ErrUncoveredRoutes) { - a.Logger().Errorf("RBAC route check failed: %v. They are served without role checks.", part) - } else { - a.Logger().Errorf("RBAC route check failed: %v. These rules protect nothing.", part) - } + if !a.rbacConfig.ReportRouteMismatches(a.rbacRoutes) || mode != rbacRouteCheckFail { + return startupOK } -} -// splitJoined returns the errors joined by errors.Join, or err itself when it is not a join. -func splitJoined(err error) []error { - var joined interface{ Unwrap() []error } - if errors.As(err, &joined) { - return joined.Unwrap() - } + a.Logger().Errorf("Stopping startup: the RBAC route check failed and %s=%s.", rbacRouteCheckKey, rbacRouteCheckFail) + + a.shutdownAfterFailedStartup() - return []error{err} + return startupFailed } // shutdownAfterFailedStartup releases what startup has already opened when the run is abandoned From d9550007b4c4cf2930b5fa2c4f74fc7a57419700 Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Tue, 6 Oct 2026 12:28:04 +0530 Subject: [PATCH 06/11] fix(rbac): report a route as covered only when the check can show it The strict coverage check still answered "covered" for four shapes its segment model did not represent: a rule catch-all followed by more segments, a segment holding more than one variable, a constraint that can match "/", and a constraint with a space mux keeps. Segments are now read the way mux reads them, and a rule catch-all covers only as the rule's last segment. GoFr's own routes are now exempted by where they were registered rather than by the /.well-known/ prefix, so an application route under /.well-known/ is checked. The check runs before the MCP port is claimed, and the HTTP server is announced only when it starts. --- docs/advanced-guide/rbac/page.md | 26 +++- pkg/gofr/gofr.go | 21 +-- pkg/gofr/rbac.go | 63 +++++++- pkg/gofr/rbac/coverage.go | 240 +++++++++++++++++++++++-------- pkg/gofr/rbac/coverage_test.go | 52 +++++-- pkg/gofr/rbac_test.go | 91 +++++++++--- pkg/gofr/run.go | 16 ++- 7 files changed, 398 insertions(+), 111 deletions(-) diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index aa821569a3..1a96f69441 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -328,8 +328,10 @@ 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`. -GoFr's own `/.well-known/*` routes and `/favicon.ico` are exempt. Other routes GoFr registers for -you do need a rule: the `./static` directory (served at `/static` and everything under `/static/` +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. @@ -363,8 +365,20 @@ A rule set that opens the static directory and a login route: 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 it -matches every request the route serves, so the same pair is reported as partly covered. +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. @@ -381,6 +395,10 @@ Set `GOFR_RBAC_ROUTE_CHECK` in your config: 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: diff --git a/pkg/gofr/gofr.go b/pkg/gofr/gofr.go index 9aea9d2cc7..85d00b4f92 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -84,9 +84,10 @@ type App struct { // complete route table. See prepareHTTPServer. rbacConfig *rbac.Config - // rbacRoutes is the route table as the RBAC route check reads it, "METHOD /template" per entry. - // httpServerSetup fills it from the walk it already makes, and only when rbacConfig is set. - rbacRoutes []string + // rbacRoutes is the route table as the RBAC route check reads it, "METHOD /template" per entry, + // and rbacBuiltInRoutes the part of it GoFr registered itself. httpServerSetup fills both from + // the walk it already makes, and only when rbacConfig is set. + rbacRoutes, rbacBuiltInRoutes []string // exit is os.Exit, indirected so a test can observe the status a failed startup reports without // taking the test binary down with it. Nil means os.Exit, which is what every real app uses. @@ -229,7 +230,11 @@ func (a *App) httpServerSetup() { } } + routes := newRBACRouteCollector(a.rbacConfig != nil) + // 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) @@ -242,6 +247,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) } @@ -250,10 +257,6 @@ 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{ function: catchAllHandler, container: a.container, @@ -261,8 +264,6 @@ func (a *App) httpServerSetup() { var registeredMethods []string - routes := newRBACRouteCollector(a.rbacConfig != nil) - _ = a.httpServer.router.Walk(func(route *mux.Route, _ *mux.Router, _ []*mux.Route) error { met, _ := route.GetMethods() for _, method := range met { @@ -276,7 +277,7 @@ func (a *App) httpServerSetup() { return nil }) - a.rbacRoutes = routes.labels + a.rbacRoutes, a.rbacBuiltInRoutes = routes.labels, routes.builtInLabels *a.httpServer.router.RegisteredRoutes = registeredMethods } diff --git a/pkg/gofr/rbac.go b/pkg/gofr/rbac.go index ed2435d2da..e2195e18ef 100644 --- a/pkg/gofr/rbac.go +++ b/pkg/gofr/rbac.go @@ -105,27 +105,73 @@ func (a *App) rbacRouteCheckMode() string { } } -// rbacRouteCollector builds the route list the RBAC route check reads, from the router walk +// rbacRouteCollector builds the route lists the RBAC route check reads, from the router walk // httpServerSetup already makes. A disabled collector adds nothing. type rbacRouteCollector struct { enabled bool seen map[string]bool - labels []string + + // 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 + + labels, builtInLabels []string } func newRBACRouteCollector(enabled bool) *rbacRouteCollector { return &rbacRouteCollector{enabled: enabled, seen: make(map[string]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 as one "METHOD /template" entry per method, "*" for a route with no method -// matcher. 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. +// matcher, among the built-in routes 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() + tmpl, err := route.GetPathTemplate() if err != nil { return @@ -152,6 +198,11 @@ func (c *rbacRouteCollector) add(route *mux.Route, methods []string) { } c.seen[label] = true - c.labels = append(c.labels, label) + + if builtIn { + c.builtInLabels = append(c.builtInLabels, label) + } else { + c.labels = append(c.labels, label) + } } } diff --git a/pkg/gofr/rbac/coverage.go b/pkg/gofr/rbac/coverage.go index cf4f16b615..54eea5b2eb 100644 --- a/pkg/gofr/rbac/coverage.go +++ b/pkg/gofr/rbac/coverage.go @@ -2,6 +2,8 @@ package rbac import ( "regexp" + "regexp/syntax" + "slices" "sort" "strings" ) @@ -10,13 +12,19 @@ import ( // without a method matcher, such as a static-file prefix. const anyMethod = "*" -// faviconPath is the built-in favicon route GoFr registers itself. -const faviconPath = "/favicon.ico" +// freeVariablePattern is what mux matches a "{name}" variable against in a path. +const freeVariablePattern = "[^/]+" -// ReportRouteMismatches compares the config against routes, the complete route table, and logs -// every mismatch it finds. Each route is "METHOD /template" in mux syntax, with "*" for a route -// registered without a method matcher; a prefix route is passed with a trailing catch-all segment, -// as "/static/{path:.*}". It reports whether any mismatch was logged as an error. +// invalidPattern stands for the pattern of a segment mux would refuse. It does not compile, so the +// segment covers nothing. +const invalidPattern = "[" + +// ReportRouteMismatches compares the config against the complete route table and logs every +// mismatch it finds. routes are the routes the application registered and builtInRoutes the ones +// GoFr registered itself, which need no rule but keep a rule written for them live. Each route is +// "METHOD /template" in mux syntax, with "*" for a route registered without a method matcher; a +// prefix route is passed with a trailing catch-all segment, as "/static/{path:.*}". It reports +// whether any mismatch was logged as an error. // // It logs one line for each kind of mismatch, listing its entries as "METHOD path": // @@ -26,13 +34,12 @@ const faviconPath = "/favicon.ico" // - an uncovered route is matched by no rule, so it is served without role checks. // - a partly covered route is matched by rules for some of its requests but not all of them. // -// GoFr's own /.well-known/* routes and /favicon.ico need no rule. -// // 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 it matches every request the route serves. -func (c *Config) ReportRouteMismatches(routes []string) bool { +// 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 (c *Config) ReportRouteMismatches(routes, builtInRoutes []string) bool { rc := newRouteChecker() rules := make([]routeEntry, len(c.rules)) @@ -40,29 +47,25 @@ func (c *Config) ReportRouteMismatches(routes []string) bool { rules[i] = rc.newEntry(c.rules[i].method, c.rules[i].pattern) } - parsed := make([]routeEntry, len(routes)) - for j, route := range routes { - parsed[j] = rc.parseEntry(route) - } - var ( live = make([]bool, len(rules)) uncovered, partial []string deadGuards, deadOpen []string ) - for j := range parsed { - if isBuiltInRoute(parsed[j].rawPath) { - markLiveRules(rules, &parsed[j], live) + for _, route := range builtInRoutes { + entry := rc.parseEntry(route) + markLiveRules(rules, &entry, live) + } - continue - } + for _, route := range routes { + entry := rc.parseEntry(route) - switch covering := classifyRoute(rules, &parsed[j], live); { + switch covering := classifyRoute(rules, &entry, live); { case covering == nil: - uncovered = append(uncovered, parsed[j].label) + uncovered = append(uncovered, entry.label) case len(covering) > 0: - partial = append(partial, parsed[j].label+" (by "+strings.Join(sortedUnique(covering), " and ")+")") + partial = append(partial, entry.label+" (by "+strings.Join(sortedUnique(covering), " and ")+")") } } @@ -163,20 +166,23 @@ func sortedUnique(entries []string) []string { return out } -// isBuiltInRoute reports whether a template belongs to a route GoFr registers itself, which is left -// out of the coverage report because the application did not write it. -func isBuiltInRoute(template string) bool { - return strings.HasPrefix(template, "/.well-known/") || template == faviconPath -} - // 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]*regexp.Regexp + 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]*regexp.Regexp)} + 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. @@ -184,13 +190,10 @@ type routeEntry struct { // method is the upper-cased method, or anyMethod. method string - // rawPath is the path as written. - rawPath string - // label is how the entry is reported: "METHOD path". label string - // segments is rawPath split on "/", empty segments kept. Nil for an empty path, which matches + // segments is the path split on "/", empty segments kept. Nil for an empty path, which matches // nothing. segments []pathSegment } @@ -200,13 +203,17 @@ type pathSegment struct { // text is the segment as written. text string - // variable is set for a "{name}" or "{name:constraint}" segment. + // variable is set for a segment holding any "{...}" variable. variable bool - // catchAll is set for a variable that can span several segments. See isCatchAllVariable. + // 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 the variable's constraint as written, trimmed; empty for a free variable. + // 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 @@ -225,7 +232,7 @@ func (rc *routeChecker) parseEntry(s string) routeEntry { } func (rc *routeChecker) newEntry(method, path string) routeEntry { - entry := routeEntry{method: method, rawPath: path, label: method + " " + path} + entry := routeEntry{method: method, label: method + " " + path} if path == "" { return entry @@ -244,45 +251,154 @@ func (rc *routeChecker) newEntry(method, path string) routeEntry { func (rc *routeChecker) newSegment(text string) pathSegment { seg := pathSegment{text: text} - if !isVariableSegment(text) { + if !strings.Contains(text, "{") { return seg } seg.variable = true - seg.catchAll = isCatchAllVariable(text) + seg.constraint = segmentPattern(text) - inner := text[1 : len(text)-1] + if seg.constraint != "" { + compiled := rc.compile(seg.constraint) + seg.re, seg.catchAll = compiled.re, compiled.re != nil && compiled.spans + } - idx := strings.Index(inner, ":") - if idx < 0 { - return seg + 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 } - seg.constraint = strings.TrimSpace(inner[idx+1:]) - seg.re = rc.compile(seg.constraint) + 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 + } - return seg + 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() } -// compile returns constraint anchored to a whole segment, as mux applies it, or nil when it does -// not compile. -func (rc *routeChecker) compile(constraint string) *regexp.Regexp { - if re, ok := rc.constraints[constraint]; ok { - return re +// 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}) + } + } } - re, err := regexp.Compile("^(?:" + constraint + ")$") + 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 { - re = 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 } - rc.constraints[constraint] = re + if re.Op == syntax.OpLiteral { + return slices.Contains(re.Rune, r) + } - return re + 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 } -func isVariableSegment(segment string) bool { - return strings.HasPrefix(segment, "{") && strings.HasSuffix(segment, "}") +// 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. @@ -364,8 +480,10 @@ func segmentsCover(a, b []pathSegment) bool { 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 a[i].coversTail(b[i:]) + return i == len(a)-1 && a[i].coversTail(b[i:]) } if !a[i].covers(&b[i]) { @@ -385,7 +503,7 @@ func (s *pathSegment) covers(other *pathSegment) bool { return false case s.constraint == "": // A free variable matches any non-empty segment, so it covers a constraint that cannot - // match an empty one. + // 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 diff --git a/pkg/gofr/rbac/coverage_test.go b/pkg/gofr/rbac/coverage_test.go index 4da303f2eb..0dc1ebecb8 100644 --- a/pkg/gofr/rbac/coverage_test.go +++ b/pkg/gofr/rbac/coverage_test.go @@ -9,8 +9,8 @@ import ( "github.com/stretchr/testify/assert" ) -// coverageRoutes is a route table shaped like the one GoFr hands the check: one entry per method -// and path, the built-in well-known routes, and a static directory served at /static. +// coverageRoutes is an application route table shaped like the one GoFr hands the check: one entry +// per method and path, and a static directory served at /static. func coverageRoutes() []string { return []string{ "GET /api/users", @@ -18,14 +18,20 @@ func coverageRoutes() []string { "GET /api/users/{id}", "DELETE /api/users/{id}", "GET /api/posts", - "GET /.well-known/health", - "GET /.well-known/alive", - "GET " + faviconPath, "* /static", "* /static/{path:.*}", } } +// coverageBuiltInRoutes is the part of the route table GoFr registers itself. +func coverageBuiltInRoutes() []string { + return []string{ + "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) EndpointMapping { return EndpointMapping{Path: path, Methods: methods, RequiredPermissions: []string{"p:x"}} @@ -53,6 +59,7 @@ const ( type routeMismatchCase struct { desc string endpoints []EndpointMapping + extraRoutes []string // application routes added to coverageRoutes for this case wantDead []string // entries expected on the dead-rule line wantUncovered []string // entries expected on the uncovered-route line wantPartial []string // entries expected on the partly-covered line @@ -127,6 +134,13 @@ func routeMismatchCases() []routeMismatchCase { guard("/static/{path:.*}", http.MethodGet, http.MethodHead)), wantPartial: []string{"* /static/{path:.*} (by GET /static/{path:.*} and HEAD /static/{path:.*})"}, }, + } +} + +// builtInAndPublicCases are the cases of TestConfig_ReportRouteMismatches about built-in routes, +// public rules, and more than one kind of mismatch at once. +func builtInAndPublicCases() []routeMismatchCase { + return []routeMismatchCase{ { desc: "a rule for a built-in route is live", endpoints: append(coveringRules(), @@ -152,7 +166,13 @@ func routeMismatchCases() []routeMismatchCase { guard("/static/{path:.*}", "*"), }, wantUncovered: []string{"GET /api/posts", "POST /api/users"}, - notLogged: []string{"/.well-known", faviconPath, "GET /api/users,", "/api/users/{id}"}, + notLogged: []string{"/.well-known", "/favicon.ico", "GET /api/users,", "/api/users/{id}"}, + }, + { + desc: "an application route under /.well-known is checked", + endpoints: coveringRules(), + extraRoutes: []string{"GET /.well-known/custom"}, + wantUncovered: []string{"GET /.well-known/custom"}, }, { desc: "dead rules and uncovered routes are both reported", @@ -164,13 +184,13 @@ func routeMismatchCases() []routeMismatchCase { } func TestConfig_ReportRouteMismatches(t *testing.T) { - for i, tc := range routeMismatchCases() { + for i, tc := range append(routeMismatchCases(), builtInAndPublicCases()...) { t.Run(tc.desc, func(t *testing.T) { logger := &mockLogger{} config := newTestConfig(t, tc.endpoints, nil) config.Logger = logger - got := config.ReportRouteMismatches(coverageRoutes()) + got := config.ReportRouteMismatches(append(coverageRoutes(), tc.extraRoutes...), coverageBuiltInRoutes()) wantMismatch := len(tc.wantDead)+len(tc.wantUncovered)+len(tc.wantPartial) > 0 assert.Equal(t, wantMismatch, got, "TEST[%d], Failed.\n%s", i, tc.desc) @@ -197,7 +217,7 @@ func TestConfig_ReportRouteMismatches(t *testing.T) { func TestConfig_ReportRouteMismatches_NilLogger(t *testing.T) { config := newTestConfig(t, coveringRules()[1:], nil) - assert.True(t, config.ReportRouteMismatches(coverageRoutes())) + assert.True(t, config.ReportRouteMismatches(coverageRoutes(), coverageBuiltInRoutes())) } // assertLogLine checks that exactly one line carries marker and lists every entry of want when want @@ -258,6 +278,10 @@ func Test_routeEntry_overlaps(t *testing.T) { {"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}, @@ -304,6 +328,14 @@ func Test_routeEntry_covers(t *testing.T) { {"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}, @@ -340,7 +372,7 @@ func BenchmarkConfig_ReportRouteMismatches(b *testing.B) { b.ReportAllocs() for b.Loop() { - if config.ReportRouteMismatches(routes) { + if config.ReportRouteMismatches(routes, nil) { b.Fatal("every route is covered, so no mismatch is expected") } } diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index af1a0683a5..c71e1d657a 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -247,6 +247,13 @@ func TestApp_prepareHTTPServer(t *testing.T) { setup: func(a *App) { a.AddStaticFiles("/", t.TempDir()) }, wantLog: []string{routeCheckFailed, "* /{path:.*}"}, }, + { + 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"}, + }, } for i, tc := range tests { @@ -257,22 +264,7 @@ func TestApp_prepareHTTPServer(t *testing.T) { t.Setenv("GOFR_RBAC_ROUTE_CHECK", tc.mode) } - path := writeRBACConfig(t, t.TempDir(), "rbac.json", tc.config) - - var outcome startupOutcome - - stderr := testutil.StderrOutputForFunc(func() { - a := New() - a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) - - if tc.setup != nil { - tc.setup(a) - } - - require.NoError(t, a.EnableRBAC(path)) - - outcome = a.prepareHTTPServer() - }) + outcome, stderr := runPrepareHTTPServer(t, tc.config, tc.setup) assert.Equal(t, tc.wantOutcome, outcome, "TEST[%d], Failed.\n%s", i, tc.desc) @@ -287,6 +279,29 @@ func TestApp_prepareHTTPServer(t *testing.T) { } } +// 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 stderr. +func runPrepareHTTPServer(t *testing.T, config string, setup func(*App)) (outcome startupOutcome, stderr string) { + t.Helper() + + path := writeRBACConfig(t, t.TempDir(), "rbac.json", config) + + stderr = testutil.StderrOutputForFunc(func() { + a := New() + a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + + if setup != nil { + setup(a) + } + + require.NoError(t, a.EnableRBAC(path)) + + outcome = a.prepareHTTPServer() + }) + + return outcome, 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. @@ -309,3 +324,47 @@ func TestRun_RBACRouteCheckFailExitsNonZero(t *testing.T) { assert.Equal(t, []int{exitCodeStartupFailed}, 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 396674ac97..d19ac129c0 100644 --- a/pkg/gofr/run.go +++ b/pkg/gofr/run.go @@ -69,22 +69,26 @@ 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.bindMCPServer(ctx); outcome != startupOK { + if outcome := a.prepareHTTPServer(); outcome != startupOK { a.finishAbandonedStartup(outcome) return } - if outcome := a.prepareHTTPServer(); outcome != startupOK { + if outcome := a.bindMCPServer(ctx); outcome != startupOK { a.finishAbandonedStartup(outcome) return @@ -312,7 +316,7 @@ func (a *App) prepareHTTPServer() startupOutcome { return startupOK } - if !a.rbacConfig.ReportRouteMismatches(a.rbacRoutes) || mode != rbacRouteCheckFail { + if !a.rbacConfig.ReportRouteMismatches(a.rbacRoutes, a.rbacBuiltInRoutes) || mode != rbacRouteCheckFail { return startupOK } @@ -385,6 +389,10 @@ func (a *App) startHTTPServer(wg *sync.WaitGroup) { if a.httpRegistered { wg.Add(1) + 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() From b80630946ff2650b8f582d577f6a2e037be3f403 Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Tue, 13 Oct 2026 11:26:27 +0530 Subject: [PATCH 07/11] refactor(rbac): move the startup route check out of the public API The route check now lives in pkg/gofr/internal/rbaccheck. Check takes typed routes (Method, Path, BuiltIn) and returns the mismatches it found instead of logging them; prepareHTTPServer writes the log lines and applies GOFR_RBAC_ROUTE_CHECK. Config.ReportRouteMismatches is removed, so the PR adds no exported API to the rbac package. The check rebuilds rules from Config.Endpoints the way the matcher does: no methods means every method, methods are upper-cased, and a repeated method and path replaces the earlier one. TestCheck_AgreesWithMiddleware drives the real loader and middleware and fails if the two disagree. Docs: add an upgrade note for the http.route change on the rbac.authorize span, and the known gap for a route registered at "/" (tracked in #4457). --- docs/advanced-guide/rbac/page.md | 25 +- pkg/gofr/gofr.go | 8 +- pkg/gofr/internal/rbaccheck/check.go | 195 +++++++++ pkg/gofr/internal/rbaccheck/check_test.go | 299 ++++++++++++++ .../rbaccheck/match.go} | 189 ++------- pkg/gofr/internal/rbaccheck/match_test.go | 120 ++++++ pkg/gofr/rbac.go | 57 ++- pkg/gofr/rbac/coverage_test.go | 381 ------------------ pkg/gofr/rbac/middleware_test.go | 6 +- pkg/gofr/rbac_test.go | 86 ++-- pkg/gofr/run.go | 2 +- 11 files changed, 778 insertions(+), 590 deletions(-) create mode 100644 pkg/gofr/internal/rbaccheck/check.go create mode 100644 pkg/gofr/internal/rbaccheck/check_test.go rename pkg/gofr/{rbac/coverage.go => internal/rbaccheck/match.go} (67%) create mode 100644 pkg/gofr/internal/rbaccheck/match_test.go delete mode 100644 pkg/gofr/rbac/coverage_test.go diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index 1a96f69441..4ef11ada90 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -730,7 +730,9 @@ RBAC middleware implements industry-standard security practices to protect sensi **Traces (OpenTelemetry):** - ✅ 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. + When the two disagree, the rule is broader or narrower than the route. `http.route` is + `` when no route matched, and for now also for an application route registered at `/` + (see the upgrade note below). - ✅ Authorization status (allowed/denied) included - ❌ Roles excluded (privacy protection - roles are PII) - ❌ Error messages sanitized (prevent information leakage) @@ -746,6 +748,27 @@ 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. + +One known gap: GoFr answers a request no route serves with a catch-all route whose template is `/`, +the same template as an application route registered at `/`. The middleware cannot tell the two +apart yet, so it reports both as `` rather than label every unmatched request as `/`. +Requests to an application route at `/` are still authorized normally; only the span label is +affected. + ### What's Never Logged RBAC middleware never logs: diff --git a/pkg/gofr/gofr.go b/pkg/gofr/gofr.go index 85d00b4f92..a30262e9fa 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -19,6 +19,7 @@ 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/logging" "gofr.dev/pkg/gofr/metrics" "gofr.dev/pkg/gofr/migration" @@ -84,10 +85,9 @@ type App struct { // complete route table. See prepareHTTPServer. rbacConfig *rbac.Config - // rbacRoutes is the route table as the RBAC route check reads it, "METHOD /template" per entry, - // and rbacBuiltInRoutes the part of it GoFr registered itself. httpServerSetup fills both from + // rbacRoutes is the route table as the RBAC route check reads it. httpServerSetup fills it from // the walk it already makes, and only when rbacConfig is set. - rbacRoutes, rbacBuiltInRoutes []string + rbacRoutes []rbaccheck.Route // exit is os.Exit, indirected so a test can observe the status a failed startup reports without // taking the test binary down with it. Nil means os.Exit, which is what every real app uses. @@ -277,7 +277,7 @@ func (a *App) httpServerSetup() { return nil }) - a.rbacRoutes, a.rbacBuiltInRoutes = routes.labels, routes.builtInLabels + a.rbacRoutes = routes.routes *a.httpServer.router.RegisteredRoutes = registeredMethods } diff --git a/pkg/gofr/internal/rbaccheck/check.go b/pkg/gofr/internal/rbaccheck/check.go new file mode 100644 index 0000000000..38fad3d7dc --- /dev/null +++ b/pkg/gofr/internal/rbaccheck/check.go @@ -0,0 +1,195 @@ +// 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 ( + "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 +} + +// 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..5ac2c97b0d --- /dev/null +++ b/pkg/gofr/internal/rbaccheck/check_test.go @@ -0,0 +1,299 @@ +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 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/rbac/coverage.go b/pkg/gofr/internal/rbaccheck/match.go similarity index 67% rename from pkg/gofr/rbac/coverage.go rename to pkg/gofr/internal/rbaccheck/match.go index 54eea5b2eb..b01c48d98d 100644 --- a/pkg/gofr/rbac/coverage.go +++ b/pkg/gofr/internal/rbaccheck/match.go @@ -1,10 +1,9 @@ -package rbac +package rbaccheck import ( "regexp" "regexp/syntax" "slices" - "sort" "strings" ) @@ -19,153 +18,6 @@ const freeVariablePattern = "[^/]+" // segment covers nothing. const invalidPattern = "[" -// ReportRouteMismatches compares the config against the complete route table and logs every -// mismatch it finds. routes are the routes the application registered and builtInRoutes the ones -// GoFr registered itself, which need no rule but keep a rule written for them live. Each route is -// "METHOD /template" in mux syntax, with "*" for a route registered without a method matcher; a -// prefix route is passed with a trailing catch-all segment, as "/static/{path:.*}". It reports -// whether any mismatch was logged as an error. -// -// It logs one line for each kind of mismatch, listing its entries as "METHOD path": -// -// - a dead rule matches no route - usually a typo in its path, or a method the route does not -// register - so the route it was written for is unguarded. A dead public rule cannot leave a -// route unguarded, so it is only warned about. -// - an uncovered route is matched by no rule, so it is served without role checks. -// - a partly covered route is matched by rules for some of its requests but not all of them. -// -// 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 (c *Config) ReportRouteMismatches(routes, builtInRoutes []string) bool { - rc := newRouteChecker() - - rules := make([]routeEntry, len(c.rules)) - for i := range c.rules { - rules[i] = rc.newEntry(c.rules[i].method, c.rules[i].pattern) - } - - var ( - live = make([]bool, len(rules)) - uncovered, partial []string - deadGuards, deadOpen []string - ) - - for _, route := range builtInRoutes { - entry := rc.parseEntry(route) - markLiveRules(rules, &entry, live) - } - - for _, route := range routes { - entry := rc.parseEntry(route) - - switch covering := classifyRoute(rules, &entry, live); { - case covering == nil: - uncovered = append(uncovered, entry.label) - case len(covering) > 0: - partial = append(partial, entry.label+" (by "+strings.Join(sortedUnique(covering), " and ")+")") - } - } - - for i := range rules { - switch { - case live[i]: - case c.rules[i].isPublic: - deadOpen = append(deadOpen, rules[i].label) - default: - deadGuards = append(deadGuards, rules[i].label) - } - } - - c.logRouteMismatches(sortedUnique(deadGuards), sortedUnique(deadOpen), sortedUnique(uncovered), sortedUnique(partial)) - - return len(deadGuards)+len(uncovered)+len(partial) > 0 -} - -// logRouteMismatches writes one line for each non-empty kind of mismatch. -func (c *Config) logRouteMismatches(deadGuards, deadOpen, uncovered, partial []string) { - if c.Logger == nil { - return - } - - if len(deadOpen) > 0 { - c.Logger.Warnf("RBAC: public rules match no registered route: %s", strings.Join(deadOpen, ", ")) - } - - if len(deadGuards) > 0 { - c.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(deadGuards, ", ")) - } - - if len(uncovered) > 0 { - c.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(uncovered, ", ")) - } - - if len(partial) > 0 { - c.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(partial, ", ")) - } -} - -// 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 []routeEntry, route *routeEntry, live []bool) []string { - covered, overlapping := false, []string{} - - for i := range rules { - if !rules[i].overlaps(route) { - continue - } - - live[i] = true - - switch { - case covered: - case rules[i].covers(route): - covered = true - default: - overlapping = append(overlapping, rules[i].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 []routeEntry, route *routeEntry, live []bool) { - for i := range rules { - if !live[i] && rules[i].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 -} - // 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 { @@ -221,16 +73,6 @@ type pathSegment struct { re *regexp.Regexp } -// parseEntry parses "METHOD path". A string with no space is a path with anyMethod. -func (rc *routeChecker) parseEntry(s string) routeEntry { - method, path, found := strings.Cut(s, " ") - if !found { - method, path = anyMethod, s - } - - return rc.newEntry(strings.ToUpper(method), path) -} - func (rc *routeChecker) newEntry(method, path string) routeEntry { entry := routeEntry{method: method, label: method + " " + path} @@ -554,3 +396,32 @@ func (s *pathSegment) mayBeEmpty() bool { 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/rbac.go b/pkg/gofr/rbac.go index e2195e18ef..b425e9c7f4 100644 --- a/pkg/gofr/rbac.go +++ b/pkg/gofr/rbac.go @@ -8,6 +8,7 @@ import ( "github.com/gorilla/mux" "go.opentelemetry.io/otel" + "gofr.dev/pkg/gofr/internal/rbaccheck" "gofr.dev/pkg/gofr/rbac" ) @@ -105,11 +106,40 @@ func (a *App) rbacRouteCheckMode() string { } } -// rbacRouteCollector builds the route lists the RBAC route check reads, from the router walk +// 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 { + m := rbaccheck.Check(a.rbacConfig.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[string]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 @@ -117,11 +147,11 @@ type rbacRouteCollector struct { // /.well-known/ is still checked. builtInFrom, builtInTo, walked int - labels, builtInLabels []string + routes []rbaccheck.Route } func newRBACRouteCollector(enabled bool) *rbacRouteCollector { - return &rbacRouteCollector{enabled: enabled, seen: make(map[string]bool)} + 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. @@ -160,9 +190,9 @@ func countRoutes(router *mux.Router) int { return n } -// add records route as one "METHOD /template" entry per method, "*" for a route with no method -// matcher, among the built-in routes 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. +// 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) { @@ -192,17 +222,12 @@ func (c *rbacRouteCollector) add(route *mux.Route, methods []string) { } for _, method := range methods { - label := strings.ToUpper(method) + " " + tmpl - if c.seen[label] { + r := rbaccheck.Route{Method: strings.ToUpper(method), Path: tmpl, BuiltIn: builtIn} + if c.seen[r] { continue } - c.seen[label] = true - - if builtIn { - c.builtInLabels = append(c.builtInLabels, label) - } else { - c.labels = append(c.labels, label) - } + c.seen[r] = true + c.routes = append(c.routes, r) } } diff --git a/pkg/gofr/rbac/coverage_test.go b/pkg/gofr/rbac/coverage_test.go deleted file mode 100644 index 0dc1ebecb8..0000000000 --- a/pkg/gofr/rbac/coverage_test.go +++ /dev/null @@ -1,381 +0,0 @@ -package rbac - -import ( - "fmt" - "net/http" - "strings" - "testing" - - "github.com/stretchr/testify/assert" -) - -// coverageRoutes is an application route table shaped like the one GoFr hands the check: one entry -// per method and path, and a static directory served at /static. -func coverageRoutes() []string { - return []string{ - "GET /api/users", - "POST /api/users", - "GET /api/users/{id}", - "DELETE /api/users/{id}", - "GET /api/posts", - "* /static", - "* /static/{path:.*}", - } -} - -// coverageBuiltInRoutes is the part of the route table GoFr registers itself. -func coverageBuiltInRoutes() []string { - return []string{ - "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) EndpointMapping { - return EndpointMapping{Path: path, Methods: methods, RequiredPermissions: []string{"p:x"}} -} - -// coveringRules guards every non-built-in route in coverageRoutes. -func coveringRules() []EndpointMapping { - return []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:.*}", "*"), - } -} - -// Markers that tell the three error lines apart. -const ( - deadLine = "rules match no registered route" - uncoveredLine = "routes covered by no rule" - partialLine = "routes only partly covered" -) - -// routeMismatchCase is one case of TestConfig_ReportRouteMismatches. -type routeMismatchCase struct { - desc string - endpoints []EndpointMapping - extraRoutes []string // application routes added to coverageRoutes for this case - wantDead []string // entries expected on the dead-rule line - wantUncovered []string // entries expected on the uncovered-route line - wantPartial []string // entries expected on the partly-covered line - notLogged []string // substrings that must appear in no error line - wantWarn []string // substrings expected in the warning output -} - -func routeMismatchCases() []routeMismatchCase { - return []routeMismatchCase{ - { - 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)), - wantDead: []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)), - wantDead: []string{"DELETE /api/posts"}, - }, - { - desc: "a wildcard-method rule is live and covers every method", - endpoints: []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)), - wantDead: []string{"GET /api/users/", "POST /api/users/"}, - wantUncovered: []string{"GET /api/users", "POST /api/users"}, - }, - { - desc: "a relative path matches nothing", - endpoints: append(coveringRules()[1:], - guard("api/users", http.MethodGet, http.MethodPost)), - wantDead: []string{"GET api/users", "POST api/users"}, - wantUncovered: []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:.*}", "*")), - wantUncovered: []string{"* /static"}, - notLogged: []string{"* /static/{path:.*}"}, - }, - { - desc: "a constraint narrower than the route covers it only partly", - endpoints: []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:.*}", "*"), - }, - wantPartial: []string{ - "GET /api/users/{id} (by GET /api/users/{id:[0-9]+})", - "DELETE /api/users/{id} (by DELETE /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)), - wantPartial: []string{"* /static/{path:.*} (by GET /static/{path:.*} and HEAD /static/{path:.*})"}, - }, - } -} - -// builtInAndPublicCases are the cases of TestConfig_ReportRouteMismatches about built-in routes, -// public rules, and more than one kind of mismatch at once. -func builtInAndPublicCases() []routeMismatchCase { - return []routeMismatchCase{ - { - desc: "a rule for a built-in route is live", - endpoints: append(coveringRules(), - EndpointMapping{Path: "/.well-known/health", Methods: []string{http.MethodGet}, Public: true}), - }, - { - desc: "a dead public rule is a warning, not an error", - endpoints: append(coveringRules(), - EndpointMapping{Path: "/healthz", Methods: []string{http.MethodGet}, Public: true}), - wantWarn: []string{"GET /healthz"}, - }, - { - desc: "a public rule covers its route", - endpoints: append(coveringRules()[1:], - EndpointMapping{Path: "/api/users", Methods: []string{"*"}, Public: true}), - }, - { - desc: "uncovered routes are an error, built-in routes left out", - endpoints: []EndpointMapping{ - guard("/api/users", http.MethodGet), - guard("/api/users/{id}", "*"), - guard("/static", "*"), - guard("/static/{path:.*}", "*"), - }, - wantUncovered: []string{"GET /api/posts", "POST /api/users"}, - notLogged: []string{"/.well-known", "/favicon.ico", "GET /api/users,", "/api/users/{id}"}, - }, - { - desc: "an application route under /.well-known is checked", - endpoints: coveringRules(), - extraRoutes: []string{"GET /.well-known/custom"}, - wantUncovered: []string{"GET /.well-known/custom"}, - }, - { - desc: "dead rules and uncovered routes are both reported", - endpoints: append(coveringRules()[1:], guard("/api/user", http.MethodGet)), - wantDead: []string{"GET /api/user"}, - wantUncovered: []string{"GET /api/users", "POST /api/users"}, - }, - } -} - -func TestConfig_ReportRouteMismatches(t *testing.T) { - for i, tc := range append(routeMismatchCases(), builtInAndPublicCases()...) { - t.Run(tc.desc, func(t *testing.T) { - logger := &mockLogger{} - config := newTestConfig(t, tc.endpoints, nil) - config.Logger = logger - - got := config.ReportRouteMismatches(append(coverageRoutes(), tc.extraRoutes...), coverageBuiltInRoutes()) - - wantMismatch := len(tc.wantDead)+len(tc.wantUncovered)+len(tc.wantPartial) > 0 - assert.Equal(t, wantMismatch, got, "TEST[%d], Failed.\n%s", i, tc.desc) - - assertLogLine(t, logger.errorLogs, deadLine, tc.wantDead, "TEST[%d], Failed.\n%s", i, tc.desc) - assertLogLine(t, logger.errorLogs, uncoveredLine, tc.wantUncovered, "TEST[%d], Failed.\n%s", i, tc.desc) - assertLogLine(t, logger.errorLogs, partialLine, tc.wantPartial, "TEST[%d], Failed.\n%s", i, tc.desc) - - errors := strings.Join(logger.errorLogs, "\n") - for _, unwanted := range tc.notLogged { - assert.NotContains(t, errors, unwanted, "TEST[%d], Failed.\n%s", i, tc.desc) - } - - warnings := strings.Join(logger.warnLogs, "\n") - for _, want := range tc.wantWarn { - assert.Contains(t, warnings, want, "TEST[%d], Failed.\n%s", i, tc.desc) - } - }) - } -} - -// TestConfig_ReportRouteMismatches_NilLogger checks that a config with no logger still reports the -// mismatch through its result instead of panicking. -func TestConfig_ReportRouteMismatches_NilLogger(t *testing.T) { - config := newTestConfig(t, coveringRules()[1:], nil) - - assert.True(t, config.ReportRouteMismatches(coverageRoutes(), coverageBuiltInRoutes())) -} - -// assertLogLine checks that exactly one line carries marker and lists every entry of want when want -// is non-empty, and that no line carries marker otherwise. -func assertLogLine(t *testing.T, lines []string, marker string, want []string, msgAndArgs ...any) { - t.Helper() - - var matched []string - - for _, line := range lines { - if strings.Contains(line, marker) { - matched = append(matched, line) - } - } - - if len(want) == 0 { - assert.Empty(t, matched, msgAndArgs...) - - return - } - - if !assert.Len(t, matched, 1, msgAndArgs...) { - return - } - - for _, entry := range want { - assert.Contains(t, matched[0], entry, msgAndArgs...) - } -} - -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 := rc.parseEntry(tc.rule), rc.parseEntry(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 := rc.parseEntry(tc.rule), rc.parseEntry(tc.route) - - assert.Equal(t, tc.want, rule.covers(&route), "TEST[%d], Failed.\n%s", i, tc.desc) - }) - } -} - -// BenchmarkConfig_ReportRouteMismatches 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 BenchmarkConfig_ReportRouteMismatches(b *testing.B) { - for _, n := range []int{50, 200, 1000} { - b.Run(fmt.Sprintf("routes=%d", n), func(b *testing.B) { - endpoints := make([]EndpointMapping, 0, n) - routes := make([]string, 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, "GET "+path) - } - - config := newTestConfig(b, endpoints, nil) - - b.ReportAllocs() - - for b.Loop() { - if config.ReportRouteMismatches(routes, nil) { - b.Fatal("every route is covered, so no mismatch is expected") - } - } - }) - } -} diff --git a/pkg/gofr/rbac/middleware_test.go b/pkg/gofr/rbac/middleware_test.go index e94345f370..568a18e2de 100644 --- a/pkg/gofr/rbac/middleware_test.go +++ b/pkg/gofr/rbac/middleware_test.go @@ -863,7 +863,6 @@ func TestHandleAuthError(t *testing.T) { // mockLogger implements the datasource.Logger interface for testing. type mockLogger struct { errorLogs []string - warnLogs []string infoLogs []string // Capture actual log messages logs []string infoArgs []any // Capture structured log arguments (used for both Info and Debug) @@ -895,10 +894,7 @@ func (m *mockLogger) Errorf(format string, args ...any) { func (m *mockLogger) Warn(_ ...any) { m.logs = append(m.logs, "WARN") } -func (m *mockLogger) Warnf(format string, args ...any) { - m.logs = append(m.logs, "WARNF") - m.warnLogs = append(m.warnLogs, fmt.Sprintf(format, args...)) -} +func (m *mockLogger) Warnf(_ string, _ ...any) { m.logs = append(m.logs, "WARNF") } func TestMiddleware_WithTracing(t *testing.T) { t.Run("starts tracing when tracer is available", func(t *testing.T) { diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index c71e1d657a..f3d3225549 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -182,19 +182,37 @@ 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 + `]}` + // routeCheckFailed starts every ERROR line the route check writes. const routeCheckFailed = "RBAC route check failed" -func TestApp_prepareHTTPServer(t *testing.T) { - tests := []struct { - desc string - config string - 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 - wantOutcome startupOutcome - }{ +// prepareHTTPServerCase is one case of TestApp_prepareHTTPServer. +type prepareHTTPServerCase struct { + desc string + config string + 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, @@ -247,6 +265,19 @@ func TestApp_prepareHTTPServer(t *testing.T) { setup: func(a *App) { a.AddStaticFiles("/", t.TempDir()) }, wantLog: []string{routeCheckFailed, "* /{path:.*}"}, }, + { + 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, @@ -255,8 +286,10 @@ func TestApp_prepareHTTPServer(t *testing.T) { notLog: []string{"/.well-known/health", "/.well-known/alive", "favicon.ico"}, }, } +} - for i, tc := range tests { +func TestApp_prepareHTTPServer(t *testing.T) { + for i, tc := range prepareHTTPServerCases(t) { t.Run(tc.desc, func(t *testing.T) { testutil.NewServerConfigs(t) @@ -264,7 +297,7 @@ func TestApp_prepareHTTPServer(t *testing.T) { t.Setenv("GOFR_RBAC_ROUTE_CHECK", tc.mode) } - outcome, stderr := runPrepareHTTPServer(t, tc.config, tc.setup) + outcome, stdout, stderr := runPrepareHTTPServer(t, tc.config, tc.setup) assert.Equal(t, tc.wantOutcome, outcome, "TEST[%d], Failed.\n%s", i, tc.desc) @@ -275,31 +308,38 @@ func TestApp_prepareHTTPServer(t *testing.T) { 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 stderr. -func runPrepareHTTPServer(t *testing.T, config string, setup func(*App)) (outcome startupOutcome, stderr string) { +// when it is set, and runs prepareHTTPServer, returning its outcome and what it wrote to stdout and +// stderr. +func runPrepareHTTPServer(t *testing.T, config string, setup func(*App)) (outcome startupOutcome, stdout, stderr string) { t.Helper() path := writeRBACConfig(t, t.TempDir(), "rbac.json", config) - stderr = testutil.StderrOutputForFunc(func() { - a := New() - a.GET("/ping", func(*Context) (any, error) { return "pong", nil }) + 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) - } + if setup != nil { + setup(a) + } - require.NoError(t, a.EnableRBAC(path)) + require.NoError(t, a.EnableRBAC(path)) - outcome = a.prepareHTTPServer() + outcome = a.prepareHTTPServer() + }) }) - return outcome, stderr + return outcome, stdout, stderr } // TestRun_RBACRouteCheckFailExitsNonZero checks that GOFR_RBAC_ROUTE_CHECK=fail reaches the diff --git a/pkg/gofr/run.go b/pkg/gofr/run.go index d19ac129c0..9bdfa3899f 100644 --- a/pkg/gofr/run.go +++ b/pkg/gofr/run.go @@ -316,7 +316,7 @@ func (a *App) prepareHTTPServer() startupOutcome { return startupOK } - if !a.rbacConfig.ReportRouteMismatches(a.rbacRoutes, a.rbacBuiltInRoutes) || mode != rbacRouteCheckFail { + if !a.checkRBACRoutes() || mode != rbacRouteCheckFail { return startupOK } From 8843c8fd7b0752869f9df13382b29e9a6786c068 Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Tue, 13 Oct 2026 12:40:47 +0530 Subject: [PATCH 08/11] fix(rbac): report a real route at "/" as "/" on the rbac.authorize span Review on #4390: GoFr's catch-all and an application route at "/" share the template "/", so the span labelled both . - The trie router keeps the matched *mux.Route in the request context instead of its template string (one allocation fewer per request). - New gofrHTTP.IsPrefixRoute tells a PathPrefix route from an exact one, under both routers. - The RBAC span reports only for the catch-all; tests cover a route at "/" under mux and trie. --- docs/advanced-guide/rbac/page.md | 10 +---- pkg/gofr/http/alloc_guard_test.go | 7 +++- pkg/gofr/http/route_context.go | 61 ++++++++++++++++++++------- pkg/gofr/http/route_context_test.go | 64 ++++++++++++++++++++++++++--- pkg/gofr/http/router.go | 6 +-- pkg/gofr/rbac/middleware.go | 7 ++-- pkg/gofr/rbac/middleware_test.go | 3 ++ 7 files changed, 119 insertions(+), 39 deletions(-) diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index 4ef11ada90..d68a826716 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -731,8 +731,8 @@ RBAC middleware implements industry-standard security practices to protect sensi - ✅ 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, and for now also for an application route registered at `/` - (see the upgrade note below). + `` 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) @@ -763,12 +763,6 @@ The value of `http.route` changes in two cases: 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. -One known gap: GoFr answers a request no route serves with a catch-all route whose template is `/`, -the same template as an application route registered at `/`. The middleware cannot tell the two -apart yet, so it reports both as `` rather than label every unmatched request as `/`. -Requests to an application route at `/` are still authorized normally; only the span label is -affected. - ### What's Never Logged RBAC middleware never logs: 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..acbcab5260 100644 --- a/pkg/gofr/http/route_context.go +++ b/pkg/gofr/http/route_context.go @@ -3,24 +3,35 @@ package http import ( "context" "net/http" + "regexp/syntax" "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{} +// routeCtxKey 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, so the route must be +// carried some other way. A pointer is stored rather than the template string: +// it fits in an interface without an allocation. +type routeCtxKey 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 == "" { +// withRoute 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 withRoute(r *http.Request, route *mux.Route) *http.Request { + if route == nil { return r } - return r.WithContext(context.WithValue(r.Context(), routeTemplateCtxKey{}, tmpl)) + return r.WithContext(context.WithValue(r.Context(), routeCtxKey{}, route)) +} + +// matchedRoute returns the route that matched r under either router, or nil. +func matchedRoute(r *http.Request) *mux.Route { + if route, _ := r.Context().Value(routeCtxKey{}).(*mux.Route); route != nil { + return route + } + + return mux.CurrentRoute(r) } // RouteTemplate returns the matched route template for r (e.g. "/users/{id}"), @@ -31,11 +42,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 := matchedRoute(r); route != nil { if tmpl, err := route.GetPathTemplate(); err == nil { return tmpl } @@ -43,3 +50,27 @@ func RouteTemplate(r *http.Request) string { return "" } + +// IsPrefixRoute reports whether the route that matched r was registered with +// PathPrefix, so it serves every path below its template rather than the +// template alone. GoFr's catch-all is such a route at "/", which is how a caller +// tells it apart from an application route registered at "/": both have the +// template "/". It reports false when no route matched. +func IsPrefixRoute(r *http.Request) bool { + route := matchedRoute(r) + if route == nil { + return false + } + + rx, err := route.GetPathRegexp() + if err != nil { + return false + } + + re, err := syntax.Parse(rx, syntax.Perl) + if err != nil { + return false + } + + return !isAnchoredBothEnds(re) +} diff --git a/pkg/gofr/http/route_context_test.go b/pkg/gofr/http/route_context_test.go index e187e832a3..7297c83680 100644 --- a/pkg/gofr/http/route_context_test.go +++ b/pkg/gofr/http/route_context_test.go @@ -6,6 +6,7 @@ import ( "net/http/httptest" "testing" + "github.com/gorilla/mux" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -57,12 +58,63 @@ 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") +} + +// TestIsPrefixRoute tells GoFr's PathPrefix("/") catch-all apart from an +// application route registered at "/": both have the template "/". +func TestIsPrefixRoute(t *testing.T) { + ok := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) }) + + testCases := []struct { + desc string + useTrie bool + path string + want bool + }{ + {"mux: route at /", false, "/", false}, + {"trie: route at /", true, "/", false}, + {"mux: catch-all", false, "/nowhere", true}, + {"trie: catch-all", true, "/nowhere", true}, + {"mux: route with a parameter", false, "/users/42", false}, + {"trie: route with a parameter", true, "/users/42", 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 = IsPrefixRoute(req) + + next.ServeHTTP(w, req) + }) + }) + r.Add(http.MethodGet, "/", ok) + r.Add(http.MethodGet, "/users/{id}", ok) + 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, IsPrefixRoute(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/rbac/middleware.go b/pkg/gofr/rbac/middleware.go index 1f27beee78..fbda05b935 100644 --- a/pkg/gofr/rbac/middleware.go +++ b/pkg/gofr/rbac/middleware.go @@ -28,11 +28,12 @@ const unknownRouteLabel = "" // 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. // -// "/" is reported as unmatched: it is the template of GoFr's PathPrefix("/") catch-all, which -// serves every request no route matched. +// GoFr's PathPrefix("/") catch-all, which serves every request no route matched, is reported as +// unmatched. An application route registered at "/" shares its template, so the two are told apart +// by whether the matched route is a prefix route. func matchedRouteLabel(r *http.Request) string { tmpl := gofrHTTP.RouteTemplate(r) - if tmpl == "" || tmpl == "/" { + if tmpl == "" || (tmpl == "/" && gofrHTTP.IsPrefixRoute(r)) { return unknownRouteLabel } diff --git a/pkg/gofr/rbac/middleware_test.go b/pkg/gofr/rbac/middleware_test.go index 568a18e2de..b540e42e16 100644 --- a/pkg/gofr/rbac/middleware_test.go +++ b/pkg/gofr/rbac/middleware_test.go @@ -1076,6 +1076,8 @@ func TestMiddleware_TraceRouteLabels(t *testing.T) { {"trie: route and rule differ", gofrHTTP.MatcherTrie, "/api/users/42", "/api/users/{id}", "/api/{path:.*}"}, {"mux: no route and no rule", gofrHTTP.MatcherMux, "/nowhere", unknownRouteLabel, unknownRouteLabel}, {"trie: no route and no rule", gofrHTTP.MatcherTrie, "/nowhere", unknownRouteLabel, unknownRouteLabel}, + {"mux: route at / is not the catch-all", gofrHTTP.MatcherMux, "/", "/", unknownRouteLabel}, + {"trie: route at / is not the catch-all", gofrHTTP.MatcherTrie, "/", "/", unknownRouteLabel}, } for i, tc := range testCases { @@ -1094,6 +1096,7 @@ func TestMiddleware_TraceRouteLabels(t *testing.T) { router := gofrHTTP.NewRouter() router.Use(Middleware(config)) + router.Add(http.MethodGet, "/", ok) router.Add(http.MethodGet, "/api/users/{id}", ok) router.PathPrefix("/").Handler(ok) From e4fc649c0e159c34f7dc1d865bd6e641beb27368 Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Tue, 13 Oct 2026 17:48:58 +0530 Subject: [PATCH 09/11] refactor(rbac): mark GoFr's catch-all once instead of probing the route regexp IsPrefixRoute was exported, parsed the route regexp on every 404, and reported a static directory at / as unmatched. The catch-all is now named where httpServerSetup registers it, in an internal package, and the span label and the route collector both ask that one mark. A lookup is a string compare with no allocation. --- pkg/gofr/gofr.go | 5 +- pkg/gofr/http/route_context.go | 57 ++-------------- pkg/gofr/http/route_context_test.go | 39 +++++++---- pkg/gofr/internal/routemark/routemark.go | 58 ++++++++++++++++ pkg/gofr/internal/routemark/routemark_test.go | 40 +++++++++++ pkg/gofr/rbac.go | 9 +-- pkg/gofr/rbac/middleware.go | 9 +-- pkg/gofr/rbac/middleware_test.go | 44 +++++++++--- pkg/gofr/rbac/route_label_bench_test.go | 68 +++++++++++++++++++ 9 files changed, 247 insertions(+), 82 deletions(-) create mode 100644 pkg/gofr/internal/routemark/routemark.go create mode 100644 pkg/gofr/internal/routemark/routemark_test.go create mode 100644 pkg/gofr/rbac/route_label_bench_test.go diff --git a/pkg/gofr/gofr.go b/pkg/gofr/gofr.go index a30262e9fa..c750a2536f 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -20,6 +20,7 @@ import ( "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" @@ -257,7 +258,9 @@ func (a *App) httpServerSetup() { a.logReadiness() - 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, }) diff --git a/pkg/gofr/http/route_context.go b/pkg/gofr/http/route_context.go index acbcab5260..ff4d5a36c5 100644 --- a/pkg/gofr/http/route_context.go +++ b/pkg/gofr/http/route_context.go @@ -1,37 +1,18 @@ package http import ( - "context" "net/http" - "regexp/syntax" "github.com/gorilla/mux" -) -// routeCtxKey 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, so the route must be -// carried some other way. A pointer is stored rather than the template string: -// it fits in an interface without an allocation. -type routeCtxKey struct{} + "gofr.dev/pkg/gofr/internal/routemark" +) -// withRoute 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. +// 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 { - if route == nil { - return r - } - - return r.WithContext(context.WithValue(r.Context(), routeCtxKey{}, route)) -} - -// matchedRoute returns the route that matched r under either router, or nil. -func matchedRoute(r *http.Request) *mux.Route { - if route, _ := r.Context().Value(routeCtxKey{}).(*mux.Route); route != nil { - return route - } - - return mux.CurrentRoute(r) + return routemark.With(r, route) } // RouteTemplate returns the matched route template for r (e.g. "/users/{id}"), @@ -42,7 +23,7 @@ func matchedRoute(r *http.Request) *mux.Route { // via mux.CurrentRoute. Callers get consistent behavior without knowing which // router is active. func RouteTemplate(r *http.Request) string { - if route := matchedRoute(r); route != nil { + if route := routemark.Matched(r); route != nil { if tmpl, err := route.GetPathTemplate(); err == nil { return tmpl } @@ -50,27 +31,3 @@ func RouteTemplate(r *http.Request) string { return "" } - -// IsPrefixRoute reports whether the route that matched r was registered with -// PathPrefix, so it serves every path below its template rather than the -// template alone. GoFr's catch-all is such a route at "/", which is how a caller -// tells it apart from an application route registered at "/": both have the -// template "/". It reports false when no route matched. -func IsPrefixRoute(r *http.Request) bool { - route := matchedRoute(r) - if route == nil { - return false - } - - rx, err := route.GetPathRegexp() - if err != nil { - return false - } - - re, err := syntax.Parse(rx, syntax.Perl) - if err != nil { - return false - } - - return !isAnchoredBothEnds(re) -} diff --git a/pkg/gofr/http/route_context_test.go b/pkg/gofr/http/route_context_test.go index 7297c83680..40002bfca7 100644 --- a/pkg/gofr/http/route_context_test.go +++ b/pkg/gofr/http/route_context_test.go @@ -4,11 +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 @@ -67,23 +72,26 @@ func TestWithRoute_NilIsNoOp(t *testing.T) { assert.NotSame(t, req, withRoute(req, mux.NewRouter().Path("/x/{id}")), "a real route must be recorded") } -// TestIsPrefixRoute tells GoFr's PathPrefix("/") catch-all apart from an -// application route registered at "/": both have the template "/". -func TestIsPrefixRoute(t *testing.T) { +// 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}, - {"trie: route at /", true, "/", false}, - {"mux: catch-all", false, "/nowhere", true}, - {"trie: catch-all", true, "/nowhere", true}, - {"mux: route with a parameter", false, "/users/42", false}, - {"trie: route with a parameter", true, "/users/42", false}, + {"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 { @@ -94,14 +102,21 @@ func TestIsPrefixRoute(t *testing.T) { r.useTrie = tc.useTrie r.Use(func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - got = IsPrefixRoute(req) + got = routemark.CatchAllAnswered(req) next.ServeHTTP(w, req) }) }) r.Add(http.MethodGet, "/", ok) r.Add(http.MethodGet, "/users/{id}", ok) - r.PathPrefix("/").Handler(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() @@ -115,6 +130,6 @@ func TestIsPrefixRoute(t *testing.T) { t.Run("unrouted request", func(t *testing.T) { req := httptest.NewRequestWithContext(context.Background(), http.MethodGet, "/", http.NoBody) - assert.False(t, IsPrefixRoute(req)) + assert.False(t, routemark.CatchAllAnswered(req)) }) } 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 b425e9c7f4..10543ffba6 100644 --- a/pkg/gofr/rbac.go +++ b/pkg/gofr/rbac.go @@ -9,6 +9,7 @@ import ( "go.opentelemetry.io/otel" "gofr.dev/pkg/gofr/internal/rbaccheck" + "gofr.dev/pkg/gofr/internal/routemark" "gofr.dev/pkg/gofr/rbac" ) @@ -202,6 +203,10 @@ func (c *rbacRouteCollector) add(route *mux.Route, methods []string) { builtIn := c.nextIsBuiltIn() + if routemark.IsCatchAll(route) { + return + } + tmpl, err := route.GetPathTemplate() if err != nil { return @@ -210,10 +215,6 @@ func (c *rbacRouteCollector) add(route *mux.Route, methods []string) { // mux anchors a Path regexp with "$" and leaves a PathPrefix one open. pathRegexp, _ := route.GetPathRegexp() if !strings.HasSuffix(pathRegexp, "$") { - if _, isCatchAll := route.GetHandler().(handler); isCatchAll && tmpl == "/" { - return - } - tmpl = strings.TrimSuffix(tmpl, "/") + "/{path:.*}" } diff --git a/pkg/gofr/rbac/middleware.go b/pkg/gofr/rbac/middleware.go index fbda05b935..f4adb2eb7a 100644 --- a/pkg/gofr/rbac/middleware.go +++ b/pkg/gofr/rbac/middleware.go @@ -16,6 +16,7 @@ import ( "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 @@ -28,12 +29,12 @@ const unknownRouteLabel = "" // 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 PathPrefix("/") catch-all, which serves every request no route matched, is reported as -// unmatched. An application route registered at "/" shares its template, so the two are told apart -// by whether the matched route is a prefix route. +// 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 == "" || (tmpl == "/" && gofrHTTP.IsPrefixRoute(r)) { + if tmpl == "" || routemark.CatchAllAnswered(r) { return unknownRouteLabel } diff --git a/pkg/gofr/rbac/middleware_test.go b/pkg/gofr/rbac/middleware_test.go index b540e42e16..659fc93be3 100644 --- a/pkg/gofr/rbac/middleware_test.go +++ b/pkg/gofr/rbac/middleware_test.go @@ -5,6 +5,8 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "testing" "github.com/golang-jwt/jwt/v5" @@ -17,6 +19,8 @@ import ( "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) { @@ -1071,13 +1075,16 @@ func TestMiddleware_TraceRouteLabels(t *testing.T) { 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:.*}"}, - {"trie: route and rule differ", gofrHTTP.MatcherTrie, "/api/users/42", "/api/users/{id}", "/api/{path:.*}"}, - {"mux: no route and no rule", gofrHTTP.MatcherMux, "/nowhere", unknownRouteLabel, unknownRouteLabel}, - {"trie: no route and no rule", gofrHTTP.MatcherTrie, "/nowhere", unknownRouteLabel, unknownRouteLabel}, - {"mux: route at / is not the catch-all", gofrHTTP.MatcherMux, "/", "/", unknownRouteLabel}, - {"trie: route at / is not the catch-all", gofrHTTP.MatcherTrie, "/", "/", unknownRouteLabel}, + {"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 { @@ -1087,9 +1094,12 @@ func TestMiddleware_TraceRouteLabels(t *testing.T) { recorder := tracetest.NewSpanRecorder() provider := sdktrace.NewTracerProvider(sdktrace.WithSpanProcessor(recorder)) - config := newTestConfig(t, []EndpointMapping{ - {Path: "/api/{path:.*}", Methods: []string{http.MethodGet}, Public: true}, - }, nil) + 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) }) @@ -1098,10 +1108,22 @@ func TestMiddleware_TraceRouteLabels(t *testing.T) { router.Use(Middleware(config)) router.Add(http.MethodGet, "/", ok) router.Add(http.MethodGet, "/api/users/{id}", ok) - router.PathPrefix("/").Handler(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) - router.ServeHTTP(httptest.NewRecorder(), req) + 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) 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) + } +} From ad286b71883ee61d6ce937b7d2e8c52439cf311c Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Tue, 13 Oct 2026 18:23:11 +0530 Subject: [PATCH 10/11] feat(rbac): check every config EnableRBAC installed, not only the last Each EnableRBAC call installs its own middleware and every one enforces its rules, but the startup check read only the last config. It could report a route as unguarded while an earlier config returned 401 for it, and under GOFR_RBAC_ROUTE_CHECK=fail stop a correctly guarded app. A route is now reported only when no config covers it, and dead rules are reported per config. --- pkg/gofr/gofr.go | 11 ++-- pkg/gofr/internal/rbaccheck/check.go | 72 +++++++++++++++++++++++ pkg/gofr/internal/rbaccheck/check_test.go | 51 ++++++++++++++++ pkg/gofr/rbac.go | 9 ++- pkg/gofr/rbac_test.go | 41 +++++++++++-- pkg/gofr/run.go | 2 +- 6 files changed, 174 insertions(+), 12 deletions(-) diff --git a/pkg/gofr/gofr.go b/pkg/gofr/gofr.go index c750a2536f..f64cd6600e 100644 --- a/pkg/gofr/gofr.go +++ b/pkg/gofr/gofr.go @@ -82,12 +82,13 @@ type App struct { // same point it reports a port it could not claim. See bindMCPServer. mcpConfigErr error - // rbacConfig is the RBAC config installed by EnableRBAC, kept so Run can check it against the - // complete route table. See prepareHTTPServer. - rbacConfig *rbac.Config + // 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 rbacConfig is set. + // 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 reports without @@ -231,7 +232,7 @@ func (a *App) httpServerSetup() { } } - routes := newRBACRouteCollector(a.rbacConfig != nil) + 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) diff --git a/pkg/gofr/internal/rbaccheck/check.go b/pkg/gofr/internal/rbaccheck/check.go index 38fad3d7dc..5f416265e3 100644 --- a/pkg/gofr/internal/rbaccheck/check.go +++ b/pkg/gofr/internal/rbaccheck/check.go @@ -5,6 +5,7 @@ package rbaccheck import ( + "fmt" "sort" "strings" @@ -106,6 +107,77 @@ func Check(endpoints []rbac.EndpointMapping, routes []Route) Mismatches { 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. diff --git a/pkg/gofr/internal/rbaccheck/check_test.go b/pkg/gofr/internal/rbaccheck/check_test.go index 5ac2c97b0d..448ccdcab2 100644 --- a/pkg/gofr/internal/rbaccheck/check_test.go +++ b/pkg/gofr/internal/rbaccheck/check_test.go @@ -208,6 +208,57 @@ func TestCheck(t *testing.T) { } } +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 diff --git a/pkg/gofr/rbac.go b/pkg/gofr/rbac.go index 10543ffba6..aac9f8b028 100644 --- a/pkg/gofr/rbac.go +++ b/pkg/gofr/rbac.go @@ -75,7 +75,7 @@ func (a *App) enableRBAC(configPath ...string) error { middlewareFunc := rbac.Middleware(config) a.UseMiddleware(middlewareFunc) - a.rbacConfig = config + a.rbacConfigs = append(a.rbacConfigs, config) return nil } @@ -111,7 +111,12 @@ func (a *App) rbacRouteCheckMode() string { // 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 { - m := rbaccheck.Check(a.rbacConfig.Endpoints, a.rbacRoutes) + 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 { diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index f3d3225549..d7554d9eb0 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -1,6 +1,7 @@ package gofr import ( + "fmt" "net/http" "os" "path/filepath" @@ -194,6 +195,13 @@ const rbacPartlyCoveringItems = `{"roleHeader":"X-User-Role",` + `"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" @@ -201,6 +209,7 @@ const routeCheckFailed = "RBAC route check failed" 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 @@ -265,6 +274,22 @@ func prepareHTTPServerCases(t *testing.T) []prepareHTTPServerCase { 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, @@ -297,7 +322,7 @@ func TestApp_prepareHTTPServer(t *testing.T) { t.Setenv("GOFR_RBAC_ROUTE_CHECK", tc.mode) } - outcome, stdout, stderr := runPrepareHTTPServer(t, tc.config, tc.setup) + 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) @@ -319,10 +344,16 @@ func TestApp_prepareHTTPServer(t *testing.T) { // 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, config string, setup func(*App)) (outcome startupOutcome, stdout, stderr string) { +func runPrepareHTTPServer(t *testing.T, configs []string, setup func(*App)) (outcome startupOutcome, stdout, stderr string) { t.Helper() - path := writeRBACConfig(t, t.TempDir(), "rbac.json", config) + 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() { @@ -333,7 +364,9 @@ func runPrepareHTTPServer(t *testing.T, config string, setup func(*App)) (outcom setup(a) } - require.NoError(t, a.EnableRBAC(path)) + for _, path := range paths { + require.NoError(t, a.EnableRBAC(path)) + } outcome = a.prepareHTTPServer() }) diff --git a/pkg/gofr/run.go b/pkg/gofr/run.go index 9bdfa3899f..92f9fd5a5f 100644 --- a/pkg/gofr/run.go +++ b/pkg/gofr/run.go @@ -307,7 +307,7 @@ func (a *App) prepareHTTPServer() startupOutcome { a.httpServerSetup() - if a.rbacConfig == nil { + if len(a.rbacConfigs) == 0 { return startupOK } From ebfaaca5617c5b4ee34f337d03896cc472158239 Mon Sep 17 00:00:00 2001 From: Akshat Singhal Date: Wed, 14 Oct 2026 14:55:25 +0530 Subject: [PATCH 11/11] test(rbac): pin the framework catch-all mark; docs: root static dir and config numbers - TestApp_prepareHTTPServer: a covering config must not report /{path:.*}, which it does if the wiring in httpServerSetup stops marking GoFr's own catch-all. - docs: a static directory at / also matches /.well-known/alive and /.well-known/health, so probes get a 401; show the public rule that opens them. - docs: describe how the check reads more than one installed config and the "config N:" prefix on dead rules. --- docs/advanced-guide/rbac/page.md | 21 +++++++++++++++++++++ pkg/gofr/rbac_test.go | 5 +++++ 2 files changed, 26 insertions(+) diff --git a/docs/advanced-guide/rbac/page.md b/docs/advanced-guide/rbac/page.md index d68a826716..801994109f 100644 --- a/docs/advanced-guide/rbac/page.md +++ b/docs/advanced-guide/rbac/page.md @@ -363,6 +363,17 @@ A rule set that opens the static directory and a login route: {"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 @@ -383,6 +394,16 @@ 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: diff --git a/pkg/gofr/rbac_test.go b/pkg/gofr/rbac_test.go index d7554d9eb0..478ce20fa3 100644 --- a/pkg/gofr/rbac_test.go +++ b/pkg/gofr/rbac_test.go @@ -227,6 +227,11 @@ func prepareHTTPServerCases(t *testing.T) []prepareHTTPServerCase { 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,