Incident-mutation routes 500 for a service-account caller: zero-value user id violates the acknowledged_by/assigned_to/user_id FK #25

New Issue

2026-10-02T19:53:33Z

niklas commented

2026-10-02 19:53:33 +00:00

Summary

Found during the authorization audit in #24 (not fixed there — deliberately
out of scope, see that PR's description). Every incident-mutation route
reads userFromContext(ctx) and writes the result's .ID into a real
foreign-key column, without checking the returned ok bool:

POST /api/incidents/{id}/acknowledge, DELETE .../acknowledge
POST .../resolve, POST .../assign
POST .../snooze, DELETE .../snooze
POST .../archive, DELETE .../archive
POST .../notes, DELETE .../notes/{eventID}

(all in internal/api/incidents.go)

For a human caller this is fine. For a team-scoped service account
calling one of these on an incident belonging to its own team — which it
can reach today, since requireTeamMember-equivalent scoping (via the
synthetic owner membership serveAsServiceAccount injects) already lets it
through — userFromContext returns ok=false and a zero-value
models.User{ID: 0}. The handler writes 0 anyway into:

incidents.acknowledged_by  BIGINT REFERENCES users(id) ON DELETE SET NULL
incidents.assigned_to      BIGINT REFERENCES users(id) ON DELETE SET NULL
incident_events.user_id    BIGINT REFERENCES users(id) ON DELETE SET NULL

(internal/db/migrations/001_baseline.sql). All three are nullable, but 0
is not NULL and no users row has id 0 — so the INSERT/UPDATE
foreign-key-violates and the request 500s, for every one of the nine routes
above.

Why this matters

SERVICE-ACCOUNTS.md already states the intended behavior: "a
service-account principal is stored and displayed distinctly... never
coerced into a user_id FK." That promise isn't implemented anywhere —
there is no service_account_id column on incidents or
incident_events, and the actual behavior is the opposite of "distinct and
displayed": a bare 0 that fails the constraint.

This is also a real, plausible capability gap, not just a crash: a
team-scoped service account is explicitly meant to act with that team's
owner-equivalent reach (escalation, dead-man switches, integrations, and —
per #24 — membership/invites). Acting on that team's own incidents
(auto-acknowledging, auto-resolving a stale one, a ChatOps-style
integration acknowledging on a channel's behalf) is a reasonable thing for
that same credential to do, and today it can reach these routes — it just
crashes the moment it tries to write anything.

Suggested fix (not prescriptive)

Needs a schema migration, which is why this is separate from #24's pure
authorization fix: an actor-attribution representation that can name either
a human (user_id) or a service account (e.g. a new nullable
service_account_id column, or a combined actor_type/actor_id pair) on
incidents.acknowledged_by/assigned_to and incident_events.user_id,
plus updating all nine handlers in internal/api/incidents.go to use
Caller.AsHuman()/Caller.ServiceAccountID() (both now exist,
internal/api/caller.go from #24) to populate the right column instead of
assuming a human.

Whatever shape is chosen, the timeline/incident JSON responses need to
render a service-account actor distinctly too (Caller.Identity() —
"service-account:7" — already exists for exactly this, unused today).

References

internal/api/incidents.go — the nine handlers.
internal/db/migrations/001_baseline.sql:101,103,139 — the three FK
columns.
internal/api/caller.go (landing in #24) — Caller.AsHuman(),
Caller.ServiceAccountID(), Caller.Identity().
SERVICE-ACCOUNTS.md — the unimplemented "stored and displayed
distinctly" promise.

## Summary Found during the authorization audit in #24 (not fixed there — deliberately out of scope, see that PR's description). Every incident-mutation route reads `userFromContext(ctx)` and writes the result's `.ID` into a real foreign-key column, without checking the returned `ok` bool: - `POST /api/incidents/{id}/acknowledge`, `DELETE .../acknowledge` - `POST .../resolve`, `POST .../assign` - `POST .../snooze`, `DELETE .../snooze` - `POST .../archive`, `DELETE .../archive` - `POST .../notes`, `DELETE .../notes/{eventID}` (all in `internal/api/incidents.go`) For a human caller this is fine. For a **team-scoped service account** calling one of these on an incident belonging to its own team — which it can reach today, since `requireTeamMember`-equivalent scoping (via the synthetic owner membership `serveAsServiceAccount` injects) already lets it through — `userFromContext` returns `ok=false` and a zero-value `models.User{ID: 0}`. The handler writes `0` anyway into: ``` incidents.acknowledged_by BIGINT REFERENCES users(id) ON DELETE SET NULL incidents.assigned_to BIGINT REFERENCES users(id) ON DELETE SET NULL incident_events.user_id BIGINT REFERENCES users(id) ON DELETE SET NULL ``` (`internal/db/migrations/001_baseline.sql`). All three are nullable, but `0` is not `NULL` and no `users` row has id `0` — so the `INSERT`/`UPDATE` foreign-key-violates and the request 500s, for every one of the nine routes above. ## Why this matters `SERVICE-ACCOUNTS.md` already states the intended behavior: *"a service-account principal is stored and displayed distinctly... never coerced into a `user_id` FK."* That promise isn't implemented anywhere — there is no `service_account_id` column on `incidents` or `incident_events`, and the actual behavior is the opposite of "distinct and displayed": a bare `0` that fails the constraint. This is also a real, plausible capability gap, not just a crash: a team-scoped service account is explicitly meant to act with that team's owner-equivalent reach (escalation, dead-man switches, integrations, and — per #24 — membership/invites). Acting on that team's own incidents (auto-acknowledging, auto-resolving a stale one, a ChatOps-style integration acknowledging on a channel's behalf) is a reasonable thing for that same credential to do, and today it can *reach* these routes — it just crashes the moment it tries to write anything. ## Suggested fix (not prescriptive) Needs a schema migration, which is why this is separate from #24's pure authorization fix: an actor-attribution representation that can name either a human (`user_id`) or a service account (e.g. a new nullable `service_account_id` column, or a combined `actor_type`/`actor_id` pair) on `incidents.acknowledged_by`/`assigned_to` and `incident_events.user_id`, plus updating all nine handlers in `internal/api/incidents.go` to use `Caller.AsHuman()`/`Caller.ServiceAccountID()` (both now exist, `internal/api/caller.go` from #24) to populate the right column instead of assuming a human. Whatever shape is chosen, the timeline/incident JSON responses need to render a service-account actor distinctly too (`Caller.Identity()` — `"service-account:7"` — already exists for exactly this, unused today). ## References - `internal/api/incidents.go` — the nine handlers. - `internal/db/migrations/001_baseline.sql:101,103,139` — the three FK columns. - `internal/api/caller.go` (landing in #24) — `Caller.AsHuman()`, `Caller.ServiceAccountID()`, `Caller.Identity()`. - `SERVICE-ACCOUNTS.md` — the unimplemented "stored and displayed distinctly" promise.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: niklas/terdut-server#25