fix(security): remediate Breeze Buddy pentest findings (BB-DEEPDIVE-2026-001) - #930
fix(security): remediate Breeze Buddy pentest findings (BB-DEEPDIVE-2026-001)#930murdore wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a broad set of backend security remediations for Breeze Buddy based on pentest BB-DEEPDIVE-2026-001, introducing shared security modules (SSRF egress guard, webhook signature verification, token revocation, password policy) and wiring them through RBAC, webhook, telephony, MCP, and template execution paths.
Changes:
- Adds shared security primitives (SSRF egress validation + redirect revalidation, telephony webhook verification, JWT revocation denylist, password policy enforcement).
- Tightens authorization and scope handling across templates/configurations/leads/chat/numbers and fixes “fail-open” scope resolution behavior to fail-closed.
- Adds targeted tests covering SSRF, sandboxed custom python, RBAC/scope fixes, and updates existing MCP approval tests to bypass SSRF during non-network logic.
Reviewed changes
Copilot reviewed 49 out of 50 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_ssrf_egress.py | Adds coverage for SSRF egress URL validation and allowlist host matching. |
| tests/test_pentest_authz.py | Adds regression tests for authorization/scope/token/password fixes from the pentest. |
| tests/test_mcp_approval.py | Updates MCP approval tests to bypass SSRF validation for placeholder hosts. |
| tests/test_custom_python_sandbox.py | Adds tests proving RestrictedPython sandboxing + kill-switch behavior for custom python_code. |
| pyproject.toml | Adds restrictedpython dependency for sandbox compilation. |
| app/schemas/breeze_buddy/users.py | Enforces shared password policy on user create/update schemas. |
| app/schemas/breeze_buddy/signup.py | Enforces password policy in signup schema and requires password for email-based account listing. |
| app/schemas/breeze_buddy/auth.py | Caps S2S token lifetime to 365 days. |
| app/database/accessor/breeze_buddy/users.py | Adds short-lived Redis cache for per-request user liveness checks + invalidation hooks. |
| app/core/security/webhook_signature.py | New shared provider webhook authentication (Twilio/Plivo signatures, Exotel token). |
| app/core/security/token_revocation.py | New Redis-backed JWT revocation denylist keyed by SHA-256 of the token. |
| app/core/security/ssrf.py | New shared SSRF egress guard + redirect-safe aiohttp request helper. |
| app/core/security/scope.py | Changes wildcard scope resolution to fail closed on broken owner chains. |
| app/core/security/password.py | Enforces bcrypt 72-byte limit as a hard failure (UTF-8 byte length). |
| app/core/security/password_policy.py | New centralized password strength policy (length/diversity/common/identifier denylist). |
| app/core/security/authorization.py | Introduces merchant_scope_permitted helper to correctly handle null-merchant reseller-scoped rows. |
| app/core/config/static.py | Adds feature flags for telephony webhook enforcement and custom python enablement. |
| app/api/security/breeze_buddy/rbac_token.py | Makes token verification async; adds revocation + user liveness enforcement. |
| app/api/routers/feature_flags/rbac.py | Awaits async RBAC token verification. |
| app/api/routers/breeze_buddy/widget_common.py | Adds cross-IP aggregate rate limiting per public_widget_key. |
| app/api/routers/breeze_buddy/webhooks/woocommerce/services.py | Awaits async RBAC token verification for webhook token validation. |
| app/api/routers/breeze_buddy/webhooks/breeze/services.py | Awaits async RBAC token verification for webhook token validation. |
| app/api/routers/breeze_buddy/users/handlers.py | Validates merchant-created users’ reseller_ids against creator scope (prevents escalation). |
| app/api/routers/breeze_buddy/templates/rbac.py | Enforces merchant scope (including null merchant reseller-scoped templates) via shared helper. |
| app/api/routers/breeze_buddy/templates/handlers.py | Revalidates authz on any (reseller, merchant) ownership move. |
| app/api/routers/breeze_buddy/templates/init.py | Switches template-create authorization to validate_template_access (reseller + merchant scope). |
| app/api/routers/breeze_buddy/telephony/callbacks/handlers.py | Adds provider webhook authentication across callback endpoints. |
| app/api/routers/breeze_buddy/telephony/answer/init.py | Adds provider webhook authentication for answer webhooks (Exotel/Plivo). |
| app/api/routers/breeze_buddy/signup/handlers.py | Requires password proof for email-based account listing; returns generic 401 to reduce enumeration. |
| app/api/routers/breeze_buddy/signup/init.py | Plumbs password through to account listing handler. |
| app/api/routers/breeze_buddy/numbers/rbac.py | Adds ownership validation and real RBAC filtering for numbers. |
| app/api/routers/breeze_buddy/numbers/handlers.py | Enforces number ownership checks on read-by-id (prevents IDOR). |
| app/api/routers/breeze_buddy/leads/rbac.py | Enforces correct handling of null merchant_id (reseller-scoped) leads/recordings. |
| app/api/routers/breeze_buddy/configurations/rbac.py | Enforces correct handling of null merchant_id (reseller-scoped) configurations. |
| app/api/routers/breeze_buddy/chat/rbac.py | Enforces correct handling of null merchant_id (reseller-scoped) chat sessions. |
| app/api/routers/breeze_buddy/chat/demo.py | Switches demo IP extraction to shared client_ip implementation (trusted last XFF hop). |
| app/api/routers/breeze_buddy/auth/handlers.py | Implements server-side logout by revoking the presented JWT. |
| app/api/routers/breeze_buddy/auth/init.py | Requires auth header on logout and passes token to revocation logout handler. |
| app/ai/voice/agents/breeze_buddy/utils/parser.py | Replaces AST denylist with RestrictedPython compilation/execution. |
| app/ai/voice/agents/breeze_buddy/utils/common.py | Adds SSRF validation + redirect-safe posting for outbound webhooks. |
| app/ai/voice/agents/breeze_buddy/template/global_function.py | Gates custom python global functions behind ENABLE_CUSTOM_PYTHON_FUNCTIONS. |
| app/ai/voice/agents/breeze_buddy/services/telephony/twilio/recording.py | Uses SSRF-safe request + host allowlist before sending Twilio credentials. |
| app/ai/voice/agents/breeze_buddy/services/telephony/plivo/recording.py | Uses SSRF-safe request + host allowlist before sending Plivo credentials. |
| app/ai/voice/agents/breeze_buddy/services/telephony/exotel/recording.py | Uses SSRF-safe request + host allowlist before sending Exotel credentials. |
| app/ai/voice/agents/breeze_buddy/mcp/init.py | Adds SSRF egress validation for MCP server URL before building credential headers. |
| app/ai/voice/agents/breeze_buddy/handlers/transport/http_requester.py | Replaces bespoke URL checks with shared SSRF guard + redirect revalidation. |
| app/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.py | Blocks LLM-sourced fields from being used in URL host position. |
| app/ai/voice/agents/breeze_buddy/agent/inbound.py | Scopes template selection by dialed number to break unauth WebSocket template-id abuse chain. |
| .env.example | Documents new security flags and SSRF dev escape hatch. |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 50 changed files (focused on security-critical additions and modifications).
New issues raised this run: 2
⚠️ MAJOR × 1:app/core/security/ssrf.pyreadsSSRF_ALLOW_PRIVATE_EGRESSdirectly viaos.getenvat import time instead of centralizing the env read inapp/core/config/static.py(project convention). Suggested moving the flag tostatic.pyand importing it.- 💡 MINOR × 1:
app/core/security/webhook_signature.pyreconstruct_public_url()appends the raw query string; this overlaps with the existing copilot review comment and should be aligned with provider library expectations.
Existing comments respected: The copilot reviewer's points on webhook_signature.py (query-string inclusion and missing query params in signature verification) were already raised; I did not duplicate them.
Blocking criteria check: No new hardcoded secrets, SQL injections, auth bypasses, SSRF/PII exposures, or migration edits were introduced. The MAJOR issue is a configuration-discipline concern, not a blocking security vulnerability.
Decision: Approve. The pentest remediation is comprehensive and the security fixes (SSRF guard, webhook signature verification, RBAC hardening, token revocation, password policy, custom-python kill switch) are well-scoped. Please address the configuration-centralization comment in a follow-up.
a5708a7 to
dd7c6b6
Compare
|
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:
WalkthroughThis PR adds Breeze Buddy security hardening across SSRF-protected outbound requests, telephony webhook verification, JWT revocation, RBAC scope checks, password validation, custom Python sandboxing, account verification, and aggregate widget rate limiting. ChangesSecurity hardening
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant WebhookRouter
participant WebhookVerifier
participant ProviderHandler
Caller->>WebhookRouter: submit telephony webhook
WebhookRouter->>WebhookVerifier: verify provider signature
WebhookVerifier-->>WebhookRouter: verification result
WebhookRouter->>ProviderHandler: dispatch verified request
ProviderHandler-->>Caller: response or fallback TwiML
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/routers/breeze_buddy/auth/__init__.py (1)
158-193: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring now contradicts the implemented behavior. The body still asserts the backend cannot invalidate the token and that the "Token remains valid until expiration but client discards it," which is stale now that logout revokes the token server-side via the denylist. Trim the client-only narrative so it matches the revocation semantics captured in the updated Note.
🤖 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/api/routers/breeze_buddy/auth/__init__.py` around lines 158 - 193, Update the logout endpoint docstring above logout_handler to remove the stale client-only claims that the backend cannot invalidate tokens and that tokens remain valid until expiration. Retain concise client cleanup guidance and document that logout adds the token to the server-side revocation denylist, preventing further use before expiry.
🧹 Nitpick comments (1)
tests/test_custom_python_sandbox.py (1)
37-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a handler test that mutates state to guard against the missing RestrictedPython guards.
test_legitimate_handler_compiles_and_runsonly returns a dict literal and usessum(), so it passes even without_write_/_inplacevar_/_print_. A handler doing e.g.out = {}; out['k'] = 1; total = 0; total += args['n']would surface the missing guards flagged inapp/ai/voice/agents/breeze_buddy/utils/parser.py. Adding such a case locks in correct runtime behavior once the guards are added.🤖 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 `@tests/test_custom_python_sandbox.py` around lines 37 - 46, Add a state-mutating handler case to test_legitimate_handler_compiles_and_runs, including dictionary item assignment and augmented assignment, then assert compile_custom_function executes it successfully with the expected mutated values. Ensure the test exercises the RestrictedPython _write_, _inplacevar_, and _print_ guard requirements without changing the existing legitimate-handler coverage.
🤖 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 `@app/ai/voice/agents/breeze_buddy/agent/inbound.py`:
- Around line 168-184: Update the inbound flow authorization in
handle_inbound_call so a missing to_number fails closed before the template
association check, matching the existing missing-number behavior. Ensure
template flow construction cannot proceed when all to/to_number sources are
absent, while preserving the current outbound-number authorization for present
numbers.
In `@app/ai/voice/agents/breeze_buddy/utils/parser.py`:
- Around line 83-93: Update _build_restricted_namespace() to inject the missing
RestrictedPython handlers _write_, _inplacevar_, and _print_ alongside the
existing guarded hooks. Reuse the project’s established restricted handler
implementations so assignments, augmented assignments, and print output execute
successfully.
In `@app/api/routers/breeze_buddy/leads/rbac.py`:
- Around line 106-108: Remove the unnecessary f-string prefix from the
`HTTPException` detail in the lead-not-found branch, keeping the existing `"Lead
not found"` message and 404 status unchanged.
In `@app/api/routers/breeze_buddy/signup/handlers.py`:
- Around line 426-440: Update the password verification flow in the
email/password branch to always perform at least one bcrypt verification, using
the established dummy hash when users is empty or a user’s password_hash is
missing. Keep active-user matching and the generic 401 response unchanged, and
ensure SSO users without passwords cannot trigger a 500.
In `@app/core/security/ssrf.py`:
- Around line 127-135: Restructure the IP-literal handling in the validation
flow so only ipaddress.ip_address(hostname) is caught for ValueError; perform
ip_block_reason(hostname), raise SSRFError when blocked, and return the literal
outside that try/except. Keep non-literal hosts falling through to
_resolve_host(hostname).
---
Outside diff comments:
In `@app/api/routers/breeze_buddy/auth/__init__.py`:
- Around line 158-193: Update the logout endpoint docstring above logout_handler
to remove the stale client-only claims that the backend cannot invalidate tokens
and that tokens remain valid until expiration. Retain concise client cleanup
guidance and document that logout adds the token to the server-side revocation
denylist, preventing further use before expiry.
---
Nitpick comments:
In `@tests/test_custom_python_sandbox.py`:
- Around line 37-46: Add a state-mutating handler case to
test_legitimate_handler_compiles_and_runs, including dictionary item assignment
and augmented assignment, then assert compile_custom_function executes it
successfully with the expected mutated values. Ensure the test exercises the
RestrictedPython _write_, _inplacevar_, and _print_ guard requirements without
changing the existing legitimate-handler coverage.
🪄 Autofix (Beta)
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
Run ID: 139558ee-779d-4388-86e4-8e45884c1420
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (49)
.env.exampleapp/ai/voice/agents/breeze_buddy/agent/inbound.pyapp/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.pyapp/ai/voice/agents/breeze_buddy/handlers/transport/http_requester.pyapp/ai/voice/agents/breeze_buddy/mcp/__init__.pyapp/ai/voice/agents/breeze_buddy/services/telephony/exotel/recording.pyapp/ai/voice/agents/breeze_buddy/services/telephony/plivo/recording.pyapp/ai/voice/agents/breeze_buddy/services/telephony/twilio/recording.pyapp/ai/voice/agents/breeze_buddy/template/global_function.pyapp/ai/voice/agents/breeze_buddy/utils/common.pyapp/ai/voice/agents/breeze_buddy/utils/parser.pyapp/api/routers/breeze_buddy/auth/__init__.pyapp/api/routers/breeze_buddy/auth/handlers.pyapp/api/routers/breeze_buddy/chat/demo.pyapp/api/routers/breeze_buddy/chat/rbac.pyapp/api/routers/breeze_buddy/configurations/rbac.pyapp/api/routers/breeze_buddy/leads/rbac.pyapp/api/routers/breeze_buddy/numbers/handlers.pyapp/api/routers/breeze_buddy/numbers/rbac.pyapp/api/routers/breeze_buddy/signup/__init__.pyapp/api/routers/breeze_buddy/signup/handlers.pyapp/api/routers/breeze_buddy/telephony/answer/__init__.pyapp/api/routers/breeze_buddy/telephony/callbacks/handlers.pyapp/api/routers/breeze_buddy/templates/__init__.pyapp/api/routers/breeze_buddy/templates/handlers.pyapp/api/routers/breeze_buddy/templates/rbac.pyapp/api/routers/breeze_buddy/users/handlers.pyapp/api/routers/breeze_buddy/webhooks/breeze/services.pyapp/api/routers/breeze_buddy/webhooks/woocommerce/services.pyapp/api/routers/breeze_buddy/widget_common.pyapp/api/routers/feature_flags/rbac.pyapp/api/security/breeze_buddy/rbac_token.pyapp/core/config/static.pyapp/core/security/authorization.pyapp/core/security/password.pyapp/core/security/password_policy.pyapp/core/security/scope.pyapp/core/security/ssrf.pyapp/core/security/token_revocation.pyapp/core/security/webhook_signature.pyapp/database/accessor/breeze_buddy/users.pyapp/schemas/breeze_buddy/auth.pyapp/schemas/breeze_buddy/signup.pyapp/schemas/breeze_buddy/users.pypyproject.tomltests/test_custom_python_sandbox.pytests/test_mcp_approval.pytests/test_pentest_authz.pytests/test_ssrf_egress.py
dd7c6b6 to
492d5b4
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 50 changed files (security, authz, telephony, MCP, widget, schemas, tests, config).
New issues raised this run: 0 blocking, 0 major, 0 minor, 0 suggestions.
Existing comments noted: Several points were already raised by prior reviewers (Copilot/CodeRabbit/Tara-ag) and addressed by the author in 492d5b4, including:
- Fail-closed PT-02 guard when
to_numberis absent. - Missing RestrictedPython hooks (
_write_,_inplacevar_,_print_). - Constant-time bcrypt work for account-list anti-enumeration.
- SSRF IP-literal short-circuit fix.
- Redundant f-string cleanup.
Blocking criteria assessment: No new blocking criteria are met. No hardcoded secrets, no SQL injection, no authz bypass, no SSRF bypass, no PII exposure, and no existing migration files were modified.
Key observations (non-blocking):
- The async refactor of
verify_rbac_tokenand_build_server_paramsis consistently awaited at all call sites in this PR. - New SSRF guard (
app/core/security/ssrf.py) is sound: resolves DNS before validating, blocks internal/metadata, re-validates redirect hops, and strips credentials off allow-list redirects. - Webhook signature module fails closed when provider secrets are unset and uses constant-time comparison for Exotel.
- JWT revocation + liveness recheck covers logout and disabled/deleted accounts.
- RBAC fixes close the null-merchant skip, template cross-merchant planting, numbers IDOR, and scope fail-open issues.
Recommendation: Approve. The PR addresses the backend pentest findings comprehensively and includes regression tests. Operational rollout notes about webhook signature enforcement and the custom-python kill switch should be followed closely.
492d5b4 to
f4407f2
Compare
PR #930 —
|
f4407f2 to
2ec67bc
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 50 changed files (security, authz, telephony, MCP, widget, schemas, tests, config).
New issues raised this run: 0 blocking, 0 major, 0 minor, 0 suggestions.
Blocking criteria assessment: No hardcoded secrets, no SQL injection, no authz bypass, no SSRF bypass, no PII exposure, and no existing migration files were modified.
Assessment:
- The pentest remediation is comprehensive and addresses the 22 backend findings with appropriate shared security primitives (SSRF egress guard, webhook signature verification, JWT revocation denylist, password policy).
- RBAC fixes correctly close the null-merchant skip, template cross-merchant planting, numbers IDOR, and scope fail-open issues.
- Async refactor of
verify_rbac_tokenand_build_server_paramsis consistently awaited at all call sites in this PR. - New tests cover SSRF, sandboxed custom python, authorization/scope/token/password fixes.
- Operational rollout notes about webhook signature enforcement and the custom-python kill switch should be followed closely.
Decision: Approve.
2ec67bc to
7817a1a
Compare
Review round addressed — force-pushed to
|
|
@narsimhaReddyJuspay @Tara-ag @swaroopvarma1 — this one is ready for a final pass and merge. 🙏
Could one of you do the final review and merge (admin-override the Yama check)? Happy to walk through any thread. |
7817a1a to
1641bba
Compare
Review summaryFiles reviewed: 50 changed files (security, authz, telephony, MCP, widget, schemas, tests, config). New issues raised this run: 0 blocking, 0 major, 0 minor, 0 suggestions. Existing comments respected: All prior review threads from Copilot, CodeRabbit, Tara-ag, narsimhaReddyJuspay, and swaroopvarma1 were reviewed. Issues already raised and addressed by the author (fail-closed PT-02 guard, RestrictedPython hooks, constant-time bcrypt, SSRF IP-literal short-circuit, configuration centralization, webhook signature query-string handling, test gaps, write-path RBAC hardening, rate-limit key hashing/XFF) were not duplicated. Blocking criteria assessment: No new blocking criteria are met. No hardcoded secrets, no SQL injection, no authz bypass, no SSRF bypass, no PII exposure, and no existing migration files were modified. Key observations:
Decision: Approve. The PR addresses the backend pentest findings comprehensively and includes targeted regression tests. Operational rollout notes about webhook signature enforcement and the custom-python kill switch should be followed closely. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 54 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
app/api/routers/breeze_buddy/numbers/handlers.py:266
get_telephony_numberis documented to return 404 for out-of-scope numbers (and it already appliesfilter_numbers_by_rbacwith template-pinned IDs). Callingvalidate_number_accesshere can return 403 before the router’s 404 logic runs, leaking that the number exists and breaking the “owned or template-pinned” visibility rule.
validate_number_access(
current_user,
number.reseller_id,
number.merchant_id,
operation="access outbound number",
1641bba to
90eaf97
Compare
|
@swaroopvarma1 — this one is verified and ready to merge; requesting your review/merge. Full proof is in this thread: written verification plus a recorded run. Summary of the wider pass across the Breeze Buddy PR set — 86/86 controls held against a live stack (Postgres + Redis, real JWT logins, real browser):
All five are |
d1767d5 to
ab8093a
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Files reviewed: 55 changed files (focused on security-critical additions and modifications).
New issues raised this run: 6
⚠️ MAJOR × 4app/core/security/ssrf.py:ssrf_safe_requeststripsauthon off-allow-list/host-changing redirects but does not strip header-based credentials (Authorization,Cookie), so bearer tokens / session cookies can leak to attacker-controlled redirect targets.app/api/routers/breeze_buddy/signup/handlers.py: PT-16 account-list timing oracle still leaks the number of active accounts per email (N password checks vs. one dummy check). Pad to a fixed cap.app/api/routers/breeze_buddy/signup/handlers.py:select_account_handlercan 500 on empty password or SSO-only account with nopassword_hashbecauseverify_passwordraisesValueError.app/api/routers/breeze_buddy/auth/handlers.py:login_handlerandgenerate_s2s_token_handlercan 500 on empty password because the Pydantic schemas do not enforcemin_length=1.
- 💡 MINOR × 2
app/api/security/breeze_buddy/rbac_token.py: PT-22 liveness recheck is cached for 30 seconds, so disabled/deleted accounts' tokens remain usable for up to 30 seconds — clarify or shorten TTL.app/api/routers/breeze_buddy/signup/handlers.py:google_login_handlerstill returns404 no_account, an account-enumeration oracle not covered by PT-16.
Existing comments respected: Points already raised by prior reviewers (Copilot/CodeRabbit/Tara-ag) and addressed by the author were not duplicated. This includes the PT-02 fail-closed guard, RestrictedPython hooks, SSRF IP-literal short-circuit, constant-time bcrypt work, and configuration centralization.
Blocking criteria assessment: The new MAJOR issues are security-adjacent but do not meet the explicit blocking criteria (no hardcoded secrets, no SQL injection, no authz bypass, no SSRF bypass, no PII exposure, no migration edits). However, the credential-leak and 500/empty-password issues are material enough that I am requesting changes before approval.
Next steps:
- Strip
Authorization/Cookieheaders inssrf_safe_requestwhen auth is stripped. - Cap/pad account-list password checks to a fixed number.
- Guard
select_account_handleragainst empty/missing passwords. - Add
min_length=1to password fields inLoginRequest/S2STokenRequest. - Optionally address the 30-second liveness cache and Google-login enumeration oracle.
manas-narra
left a comment
manas-narra
left a comment
There was a problem hiding this comment.
Argus review — see summary comment.
|
This PR is the remediation pass for the BB-DEEPDIVE-2026-001 pentest: it adds a shared SSRF egress guard ( 5 new issues found. |
ab8093a to
4b7ac3c
Compare
Review round addressed — pushed
|
| Finding | Fix |
|---|---|
| MAJOR · PT-16 leaks account count | list_accounts_handler now spends a constant 5 bcrypts (ACCOUNT_PASSWORD_CHECK_BUDGET), padding with the dummy hash |
| MAJOR · empty password → 500 | LoginRequest/S2STokenRequest/SelectAccountRequest passwords are min_length=1 → 422 at the boundary |
MAJOR · select_account_handler no-hash → 500 |
guards the missing-hash case, spends one dummy bcrypt, returns generic 401 |
| MAJOR · redirect leaks header creds | credential-dropping hops now reduce caller headers to a 5-entry allow-list (deny-list would miss X-Api-Key etc.) |
| MINOR · PT-22 "immediate" vs 30s cache | TTL 30s → 10s and the comment now states the real guarantee |
| MINOR · SSO enumeration | pushing back — see the thread |
@manas-narra
| Finding | Fix |
|---|---|
| Exotel callbacks would 401 in prod | callback URL now carries ?auth_token=…; unset token logs ERROR. This was the most serious one — thank you |
urlparse netloc empty for {base_url}/orders |
replaced with _authority_region(); verified against 6 template shapes |
| logout claims revocation that didn't happen | returns success: false + honest message when revoke_token fails |
reporting_webhook_url https-only regression |
not deliberate — allow_http=True restored, SSRF checks unchanged, plaintext logged |
validate_number_access has no callers |
deleted; its tests retargeted onto filter_numbers_by_rbac (the control the router runs), plus a new pin-aware case |
Two notes on my own verification
The PT-16 fix exposed a hole in my testing, not just the code: the old test asserted calls["n"] == 1 for the empty case, so it passed while the count-leak was live. It's replaced with an assertion that the bcrypt count is identical across 0/1/3/5/9 accounts — which fails against the old implementation.
Likewise the numbers tests asserted on a helper nothing called. Both were false confidence, and both are now pointed at the code that actually executes.
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Autonomous review summary
Scope: 56 changed files across the Breeze Buddy pentest remediation (BB-DEEPDIVE-2026-001).
Existing review state: 40 prior review threads loaded. The majority are resolved; the remaining unresolved thread (google_login_handler 404 for unknown Google emails) has a reasoned pushback from the author that I agree with — probing an email requires a Google-signed token for that email, so it is not a practical enumeration oracle.
New issues raised in this run: 0
Blocking criteria check:
- No hardcoded secrets/API keys/credentials in source.
- No string-interpolated / f-string / %-formatted asyncpg SQL observed in changed code.
- No existing
database/migrations/*.sqlfiles modified. - No new critical auth/authz bypass, SSRF, command/template injection, or PII exposure identified beyond what was already discussed and fixed in prior review rounds.
Assessment: The PR comprehensively remediates the backend pentest findings with:
- A shared SSRF egress guard (
app/core/security/ssrf.py) that resolves DNS and blocks internal/metadata/private addresses, plus per-hop redirect revalidation and credential-header stripping. - Provider webhook signature/token verification (
app/core/security/webhook_signature.py) for Twilio/Plivo/Exotel, fail-closed when secrets are unset. - RBAC hardening via
merchant_scope_permittedacross templates, configurations, leads, chat, and numbers. - Server-side JWT revocation denylist + per-request
is_activerecheck. - Constant-time bcrypt budget for account listing and login/S2S no-such-user paths.
- Per-IP and per-username rate limiting on credential endpoints.
- RestrictedPython compilation + default-OFF gating for custom
python_code. - Recording downloads pinned to provider hosts with
ssrf_safe_request.
All identified concerns from prior reviewers appear to have been addressed in the latest commit. No new blocking issues remain.
Decision: APPROVE.
Verification for this round — with a negative controlThe earlier suite reported "86/86 controls held" and still missed everything @Tara-ag and @manas-narra found. That was a testing failure as much as a code one, so this round adds a regression for each miss and checks each one actually fails against the pre-fix code ( 15/15 held against the live stack (real server, Postgres 16 + 43 migrations, Redis, real JWT logins).
Negative control — the same checks against
|
Proof of testing — recorded runGatesSecurity suites added by this PR
Negative control for the last review roundPT-16 was a bcrypt timing oracle leaking how many accounts share an email. The fix spends a constant budget regardless: The other fixes from that round are each present and covered: Live-stack attack-suite evidence (42/42 and 86/86 controls held against real Postgres + Redis, real JWT logins, genuine vs tampered Plivo signatures, cross-IP rate caps) is already attached to this PR from the earlier rounds. Status: APPROVED · 8/8 checks green · 1 commit. Two threads deliberately left open where I pushed back on the SSO-enumeration finding — happy to be overruled. |
Proof of testing — recorded end to endRecorded against this branch at p2-clairvoyance-930-pentest-backend.mp4What the run shows, in order:
On PT-16The pre-fix code ran one bcrypt verification per matching account, so response time leaked how many accounts share an email — an unauthenticated enumeration primitive. The fix spends a constant budget (
Pinned by The live-stack attack-suite videos from the earlier rounds are already attached to this PR (42/42 and 86/86 controls held against a running server). This recording is the current-HEAD re-verification plus the negative control for the newest fixes. @swaroopvarma1 ready for review + merge. |
Attack-side proof of every control, and one gap it foundFor a security PR, a green suite proves the tests pass, not that the controls hold. Each control below was exercised by running the attack it exists to stop against the shipped modules imported from this worktree, with an unsandboxed negative control alongside so the difference is visible rather than asserted. First, the suite this PR adds, each file individually and verbatim — 93 tests, all green: PT-01 — RestrictedPython sandbox, 12 escape attemptsRestrictedPython enforces at two stages, so each payload was compiled and executed through The negative control is not rhetorical — running the same list through a plain PT-03/07/11 — SSRF egress guardEvery reserved range denied, a public address allowed, and scheme/hostname handling checked: PT-17 — allow-list matching is suffix-based, not substringThat second one is the whole point — a naive PT-24 — password policyTested against the rule the code actually implements (≥12 chars, ≥3 of 4 character classes, not in the common list, no identifier substring), not a stricter one imagined from the commit message: PT-05/12/23 — provider webhook signaturesThe gap: PT-21 caps one of the two S2S mint paths
That cap landed on
Executed against the shipped schemas: The token it issues is a real RBAC JWT — Fairness on severity: this is not privilege escalation — the caller is already an admin or a reseller acting within their own scope — and PT-22's denylist does cover these tokens ( The fix is one field: # app/schemas/breeze_buddy/merchants.py
token_lifetime_days: int = Field(
default=365, ge=1, le=365,
description="Lifetime of the issued token in days (only when issue_token).",
)Happy to push that onto this branch — say the word and I'll amend, or it can go as a follow-up if you'd rather not re-run the approval. Recorded, not runThe rate-limiting, RBAC/IDOR and revocation controls (PT-08/09/13/14/15/16/18/19/20/22) are covered by the 35-case |
…026-001) Backend remediation for the white-box pentest. New shared security modules (ssrf, webhook_signature, token_revocation, password_policy) plus per-finding fixes: - PT-01 RCE: template python_code now compiled under RestrictedPython (blocks __class__/__subclasses__ dunder-traversal escape) AND gated OFF by default via ENABLE_CUSTOM_PYTHON_FUNCTIONS at flow-build time. - PT-02: scope the query-param template lookup to the dialed outbound number so an unauthenticated media WebSocket can't build an arbitrary template. - PT-03/07/11: single hardened SSRF egress guard (resolve host, deny private/loopback/link-local/metadata, https-only, per-hop redirect revalidation) applied to MCP, HttpRequestExecutor and the webhook sender. - PT-05/12/23: provider signature/token verification (Twilio/Plivo/Exotel) on all telephony callbacks + answer routes; recording downloads pinned to the provider's own hosts so master credentials never leak to an attacker URL. - PT-08: validate reseller_ids in the merchant user-create branch. - PT-09/14/15: template create/update authorize on merchant scope; null merchant_id treated as reseller-scoped (deny for merchant/user roles) across templates/configurations/leads/chat/numbers RBAC — on the read AND the write/create paths (validate_lead_access, validate_chat_create_access) so a write can never be looser than its read sibling. - PT-13: real RBAC filtering + ownership check on /numbers. - PT-16: /auth/accounts email branch requires password proof (no enumeration oracle); /login and /auth/s2s/token also spend one bcrypt against a shared dummy hash on the no-such-user branch so an unknown username is timing- indistinguishable from a wrong password; per-IP and per-username fixed-window brute-force rate limiting (fail-open on a Redis outage) guards /login, /auth/s2s/token, /signup and /auth/accounts. - PT-17: block LLM-sourced values in URL host position; SSRF guard on HTTP tools. - PT-18: cross-IP aggregate rate cap per public_widget_key. - PT-19: derive chat-demo client IP from the trusted last XFF hop. - PT-20: scope resolution fails closed (deny) on a broken owner chain. - PT-21: cap S2S token lifetime at 365 days on BOTH mint paths. There are two: POST /auth/s2s/token (admin-only) and POST /merchant with issue_token=true, which is reachable by resellers as well as admins and hands its value to the same create_access_token_with_rbac. The merchant one defaulted to 3650 days and allowed 365000, so capping only the first left a reseller able to mint a ten-year credential by default. Both now read MAX_S2S_TOKEN_LIFETIME_DAYS rather than their own literal, which is how they drifted apart in the first place. - PT-22: server-side JWT revocation denylist + per-request is_active liveness recheck; logout now revokes. - PT-24: password policy (length, complexity, common/identifier deny-list, bcrypt 72-byte guard). PT-04 (legacy admin login) was already removed on release; a regression test guards it. Adds tests: SSRF egress (incl. per-hop redirect revalidation + credential stripping on off-allow-list redirects), custom-python sandbox, an authz/scope/policy/token suite, and credential-endpoint rate limiting.
4b7ac3c to
fc00f48
Compare
PT-21 gap closed, plus the rate-limit and revocation controls proven against live infrastructureAmended to The fixBoth S2S mint paths now read one constant instead of their own literal — which is exactly how they drifted to 365 and 365000: # app/schemas/breeze_buddy/auth.py
# PT-21. Every path that mints a long-lived S2S token must bound it by this,
# not by its own literal — there is more than one such path (POST /auth/s2s/token
# and POST /merchant with issue_token=true), and they both hand the value
# straight to rbac_token_manager.create_access_token_with_rbac. A per-schema
# literal is how the two drifted to 365 and 365000 in the first place.
MAX_S2S_TOKEN_LIFETIME_DAYS = 365
Full suite: PT-16 / PT-18 / PT-19 / PT-22 against a live Redis
The per-username result is the one worth reading twice: 15 guesses against One more, found by accidentWiring the harness tripped two startup guards that turn out to be worth stating explicitly: The service refuses to boot with an unsigned-token algorithm rather than accepting Still not provenThe RBAC/IDOR controls (PT-08/09/13/14/15/20) are covered by the 36-case |
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary — PR #930
Scope: Breeze Buddy pentest remediation (22 backend findings). 57 files changed (+5,040 / −2,776), single commit.
New issues raised this run: 0
I walked the diff file-by-file against the project standards and the existing review threads. No new blocking security, authz, SSRF, SQL-injection, secret-leak, migration-editing, or PII-exposure issues were found in the current head. Existing review comments (including the resolved/outdated ones) already cover the points that were iterated on, and the author has addressed them in subsequent commits.
Key areas verified:
- SSRF:
validate_egress_urlresolves DNS and blocks internal/metadata;ssrf_safe_requestre-validates every redirect hop and stripsauth/headers on cross-host/off-allow-list redirects. - Telephony webhooks: Twilio/Plivo signatures and Exotel token verified via shared
verify_provider_webhook; recording downloads host-pinned to provider domains. - Authz:
merchant_scope_permittedconsistently denies null-merchant access to merchant/user roles across templates, leads, chat, configurations, and numbers; scope resolution fails closed on broken owner chains. - Auth: credential endpoints have per-IP + per-username rate limiting and bcrypt timing equalization; JWT revocation denylist + liveness recheck added.
- Custom Python: gated off by default and compiled under RestrictedPython.
- Migrations: no existing migration files were edited.
- Tests: new coverage for SSRF egress, custom-python sandbox, authz/scope/policy, and credential rate limiting.
Decision: Approve. The remaining open thread (google_login_handler 404) is a MINOR suggestion with an unresolved disagreement; it is not a blocking criterion under the project standards.
The last block: RBAC/IDOR driven over HTTP against the real appPreviously listed as outstanding, now closed. The unit suite asserts the RBAC helpers in isolation, which cannot catch a control that is correct but never wired into a route. This mounts the actual FastAPI app — real routers, real dependencies, real token verification — on the live Postgres and Redis, and issues real requests with real JWTs. Tenancy: reseller The Two things I got wrong first, and the code was right both times
The liveness recheck looked broken and is not. I flipped Coverage nowEvery PT-* control in this PR has attack-side evidence: PT-01 sandbox (12 payloads, blocked at compile or runtime, 12/12 executing unsandboxed), PT-03/07/11/17 SSRF, PT-05/12/23 signatures, PT-16/18/19 rate limiting against live Redis, PT-21 both mint paths, PT-22 revocation and liveness, PT-24 password policy, and PT-08/09/13/14/15/20 over HTTP above. |
Superseded — split into seven reviewable PRsThis PR was 57 files and +5040/-2776 in a single commit covering 20 pentest findings, which is not a reviewable unit. It has been split along the security-control seams, one PR per control, each with its own tests:
They form a stacked chain — #987 targets The split is verified, not assertedThe seven slices reassemble byte-for-byte into this PR's tree across One thing found while splittingThis branch was behind
All three are fixed in the split. Closing this in favour of the seven; the branch is untouched if anything needs to be recovered from it. |
Breeze Buddy pentest remediation — backend (Clairvoyance)
White-box pentest
BB-DEEPDIVE-2026-001(25 findings; 22 backend, 3 frontend). This PR closes the 22 backend findings. The 3 frontend findings (PT-06/10/25) land in the companion loom PR.Based on
release. Single commit (CI-enforced). New shared modules:app/core/security/{ssrf,webhook_signature,token_revocation,password_policy}.py.Findings → fix
python_codecompiled under RestrictedPython (blocks__class__/__subclasses__dunder-traversal escape at compile time) and gated OFF by default viaENABLE_CUSTOM_PYTHON_FUNCTIONSat flow-build.HttpRequestExecutoruses the shared SSRF guard (resolves DNS, blocks internal/metadata) with per-hop redirect revalidation.reseller_idsagainst the creator's scope.validate_template_access), incl. destination-merchant re-check on move.send_webhook_with_retryvalidates the URL (fail-closed, zero attempts) + redirect-safe fetch./numbers.merchant_id= reseller-scoped → deny for merchant/user roles across templates/configurations/leads/chat/numbers RBAC (sharedmerchant_scope_permitted)./auth/accountsemail branch requires password proof; generic 401 removes the enumeration oracle.public_widget_key.token_lifetime_dayscapped at 365.is_activeliveness recheck; logout revokes.Verification
black --check,isort --check,autoflake,pyrefly check— clean.uv run pytest— 680 passed (2 pre-existingtest_chat_analyticsfailures are unrelated to this PR — they fail onreleasetoo). New tests:test_ssrf_egress.py,test_custom_python_sandbox.py,test_pentest_authz.py.ENFORCE_TELEPHONY_WEBHOOK_SIGNATURES=true(default). RequiresTWILIO_AUTH_TOKEN/PLIVO_AUTH_TOKEN/EXOTEL_WEBHOOK_AUTH_TOKENand a correctAPP_BASE_URLper provider in every env, or those webhooks will 401. A temporary escape hatch exists (set tofalse) but must not be used in prod.ENABLE_CUSTOM_PYTHON_FUNCTIONS=false(default) — any template relying on custompython_codewill have that function skipped until an operator opts in.SSRF_ALLOW_PRIVATE_EGRESS=false(default) — settrueonly for local dev against localhost backends.Follow-ups (documented, out of scope for this PR)
Summary by CodeRabbit
Security Enhancements
Account & Password Improvements
Testing