From f44e8fa3b43742b6a72c9b1966c968f7fcc4deea Mon Sep 17 00:00:00 2001 From: Suriya Date: Thu, 6 Aug 2026 12:32:13 +0530 Subject: [PATCH] fix: access checks that returned utils.Forbidden never blocked anything MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit utils.Forbidden and utils.NotFound write the response and return c.JSON's nil. Any helper that signalled refusal by returning one of them handed its caller a nil error, so every `if err != nil { return err }` guard passed and the handler carried straight on. The observable result: GET /admin/milers?tenantid=14 as a DailyGrubs login returned HTTP 403 with all 30 of the network's riders in the body. Status line correct, payload leaked. Three helpers were affected: effectiveTenantID (yesterday, mine) — cross-tenant read returned the unfiltered list under a 403 canAccessBooking (was assertBookingAccess, shipped in 6d9232f) — four mutating booking handlers were unguarded findMilerForConsole (was assertMilerAccess) — worse, callers went on to dereference the nil profile All three now return a bool and the caller writes the refusal itself, so the control flow is visible at the call site instead of hiding in a helper. Adds a test that pins utils.Forbidden/NotFound returning nil, so if that ever changes the assumption breaks loudly rather than silently, plus table tests for effectiveTenantID. Co-Authored-By: Claude Opus 5 --- controllers/adminController.go | 120 +++++++++++++------------ controllers/adminMilerOpsController.go | 60 +++++++------ controllers/tenantscope_test.go | 103 +++++++++++++++++++++ 3 files changed, 199 insertions(+), 84 deletions(-) create mode 100644 controllers/tenantscope_test.go diff --git a/controllers/adminController.go b/controllers/adminController.go index c39c183..a902c49 100644 --- a/controllers/adminController.go +++ b/controllers/adminController.go @@ -46,20 +46,25 @@ func isDoormileConsoleStaff(c *fiber.Ctx) bool { // one client's slice of the network. Returns 0 for "no restriction", which only // Doormile staff can reach. // -// A client passing ?tenantid= for someone else is refused rather than silently -// ignored — a filter that quietly returns the wrong tenant's data is worse than -// an error either way. -func effectiveTenantID(c *fiber.Ctx) (int, error) { +// The second return is "allowed", not an error, and deliberately so: the +// utils.* response helpers all return c.JSON's nil, so a helper that signalled +// refusal by returning utils.Forbidden(...) would hand its caller a nil error. +// Every `if err != nil` guard built that way silently passes and the handler +// carries on to write real data into a response already stamped 403. Callers +// must write the refusal themselves. +func effectiveTenantID(c *fiber.Ctx) (tenantID int, allowed bool) { own := consoleTenantID(c) requested := c.QueryInt("tenantid", 0) if own != 0 { + // A client asking for someone else's tenant is refused rather than + // silently given their own data back under the wrong label. if requested != 0 && requested != own { - return 0, utils.Forbidden(c, "you can only view your own tenant") + return 0, false } - return own, nil + return own, true } - return requested, nil + return requested, true } // scopeToTenant restricts a query to one tenant by its own tenant column, @@ -130,25 +135,26 @@ func canAccessTenant(c *fiber.Ctx, tenantID int) bool { return own == 0 || own == tenantID } -// assertBookingAccess checks that the caller may act on a booking addressed by -// id. Returns nil for Doormile staff. Mutating handlers take the booking id +// canAccessBooking reports whether the caller may act on a booking addressed by +// id. Always true for Doormile staff. Mutating handlers take the booking id // straight from the path, so a scoped SELECT elsewhere in the handler does not // protect them — this has to run before the write. -func assertBookingAccess(c *fiber.Ctx, bookingID int) error { +// +// Returns a bool rather than an error for the reason spelled out on +// effectiveTenantID: utils.NotFound returns nil, so the previous +// error-returning version never actually blocked anything. +func canAccessBooking(c *fiber.Ctx, bookingID int) bool { own := consoleTenantID(c) if own == 0 { - return nil + return true } var booking models.PickupBooking if err := db.DB.Select("bookingid", "tenantid").First(&booking, bookingID).Error; err != nil { - return utils.NotFound(c, "booking not found") + return false } // A booking with no tenant predates tenant attribution and can't be proven // to belong to this client, so it stays invisible to them. - if booking.Tenantid == nil || *booking.Tenantid != own { - return utils.NotFound(c, "booking not found") - } - return nil + return booking.Tenantid != nil && *booking.Tenantid == own } // Helper to generate tripsheet number @@ -229,9 +235,9 @@ func LoginAdmin(cfg *config.Config) fiber.Handler { func GetAdminDashboard(c *fiber.Ctx) error { // Pinned to the caller's own tenant for a client login; Doormile staff pass // ?tenantid= to see one client's numbers, or omit it for the whole network. - tenantID, terr := effectiveTenantID(c) - if terr != nil { - return terr + tenantID, allowed := effectiveTenantID(c) + if !allowed { + return utils.Forbidden(c, "you can only view your own tenant") } var totalTenants int64 @@ -299,9 +305,9 @@ func GetAdminReports(c *fiber.Ctx) error { // ownTenant is 0 for a Doormile-staff whole-network view, the client's own // tenant for a client login, or the tenant Doormile staff asked for with // ?tenantid=. Every figure below is restricted to it. - ownTenant, terr := effectiveTenantID(c) - if terr != nil { - return terr + ownTenant, allowed := effectiveTenantID(c) + if !allowed { + return utils.Forbidden(c, "you can only view your own tenant") } var totalBookings int64 @@ -862,9 +868,9 @@ func GetAdminCustomers(c *fiber.Ctx) error { // A client sees only the customers they have actually delivered to, not // Doormile's whole B2C address book. Doormile staff can ask for one // client's customers with ?tenantid=. - tenantID, terr := effectiveTenantID(c) - if terr != nil { - return terr + tenantID, allowed := effectiveTenantID(c) + if !allowed { + return utils.Forbidden(c, "you can only view your own tenant") } if tenantID != 0 { query = query.Where("appcustomerid IN (?)", @@ -1406,9 +1412,9 @@ func DeleteVehicle(c *fiber.Ctx) error { // -------------------- func GetMilers(c *fiber.Ctx) error { - tenantID, terr := effectiveTenantID(c) - if terr != nil { - return terr + tenantID, allowed := effectiveTenantID(c) + if !allowed { + return utils.Forbidden(c, "you can only view your own tenant") } var profiles []models.MilerProfile @@ -1511,9 +1517,9 @@ func GetMilerDetails(c *fiber.Ctx) error { // GetMilers scopes the roster to the caller's own fleet, but reading one // rider by id did not — a client login could walk the whole network's riders // by incrementing the id. - profile, err := assertMilerAccess(c, id) - if err != nil { - return err + profile, ok := findMilerForConsole(c, id) + if !ok { + return utils.NotFound(c, "miler not found") } return utils.OK(c, profile) } @@ -1540,9 +1546,9 @@ func AdminNotifyMiler(c *fiber.Ctx) error { return utils.BadRequest(c, "title and message are required") } - profile, aerr := assertMilerAccess(c, id) - if aerr != nil { - return aerr + profile, ok := findMilerForConsole(c, id) + if !ok { + return utils.NotFound(c, "miler not found") } if profile.Devicetoken == "" { @@ -1558,9 +1564,9 @@ func AdminNotifyMiler(c *fiber.Ctx) error { func UpdateMiler(c *fiber.Ctx) error { id, _ := strconv.Atoi(c.Params("id")) - profile, aerr := assertMilerAccess(c, id) - if aerr != nil { - return aerr + profile, ok := findMilerForConsole(c, id) + if !ok { + return utils.NotFound(c, "miler not found") } type MilerUpdate struct { @@ -1593,9 +1599,9 @@ func UpdateMiler(c *fiber.Ctx) error { func BlockMiler(c *fiber.Ctx) error { id, _ := strconv.Atoi(c.Params("id")) - profile, aerr := assertMilerAccess(c, id) - if aerr != nil { - return aerr + profile, ok := findMilerForConsole(c, id) + if !ok { + return utils.NotFound(c, "miler not found") } tx := db.DB.Begin() @@ -1630,9 +1636,9 @@ func AssignMilerVehicle(c *fiber.Ctx) error { return utils.BadRequest(c, "invalid request body") } - profile, aerr := assertMilerAccess(c, id) - if aerr != nil { - return aerr + profile, ok := findMilerForConsole(c, id) + if !ok { + return utils.NotFound(c, "miler not found") } // A vehicle can only be handed to a rider the caller owns, and only from @@ -1661,9 +1667,9 @@ func GetAdminBookings(c *fiber.Ctx) error { pagesize := min(100, max(1, c.QueryInt("pagesize", 20))) offset := (pageno - 1) * pagesize - tenantID, terr := effectiveTenantID(c) - if terr != nil { - return terr + tenantID, allowed := effectiveTenantID(c) + if !allowed { + return utils.Forbidden(c, "you can only view your own tenant") } query := scopeToTenant(db.DB.Model(&models.PickupBooking{}), "tenantid", tenantID) if status := c.Query("status"); status != "" { @@ -2058,8 +2064,8 @@ func GetAdminBookingDetails(c *fiber.Ctx) error { func AdminAssignMiler(c *fiber.Ctx) error { id, _ := strconv.Atoi(c.Params("id")) - if err := assertBookingAccess(c, id); err != nil { - return err + if !canAccessBooking(c, id) { + return utils.NotFound(c, "booking not found") } type MilerAssign struct { @@ -2082,8 +2088,8 @@ func AdminAssignMiler(c *fiber.Ctx) error { func AdminAssignVehicle(c *fiber.Ctx) error { id, _ := strconv.Atoi(c.Params("id")) - if err := assertBookingAccess(c, id); err != nil { - return err + if !canAccessBooking(c, id) { + return utils.NotFound(c, "booking not found") } type VehicleAssign struct { @@ -2111,8 +2117,8 @@ func AdminAssignVehicle(c *fiber.Ctx) error { func AdminUpdateBookingStatus(c *fiber.Ctx) error { id, _ := strconv.Atoi(c.Params("id")) - if err := assertBookingAccess(c, id); err != nil { - return err + if !canAccessBooking(c, id) { + return utils.NotFound(c, "booking not found") } type StatusUpdate struct { @@ -2148,8 +2154,8 @@ func AdminCancelBooking(c *fiber.Ctx) error { if err != nil { return utils.BadRequest(c, "invalid booking ID") } - if err := assertBookingAccess(c, id); err != nil { - return err + if !canAccessBooking(c, id) { + return utils.NotFound(c, "booking not found") } var booking models.PickupBooking @@ -2281,9 +2287,9 @@ func AdminBulkCancelBookings(c *fiber.Ctx) error { func GetAdminConsignments(c *fiber.Ctx) error { page := utils.ParsePage(c) - tenantID, terr := effectiveTenantID(c) - if terr != nil { - return terr + tenantID, allowed := effectiveTenantID(c) + if !allowed { + return utils.Forbidden(c, "you can only view your own tenant") } var total int64 diff --git a/controllers/adminMilerOpsController.go b/controllers/adminMilerOpsController.go index dcc8d66..9b30bf3 100644 --- a/controllers/adminMilerOpsController.go +++ b/controllers/adminMilerOpsController.go @@ -26,45 +26,51 @@ import ( "github.com/redis/go-redis/v9" ) -// assertMilerAccess resolves a miler profile by its profile id and proves the -// caller is allowed to see that rider. A rider belongs to a client through the -// tenantid on their appusers row — the same link GetMilers filters on. Doormile -// staff (tenant 0) skip the check. -func assertMilerAccess(c *fiber.Ctx, milerProfileID int) (*models.MilerProfile, error) { +// findMilerForConsole resolves a miler profile by its profile id, but only if +// the caller is allowed to see that rider. A rider belongs to a client through +// the tenantid on their appusers row — the same link GetMilers filters on. +// Doormile staff (tenant 0) skip the check. +// +// The second return is "found", not an error, for the reason on +// effectiveTenantID: a version that returned utils.NotFound(...) as its error +// hands the caller a nil, so the guard passes and the handler goes on to +// dereference a nil profile. +// +// Callers report a miss as "not found" rather than "forbidden" — a client +// should not be able to probe which rider ids exist outside their own fleet. +func findMilerForConsole(c *fiber.Ctx, milerProfileID int) (*models.MilerProfile, bool) { var profile models.MilerProfile if err := db.DB.Where("milerprofileid = ?", milerProfileID).First(&profile).Error; err != nil { - return nil, utils.NotFound(c, "miler not found") + return nil, false } own := consoleTenantID(c) if own == 0 { - return &profile, nil + return &profile, true } var user models.AppUser if err := db.DB.Select("userid", "tenantid").Where("userid = ?", profile.Userid).First(&user).Error; err != nil { - return nil, utils.NotFound(c, "miler not found") + return nil, false } - // Deliberately "not found" rather than "forbidden": a client should not be - // able to probe which rider ids exist outside their own fleet. if user.Tenantid != own { - return nil, utils.NotFound(c, "miler not found") + return nil, false } - return &profile, nil + return &profile, true } // visibleMilerUserIDs lists the appusers.userid of every rider the request // should cover — the caller's own fleet for a client login, or the tenant // Doormile staff asked for with ?tenantid=. Returns nil for "no restriction". -func visibleMilerUserIDs(c *fiber.Ctx) ([]int, error) { - tenantID, err := effectiveTenantID(c) - if err != nil { - return nil, err +func visibleMilerUserIDs(c *fiber.Ctx) (ids []int, allowed bool) { + tenantID, allowed := effectiveTenantID(c) + if !allowed { + return nil, false } if tenantID == 0 { - return nil, nil + return nil, true } - return milerUserIDsForTenant(tenantID), nil + return milerUserIDsForTenant(tenantID), true } // milerSummaryRow is one line of the roster table — the shape the console's @@ -115,9 +121,9 @@ func GetMilerSummary(c *fiber.Ctx) error { if hubID := c.Query("hubid"); hubID != "" { query = query.Where("hubid = ?", hubID) } - visible, verr := visibleMilerUserIDs(c) - if verr != nil { - return verr + visible, allowed := visibleMilerUserIDs(c) + if !allowed { + return utils.Forbidden(c, "you can only view your own tenant") } if visible != nil { if len(visible) == 0 { @@ -266,9 +272,9 @@ func GetMilerLogs(c *fiber.Ctx) error { if err != nil { return utils.BadRequest(c, "invalid miler ID") } - profile, aerr := assertMilerAccess(c, id) - if aerr != nil { - return aerr + profile, ok := findMilerForConsole(c, id) + if !ok { + return utils.NotFound(c, "miler not found") } if db.Rdb == nil { return utils.Internal(c, "cache service unavailable") @@ -376,9 +382,9 @@ func GetMilerActivity(c *fiber.Ctx) error { if err != nil { return utils.BadRequest(c, "invalid miler ID") } - profile, aerr := assertMilerAccess(c, id) - if aerr != nil { - return aerr + profile, ok := findMilerForConsole(c, id) + if !ok { + return utils.NotFound(c, "miler not found") } from, to, err := parseHubDateRange(c) if err != nil { diff --git a/controllers/tenantscope_test.go b/controllers/tenantscope_test.go new file mode 100644 index 0000000..f402453 --- /dev/null +++ b/controllers/tenantscope_test.go @@ -0,0 +1,103 @@ +package controllers + +import ( + "testing" + + "doormile/utils" + + "github.com/gofiber/fiber/v2" + "github.com/valyala/fasthttp" +) + +// newCtx builds a throwaway request context with the given console identity. +func newCtx(t *testing.T, tenantID int, query string) (*fiber.Ctx, func()) { + t.Helper() + app := fiber.New() + fctx := &fasthttp.RequestCtx{} + fctx.Request.SetRequestURI("/admin/milers?" + query) + c := app.AcquireCtx(fctx) + c.Locals("tenantid", tenantID) + return c, func() { app.ReleaseCtx(c) } +} + +// TestResponseHelpersReturnNil pins the trap that broke both console access +// checks: utils.Forbidden and utils.NotFound write the response and return +// c.JSON's nil. A helper that signals refusal by returning one of them hands +// its caller a nil error, every `if err != nil` guard passes, and the handler +// carries on to write real data into a response already stamped 403 or 404. +// +// If this test ever fails because the helpers started returning a real error, +// the bool-returning access checks can go back to returning errors. +func TestResponseHelpersReturnNil(t *testing.T) { + app := fiber.New() + c := app.AcquireCtx(&fasthttp.RequestCtx{}) + defer app.ReleaseCtx(c) + + if err := utils.Forbidden(c, "denied"); err != nil { + t.Errorf("utils.Forbidden returned %v; the access checks assume nil — see effectiveTenantID", err) + } + if err := utils.NotFound(c, "missing"); err != nil { + t.Errorf("utils.NotFound returned %v; the access checks assume nil — see findMilerForConsole", err) + } +} + +func TestEffectiveTenantID(t *testing.T) { + cases := []struct { + name string + own int + query string + wantTenant int + wantAllowed bool + }{ + { + name: "doormile staff with no filter see the whole network", + own: 0, + query: "", + wantTenant: 0, + wantAllowed: true, + }, + { + name: "doormile staff can ask for one client's slice", + own: 0, + query: "tenantid=13", + wantTenant: 13, + wantAllowed: true, + }, + { + name: "a client login is pinned to its own tenant", + own: 13, + query: "", + wantTenant: 13, + wantAllowed: true, + }, + { + name: "a client asking for its own tenant is fine", + own: 13, + query: "tenantid=13", + wantTenant: 13, + wantAllowed: true, + }, + { + name: "a client asking for another tenant is refused", + own: 13, + query: "tenantid=14", + wantTenant: 0, + wantAllowed: false, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c, release := newCtx(t, tc.own, tc.query) + defer release() + + gotTenant, gotAllowed := effectiveTenantID(c) + if gotAllowed != tc.wantAllowed { + t.Errorf("allowed = %v, want %v", gotAllowed, tc.wantAllowed) + } + if gotAllowed && gotTenant != tc.wantTenant { + t.Errorf("tenant = %d, want %d", gotTenant, tc.wantTenant) + } + }) + } +}