From 6966f14dd4aa8ed4162bd8d70f8a111f3068e099 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Sat, 5 Sep 2026 21:31:45 +0800 Subject: [PATCH] =?UTF-8?q?feat=E2=9C=A8(checksilent):=20report=20a=20rout?= =?UTF-8?q?e=20whose=20handler=20reads=20a=20data=20permission=20nobody=20?= =?UTF-8?q?supplies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetPermissionFromContext cannot fail. When no middleware put a *DataPermission in the context it returns the zero value, and the zero value's DataScope is the empty string - which is not one of the five scopes Permission recognises, so it takes the default branch and fails closed. The query is handed `1 = 0` and matches nothing. The endpoint then reports "not found" or "no permission" for rows that plainly exist, and only where enabledp is true. With data permissions off - the repository default - Permission returns the query untouched and the missing middleware costs nothing at all. A test suite and a CI that run on the default cannot see it. That is what happened to /api/v1/getinfo: it read the permission on a group carrying only the JWT middleware, so every login on a deployment with data permissions enabled ended in a 401 from the endpoint the browser calls immediately after signing in, and went back to the login page. Three /sys-api routes had the same shape. The check matches a handler to the group it is registered on, through the AST rather than through the text - a scratch grep for the same thing reported four false positives from a comment that happened to contain the function's name, and before that, a dozen from matching handler names across packages. Handlers are keyed by package, type and method, so two SysUser types are two handlers. Subgroups inherit their parent's chain, as gin does, and a `.Use` written below a registration still counts, because the chain is assembled before anything is served. Either half is a fix and the message says both, because which one is right depends on the route. A handler reading other people's rows wants the middleware. A handler reading the caller's own row - id from the token - wants no scope at all: DataScopeSelf matches on create_by, so scoping a self-read rejects every user who did not create their own account. Reporting only "add the middleware" would have turned /getinfo from broken into worse. Five tests: the mistake, both fixes, subgroup inheritance, and a same-named handler in another package. TestThisRepositoryIsClean covers the real tree, and it is what fails on the commit before this one - four findings, all real. --- tools/checksilent/checks.go | 2 + tools/checksilent/datascope.go | 386 ++++++++++++++++++++++++++++ tools/checksilent/datascope_test.go | 163 ++++++++++++ 3 files changed, 551 insertions(+) create mode 100644 tools/checksilent/datascope.go create mode 100644 tools/checksilent/datascope_test.go diff --git a/tools/checksilent/checks.go b/tools/checksilent/checks.go index 7cac2ee1..38d29588 100644 --- a/tools/checksilent/checks.go +++ b/tools/checksilent/checks.go @@ -20,6 +20,7 @@ const ( checkMenuIDConflict = "menu-id-collision" checkImportBoundary = "contract-import-boundary" checkShimAlias = "contract-shim-alias" + checkDataScopeRoute = "datascope-route-unguarded" ) // Package paths, relative to the module. Spelled once so a module rename @@ -47,6 +48,7 @@ func runChecks(s *snapshot, opt options) ([]Finding, error) { out = append(out, checkMenuIDCollisions(s)...) out = append(out, checkContractImportBoundary(s)...) out = append(out, checkContractShimAlias(s)...) + out = append(out, checkDataScopeRoutes(s)...) if opt.UIDir != "" { fs, err := checkMenuNames(s, opt.UIDir) diff --git a/tools/checksilent/datascope.go b/tools/checksilent/datascope.go new file mode 100644 index 00000000..0d35e0cc --- /dev/null +++ b/tools/checksilent/datascope.go @@ -0,0 +1,386 @@ +package main + +import ( + "go/ast" + "strings" +) + +// --------------------------------------------------------------------------- +// check 8: a handler that reads the data permission, on a route that never +// installs the middleware which puts one there +// --------------------------------------------------------------------------- + +// permissionGetter is the function a handler calls to obtain the caller's data +// scope, and permissionMiddleware is the middleware that puts one in the +// context. Matched by name rather than by resolved symbol: the tool parses +// without type checking, and both names are distinctive enough that a +// same-named function from somewhere else would still be worth a look. +const ( + permissionGetter = "GetPermissionFromContext" + permissionMiddleware = "PermissionAction" +) + +// actionsPkgSuffix identifies the package the two names above live in - this +// repository's common/actions shim and core's sdk/contract/actions both end +// this way, and a module rename changes neither. +const actionsPkgSuffix = "/actions" + +// handlerKey identifies one handler method uniquely across packages, so that +// two types named SysUser in different packages are not confused. +type handlerKey struct { + Pkg string + Type string + Func string +} + +// checkDataScopeRoutes reports a route whose handler asks for the caller's data +// permission while the group it is registered on never installs the middleware +// that supplies one. +// +// GetPermissionFromContext cannot fail. When nothing put a *DataPermission in +// the context it hands back a zero value, whose DataScope is the empty string - +// and the empty string is not one of the five scopes Permission recognises, so +// it takes the default branch. That branch fails closed: the query is given +// `1 = 0` and matches nothing. +// +// The result is an endpoint that answers "not found" or "no permission" for +// rows that plainly exist, and only on deployments that set enabledp: true - +// with data permissions off, Permission returns the query untouched and the +// missing middleware costs nothing. That is the shape this check exists for: a +// default configuration where the mistake is invisible, and a test suite that +// runs on it. +// +// It happened. /api/v1/getinfo read the permission on a group carrying only the +// JWT middleware, so every login on a deployment with data permissions enabled +// ended in a 401 from the endpoint the browser calls immediately after signing +// in - and went back to the login page. +// +// Either half is a fix, and which one depends on the route. A handler that +// reads somebody else's rows wants the middleware. A handler reading the +// caller's own row - where the id comes from the token - wants no scope at all, +// because a scope has nothing left to restrict there and DataScopeSelf, which +// matches on create_by, would reject every user who did not create their own +// account. The check reports the mismatch and leaves the choice. +func checkDataScopeRoutes(s *snapshot) []Finding { + handlers := permissionReadingHandlers(s) + if len(handlers) == 0 { + return nil + } + + var out []Finding + for _, sf := range s.Files { + if sf.isTest() { + continue + } + for _, decl := range sf.Syntax.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + out = append(out, s.routeFindings(sf, fn, handlers)...) + } + } + return out +} + +// permissionReadingHandlers collects every method whose body calls the getter. +// +// Test files are included deliberately: a handler is a handler wherever it is +// declared, and skipping them would let a route registered from a test fixture +// go unchecked while the fixture is exactly where a new one gets written first. +func permissionReadingHandlers(s *snapshot) map[handlerKey]bool { + out := map[handlerKey]bool{} + for _, sf := range s.Files { + for _, decl := range sf.Syntax.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil || fn.Recv == nil || len(fn.Recv.List) == 0 { + continue + } + recv := receiverTypeName(fn.Recv.List[0].Type) + if recv == "" { + continue + } + if callsPackageFunc(sf, fn.Body, permissionGetter) { + out[handlerKey{Pkg: sf.Pkg, Type: recv, Func: fn.Name.Name}] = true + } + } + } + return out +} + +// routeFindings walks one function looking for group definitions and the routes +// registered on them. +func (s *snapshot) routeFindings(sf *sourceFile, fn *ast.FuncDecl, handlers map[handlerKey]bool) []Finding { + // Local variable bindings, filled as the body is walked in order so that a + // registration only ever sees definitions that precede it. + apiVars := map[string]handlerKey{} // var -> the type it holds + guarded := map[string]bool{} // group var -> middleware installed + known := map[string]bool{} // group var -> is a router group at all + prefix := map[string]string{} // group var -> the path it was declared with + + var out []Finding + ast.Inspect(fn.Body, func(n ast.Node) bool { + switch stmt := n.(type) { + case *ast.AssignStmt: + for i, lhs := range stmt.Lhs { + id, ok := lhs.(*ast.Ident) + if !ok || i >= len(stmt.Rhs) { + continue + } + rhs := stmt.Rhs[i] + if key, ok := apiTypeOf(sf, rhs); ok { + apiVars[id.Name] = key + continue + } + if parent, isGroup := groupSource(rhs); isGroup { + known[id.Name] = true + prefix[id.Name] = prefix[parent] + groupPath(rhs) + // A subgroup inherits whatever its parent already had: + // gin copies the parent's handler chain into the child. + guarded[id.Name] = guarded[parent] || containsCallNamed(rhs, permissionMiddleware) + } + } + case *ast.ExprStmt: + // A separate `g.Use(...)` after the group was defined. + call, ok := stmt.X.(*ast.CallExpr) + if !ok { + return true + } + if target, ok := receiverIdentOf(call, "Use"); ok && known[target] { + if containsCallNamed(call, permissionMiddleware) { + guarded[target] = true + } + } + } + return true + }) + + // Second pass for the registrations, so that a `.Use` written below a route + // still counts - the middleware chain is assembled before any request is + // served, not in source order. + ast.Inspect(fn.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + gvar, method, ok := routeRegistration(call) + if !ok || !known[gvar] || guarded[gvar] { + return true + } + route, handlerVar, handlerName, ok := routeArgs(call) + if !ok { + return true + } + key, ok := apiVars[handlerVar] + if !ok { + return true + } + key.Func = handlerName + if !handlers[key] { + return true + } + out = append(out, s.finding(Error, checkDataScopeRoute, sf, call, + "%s %q is handled by %s.%s, which reads the caller's data permission,\n"+ + " but the group it is registered on never installs %s.\n"+ + " GetPermissionFromContext then returns the zero value, whose empty DataScope is not a\n"+ + " recognised scope, so Permission fails closed and the query matches nothing - on any\n"+ + " deployment with enabledp: true. With data permissions off the route works, which is\n"+ + " why this does not show up in the default configuration or in CI.\n"+ + " Add %s() to the group, or stop scoping a query that is already limited to the caller.", + method, prefix[gvar]+route, key.Type, handlerName, permissionMiddleware, permissionMiddleware)) + return true + }) + return out +} + +// receiverTypeName returns the bare type name of a method receiver, for both +// `(e SysUser)` and `(e *SysUser)`. +func receiverTypeName(expr ast.Expr) string { + if star, ok := expr.(*ast.StarExpr); ok { + expr = star.X + } + if id, ok := expr.(*ast.Ident); ok { + return id.Name + } + return "" +} + +// callsPackageFunc reports whether body calls name on a package whose import +// path ends in actionsPkgSuffix. +func callsPackageFunc(sf *sourceFile, body ast.Node, name string) bool { + found := false + ast.Inspect(body, func(n ast.Node) bool { + if found { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != name { + return true + } + pkg, ok := sel.X.(*ast.Ident) + if !ok { + return true + } + if path, ok := sf.imports[pkg.Name]; ok && strings.HasSuffix(path, actionsPkgSuffix) { + found = true + return false + } + return true + }) + return found +} + +// apiTypeOf recognises `apis.SysUser{}` and returns the package path and type. +func apiTypeOf(sf *sourceFile, expr ast.Expr) (handlerKey, bool) { + lit, ok := expr.(*ast.CompositeLit) + if !ok { + return handlerKey{}, false + } + sel, ok := lit.Type.(*ast.SelectorExpr) + if !ok { + return handlerKey{}, false + } + pkg, ok := sel.X.(*ast.Ident) + if !ok { + return handlerKey{}, false + } + path, ok := sf.imports[pkg.Name] + if !ok { + return handlerKey{}, false + } + return handlerKey{Pkg: path, Type: sel.Sel.Name}, true +} + +// groupSource reports whether expr builds a router group, and names the +// variable it was built from when there is one. +func groupSource(expr ast.Expr) (string, bool) { + parent := "" + isGroup := false + ast.Inspect(expr, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Group" { + return true + } + isGroup = true + if id, ok := sel.X.(*ast.Ident); ok { + parent = id.Name + } + return true + }) + return parent, isGroup +} + +// groupPath returns the literal path a group was declared with, or "" when it +// is not a literal - a computed prefix is left out of the message rather than +// printed as something it is not. +func groupPath(expr ast.Expr) string { + out := "" + ast.Inspect(expr, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "Group" || len(call.Args) == 0 { + return true + } + if lit, ok := call.Args[0].(*ast.BasicLit); ok { + out = strings.Trim(lit.Value, `"`) + } + return true + }) + return out +} + +// containsCallNamed reports whether expr contains a call to a function with +// this name, at any depth of a method chain or argument list. +func containsCallNamed(expr ast.Node, name string) bool { + found := false + ast.Inspect(expr, func(n ast.Node) bool { + if found { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + switch fun := call.Fun.(type) { + case *ast.SelectorExpr: + if fun.Sel.Name == name { + found = true + return false + } + case *ast.Ident: + if fun.Name == name { + found = true + return false + } + } + return true + }) + return found +} + +// receiverIdentOf returns the variable a `x.method(...)` call was made on. +func receiverIdentOf(call *ast.CallExpr, method string) (string, bool) { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != method { + return "", false + } + id, ok := sel.X.(*ast.Ident) + if !ok { + return "", false + } + return id.Name, true +} + +// httpMethods are the registration calls this check understands. Any and Match +// are absent on purpose: they take the method as data, and a check that half +// understands a registration is worse than one that says nothing about it. +var httpMethods = map[string]bool{ + "GET": true, "POST": true, "PUT": true, "DELETE": true, "PATCH": true, "HEAD": true, "OPTIONS": true, +} + +// routeRegistration recognises `g.GET(...)` and names the group and method. +func routeRegistration(call *ast.CallExpr) (string, string, bool) { + sel, ok := call.Fun.(*ast.SelectorExpr) + if !ok || !httpMethods[sel.Sel.Name] { + return "", "", false + } + id, ok := sel.X.(*ast.Ident) + if !ok { + return "", "", false + } + return id.Name, sel.Sel.Name, true +} + +// routeArgs pulls the path and the `api.Handler` argument out of a +// registration, ignoring any middleware written between them. +func routeArgs(call *ast.CallExpr) (route, handlerVar, handlerName string, ok bool) { + if len(call.Args) < 2 { + return "", "", "", false + } + lit, isLit := call.Args[0].(*ast.BasicLit) + if !isLit { + return "", "", "", false + } + route = strings.Trim(lit.Value, `"`) + // The handler is the last argument; anything before it is middleware. + sel, isSel := call.Args[len(call.Args)-1].(*ast.SelectorExpr) + if !isSel { + return "", "", "", false + } + id, isIdent := sel.X.(*ast.Ident) + if !isIdent { + return "", "", "", false + } + return route, id.Name, sel.Sel.Name, true +} diff --git a/tools/checksilent/datascope_test.go b/tools/checksilent/datascope_test.go new file mode 100644 index 00000000..ac0363c0 --- /dev/null +++ b/tools/checksilent/datascope_test.go @@ -0,0 +1,163 @@ +package main + +import ( + "strings" + "testing" +) + +// apisFile is a handler package with two methods: one that reads the caller's +// data permission and one that does not. +const apisFile = `package apis + +import ( + "github.com/gin-gonic/gin" + "go-admin/common/actions" +) + +type SysUser struct{} + +func (e SysUser) Scoped(c *gin.Context) { + p := actions.GetPermissionFromContext(c) + _ = p +} + +func (e SysUser) Unscoped(c *gin.Context) {} +` + +func routerFile(uses string) string { + return `package router + +import ( + "github.com/gin-gonic/gin" + "go-admin/app/admin/apis" + "go-admin/common/actions" +) + +var _ = actions.PermissionAction + +func register(v1 *gin.RouterGroup) { + api := apis.SysUser{} + r := v1.Group("/sys-user")` + uses + ` + { + r.GET("/:id", api.Scoped) + } +} +` +} + +// The mistake itself: a handler that reads the permission, on a group that +// never installs the middleware which puts one there. +func TestDataScopeRouteWithoutTheMiddlewareIsReported(t *testing.T) { + root := fixture(t, map[string]string{ + "app/admin/apis/sys_user.go": apisFile, + "app/admin/router/sys_user.go": routerFile(`.Use(gin.Logger())`), + }) + f := requireOne(t, check(t, root, options{}), checkDataScopeRoute) + for _, want := range []string{`GET "/sys-user/:id"`, "SysUser.Scoped", "PermissionAction"} { + if !strings.Contains(f.Message, want) { + t.Errorf("message does not mention %q:\n%s", want, f.Message) + } + } +} + +// The middleware installed in the chain is the fix, and must silence it. +func TestDataScopeRouteWithTheMiddlewareIsQuiet(t *testing.T) { + root := fixture(t, map[string]string{ + "app/admin/apis/sys_user.go": apisFile, + "app/admin/router/sys_user.go": routerFile(`.Use(gin.Logger()).Use(actions.PermissionAction())`), + }) + if got := only(t, check(t, root, options{}), checkDataScopeRoute); len(got) != 0 { + t.Errorf("reported %d findings for a guarded group:\n%v", len(got), got) + } +} + +// The other fix - the handler stops reading the permission - must silence it +// too. Reporting a route whose handler needs no scope would push people to +// install middleware they do not want, which is how /getinfo would have been +// "fixed" into rejecting every user who did not create their own account. +func TestARouteWhoseHandlerReadsNoPermissionIsQuiet(t *testing.T) { + root := fixture(t, map[string]string{ + "app/admin/apis/sys_user.go": apisFile, + "app/admin/router/sys_user.go": `package router + +import ( + "github.com/gin-gonic/gin" + "go-admin/app/admin/apis" +) + +func register(v1 *gin.RouterGroup) { + api := apis.SysUser{} + r := v1.Group("") + { + r.GET("/getinfo", api.Unscoped) + } +} +`, + }) + if got := only(t, check(t, root, options{}), checkDataScopeRoute); len(got) != 0 { + t.Errorf("reported %d findings for a handler that reads no permission:\n%v", len(got), got) + } +} + +// gin copies the parent's handler chain into a subgroup, so a group carved out +// of a guarded one is guarded. Reporting it would be a false positive, and a +// check that cries wolf is one people switch off. +func TestASubgroupInheritsTheMiddleware(t *testing.T) { + root := fixture(t, map[string]string{ + "app/admin/apis/sys_user.go": apisFile, + "app/admin/router/sys_user.go": `package router + +import ( + "github.com/gin-gonic/gin" + "go-admin/app/admin/apis" + "go-admin/common/actions" +) + +func register(v1 *gin.RouterGroup) { + api := apis.SysUser{} + parent := v1.Group("/sys").Use(actions.PermissionAction()) + child := parent.Group("/user") + { + child.GET("/:id", api.Scoped) + } +} +`, + }) + if got := only(t, check(t, root, options{}), checkDataScopeRoute); len(got) != 0 { + t.Errorf("reported %d findings for a subgroup of a guarded group:\n%v", len(got), got) + } +} + +// Two packages can both declare a SysUser. Only the one whose method reads the +// permission may be reported, or the check becomes a name search. +func TestAHandlerIsMatchedByPackageNotJustName(t *testing.T) { + root := fixture(t, map[string]string{ + "app/admin/apis/sys_user.go": apisFile, + "app/other/apis/sys_user.go": `package apis + +import "github.com/gin-gonic/gin" + +type SysUser struct{} + +func (e SysUser) Scoped(c *gin.Context) {} +`, + "app/other/router/sys_user.go": `package router + +import ( + "github.com/gin-gonic/gin" + "go-admin/app/other/apis" +) + +func register(v1 *gin.RouterGroup) { + api := apis.SysUser{} + r := v1.Group("/other") + { + r.GET("/:id", api.Scoped) + } +} +`, + }) + if got := only(t, check(t, root, options{}), checkDataScopeRoute); len(got) != 0 { + t.Errorf("reported %d findings for a same-named handler in another package:\n%v", len(got), got) + } +}