From 1a37f344018c8db59bba0daa9105f74693c2c39c Mon Sep 17 00:00:00 2001 From: "Gan, Jimmy" Date: Sun, 3 May 2026 13:45:42 +0800 Subject: [PATCH] =?UTF-8?q?auth:=20Sprint=2002=20=E2=80=94=20auth=20harden?= =?UTF-8?q?ing=20(Pydantic=20models,=20challenge=20binding,=20admin=20gate?= =?UTF-8?q?s)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add max_length validation to LoginRequest (username 128, password 1024) - Replace raw request.json() with Pydantic models in RBAC override/update endpoints - Replace raw request.json() with Pydantic models in passkey register/login/delete - Bind passkey challenges to session-bound challenge_id (prevents cross-session replay) - Gate audit log and security log endpoints behind admin role - Fix fragile opc_db.json.dumps() → import json directly - Add COOKIE_SECURE=False startup warning (suppress with ALLOW_INSECURE_COOKIES) Co-Authored-By: Claude Opus 4.7 --- dashboard/backend/main.py | 8 ++ dashboard/backend/routers/auth.py | 40 +++++---- dashboard/backend/routers/passkey.py | 85 ++++++++++++------- dashboard/backend/routers/security.py | 8 +- dashboard/backend/routers/system.py | 7 +- dashboard/backend/services/agent_executor.py | 9 +- .../tests/integration/test_auth_flow.py | 6 +- 7 files changed, 101 insertions(+), 62 deletions(-) diff --git a/dashboard/backend/main.py b/dashboard/backend/main.py index 4921663..7d4d1d8 100644 --- a/dashboard/backend/main.py +++ b/dashboard/backend/main.py @@ -51,6 +51,14 @@ async def lifespan(app: FastAPI): # Startup print("=== Application Startup ===") + # Warn if cookies are not marked Secure (must be behind TLS-terminating proxy) + import auth_service as auth_svc + + if not auth_svc.COOKIE_SECURE and not os.environ.get("ALLOW_INSECURE_COOKIES"): + print("WARNING: COOKIE_SECURE=False — auth cookies not marked Secure. " + "This requires a TLS-terminating reverse proxy (e.g. Caddy) in front. " + "Set ALLOW_INSECURE_COOKIES=true to suppress this warning.") + # Initialize OPC database from db import opc_db diff --git a/dashboard/backend/routers/auth.py b/dashboard/backend/routers/auth.py index 90efb49..5e40a8a 100644 --- a/dashboard/backend/routers/auth.py +++ b/dashboard/backend/routers/auth.py @@ -7,7 +7,7 @@ from datetime import timedelta import httpx import jwt from fastapi import APIRouter, Body, Depends, HTTPException, Request, Response, status -from pydantic import BaseModel +from pydantic import BaseModel, Field from slowapi import Limiter from slowapi.util import get_remote_address @@ -20,8 +20,8 @@ limiter = Limiter(key_func=get_remote_address) class LoginRequest(BaseModel): - username: str - password: str + username: str = Field(..., max_length=128) + password: str = Field(..., max_length=1024) totp_code: str = "" @@ -239,20 +239,23 @@ async def rbac_config(current_user=Depends(auth.get_current_user)): return load_rbac() +class RbacOverrideRequest(BaseModel): + pages: list[str] | str + +class RbacUpdateRoleRequest(BaseModel): + pages: list[str] | str + sidebar_links: list[str] | str | None = None + + @router.put("/rbac/overrides/{username}") -async def rbac_set_override(username: str, request: Request, current_user=Depends(auth.get_current_user)): +async def rbac_set_override(username: str, body: RbacOverrideRequest, current_user=Depends(auth.get_current_user)): if current_user.role != "admin": raise HTTPException(status_code=403, detail="Admin only") from rbac import update_rbac - body = await request.json() - pages = body.get("pages") - if pages is None: - raise HTTPException(status_code=400, detail="Missing pages field") - def mutator(rbac): existing = rbac.setdefault("user_overrides", {}).get(username, {}) - existing["pages"] = pages + existing["pages"] = body.pages rbac["user_overrides"][username] = existing update_rbac(mutator) @@ -301,25 +304,20 @@ async def rbac_delete_override(username: str, current_user=Depends(auth.get_curr @router.put("/rbac/roles/{role}") -async def rbac_update_role(role: str, request: Request, current_user=Depends(auth.get_current_user)): +async def rbac_update_role(role: str, body: RbacUpdateRoleRequest, current_user=Depends(auth.get_current_user)): if current_user.role != "admin": raise HTTPException(status_code=403, detail="Admin only") from rbac import update_rbac - body = await request.json() - pages = body.get("pages") - sidebar_links = body.get("sidebar_links") - if pages is None: - raise HTTPException(status_code=400, detail="Missing pages field") - if pages != "*" and not isinstance(pages, list): + if body.pages != "*" and not isinstance(body.pages, list): raise HTTPException(status_code=400, detail="pages must be '*' or a list") - if sidebar_links is not None and sidebar_links != "*" and not isinstance(sidebar_links, list): + if body.sidebar_links is not None and body.sidebar_links != "*" and not isinstance(body.sidebar_links, list): raise HTTPException(status_code=400, detail="sidebar_links must be '*' or a list") def mutator(rbac): - role_config = {"pages": pages} - if sidebar_links is not None: - role_config["sidebar_links"] = sidebar_links + role_config = {"pages": body.pages} + if body.sidebar_links is not None: + role_config["sidebar_links"] = body.sidebar_links rbac.setdefault("role_defaults", {})[role] = role_config update_rbac(mutator) diff --git a/dashboard/backend/routers/passkey.py b/dashboard/backend/routers/passkey.py index 332992d..d00d5ec 100644 --- a/dashboard/backend/routers/passkey.py +++ b/dashboard/backend/routers/passkey.py @@ -3,10 +3,13 @@ import json import logging import time +import uuid from datetime import timedelta +from typing import Any from fastapi import APIRouter, Depends, HTTPException, Request, Response from fastapi.responses import JSONResponse +from pydantic import BaseModel from slowapi import Limiter from slowapi.util import get_remote_address from webauthn import ( @@ -25,22 +28,26 @@ router = APIRouter() limiter = Limiter(key_func=get_remote_address) logger = logging.getLogger(__name__) -# In-memory challenge store with TTL -_challenges = {} +# In-memory challenge store with TTL: {challenge_id: (challenge_bytes, timestamp)} +_challenges: dict[str, tuple[bytes, float]] = {} -def _store_challenge(challenge: bytes): - """Store a WebAuthn challenge with timestamp for TTL tracking.""" - _challenges[challenge] = time.time() +def _store_challenge(challenge: bytes) -> str: + """Store a WebAuthn challenge and return a session-bound challenge_id.""" + challenge_id = uuid.uuid4().hex + _challenges[challenge_id] = (challenge, time.time()) # Clean up expired challenges (>5 minutes old) now = time.time() - expired = [k for k, v in _challenges.items() if now - v > 300] + expired = [k for k, v in _challenges.items() if now - v[1] > 300] for k in expired: _challenges.pop(k, None) + return challenge_id -def _get_challenge(client_data_b64: str) -> bytes: - """Extract and validate challenge from clientDataJSON.""" +def _get_challenge(challenge_id: str | None, client_data_b64: str) -> bytes: + """Look up challenge by session-bound challenge_id and validate clientDataJSON.""" + if not challenge_id: + raise HTTPException(status_code=400, detail="Missing challenge_id") if not client_data_b64: raise HTTPException(status_code=400, detail="Missing clientDataJSON") try: @@ -49,12 +56,31 @@ def _get_challenge(client_data_b64: str) -> bytes: except Exception: raise HTTPException(status_code=400, detail="Invalid clientDataJSON") - entry = _challenges.pop(chal_bytes, None) - if not entry or time.time() - entry > 300: + entry = _challenges.pop(challenge_id, None) + if not entry or time.time() - entry[1] > 300: raise HTTPException(status_code=400, detail="Challenge expired or invalid") + stored_challenge = entry[0] + if stored_challenge != chal_bytes: + raise HTTPException(status_code=400, detail="Challenge mismatch") return chal_bytes +# Pydantic models for passkey request validation +class PasskeyVerifyRequest(BaseModel): + id: str = "" + rawId: str = "" + response: dict[str, Any] = {} + type: str = "public-key" + challenge_id: str | None = None + name: str = "" + # Allow extra fields for authenticator-specific extensions + model_config = {"extra": "allow"} + + +class PasskeyDeleteRequest(BaseModel): + id: str + + @router.post("/passkey/register/options") @limiter.limit("5/minute") async def passkey_register_options(request: Request, current_user=Depends(auth.get_current_user)): @@ -69,23 +95,24 @@ async def passkey_register_options(request: Request, current_user=Depends(auth.g user_display_name=current_user.username, exclude_credentials=exclude, ) - _store_challenge(options.challenge) - return JSONResponse(content=json.loads(options_to_json(options))) + chal_id = _store_challenge(options.challenge) + result = json.loads(options_to_json(options)) + result["challenge_id"] = chal_id + return JSONResponse(content=result) @router.post("/passkey/register/verify") -async def passkey_register_verify(request: Request, current_user=Depends(auth.get_current_user)): +async def passkey_register_verify(body: PasskeyVerifyRequest, current_user=Depends(auth.get_current_user)): """Verify and save a new passkey registration.""" - body = await request.json() - challenge = _get_challenge(body.get("response", {}).get("clientDataJSON", "")) + challenge = _get_challenge(body.challenge_id, body.response.get("clientDataJSON", "")) try: verification = verify_registration_response( - credential=body, + credential=body.model_dump(), expected_challenge=challenge, expected_rp_id=config.WEBAUTHN_RP_ID, expected_origin=config.WEBAUTHN_ORIGINS, ) - except Exception as e: + except Exception: logger.exception("Passkey registration verification failed") raise HTTPException(status_code=400, detail="Invalid passkey registration") @@ -94,7 +121,7 @@ async def passkey_register_verify(request: Request, current_user=Depends(auth.ge "credential_id": bytes_to_base64url(verification.credential_id), "public_key": bytes_to_base64url(verification.credential_public_key), "sign_count": verification.sign_count, - "name": body.get("name", "Passkey"), + "name": body.name or "Passkey", "created_at": time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime()), "username": current_user.username, "role": current_user.role, @@ -117,32 +144,33 @@ async def passkey_login_options(request: Request): allow_credentials=allow, user_verification=UserVerificationRequirement.PREFERRED, ) - _store_challenge(options.challenge) - return JSONResponse(content=json.loads(options_to_json(options))) + chal_id = _store_challenge(options.challenge) + result = json.loads(options_to_json(options)) + result["challenge_id"] = chal_id + return JSONResponse(content=result) @router.post("/passkey/login/verify") @limiter.limit("10/minute") -async def passkey_login_verify(request: Request, response: Response): +async def passkey_login_verify(body: PasskeyVerifyRequest, request: Request, response: Response): """Verify passkey authentication and issue tokens.""" - body = await request.json() - challenge = _get_challenge(body.get("response", {}).get("clientDataJSON", "")) + challenge = _get_challenge(body.challenge_id, body.response.get("clientDataJSON", "")) creds = auth.load_passkey_credentials() - cred_id_b64 = body.get("id", "") + cred_id_b64 = body.id matched = next((c for c in creds if c["credential_id"] == cred_id_b64), None) if not matched: raise HTTPException(status_code=400, detail="Unknown credential") try: verification = verify_authentication_response( - credential=body, + credential=body.model_dump(), expected_challenge=challenge, expected_rp_id=config.WEBAUTHN_RP_ID, expected_origin=config.WEBAUTHN_ORIGINS, credential_public_key=base64url_to_bytes(matched["public_key"]), credential_current_sign_count=matched["sign_count"], ) - except Exception as e: + except Exception: logger.exception("Passkey authentication verification failed") raise HTTPException(status_code=400, detail="Invalid passkey authentication") @@ -184,10 +212,9 @@ async def passkey_list(current_user=Depends(auth.get_current_user)): @router.post("/passkey/delete") -async def passkey_delete(request: Request, current_user=Depends(auth.get_current_user)): +async def passkey_delete(body: PasskeyDeleteRequest, current_user=Depends(auth.get_current_user)): """Delete a specific passkey by credential ID.""" - body = await request.json() - cred_id = body.get("id") + cred_id = body.id data = auth._load_auth_data() creds = data.get("passkey_credentials", []) data["passkey_credentials"] = [c for c in creds if c["credential_id"] != cred_id] diff --git a/dashboard/backend/routers/security.py b/dashboard/backend/routers/security.py index 5e13a9e..dbcd983 100644 --- a/dashboard/backend/routers/security.py +++ b/dashboard/backend/routers/security.py @@ -4,8 +4,9 @@ from collections import Counter from datetime import datetime, time, timedelta, timezone import docker -from fastapi import APIRouter, Query +from fastapi import APIRouter, Depends, HTTPException, Query +import auth_service as auth from config import DOCKER_HOST router = APIRouter() @@ -203,8 +204,11 @@ def security_logs( ip: str | None = Query(None), start_date: str | None = Query(None), end_date: str | None = Query(None), + current_user=Depends(auth.get_current_user), ): - """Fetch and parse security-relevant logs from Authelia and Caddy.""" + """Fetch and parse security-relevant logs from Authelia and Caddy. Admin only.""" + if current_user.role != "admin": + raise HTTPException(status_code=403, detail="Admin only") results = [] # Authelia logs diff --git a/dashboard/backend/routers/system.py b/dashboard/backend/routers/system.py index 040dedd..f1a6158 100644 --- a/dashboard/backend/routers/system.py +++ b/dashboard/backend/routers/system.py @@ -5,8 +5,9 @@ import time import httpx import psutil -from fastapi import APIRouter, HTTPException, Query, Request +from fastapi import APIRouter, Depends, HTTPException, Query, Request +import auth_service as auth import config from config import OPENCLAW_GATEWAY_TOKEN, TELEGRAM_BOT_TOKEN, TELEGRAM_CHAT_ID, VOLUME_ROOT @@ -79,7 +80,9 @@ def system_stats(): @router.get("/audit-log") -def audit_log(lines: int = Query(100, le=500)): +def audit_log(lines: int = Query(100, le=500), current_user=Depends(auth.get_current_user)): + if current_user.role != "admin": + raise HTTPException(status_code=403, detail="Admin only") log_path = os.path.join(VOLUME_ROOT, "docker/nas-dashboard/audit.log") if not os.path.exists(log_path): return {"entries": []} diff --git a/dashboard/backend/services/agent_executor.py b/dashboard/backend/services/agent_executor.py index 92a56b7..8ba2221 100644 --- a/dashboard/backend/services/agent_executor.py +++ b/dashboard/backend/services/agent_executor.py @@ -3,6 +3,7 @@ Agent Executor Service - Executes agent tasks and manages their lifecycle """ import asyncio +import json import logging import os from datetime import datetime @@ -270,8 +271,8 @@ class AgentExecutor: WHERE id = $4 """, "completed", - opc_db.json.dumps(result), - opc_db.json.dumps(result.get("actions", [])), + json.dumps(result), + json.dumps(result.get("actions", [])), execution_id, ) @@ -304,8 +305,8 @@ class AgentExecutor: WHERE id = $4 """, "pending_approval", - opc_db.json.dumps(result), - opc_db.json.dumps(result.get("actions", [])), + json.dumps(result), + json.dumps(result.get("actions", [])), execution_id, ) diff --git a/dashboard/backend/tests/integration/test_auth_flow.py b/dashboard/backend/tests/integration/test_auth_flow.py index b77573c..1a41c30 100644 --- a/dashboard/backend/tests/integration/test_auth_flow.py +++ b/dashboard/backend/tests/integration/test_auth_flow.py @@ -362,8 +362,7 @@ class TestRBACConfig: """Test setting user override without pages field.""" response = test_app.put("/api/auth/rbac/overrides/testuser", headers=admin_headers, json={}) - assert response.status_code == 400 - assert "Missing pages field" in response.json()["detail"] + assert response.status_code == 422 def test_set_user_override_as_member(self, test_app, mock_config, temp_auth_file, temp_rbac_file): """Test that non-admin cannot set user override.""" @@ -437,8 +436,7 @@ class TestRBACConfig: "/api/auth/rbac/roles/member", headers=admin_headers, json={"sidebar_links": ["navidrome"]} ) - assert response.status_code == 400 - assert "Missing pages field" in response.json()["detail"] + assert response.status_code == 422 def test_update_role_config_invalid_pages_type( self, test_app, mock_config, temp_auth_file, temp_rbac_file, admin_headers