From 092c973eacebe9d480f011ff50e1556ad64279fb Mon Sep 17 00:00:00 2001 From: BADRI NARAYANAN Date: Sun, 6 Sep 2026 17:41:15 -0700 Subject: [PATCH] fix(api): orders return 401 for no session, not 403; clean up rate-limit gaps QA pass found orders.py conflating "no session" with "wrong role" on all four routes -- e.g. create_order's `if not current_user.is_authenticated or current_user.role != "customer": raise ForbiddenError(...)` raised 403 for both cases. Every other blueprint (portfolios.py's own _authorize_customer_id is the reference pattern) correctly returns 401 for no session and 403 only once a session exists with the wrong role. A frontend that keys "session expired -> re-login" off 401 wouldn't trigger re-auth on any orders screen. Split each of the four checks into two: UnauthenticatedError first (401) when there's no session at all, ForbiddenError after (403) only once a session exists but the role is wrong. list_orders/get_order already had the checks in separate if-statements but raised the wrong class on the first one; create_order/approve_order had them combined in one boolean and needed splitting. Updated test_list_orders_with_no_session_is_rejected (previously asserted the wrong 403) and added test_get_order_with_no_session_is_rejected (untested for this exact case before). Two smaller gaps from the same pass: - POST /auth/logout carried no rate limit, unlike every sibling auth route. Added 60/min, matching /session's own limit -- logout has no meaningful abuse surface, but consistency with the stated per-endpoint throttling policy matters. - RateLimitedError's bare `code="rate_limited"` is never what a flask-limiter throttle actually puts on the wire (that path goes through the generic HTTPException handler and emits "too_many_requests" instead) -- the class itself is genuinely used (chat.py's daily query cap, always with its own code= override), just its default was flagged as misleading. Added a comment explaining the two paths rather than removing anything, since deleting the class would break chat.py's real usage. Also: two frontend comments and one README bullet claimed the /lots, /admin/customers*, /admin/kyc-overrides/, and /statements//export backend routes "don't exist yet" -- false; all four are live, registered, and covered by the 799 backend tests. The frontend genuinely hasn't been wired to them (still on mockClient, honestly labelled "Simulated" in the UI per docs/requirements/real-vs-simulated-rules.md -- that disclosure is correct and unchanged), but the routes exist. Corrected all three claims to say so. Verified: full backend suite (uv run pytest) run twice -- 799 passed, 7 skipped, 2 xpassed both times, no flakes. ruff and mypy --strict clean on every touched file (2 pre-existing, unrelated mypy errors remain elsewhere, confirmed not introduced by this change). Live-verified the 401 fix against the running local backend: GET /api/v1/orders with no cookie now returns 401 {"code":"unauthenticated"}. Frontend lint and build clean (comment-only changes, no behavior touched). Co-Authored-By: Claude Opus 5 --- README.md | 9 +++++---- backend/app/controllers/api/auth.py | 1 + backend/app/controllers/api/orders.py | 12 ++++++++---- backend/app/core/errors.py | 7 ++++++- backend/tests/api/test_orders.py | 11 ++++++++++- .../admin-customers/api/adminCustomersApi.js | 4 +++- frontend/src/features/lots/api/lotsApi.js | 4 +++- .../src/features/statements/api/statementsApi.js | 4 +++- 8 files changed, 39 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 5663e71..2d2bc47 100644 --- a/README.md +++ b/README.md @@ -48,10 +48,11 @@ an internal-only Container App (ADR 13/15: sessions and rate limits only, never - Postgres allows public access from any IP (`--public-access 0.0.0.0-255.255.255.255`) rather than VNet-integrated private access. - Three domains are still mocked client-side, honestly labelled "Simulated" in the UI: tax lots, - the admin customer directory/detail/KYC-override, and statement export — S8's `/lots`, - `/admin/customers*`, and `/admin/kyc-overrides/` routes don't exist yet - (`docs/superpowers/specs/2026-09-05-frontend-completion-design.md` has the full design for - closing this). + the admin customer directory/detail/KYC-override, and statement export. The backend has since + caught up — `/lots`, `/admin/customers*`, `/admin/kyc-overrides/`, and + `/statements//export` are all real, registered, tested routes — the frontend just + hasn't been wired to them yet (`docs/superpowers/specs/2026-09-05-frontend-completion-design.md` + has the full design for closing this). - No real-time SSE push (ADR 20/S12 §6) — every screen refetches on mount instead, the documented fallback. - No new automated test coverage was added for this pass (explicit scope cut — ship fast, harden diff --git a/backend/app/controllers/api/auth.py b/backend/app/controllers/api/auth.py index 962d9d8..4e260df 100644 --- a/backend/app/controllers/api/auth.py +++ b/backend/app/controllers/api/auth.py @@ -194,6 +194,7 @@ def session_info() -> Any: @auth_bp.route("/logout", methods=["POST"]) +@limiter.limit("60 per minute") def logout() -> Any: logout_user() flask_session.clear() diff --git a/backend/app/controllers/api/orders.py b/backend/app/controllers/api/orders.py index a871791..532f5e5 100644 --- a/backend/app/controllers/api/orders.py +++ b/backend/app/controllers/api/orders.py @@ -126,7 +126,9 @@ def _to_order_response(uow: OrdersUnitOfWork, order: Order) -> OrderResponse: @orders_bp.route("", methods=["POST"]) @limiter.limit("30 per minute") def create_order() -> Any: - if not current_user.is_authenticated or current_user.role != "customer": + if not current_user.is_authenticated: + raise UnauthenticatedError("Authentication required") + if current_user.role != "customer": raise ForbiddenError("Only a customer may place an order") idempotency_key = request.headers.get("Idempotency-Key") @@ -188,7 +190,9 @@ def create_order() -> Any: @orders_bp.route("//approve", methods=["POST"]) @limiter.limit("30 per minute") def approve_order(order_id: uuid.UUID) -> Any: - if not current_user.is_authenticated or current_user.role != "customer": + if not current_user.is_authenticated: + raise UnauthenticatedError("Authentication required") + if current_user.role != "customer": raise ForbiddenError("Only the owning customer may approve an order") customer_id = _resolve_customer_id() @@ -215,7 +219,7 @@ def approve_order(order_id: uuid.UUID) -> Any: @limiter.limit("60 per minute") def list_orders() -> Any: if not current_user.is_authenticated: - raise ForbiddenError("Authentication required") + raise UnauthenticatedError("Authentication required") if current_user.role not in ("customer", *_STAFF_ROLES): raise ForbiddenError(f"role {current_user.role} not authorized for this endpoint") @@ -233,7 +237,7 @@ def list_orders() -> Any: @limiter.limit("60 per minute") def get_order(order_id: uuid.UUID) -> Any: if not current_user.is_authenticated: - raise ForbiddenError("Authentication required") + raise UnauthenticatedError("Authentication required") if current_user.role not in ("customer", *_STAFF_ROLES): raise ForbiddenError(f"role {current_user.role} not authorized for this endpoint") diff --git a/backend/app/core/errors.py b/backend/app/core/errors.py index aa0574e..e1878a9 100644 --- a/backend/app/core/errors.py +++ b/backend/app/core/errors.py @@ -76,7 +76,12 @@ class ConflictError(AppError): class RateLimitedError(AppError): - """The caller has exceeded a configured rate limit (429).""" + """An application-level rate limit (429) -- e.g. `chat.py`'s daily query cap, always raised + with its own `code=` override. flask-limiter's own per-route limits (`@limiter.limit(...)`) + never reach here: those 429s come from flask-limiter's werkzeug exception instead, caught by + the generic `HTTPException` handler in `app/__init__.py`, which derives its own `code` from + the exception name (`too_many_requests`) -- so the bare `code` below is never what a + flask-limiter throttle actually puts on the wire.""" status = 429 title = "Too Many Requests" diff --git a/backend/tests/api/test_orders.py b/backend/tests/api/test_orders.py index c3b9963..09ecfe6 100644 --- a/backend/tests/api/test_orders.py +++ b/backend/tests/api/test_orders.py @@ -452,4 +452,13 @@ def test_get_order_rejects_another_customers_order( def test_list_orders_with_no_session_is_rejected(api_client: FlaskClient) -> None: response = api_client.get("/api/v1/orders") - assert response.status_code == 403 + # 401, not 403: no session is an authentication failure, not an authorization one -- a + # frontend that keys "session expired -> re-login" off 401 must actually see it here. + assert response.status_code == 401 + assert response.get_json()["code"] == "unauthenticated" + + +def test_get_order_with_no_session_is_rejected(api_client: FlaskClient) -> None: + response = api_client.get(f"/api/v1/orders/{uuid.uuid4()}") + assert response.status_code == 401 + assert response.get_json()["code"] == "unauthenticated" diff --git a/frontend/src/features/admin-customers/api/adminCustomersApi.js b/frontend/src/features/admin-customers/api/adminCustomersApi.js index f888c13..26b50cf 100644 --- a/frontend/src/features/admin-customers/api/adminCustomersApi.js +++ b/frontend/src/features/admin-customers/api/adminCustomersApi.js @@ -1,4 +1,6 @@ -// MOCK — no backend endpoint exists yet (S8). Replace with a real fetch call once that spec ships. +// MOCK -- the real backend routes (GET /admin/customers*, POST /admin/kyc-overrides/) exist +// and are tested (backend/app/controllers/admin/), the frontend just hasn't been wired to them +// yet. See docs/superpowers/specs/2026-09-05-frontend-completion-design.md. import { mockClient } from '../../../services/mockClient.js' const NAMESPACE = 'admin-customers' diff --git a/frontend/src/features/lots/api/lotsApi.js b/frontend/src/features/lots/api/lotsApi.js index 476ffc7..03636bf 100644 --- a/frontend/src/features/lots/api/lotsApi.js +++ b/frontend/src/features/lots/api/lotsApi.js @@ -1,4 +1,6 @@ -// MOCK — no backend endpoint exists yet (S5). Replace with a real fetch call once that spec ships. +// MOCK -- the real backend route (GET /lots) exists and is tested +// (backend/app/controllers/api/lots.py), the frontend just hasn't been wired to it yet. See +// docs/superpowers/specs/2026-09-05-frontend-completion-design.md. import { mockClient } from '../../../services/mockClient.js' const NAMESPACE = 'lots' diff --git a/frontend/src/features/statements/api/statementsApi.js b/frontend/src/features/statements/api/statementsApi.js index db59ae8..542e630 100644 --- a/frontend/src/features/statements/api/statementsApi.js +++ b/frontend/src/features/statements/api/statementsApi.js @@ -1,6 +1,8 @@ import { apiClient } from '../../../services/apiClient.js' -// `export` isn't wired here yet — no export-as-file endpoint exists (Phase 4 mock domain). +// `export` isn't wired here yet -- the real backend route (GET /statements//export) +// exists and is tested (backend/app/controllers/api/statements.py), the frontend just hasn't +// been pointed at it. See docs/superpowers/specs/2026-09-05-frontend-completion-design.md. export const statementsApi = { list: () => apiClient.get('/statements'), get: (periodStart, publishWatermark) => {