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

New Issue

2026-08-26T09:31:22-04:00

logan commented

2026-08-26 09:31:22 -04:00

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.

logan referenced this issue

2026-08-26 20:29:24 -04:00

Two AI spend paths escape config/ai_features entirely, so "AI is off" is not true and #45 cannot measure spend #76

logan referenced this issue

2026-08-26 20:30:54 -04:00

Two AI spend paths escape config/ai_features entirely, so "AI is off" is not true and #45 cannot measure spend #76

logan referenced this issue from a commit

2026-08-27 02:49:19 -04:00

Make "AI is off" true, and stop the transcript PATCH from destroying calls

logan commented

2026-08-27 02:51:08 -04:00

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.

logan referenced this issue

2026-08-27 02:56:26 -04:00

Two AI spend paths escape config/ai_features entirely, so "AI is off" is not true and #45 cannot measure spend #76

logan commented

2026-09-01 02:45:24 -04:00

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 referenced this issue

2026-09-01 02:45:35 -04:00

trip_chat spend gate relies solely on per-caller rate limit, not a role check (found during #81) #98

logan closed this issue

2026-09-01 02:45:36 -04:00

logan referenced this issue

2026-09-01 02:58:58 -04:00

Registry credential expires mid-build, so a deploy fails after some images have already pushed — cost us a night's deploy on 2026-09-01 #100

logan added the area/auth area/billing

type

security

labels 2026-10-05 02:04:39 -04:00