Doormile/doormile_backend
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: access checks that returned utils.Forbidden never blocked anything

Browse Source 
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 <noreply@anthropic.com>
This commit is contained in:
Suriya
2026-08-06 12:32:13 +05:30
parent 42f2a41ff6
commit f44e8fa3b4
3 changed files with 199 additions and 84 deletions
Download Patch File Download Diff File Expand all files Collapse all files

120
controllers/adminController.go
View File

@@ -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

60
controllers/adminMilerOpsController.go
View File

@@ -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 {

103
controllers/tenantscope_test.go Normal file
View File

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