ReviewOS

stacks/analyticshq

PATCH /api/sites/{siteId} answers 400 to a viewer where its own guard should answer 403

#49
Closed glennmichael123 opened this 9 days ago ·
9 days ago

Observed while testing per-site roles end to end. Not diagnosed — filing with what I have rather than sitting on it.

What happened

A user holding a viewer membership on a site sent:

PATCH /api/sites/{siteId}
Authorization: Bearer <viewer token>
Content-Type: application/json

{"name": "renamed"}

and got 400. Every other rank check in the same probe behaved correctly: an outsider got 403 on the members and invites endpoints, a viewer got 403 when trying to invite, and the same viewer got 200 on GET .../stats. Only this endpoint answered 400.

Why that is odd

The handler checks the role first, before it looks at the body at all:

route.patch('/api/sites/{siteId}', async (request: any) => {
  const siteId = request.params.siteId
  const denied = await requireSiteRole(request, siteId, 'admin')
  if (denied) return denied          // <- 401 / 403 / 404, never 400

  const body = request.jsonBody ?? {}
  ...
  if (!name) return json({ error: 'name cannot be empty' }, 400)

requireSiteRole only ever returns 401, 403 or 404. satisfies('viewer', 'admin') is false and is covered by unit tests. And the body sent was a non-empty string, so the one 400 in the handler should not have fired either.

So either the request never reached this handler, or something upstream answered first.

Candidates I did not rule out

  • A competing route. app/Models/Site.ts declares useApi: { uri: 'sites', routes: [..., 'update', ...] }. If auto-CRUD registers its own PATCH for this path and wins, the app's guard never runs — which would be an authorization problem, not just a wrong status code. routes/auth.ts:30-38 says no framework default routes are mounted in this app, which argues against it, but I did not verify it for model-generated routes specifically.
  • CSRF or body-parsing middleware rejecting before the handler. The route does carry .skipCsrf().
  • Something in my probe. Possible, though the same helper produced correct 403s against four other endpoints in the same run.

Why it is worth chasing

tests/unit/api-authz.test.ts asserts the guard is present in the source, by reading the file as text. That is a good guard against someone deleting the check, and no guard at all against the check never executing. If a generated route is shadowing a hand-written one anywhere, that test would keep passing while the endpoint went unprotected.

So the status code is the symptom worth following, whatever the cause turns out to be.

Reproduction

Owner creates a site, adds a second user as viewer in site_members, second user PATCHes the site with a valid {"name": "..."}. Expected 403, observed 400. Response body was not captured — capturing it is the obvious first step.

9 days ago

Worth recording what this turned out to be, since the title undersells it.

Cause. useApi on a model makes the ORM generate CRUD at /api/{uri}. Site declared it with the same five paths routes/analytics.ts serves by hand, and the generated PATCH /api/sites/{id} won. The 400 was Invalid ID parameter — verbatim from the generated handler, which coerces the id to a number while site ids here are strings.

So site updates were broken outright, for everyone including the owner: renaming a site, changing its timezone, editing its domains. The guard in routes/analytics.ts was correct, was asserted by api-authz.test.ts, and never executed.

And it was worse than PATCH. User declared useApi too, so GET /api/users returned every name and email in the database to an unauthenticated request locally. Generated reads are public unless a model opts into middleware, and row scoping only engages for a model declaring ownership or carrying team_id — User does neither. Production answers 401 to anonymous callers, so exposure there was to any signed-in account rather than the open internet; still every user's address. /api/goals was the same shape over every site's conversion data.

Nothing in the app ever called any of them. Removed the trait from all three in 9fb2d1c.

Verified: anonymous and authenticated GET /api/users no longer return the table, /api/goals is 404, the owner can rename a site and it persists, and a stranger PATCHing gets 403 from the role guard rather than a coercion error.

The lesson. Every test in api-authz.test.ts reads source and asserts a guard is present. None could see it was unreachable. Added a sweep asserting no model declares useApi, since re-adding it can simultaneously expose an unguarded surface and disable a guarded one.

Sign in to comment on this issue.