Loyaly/Behavision
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 22 Wiki Activity

Five live endpoints answered 500 to a platform admin

Browse Source 
/api/visits, /api/cameras, /api/sites, /api/visitors and
/api/reports/footfall, all in production, all before today's work. A
platform admin is defined by having NO client, and every tenant query
scopes on client_id = $1::uuid - so the empty string reaches Postgres as
''::uuid, which is a cast ERROR rather than an empty result. Found by
calling them while verifying the new routes, which have the same shape
and were failing the same way.

tenantOnly is the guard, beside adminOnly and for the opposite audience.
Per-query casts would have been the wrong fix twice over: it is a fix the
next query forgets, and the next query would then 500 in production
exactly as these did.

403, not adminOnly's 404, because the two hide opposite things. A tenant
must not learn a platform surface exists. A platform admin already knows
the tenant surface does - they are reading its data through /api/admin -
so nothing is concealed by pretending otherwise, and the refusal names
the route to use instead. "Forbidden" alone sends somebody hunting a
permissions problem that does not exist.

/api/auth/* stays on plain authed: a session is not a company's data, and
signing out or revoking a lost device must keep working for an account
with no tenant.

The fake could not have caught this either - it compares client ids as
strings and is perfectly content with "". The test asserts the contract
(403 and a message naming /api/admin) and a third case that matters more
than either: an ordinary tenant user still reaches all of it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KGcjxF1cNLcuwc3DAPcnfj
This commit is contained in:
Suriyakumarvijayanayagam
2026-09-28 19:18:41 +05:30
parent 4db71e9381
commit 830c1c1573
2 changed files with 133 additions and 35 deletions
Download Patch File Download Diff File Expand all files Collapse all files

100
server/internal/api/api.go
View File

@@ -300,68 +300,68 @@ func (s *Server) Routes() *http.ServeMux {
s.authed(s.handleRevokeOtherSessions))
// --- the people who work here ---
mux.HandleFunc("GET /api/team", s.authed(s.handleTeam))
mux.HandleFunc("PATCH /api/team/{id}", s.authed(s.handleUpdateTeamMember))
mux.HandleFunc("POST /api/team/members", s.authed(s.handleCreateMember))
mux.HandleFunc("POST /api/team/{id}/password", s.authed(s.handleResetPassword))
mux.HandleFunc("GET /api/team/invitations", s.authed(s.handleInvitations))
mux.HandleFunc("POST /api/team/invitations", s.authed(s.handleInvite))
mux.HandleFunc("GET /api/team", s.tenantOnly(s.handleTeam))
mux.HandleFunc("PATCH /api/team/{id}", s.tenantOnly(s.handleUpdateTeamMember))
mux.HandleFunc("POST /api/team/members", s.tenantOnly(s.handleCreateMember))
mux.HandleFunc("POST /api/team/{id}/password", s.tenantOnly(s.handleResetPassword))
mux.HandleFunc("GET /api/team/invitations", s.tenantOnly(s.handleInvitations))
mux.HandleFunc("POST /api/team/invitations", s.tenantOnly(s.handleInvite))
mux.HandleFunc("DELETE /api/team/invitations/{id}",
s.authed(s.handleRevokeInvitation))
s.tenantOnly(s.handleRevokeInvitation))
mux.HandleFunc("GET /api/reports/footfall", s.authed(s.handleFootfall))
mux.HandleFunc("GET /api/reports/conversion", s.authed(s.handleConversion))
mux.HandleFunc("GET /api/sites", s.authed(s.handleSites))
mux.HandleFunc("POST /api/sites", s.authed(s.handleCreateSite))
mux.HandleFunc("DELETE /api/sites/{site}", s.authed(s.handleDeleteSite))
mux.HandleFunc("PATCH /api/sites/{site}", s.authed(s.handleUpdateSite))
mux.HandleFunc("GET /api/reports/footfall", s.tenantOnly(s.handleFootfall))
mux.HandleFunc("GET /api/reports/conversion", s.tenantOnly(s.handleConversion))
mux.HandleFunc("GET /api/sites", s.tenantOnly(s.handleSites))
mux.HandleFunc("POST /api/sites", s.tenantOnly(s.handleCreateSite))
mux.HandleFunc("DELETE /api/sites/{site}", s.tenantOnly(s.handleDeleteSite))
mux.HandleFunc("PATCH /api/sites/{site}", s.tenantOnly(s.handleUpdateSite))
// Cameras, onboarded from head office. The shop PC still does the
// connecting - it is the only thing on the camera's network - so these
// write desired state that its agent pulls and applies.
mux.HandleFunc("GET /api/cameras", s.authed(s.handleCameras))
mux.HandleFunc("POST /api/sites/{site}/cameras", s.authed(s.handleCreateCamera))
mux.HandleFunc("PATCH /api/cameras/{id}", s.authed(s.handleUpdateCamera))
mux.HandleFunc("DELETE /api/cameras/{id}", s.authed(s.handleDeleteCamera))
mux.HandleFunc("GET /api/cameras/{id}/snapshot.jpg", s.authed(s.handleGetSnapshot))
mux.HandleFunc("GET /api/cameras/{id}/live", s.authed(s.handleWatchLive))
mux.HandleFunc("GET /api/cameras", s.tenantOnly(s.handleCameras))
mux.HandleFunc("POST /api/sites/{site}/cameras", s.tenantOnly(s.handleCreateCamera))
mux.HandleFunc("PATCH /api/cameras/{id}", s.tenantOnly(s.handleUpdateCamera))
mux.HandleFunc("DELETE /api/cameras/{id}", s.tenantOnly(s.handleDeleteCamera))
mux.HandleFunc("GET /api/cameras/{id}/snapshot.jpg", s.tenantOnly(s.handleGetSnapshot))
mux.HandleFunc("GET /api/cameras/{id}/live", s.tenantOnly(s.handleWatchLive))
// Prove a camera works: "connection" asks whether the shop PC can open the
// stream, "placement" asks whether somebody walking past produces a view
// good enough to recognise. Two questions, because a camera passes the
// first and fails the second all the time - that is the Office1 case.
mux.HandleFunc("POST /api/cameras/{id}/check", s.authed(s.handleRequestCheck))
mux.HandleFunc("POST /api/cameras/{id}/check", s.tenantOnly(s.handleRequestCheck))
// The end-to-end answer for one shop, assembled from what head office
// already knows - so it works even when the shop PC is off, which is one of
// the things it reports.
mux.HandleFunc("GET /api/sites/{site}/check", s.authed(s.handleSiteCheck))
mux.HandleFunc("GET /api/sites/{site}/check", s.tenantOnly(s.handleSiteCheck))
mux.HandleFunc("POST /api/sites/{site}/enrolment-code",
s.authed(s.handleIssueEnrolmentCode))
s.tenantOnly(s.handleIssueEnrolmentCode))
// The assistant. Every tool it calls runs as the signed-in user, so it can
// only ever see what the person asking could already see.
mux.HandleFunc("POST /api/assistant", s.authed(s.handleAssistant))
mux.HandleFunc("POST /api/assistant", s.tenantOnly(s.handleAssistant))
// The live feed. `visitors` searches a customer list by name; `visits`
// answers the question a shop screen or a mobile app actually asks - who
// came through the door just now - and carries each person's photo with
// them so rendering four simultaneous arrivals is one request, not nine.
mux.HandleFunc("GET /api/visits", s.authed(s.handleArrivals))
mux.HandleFunc("GET /api/visits/stream", s.authed(s.handleArrivalStream))
mux.HandleFunc("GET /api/visits", s.tenantOnly(s.handleArrivals))
mux.HandleFunc("GET /api/visits/stream", s.tenantOnly(s.handleArrivalStream))
mux.HandleFunc("GET /api/visitors", s.authed(s.handleVisitors))
mux.HandleFunc("GET /api/visitors/{id}/history", s.authed(s.handleVisitorHistory))
mux.HandleFunc("PUT /api/visitors/{id}/profile", s.authed(s.handleSaveProfile))
mux.HandleFunc("POST /api/purchases", s.authed(s.handlePurchase))
mux.HandleFunc("GET /api/visitors", s.tenantOnly(s.handleVisitors))
mux.HandleFunc("GET /api/visitors/{id}/history", s.tenantOnly(s.handleVisitorHistory))
mux.HandleFunc("PUT /api/visitors/{id}/profile", s.tenantOnly(s.handleSaveProfile))
mux.HandleFunc("POST /api/purchases", s.tenantOnly(s.handlePurchase))
// Reading sales, not just aggregating them. /api/reports/conversion has
// summed this table since it existed; nothing could read a row of it, so
// "revenue was 41,000" could not be checked against a till.
mux.HandleFunc("GET /api/sales", s.authed(s.handleSales))
mux.HandleFunc("GET /api/sales/{id}", s.authed(s.handleSale))
mux.HandleFunc("GET /api/sales", s.tenantOnly(s.handleSales))
mux.HandleFunc("GET /api/sales/{id}", s.tenantOnly(s.handleSale))
// The merchant home screen in one call, composed from the functions the
// reports already use rather than from new arithmetic.
mux.HandleFunc("GET /api/dashboard/summary", s.authed(s.handleDashboard))
mux.HandleFunc("GET /api/dashboard/summary", s.tenantOnly(s.handleDashboard))
// Platform administration. Not public registration: an open endpoint that
// mints tenants is a far larger thing to secure than one behind an account
@@ -402,15 +402,15 @@ func (s *Server) Routes() *http.ServeMux {
mux.HandleFunc("GET /api/agent/checks", s.agentAuthed(s.handleAgentChecks))
mux.HandleFunc("POST /api/agent/checks", s.agentAuthed(s.handleAgentCheckResult))
mux.HandleFunc("GET /api/visitors/{id}/image", s.authed(s.handleVisitorImage))
mux.HandleFunc("GET /api/visitors/{id}/image", s.tenantOnly(s.handleVisitorImage))
// The bytes of a face this server holds itself. Session-authenticated
// rather than a signed link: there is no third party to delegate to, and an
// unauthenticated URL would be a way to reach a customer's photograph with
// no session at all.
mux.HandleFunc("GET /api/faces/{id}", s.authed(s.handleGetFace))
mux.HandleFunc("GET /api/faces/{id}", s.tenantOnly(s.handleGetFace))
// The erasure path. Destroys the template and the photo; keeps the
// anonymous visit counts, which are legitimate aggregate data.
mux.HandleFunc("DELETE /api/visitors/{id}", s.authed(s.handleForgetVisitor))
mux.HandleFunc("DELETE /api/visitors/{id}", s.tenantOnly(s.handleForgetVisitor))
return mux
}
@@ -428,6 +428,36 @@ func PrincipalFrom(ctx context.Context) auth.Principal {
return p
}
// tenantOnly gates the routes that read or write one company's data.
//
// It exists because a platform admin has NO client - that absence is what
// defines them - and every tenant query scopes on `client_id = $1::uuid`.
// Handing it the empty string makes Postgres cast ” to a uuid, which is an
// ERROR rather than an empty result, so five live endpoints answered 500 to a
// signed-in platform admin: /api/visits, /api/cameras, /api/sites,
// /api/visitors and /api/reports/footfall. Found by calling them.
//
// 403 and not 404, unlike adminOnly. The two hide opposite things: a tenant
// must not learn that a platform surface exists, while a platform admin
// already knows the tenant surface does - they are looking at its data through
// /api/admin. Nothing is concealed by pretending otherwise, and "use the admin
// routes" is the useful answer.
//
// Guarding here rather than in each query is deliberate: a per-query fix is
// one a new query forgets, and the next one would 500 in production exactly
// like these did.
func (s *Server) tenantOnly(next http.HandlerFunc) http.HandlerFunc {
return s.authed(func(w http.ResponseWriter, r *http.Request) {
if PrincipalFrom(r.Context()).ClientID == "" {
writeErr(w, http.StatusForbidden, "not_a_tenant_account",
"This is a company's own data. A platform administrator "+
"reads it through /api/admin/clients/{id}/...")
return
}
next(w, r)
})
}
func (s *Server) authed(next http.HandlerFunc) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
tok := auth.BearerToken(r)

68
server/internal/api/tenant_guard_test.go Normal file
View File

@@ -0,0 +1,68 @@
package api
import (
"net/http"
"strings"
"testing"
)
// A platform admin has no client, and every tenant query scopes on one. Before
// this guard the empty string reached Postgres as `client_id = ”::uuid`,
// which is a cast ERROR and not an empty result - so five live endpoints
// answered 500 to a signed-in platform admin. Found by calling them, not by a
// test: the in-memory fake compares strings and is perfectly happy with "".
func TestATenantRouteRefusesAnAccountWithNoCompany(t *testing.T) {
s, fs := newServer(t)
seedPlatformAdmin(fs)
sess := login(t, s, "root@loyaly.ai", "admin123")
for _, path := range []string{
"/api/visits", "/api/cameras", "/api/sites", "/api/visitors",
"/api/reports/footfall", "/api/sales", "/api/dashboard/summary",
} {
rec := do(t, s, "GET", path, sess.Token, nil)
if rec.Code != http.StatusForbidden {
t.Errorf("%s: got %d, want 403 - a platform admin reads a company's "+
"data through /api/admin, and a 500 here reads as a broken "+
"server rather than a wrong door", path, rec.Code)
}
// The message has to say where to go instead; "forbidden" alone sends
// somebody hunting a permissions problem that does not exist.
if !strings.Contains(rec.Body.String(), "/api/admin") {
t.Errorf("%s: refusal should point at the admin routes: %s",
path, rec.Body.String())
}
}
}
// The guard must not lock a platform admin out of their own session, which is
// not a company's data and is how they sign out or revoke a lost device.
func TestAPlatformAdminKeepsTheirOwnSessionRoutes(t *testing.T) {
s, fs := newServer(t)
seedPlatformAdmin(fs)
sess := login(t, s, "root@loyaly.ai", "admin123")
for _, path := range []string{"/api/auth/me", "/api/auth/sessions"} {
if rec := do(t, s, "GET", path, sess.Token, nil); rec.Code != http.StatusOK {
t.Errorf("%s: got %d, want 200", path, rec.Code)
}
}
}
// And the ordinary case must be untouched: a tenant user still reaches
// everything they always did.
func TestATenantUserIsUnaffectedByTheGuard(t *testing.T) {
s, fs := newServer(t)
seedUser(fs)
sess := login(t, s, "manager@acme.com", "correct horse battery")
for _, path := range []string{
"/api/visits", "/api/cameras", "/api/sites", "/api/visitors",
"/api/sales", "/api/dashboard/summary",
} {
if rec := do(t, s, "GET", path, sess.Token, nil); rec.Code != http.StatusOK {
t.Errorf("%s: got %d, want 200 for an ordinary tenant account: %s",
path, rec.Code, rec.Body.String())
}
}
}