fix(security): verify provider signatures on telephony webhooks and pin recording hosts (PT-05/12/23) - #989
fix(security): verify provider signatures on telephony webhooks and pin recording hosts (PT-05/12/23)#989murdore wants to merge 1 commit into
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughTelephony webhook verification is centralized for Twilio, Plivo, and Exotel. Configuration controls enforcement and trusted URL prefixes. Callback handlers verify requests before processing. Exotel callbacks include authentication, and sensitive logging is redacted. ChangesTelephony webhook authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Provider
participant TelephonyHandler
participant verify_provider_webhook
participant CallbackAction
Provider->>TelephonyHandler: Send callback webhook
TelephonyHandler->>verify_provider_webhook: Verify provider request
verify_provider_webhook-->>TelephonyHandler: Return verification result
TelephonyHandler->>CallbackAction: Process verified callback
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
EvidenceA real screencast — Chrome DevTools Each take runs three acts: attack Telephony callback signaturespr989-telephony-signatures.mp4local original: Unsigned POSTs to And what must still work — routes stay mounted and reachable, signed traffic unaffected, all accounts still sign in. 932 tests pass. Token comparison is constant-time and a missing expected token denies rather than admits, so a misconfigured deploy fails closed.
|
dd556e1 to
3619142
Compare
4bc5aed to
4ffa725
Compare
4ffa725 to
d03fd7c
Compare
There was a problem hiding this comment.
Pull request overview
Adds shared, fail-closed authentication for telephony provider webhooks to prevent forged callback/answer requests from driving call state or injecting attacker-controlled recording URLs.
Changes:
- Introduces
app/core/security/webhook_signature.pywith Twilio (X-Twilio-Signature), Plivo (V3/V2), and Exotel (auth_token) verification plus a sharedverify_provider_webhook()gate. - Applies webhook verification across Breeze Buddy telephony callback and answer handlers, and adds
ENFORCE_TELEPHONY_WEBHOOK_SIGNATURES(default enabled) as a rollout escape hatch. - Updates Exotel outbound call initiation to embed the shared
auth_tokeninto the status callback URL; adds a focused pentest regression test for Exotel token verification.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_pentest_telephony_auth.py | Adds pentest regression test coverage for Exotel webhook token verification. |
| app/core/security/webhook_signature.py | New shared verification module for Twilio/Plivo signatures and Exotel auth token, with centralized reject behavior. |
| app/core/config/static.py | Adds ENFORCE_TELEPHONY_WEBHOOK_SIGNATURES static config flag (default true). |
| app/api/routers/breeze_buddy/telephony/callbacks/handlers.py | Enforces provider authentication before processing callback/transfer/status/twiml-fallback requests. |
| app/api/routers/breeze_buddy/telephony/answer/init.py | Enforces provider authentication for Exotel/Plivo answer webhooks via shared verifier. |
| app/ai/voice/agents/breeze_buddy/services/telephony/exotel/exotel.py | Embeds Exotel auth_token into status callback URL used in outbound call creation. |
| .env.example | Documents and exposes ENFORCE_TELEPHONY_WEBHOOK_SIGNATURES default. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| """PT-23/PT-05: constant-time provider token comparison, fail-closed when unset.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from app.schemas import ( | ||
| UserInfo, | ||
| UserRole, | ||
| ) | ||
|
|
||
|
|
||
| def _user(role: str, resellers, merchants, owner_id=None) -> UserInfo: | ||
| return UserInfo( | ||
| id="u1", | ||
| username="u1", | ||
| role=UserRole(role), | ||
| email=None, | ||
| reseller_ids=list(resellers), | ||
| merchant_ids=list(merchants), | ||
| permissions=[], | ||
| owner_id=owner_id, | ||
| ) | ||
|
|
||
|
|
||
| # ── PT-23/05: constant-time exotel token + fail-closed when unset ───────── | ||
| def test_verify_exotel_token(monkeypatch): | ||
| from app.core.security import webhook_signature as ws | ||
|
|
||
| monkeypatch.setattr(ws, "EXOTEL_WEBHOOK_AUTH_TOKEN", "s3cret") | ||
| assert ws.verify_exotel_token("s3cret") is True | ||
| assert ws.verify_exotel_token("wrong") is False | ||
| monkeypatch.setattr(ws, "EXOTEL_WEBHOOK_AUTH_TOKEN", "") | ||
| assert ws.verify_exotel_token("s3cret") is False # fail closed when unset |
| logger.error( | ||
| "EXOTEL_WEBHOOK_AUTH_TOKEN is unset — Exotel status callbacks will be " | ||
| "rejected. Set it (and keep it in sync with this deployment) or " | ||
| "Exotel outcome/retry handling will not run." | ||
| ) |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed all 7 changed files. The PR correctly centralizes webhook signature verification and switches Exotel to a constant-time compare, which addresses PT-05/PT-23. No existing migration files were modified, and no hardcoded secrets were introduced.
New issues raised
- 1 CRITICAL —
EXOTEL_WEBHOOK_AUTH_TOKENis embedded in the callback URL, exposing the secret to Exotel/proxy logs and the dashboard; the payload is also logged unredacted. - 3 MAJOR — path-rewriting proxy can break signature reconstruction; Twilio POST signature validation coerces
UploadFile/form values; callback handlers pass raw mixed-caseproviderto the verifier before normalization. - 2 MINOR — misleading "will be rejected" log wording; unused
HTTPExceptionimport. - 1 SUGGESTION — confirm Plivo V2 signature helper expects full URL vs. URI-only.
Existing comments noted
Two Copilot review comments were already present and are not duplicated here:
- Test coverage gap for Twilio/Plivo verifiers in
tests/test_pentest_telephony_auth.py. - Imprecise log wording in
exotel.pyabout rejection when the token is unset.
Decision
None of the newly raised issues meet the strict <blocking-criteria> (no hardcoded secrets, no SQL injection, no auth bypass/cross-tenant access, no SSRF, no command/template injection, no PII exposure, no migration edits). However, the CRITICAL secret-in-URL concern and the MAJOR signature-correctness issues should be addressed or explicitly accepted before this ships to production.
Recommended next steps:
- Redact
auth_tokenfrom Exotel payload/logging and document the token-in-URL trade-off. - Normalize
providerto lowercase before callingverify_provider_webhookin callback handlers. - Verify Twilio signature reconstruction works with the actual form body/encoding and behind the production ingress path.
- Expand tests to cover Twilio and Plivo verifiers as suggested by the existing Copilot comment.
| is explicitly disabled. | ||
| """ | ||
| url = base_url.rstrip("/") + _EXOTEL_STATUS_CALLBACK_PATH | ||
| if not EXOTEL_WEBHOOK_AUTH_TOKEN: |
There was a problem hiding this comment.
🔒 CRITICAL: Embedding the raw EXOTEL_WEBHOOK_AUTH_TOKEN in the callback URL leaks the secret to Exotel logs, any proxy in path, and to anyone who can inspect the registered applet/dashboard config. A shared secret should be a bearer presented in a header, not a query param. Since Exotel cannot sign, consider rotating to a per-call or per-URL nonce derived via HMAC from the token + call SID, or accept that the token is single-purpose and treat it as low-entropy. At minimum, do not log this URL anywhere (the logger.info(f"Payload: {payload}") below prints it). If you must keep this design, redact auth_token from all logs.
Suggested fix: build a helper redact_query_param(url, "auth_token") and log only the redacted URL; also document that EXOTEL_WEBHOOK_AUTH_TOKEN is effectively exposed to Exotel.
| ) -> bool: | ||
| """Verify a Plivo V3 (preferred) or V2 signature. Fails closed if unset.""" | ||
| if not PLIVO_AUTH_TOKEN: | ||
| return False |
There was a problem hiding this comment.
💬 SUGGESTION: Plivo V2 signature validation uses validate_signature(url, v2_nonce, v2_sig, PLIVO_AUTH_TOKEN). Per Plivo docs, V2 validation requires the URI without query string and the POST body for POST requests. Passing the full URL with query string may fail validation for GET requests or mixed-method setups. Verify the SDK helper accepts the full URL; if not, split the URL at ? for V2 as you do for V3.
| """Verify an X-Twilio-Signature header. Fails closed if the token is unset.""" | ||
| if not TWILIO_AUTH_TOKEN or not signature: | ||
| return False | ||
| try: |
There was a problem hiding this comment.
verify_twilio_signature passes dict(params) to RequestValidator.validate. Starlette UploadFile values in a form are not str; the earlier {k: str(v) for k, v in form.items()} converts them, but Twilio's validator expects the raw form bytes/string exactly as received. Converting uploaded files to "<UploadFile: ...>" will break signature validation for any POST that includes media, and more subtly changes encoding. Also, Twilio signs the URL for GET requests and url + sorted form params for POST; the url here already includes the query string, which is correct for GET but for POST the validator internally appends params, so passing the full URL is the documented pattern. However, you should pass the original form as a flat dict of strings without file coercion. Consider using request.body() once and handing bytes to the validator, or ensure no file fields are present in these webhooks.
Suggested fix: for Twilio POST, read await request.body() and let RequestValidator consume the raw body/params; do not coerce UploadFile objects.
| shared secret supplied as the ``auth_token`` query parameter. That token has | ||
| to be embedded in the URL we register with Exotel — it is the only channel | ||
| by which Exotel can present it. When no token is configured the URL is left | ||
| bare, which the verifier rejects (fail-closed) unless signature enforcement |
There was a problem hiding this comment.
💡 MINOR: The log says callbacks "will be rejected" unconditionally, but rejection only happens when ENFORCE_TELEPHONY_WEBHOOK_SIGNATURES is true. If the escape hatch is disabled, the bare URL is accepted. Rephrase to "will be rejected while enforcement is enabled" to match the actual behavior and avoid operator confusion during rollout.
| - Plivo: X-Plivo-Signature-V3/V2 HMAC verification against PLIVO_AUTH_TOKEN | ||
| Both go through the shared ``verify_provider_webhook`` (fail-closed if the | ||
| provider's secret is unset). | ||
| """ |
There was a problem hiding this comment.
💡 MINOR: HTTPException is imported but no longer used in this file after replacing the inline auth checks with verify_provider_webhook. Remove the unused import.
| ) | ||
| url = str(request.url).split("?", 1)[0] | ||
| query = request.url.query | ||
| return f"{url}?{query}" if query else url |
There was a problem hiding this comment.
reconstruct_public_url rebuilds the signed URL using APP_BASE_URL + request.url.path + request.url.query. If the application is served behind a path-rewriting proxy (e.g., /agent/voice/... is stripped or prefixed), the reconstructed path will not match what the provider signed, causing all legitimate webhooks to be rejected. Twilio/Plivo signatures are over the exact public URL including any path prefix. You need to either (1) make the public path prefix configurable (e.g., WEBHOOK_PATH_PREFIX), or (2) document that APP_BASE_URL must include the exact externally-visible path and no rewriting may occur. Without this, a common ingress configuration will break callbacks.
Suggested fix: add an optional WEBHOOK_PUBLIC_PATH_PREFIX env var and apply it: f"{base}{WEBHOOK_PUBLIC_PATH_PREFIX}{request.url.path}".
| @@ -61,6 +62,11 @@ async def handle_callback_details_get( | |||
| Raises: | |||
There was a problem hiding this comment.
verify_provider_webhook is called with the raw provider string before provider.lower() is applied. If a caller uses mixed-case path like /Twilio/..., the verifier will hit the unknown-provider branch and reject a legitimate webhook. Normalize the provider string before verification, or make verify_provider_webhook case-insensitive internally.
Suggested fix: pass provider.lower() to verify_provider_webhook, or normalize at the top of verify_provider_webhook before dispatch.
d03fd7c to
4e7ae8c
Compare
Addressed —
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
app/core/security/webhook_signature.py (1)
105-107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Loguru exception-aware logging.
Both handlers interpolate the exception object into the message string. Use
logger.opt(exception=exc)instead, so the traceback is captured and the exception text is never treated as format input.♻️ Proposed change
- except Exception as exc: # pragma: no cover - defensive - logger.warning(f"Twilio signature validation error: {exc}") - return False + except Exception as exc: # pragma: no cover - defensive + logger.opt(exception=exc).warning("Twilio signature validation error") + return False- except Exception as exc: # pragma: no cover - defensive - logger.warning(f"Plivo signature validation error: {exc}") - return False + except Exception as exc: # pragma: no cover - defensive + logger.opt(exception=exc).warning("Plivo signature validation error") + return FalseBased on learnings: "use Loguru's exception-aware logging in exception handlers, such as
logger.opt(exception=e).error(...)... do not interpolate exception objects directly into Loguru message strings".Also applies to: 139-141
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/core/security/webhook_signature.py` around lines 105 - 107, Update the exception handlers around the Twilio signature validation and the additional handler near the second referenced block to use Loguru’s exception-aware logging via logger.opt(exception=exc) before warning, passing a static message without interpolating the exception object. Preserve each handler’s existing return behavior.Source: Learnings
app/core/config/static.py (1)
469-478: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNormalize the leading slash too.
.rstrip("/")handles a trailing slash only. If an operator setsTELEPHONY_WEBHOOK_PATH_PREFIX=agent/voice,reconstruct_public_urlproduceshttps://hostagent/voice/..., and every Twilio and Plivo webhook fails verification with no obvious cause.♻️ Proposed normalization
-TELEPHONY_WEBHOOK_PATH_PREFIX = os.environ.get( - "TELEPHONY_WEBHOOK_PATH_PREFIX", "" -).rstrip("/") +_RAW_TELEPHONY_WEBHOOK_PATH_PREFIX = os.environ.get( + "TELEPHONY_WEBHOOK_PATH_PREFIX", "" +).strip().rstrip("/") +TELEPHONY_WEBHOOK_PATH_PREFIX = ( + f"/{_RAW_TELEPHONY_WEBHOOK_PATH_PREFIX.lstrip('/')}" + if _RAW_TELEPHONY_WEBHOOK_PATH_PREFIX + else "" +)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/core/config/static.py` around lines 469 - 478, Update the TELEPHONY_WEBHOOK_PATH_PREFIX normalization so it removes trailing slashes and ensures the configured prefix begins with exactly one leading slash, preserving the empty-string default. This must keep reconstruct_public_url generating valid paths for values such as agent/voice and /agent/voice.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 151-156: Update the TELEPHONY_WEBHOOK_PATH_PREFIX default in
.env.example to be an unquoted empty assignment, ensuring loaders pass an actual
empty value rather than literal quote characters.
In `@app/ai/voice/agents/breeze_buddy/services/telephony/exotel/exotel.py`:
- Around line 33-52: Document in _exotel_status_callback_url that it covers only
the status callback, and add rollout notes requiring the dashboard-registered
answer/voicebot URL and call-details GET URL to include the same ?auth_token=
value. In app/api/routers/breeze_buddy/telephony/answer/__init__.py lines 62-65,
confirm the configured Exotel answer URL carries auth_token before enabling
enforcement; in app/api/routers/breeze_buddy/telephony/callbacks/handlers.py
lines 65-68, confirm the call-details URL carries auth_token. No direct code
change is required at either sibling site if these dashboard configuration
confirmations are documented.
In `@app/core/security/webhook_signature.py`:
- Around line 87-91: Update verify_exotel_token to UTF-8 encode both the
provided token and EXOTEL_WEBHOOK_AUTH_TOKEN before passing them to
hmac.compare_digest, preserving the fail-closed behavior for an unset configured
token. Add a test covering a non-ASCII auth token and verify it returns False
rather than raising an exception.
---
Nitpick comments:
In `@app/core/config/static.py`:
- Around line 469-478: Update the TELEPHONY_WEBHOOK_PATH_PREFIX normalization so
it removes trailing slashes and ensures the configured prefix begins with
exactly one leading slash, preserving the empty-string default. This must keep
reconstruct_public_url generating valid paths for values such as agent/voice and
/agent/voice.
In `@app/core/security/webhook_signature.py`:
- Around line 105-107: Update the exception handlers around the Twilio signature
validation and the additional handler near the second referenced block to use
Loguru’s exception-aware logging via logger.opt(exception=exc) before warning,
passing a static message without interpolating the exception object. Preserve
each handler’s existing return behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9564c6d7-9f0a-4177-a78c-eaa933f30adb
📒 Files selected for processing (7)
.env.exampleapp/ai/voice/agents/breeze_buddy/services/telephony/exotel/exotel.pyapp/api/routers/breeze_buddy/telephony/answer/__init__.pyapp/api/routers/breeze_buddy/telephony/callbacks/handlers.pyapp/core/config/static.pyapp/core/security/webhook_signature.pytests/test_pentest_telephony_auth.py
| # Public path prefix an ingress strips before requests reach this app. Providers | ||
| # sign the externally-visible URL, so if https://host/agent/voice/... is proxied | ||
| # to /... internally, set this to "/agent/voice" or every legitimate webhook is | ||
| # rejected. Leave empty when no path rewriting happens. Not read from | ||
| # X-Forwarded-Prefix on purpose — that header is caller-controlled. | ||
| TELEPHONY_WEBHOOK_PATH_PREFIX="" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the quotes from the empty default.
Some loaders keep quote characters literally. Docker Compose env_file and set -a; source .env do not strip them. If the literal value "" reaches TELEPHONY_WEBHOOK_PATH_PREFIX, reconstruct_public_url builds https://host""/agent/... and every Twilio and Plivo webhook returns 401.
🔧 Proposed fix
-TELEPHONY_WEBHOOK_PATH_PREFIX=""
+TELEPHONY_WEBHOOK_PATH_PREFIX=📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Public path prefix an ingress strips before requests reach this app. Providers | |
| # sign the externally-visible URL, so if https://host/agent/voice/... is proxied | |
| # to /... internally, set this to "/agent/voice" or every legitimate webhook is | |
| # rejected. Leave empty when no path rewriting happens. Not read from | |
| # X-Forwarded-Prefix on purpose — that header is caller-controlled. | |
| TELEPHONY_WEBHOOK_PATH_PREFIX="" | |
| # Public path prefix an ingress strips before requests reach this app. Providers | |
| # sign the externally-visible URL, so if https://host/agent/voice/... is proxied | |
| # to /... internally, set this to "/agent/voice" or every legitimate webhook is | |
| # rejected. Leave empty when no path rewriting happens. Not read from | |
| # X-Forwarded-Prefix on purpose — that header is caller-controlled. | |
| TELEPHONY_WEBHOOK_PATH_PREFIX= |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 156-156: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example around lines 151 - 156, Update the
TELEPHONY_WEBHOOK_PATH_PREFIX default in .env.example to be an unquoted empty
assignment, ensuring loaders pass an actual empty value rather than literal
quote characters.
Source: Linters/SAST tools
| def _exotel_status_callback_url(base_url: str) -> str: | ||
| """Build the Exotel status-callback URL, carrying the shared auth token. | ||
|
|
||
| Exotel does not sign its webhooks, so ``verify_provider_webhook`` checks a | ||
| shared secret supplied as the ``auth_token`` query parameter. That token has | ||
| to be embedded in the URL we register with Exotel — it is the only channel | ||
| by which Exotel can present it. When no token is configured the URL is left | ||
| bare, which the verifier rejects (fail-closed) unless signature enforcement | ||
| is explicitly disabled. | ||
| """ | ||
| url = base_url.rstrip("/") + _EXOTEL_STATUS_CALLBACK_PATH | ||
| if not EXOTEL_WEBHOOK_AUTH_TOKEN: | ||
| logger.error( | ||
| "EXOTEL_WEBHOOK_AUTH_TOKEN is unset — Exotel status callbacks will be " | ||
| "rejected while ENFORCE_TELEPHONY_WEBHOOK_SIGNATURES is enabled (the " | ||
| "default). Set it (and keep it in sync with this deployment) or " | ||
| "Exotel outcome/retry handling will not run." | ||
| ) | ||
| return url | ||
| return f"{url}?auth_token={quote(EXOTEL_WEBHOOK_AUTH_TOKEN, safe='')}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Exotel auth_token coverage is incomplete across the three Exotel webhook entry points. This repository generates only the StatusCallback URL with auth_token, but enforcement now applies to all Exotel webhook routes. The Exotel answer URL and the call-details GET URL are registered in the Exotel applet dashboard, so they will return 401 unless the dashboard configuration is updated in the same rollout.
app/ai/voice/agents/breeze_buddy/services/telephony/exotel/exotel.py#L33-L52: document that_exotel_status_callback_urlcovers only the status callback, and record the required?auth_token=suffix for the dashboard-registered answer and call-details URLs in the rollout notes.app/api/routers/breeze_buddy/telephony/answer/__init__.py#L62-L65: confirm the Exotel applet answer/voicebot URL carriesauth_tokenbefore enablingENFORCE_TELEPHONY_WEBHOOK_SIGNATURES, otherwise every inbound Exotel call fails.app/api/routers/breeze_buddy/telephony/callbacks/handlers.py#L65-L68: confirm the Exotel call-details GET URL carriesauth_token, otherwise recording updates stop.
📍 Affects 3 files
app/ai/voice/agents/breeze_buddy/services/telephony/exotel/exotel.py#L33-L52(this comment)app/api/routers/breeze_buddy/telephony/answer/__init__.py#L62-L65app/api/routers/breeze_buddy/telephony/callbacks/handlers.py#L65-L68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ai/voice/agents/breeze_buddy/services/telephony/exotel/exotel.py` around
lines 33 - 52, Document in _exotel_status_callback_url that it covers only the
status callback, and add rollout notes requiring the dashboard-registered
answer/voicebot URL and call-details GET URL to include the same ?auth_token=
value. In app/api/routers/breeze_buddy/telephony/answer/__init__.py lines 62-65,
confirm the configured Exotel answer URL carries auth_token before enabling
enforcement; in app/api/routers/breeze_buddy/telephony/callbacks/handlers.py
lines 65-68, confirm the call-details URL carries auth_token. No direct code
change is required at either sibling site if these dashboard configuration
confirmations are documented.
| def verify_exotel_token(token: Optional[str]) -> bool: | ||
| """Constant-time compare of the Exotel webhook token. Fails closed if unset.""" | ||
| if not EXOTEL_WEBHOOK_AUTH_TOKEN: | ||
| return False | ||
| return hmac.compare_digest(token or "", EXOTEL_WEBHOOK_AUTH_TOKEN) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Python hmac.compare_digest TypeError non-ASCII str arguments
💡 Result:
The hmac.compare_digest function in Python is designed to perform constant-time comparisons to mitigate timing attacks during cryptographic verification [1][2]. According to the official Python documentation, when using string (str) arguments, the function specifically requires that they be ASCII-only [1][2]. If you provide a string containing non-ASCII characters, the function will raise a TypeError [3][4]. This restriction exists because the function is primarily intended to compare digests (such as those produced by HMAC.hexdigest), which are inherently ASCII-based [2][4]. To resolve this issue, you must ensure that the arguments passed to hmac.compare_digest are either: 1. Bytes-like objects (recommended for cryptographic operations) [1][2]. 2. ASCII-only strings [1][2]. If you are dealing with data that contains non-ASCII characters, you should encode the data into bytes (e.g., using.encode('utf-8')) before passing it to the function [4]. This approach ensures compatibility with the security-sensitive nature of the comparison while adhering to the function's requirements [4].
Citations:
- 1: https://docs.python.org/3/library/hmac.html
- 2: https://github.com/python/cpython/blob/3.11/Doc/library/hmac.rst
- 3: TypeError: comparing strings with non-ASCII characters is not supported pyauth/pyotp#22
- 4: PEP 466: backport hmac.compare_digest python/cpython#65505
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- relevant files ---'
git ls-files 'app/core/security/webhook_signature.py' 'app/core/config/static.py' 'tests/test_pentest_telephony_auth.py'
printf '%s\n' '--- webhook module outline ---'
ast-grep outline app/core/security/webhook_signature.py
printf '%s\n' '--- webhook implementation ---'
cat -n app/core/security/webhook_signature.py
printf '%s\n' '--- related symbols and route callers ---'
rg -n -C 4 'verify_exotel_token|verify_provider_webhook|EXOTEL_WEBHOOK_AUTH_TOKEN|auth_token' app tests
printf '%s\n' '--- relevant configuration ---'
rg -n -C 3 'EXOTEL_WEBHOOK_AUTH_TOKEN' app/core/config/static.pyRepository: juspay/clairvoyance
Length of output: 39634
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import hmac
from urllib.parse import parse_qsl
secret = "s3cret"
queries = [
"?auth_token=s3cret",
"?auth_token=wrong",
"?auth_token=%C3%A9",
"?auth_token=%E2%82%AC",
]
for query in queries:
token = dict(parse_qsl(query.lstrip("?"), keep_blank_values=True)).get("auth_token")
try:
result = hmac.compare_digest(token or "", secret)
outcome = f"returns {result!r}"
except Exception as exc:
outcome = f"raises {type(exc).__name__}: {exc}"
print(f"{query} -> token={token!r}; {outcome}")
print("bytes comparison:")
for token in ["é", "€", "s3cret"]:
print(
token,
hmac.compare_digest(token.encode("utf-8"), secret.encode("utf-8")),
)
PYRepository: juspay/clairvoyance
Length of output: 531
Use byte operands for the constant-time comparison. A non-ASCII auth_token causes hmac.compare_digest to raise TypeError, so the uncaught exception returns 500 instead of 401. Encode both operands as UTF-8 and add a test for this case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/core/security/webhook_signature.py` around lines 87 - 91, Update
verify_exotel_token to UTF-8 encode both the provided token and
EXOTEL_WEBHOOK_AUTH_TOKEN before passing them to hmac.compare_digest, preserving
the fail-closed behavior for an unset configured token. Add a test covering a
non-ASCII auth token and verify it returns False rather than raising an
exception.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed all 7 changed files. The PR correctly centralizes webhook signature verification and switches Exotel to a constant-time compare, which addresses PT-05/PT-23. No existing migration files were modified, and no hardcoded secrets were introduced.
New issues raised
- 1 CRITICAL —
EXOTEL_WEBHOOK_AUTH_TOKENis embedded in the callback URL, exposing the secret to Exotel/proxy logs and the dashboard; the payload is also logged unredacted. - 3 MAJOR — path-rewriting proxy can break signature reconstruction; Twilio POST signature validation coerces
UploadFile/form values; callback handlers pass raw mixed-caseproviderto the verifier before normalization. - 2 MINOR — misleading "will be rejected" log wording; unused
HTTPExceptionimport. - 1 SUGGESTION — confirm Plivo V2 signature helper expects full URL vs. URI-only.
Existing comments noted
Two Copilot review comments were already present and are not duplicated here:
- Test coverage gap for Twilio/Plivo verifiers in
tests/test_pentest_telephony_auth.py. - Imprecise log wording in
exotel.pyabout rejection when the token is unset.
Decision
None of the newly raised issues meet the strict <blocking-criteria> (no hardcoded secrets, no SQL injection, no auth bypass/cross-tenant access, no SSRF, no command/template injection, no PII exposure, no migration edits). However, the CRITICAL secret-in-URL concern and the MAJOR signature-correctness issues should be addressed or explicitly accepted before this ships to production.
Recommended next steps:
- Redact
auth_tokenfrom Exotel payload/logging and document the token-in-URL trade-off. - Normalize
providerto lowercase before callingverify_provider_webhookin callback handlers. - Verify Twilio signature reconstruction works with the actual form body/encoding and behind the production ingress path.
- Expand tests to cover Twilio and Plivo verifiers as suggested by the existing Copilot comment.
4e7ae8c to
5fc76f7
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Re-reviewed all 7 changed files at 5fc76f7. No new blocking issues were identified in this pass.
New issues raised this run
- 0 (none)
Existing comments noted (not duplicated)
Prior reviews already raised the following points; they remain visible on the PR and should be tracked to resolution before production:
- CRITICAL —
EXOTEL_WEBHOOK_AUTH_TOKENembedded in the Exotel status-callback URL leaks the secret to Exotel/proxy logs and dashboard config. - MAJOR —
TELEPHONY_WEBHOOK_PATH_PREFIX/APP_BASE_URLmismatch can break signature reconstruction behind path-rewriting ingress. - MAJOR (now addressed in code) — Twilio/Plivo form handling previously coerced
UploadFilevalues; current code filters tostrfields only. - MAJOR (now addressed in code) —
verify_provider_webhooknormalizesproviderto lowercase internally, so mixed-case path segments no longer fall through to unknown-provider rejection. - CodeRabbit items: quoted empty default in
.env.example,hmac.compare_digestnon-ASCII operand handling, and Exotelauth_tokencoverage across all dashboard-registered URLs.
Decision
No new issues meet the <blocking-criteria> (no hardcoded secrets, no SQL injection, no auth bypass/cross-tenant access, no SSRF, no command/template injection, no PII exposure, no migration edits). Approving so the pentest fix can land; please resolve the outstanding CRITICAL secret-in-URL concern and MAJOR ingress-path concern before enabling enforcement in production.
…5/23) Independent of the other pentest PRs — targets release, merges in any order. The callback and answer routes accepted any caller: anyone who knew a URL could drive call state. Verified on release — unsigned POSTs to the twilio, plivo and exotel status callbacks are all accepted. verify_provider_webhook (new) validates Twilio X-Twilio-Signature, Plivo V3 signatures and the Exotel auth token, and is applied to every callback and answer route. Token comparison is constant-time and fails CLOSED when the expected token is unset, so a missing env var denies rather than admits. Enforcement is on by default via ENFORCE_TELEPHONY_WEBHOOK_SIGNATURES. It exists to avoid a hard outage during rollout, not as a supported posture. PT-12 (recording host pinning) is NOT here: its fix is built on the SSRF egress guard, so it ships in that PR instead of forcing a dependency between the two. 932 tests pass on this branch.
5fc76f7 to
72fbb2a
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Re-reviewed the 7 changed files at HEAD (72fbb2a). No new blocking issues were identified in this pass.
New issues raised this run
- 0 (none)
Existing comments noted (not duplicated)
Prior reviews already raised the following points; they remain visible on the PR and should be tracked to resolution before production:
- CRITICAL —
EXOTEL_WEBHOOK_AUTH_TOKENembedded in the Exotel status-callback URL leaks the secret to Exotel/proxy logs and dashboard config. - MAJOR —
TELEPHONY_WEBHOOK_PATH_PREFIX/APP_BASE_URLmismatch can break signature reconstruction behind path-rewriting ingress. - MAJOR (addressed in code) —
verify_provider_webhooknormalizesproviderto lowercase internally, so mixed-case path segments no longer fall through to unknown-provider rejection. - CodeRabbit items: quoted empty default in
.env.example,hmac.compare_digestnon-ASCII operand handling, and Exotelauth_tokencoverage across all dashboard-registered URLs.
Decision
No new issues meet the <blocking-criteria> (no hardcoded secrets, no SQL injection, no auth bypass/cross-tenant access, no SSRF, no command/template injection, no PII exposure, no migration edits). Approving so the pentest fix can land; please resolve the outstanding CRITICAL secret-in-URL concern and MAJOR ingress-path concern before enabling enforcement in production.
Independent of the other pentest PRs — targets release, merges in any order.
The callback and answer routes accepted any caller: anyone who knew a URL could
drive call state. Verified on release — unsigned POSTs to the twilio, plivo and
exotel status callbacks are all accepted.
verify_provider_webhook (new) validates Twilio X-Twilio-Signature, Plivo V3
signatures and the Exotel auth token, and is applied to every callback and
answer route. Token comparison is constant-time and fails CLOSED when the
expected token is unset, so a missing env var denies rather than admits.
Enforcement is on by default via ENFORCE_TELEPHONY_WEBHOOK_SIGNATURES. It exists
to avoid a hard outage during rollout, not as a supported posture.
PT-12 (recording host pinning) is NOT here: its fix is built on the SSRF egress
guard, so it ships in that PR instead of forcing a dependency between the two.
932 tests pass on this branch.
Independent by construction
This PR targets
releasedirectly. It shares no file with any other pentest PR, and all five were trial-merged pairwise — 10 of 10 pairs merge with no conflict, so they can land in any order.The four config blocks that previously forced a chain (
static.py,.env.example) now sit at four distinct anchors hundreds of lines apart, each next to the settings it belongs with, so independent branches auto-merge instead of colliding.Merging all five reproduces the original #930 tree —
static.pyis identical content in a different order,.env.examplediffers only by one now-meaningless banner comment — and the combined suite runs 992 passed.Evidence recording is in the comment below.
Summary by CodeRabbit
New Features
Bug Fixes
Tests