Enforce the checks the service edges only appeared to have - #24
Conversation
…d to have Three edges, each with a check that read as present and was not. **Anyone could open an intake session.** The WebSocket route called `accept()` unconditionally. `allowed_origins` was wired only into `CORSMiddleware`, which never sees this request — a browser sends no preflight for a WebSocket handshake and applies no same-origin rule to it, so any page on the internet could open a session. The handshake is now rejected before `accept()` for an origin that is not allowed. The Origin header cannot be forged from script, which is exactly the attacker this is for; it is not a substitute for the session auth that arrives with the real deployment. `localhost` joins `127.0.0.1` in the default allowlist. They are different origins to a browser and the demo page is reachable at either — a check that rejects half the URLs a developer types gets widened to `*` and stays there. **The persistence stub accepted any bearer token.** It tested that the header started with `Bearer ` and never compared it to the configured secret, while `persist_turn_token` sitting in config made it read as enforced. Now compared with `compare_digest`. It is a stub, but the real TypeScript route gets written from this file, and a stub that teaches the shape of a check without the substance of it teaches the wrong thing. **The ADR-013 self-hosted deployment was unreachable.** `allow_self_hosted_host` was threaded through every URL builder and passed by no call site, so pointing `deepgram_host` at a private host in ca-central-1 — the deployment the ADR specifies — raised `DeepgramUrlError` from every call. It now defaults from a new `deepgram_allow_self_hosted_host` setting, off by default so a typo in the host still fails loudly rather than quietly sending audio somewhere unintended. A flag only the tests can set is not a flag. The URL choke point itself is unchanged: `mip_opt_out=true` is still welded on after the caller's parameters, on every endpoint, including this path where it is inert. Co-Authored-By: Claude <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR tightens three “service-edge” checks in the voice gateway: (1) rejecting cross-origin WebSocket intake handshakes before accept(), (2) enforcing the configured persistence stub bearer token, and (3) making the ADR-013 self-hosted Deepgram host reachable via configuration while still failing unknown hosts by default.
Changes:
- Add explicit Origin allowlist enforcement for
/v1/intake/streambefore accepting the WebSocket. - Enforce persistence stub bearer token value (not just “Bearer ” shape) using constant-time comparison.
- Default Deepgram self-hosted host allowance from config (
deepgram_allow_self_hosted_host) and document it in.env.example, with tests covering these boundaries.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| services/voice-gateway/tests/test_service_boundaries.py | Adds end-to-end and unit tests for Origin matching, stub token enforcement, and self-hosted Deepgram host configuration. |
| services/voice-gateway/src/voice_gateway/persistence/stub_server.py | Enforces the configured persistence stub bearer token using secrets.compare_digest. |
| services/voice-gateway/src/voice_gateway/deepgram/urls.py | Makes allow_self_hosted_host default from config and updates the host validation/error path accordingly. |
| services/voice-gateway/src/voice_gateway/config.py | Adds deepgram_allow_self_hosted_host setting and expands default allowed_origins to include localhost. |
| services/voice-gateway/src/voice_gateway/app.py | Adds _origin_is_allowed and rejects disallowed WebSocket Origins prior to accept(). |
| services/voice-gateway/.env.example | Documents VOICE_GATEWAY_DEEPGRAM_ALLOW_SELF_HOSTED_HOST. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not authorization or not authorization.startswith("Bearer "): | ||
| raise HTTPException(status_code=401, detail="missing bearer token") | ||
|
|
||
| # Compare it, rather than merely observing that it is shaped like a token. | ||
| # A stub that accepts any bearer teaches the shape of the check without the | ||
| # substance of it, and the TypeScript route gets written from this file. | ||
| # `compare_digest` because the comparison is against a shared secret. | ||
| expected = get_settings().persist_turn_token | ||
| if not secrets.compare_digest(authorization.removeprefix("Bearer "), expected): | ||
| raise HTTPException(status_code=401, detail="invalid bearer token") |
|
| Filename | Overview |
|---|---|
| services/voice-gateway/src/voice_gateway/app.py | Adds _origin_is_allowed helper and moves the origin check before websocket.accept() — correct ASGI pattern for rejecting a handshake at the HTTP layer rather than opening then closing. |
| services/voice-gateway/src/voice_gateway/config.py | Adds deepgram_allow_self_hosted_host (defaults False) and expands allowed_origins default to include http://localhost:8080 alongside http://127.0.0.1:8080; both changes are consistent with the rest of the settings model. |
| services/voice-gateway/src/voice_gateway/deepgram/urls.py | Changes allow_self_hosted_host parameter type from bool = False to `bool |
| services/voice-gateway/src/voice_gateway/persistence/stub_server.py | Adds actual bearer token comparison with secrets.compare_digest — previously the endpoint only verified the Bearer prefix, not the value; now enforces the configured secret against the presented token. |
| services/voice-gateway/tests/test_service_boundaries.py | New test file covering all three edges: cross-origin WS rejection end-to-end, origin-matching table including the prefix-match non-vulnerability, token enforcement, and self-hosted host reachability by configuration. Token test uses Settings() directly rather than the cached get_settings(), which is consistent in practice but creates a subtle coupling. |
| services/voice-gateway/.env.example | Documents the new VOICE_GATEWAY_DEEPGRAM_ALLOW_SELF_HOSTED_HOST variable with a clear note that it is off by default and why. |
Sequence Diagram
sequenceDiagram
participant B as Browser
participant GW as Voice Gateway (app.py)
participant S as Settings
B->>GW: WebSocket Upgrade (Origin: https://evil.example)
GW->>S: app.state.settings.allowed_origins
S-->>GW: ["http://127.0.0.1:8080", "http://localhost:8080"]
GW->>GW: _origin_is_allowed("https://evil.example", allowed) → False
GW-->>B: websocket.close(1008) — before accept()
B->>GW: WebSocket Upgrade (Origin: http://localhost:8080)
GW->>S: app.state.settings.allowed_origins
S-->>GW: ["http://127.0.0.1:8080", "http://localhost:8080"]
GW->>GW: _origin_is_allowed("http://localhost:8080", allowed) → True
GW-->>B: websocket.accept() → 101 Switching Protocols
GW->>GW: IntakeSession.start()
Prompt To Fix All With AI
### Issue 1
services/voice-gateway/tests/test_service_boundaries.py:82
**`Settings()` and `get_settings()` can diverge under monkeypatching**
`_post(client, Settings().persist_turn_token)` constructs a fresh `Settings` instance, while the stub endpoint calls the `@lru_cache`-ed `get_settings()`. In this file those two agree because neither is monkeypatched here. However, `get_settings` is cached at the process level, so if any fixture or test that ran earlier in the session replaced it (e.g., the `monkeypatch.setattr(config, "get_settings", ...)` calls below), and the monkeypatch teardown restored the *function reference* but the cache was cold again, a subsequent call to `get_settings()` in the stub endpoint re-populates from the real environment while `Settings()` also reads from it — still consistent. The subtle risk is that a test helper in another file that patches `get_settings` without the `monkeypatch` fixture (i.e., without automatic teardown) could leave the stub endpoint returning a cached token that differs from what `Settings()` produces. Using `get_settings().persist_turn_token` instead of `Settings().persist_turn_token` would keep both sides of the assertion on the same instance.
### Issue 2
services/voice-gateway/src/voice_gateway/app.py:114-115
**`"*"` wildcard also admits `None`-origin (native) clients**
The check `if "*" in allowed: return True` fires before the `origin is None` guard, so when `VOICE_GATEWAY_ALLOWED_ORIGINS=["*"]`, `_origin_is_allowed(None, ["*"])` returns `True` — admitting `wscat`, curl, or any raw WebSocket client with no `Origin` header. The test at line 44 documents this explicitly, and the behaviour is intentional for test/dev environments. It is worth confirming that the production deployment does *not* set `"*"` as the only allowed origin, since doing so would bypass the check for native clients while the session-level auth is still marked "not yet" in the code comment.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(voice-gateway): enforce the checks t..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| with TestClient(stub) as client: | ||
| assert _post(client, None) == 401 | ||
| assert _post(client, "not-the-secret") == 401 | ||
| assert _post(client, Settings().persist_turn_token) == 201 |
There was a problem hiding this comment.
Settings() and get_settings() can diverge under monkeypatching
_post(client, Settings().persist_turn_token) constructs a fresh Settings instance, while the stub endpoint calls the @lru_cache-ed get_settings(). In this file those two agree because neither is monkeypatched here. However, get_settings is cached at the process level, so if any fixture or test that ran earlier in the session replaced it (e.g., the monkeypatch.setattr(config, "get_settings", ...) calls below), and the monkeypatch teardown restored the function reference but the cache was cold again, a subsequent call to get_settings() in the stub endpoint re-populates from the real environment while Settings() also reads from it — still consistent. The subtle risk is that a test helper in another file that patches get_settings without the monkeypatch fixture (i.e., without automatic teardown) could leave the stub endpoint returning a cached token that differs from what Settings() produces. Using get_settings().persist_turn_token instead of Settings().persist_turn_token would keep both sides of the assertion on the same instance.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/voice-gateway/tests/test_service_boundaries.py
Line: 82
Comment:
**`Settings()` and `get_settings()` can diverge under monkeypatching**
`_post(client, Settings().persist_turn_token)` constructs a fresh `Settings` instance, while the stub endpoint calls the `@lru_cache`-ed `get_settings()`. In this file those two agree because neither is monkeypatched here. However, `get_settings` is cached at the process level, so if any fixture or test that ran earlier in the session replaced it (e.g., the `monkeypatch.setattr(config, "get_settings", ...)` calls below), and the monkeypatch teardown restored the *function reference* but the cache was cold again, a subsequent call to `get_settings()` in the stub endpoint re-populates from the real environment while `Settings()` also reads from it — still consistent. The subtle risk is that a test helper in another file that patches `get_settings` without the `monkeypatch` fixture (i.e., without automatic teardown) could leave the stub endpoint returning a cached token that differs from what `Settings()` produces. Using `get_settings().persist_turn_token` instead of `Settings().persist_turn_token` would keep both sides of the assertion on the same instance.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if "*" in allowed: | ||
| return True |
There was a problem hiding this comment.
"*" wildcard also admits None-origin (native) clients
The check if "*" in allowed: return True fires before the origin is None guard, so when VOICE_GATEWAY_ALLOWED_ORIGINS=["*"], _origin_is_allowed(None, ["*"]) returns True — admitting wscat, curl, or any raw WebSocket client with no Origin header. The test at line 44 documents this explicitly, and the behaviour is intentional for test/dev environments. It is worth confirming that the production deployment does not set "*" as the only allowed origin, since doing so would bypass the check for native clients while the session-level auth is still marked "not yet" in the code comment.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/voice-gateway/src/voice_gateway/app.py
Line: 114-115
Comment:
**`"*"` wildcard also admits `None`-origin (native) clients**
The check `if "*" in allowed: return True` fires before the `origin is None` guard, so when `VOICE_GATEWAY_ALLOWED_ORIGINS=["*"]`, `_origin_is_allowed(None, ["*"])` returns `True` — admitting `wscat`, curl, or any raw WebSocket client with no `Origin` header. The test at line 44 documents this explicitly, and the behaviour is intentional for test/dev environments. It is worth confirming that the production deployment does *not* set `"*"` as the only allowed origin, since doing so would bypass the check for native clients while the session-level auth is still marked "not yet" in the code comment.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Brings in the two review fixes that had not yet landed: frozen wire collections with the extraction attribution hole closed, and the failure-recording and splice holes found in review. Every other slice (schema, intake, eval, voice, voice-boundaries, voice-turn-binding) was already contained in rebuild via #22-#24. Co-Authored-By: Claude <noreply@anthropic.com>
Stack — merge bottom to top
slice/voice→prod)Three edges of this service, each with a check that read as present and was not.
Anyone could open an intake session
The WebSocket route called
accept()unconditionally.allowed_originswas wired only intoCORSMiddleware— which never sees this request. A browser sends no preflight for a WebSocket handshake and applies no same-origin rule to it, so any page on the internet could open a session against a service that sits inside the PHI boundary.The handshake is now rejected before
accept(). The Origin header is set by the browser and cannot be forged from script, which is exactly the attacker this is for; it is not a substitute for the session auth that arrives with the real deployment, and the code says so.localhostjoins127.0.0.1in the default allowlist. They are different origins to a browser and the demo page is reachable at either — a check that rejects half the URLs a developer actually types gets widened to*and stays there.The persistence stub accepted any bearer token
It tested that the header started with
Bearerand never compared it to the configured secret, whilepersist_turn_tokensitting in config made it read as enforced. Now compared withcompare_digest.It is a stub and stays one. But the real TypeScript route gets written from this file, and a stub that teaches the shape of a check without the substance of it teaches the wrong thing.
The ADR-013 self-hosted deployment was unreachable
allow_self_hosted_hostwas threaded through every URL builder and passed by no call site. Pointingdeepgram_hostat a private host inca-central-1— the deployment ADR-013 specifies, because there is no Canadian Deepgram region — raisedDeepgramUrlErrorfrom every call.It now defaults from a new
deepgram_allow_self_hosted_hostsetting. Off by default, so a typo in the host still fails loudly rather than quietly sending audio somewhere unintended. A flag only the tests can set is not a flag.The choke point is unchanged.
mip_opt_out=trueis still welded on after the caller's parameters, on every endpoint, including the self-hosted path where it is inert — a guarantee that holds only while one deployment mode holds is not a guarantee. The AST test still passes.Tests
tests/test_service_boundaries.py, ten cases: cross-origin handshake rejection end to end, the origin-matching table (including that it is not a prefix match), the stub rejecting a wrong token and accepting the right one, and the self-hosted host reachable by configuration while an unrecognised host still fails without the flag.129 pass. Cross-origin rejection also confirmed against the running server:
https://evil.examplegets HTTP 403 at the handshake,http://127.0.0.1:8080connects.🤖 Generated with Claude Code