f44e8fa3b4
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>
104 lines
2.8 KiB
Go
104 lines
2.8 KiB
Go
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)
|
|
}
|
|
})
|
|
}
|
|
}
|