From 4156387eb9e35f3b3a683385cc85f9eb648da6af Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Fri, 4 Sep 2026 17:23:01 +0800 Subject: [PATCH 1/4] =?UTF-8?q?fix=F0=9F=90=9B:=20enforce=20Casbin=20when?= =?UTF-8?q?=20editing=20another=20user?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PUT /api/v1/sys-user sits in CasbinExclude so the profile page can reach it, which means AuthCheckRole never runs for this route. The handler took the target user id from the request body, so any authenticated caller could edit another user's record - including their roleId. The route has to stay excluded: the profile page and the admin user list share this one endpoint, so removing the exclusion would break self-service editing for every non-admin role. The check therefore moves into the handler: when the target is not the caller, the request is put through Casbin explicitly. EnforceRoleFor carries the same admin short-circuit and enforcement AuthCheckRole uses, so a route that opts out of the middleware can still ask the same question. Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx --- app/admin/apis/sys_user.go | 28 ++++++++++++++++++++++++++-- common/middleware/permission.go | 27 +++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/app/admin/apis/sys_user.go b/app/admin/apis/sys_user.go index 70d20688..07b8c6dd 100644 --- a/app/admin/apis/sys_user.go +++ b/app/admin/apis/sys_user.go @@ -1,6 +1,7 @@ package apis import ( + "errors" "github.com/gin-gonic/gin/binding" "go-admin/app/admin/models" "golang.org/x/crypto/bcrypt" @@ -15,6 +16,7 @@ import ( "go-admin/app/admin/service" "go-admin/app/admin/service/dto" "go-admin/common/actions" + "go-admin/common/middleware" ) type SysUser struct { @@ -149,12 +151,34 @@ func (e SysUser) Update(c *gin.Context) { return } - req.SetUpdateBy(user.GetUserId(c)) + callerId := user.GetUserId(c) + + // This route is in CasbinExclude so the personal-center screen can edit + // the caller's own record without a policy grant (see settings.go). That + // exclusion covers the whole route, not just the caller's own record, and + // the request carries the target userId in the body - so without this + // check here, any authenticated caller could edit any other user, up to + // and including their roleId. When the target is someone else, ask Casbin + // directly for the permission AuthCheckRole skipped. + if req.UserId != callerId { + allowed, err := middleware.EnforceRoleFor(c, c.Request.URL.Path, c.Request.Method) + if err != nil { + e.Logger.Error(err) + e.Error(500, err, err.Error()) + return + } + if !allowed { + e.Error(http.StatusForbidden, errors.New("无权更新其他用户数据"), "对不起，您没有该接口访问权限，请联系管理员") + return + } + } + + req.SetUpdateBy(callerId) //数据权限检查 p := actions.GetPermissionFromContext(c) - err = s.Update(&req, p) + err = s.Update(&req, p, callerId) if err != nil { e.Logger.Error(err) return diff --git a/common/middleware/permission.go b/common/middleware/permission.go index 993fde77..99a9aa5e 100644 --- a/common/middleware/permission.go +++ b/common/middleware/permission.go @@ -59,6 +59,33 @@ func AuthCheckRole() gin.HandlerFunc { } } +// EnforceRoleFor reports whether the caller's role has explicit Casbin +// permission to act on path with method. +// +// AuthCheckRole never calls Enforce for a route CasbinExclude lists - that +// is the whole point of the list. A handler on such a route can still need +// the real answer for part of what it does: sys_user.go's Update shares its +// excluded route between the personal-center screen editing the caller's own +// record (which is why the route is excluded at all) and an admin editing +// someone else's, and only the second case is meant to require a policy +// grant. That handler asks here instead of assuming the middleware already +// checked. +func EnforceRoleFor(c *gin.Context, path, method string) (bool, error) { + data, ok := c.Get(jwtauth.JwtPayloadKey) + if !ok { + return false, nil + } + v, ok := data.(jwtauth.MapClaims) + if !ok { + return false, nil + } + if v["rolekey"] == "admin" { + return true, nil + } + e := sdk.Runtime.GetCasbinByTenant(c.Request.Host) + return e.Enforce(v["rolekey"], path, method) +} + // excludedFromCasbin reports whether the route skips the permission check. // // It runs for every non-admin request, so the order matters: the method rules From 07ff92aa55a2e4420a8e017eef5b72b1654b29ac Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Fri, 4 Sep 2026 17:23:07 +0800 Subject: [PATCH 2/4] =?UTF-8?q?fix=F0=9F=90=9B:=20lock=20privileged=20fiel?= =?UTF-8?q?ds=20on=20self-edit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The profile page posts the whole user object back, including roleId, deptId and status, because it renders from a full SysUser it fetched earlier. A caller editing their own record can therefore hand back a tampered roleId. Self-edits now reload those three fields from the database and ignore whatever the request carried. For an honest client this is a no-op - the values it sends are already its own - so the profile page keeps working unchanged. Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx --- app/admin/service/sys_user.go | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/admin/service/sys_user.go b/app/admin/service/sys_user.go index 8361e5d4..839a9b3b 100644 --- a/app/admin/service/sys_user.go +++ b/app/admin/service/sys_user.go @@ -84,7 +84,16 @@ func (e *SysUser) Insert(c *dto.SysUserInsertReq) error { } // Update 修改SysUser对象 -func (e *SysUser) Update(c *dto.SysUserUpdateReq, p *actions.DataPermission) error { +// +// callerId is who is asking, not who SetUpdateBy recorded - that field only +// says who to blame, it never constrained who could be edited. When the +// target is the caller themselves, roleId/deptId/status are kept at whatever +// the database already has no matter what the request body carries: this is +// the personal-center screen's route (see CasbinExclude in settings.go, and +// the check in the API handler ahead of this call), and letting a caller +// grant themselves a different role or department through it would be a +// privilege escalation the exclusion was never meant to open. +func (e *SysUser) Update(c *dto.SysUserUpdateReq, p *actions.DataPermission, callerId int) error { var err error var model models.SysUser db := e.Orm.Scopes( @@ -98,6 +107,11 @@ func (e *SysUser) Update(c *dto.SysUserUpdateReq, p *actions.DataPermission) err return errors.New("无权更新该数据") } + if model.UserId == callerId { + c.RoleId = model.RoleId + c.DeptId = model.DeptId + c.Status = model.Status + } c.Generate(&model) update := e.Orm.Model(&model).Where("user_id = ?", &model.UserId).Omit("password", "salt").Updates(&model) if err = update.Error; err != nil { From 4d6456a588a32a6205de4e47652d771a2fd1c470 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Fri, 4 Sep 2026 17:23:07 +0800 Subject: [PATCH 3/4] =?UTF-8?q?test=E2=9C=85:=20cover=20vertical=20privile?= =?UTF-8?q?ge=20escalation=20on=20sys-user=20update?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two directions, because the fix has to hold both: an attacker with no policy on this route cannot raise another user's role, and a self-edit cannot raise its own. The second one is what keeps the fix from being "just remove the route from CasbinExclude", which would break the profile page. The tests drive the handler directly rather than through the router, because the middleware is exactly what does not run for this route - the defence lives in the handler, so that is where it has to be proven. Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx --- app/admin/apis/sys_user_privesc_test.go | 170 ++++++++++++++++++++++++ 1 file changed, 170 insertions(+) create mode 100644 app/admin/apis/sys_user_privesc_test.go diff --git a/app/admin/apis/sys_user_privesc_test.go b/app/admin/apis/sys_user_privesc_test.go new file mode 100644 index 00000000..3ff837d6 --- /dev/null +++ b/app/admin/apis/sys_user_privesc_test.go @@ -0,0 +1,170 @@ +package apis + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/gin-gonic/gin" + "github.com/glebarez/sqlite" + mycasbin "github.com/go-admin-team/go-admin-core/v2/casbin" + jwt "github.com/go-admin-team/go-admin-core/v2/jwtauth" + "github.com/go-admin-team/go-admin-core/v2/logger" + "github.com/go-admin-team/go-admin-core/v2/sdk" + "github.com/go-admin-team/go-admin-core/v2/sdk/pkg" + "gorm.io/gorm" + + "go-admin/app/admin/models" +) + +// PUT /api/v1/sys-user is in settings.go's CasbinExclude so the +// personal-center screen (go-admin-ui's userInfo.vue) can edit the caller's +// own record without holding a policy grant on this route. AuthCheckRole +// skips Enforce entirely for an excluded route, so this file's job is to pin +// what the handler itself now has to hold shut: the target userId comes from +// the request body, and nothing upstream of the handler ever checked it +// against the caller. + +// setupPrivescDB wires an in-memory database and a Casbin enforcer with an +// empty policy - the state of a fresh install for any role but admin - under +// a tenant unique to the calling test, so mycasbin's process-wide enforcer +// cache can't hand one test's database to another. +func setupPrivescDB(t *testing.T) (*gorm.DB, string) { + t.Helper() + + db, err := gorm.Open(sqlite.Open("file::memory:"), &gorm.Config{}) + if err != nil { + t.Skipf("sqlite unavailable: %v", err) + } + if err := db.AutoMigrate(&models.SysUser{}); err != nil { + t.Skipf("automigrate: %v", err) + } + + tenant := "sys-user-privesc-" + t.Name() + + previousInterval := mycasbin.ReloadInterval + mycasbin.ReloadInterval = 0 // opt out of the background reload goroutine; the test never writes a policy + t.Cleanup(func() { mycasbin.ReloadInterval = previousInterval }) + + e := mycasbin.Setup(db, tenant) + previousEnforcer := sdk.Runtime.GetCasbinByTenant(tenant) + sdk.Runtime.SetCasbinByTenant(tenant, e) + t.Cleanup(func() { sdk.Runtime.SetCasbinByTenant(tenant, previousEnforcer) }) + + return db, tenant +} + +// callUpdate drives SysUser.Update the way the router does for an +// authenticated, non-admin caller: JWT claims already decoded into the +// context (that is jwtauth's job, not this handler's) and a database - but +// without AuthCheckRole, since that middleware never runs Enforce for this +// route at all. +func callUpdate(t *testing.T, db *gorm.DB, tenant string, callerId int, body map[string]interface{}) *httptest.ResponseRecorder { + t.Helper() + gin.SetMode(gin.TestMode) + + raw, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal request body: %v", err) + } + + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Request = httptest.NewRequest(http.MethodPut, "/api/v1/sys-user", bytes.NewReader(raw)) + c.Request.Host = tenant + c.Request.Header.Set("Content-Type", "application/json") + + c.Set("db", db) + c.Set(pkg.LoggerKey, logger.NewHelper(logger.DefaultLogger)) + c.Set(jwt.JwtPayloadKey, jwt.MapClaims{ + "identity": float64(callerId), + "rolekey": "ordinary-role", // holds no Casbin policy anywhere in this test + }) + + SysUser{}.Update(c) + return w +} + +// TestUpdate_CannotEscalatePrivilegeThroughAnotherUsersRecord is the +// regression for H6. Before the fix, an ordinary authenticated user could PUT +// a body naming another user's id and change that user's roleId - the route +// being Casbin-excluded meant no permission check ever ran, and the data +// permission scope that would otherwise gate this is off by default. +func TestUpdate_CannotEscalatePrivilegeThroughAnotherUsersRecord(t *testing.T) { + db, tenant := setupPrivescDB(t) + + victim := models.SysUser{Username: "bob", NickName: "Bob", RoleId: 2, DeptId: 1, Status: "1"} + if err := db.Create(&victim).Error; err != nil { + t.Fatal(err) + } + attacker := models.SysUser{Username: "alice", NickName: "Alice", RoleId: 2, DeptId: 1, Status: "1"} + if err := db.Create(&attacker).Error; err != nil { + t.Fatal(err) + } + + const elevatedRoleId = 1 // a role the attacker does not hold and has no policy for + + callUpdate(t, db, tenant, attacker.UserId, map[string]interface{}{ + "userId": victim.UserId, + "username": victim.Username, + "nickName": "pwned", + "phone": "13800000000", + "email": "bob@example.com", + "roleId": elevatedRoleId, + "deptId": victim.DeptId, + "status": victim.Status, + }) + + var after models.SysUser + if err := db.First(&after, victim.UserId).Error; err != nil { + t.Fatal(err) + } + if after.RoleId == elevatedRoleId { + t.Fatalf("an attacker with no Casbin permission on this route escalated the victim's roleId to %d", after.RoleId) + } + if after.NickName == "pwned" { + t.Fatalf("an attacker with no Casbin permission on this route modified another user's record: %+v", after) + } +} + +// TestUpdate_SelfEditCannotChangePrivilegedFields covers the case the +// CasbinExclude entry exists for: the personal-center screen has to keep +// working for the caller's own record. The fields that screen exposes +// (nickName/phone/email/sex) must still save, while roleId/deptId/status stay +// whatever the database already had even if the request carries something +// else - a compromised or hand-crafted client is the only way that request +// would ever differ from what the honest form sends. +func TestUpdate_SelfEditCannotChangePrivilegedFields(t *testing.T) { + db, tenant := setupPrivescDB(t) + + self := models.SysUser{Username: "carol", NickName: "Carol", RoleId: 2, DeptId: 1, Status: "1"} + if err := db.Create(&self).Error; err != nil { + t.Fatal(err) + } + + const elevatedRoleId = 1 + + callUpdate(t, db, tenant, self.UserId, map[string]interface{}{ + "userId": self.UserId, + "username": self.Username, + "nickName": "Carol Updated", + "phone": "13900000000", + "email": "carol@example.com", + "roleId": elevatedRoleId, // tampered; must not take effect + "deptId": self.DeptId, + "status": self.Status, + }) + + var after models.SysUser + if err := db.First(&after, self.UserId).Error; err != nil { + t.Fatal(err) + } + if after.RoleId == elevatedRoleId { + t.Fatalf("a self-edit changed the caller's own roleId to %d", after.RoleId) + } + if after.NickName != "Carol Updated" { + t.Fatalf("the legitimate personal-center edit did not go through: %+v", after) + } +} From f406ca0160b4993c7eaf83abe4997178a8da3028 Mon Sep 17 00:00:00 2001 From: zhangwenjian Date: Fri, 4 Sep 2026 19:09:41 +0800 Subject: [PATCH 4/4] =?UTF-8?q?test=E2=9C=85:=20fail=20loudly=20instead=20?= =?UTF-8?q?of=20skipping=20when=20the=20sqlite=20setup=20breaks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The privilege-escalation tests skipped themselves when opening the in-memory database or running AutoMigrate failed. Both depend on nothing outside the process, so a failure there means the environment is genuinely broken - and a security regression that quietly does not run is worse than one that is missing, because CI stays green either way. Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx --- app/admin/apis/sys_user_privesc_test.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/admin/apis/sys_user_privesc_test.go b/app/admin/apis/sys_user_privesc_test.go index 3ff837d6..b2e7eb45 100644 --- a/app/admin/apis/sys_user_privesc_test.go +++ b/app/admin/apis/sys_user_privesc_test.go @@ -34,12 +34,17 @@ import ( func setupPrivescDB(t *testing.T) (*gorm.DB, string) { t.Helper() + // Fatalf, not Skipf: this database is in-memory sqlite with no external + // dependency, so failing to open or migrate it means the environment is + // actually broken. Skipping here would let these two anti-privesc + // regression tests silently stop running while CI stays green - a + // standing assertion that never fires is worse than no assertion. db, err := gorm.Open(sqlite.Open("file::memory:"), &gorm.Config{}) if err != nil { - t.Skipf("sqlite unavailable: %v", err) + t.Fatalf("sqlite unavailable: %v", err) } if err := db.AutoMigrate(&models.SysUser{}); err != nil { - t.Skipf("automigrate: %v", err) + t.Fatalf("automigrate: %v", err) } tenant := "sys-user-privesc-" + t.Name()