#64 - fix(api): lock down unauthenticated write endpoints — delete /event, gate /simulate, restrict webhook sources - gabogg/hikcentral

gabogg commented

2026-09-23 17:04:41 +00:00

Owner

Found while addressing the review of PR #53. Triaged 2026-09-24 (grilling session); the scope below is settled.

Problem

Three write paths accept data without the checks they need:

POST /api/occupancy/event (app/controllers/occupancy_controller.py, receive_counting_event) writes people-counting passages with no authentication at all. Those passages feed live occupancy, the published KPIs, calibration (k, baseline offset), the Trust Index and the operator flux stream. After #53 it only accepts registered cameras, and after #52 only IN/OUT (PASS removed), but anyone who can reach the server can still inject traffic. The payload has no lower bound on count and no timestamp bounds (backdating into closed cycles). Nothing uses it: no caller in app/, scripts/ or docs/, and zero requests in the production access log of 2026-09-18 (~453k logged API requests, checked read-only). Counting is 100% the Artemis polling sync.
POST /api/occupancy/simulate is admin-only, but it writes synthetic passages into the real people_counting_events table, where they move published KPIs, k and the Trust Index and can't be told apart from real traffic afterwards. Unused in production (zero requests in the same log).
POST /api/event/webhook (app/controllers/webhook_controller.py) is HikCentral's inbound callback for door events. It is unauthenticated, so anyone who can reach the server can post fake door opens, closes and alarms. It can't use bearer auth: HikCentral is the caller and has no user account. HikCentral runs on the same VM and subscribes with the address from webhook_manager.get_local_ip(). Production has received 3,453 door transitions through it, so it must keep working.

Settled scope

Delete POST /api/occupancy/event and its CountingEventWebhookPayload schema. Move the tests that used it: the INVALID_DIRECTION error-code check in tests/test_i18n.py, the event call in test_occupancy_api_endpoints_rbac, and the endpoint tests from #53 (test_event_endpoint_rejects_unregistered_camera) and #52 (test_event_endpoint_rejects_pass_direction). Keep the behaviours they covered under the batch ingestion interface where it still applies, and drop the rest. Remove now-unused i18n strings.
Gate /api/occupancy/simulate behind a new setting, enable_simulation, off by default (production off). When disabled it returns 403 with a clear error_code. Kept because it's useful in development and test.
Restrict POST /api/event/webhook by source address: accept loopback and the address the webhook subscribed with, plus an optional extra allowlist in config for a deployment where HikCentral runs on another host. Anything else gets 403 and a warning log line. Production needs no config change.

Door-command authorization (who may unlock or hold doors open) was split out to #72. It's a role policy, not a missing check.

Acceptance criteria

POST /api/occupancy/event and CountingEventWebhookPayload removed; no reference left in app code, frontend or i18n.
Tests that used /event migrated or removed as above; their still-relevant behaviours remain covered.
/api/occupancy/simulate refuses with 403 unless enable_simulation is on; default off; tests cover both states.
The webhook accepts loopback and the subscribed address, refuses others with 403 and a log line; the extra allowlist is configurable; tests cover accepted, refused and configured-extra sources.
Live door events on production unaffected (HikCentral on the same VM).
Full suite green (pytest + node --test).

Related: #29, PR #53, PR #52, #72.

> Found while addressing the review of PR #53. **Triaged 2026-09-24** (grilling session); the scope below is settled. ## Problem Three write paths accept data without the checks they need: 1. **`POST /api/occupancy/event`** (`app/controllers/occupancy_controller.py`, `receive_counting_event`) writes people-counting passages with **no authentication at all**. Those passages feed live occupancy, the published KPIs, calibration (`k`, baseline offset), the Trust Index and the operator flux stream. After #53 it only accepts registered cameras, and after #52 only `IN`/`OUT` (`PASS` removed), but anyone who can reach the server can still inject traffic. The payload has no lower bound on `count` and no timestamp bounds (backdating into closed cycles). **Nothing uses it:** no caller in `app/`, `scripts/` or `docs/`, and zero requests in the production access log of 2026-09-18 (~453k logged API requests, checked read-only). Counting is 100% the Artemis polling sync. 2. **`POST /api/occupancy/simulate`** is admin-only, but it writes **synthetic passages into the real `people_counting_events` table**, where they move published KPIs, `k` and the Trust Index and can't be told apart from real traffic afterwards. Unused in production (zero requests in the same log). 3. **`POST /api/event/webhook`** (`app/controllers/webhook_controller.py`) is HikCentral's inbound callback for door events. It is **unauthenticated**, so anyone who can reach the server can post fake door opens, closes and alarms. It can't use bearer auth: HikCentral is the caller and has no user account. HikCentral runs on the same VM and subscribes with the address from `webhook_manager.get_local_ip()`. Production has received 3,453 door transitions through it, so it must keep working. ## Settled scope 1. **Delete `POST /api/occupancy/event`** and its `CountingEventWebhookPayload` schema. Move the tests that used it: the `INVALID_DIRECTION` error-code check in `tests/test_i18n.py`, the event call in `test_occupancy_api_endpoints_rbac`, and the endpoint tests from #53 (`test_event_endpoint_rejects_unregistered_camera`) and #52 (`test_event_endpoint_rejects_pass_direction`). Keep the behaviours they covered under the batch ingestion interface where it still applies, and drop the rest. Remove now-unused i18n strings. 2. **Gate `/api/occupancy/simulate`** behind a new setting, `enable_simulation`, **off by default** (production off). When disabled it returns 403 with a clear `error_code`. Kept because it's useful in development and test. 3. **Restrict `POST /api/event/webhook` by source address:** accept loopback and the address the webhook subscribed with, plus an optional extra allowlist in config for a deployment where HikCentral runs on another host. Anything else gets **403 and a warning log line**. Production needs no config change. Door-command authorization (who may unlock or hold doors open) was split out to #72. It's a role policy, not a missing check. ## Acceptance criteria - [ ] `POST /api/occupancy/event` and `CountingEventWebhookPayload` removed; no reference left in app code, frontend or i18n. - [ ] Tests that used `/event` migrated or removed as above; their still-relevant behaviours remain covered. - [ ] `/api/occupancy/simulate` refuses with 403 unless `enable_simulation` is on; default off; tests cover both states. - [ ] The webhook accepts loopback and the subscribed address, refuses others with 403 and a log line; the extra allowlist is configurable; tests cover accepted, refused and configured-extra sources. - [ ] Live door events on production unaffected (HikCentral on the same VM). - [ ] Full suite green (pytest + `node --test`). Related: #29, PR #53, PR #52, #72.

gabogg referenced this issue

2026-09-23 17:04:53 +00:00

fix(occupancy): camera-group schema, orphan guard and migration — phase 1 (#29) #53

gabogg referenced this issue

2026-09-23 18:05:19 +00:00

refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14) #52

gabogg referenced this issue from a commit

2026-09-23 18:43:49 +00:00

fix(telemetry): address PR #52 review round 1

gabogg referenced this issue

2026-09-23 18:44:24 +00:00

refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14) #52

gabogg referenced this issue

2026-09-23 18:44:26 +00:00

refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14) #52

gabogg commented

2026-09-23 18:44:27 +00:00

Author

Owner

Partly addressed by PR #52 (2b78fdb): the PASS direction is gone. POST /api/occupancy/event now accepts only IN/OUT (400 INVALID_DIRECTION otherwise), and the batch input schema enforces Literal["IN", "OUT"]. Still open here: authentication, count >= 1, the timestamp bounds, or deleting the endpoint.

**Partly addressed by PR #52** (`2b78fdb`): the `PASS` direction is gone. `POST /api/occupancy/event` now accepts only `IN`/`OUT` (400 `INVALID_DIRECTION` otherwise), and the batch input schema enforces `Literal["IN", "OUT"]`. Still open here: authentication, `count >= 1`, the timestamp bounds, or deleting the endpoint.

gabogg referenced this issue

2026-09-23 22:15:44 +00:00

fix(telemetry): PR #52 review follow-ups — throttle ignores direct fetches, direction typing, engine contract docs #67

gabogg referenced this issue

2026-09-23 22:16:11 +00:00

refactor(telemetry): remove push-then-pull round-trip and batch the sync broadcast (#14) #52

gabogg changed title from ~~[security] POST /api/occupancy/event is unauthenticated and accepts unvalidated passages~~ to fix(occupancy): POST /api/occupancy/event is unauthenticated and accepts unvalidated passages

2026-09-24 10:15:34 +00:00

gabogg added the

needs-triage

label

2026-09-24 10:15:35 +00:00

gabogg changed title from ~~fix(occupancy): POST /api/occupancy/event is unauthenticated and accepts unvalidated passages~~ to fix(api): lock down unauthenticated write endpoints — delete /event, gate /simulate, restrict webhook sources

2026-09-24 12:56:28 +00:00

gabogg added

ready-for-agent

and removed

needs-triage

labels

2026-09-24 12:56:29 +00:00

gabogg commented

2026-09-24 12:56:30 +00:00

Author

Owner

Triaged 2026-09-24 (grilling session). Scope settled and written into the description above; relabelled ready-for-agent and retitled.

Decisions:

Delete /event: unused (no code caller, zero production requests in the logged period); /simulate covers manual testing.
Gate /simulate behind enable_simulation, default off. Kept, but it can no longer pollute production figures by accident.
Webhook: can't take user auth (HikCentral is the caller), so it's locked by source address: loopback plus the subscribed address, plus an optional configured allowlist.
Door-command roles (who may hold doors open) split into their own issue, since that's a role policy rather than a missing check.

**Triaged 2026-09-24** (grilling session). Scope settled and written into the description above; relabelled `ready-for-agent` and retitled. Decisions: - **Delete `/event`**: unused (no code caller, zero production requests in the logged period); `/simulate` covers manual testing. - **Gate `/simulate`** behind `enable_simulation`, default off. Kept, but it can no longer pollute production figures by accident. - **Webhook**: can't take user auth (HikCentral is the caller), so it's locked by source address: loopback plus the subscribed address, plus an optional configured allowlist. - **Door-command roles** (who may hold doors open) split into their own issue, since that's a role policy rather than a missing check.

gabogg referenced this issue

2026-09-24 12:56:31 +00:00

feat(doors): role policy and audit for door commands #72

gabogg referenced this issue

2026-09-25 01:11:43 +00:00

fix(telemetry): direct fetches reset the refresh throttle; one Direction type (#67) #91

gabogg referenced this issue

2026-09-25 08:39:17 +00:00

fix(telemetry): direct fetches reset the refresh throttle; one Direction type (#67) #91