Any signed-in viewer can trigger OpenAI summary spend: incident summary routes use require_service_or_firebase_token, not require_admin_token #81

Closed
opened 2026-08-26 09:31:22 -04:00 by logan · 2 comments
Owner

Found during the standing correlation static pass, 2026-08-26.

The incident summary routes in routers/incidents.py are gated by require_service_or_firebase_token. That accepts any authenticated Firebase user, including a viewer. Hitting them spends OpenAI credits.

This is the same hole that was already closed on the call side in routers/calls.py, which was moved to require_admin_token. The incident side was not moved with it.

Why it matters now

  • Uncapped spend triggerable by the lowest-privilege account we issue. Directly relevant to the comped friends-and-family tier having no spend cap (#50).
  • It corrupts the 7-day AI cost measurement that Gate B5 (#45) depends on, because spend can originate outside the pipeline being measured.

Fix shape

Move the summary routes to require_admin_token, matching calls.py. Check first whether the frontend calls them as a signed-in non-admin — if it does, this needs an explicit operator-or-above check rather than admin-only.

Also in the same file: the route that returns a silent {"ok": true} when it did nothing because summaries_enabled was off should report the flag state, so a caller can tell "done" from "skipped". That part is being fixed alongside #76.

Refs #76, #45, #50, #64.

Found during the standing correlation static pass, 2026-08-26. The incident summary routes in `routers/incidents.py` are gated by `require_service_or_firebase_token`. That accepts **any** authenticated Firebase user, including a `viewer`. Hitting them spends OpenAI credits. This is the same hole that was already closed on the call side in `routers/calls.py`, which was moved to `require_admin_token`. The incident side was not moved with it. ## Why it matters now - Uncapped spend triggerable by the lowest-privilege account we issue. Directly relevant to the comped friends-and-family tier having no spend cap (#50). - It corrupts the 7-day AI cost measurement that Gate B5 (#45) depends on, because spend can originate outside the pipeline being measured. ## Fix shape Move the summary routes to `require_admin_token`, matching `calls.py`. Check first whether the frontend calls them as a signed-in non-admin — if it does, this needs an explicit operator-or-above check rather than admin-only. Also in the same file: the route that returns a silent `{"ok": true}` when it did nothing because `summaries_enabled` was off should report the flag state, so a caller can tell "done" from "skipped". That part is being fixed alongside #76. Refs #76, #45, #50, #64.
Author
Owner

Partially addressed by d18e4f0 (unattended run 2026-08-27). Leaving this open — the main defect is untouched.

Done:

  • The silent {"ok": true} is gone. POST /incidents/{id}/summarize now reports summaries_enabled in its response, so a caller can tell "done" from "skipped".
  • _summarize_incident itself now checks the flag, so the guard holds at the worker and not only at the loop that schedules it.
  • The same commit closed the sibling hole in PATCH /calls/{id}/transcript, where learn_from_correction was spending with vocabulary_learning_enabled off.

Still open — the actual issue title. The incident summary routes remain on require_service_or_firebase_token, so any authenticated Firebase user including a viewer can still trigger them. Not moved to require_admin_token because this issue's own body flags the open question first: whether the frontend calls these routes as a signed-in non-admin. There are zero JS/TS tests in the project, so a wrong guess there breaks a live page with nothing to catch it. That check is the next step and it is a frontend read, not a backend one.

One inconsistency worth resolving when this is picked up. The flags-off refusal is now expressed two different ways in adjacent files: PATCH /calls/{id}/transcript raises 409, while POST /incidents/{id}/summarize returns 200 with {"ok": false}. A caller checking only the status code will read the second as success. The 409 is the better convention — the summarize route was left as-is in the same pass specifically because changing its status code is a frontend-visible contract change.

Refs #76, #45, #50, #64.

**Partially addressed by `d18e4f0` (unattended run 2026-08-27). Leaving this open — the main defect is untouched.** Done: - The silent `{"ok": true}` is gone. `POST /incidents/{id}/summarize` now reports `summaries_enabled` in its response, so a caller can tell "done" from "skipped". - `_summarize_incident` itself now checks the flag, so the guard holds at the worker and not only at the loop that schedules it. - The same commit closed the sibling hole in `PATCH /calls/{id}/transcript`, where `learn_from_correction` was spending with `vocabulary_learning_enabled` off. **Still open — the actual issue title.** The incident summary routes remain on `require_service_or_firebase_token`, so any authenticated Firebase user including a `viewer` can still trigger them. Not moved to `require_admin_token` because this issue's own body flags the open question first: whether the frontend calls these routes as a signed-in non-admin. There are zero JS/TS tests in the project, so a wrong guess there breaks a live page with nothing to catch it. That check is the next step and it is a frontend read, not a backend one. **One inconsistency worth resolving when this is picked up.** The flags-off refusal is now expressed two different ways in adjacent files: `PATCH /calls/{id}/transcript` raises **409**, while `POST /incidents/{id}/summarize` returns **200** with `{"ok": false}`. A caller checking only the status code will read the second as success. The 409 is the better convention — the summarize route was left as-is in the same pass specifically because changing its status code is a frontend-visible contract change. Refs #76, #45, #50, #64.
Author
Owner

Fixed in 8b6c170 (unattended run 2026-09-01).

Actual defect: POST /incidents/{id}/summarize used require_service_or_firebase_token, which accepts any authenticated Firebase user including role viewer, and that route spends OpenAI credits via the background summarizer. Moved to require_admin_token, matching calls.py.

Swept the rest of incidents.py: list_incidents and get_incident are reads with no spend and correctly stay on require_service_or_firebase_token; create/update/delete/link/unlink incident were already require_admin_token. No other route in the file needed changing.

Frontend-caller check (the mandatory step): both summarize-triggering UI elements on app/incidents/[id]/page.tsx (the Regenerate summary button and the inline Generate now link) are already wrapped in isAdmin. So locking the backend to admin-only matches existing UI behavior exactly — no frontend change made, nothing on screen breaks.

Added tests/test_incident_summarize_auth.py, which asserts the route dependency wiring directly (the convention already used in test_admin_feature_flags.py) so a revert back to the weak dependency fails a test immediately.

Tests: 293 passed (289 baseline + 4 new), 0 failed, Server/drb-c2-core suite.

Found but out of scope, filed separately: POST /trips/{trip_id}/chat (drb-c2-core/app/routers/trips.py) has the identical shape — require_service_or_firebase_token guarding a gpt-4o-mini call — but it is a deliberately-designed feature (trips-feature-intentional) already carrying its own per-caller rate limiter as the spend control, so I did not touch it. See new issue for a scoped decision on whether that limiter is sufficient or it also needs a role gate.

No pipeline files (upload.py, incident_correlator.py, llm_correlator.py, intelligence.py) were touched.

Fixed in 8b6c170 (unattended run 2026-09-01). Actual defect: POST /incidents/{id}/summarize used require_service_or_firebase_token, which accepts any authenticated Firebase user including role viewer, and that route spends OpenAI credits via the background summarizer. Moved to require_admin_token, matching calls.py. Swept the rest of incidents.py: list_incidents and get_incident are reads with no spend and correctly stay on require_service_or_firebase_token; create/update/delete/link/unlink incident were already require_admin_token. No other route in the file needed changing. Frontend-caller check (the mandatory step): both summarize-triggering UI elements on app/incidents/[id]/page.tsx (the Regenerate summary button and the inline Generate now link) are already wrapped in isAdmin. So locking the backend to admin-only matches existing UI behavior exactly — no frontend change made, nothing on screen breaks. Added tests/test_incident_summarize_auth.py, which asserts the route dependency wiring directly (the convention already used in test_admin_feature_flags.py) so a revert back to the weak dependency fails a test immediately. Tests: 293 passed (289 baseline + 4 new), 0 failed, Server/drb-c2-core suite. Found but out of scope, filed separately: POST /trips/{trip_id}/chat (drb-c2-core/app/routers/trips.py) has the identical shape — require_service_or_firebase_token guarding a gpt-4o-mini call — but it is a deliberately-designed feature (trips-feature-intentional) already carrying its own per-caller rate limiter as the spend control, so I did not touch it. See new issue for a scoped decision on whether that limiter is sufficient or it also needs a role gate. No pipeline files (upload.py, incident_correlator.py, llm_correlator.py, intelligence.py) were touched.
logan closed this issue 2026-09-01 02:45:36 -04:00
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: logan/server-26#81