fix(security): password policy, credential rate limiting and token revocation (PT-16/18/19/21/22/24) - #996
fix(security): password policy, credential rate limiting and token revocation (PT-16/18/19/21/22/24)#996murdore 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:
WalkthroughThe change adds credential rate limits, shared password policies, enumeration-resistant authentication, JWT revocation, account-liveness checks, asynchronous RBAC verification, stricter token lifetimes, and widget-level aggregate limits. ChangesAuthentication security
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthRouter
participant RateLimiter
participant Redis
participant AuthHandler
Client->>AuthRouter: Submit credentials
AuthRouter->>RateLimiter: Enforce IP and identifier limits
RateLimiter->>Redis: Read hourly buckets
Redis-->>RateLimiter: Return bucket status
RateLimiter-->>AuthRouter: Allow or return HTTP 429
AuthRouter->>AuthHandler: Verify credentials
AuthHandler-->>Client: Return authentication result
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
There was a problem hiding this comment.
Pull request overview
This PR implements multiple coupled security remediations in the Breeze Buddy auth surface: enforcing a shared password-strength policy where passwords are set, bounding online credential guessing via Redis-backed fixed-window rate limiting, adding server-side JWT revocation plus per-request user liveness checks, and enforcing a single S2S token lifetime cap across both mint paths.
Changes:
- Add password policy enforcement (min length, complexity, common/identifier deny-list, 72-byte bcrypt guard) at signup and user management schema boundaries.
- Add credential endpoint rate limiting (/login, /auth/s2s/token, /signup, /auth/accounts) and fix timing/account enumeration behavior using a dummy bcrypt verification budget.
- Add Redis-backed JWT revocation on logout plus per-request token revocation + user is_active recheck; unify S2S lifetime cap via a shared constant across both mint paths.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_token_lifetime_revocation.py | Adds coverage for S2S lifetime caps and token revocation denylist behavior. |
| tests/test_stt_stream.py | Updates tests for async WebSocket auth helpers after RBAC verification becomes async. |
| tests/test_password_policy.py | Adds tests for password strength policy enforcement. |
| tests/test_auth_rate_limit.py | Adds tests for credential endpoint rate limiting (per-IP and per-identifier). |
| tests/test_auth_enumeration.py | Adds tests ensuring account listing and auth endpoints don’t leak account existence/timing. |
| app/schemas/breeze_buddy/users.py | Enforces password policy on user create/update via Pydantic model validators. |
| app/schemas/breeze_buddy/signup.py | Enforces password policy on signup and requires password proof for email-based account listing. |
| app/schemas/breeze_buddy/merchants.py | Applies unified MAX_S2S_TOKEN_LIFETIME_DAYS cap to merchant mint path. |
| app/schemas/breeze_buddy/auth.py | Introduces MAX_S2S_TOKEN_LIFETIME_DAYS and tightens auth request validation. |
| app/database/accessor/breeze_buddy/users.py | Adds Redis-cached user liveness check + invalidation hooks for PT-22. |
| app/core/security/token_revocation.py | Implements Redis-backed JWT revocation denylist keyed by token hash. |
| app/core/security/password/password.py | Enforces bcrypt 72-byte guard and introduces a dummy hash for timing equalization. |
| app/core/security/password/password_policy.py | Adds shared password-strength policy implementation. |
| app/core/security/password/init.py | Exposes password primitives and policy via a single package import path. |
| app/core/config/static.py | Adds env-configured rate limit caps for credential endpoints. |
| app/api/security/breeze_buddy/rbac_token.py | Makes RBAC verification async; adds revocation + liveness checks. |
| app/api/routers/feature_flags/rbac.py | Updates RBAC verification call to await async verifier. |
| app/api/routers/breeze_buddy/widget_common.py | Adds cross-IP aggregate widget rate limiting and last-hop XFF IP derivation. |
| app/api/routers/breeze_buddy/webhooks/woocommerce/services.py | Awaits async RBAC verification for stored JWT validation. |
| app/api/routers/breeze_buddy/webhooks/breeze/services.py | Awaits async RBAC verification for stored JWT validation. |
| app/api/routers/breeze_buddy/stt/handlers.py | Awaits async get_user_from_websocket for WebSocket auth. |
| app/api/routers/breeze_buddy/signup/handlers.py | Enforces constant bcrypt budget for account listing and adds password proof requirement. |
| app/api/routers/breeze_buddy/signup/init.py | Wires credential rate limiting into signup and account listing routes. |
| app/api/routers/breeze_buddy/chat/demo.py | Reuses shared last-hop XFF client_ip helper for rate limiting. |
| app/api/routers/breeze_buddy/auth/rate_limit.py | Adds shared credential rate limiting helper used across auth endpoints. |
| app/api/routers/breeze_buddy/auth/handlers.py | Adds dummy bcrypt timing equalization, and logout token revocation. |
| app/api/routers/breeze_buddy/auth/init.py | Wires auth rate limiting and updates logout to revoke server-side. |
| .env.example | Documents new auth rate limit environment variables. |
Suppressed comments (1)
app/core/security/password/password.py:199
- DUMMY_PASSWORD_HASH is generated by calling bcrypt hashing at module import time. Given the comment above notes bcrypt can take ~240ms, doing a hash during import can noticeably slow cold starts for every worker/process. Prefer a precomputed valid bcrypt hash constant (still same work factor during verification) rather than paying hashpw at import.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| async def revoke_token(token: str, exp: int) -> bool: | ||
| """Add a token to the denylist until its own expiry. Returns success.""" | ||
| ttl = int(exp - time.time()) | ||
| if ttl <= 0: | ||
| return True # already expired — nothing to revoke | ||
| try: | ||
| redis = await get_redis_service() | ||
| return await redis.setex(_denylist_key(token), "1", ttl_seconds=ttl) | ||
| except Exception as e: | ||
| logger.error(f"Failed to revoke JWT: {e}") | ||
| return False |
| def test_list_accounts_request_requires_password_with_email(): | ||
| from app.schemas.breeze_buddy.signup import ListAccountsRequest | ||
|
|
||
| with pytest.raises(Exception): | ||
| ListAccountsRequest(email="victim@company.com") # no password | ||
| ListAccountsRequest(email="v@c.com", password="x") # ok | ||
| ListAccountsRequest(id_token="tok") # ok |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
app/core/security/token_revocation.py (1)
35-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse exception-aware Loguru logging in the new handlers.
Interpolating
einto the message can lose exception context and can mis-handle braces from exception text. Uselogger.opt(exception=e).error(...)with structured arguments.
app/core/security/token_revocation.py#L35-L46: replace both interpolated exception messages with exception-aware Loguru calls.app/database/accessor/breeze_buddy/users.py#L153-L180: replace each interpolated exception message with exception-aware Loguru calls.Based on learnings: use
logger.opt(exception=e).error(...)in exception handlers, and do not interpolate exception objects into Loguru messages.🤖 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/token_revocation.py` around lines 35 - 46, In app/core/security/token_revocation.py lines 35-46, update both exception handlers in the token revocation functions to use logger.opt(exception=e).error(...) with structured message arguments instead of interpolating e. Apply the same exception-aware logging change to each interpolated exception message in app/database/accessor/breeze_buddy/users.py lines 153-180; preserve the existing handler behavior and log context.Source: Learnings
app/api/routers/breeze_buddy/widget_common.py (1)
243-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument both rate-limit denial paths.
The surrounding docstrings describe only per-IP enforcement. These changes add a cross-IP aggregate counter and a second HTTP 429 path. Document the aggregate scope and state that
Retry-Afterapplies to either limit.Also applies to: 271-276
🤖 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/widget_common.py` around lines 243 - 249, Update the surrounding widget rate-limit docstrings near the per-IP enforcement and _enforce_widget_aggregate_limit call to document both denial paths: the per-IP limit and cross-IP aggregate limit for the merchant’s public_widget_key. State that either limit can return HTTP 429 and that Retry-After applies in both cases.app/core/config/static.py (1)
550-555: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the integer parse against empty or invalid values.
int(os.environ.get(...))raisesValueErrorat import time if the variable is set to an empty string or a non-numeric value. That fails application startup rather than falling back to the documented default. Deployment tooling commonly sets empty values for unused variables. Also clamp negative values, because a negative limit denies every credential request.If the surrounding file already parses integer environment variables with a shared helper, use that helper instead.
🛡️ Proposed fix
-AUTH_RATE_LIMIT_PER_IP_PER_HOUR = int( - os.environ.get("AUTH_RATE_LIMIT_PER_IP_PER_HOUR", "40") -) -AUTH_RATE_LIMIT_PER_USERNAME_PER_HOUR = int( - os.environ.get("AUTH_RATE_LIMIT_PER_USERNAME_PER_HOUR", "15") -) +def _int_env(name: str, default: int) -> int: + raw = (os.environ.get(name) or "").strip() + if not raw: + return default + try: + return max(0, int(raw)) + except ValueError: + return default + + +AUTH_RATE_LIMIT_PER_IP_PER_HOUR = _int_env("AUTH_RATE_LIMIT_PER_IP_PER_HOUR", 40) +AUTH_RATE_LIMIT_PER_USERNAME_PER_HOUR = _int_env( + "AUTH_RATE_LIMIT_PER_USERNAME_PER_HOUR", 15 +)🤖 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 550 - 555, Update the AUTH_RATE_LIMIT_PER_IP_PER_HOUR and AUTH_RATE_LIMIT_PER_USERNAME_PER_HOUR configuration parsing to use the surrounding shared integer-environment helper if available, falling back to the documented defaults for empty or invalid values and clamping negative results to zero. Preserve the existing defaults of 40 and 15 respectively.app/api/routers/breeze_buddy/auth/rate_limit.py (2)
46-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving
client_ipto a shared request-utility module.The auth rate limiter and
app/api/routers/breeze_buddy/chat/demo.pyboth importclient_ipfromwidget_common. The helper is not widget-specific. A neutral location, for exampleapp/api/routers/breeze_buddy/request_utils.pyorapp/core/, states the dependency direction more clearly and avoids auth code depending on widget code.🤖 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/rate_limit.py` at line 46, Move the client_ip helper from widget_common into a neutral shared request-utility module, then update the auth rate limiter and chat/demo.py imports to use the new location. Remove the old widget_common import or re-export only if existing consumers require compatibility, while preserving client_ip behavior.
105-111: 🔒 Security & Privacy | 🔵 TrivialNote the account-lockout vector of the per-username cap.
The per-username bucket increments before authentication. An attacker who knows a victim's username can send 15 failed attempts per hour from any set of IPs and lock the victim out of login for the remainder of the window. The per-IP cap does not prevent this from distributed sources.
Two options reduce the impact:
- Increment the per-username counter only on a failed authentication, not on every attempt. A successful login then never counts against the victim.
- Reset or decrement the per-username counter after a successful authentication.
Add a metric on 429 responses split by bucket so operators can distinguish a real brute-force attempt from a targeted lockout.
🤖 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/rate_limit.py` around lines 105 - 111, Update the authentication flow around check_rate_limit so the credential_user bucket increments only after a failed authentication, or is reset/decremented after successful authentication, preventing attackers from locking out known usernames. Preserve the per-IP protection, and add a 429 metric labeled by the rate-limit bucket so operators can distinguish username and IP throttling.app/api/routers/breeze_buddy/auth/handlers.py (1)
436-436: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a Pydantic model instead of a bare dict.
logout_handlerreturns an untypeddictwith three keys, and the router returns it directly as the endpoint response. The coding guidelines require Pydantic models for all API request/response schemas. Define aLogoutResponsemodel inapp/schemas/breeze_buddy/auth.pywithsuccess,message, andrevokedfields, set it as the routeresponse_model, and annotate the handler return type. The response shape then appears in the OpenAPI schema, and callers can rely on therevokedflag.As per coding guidelines: "Use Pydantic models for all API request/response schemas and data transfer" for
app/api/routers/**/*.py, and "Use Optional[T], List[T], Dict[str, Any], Union for type annotations".Also applies to: 471-484
🤖 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/handlers.py` at line 436, Define a LogoutResponse Pydantic model in the breeze buddy auth schemas with success, message, and revoked fields; configure the logout route to use it as response_model, and update logout_handler to return LogoutResponse instead of dict while preserving the existing response values.Source: Coding guidelines
tests/test_auth_rate_limit.py (1)
97-104: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a test for the documented "0 disables the cap" behaviour.
.env.exampleandapp/core/config/static.pyboth state that a cap of0disables that dimension. No test pins that contract. Ifcheck_rate_limittreatslimit=0as "deny everything", setting the variable to0locks every user out of login. Add a test that setsrl.AUTH_RATE_LIMIT_PER_IP_PER_HOURto0and asserts noHTTPException.🤖 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_auth_rate_limit.py` around lines 97 - 104, Add a test alongside test_fails_open_when_redis_unconfigured that sets rl.AUTH_RATE_LIMIT_PER_IP_PER_HOUR to 0, invokes rl.enforce_credential_rate_limit with a request and username, and asserts no HTTPException is raised, preserving the documented disabled-cap behavior.app/schemas/breeze_buddy/auth.py (1)
98-102: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
token_lifetime_daysdefaults to the maximum on both S2S mint paths. Both schemas reuseMAX_S2S_TOKEN_LIFETIME_DAYSas the field default and as the upper bound, so a request that omits the field receives the longest permitted token. Keep the cap and lower the default so a long-lived token becomes an explicit choice.
app/schemas/breeze_buddy/auth.py#L98-L102: set a shorterdefault(for example 30) onS2STokenRequest.token_lifetime_daysand keeple=MAX_S2S_TOKEN_LIFETIME_DAYS.app/schemas/breeze_buddy/merchants.py#L45-L52: apply the same shorter default onMerchantCreate.token_lifetime_days; this endpoint is reachable by resellers, so the default has wider reach. Update the assertion intests/test_token_lifetime_revocation.pythat compares the two defaults, which continues to hold if both read the same new constant.🤖 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/schemas/breeze_buddy/auth.py` around lines 98 - 102, Change the token_lifetime_days default used by S2STokenRequest in app/schemas/breeze_buddy/auth.py:98-102 and MerchantCreate in app/schemas/breeze_buddy/merchants.py:45-52 to the same shorter default, such as 30, while retaining le=MAX_S2S_TOKEN_LIFETIME_DAYS. Update the assertion in tests/test_token_lifetime_revocation.py to continue comparing both schemas against the shared new default.
🤖 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/api/routers/breeze_buddy/auth/__init__.py`:
- Around line 211-230: Update logout_user’s route documentation to describe
authentication failures, including revoked-token repeat attempts, as completed
sign-out conditions for clients. Add the {"success": false, "revoked": false}
response returned by logout_handler, and explicitly document that clients should
clear stored credentials/UI even when the endpoint responds with 401.
In `@app/api/routers/breeze_buddy/auth/handlers.py`:
- Around line 447-455: Update the token revocation flow around the payload
parsing and revoke_token call to require a usable exp claim before revoking.
Reject tokens with a missing, non-integer, or otherwise invalid expiry instead
of defaulting to 0, and ensure the handler does not report revoked=true unless
denylisting actually occurred.
- Around line 456-458: Update the exception handler in the logout flow to
replace the interpolated exception in the logger.error message with Loguru’s
exception-aware logger.opt(exception=e).error(...) call using a static message,
while preserving revoked = False.
In `@app/api/routers/breeze_buddy/auth/rate_limit.py`:
- Around line 112-119: Update the warning in the user-decision rate-limit branch
to log a bounded prefix of the existing SHA-256 username digest instead of
username[:64], while preserving the count, limit, and retry behavior. Reuse the
digest symbol already available in the surrounding rate-limit logic.
In `@app/api/routers/breeze_buddy/signup/__init__.py`:
- Around line 84-89: Update select_account to accept the HTTP Request, call
enforce_credential_rate_limit before invoking select_account_handler for
password verification, and use the target account identifier for the secondary
rate-limit bucket while preserving the existing per-IP limit.
In `@app/api/routers/breeze_buddy/signup/handlers.py`:
- Around line 476-481: Update the failed account-list handling in the matched
check to stop interpolating the submitted email into logs. Replace the warning
with a fixed event message, using the existing Loguru/contextvars correlation
context for diagnosis, while preserving the HTTPException response unchanged.
In `@app/api/routers/breeze_buddy/widget_common.py`:
- Around line 155-162: Update check_rate_limit, used by the aggregate bucket
call, to make the counter increment and TTL assignment atomic, preferably via a
Redis script or equivalent atomic command. Preserve fail_closed behavior while
ensuring a successful increment cannot remain without its window expiration; if
atomicity is unavailable, repair or delete the key when TTL setup fails.
- Around line 147-158: The aggregate limiter currently creates separate caps per
action through the bucket suffix, so it does not enforce one total widget spend
limit. Update _enforce_widget_aggregate_limit and its callers to use a shared
aggregate bucket for each widget_config_id, and ensure the chosen shared limit
clearly reflects how per-action limits contribute to the total cap. If separate
action budgets are intentional instead, revise the PT-18 comments/docstrings to
describe independent limits rather than total spend.
In `@app/database/accessor/breeze_buddy/users.py`:
- Around line 156-161: Update the user-active lookup around get_user_in_db_by_id
so database exceptions are logged and then re-raised (or return False), never
converted to an active-user result; preserve fail-open behavior only for Redis
failures, and add a test verifying a database failure rejects the token.
In `@app/schemas/breeze_buddy/users.py`:
- Around line 80-87: Update the user update handler that persists UserUpdate
changes to load the target user before applying the update, then validate the
new password against the stored username, ID, existing email local-part, and new
email local-part. Adjust _validate_password_policy or bypass its insufficient
context so password-only updates still use the persisted account identifiers
while preserving validation for non-password updates.
In `@tests/test_auth_enumeration.py`:
- Around line 38-41: Update the pytest.raises assertion around
ListAccountsRequest(email="victim@company.com") to expect Pydantic’s specific
ValidationError rather than the broad Exception, while leaving the valid
password and id_token cases unchanged.
In `@tests/test_auth_rate_limit.py`:
- Around line 40-44: Ensure pytest executes the async tests rather than
collecting unawaited coroutines: first confirm asyncio_mode = auto in the pytest
configuration; if it is absent, apply pytest.mark.asyncio at module level or to
all seven async tests in tests/test_auth_rate_limit.py, including
test_blocks_when_ip_over_cap, and to test_revoke_then_is_revoked and
test_is_token_revoked_fails_open_on_redis_error in
tests/test_token_lifetime_revocation.py. Both affected sites require the same
marker/configuration change.
---
Nitpick comments:
In `@app/api/routers/breeze_buddy/auth/handlers.py`:
- Line 436: Define a LogoutResponse Pydantic model in the breeze buddy auth
schemas with success, message, and revoked fields; configure the logout route to
use it as response_model, and update logout_handler to return LogoutResponse
instead of dict while preserving the existing response values.
In `@app/api/routers/breeze_buddy/auth/rate_limit.py`:
- Line 46: Move the client_ip helper from widget_common into a neutral shared
request-utility module, then update the auth rate limiter and chat/demo.py
imports to use the new location. Remove the old widget_common import or
re-export only if existing consumers require compatibility, while preserving
client_ip behavior.
- Around line 105-111: Update the authentication flow around check_rate_limit so
the credential_user bucket increments only after a failed authentication, or is
reset/decremented after successful authentication, preventing attackers from
locking out known usernames. Preserve the per-IP protection, and add a 429
metric labeled by the rate-limit bucket so operators can distinguish username
and IP throttling.
In `@app/api/routers/breeze_buddy/widget_common.py`:
- Around line 243-249: Update the surrounding widget rate-limit docstrings near
the per-IP enforcement and _enforce_widget_aggregate_limit call to document both
denial paths: the per-IP limit and cross-IP aggregate limit for the merchant’s
public_widget_key. State that either limit can return HTTP 429 and that
Retry-After applies in both cases.
In `@app/core/config/static.py`:
- Around line 550-555: Update the AUTH_RATE_LIMIT_PER_IP_PER_HOUR and
AUTH_RATE_LIMIT_PER_USERNAME_PER_HOUR configuration parsing to use the
surrounding shared integer-environment helper if available, falling back to the
documented defaults for empty or invalid values and clamping negative results to
zero. Preserve the existing defaults of 40 and 15 respectively.
In `@app/core/security/token_revocation.py`:
- Around line 35-46: In app/core/security/token_revocation.py lines 35-46,
update both exception handlers in the token revocation functions to use
logger.opt(exception=e).error(...) with structured message arguments instead of
interpolating e. Apply the same exception-aware logging change to each
interpolated exception message in app/database/accessor/breeze_buddy/users.py
lines 153-180; preserve the existing handler behavior and log context.
In `@app/schemas/breeze_buddy/auth.py`:
- Around line 98-102: Change the token_lifetime_days default used by
S2STokenRequest in app/schemas/breeze_buddy/auth.py:98-102 and MerchantCreate in
app/schemas/breeze_buddy/merchants.py:45-52 to the same shorter default, such as
30, while retaining le=MAX_S2S_TOKEN_LIFETIME_DAYS. Update the assertion in
tests/test_token_lifetime_revocation.py to continue comparing both schemas
against the shared new default.
In `@tests/test_auth_rate_limit.py`:
- Around line 97-104: Add a test alongside
test_fails_open_when_redis_unconfigured that sets
rl.AUTH_RATE_LIMIT_PER_IP_PER_HOUR to 0, invokes
rl.enforce_credential_rate_limit with a request and username, and asserts no
HTTPException is raised, preserving the documented disabled-cap 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: 3d8f86f8-f233-4bf3-962d-b200aaef2c88
📒 Files selected for processing (28)
.env.exampleapp/api/routers/breeze_buddy/auth/__init__.pyapp/api/routers/breeze_buddy/auth/handlers.pyapp/api/routers/breeze_buddy/auth/rate_limit.pyapp/api/routers/breeze_buddy/chat/demo.pyapp/api/routers/breeze_buddy/signup/__init__.pyapp/api/routers/breeze_buddy/signup/handlers.pyapp/api/routers/breeze_buddy/stt/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/password/__init__.pyapp/core/security/password/password.pyapp/core/security/password/password_policy.pyapp/core/security/token_revocation.pyapp/database/accessor/breeze_buddy/users.pyapp/schemas/breeze_buddy/auth.pyapp/schemas/breeze_buddy/merchants.pyapp/schemas/breeze_buddy/signup.pyapp/schemas/breeze_buddy/users.pytests/test_auth_enumeration.pytests/test_auth_rate_limit.pytests/test_password_policy.pytests/test_stt_stream.pytests/test_token_lifetime_revocation.py
| async def logout_user( | ||
| credentials: HTTPAuthorizationCredentials = Depends(security), | ||
| current_user: UserInfo = Depends(get_current_user_with_rbac), | ||
| ): | ||
| """ | ||
| Logout endpoint for JWT token-based authentication. | ||
| Log out a JWT-authenticated user. | ||
|
|
||
| Since JWT tokens are stateless and stored client-side: | ||
| - Backend cannot invalidate the token (no session to destroy) | ||
| - Client must delete the token from localStorage/cookies | ||
| - Token will naturally expire after its lifetime | ||
|
|
||
| This endpoint exists for: | ||
| - API consistency (REST convention) | ||
| - Future enhancements (e.g., token blacklisting) | ||
| - Logging logout events | ||
|
|
||
| Client-side logout steps: | ||
| 1. Call this endpoint (optional, for logging) | ||
| 2. Remove token from localStorage/cookies | ||
| 3. Redirect to login page | ||
| 4. Clear any user state in application | ||
| The presented token is added to the server-side revocation denylist (keyed | ||
| by a hash of the token, with a TTL equal to its remaining lifetime), so it | ||
| can no longer authenticate even though its signature stays valid until its | ||
| natural expiry (PT-22). Clients should still discard their stored copy. | ||
|
|
||
| Returns: | ||
| { | ||
| "success": true, | ||
| "message": "Logout acknowledged. Client should clear token from storage.", | ||
| "instructions": { | ||
| "step_1": "Remove token from localStorage or cookies", | ||
| "step_2": "Clear user state in your application", | ||
| "step_3": "Redirect to login page", | ||
| "note": "Token remains valid until expiration but client discards it" | ||
| } | ||
| "message": "Logout successful. Token has been revoked server-side.", | ||
| "revoked": true | ||
| } | ||
|
|
||
| Note: | ||
| The actual logout happens client-side by removing the token. | ||
| The token remains technically valid until expiration, but the client | ||
| discards it and can no longer use it. | ||
| """ | ||
| return await logout_handler() | ||
| return await logout_handler(credentials.credentials) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers of the logout endpoint and check how they handle non-2xx responses.
set -euo pipefail
rg -n -C4 --iglob '!**/node_modules/**' '/auth/logout' . | head -60Repository: juspay/clairvoyance
Length of output: 2533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant symbols and inspect the auth router plus likely logout handler/security dependencies.
printf '--- relevant files ---\n'
fd -a '^(auth|jwt|rbac_token|logout_handler|logout).*\.py$|auth/__init__.py$' app | sed 's#^\./##' | sort
printf '\n--- auth router outline ---\n'
ast-grep outline app/api/routers/breeze_buddy/auth/__init__.py --view expanded | sed -n '1,220p'
printf '\n--- logout_user area ---\n'
sed -n '1,280p' app/api/routers/breeze_buddy/auth/__init__.py
printf '\n--- logout_handler definitions/usages ---\n'
rg -n -C3 --iglob '!**/node_modules/**' '(def\s+logout_handler|async\s+def\s+logout_handler|logout_handler\s*=|\blogout_handler\b)' app
printf '\n--- security and get_current_user_with_rbac definitions ---\n'
rg -n -C4 --iglob '!**/node_modules/**' '(def\s+security|async\s+def\s+security|\bsecurity\s*|\bget_current_user_with_rbac\b|get_current_user_with_rbac\s*=|\bDependencies\s*=)' app/api app/core | head -220Repository: juspay/clairvoyance
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- relevant files ---'
fd '^(auth|jwt|rbac_token|logout_handler|logout).*\.py$|auth/__init__.py$' app | sed 's#^\./##' | sort
echo
echo '--- auth router outline ---'
ast-grep outline app/api/routers/breeze_buddy/auth/__init__.py --view expanded | sed -n '1,240p'
echo
echo '--- logout_user area ---'
sed -n '1,280p' app/api/routers/breeze_buddy/auth/__init__.py
echo
echo '--- logout_handler definitions/usages ---'
rg -n -C3 --iglob '!**/node_modules/**' '(def\s+logout_handler|async\s+def\s+logout_handler|logout_handler\s*=|\blogout_handler\b)' app || true
echo
echo '--- security and get_current_user_with_rbac definitions ---'
rg -n -C4 --iglob '!**/node_modules/**' '(def\s+security|async\s+def\s+security|\bsecurity\s*|\bget_current_user_with_rbac\b|get_current_user_with_rbac\s*=|\bDependencies\s*=)' app/api app/core || trueRepository: juspay/clairvoyance
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- logout handler ---'
sed -n '400,520p' app/api/routers/breeze_buddy/auth/handlers.py
echo
echo '--- RBAC token dependency relevant sections ---'
sed -n '200,345p' app/api/security/breeze_buddy/rbac_token.py
echo
echo '--- token revocation helpers ---'
fd -a 'token_revocation.py|revok' app/core/app | sed 's#^\./##'
rg -n -C4 --iglob '!**/node_modules/**' 'def\s+is_token_revoked|async\s+def\s+is_token_revoked|revok' app/core/security/token_revocation.py || true
sed -n '1,220p' app/core/security/token_revocation.py
echo
echo '--- repository logout endpoint callers (frontend/UI/text references) ---'
rg -n --iglob '!**/node_modules/**' 'auth/logout|/auth/logout|logout\(' . | head -120Repository: juspay/clairvoyance
Length of output: 8607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- token revocation source ---'
rg -n -C4 --iglob '!**/node_modules/**' 'def\s+revoke_token|async\s+def\s+revoke_token|is_token_revoked|RedisService\.setex|redis_set' app/core app/api | head -220
sed -n '1,220p' app/core/security/token_revocation.py || true
echo
echo '--- repository references to auth/logout/logout handlers ---'
rg -n --iglob '!**/node_modules/**' --glob '!**/.venv/**' --glob '!**/dist/**' --glob '!**/build/**' 'auth/logout|/auth/logout|breeze_buddy/auth/handlers|POST /auth/logout|logout_handler' . | head -160Repository: juspay/clairvoyance
Length of output: 6479
Treat POST /auth/logout as authenticated and handle its denial responses as completed sign-out.
logout_user requires a bearer token through Depends(security) and get_current_user_with_rbac, so auth failures before logout_handler run now return 401. The get_current_user_with_rbac dependency also rejects revoked tokens, so a repeat logout with an already-revoked token is denied. The frontend should not fail logout; it should still clear the token/UI.
Document the deny responses in the route docs, or update the client contract that 401 means logout completion. The docstring should also list the {"success": false, "revoked": false} case from logout_handler.
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 212-212: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 213-213: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 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 211 - 230, Update
logout_user’s route documentation to describe authentication failures, including
revoked-token repeat attempts, as completed sign-out conditions for clients. Add
the {"success": false, "revoked": false} response returned by logout_handler,
and explicitly document that clients should clear stored credentials/UI even
when the endpoint responds with 401.
| try: | ||
| payload = pyjwt.decode( | ||
| token, | ||
| rbac_token_manager.jwt_manager.secret_key, | ||
| algorithms=[rbac_token_manager.jwt_manager.algorithm], | ||
| options={"verify_exp": False}, | ||
| ) | ||
| exp = int(payload.get("exp", 0)) | ||
| revoked = await revoke_token(token, exp) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
A token without an exp claim reports a false revocation.
payload.get("exp", 0) yields 0 when the claim is absent. revoke_token then computes a negative TTL and returns True on its "already expired — nothing to revoke" path. The handler treats that as success and responds with "revoked": true, but the token was never written to the denylist. A token with no expiry is exactly the token that must be denylisted, and it stays valid forever.
The handler comment states the intent: do not claim a revocation that did not happen. Reject a token that carries no usable exp instead of assuming zero.
🔒️ Proposed fix
exp = int(payload.get("exp", 0))
- revoked = await revoke_token(token, exp)
+ if exp <= 0:
+ # No usable expiry: revoke_token would short-circuit on a negative
+ # TTL and report success without writing the denylist entry.
+ raise ValueError("token has no exp claim; cannot bound denylist TTL")
+ revoked = await revoke_token(token, exp)📝 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.
| try: | |
| payload = pyjwt.decode( | |
| token, | |
| rbac_token_manager.jwt_manager.secret_key, | |
| algorithms=[rbac_token_manager.jwt_manager.algorithm], | |
| options={"verify_exp": False}, | |
| ) | |
| exp = int(payload.get("exp", 0)) | |
| revoked = await revoke_token(token, exp) | |
| try: | |
| payload = pyjwt.decode( | |
| token, | |
| rbac_token_manager.jwt_manager.secret_key, | |
| algorithms=[rbac_token_manager.jwt_manager.algorithm], | |
| options={"verify_exp": False}, | |
| ) | |
| exp = int(payload.get("exp", 0)) | |
| if exp <= 0: | |
| # No usable expiry: revoke_token would short-circuit on a negative | |
| # TTL and report success without writing the denylist entry. | |
| raise ValueError("token has no exp claim; cannot bound denylist TTL") | |
| revoked = await revoke_token(token, exp) |
🤖 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/handlers.py` around lines 447 - 455, Update
the token revocation flow around the payload parsing and revoke_token call to
require a usable exp claim before revoking. Reject tokens with a missing,
non-integer, or otherwise invalid expiry instead of defaulting to 0, and ensure
the handler does not report revoked=true unless denylisting actually occurred.
| except Exception as e: | ||
| logger.error(f"Logout: failed to revoke token: {e}") | ||
| revoked = False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use Loguru's exception-aware logging.
Line 457 interpolates the exception object into the message string. Exception text can contain formatting braces, which breaks Loguru formatting. Use logger.opt(exception=e).error(...) with a static message.
♻️ Proposed fix
except Exception as e:
- logger.error(f"Logout: failed to revoke token: {e}")
+ logger.opt(exception=e).error("Logout: failed to revoke token")
revoked = FalseBased on learnings: in the Clairvoyance Python codebase, 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.
📝 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.
| except Exception as e: | |
| logger.error(f"Logout: failed to revoke token: {e}") | |
| revoked = False | |
| except Exception as e: | |
| logger.opt(exception=e).error("Logout: failed to revoke token") | |
| revoked = False |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 456-456: Do not catch blind exception: Exception
(BLE001)
🤖 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/handlers.py` around lines 456 - 458, Update
the exception handler in the logout flow to replace the interpolated exception
in the logger.error message with Loguru’s exception-aware
logger.opt(exception=e).error(...) call using a static message, while preserving
revoked = False.
Source: Learnings
| if not user_decision.allowed: | ||
| # Log a bounded prefix of the raw username (not the hash) for ops | ||
| # triage without letting a huge identifier bloat the log line. | ||
| logger.warning( | ||
| f"auth rate limit hit (per-username " | ||
| f"{user_decision.count}/{user_decision.limit}) for {username[:64]!r}" | ||
| ) | ||
| raise _too_many(user_decision.retry_after_seconds) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not log the raw username in the rate-limit warning.
Line 117 writes a 64-character prefix of the raw identifier to the log. On the /signup and /auth/accounts paths that identifier is the user's email address (see app/api/routers/breeze_buddy/signup/__init__.py:84-91), so this places email addresses in application logs. Log the SHA-256 digest prefix instead. Operators can still correlate repeated hits on the same bucket, and the log holds no personal data.
🔒️ Proposed fix
if not user_decision.allowed:
- # Log a bounded prefix of the raw username (not the hash) for ops
- # triage without letting a huge identifier bloat the log line.
+ # Log a bounded prefix of the *hashed* identifier. It is stable across
+ # requests, so ops can correlate repeated hits on one bucket, and no
+ # username or email address reaches the logs.
logger.warning(
f"auth rate limit hit (per-username "
- f"{user_decision.count}/{user_decision.limit}) for {username[:64]!r}"
+ f"{user_decision.count}/{user_decision.limit}) for "
+ f"identifier hash {username_key[:16]}"
)📝 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.
| if not user_decision.allowed: | |
| # Log a bounded prefix of the raw username (not the hash) for ops | |
| # triage without letting a huge identifier bloat the log line. | |
| logger.warning( | |
| f"auth rate limit hit (per-username " | |
| f"{user_decision.count}/{user_decision.limit}) for {username[:64]!r}" | |
| ) | |
| raise _too_many(user_decision.retry_after_seconds) | |
| if not user_decision.allowed: | |
| # Log a bounded prefix of the *hashed* identifier. It is stable across | |
| # requests, so ops can correlate repeated hits on one bucket, and no | |
| # username or email address reaches the logs. | |
| logger.warning( | |
| f"auth rate limit hit (per-username " | |
| f"{user_decision.count}/{user_decision.limit}) for " | |
| f"identifier hash {username_key[:16]}" | |
| ) | |
| raise _too_many(user_decision.retry_after_seconds) |
🤖 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/rate_limit.py` around lines 112 - 119,
Update the warning in the user-decision rate-limit branch to log a bounded
prefix of the existing SHA-256 username digest instead of username[:64], while
preserving the count, limit, and retry behavior. Reuse the digest symbol already
available in the surrounding rate-limit logic.
| async def list_accounts( | ||
| request: ListAccountsRequest, http_request: Request | ||
| ) -> AccountsResponse: | ||
| await enforce_credential_rate_limit(http_request, request.email) | ||
| accounts = await list_accounts_handler( | ||
| id_token=request.id_token, email=request.email | ||
| id_token=request.id_token, email=request.email, password=request.password |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Rate-limit password-based account selection.
This change rate-limits account listing, but select_account still sends a supplied password to select_account_handler without a credential limit. A caller can repeatedly invoke that endpoint against target account IDs and bypass the new protection.
Accept Request in select_account. Call enforce_credential_rate_limit before password verification. Preserve the per-IP limit and use the target identifier for the secondary bucket.
🤖 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/signup/__init__.py` around lines 84 - 89, Update
select_account to accept the HTTP Request, call enforce_credential_rate_limit
before invoking select_account_handler for password verification, and use the
target account identifier for the secondary rate-limit bucket while preserving
the existing per-IP limit.
| decision = await check_rate_limit( | ||
| bucket=f"{bucket}:agg", | ||
| identifier=widget_config_id, | ||
| limit=limit, | ||
| window_seconds=_RATE_WINDOW_SECONDS, | ||
| prefix=_RATE_LIMIT_PREFIX, | ||
| fail_closed=True, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 15 \
'async def check_rate_limit|redis\.incr|redis\.expire|fail_closed' \
app/services/redis/rate_limit.pyRepository: juspay/clairvoyance
Length of output: 4419
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== redis/rate_limit.py full relevant implementation =="
cat -n app/services/redis/rate_limit.py | sed -n '1,180p'
echo
echo "== widget_common rate limit call context =="
cat -n app/api/routers/breeze_buddy/widget_common.py | sed -n '130,175p'
echo
echo "== check_rate_limit call sites =="
rg -n "check_rate_limit\\(" appRepository: juspay/clairvoyance
Length of output: 9124
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files mentioning expireat/incrby/nx/SET=1 rate limiting =="
rg -n "expireat|INCRBY|LIMIT|NX|SETNX|rate limit|ratelimit|ttl|expire" app/services/redis app/api/routers app -g '*.py' | sed -n '1,220p'
echo
echo "== inspect redis wrapper methods if present =="
fd -a 'redis.*\.py$|.*redis.*\.py$' app | sed -n '1,120p'Repository: juspay/clairvoyance
Length of output: 28833
Make the rate-limit increment and TTL write atomic.
check_rate_limit() increments the bucket key and later calls expire() separately. If INCR succeeds and EXPIRE fails, the key is created without TTL and fail_closed=True returns allowed=False. Subsequent calls cannot set TTL because the count is no longer 1, so the widget aggregate bucket can stay blocked for the rest of its window. Use an atomic script/command for increment plus TTL, or repair/delete the key when TTL setup fails.
🤖 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/widget_common.py` around lines 155 - 162, Update
check_rate_limit, used by the aggregate bucket call, to make the counter
increment and TTL assignment atomic, preferably via a Redis script or equivalent
atomic command. Preserve fail_closed behavior while ensuring a successful
increment cannot remain without its window expiration; if atomicity is
unavailable, repair or delete the key when TTL setup fails.
| try: | ||
| user = await get_user_in_db_by_id(user_id) | ||
| active = bool(user and user.is_active) | ||
| except Exception as e: | ||
| logger.error(f"user-active DB lookup failed for {user_id} (failing open): {e}") | ||
| return True |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject authentication when the database lookup fails.
Line 161 returns True after a database failure. This accepts tokens for disabled or deleted users while the database is unavailable. Only Redis failures should fail open. Re-raise the database error or return False, and add a test that verifies a database failure rejects the token.
Proposed fix
except Exception as e:
- logger.error(f"user-active DB lookup failed for {user_id} (failing open): {e}")
- return True
+ logger.opt(exception=e).error(
+ "user-active DB lookup failed for {}",
+ user_id,
+ )
+ raiseBased on learnings: accessor database exceptions must be logged and re-raised rather than converted into valid responses.
📝 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.
| try: | |
| user = await get_user_in_db_by_id(user_id) | |
| active = bool(user and user.is_active) | |
| except Exception as e: | |
| logger.error(f"user-active DB lookup failed for {user_id} (failing open): {e}") | |
| return True | |
| try: | |
| user = await get_user_in_db_by_id(user_id) | |
| active = bool(user and user.is_active) | |
| except Exception as e: | |
| logger.opt(exception=e).error( | |
| "user-active DB lookup failed for {}", | |
| user_id, | |
| ) | |
| raise |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 159-159: Do not catch blind exception: Exception
(BLE001)
🤖 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/database/accessor/breeze_buddy/users.py` around lines 156 - 161, Update
the user-active lookup around get_user_in_db_by_id so database exceptions are
logged and then re-raised (or return False), never converted to an active-user
result; preserve fail-open behavior only for Redis failures, and add a test
verifying a database failure rejects the token.
Source: Learnings
| @model_validator(mode="after") | ||
| def _validate_password_policy(self) -> "UserUpdate": | ||
| if self.password is not None: | ||
| validate_password_strength( | ||
| self.password, | ||
| disallowed_substrings=[(self.email or "").split("@")[0]], | ||
| ) | ||
| return self |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate persisted account identifiers during password updates.
UserUpdate has no existing username, ID, or email. A password-only update therefore checks no account identifier. An update that also changes email checks only the new email.
Load the target user in the update handler. Validate the new password against the stored username, ID, existing email local-part, and new email local-part before persistence.
🤖 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/schemas/breeze_buddy/users.py` around lines 80 - 87, Update the user
update handler that persists UserUpdate changes to load the target user before
applying the update, then validate the new password against the stored username,
ID, existing email local-part, and new email local-part. Adjust
_validate_password_policy or bypass its insufficient context so password-only
updates still use the persisted account identifiers while preserving validation
for non-password updates.
| with pytest.raises(Exception): | ||
| ListAccountsRequest(email="victim@company.com") # no password | ||
| ListAccountsRequest(email="v@c.com", password="x") # ok | ||
| ListAccountsRequest(id_token="tok") # ok |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)tests/test_auth_enumeration\.py$|ListAccountsRequest|requests|models|schemas|core/config/static\.py' || true
echo
echo "== file outline/size =="
if [ -f tests/test_auth_enumeration.py ]; then
wc -l tests/test_auth_enumeration.py
ast-grep outline tests/test_auth_enumeration.py || true
echo
echo "== test file relevant section =="
cat -n tests/test_auth_enumeration.py | sed -n '1,120p'
fi
echo
echo "== ListAccountsRequest definitions/usages =="
rg -n "class ListAccountsRequest|ListAccountsRequest\(|ListAccountsRequest" .Repository: juspay/clairvoyance
Length of output: 7777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== signup schema relevant section =="
cat -n app/schemas/breeze_buddy/signup.py | sed -n '1,240p'
echo
echo "== parse ListAccountsRequest validators =="
python3 - <<'PY'
from pathlib import Path
from pydantic import BaseModel
# Import does not require a running dependency tree if pydantic is available in the sandbox;
# otherwise this will fail and be useful for the developer.
import importlib.util
spec = importlib.util.find_spec("app.schemas.breeze_buddy.signup")
print("module_found=", bool(spec))
if spec:
module = importlib.util.module_from_spec(spec)
spec.loader.exec_module(module)
cls = module.ListAccountsRequest
print("fields=", list(cls.model_fields.keys()))
print("model_fields=", {
name: {"type": str(field.annotation), "is_required": field.is_required, "default": field.default}
for name, field in cls.model_fields.items()
})
try:
r = cls(email="a@b.com")
print("email_only=", r)
except Exception as exc:
print("email_only_exception_type=", type(exc).__module__ + "." + type(exc).__qualname__)
print("email_only_exception=", repr(exc))
try:
r = cls(email="a@b.com", password="pw")
print("email_password_valid=", r)
except Exception as exc:
print("email_password_exception=", type(exc).__module__ + "." + type(exc).__qualname__, repr(exc))
try:
r = cls(id_token="tok")
print("id_token_valid=", r)
except Exception as exc:
print("id_token_exception=", type(exc).__module__ + "." + type(exc).__qualname__, repr(exc))
PYRepository: juspay/clairvoyance
Length of output: 9188
Assert the expected validation error.
ListAccountsRequest._require_proof raises a Pydantic validation error for email requests missing password, so assert ValidationError instead of the broad Exception.
Proposed fix
from fastapi import HTTPException
+from pydantic import ValidationError
@@
- with pytest.raises(Exception):
+ with pytest.raises(ValidationError):📝 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.
| with pytest.raises(Exception): | |
| ListAccountsRequest(email="victim@company.com") # no password | |
| ListAccountsRequest(email="v@c.com", password="x") # ok | |
| ListAccountsRequest(id_token="tok") # ok | |
| with pytest.raises(ValidationError): | |
| ListAccountsRequest(email="victim@company.com") # no password | |
| ListAccountsRequest(email="v@c.com", password="x") # ok | |
| ListAccountsRequest(id_token="tok") # ok |
🧰 Tools
🪛 Ruff (0.16.1)
[warning] 38-38: Do not assert blind exception: Exception
(B017)
[error] 40-40: Possible hardcoded password assigned to argument: "password"
(S106)
[error] 41-41: Possible hardcoded password assigned to argument: "id_token"
(S106)
🤖 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_auth_enumeration.py` around lines 38 - 41, Update the
pytest.raises assertion around ListAccountsRequest(email="victim@company.com")
to expect Pydantic’s specific ValidationError rather than the broad Exception,
while leaving the valid password and id_token cases unchanged.
Source: Linters/SAST tools
| async def test_blocks_when_ip_over_cap(monkeypatch): | ||
| seen: list = [] | ||
| monkeypatch.setattr(rl, "check_rate_limit", _fake_check({"credential_ip"}, seen)) | ||
| with pytest.raises(HTTPException) as e: | ||
| await rl.enforce_credential_rate_limit(_req(), "alice") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Both new test files declare async tests with no asyncio marker. Every async test in these two files relies on pytest-asyncio (or anyio) auto mode being configured. If auto mode is not set, pytest collects the coroutines, emits a warning, and never runs the assertions, so the suite reports a pass for tests that executed no code. The PR reports 951 passing tests, which does not distinguish executed tests from skipped coroutines.
tests/test_auth_rate_limit.py#L40-L44: confirmasyncio_mode = autoin the pytest configuration; otherwise add@pytest.mark.asyncioto all seven async tests in this file, or applypytestmark = pytest.mark.asyncioat module level.tests/test_token_lifetime_revocation.py#L92-L113: apply the same marker totest_revoke_then_is_revokedandtest_is_token_revoked_fails_open_on_redis_error.
#!/bin/bash
# Description: Check the configured pytest asyncio mode and the convention in existing async tests.
set -euo pipefail
fd -t f -d 2 'pytest.ini|pyproject.toml|setup.cfg|tox.ini' --exec rg -n 'asyncio_mode|anyio|\[tool\.pytest|asyncio' {}
echo '=== root conftest ==='
fd -t f 'conftest.py' -d 2 --exec rg -n 'asyncio|anyio|event_loop' {}
echo '=== marker usage in existing tests ==='
rg -c 'pytest.mark.asyncio' tests --glob '*.py' | head -20📍 Affects 2 files
tests/test_auth_rate_limit.py#L40-L44(this comment)tests/test_token_lifetime_revocation.py#L92-L113
🤖 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_auth_rate_limit.py` around lines 40 - 44, Ensure pytest executes
the async tests rather than collecting unawaited coroutines: first confirm
asyncio_mode = auto in the pytest configuration; if it is absent, apply
pytest.mark.asyncio at module level or to all seven async tests in
tests/test_auth_rate_limit.py, including test_blocks_when_ip_over_cap, and to
test_revoke_then_is_revoked and test_is_token_revoked_fails_open_on_redis_error
in tests/test_token_lifetime_revocation.py. Both affected sites require the same
marker/configuration change.
Evidence — three findings, three recordingsA real screencast — Chrome DevTools Each take runs three acts: attack PT-24 — password policypr996a-password-policy.mp4local original: All four are 8+ characters on purpose — the schema already enforces PT-16/18/19 — credential guessingpr996b-rate-limit.mp4local original: Driven through the real login form. On the baseline: 60 guesses at one account, all 401, no cap ever engages, and the real password then logs in (200). On this branch attempts 1–15 return 401, attempt 16 returns 429, and then the correct password is refused too — a lockout, not a credential check. The regression check matters here: PT-21/22 — token revocationpr996c-token-revocation.mp4local original: A fresh login still works afterwards and other sessions are untouched. One wrinkle worth recording: tokens carry no 951 tests pass on this branch. Rate limiting and the liveness recheck both fail open on a Redis outage — deliberate and bounded.
|
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the 28 changed files in this security-hardening PR. I did not duplicate points already raised by Copilot/CodeRabbit (token-revocation exp=0 handling, UserUpdate password-policy context, is_user_active DB fail-open, widget aggregate-bucket semantics, Redis rate-limit atomicity, async test markers, broad pytest.raises(Exception), etc.).
New issues raised in this review
| Severity | Count | Topic |
|---|---|---|
| 🔒 CRITICAL | 3 | PII logged in new failure/rate-limit paths |
| 1 | bcrypt hash computed at module import time |
🔒 CRITICAL — PII in logs
app/api/routers/breeze_buddy/auth/handlers.py: new failed-login and failed-S2S logs interpolate the rawusername(documented as the user's email address). Email addresses are PII and must not reach application logs.app/api/routers/breeze_buddy/auth/rate_limit.py: new per-IP rate-limit log interpolates the client IP address. IPs are personal data and must not be logged verbatim.
Replace these with fixed event messages and rely on request correlation context (or one-way hash prefixes if ops correlation is required).
app/core/security/password/password.py:DUMMY_PASSWORD_HASH = hash_password(...)runs a full bcrypt hash at module import. At cost 12 this is ~240 ms per worker cold start, which delays readiness and can trip health checks. Use a precomputed valid bcrypt hash constant instead; verification still costs the same, but the one-time work is paid at authoring/build time.
Next steps
- Redact the email/username and IP values from the new log lines.
- Precompute
DUMMY_PASSWORD_HASHas a constant. - Address the existing unresolved comments from Copilot/CodeRabbit (especially the
exp=0revocation short-circuit and theUserUpdatedeny-list context).
Once the above are resolved, this PR can be approved.
| raise _too_many(ip_decision.retry_after_seconds) | ||
|
|
||
| username = (identifier or "").strip().lower() | ||
| if not username: |
There was a problem hiding this comment.
🔒 CRITICAL: This log line records the client IP address on a rate-limit hit. IP addresses are PII and must not be logged verbatim. Log a fixed event message (e.g. auth rate limit hit (per-IP)) and use request correlation context for triage; if per-bucket correlation is required, log a one-way hash prefix of the IP.
logger.warning(
f"auth rate limit hit (per-IP {ip_decision.count}/{ip_decision.limit})"
)| await verify_password_async(login_request.password, DUMMY_PASSWORD_HASH) | ||
| logger.warning(f"Failed login attempt for unknown user: {login_request.username}") | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, |
There was a problem hiding this comment.
🔒 CRITICAL: This log line writes the raw username (which is the user's email address in this flow) to application logs on every failed login. Email addresses are PII and must not be logged. Replace with a fixed event message and rely on request correlation context for ops triage, or log a one-way hash prefix if correlation requires it.
logger.warning("Failed login attempt for unknown user")| user = await get_user_by_username(request.username) | ||
| if not user: | ||
| # Spend one bcrypt against a dummy hash before failing so an unknown | ||
| # username is timing-indistinguishable from a wrong password — this path |
There was a problem hiding this comment.
🔒 CRITICAL: This log line writes the raw username (email address) on a failed S2S token request. Email addresses are PII and must not be logged. Use a fixed event message instead.
logger.warning("S2S token request failed: user not found")| ) | ||
| if len(password.encode("utf-8")) > 72: | ||
| # bcrypt silently truncates beyond 72 bytes — reject rather than | ||
| # hash a truncated password (PT-24). |
There was a problem hiding this comment.
DUMMY_PASSWORD_HASH at module import runs a full bcrypt hash (~240ms at cost 12) on every worker/process cold start. In a multi-worker/serverless deployment this noticeably delays readiness and startup. Use a precomputed valid bcrypt hash constant instead; verification still pays the same bcrypt cost, but the one-time hash is paid at build/authoring time rather than at runtime.
# Precomputed with bcrypt.gensalt(rounds=12) from a random plaintext.
# It can never match a real password, but verification costs the same.
DUMMY_PASSWORD_HASH = "$2b$12$..."
bd5c094 to
1dc97c8
Compare
|
Thanks — all four of your findings are fixed, along with the CodeRabbit and Copilot points. Two of them were more than the log-hygiene they looked like, so taking those first. 🔒 PII in logs — fixed, and the hashing had to move earlier than the log lineThe per-IP bucket now hashes the address before it is handed to ip_key = hashlib.sha256(client_ip(request).encode("utf-8")).hexdigest()
ip_decision = await check_rate_limit(bucket="credential_ip", identifier=ip_key, ...)Redacting only my own One correction on scope: the failed-login and failed-S2S lines are not new in this PR — they are on
|
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the 30 changed files in this security-hardening PR. I did not raise any new inline comments because the outstanding points from Copilot/CodeRabbit/Tara-ag already cover the remaining concerns, and the current revision appears to have addressed several of them in code (e.g., DUMMY_PASSWORD_HASH is now a precomputed literal, failed-auth logs no longer interpolate raw usernames/emails, rate-limit logs use hashed identifiers, select_account is rate-limited, logout_handler rejects tokens without a usable exp, and is_user_active fails closed on DB errors).
New issues raised in this review
| Severity | Count |
|---|---|
| 🔒 CRITICAL | 0 |
| 0 | |
| 💡 MINOR | 0 |
| 💬 SUGGESTION | 0 |
Blocking criteria check
- No hardcoded secrets/API keys/credentials in source. ✅
- No SQL injection or non-parameterized asyncpg queries in changed code. ✅
- No auth/authorization bypass or cross-tenant data access introduced. ✅
- No SSRF, command/template injection, or unsafe eval added. ✅
- No PII exposure in new log lines or responses (current revision redacts usernames, emails, and IPs). ✅
- No existing
database/migrations/*.sqlfiles modified. ✅
Notes on existing unresolved comments
The following previously raised threads should still be verified/resolved by the author before merge, but they are not duplicated here:
UserUpdatepassword-policy context (CodeRabbit) — password-only updates currently validate only against the new email local-part, not the persisted username/id/email.S2STokenRequest/MerchantCreatedefault token lifetime of 365 days (CodeRabbit nitpick) — consider whether the default should be shorter than the cap.AUTH_RATE_LIMIT_*env parsing instatic.py(CodeRabbit nitpick) —int(os.environ.get(...))crashes on empty/non-numeric values; consider a tolerant helper.logout_userusingDepends(...)in argument defaults triggers Ruff B008.
Overall, the security posture of the PR is sound and the blocking criteria are not met by any new issues. Approved.
1dc97c8 to
7ce320d
Compare
Rebased onto
|
7ce320d to
67c2d47
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the 30 changed files in this security-hardening PR. I did not raise any new blocking issues; the current revision addresses the critical concerns from earlier review rounds (PII-redacted logs, precomputed dummy bcrypt hash, exp-less token revocation, DB-fail-closed liveness, and select_account rate limiting).
New issues raised in this review
| Severity | Count | Topic |
|---|---|---|
| 🔒 CRITICAL | 0 | |
| 0 | ||
| 💡 MINOR | 3 | UserUpdate password-policy context; AUTH_RATE_LIMIT_* env parsing robustness; Depends(...) in route arg defaults (Ruff B008) |
| 💬 SUGGESTION | 0 |
Blocking criteria check
- No hardcoded secrets/API keys/credentials in source. ✅
- No SQL injection or non-parameterized asyncpg queries in changed code. ✅
- No auth/authorization bypass or cross-tenant data access introduced. ✅
- No SSRF, command/template injection, or unsafe eval added. ✅
- No PII exposure in new log lines or responses. ✅
- No existing
database/migrations/*.sqlfiles modified. ✅
Notes on existing unresolved comments
The following previously raised threads should still be verified/resolved by the author before merge, but they are not duplicated here:
UserUpdatepassword-policy context (CodeRabbit) — password-only updates currently validate only against the new email local-part, not the persisted username/id/email.S2STokenRequest/MerchantCreatedefault token lifetime of 365 days (CodeRabbit nitpick) — consider whether the default should be shorter than the cap.AUTH_RATE_LIMIT_*env parsing instatic.py(CodeRabbit nitpick) —int(os.environ.get(...))crashes on empty/non-numeric values; consider a tolerant helper.logout_userusingDepends(...)in argument defaults triggers Ruff B008.
Overall, the security posture of the PR is sound and the blocking criteria are not met by any new issues. Approved.
| self.password, | ||
| disallowed_substrings=[(self.email or "").split("@")[0]], | ||
| ) | ||
| return self |
There was a problem hiding this comment.
💡 MINOR: UserUpdate._validate_password_policy only checks the new email local-part. For password-only updates it misses the persisted username, id, and existing email, allowing a password that contains the user's own identifiers. Consider passing the loaded user's identifiers into the validator or validating in the update handler after loading the target user.
There was a problem hiding this comment.
💡 MINOR: int(os.environ.get(...)) will crash at import with ValueError if the env var is set to an empty string or a non-numeric value. Use the repo's existing integer-env helper (or add a small _int_env helper) to fall back to the documented defaults and clamp negatives to zero.
|
|
||
| @router.post("/auth/logout") | ||
| async def logout_user(): | ||
| async def logout_user( |
There was a problem hiding this comment.
💡 MINOR: Using Depends(...) in function argument defaults triggers Ruff B008. Define module-level dependency instances (e.g. security_dep = Depends(security)) and reference them in the route signatures, or move the dependency calls inside the function body.
67c2d47 to
6c5959f
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Re-reviewed the current revision (HEAD 6c5959faeb8d36522aa1ca8965e5d6224fb1dfb3). No new blocking issues were found in this pass.
Previously raised concerns — current state
| Concern | Status |
|---|---|
Import-time bcrypt for DUMMY_PASSWORD_HASH |
Resolved — now a precomputed literal |
| Raw username/email in failed-auth logs | Resolved — fixed event messages |
| Raw client IP in rate-limit logs | Resolved — logs use hashed prefixes |
Token revocation for tokens without exp |
Resolved — logout_handler rejects exp <= 0 |
is_user_active DB failure failing open |
Resolved — DB errors return False |
select_account rate limiting |
Resolved — wired through enforce_credential_rate_limit |
Blocking criteria check
- No hardcoded secrets/API keys/credentials in source. ✅
- No SQL injection or non-parameterized asyncpg queries in changed code. ✅
- No auth/authorization bypass or cross-tenant data access introduced. ✅
- No SSRF, command/template injection, or unsafe eval added. ✅
- No PII exposure in new log lines or responses. ✅
- No existing
database/migrations/*.sqlfiles modified. ✅
Non-blocking items still worth addressing before merge
The following MINOR threads from earlier review rounds remain valid but do not block approval:
UserUpdatepassword-policy context — password-only updates validate only against the new email local-part, not persisted username/id/email.AUTH_RATE_LIMIT_*env parsing instatic.py—int(os.environ.get(...))crashes on empty/non-numeric values; consider a tolerant helper.logout_userusingDepends(...)in argument defaults triggers Ruff B008.- S2S token default lifetime equals the 365-day cap; consider whether the default should be shorter.
Approved.
…vocation (PT-16/18/19/21/22/24) Independent of the other pentest PRs — targets release, merges in any order. These three findings are one PR because they are genuinely coupled, not to save review effort: the PT-16 timing fix imports DUMMY_PASSWORD_HASH from the password module PT-24 introduces, and the PT-22 logout revocation edits the same two auth-router files as the PT-16 rate-limit wiring. Splitting them would produce PRs that do not build alone. PT-24 — no password strength requirement existed. Verified on release: 'password', '12345678' and the user's own address were all accepted. Adds a policy applied at both schema boundaries: length, 3-of-4 character classes, a common-password and identifier deny-list, and an explicit 72-byte guard because bcrypt silently truncates past that. password.py becomes a package so the bcrypt primitives and the policy live in separate modules behind one import path. PT-16 — /auth/accounts required no proof of ownership on the email branch, and /login and /auth/s2s/token returned faster for an unknown username than for a wrong password because the no-such-user branch skipped bcrypt. Both now spend one verification against a shared dummy hash; the account-listing path spends a constant budget regardless of how many accounts share an email, since verifying once per candidate leaks the count through response time. Fixed-window per-IP and per-username caps bound online guessing. PT-18 — a cross-IP aggregate cap per public_widget_key, so distributing an attack across source addresses does not buy unlimited attempts. PT-19 — the chat-demo client IP is derived from the trusted last XFF hop. PT-22 — logout was a no-op: a stolen token stayed valid for its full lifetime. Adds a revocation denylist keyed by a hash of the token with a TTL equal to its remaining lifetime, plus a per-request is_active liveness recheck. verify_rbac_token and get_user_from_websocket become coroutines because the check is a Redis round-trip; four `await` additions elsewhere are that change and nothing else. PT-21 — the 365-day S2S cap. There are two mint paths: POST /auth/s2s/token (admin-only) and POST /merchant with issue_token=true, which resellers can reach and which defaulted to 3650 days and allowed 365000. Both now read MAX_S2S_TOKEN_LIFETIME_DAYS rather than their own literal. Rate limiting and the liveness recheck both fail OPEN on a Redis outage. That is deliberate and bounded: a Redis blip losing the cap beats one locking every operator out, and bcrypt still bounds throughput underneath. 951 tests pass on this branch. Review round 2 (Tara-ag, CodeRabbit, Copilot): PII out of the logs. The per-IP credential bucket now hashes the address before it is used, so the raw IP reaches neither the Redis key nor a log line — note check_rate_limit logs its own identifier on the Redis-error paths, so passing the raw value leaked it there too. The per-username 429 logs the digest prefix instead of a 64-char slice of the address. The failed account-list, failed-login, failed-S2S and inactive-user lines drop the username entirely; those four predate this PR, but they are email addresses in an auth file this PR is already hardening, and they are attacker-driven, so they are also a log-injection sink. DUMMY_PASSWORD_HASH becomes a literal. Computing it ran a cost-12 bcrypt (~240ms) at import, once per worker cold start. Verification cost is unchanged — the cost factor is in the hash — so the timing-oracle defence is identical. is_user_active now fails CLOSED on a database error. Returning True there accepted tokens for disabled and deleted users for the length of the outage, which is the window a stolen token wants. Redis failures still fail open: the cache is an optimisation and the authoritative read runs behind it. Logout no longer claims a revocation that did not happen. A token with no usable exp handed revoke_token a 0, which it read as a negative TTL and reported as "already expired" success without writing the denylist entry — for the one kind of token that never expires by itself. revoke_token also rounds its TTL up rather than truncating, so a token with a fraction of a second left still gets an entry instead of falling into the same path. /auth/select-account is rate-limited. It verifies a password, so leaving it uncapped let an attacker who knows an account_id guess at full speed while /auth/accounts and /login were throttled. check_rate_limit deletes a counter whose TTL could not be set. Only the first request in a window sets one, so a key that survives a failed EXPIRE can never be repaired, and once it passed the limit a fail-closed caller — the widget caps — would 429 that key permanently rather than for one window. Also: the widget aggregate cap is documented as per-action rather than as one shared budget, which is what it is; the logout route documents its 401 and revoked:false responses as completed sign-out; and the enumeration test asserts ValidationError rather than bare Exception. Not changed: CodeRabbit reported both new test files declare async tests with no asyncio marker and may be collected without running. asyncio_mode = "auto" is set in pyproject.toml, and an async test asserting False fails, so they do run. 966 tests pass (was 951).
6c5959f to
3caee5a
Compare
Tara-ag
left a comment
Tara-ag
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the 30 changed files in this auth-hardening PR. No new blocking issues were identified that are not already covered by existing review comments.
Existing comments I reviewed and agree are worth resolving
app/schemas/breeze_buddy/users.py:UserUpdate._validate_password_policyshould validate against persisted identifiers (username/id/existing email), not just the new email local-part.app/core/config/static.py:int(os.environ.get(...))for the new rate-limit env vars can crash on empty/non-numeric values; consider a defensive int-env helper.app/api/routers/breeze_buddy/auth/__init__.py:Depends(...)in route-signature defaults triggers Ruff B008; use module-level dependency instances.
What looks good
- SQL queries remain parameterized; no string-interpolated SQL was introduced.
- No hardcoded secrets;
DUMMY_PASSWORD_HASHis a precomputed bcrypt literal with a clear rationale. - Token revocation, liveness recheck, and rate-limiting all fail open on Redis as documented.
- Constant bcrypt budget on the no-such-user and account-list branches closes the timing oracle.
- S2S token lifetime is capped at
MAX_S2S_TOKEN_LIFETIME_DAYSon both mint paths. - PII is no longer logged on failed credential attempts.
Approving since the remaining concerns are already tracked in existing comment threads and none meet the blocking criteria as new findings.
Independent PR — targets
releasedirectly and shares no file with #987, #988, #989 or #990. Merge in any order.Replaces #991, #992 and #993, which were chained. Those three are combined here because their dependencies are real, not stylistic:
DUMMY_PASSWORD_HASHfrom the password module PT-24 introducesauth/__init__.py,auth/handlers.py) as the PT-16 rate-limit wiringSplit apart, none of the three builds on its own.
Independent of the other pentest PRs — targets release, merges in any order.
These three findings are one PR because they are genuinely coupled, not to save
review effort: the PT-16 timing fix imports DUMMY_PASSWORD_HASH from the password
module PT-24 introduces, and the PT-22 logout revocation edits the same two
auth-router files as the PT-16 rate-limit wiring. Splitting them would produce
PRs that do not build alone.
PT-24 — no password strength requirement existed. Verified on release: 'password',
'12345678' and the user's own address were all accepted. Adds a policy applied at
both schema boundaries: length, 3-of-4 character classes, a common-password and
identifier deny-list, and an explicit 72-byte guard because bcrypt silently
truncates past that. password.py becomes a package so the bcrypt primitives and
the policy live in separate modules behind one import path.
PT-16 — /auth/accounts required no proof of ownership on the email branch, and
/login and /auth/s2s/token returned faster for an unknown username than for a
wrong password because the no-such-user branch skipped bcrypt. Both now spend one
verification against a shared dummy hash; the account-listing path spends a
constant budget regardless of how many accounts share an email, since verifying
once per candidate leaks the count through response time. Fixed-window per-IP and
per-username caps bound online guessing.
PT-18 — a cross-IP aggregate cap per public_widget_key, so distributing an attack
across source addresses does not buy unlimited attempts.
PT-19 — the chat-demo client IP is derived from the trusted last XFF hop.
PT-22 — logout was a no-op: a stolen token stayed valid for its full lifetime.
Adds a revocation denylist keyed by a hash of the token with a TTL equal to its
remaining lifetime, plus a per-request is_active liveness recheck. verify_rbac_token
and get_user_from_websocket become coroutines because the check is a Redis
round-trip; four
awaitadditions elsewhere are that change and nothing else.PT-21 — the 365-day S2S cap. There are two mint paths: POST /auth/s2s/token
(admin-only) and POST /merchant with issue_token=true, which resellers can reach
and which defaulted to 3650 days and allowed 365000. Both now read
MAX_S2S_TOKEN_LIFETIME_DAYS rather than their own literal.
Rate limiting and the liveness recheck both fail OPEN on a Redis outage. That is
deliberate and bounded: a Redis blip losing the cap beats one locking every
operator out, and bcrypt still bounds throughput underneath.
951 tests pass on this branch.
Independence, verified
All five clairvoyance PRs were trial-merged against each other — 10 of 10 pairs merge with no conflict, so any merge order works. The four config blocks that previously collided now sit at four distinct anchors in
static.py, hundreds of lines apart.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 recordings for the three findings are in the comment below.
Summary by CodeRabbit
Security Enhancements
Bug Fixes