feat(security): prevent merchant credentials in templates and resolve at request time inmemory - #961
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:
WalkthroughAdds merchant-scoped credentials, server-side credential resolution, custom HTTP authentication, and credential-aware MCP loading. Credential APIs, database access, pre-checks, HTTP handlers, templates, and chat/voice integrations now pass reseller and merchant scope. ChangesCredential-scoped authentication
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant http_function_handler
participant resolve_credential_auth
participant CredentialDatabase
participant executor.execute
Caller->>http_function_handler: provide request configuration
http_function_handler->>resolve_credential_auth: resolve scoped credential
resolve_credential_auth->>CredentialDatabase: fetch active credential
CredentialDatabase-->>resolve_credential_auth: credential value
resolve_credential_auth-->>http_function_handler: resolved auth
http_function_handler->>executor.execute: execute resolved request
executor.execute-->>Caller: return response
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
MonishJuspay
changed the title
MonishJuspay
changed the title
There was a problem hiding this comment.
Pull request overview
This PR introduces credential scoping (global, reseller-shared, merchant-specific) and shifts HTTP/MCP authentication to reference credentials by ID, resolving secrets server-side at request/connection time so templates don’t embed merchant credential values.
Changes:
- Add
merchant_idscoping to credentials (schema, queries, decoder, migration, API handlers/RBAC). - Add
HttpAuthConfig.credential_id+customauth support, and resolve credential-backed auth server-side at execution time. - Thread credential resolution through hooks, HTTP global functions, pre-checks, and MCP tool discovery/connection setup.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/services/credential_auth.py | Adds server-side resolution of credential_id into in-memory HttpAuthConfig values. |
| app/schemas/breeze_buddy/credentials.py | Adds merchant_id to credential create/model and validates scope rules. |
| app/database/queries/breeze_buddy/credentials.py | Extends insert/list queries for merchant scope and adds scoped “active by id” query. |
| app/database/migrations/042_add_credential_merchant_scope.sql | Adds merchant_id column and supporting indexes for scoped uniqueness/lookups. |
| app/database/decoder/breeze_buddy/credentials.py | Decodes merchant_id; keeps legacy template var flattening with updated wording. |
| app/database/accessor/breeze_buddy/credentials.py | Threads merchant_id through create/list; adds accessor for scoped active credential by id. |
| app/database/accessor/init.py | Exposes get_active_credential_by_id_for_scope at accessor package level. |
| app/api/routers/breeze_buddy/credentials/handlers.py | Passes merchant_id through create/list handlers; updates list logging. |
| app/api/routers/breeze_buddy/credentials/init.py | Adds merchant_id filter and updates RBAC checks for reseller/merchant scopes. |
| app/ai/voice/agents/breeze_buddy/template/types.py | Adds credential_id, custom auth, and validation preventing inline secrets with credential refs. |
| app/ai/voice/agents/breeze_buddy/template/loader.py | Reframes legacy credential placeholder loading as a compatibility path. |
| app/ai/voice/agents/breeze_buddy/template/hooks.py | Resolves credential-backed auth before executing external HTTP hooks. |
| app/ai/voice/agents/breeze_buddy/template/generator/prompts.py | Updates prompt examples to use credential_id instead of {placeholder} secrets. |
| app/ai/voice/agents/breeze_buddy/mcp/cache.py | Updates caching docs to reflect credential-id-based auth resolution. |
| app/ai/voice/agents/breeze_buddy/mcp/init.py | Makes MCP auth/header building async and resolves auth via credential_id at connection time. |
| app/ai/voice/agents/breeze_buddy/managers/pre_checks.py | Adds custom auth parsing and resolves credential_id before executing pre-check HTTP calls. |
| app/ai/voice/agents/breeze_buddy/handlers/transport/http_requester.py | Adds support for injecting resolved custom headers into outbound HTTP requests. |
| app/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.py | Resolves credential_id-backed auth before executing HTTP global functions. |
| app/ai/voice/agents/breeze_buddy/chat/turn_core.py | Minor renaming/clarity around legacy credential placeholder loading in chat. |
| app/ai/voice/agents/breeze_buddy/chat/agent.py | Passes reseller/merchant IDs into cached MCP tool loading for auth resolution. |
| app/ai/voice/agents/breeze_buddy/agent/init.py | Passes reseller/merchant IDs into MCP tool loading for auth resolution in voice agent. |
| ALTER TABLE credentials | ||
| ADD COLUMN IF NOT EXISTS merchant_id VARCHAR(255); | ||
|
|
||
| CREATE INDEX IF NOT EXISTS idx_credentials_reseller_merchant_active | ||
| ON credentials(reseller_id, merchant_id) | ||
| WHERE is_active = TRUE; |
| List credentials with optional merchant filter. | ||
|
|
||
| - Admin: sees all credentials or filtered by merchant | ||
| - Merchant: must provide reseller_id, sees merchant + global credentials | ||
| - Values are always masked in responses |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/ai/voice/agents/breeze_buddy/managers/pre_checks.py (1)
182-234: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAuth dict conversion silently drops ALL auth on a credential_id/inline-value conflict.
HttpAuthConfig(...)raises whencredential_idand any inline secret (token/username/password/api_key_value) are both present inauth_dict(per the newvalidate_credential_referencevalidator intypes.py) — e.g. a staletokenfield left behind when a pre-check config is migrated tocredential_id. Theexcepthere only logs a warning and returnsNone, sorun_pre_checksproceeds withauth=None, sending the pre-check's external request completely unauthenticated instead of failing the check. Since pre-check pass/fail gates whether the call proceeds, this can silently produce a wrong proceed/deny decision.Consider stripping/ignoring stale inline fields when
credential_idis present before constructingHttpAuthConfig, or treat a conversion failure as an execution error (matching the pattern already used for other pre-check failures) rather than silently falling through to no-auth.🛡️ Proposed fix
- return HttpAuthConfig( - type=auth_type, - credential_id=auth_dict.get("credential_id"), - token=auth_dict.get("token"), - username=auth_dict.get("username"), - password=auth_dict.get("password"), - api_key_name=auth_dict.get("api_key_name"), - api_key_value=auth_dict.get("api_key_value"), - header_bindings=auth_dict.get("header_bindings") or {}, - ) + credential_id = auth_dict.get("credential_id") + return HttpAuthConfig( + type=auth_type, + credential_id=credential_id, + token=None if credential_id else auth_dict.get("token"), + username=None if credential_id else auth_dict.get("username"), + password=None if credential_id else auth_dict.get("password"), + api_key_name=auth_dict.get("api_key_name"), + api_key_value=None if credential_id else auth_dict.get("api_key_value"), + header_bindings=auth_dict.get("header_bindings") or {}, + )🤖 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/managers/pre_checks.py` around lines 182 - 234, Update _convert_auth_dict_to_config so credential_id conflicts with stale inline secrets are handled before constructing HttpAuthConfig, preferably by ignoring/removing token, username, password, and api_key_value when a credential_id is present; alternatively propagate conversion failure as a pre-check execution error instead of returning None. Preserve unauthenticated behavior only for genuinely absent auth configuration.
🧹 Nitpick comments (2)
app/api/routers/breeze_buddy/credentials/__init__.py (1)
134-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding tests for the new scope-authorization matrix.
_has_scope-based checks now gate credential visibility/mutation across global/reseller-shared/merchant tiers. Given the security sensitivity, targeted unit tests for admin vs. non-admin, in-scope vs. out-of-scope reseller/merchant combinations would help guard against regressions.🤖 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/credentials/__init__.py` around lines 134 - 204, Add targeted unit tests for get_credential_endpoint and update_credential_endpoint covering admin access, non-admin access to global, reseller-shared, and merchant-scoped credentials, plus in-scope and out-of-scope reseller/merchant combinations. Assert that permitted requests proceed and denied requests raise HTTPException with the expected 403 status, preserving the existing _has_scope authorization matrix.app/services/credential_auth.py (1)
18-85: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider caching resolved credentials per call/session.
resolve_credential_authhits the DB on every invocation, and it's now called from HTTP functions, hooks, pre-checks, and MCP server connections — several of these can fire multiple times per call (e.g. one MCP connection per server, one hook per triggering function). A short-lived, per-call cache keyed bycredential_id(e.g. attached toTemplateContext/call state) would cut redundant DB round-trips without weakening the "resolve at request time" security property, since the cache would still be scoped to a single call's lifetime.🤖 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/services/credential_auth.py` around lines 18 - 85, Add a short-lived credential-resolution cache scoped to the current call or session state, keyed by credential_id and reused by resolve_credential_auth and its HTTP, hook, pre-check, and MCP callers. Preserve request-time resolution and existing reseller_id/merchant_id scope validation, while avoiding repeated database calls within the same call lifetime; keep independent calls from sharing cached credentials.
🤖 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/handlers/transport/http_handler.py`:
- Around line 127-148: Extract the template-first, lead-fallback
reseller_id/merchant_id resolution into a shared helper and use it for
credential auth scope resolution. Update
app/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.py:127-148 and
app/ai/voice/agents/breeze_buddy/template/hooks.py:403-414 to call the helper,
and update app/ai/voice/agents/breeze_buddy/managers/pre_checks.py:318-324 to
pass its existing template argument instead of resolving from lead alone;
preserve the template-over-lead precedence across all three call sites.
In `@app/ai/voice/agents/breeze_buddy/mcp/__init__.py`:
- Around line 495-512: Update _build_auth_headers to record whether server.auth
was originally credential-backed before calling resolve_credential_auth. When
the resolved auth contains SecretStr credential fields, use their underlying
values verbatim and do not pass them through resolve; retain template
substitution only for values supplied directly in the original template
configuration, including the related logic around lines 527-531.
In `@app/ai/voice/agents/breeze_buddy/template/generator/prompts.py`:
- Around line 331-340: Update the authentication guidance in the prompt template
documentation to include custom authentication alongside bearer, basic, and
API-key authentication. State that custom auth must use a configured
credential_id with header_bindings, and that its credential values are resolved
only when making the request rather than embedded in templates.
In `@app/api/routers/breeze_buddy/credentials/__init__.py`:
- Around line 147-162: The RBAC denial paths in get_credential_endpoint and
update_credential_endpoint must not expose credential.reseller_id or
credential.merchant_id. Replace both scope-specific 403 detail messages with a
generic access-denied message, preserving the existing scope checks and status
code.
---
Outside diff comments:
In `@app/ai/voice/agents/breeze_buddy/managers/pre_checks.py`:
- Around line 182-234: Update _convert_auth_dict_to_config so credential_id
conflicts with stale inline secrets are handled before constructing
HttpAuthConfig, preferably by ignoring/removing token, username, password, and
api_key_value when a credential_id is present; alternatively propagate
conversion failure as a pre-check execution error instead of returning None.
Preserve unauthenticated behavior only for genuinely absent auth configuration.
---
Nitpick comments:
In `@app/api/routers/breeze_buddy/credentials/__init__.py`:
- Around line 134-204: Add targeted unit tests for get_credential_endpoint and
update_credential_endpoint covering admin access, non-admin access to global,
reseller-shared, and merchant-scoped credentials, plus in-scope and out-of-scope
reseller/merchant combinations. Assert that permitted requests proceed and
denied requests raise HTTPException with the expected 403 status, preserving the
existing _has_scope authorization matrix.
In `@app/services/credential_auth.py`:
- Around line 18-85: Add a short-lived credential-resolution cache scoped to the
current call or session state, keyed by credential_id and reused by
resolve_credential_auth and its HTTP, hook, pre-check, and MCP callers. Preserve
request-time resolution and existing reseller_id/merchant_id scope validation,
while avoiding repeated database calls within the same call lifetime; keep
independent calls from sharing cached credentials.
🪄 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 Plus
Run ID: 09c81280-95c2-4e46-a7e6-d6a1dc6b5816
📒 Files selected for processing (21)
app/ai/voice/agents/breeze_buddy/agent/__init__.pyapp/ai/voice/agents/breeze_buddy/chat/agent.pyapp/ai/voice/agents/breeze_buddy/chat/turn_core.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/managers/pre_checks.pyapp/ai/voice/agents/breeze_buddy/mcp/__init__.pyapp/ai/voice/agents/breeze_buddy/mcp/cache.pyapp/ai/voice/agents/breeze_buddy/template/generator/prompts.pyapp/ai/voice/agents/breeze_buddy/template/hooks.pyapp/ai/voice/agents/breeze_buddy/template/loader.pyapp/ai/voice/agents/breeze_buddy/template/types.pyapp/api/routers/breeze_buddy/credentials/__init__.pyapp/api/routers/breeze_buddy/credentials/handlers.pyapp/database/accessor/__init__.pyapp/database/accessor/breeze_buddy/credentials.pyapp/database/decoder/breeze_buddy/credentials.pyapp/database/migrations/042_add_credential_merchant_scope.sqlapp/database/queries/breeze_buddy/credentials.pyapp/schemas/breeze_buddy/credentials.pyapp/services/credential_auth.py
| template = getattr(context.bot, "template", None) | ||
| reseller_id = getattr(template, "reseller_id", None) | ||
| merchant_id = getattr(template, "merchant_id", None) | ||
| if not reseller_id: | ||
| reseller_id = getattr(context.lead, "reseller_id", None) | ||
| if merchant_id is None: | ||
| merchant_id = getattr(context.lead, "merchant_id", None) | ||
| resolved_auth = await resolve_credential_auth( | ||
| config.http_request.auth, | ||
| reseller_id=reseller_id, | ||
| merchant_id=merchant_id, | ||
| ) | ||
| request_config = config.http_request.model_copy( | ||
| update={"auth": resolved_auth} | ||
| ) | ||
|
|
||
| # Step 2: Create executor | ||
| executor = HttpRequestExecutor(session=context.aiohttp_session) | ||
|
|
||
| logger.info( | ||
| f"[{function_name}] Executing HTTP {config.http_request.method.value} " | ||
| f"request to {config.http_request.url}" | ||
| f"[{function_name}] Executing HTTP {request_config.method.value} " | ||
| f"request to {request_config.url}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Credential-scope (reseller_id/merchant_id) derivation is duplicated and inconsistent across call sites.
http_handler.py and hooks.py both derive scope by trying context.bot.template.reseller_id/merchant_id first and falling back to context.lead, while pre_checks.py uses lead.reseller_id/lead.merchant_id directly even though run_pre_checks already receives the template parameter. This means the same logical operation — resolving a credential-scoped HttpAuthConfig for an outbound HTTP call — can pick a different reseller/merchant scope depending on whether it's triggered via an HTTP global function, a hook, or a pre-check, risking a credential lookup succeeding/failing (or resolving to the wrong scope) inconsistently between these paths.
app/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.py#L127-L148: extract the template-then-lead fallback into a shared helper (e.g. incredential_auth.pyor a small scope-resolution util) and use it here.app/ai/voice/agents/breeze_buddy/template/hooks.py#L403-L414: use the same shared helper instead of re-implementing the fallback.app/ai/voice/agents/breeze_buddy/managers/pre_checks.py#L318-L324: use the same shared helper (passing the already-availabletemplateargument) instead of resolving scope fromleadalone, so pre-checks match the precedence used by HTTP functions/hooks.
📍 Affects 3 files
app/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.py#L127-L148(this comment)app/ai/voice/agents/breeze_buddy/template/hooks.py#L403-L414app/ai/voice/agents/breeze_buddy/managers/pre_checks.py#L318-L324
🤖 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/handlers/transport/http_handler.py` around
lines 127 - 148, Extract the template-first, lead-fallback
reseller_id/merchant_id resolution into a shared helper and use it for
credential auth scope resolution. Update
app/ai/voice/agents/breeze_buddy/handlers/transport/http_handler.py:127-148 and
app/ai/voice/agents/breeze_buddy/template/hooks.py:403-414 to call the helper,
and update app/ai/voice/agents/breeze_buddy/managers/pre_checks.py:318-324 to
pass its existing template argument instead of resolving from lead alone;
preserve the template-over-lead precedence across all three call sites.
| async def _build_auth_headers( | ||
| server: McpServerConfig, | ||
| template_vars: Dict[str, Any], | ||
| reseller_id: Optional[str], | ||
| merchant_id: Optional[str], | ||
| ) -> Dict[str, str]: | ||
| """Resolve auth config into HTTP headers, substituting {variable} placeholders.""" | ||
| """Resolve MCP auth into HTTP headers without exposing credential values.""" | ||
| if not server.auth or server.auth.type == HttpAuthType.NONE: | ||
| return {} | ||
|
|
||
| def resolve(value: str) -> str: | ||
| return _resolve_placeholders(value, template_vars) | ||
|
|
||
| auth = server.auth | ||
| auth = await resolve_credential_auth( | ||
| server.auth, | ||
| reseller_id=reseller_id, | ||
| merchant_id=merchant_id, | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not apply template substitution to resolved credential secrets.
resolve_credential_auth() returns SecretStr credential values and clears credential_id. The subsequent resolve(...) calls therefore treat those secrets as template strings; any credential containing {...} can be rewritten using payload/template variables, causing authentication failures and allowing untrusted data to influence credential material.
Capture whether the original config was credential-backed and return those values verbatim; only interpolate values supplied directly as template configuration.
Based on the resolver contract, credential values are returned in memory as SecretStr fields.
Suggested fix
+ credential_backed = server.auth.credential_id is not None
+
auth = await resolve_credential_auth(
server.auth,
reseller_id=reseller_id,
merchant_id=merchant_id,
)
def resolve(value: str) -> str:
- return _resolve_placeholders(value, template_vars)
+ return value if credential_backed else _resolve_placeholders(
+ value, template_vars
+ )Also applies to: 527-531
🤖 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/mcp/__init__.py` around lines 495 - 512,
Update _build_auth_headers to record whether server.auth was originally
credential-backed before calling resolve_credential_auth. When the resolved auth
contains SecretStr credential fields, use their underlying values verbatim and
do not pass them through resolve; retain template substitution only for values
supplied directly in the original template configuration, including the related
logic around lines 527-531.
| "auth": {"type": "bearer", "credential_id": "configured-credential-id"}, | ||
| "body": {"booking_id": "{booking_id}", "refund_reason": "{refund_reason}"}, | ||
| "timeout": 10, | ||
| "max_retries": 3 | ||
| }, | ||
| "cancel_on_interruption": false | ||
| } | ||
| ``` | ||
| Auth types: `none`, `bearer` (token), `basic` (username+password), | ||
| `api_key` (header name+value). | ||
| For bearer, basic, and API-key authentication, use a configured | ||
| `credential_id`; the credential value is resolved only while making the request. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document custom credential resolution here too.
The new guidance covers bearer, basic, and API-key auth but omits custom, even though custom credentials use credential_id plus header_bindings. Without this, template authors may continue embedding custom header secrets in templates.
Based on the PR objectives, custom credentials are also required to remain server-side.
🤖 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/template/generator/prompts.py` around lines
331 - 340, Update the authentication guidance in the prompt template
documentation to include custom authentication alongside bearer, basic, and
API-key authentication. State that custom auth must use a configured
credential_id with header_bindings, and that its credential values are resolved
only when making the request rather than embedded in templates.
| # RBAC check: non-admin users can only access credentials in their scope. | ||
| if current_user.role != "admin": | ||
| if credential.reseller_id is not None: | ||
| if ( | ||
| credential.reseller_id not in current_user.reseller_ids | ||
| and "*" not in current_user.reseller_ids | ||
| ): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail=f"Access denied to credential for reseller {credential.reseller_id}", | ||
| ) | ||
| if credential.reseller_id and not _has_scope( | ||
| current_user.reseller_ids, credential.reseller_id | ||
| ): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail=f"Access denied to credential for reseller {credential.reseller_id}", | ||
| ) | ||
| if credential.merchant_id and not _has_scope( | ||
| current_user.merchant_ids, credential.merchant_id | ||
| ): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail=f"Access denied to credential for merchant {credential.merchant_id}", | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
RBAC-denied responses leak the credential's scope identifiers to unauthorized callers.
Both get_credential_endpoint and update_credential_endpoint fetch the credential before checking scope, then echo credential.reseller_id/credential.merchant_id in the 403 detail when access is denied. An authenticated caller without access to a given reseller/merchant can submit an arbitrary credential_id and learn which reseller/merchant it belongs to purely from the error message.
Consider returning a generic "Access denied" (or 404, to avoid confirming existence) instead of echoing the resource's actual scope values.
🔒 Proposed fix: generic denial message
- if credential.reseller_id and not _has_scope(
- current_user.reseller_ids, credential.reseller_id
- ):
- raise HTTPException(
- status_code=status.HTTP_403_FORBIDDEN,
- detail=f"Access denied to credential for reseller {credential.reseller_id}",
- )
- if credential.merchant_id and not _has_scope(
- current_user.merchant_ids, credential.merchant_id
- ):
- raise HTTPException(
- status_code=status.HTTP_403_FORBIDDEN,
- detail=f"Access denied to credential for merchant {credential.merchant_id}",
- )
+ if credential.reseller_id and not _has_scope(
+ current_user.reseller_ids, credential.reseller_id
+ ):
+ raise HTTPException(
+ status_code=status.HTTP_403_FORBIDDEN,
+ detail="Access denied to this credential",
+ )
+ if credential.merchant_id and not _has_scope(
+ current_user.merchant_ids, credential.merchant_id
+ ):
+ raise HTTPException(
+ status_code=status.HTTP_403_FORBIDDEN,
+ detail="Access denied to this credential",
+ )Also applies to: 191-202
🤖 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/credentials/__init__.py` around lines 147 - 162,
The RBAC denial paths in get_credential_endpoint and update_credential_endpoint
must not expose credential.reseller_id or credential.merchant_id. Replace both
scope-specific 403 detail messages with a generic access-denied message,
preserving the existing scope checks and status code.
PR #961 —
|
e80efaf to
3e07751
Compare
ad62a3e to
29b1f20
Compare
| credential_type = credential.credential_type | ||
| value = credential.value | ||
| if auth_config.type == HttpAuthType.BEARER: | ||
| if credential_type != CredentialType.BEARER_TOKEN or not value.get("token"): |
There was a problem hiding this comment.
Just a safety check if the request is for Bearer token but the credential type stored in the credential table is not matching then raise the error
| if auth_config.type == HttpAuthType.BEARER: | ||
| if credential_type != CredentialType.BEARER_TOKEN or not value.get("token"): | ||
| raise ValueError("Bearer auth requires a bearer_token credential") | ||
| updates = {"token": SecretStr(value["token"])} |
There was a problem hiding this comment.
SecretStr masking the token value now , so that anywhere in the logs the actual token wont be logged, the actual token from the SecretStr is extracted just before making the http request
| "username": value["username"], | ||
| "password": SecretStr(value["password"]), | ||
| } | ||
| elif auth_config.type == HttpAuthType.CUSTOM: |
There was a problem hiding this comment.
CUSTOM type supports multiple credentials in the value json.
| raise ValueError("Custom auth requires a custom credential") | ||
|
|
||
| custom_headers = {} | ||
| for header_name, credential_field in auth_config.header_bindings.items(): |
There was a problem hiding this comment.
From the templateJSON.In a custom-auth HTTP function, the template contains something like:
"auth": {
"type": "custom",
"credential_id": "abc-123",
"header_bindings": {
"X-Client-Secret": "client_secret",
"X-Shop-Id": "shop_id"
}
}
when the header_bindings is loaded from the template, pydantic creates the httpAuthConfig object which has header_bindings made as python dictionary.
like:
"auth_config.header_bindings == {
"X-Client-Secret": "client_secret",
"X-Shop-Id": "shop_id",
}"
| "Custom auth credential is missing a non-empty field " | ||
| f"'{credential_field}' for header '{header_name}'" | ||
| ) | ||
| custom_headers[header_name] = SecretStr(field_value) |
There was a problem hiding this comment.
since the header_bindings has the
"auth_config.header_bindings == {
"X-Client-Secret": "client_secret",
"X-Shop-Id": "shop_id",
}"
and now the value from the DB is loaded and replaced with client_secret,shop_id as the key
|
|
||
|
|
||
| def _build_server_params( | ||
| async def _build_server_params( |
There was a problem hiding this comment.
As we have to make a db call now to get the credentials so await, so async in this method
| "bearer": HttpAuthType.BEARER, | ||
| "basic": HttpAuthType.BASIC, | ||
| "api_key": HttpAuthType.API_KEY, | ||
| "custom": HttpAuthType.CUSTOM, |
There was a problem hiding this comment.
Added to the type_mapping
| # Convert auth dict to HttpAuthConfig (if present) | ||
| converted_auth = _convert_auth_dict_to_config(http_cfg.auth) | ||
| # Convert raw pre-check JSON, then resolve any credential reference. | ||
| auth_config = _convert_auth_dict_to_config(http_cfg.auth) |
There was a problem hiding this comment.
returns the object: HttpAuthConfig
| # Convert raw pre-check JSON, then resolve any credential reference. | ||
| auth_config = _convert_auth_dict_to_config(http_cfg.auth) | ||
| reseller_id, merchant_id = resolve_credential_scope(template, lead) | ||
| resolved_auth = await resolve_credential_auth( |
There was a problem hiding this comment.
getting credentials for the pre-check
| http_cfg.query_params | ||
| ) | ||
|
|
||
| http_config = HttpRequestConfig( |
There was a problem hiding this comment.
Creating the python object that represents the full http request,
auth: has the values like:
"resolved_auth = {
type: "bearer",
credential_id: None,
token: SecretStr("real-token")
}"
But auth is not itself sent as JSON in the request body.
Later, HttpRequestExecutor reads this object and creates the real HTTP headers
1e70df7 to
b08323e
Compare
| credential_keys = set(credential_vars) | ||
| for override in overrides: | ||
| if override: | ||
| credential_keys.difference_update(override) |
There was a problem hiding this comment.
if in case the credential is being overrided by the another key in the lead_payload or template_secrets, remove that from checking, means that template is not just dependent on the keys coming from the credential table
| if not credential_keys: | ||
| return | ||
|
|
||
| template_content = json.dumps(template.flow or {}, default=str) |
There was a problem hiding this comment.
flattening the python dictionary to the normal python string
| from app.core.logger import logger | ||
|
|
||
|
|
||
| def log_legacy_credential_placeholder_usage( |
There was a problem hiding this comment.
Used to check the template that is having the credential string usage in the template
|
|
||
| template_content = json.dumps(template.flow or {}, default=str) | ||
| if template.configurations: | ||
| template_content += template.configurations.model_dump_json() |
There was a problem hiding this comment.
since the mcps are configured in the template so we have to check that as well for the key words
| ) | ||
|
|
||
| template_vars = {} | ||
| credential_vars = {} |
There was a problem hiding this comment.
since we have to pass to the function : log_legacy_credential_placeholder_usage
for the checking if the credentials is being used by the template and thrown an alert
| """Get AIO Http Session instance""" | ||
| return self.bot.aiohttp_session | ||
|
|
||
| @property |
There was a problem hiding this comment.
How it helps
HTTP function
-> context.credential_cache
-> cache is empty
-> fetch credential abc-123 from DB
-> save it in cache
-> make HTTP request
Later, a hook uses the same credential ID:
HTTP hook
-> context.credential_cache
-> finds abc-123 already there
-> no DB fetch
-> make HTTP request
| return self.bot.aiohttp_session | ||
|
|
||
| @property | ||
| def credential_cache(self) -> Dict[str, Any]: |
There was a problem hiding this comment.
Why are we having the credential_cache in the templatecontext object ?
This templateContext is created whenever a functionHandler is invoked so that the function can data related to that call. like lead,httpSession, credentialCache now..
This actually creates the credential_cache in the main agent object, so across the call, even for the different function handler, ( everytime a new templateContext object is created ) but the underlyting credential_cache remains same because the field is there in the agent object.
Why didnt create directly in the Agent class directly??
this is because of the lazy creation,
create the cache only when some HTTP handler actually needs a credential
b08323e to
aff5a11
Compare
| ) | ||
| from app.ai.voice.agents.breeze_buddy.template.utils import render_messages_with_vars | ||
| from app.ai.voice.agents.breeze_buddy.template.utils import ( | ||
| log_legacy_credential_placeholder_usage, |
There was a problem hiding this comment.
def log_deprecated_fields
There was a problem hiding this comment.
Here log_legacy_credential_placeholder_usage function checks whether a template is still using the old unsafe credential-loading method, so that in the template load path, it takes the template object, credential_vars and convert template flow,configurations(because it has mcp.., mcp uses token) into the string checks if the credentials vars is present or not
we cannot reuse the existing log_deprecated_fields() as it is, because it checks "Does this known field exist in this Python object?handles old JSON field names".
Our function checks: “Do any credential names loaded from the DB appear as {placeholders} inside this template?”
aff5a11 to
286ddaf
Compare
|
Resolved credential secrets get re-run through This is the same bug class already flagged on Flow, all new in this PR:
(
The same function also re-applies Net effect: if a stored credential value happens to contain a Suggest tracking whether |
| password: Optional[SecretStr] = None # For basic auth | ||
| api_key_name: Optional[str] = None # Header name for API key | ||
| api_key_value: Optional[SecretStr] = None # API key value | ||
| header_bindings: Dict[str, str] = Field( |
There was a problem hiding this comment.
Why header_bindings ?
This is stored in the template. It says:
Which field from this custom credential should go into which HTTP header?
Example template configuration:
{
"type": "custom",
"credential_id": "abc-uuid",
"header_bindings": {
"X-Client-ID": "client_id",
"X-Client-Secret": "client_secret"
}
}
custom_headers
This is created only at runtime after the credential is fetched from DB.
Credential DB value:
{
"client_id": "real-id",
"client_secret": "real-secret"
}
Using header_bindings, our code creates:
custom_headers = {
"X-Client-ID": SecretStr("real-id"),
"X-Client-Secret": SecretStr("real-secret"),
}
Then the HTTP executor sends those as real headers.
| @@ -6,10 +6,9 @@ | |||
| same template + same resolved URL share one discovery for the TTL window. | |||
|
|
|||
There was a problem hiding this comment.
Just comment changes for developer ref
… them at request time - add global, reseller-shared, and merchant credential scopes - resolve bearer, API key, basic, and custom credentials at request time - support credential IDs in HTTP functions, hooks, pre-checks, and MCP - keep credential-ID secrets in memory only during outbound requests
286ddaf to
f4bff35
Compare
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail=f"Access denied to credential for reseller {credential.reseller_id}", | ||
| ) | ||
| if credential.merchant_id and not _has_scope( |
There was a problem hiding this comment.
support for checking the merchantId, if a merchantId is passed but the current user dont have the access to this merchant credentials then return the FORBIDDEN error
| return await list_credentials_handler(reseller_id, merchant_id, current_user) | ||
|
|
||
|
|
||
| @router.get("/credentials/{credential_id}", response_model=Credential) |
There was a problem hiding this comment.
This get endpoint wont return the plain credentials as it is. it returns the masked values, seems to be used by our dashboard
| # values here so legacy MCP auth placeholders can be detected. | ||
| template_content += template.configurations.model_dump_json( | ||
| context={"reveal_secrets": True} | ||
| ) |
There was a problem hiding this comment.
SecretStr hides its content when converted to JSON:
token = SecretStr("{wismo_secret}")
model_dump_json()
would produce:
{
"token": "**********"
}
Then our detector cannot see:
{wismo_secret}
So it would miss the old unsafe use.
With:
model_dump_json(
context={"reveal_secrets": True}
)
it temporarily produces:
{
"token": "{wismo_secret}"
}
Now the detector can find it and log
| http_request.auth, | ||
| reseller_id=reseller_id, | ||
| merchant_id=merchant_id, | ||
| credential_cache=context.credential_cache, |
There was a problem hiding this comment.
credential cache is the object in the agent class to store the creds fetched in that agent conversation, so that any function which is going to reuse the credentials can reuse that without fetching from the DB.
preventing the DB call.
Flow:
- First HTTP global function or hook uses a credential_id.
- It fetches that credential from DB and saves it in the agent’s in-memory dictionary.
- Another HTTP function/hook in the same call using the same credential_id reuses it from memory.
- When that call’s agent ends, the cache disappears.
It is not sent to the LLM and is not stored in template_vars.
Currently, HTTP global functions and hooks share this cache. Pre-checks use their own cache for that pre-check run, and MCP does not yet pass this shared cache.
Current PR follows the safe migration path:
Where the default credentials from the DB reseller levell is currently refered by the multiple templates, so after the current PR is getting released, those templates can be added with the new credential_id based flow instead of having the {token}, post the template token migration, we can remove the credential to template_vars for blocking the path of credential to LLM leakage completely.
Test proof:
Global HTTP function
Hook Path Test:
Callexecution-config Pre-check credential loading path
MCP-server credential loading path
Credential_type: Basic
Deprecated Alert
For migration of the existing templates
Summary by CodeRabbit