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) + } +}