From b1a136b5be0bddddaa5844808cb538b9578387d7 Mon Sep 17 00:00:00 2001 From: curo1305 Date: Sat, 30 May 2026 18:04:09 +0200 Subject: [PATCH] fix(05-12): resolve 3 critical code review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR-01: add `except HTTPException: raise` before broad except in stream_document_content — prevents 503 (reconnect prompt) from being swallowed and replaced with misleading 502 CR-02: move pre-flight credential checks BEFORE Redis setex in oauth_initiate — no orphan state tokens written for unconfigured providers; also adds onedrive_tenant_id to OneDrive pre-flight condition (WR-02) CR-03: add CLOUD_CREDS_KEY to celery-worker environment in docker-compose.yml — worker cannot decrypt cloud credentials without this key; every cloud document task was silently failing at runtime WR-03: assert Redis store empty after 400 pre-flight responses in both new tests — confirms no token leak on misconfigured-provider requests Co-Authored-By: Claude Sonnet 4.6 --- backend/api/cloud.py | 19 ++++++++++++------- backend/api/documents.py | 2 ++ backend/tests/test_cloud.py | 10 ++++++++-- docker-compose.yml | 1 + 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/backend/api/cloud.py b/backend/api/cloud.py index 6046291..f995099 100644 --- a/backend/api/cloud.py +++ b/backend/api/cloud.py @@ -339,23 +339,28 @@ async def oauth_initiate( detail=f"Unsupported OAuth provider: {provider}. Valid providers: {sorted(VALID_OAUTH_PROVIDERS)}", ) - state_token = secrets.token_urlsafe(32) - redis_client = request.app.state.redis - await redis_client.setex(f"oauth_state:{state_token}", 1800, str(current_user.id)) - - redirect_uri = f"{settings.backend_url}/api/cloud/oauth/callback/{provider}" - + # Pre-flight: validate credentials are configured before allocating Redis state if provider == "google_drive" and (not settings.google_client_id or not settings.google_client_secret): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail="Google Drive OAuth is not configured on this server. Set GOOGLE_CLIENT_ID and GOOGLE_CLIENT_SECRET in your environment.", ) - if provider == "onedrive" and (not settings.onedrive_client_id or not settings.onedrive_client_secret): + if provider == "onedrive" and ( + not settings.onedrive_client_id + or not settings.onedrive_client_secret + or not settings.onedrive_tenant_id + ): raise HTTPException( status_code=status.HTTP_400_BAD_REQUEST, detail="OneDrive OAuth is not configured on this server. Set ONEDRIVE_CLIENT_ID, ONEDRIVE_CLIENT_SECRET, and ONEDRIVE_TENANT_ID in your environment.", ) + state_token = secrets.token_urlsafe(32) + redis_client = request.app.state.redis + await redis_client.setex(f"oauth_state:{state_token}", 1800, str(current_user.id)) + + redirect_uri = f"{settings.backend_url}/api/cloud/oauth/callback/{provider}" + if provider == "google_drive": from google_auth_oauthlib.flow import Flow # lazy import diff --git a/backend/api/documents.py b/backend/api/documents.py index fe28580..d7d271c 100644 --- a/backend/api/documents.py +++ b/backend/api/documents.py @@ -756,6 +756,8 @@ async def stream_document_content( status_code=503, detail="Cloud connection requires re-authentication. Please reconnect in Settings.", ) from exc + except HTTPException: + raise except Exception as exc: raise HTTPException( status_code=502, diff --git a/backend/tests/test_cloud.py b/backend/tests/test_cloud.py index 6321a40..55ce2af 100644 --- a/backend/tests/test_cloud.py +++ b/backend/tests/test_cloud.py @@ -784,7 +784,8 @@ async def test_oauth_initiate_returns_json_url(async_client, db_session, monkeyp async def test_oauth_initiate_google_drive_not_configured(async_client, db_session, monkeypatch): """GET /api/cloud/oauth/initiate/google_drive returns 400 with env-var hint when creds missing. - Pre-flight check (plan 05-12): empty GOOGLE_CLIENT_ID/SECRET → 400 before touching OAuth libs. + Pre-flight check (plan 05-12): empty GOOGLE_CLIENT_ID/SECRET → 400 BEFORE Redis state write. + Asserts Redis store is empty to confirm no orphan state tokens are created on misconfigured calls. """ from main import app from config import settings @@ -802,16 +803,19 @@ async def test_oauth_initiate_google_drive_not_configured(async_client, db_sessi follow_redirects=False, ) + redis_keys = list(fake_redis._store.keys()) app.state.redis = None assert resp.status_code == 400, f"Expected 400, got {resp.status_code}: {resp.text}" assert "GOOGLE_CLIENT_ID" in resp.json()["detail"], f"Unexpected detail: {resp.json()['detail']}" + assert len(redis_keys) == 0, f"Expected no Redis state token written on pre-flight failure, got: {redis_keys}" async def test_oauth_initiate_onedrive_not_configured(async_client, db_session, monkeypatch): """GET /api/cloud/oauth/initiate/onedrive returns 400 with env-var hint when creds missing. - Pre-flight check (plan 05-12): empty ONEDRIVE_CLIENT_ID → 400 before touching MSAL. + Pre-flight check (plan 05-12): empty ONEDRIVE_CLIENT_ID → 400 BEFORE Redis state write. + Asserts Redis store is empty to confirm no orphan state tokens are created on misconfigured calls. """ from main import app from config import settings @@ -829,10 +833,12 @@ async def test_oauth_initiate_onedrive_not_configured(async_client, db_session, follow_redirects=False, ) + redis_keys = list(fake_redis._store.keys()) app.state.redis = None assert resp.status_code == 400, f"Expected 400, got {resp.status_code}: {resp.text}" assert "ONEDRIVE_CLIENT_ID" in resp.json()["detail"], f"Unexpected detail: {resp.json()['detail']}" + assert len(redis_keys) == 0, f"Expected no Redis state token written on pre-flight failure, got: {redis_keys}" async def test_oauth_initiate_requires_auth(async_client, db_session): diff --git a/docker-compose.yml b/docker-compose.yml index 9034b77..f1ef398 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -87,6 +87,7 @@ services: - MINIO_SECRET_KEY=${MINIO_SECRET_KEY} - MINIO_BUCKET=${MINIO_BUCKET} - REDIS_URL=${REDIS_URL} + - CLOUD_CREDS_KEY=${CLOUD_CREDS_KEY} - PYTHONDONTWRITEBYTECODE=1 volumes: - ./backend:/app