diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index 87e8ede..86db9a1 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -21,10 +21,52 @@ security = HTTPBearer() # JWT Configuration -SECRET_KEY = os.getenv("SECRET_KEY", "your-secret-key") +# SECURITY (MEDIUM-2 / appsec #18): the JWT signing key must come from the +# environment with NO insecure default. The previous default ("your-secret-key") +# made tokens forgeable by anyone who knew the public source, which undermines +# EVERY `get_current_user` object-level check downstream. We fail CLOSED: +# - A blank/unset SECRET_KEY leaves the module importable (other routers import +# this module at startup) but `_require_secret_key()` raises HTTP 503 at +# request time on any token issue/verify, so no token is ever signed or +# accepted with an absent/weak key. +# - In a non-test environment we additionally reject the known-insecure legacy +# placeholder value so it can never be reintroduced via env. +SECRET_KEY = os.getenv("SECRET_KEY") ALGORITHM = os.getenv("ALGORITHM", "HS256") ACCESS_TOKEN_EXPIRE_MINUTES = int(os.getenv("ACCESS_TOKEN_EXPIRE_MINUTES", "30")) +# Known-insecure placeholder values that must never be used to sign/verify tokens. +_INSECURE_SECRET_VALUES = {"your-secret-key", "your-super-secret-key-here-change-this-in-production"} + +if not SECRET_KEY or not SECRET_KEY.strip(): + logger.warning( + "SECRET_KEY is not set. JWT issuance/verification will fail closed with " + "HTTP 503 until a strong SECRET_KEY is configured. No insecure default is used." + ) +elif SECRET_KEY.strip() in _INSECURE_SECRET_VALUES: + logger.error( + "SECRET_KEY is set to a known-insecure placeholder value. JWT issuance/" + "verification will fail closed with HTTP 503 until a strong SECRET_KEY is " + "configured." + ) + + +def _require_secret_key() -> str: + """Return a usable JWT signing key or fail closed. + + SECURITY: raises HTTPException(503) when SECRET_KEY is missing/blank or is the + known-insecure placeholder, so tokens can never be signed or validated with a + forgeable key. Every create/verify path routes through here. + """ + key = SECRET_KEY + if not key or not key.strip() or key.strip() in _INSECURE_SECRET_VALUES: + logger.error("Refusing JWT operation: SECRET_KEY is unset, blank, or insecure.") + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Authentication is not configured", + ) + return key.strip() + # Pydantic models class UserCreate(BaseModel): @@ -66,7 +108,7 @@ def create_access_token(data: dict, expires_delta: Optional[timedelta] = None): else: expire = datetime.utcnow() + timedelta(minutes=15) to_encode.update({"exp": expire}) - encoded_jwt = jwt.encode(to_encode, SECRET_KEY, algorithm=ALGORITHM) + encoded_jwt = jwt.encode(to_encode, _require_secret_key(), algorithm=ALGORITHM) return encoded_jwt @@ -82,7 +124,7 @@ async def get_current_user( ) try: - payload = jwt.decode(credentials.credentials, SECRET_KEY, algorithms=[ALGORITHM]) + payload = jwt.decode(credentials.credentials, _require_secret_key(), algorithms=[ALGORITHM]) user_id: int = payload.get("sub") if user_id is None: raise credentials_exception diff --git a/backend/app/routers/chat.py b/backend/app/routers/chat.py index ce52c40..acd5d0a 100644 --- a/backend/app/routers/chat.py +++ b/backend/app/routers/chat.py @@ -1,5 +1,6 @@ from fastapi import APIRouter, Depends, HTTPException, status from sqlalchemy.ext.asyncio import AsyncSession +from sqlalchemy.future import select from pydantic import BaseModel from typing import List, Optional, Dict, Any import openai diff --git a/backend/app/routers/credentials.py b/backend/app/routers/credentials.py index 48617aa..afbb74b 100644 --- a/backend/app/routers/credentials.py +++ b/backend/app/routers/credentials.py @@ -132,6 +132,95 @@ def _load_service_api_keys() -> Dict[str, str]: # API key storage for microservice authentication (no hardcoded defaults). VALID_API_KEYS = _load_service_api_keys() + +# --- SECURITY (HIGH-2 / appsec #18): per-tenant service-key scoping. --- +# Previously the unresolved TODO meant a valid service key was trusted to act on +# behalf of ANY identity it named: get_identity_accounts (and the credential +# routes) bound the lookup to a raw identity_id with no relationship to the +# calling key, so any holder of a service key could enumerate/operate on ANY +# identity's accounts (cross-tenant BOLA via a trusted key). +# +# We now bind each service key to the explicit set of identity ids it is allowed +# to act for, configured per service via env, e.g.: +# SCRAPER_ALLOWED_IDENTITY_IDS="1,2,3" +# MOBILE_ALLOWED_IDENTITY_IDS="*" # explicit wildcard = all identities +# AUTOMATION_ALLOWED_IDENTITY_IDS="" # unset/blank = NO identities (deny) +# +# Semantics (fail closed): +# - unset / blank -> the key may act for NO identity (every scoped request 403). +# - "*" -> explicit operator opt-in to act for ANY identity. +# - "1,2,3" -> only those identity ids; anything else 403. +# This closes the cross-tenant enumeration gap: a key can only reach identities an +# operator has explicitly granted it, instead of every identity in the system. +_ALLOWED_IDS_ENV = { + "scraper-service": "SCRAPER_ALLOWED_IDENTITY_IDS", + "mobile-service": "MOBILE_ALLOWED_IDENTITY_IDS", + "automation-service": "AUTOMATION_ALLOWED_IDENTITY_IDS", +} + + +def _load_service_identity_scopes() -> Dict[str, Any]: + """Build the service -> allowed-identity-scope map from env. + + Returns a dict where each value is either the string "*" (wildcard) or a + set[int] of explicitly allowed identity ids. A service with an unset/blank + var is omitted entirely, which means it is allowed NO identities (fail + closed in _key_may_access_identity). + """ + scopes: Dict[str, Any] = {} + for service, env_var in _ALLOWED_IDS_ENV.items(): + raw = os.getenv(env_var) + if raw is None or not raw.strip(): + continue # omitted -> deny all (fail closed) + value = raw.strip() + if value == "*": + scopes[service] = "*" + continue + allowed_ids = set() + for part in value.split(","): + part = part.strip() + if not part: + continue + try: + allowed_ids.add(int(part)) + except ValueError: + logger.warning( + "Ignoring non-integer identity id %r in %s for %s", + part, env_var, service, + ) + if allowed_ids: + scopes[service] = allowed_ids + return scopes + + +# service_name -> "*" | set[int]; absent service => allowed no identities. +SERVICE_IDENTITY_SCOPES = _load_service_identity_scopes() + + +def require_identity_scope(service_name: str, identity_id: int) -> None: + """Fail closed unless ``service_name``'s key is scoped to ``identity_id``. + + SECURITY (HIGH-2): raises HTTPException(403) when the calling service key is + not permitted to act for the given identity (including when no scope is + configured for the service at all). Closes the IDOR/BOLA TODO so a valid key + can no longer enumerate or operate on arbitrary identities. + """ + scope = SERVICE_IDENTITY_SCOPES.get(service_name) + allowed = scope == "*" or (isinstance(scope, set) and identity_id in scope) + if not allowed: + log_security_event( + "identity_scope_denied", + details={ + "service": service_name, + "identity_id": identity_id, + "reason": "service_not_scoped_for_identity", + }, + ) + raise HTTPException( + status_code=403, + detail="Service key is not authorized for this identity", + ) + # Request/Response Models with enhanced documentation class CredentialRequest(BaseModel): """Request model for generating credentials for an identity""" @@ -459,11 +548,16 @@ async def request_identity_credentials( """Request credentials for an identity to sign up for a specific site""" try: + # SECURITY (HIGH-2 / appsec #18): per-tenant key scoping — the calling key + # must be authorized for this identity before we generate credentials from + # its PII. + require_identity_scope(service_name, request.identity_id) + # Get identity identity = db.query(Identity).filter(Identity.id == request.identity_id).first() if not identity: raise HTTPException(status_code=404, detail="Identity not found") - + logger.info(f"Generating credentials for identity {identity.name} on {request.site_name} by {service_name}") # Generate credentials based on identity and site requirements @@ -540,9 +634,13 @@ async def request_account_credentials( # so a valid service key is trusted to act on behalf of any identity it # names. This closes cross-account IDOR via raw id but does NOT yet bind a # key to a specific set of identities. - # TODO: introduce per-tenant key scoping (map each service key to the - # tenant/identities it may act for) once a tenant model exists, and verify - # request.identity_id is within that key's allowed scope. + # SECURITY (HIGH-2 / appsec #18 — RESOLVED): per-tenant key scoping. The + # calling key must be authorized for request.identity_id before we look up + # (and decrypt) the account's credentials. Combined with the identity-scoped + # account filter below, a key can neither name an arbitrary identity nor + # reach an account outside the identity it was granted. + require_identity_scope(service_name, request.identity_id) + account = ( db.query(Account) .join(Identity, Account.identity_id == Identity.id) @@ -647,10 +745,14 @@ async def store_account_credentials( """Store/update credentials for an account after successful signup""" try: + # SECURITY (HIGH-2 / appsec #18): per-tenant key scoping — the calling key + # must be authorized for request.identity_id before mutating any of its + # accounts' stored credentials. + require_identity_scope(service_name, request.identity_id) + # SECURITY (VULN 2 - IDOR): scope the mutation to the supplied owning - # identity (see request_account_credentials for the full rationale and the - # residual per-tenant-scoping TODO). 404 (not 403) avoids leaking the - # existence of accounts owned by a different identity. + # identity. 404 (not 403) here avoids leaking the existence of accounts + # owned by a different identity within the key's allowed scope. account = ( db.query(Account) .join(Identity, Account.identity_id == Identity.id) @@ -761,11 +863,18 @@ async def get_identity_accounts( """Get all accounts for an identity""" try: + # SECURITY (HIGH-2 / appsec #18): enforce per-tenant key scoping BEFORE any + # lookup, so a valid key cannot enumerate an identity it is not scoped for. + # 403 (not 404) here is intentional: scope is a property of the calling key, + # not of whether the identity exists, so denying out-of-scope access does + # not leak identity existence (we never query for it). + require_identity_scope(service_name, identity_id) + # Get identity identity = db.query(Identity).filter(Identity.id == identity_id).first() if not identity: raise HTTPException(status_code=404, detail="Identity not found") - + # Get accounts for this identity accounts = db.query(Account).filter(Account.identity_id == identity_id).all() diff --git a/backend/app/routers/google_integration.py b/backend/app/routers/google_integration.py index a8e7678..f516243 100644 --- a/backend/app/routers/google_integration.py +++ b/backend/app/routers/google_integration.py @@ -1,16 +1,33 @@ """ Google integration API routes. + +SECURITY (CRITICAL-1 / appsec #18): every route here is now gated by +`get_current_user` (JWT) AND scopes the identity/account it touches to the +calling user via the ownership chain Account.identity_id -> Identity.id -> +Identity.user_id == current_user.id. This mirrors the gold-standard pattern in +`accounts.py` / `identities.py`. A caller can only sign up / read accounts / +convert PII for an identity they own; any other identity returns 404 (we return +404 — not 403 — so we do not leak whether an identity id exists under a +different owner). The previous routes decrypted PII and drove real account +creation by raw path id with no auth, which was an unauthenticated PII +exfiltration + account-creation vector. + +Note: these handlers use the async SQLAlchemy session (the app's `get_db` yields +an AsyncSession) and `select(...)`/`await db.execute(...)`, consistent with the +rest of the codebase. """ import logging from typing import Dict, Any from fastapi import APIRouter, HTTPException, Depends, BackgroundTasks -from sqlalchemy.orm import Session +from sqlalchemy.ext.asyncio import AsyncSession +from sqlalchemy.future import select from app.database import get_db from app.models.identity import Identity from app.models.account import Account from app.models.user import User +from app.routers.auth import get_current_user from app.integrations.google.backend.signup import GoogleSignupService from app.integrations.google.backend.models import GoogleSignupConfig, GoogleSignupData, GoogleSignupResult from app.utils.encryption import encrypt_field @@ -20,28 +37,46 @@ router = APIRouter(prefix="/api/google", tags=["Google Integration"]) +async def _get_owned_identity(identity_id: int, current_user: User, db: AsyncSession) -> Identity: + """Fetch an identity ONLY if it belongs to the current user. + + SECURITY (CRITICAL-1): scopes the lookup to the caller. Returns 404 (not 403) + on no-match so we never leak the existence of another user's identity. + """ + result = await db.execute( + select(Identity).where( + (Identity.id == identity_id) & (Identity.user_id == current_user.id) + ) + ) + identity = result.scalar_one_or_none() + if not identity: + raise HTTPException(status_code=404, detail="Identity not found") + return identity + + @router.post("/signup/{identity_id}") async def signup_with_identity( identity_id: int, config: GoogleSignupConfig = None, background_tasks: BackgroundTasks = None, - db: Session = Depends(get_db) + current_user: User = Depends(get_current_user), + db: AsyncSession = Depends(get_db) ) -> Dict[str, Any]: """ Create a Google account using the specified identity. + + SECURITY: requires auth and that the identity is owned by the caller. """ try: - # Get the identity - identity = db.query(Identity).filter(Identity.id == identity_id).first() - if not identity: - raise HTTPException(status_code=404, detail="Identity not found") - + # Ownership-scoped lookup (404 if not owned / not found). + identity = await _get_owned_identity(identity_id, current_user, db) + # Create signup service signup_service = GoogleSignupService(config or GoogleSignupConfig()) - + # Perform signup result = await signup_service.signup_with_identity(identity) - + # If successful, create account record if result.success and result.account_email: account = Account( @@ -58,9 +93,9 @@ async def signup_with_identity( } ) db.add(account) - db.commit() - db.refresh(account) - + await db.commit() + await db.refresh(account) + return { "success": True, "message": "Google account created successfully", @@ -76,28 +111,34 @@ async def signup_with_identity( "verification_required": result.verification_required, "verification_type": result.verification_type } - + + except HTTPException: + raise except Exception as e: logger.error(f"Error in Google signup: {str(e)}") - raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") + raise HTTPException(status_code=500, detail="Internal server error") @router.post("/signup/manual") async def manual_signup( signup_data: GoogleSignupData, config: GoogleSignupConfig = None, - db: Session = Depends(get_db) + current_user: User = Depends(get_current_user), + db: AsyncSession = Depends(get_db) ) -> Dict[str, Any]: """ Create a Google account with manually provided data. + + SECURITY: requires auth. This route drives real signup automation from the + request body and must not be reachable unauthenticated. """ try: # Create signup service signup_service = GoogleSignupService(config or GoogleSignupConfig()) - + # Perform signup result = await signup_service.signup(signup_data) - + return { "success": result.success, "message": result.error_message if not result.success else "Google account created successfully", @@ -107,16 +148,23 @@ async def manual_signup( "verification_type": result.verification_type, "additional_data": result.additional_data } - + + except HTTPException: + raise except Exception as e: logger.error(f"Error in manual Google signup: {str(e)}") - raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") + raise HTTPException(status_code=500, detail="Internal server error") @router.get("/config/default") -async def get_default_config() -> GoogleSignupConfig: +async def get_default_config( + current_user: User = Depends(get_current_user), +) -> GoogleSignupConfig: """ Get the default configuration for Google signup. + + SECURITY: requires auth. Even though it returns no PII, keep the whole + router authenticated so there is no unauthenticated surface here. """ return GoogleSignupConfig() @@ -124,23 +172,27 @@ async def get_default_config() -> GoogleSignupConfig: @router.post("/test/identity-conversion/{identity_id}") async def test_identity_conversion( identity_id: int, - db: Session = Depends(get_db) + current_user: User = Depends(get_current_user), + db: AsyncSession = Depends(get_db) ) -> Dict[str, Any]: """ Test converting an identity to Google signup data without actually creating an account. + + SECURITY (CRITICAL-1): this endpoint returns DECRYPTED PII (names, username, + phone, recovery email, birth date, gender). It now requires auth and that the + identity belongs to the caller; otherwise 404. This closes the unauthenticated + PII-exfiltration path. """ try: - # Get the identity - identity = db.query(Identity).filter(Identity.id == identity_id).first() - if not identity: - raise HTTPException(status_code=404, detail="Identity not found") - + # Ownership-scoped lookup (404 if not owned / not found). + identity = await _get_owned_identity(identity_id, current_user, db) + # Create signup service signup_service = GoogleSignupService() - + # Convert identity to signup data signup_data = await signup_service._identity_to_signup_data(identity) - + return { "success": True, "message": "Identity conversion successful", @@ -155,27 +207,40 @@ async def test_identity_conversion( "gender": signup_data.gender } } - + + except HTTPException: + raise except Exception as e: logger.error(f"Error in identity conversion test: {str(e)}") - raise HTTPException(status_code=400, detail=f"Conversion error: {str(e)}") + raise HTTPException(status_code=400, detail="Conversion error") @router.get("/accounts/{identity_id}") async def get_google_accounts( identity_id: int, - db: Session = Depends(get_db) + current_user: User = Depends(get_current_user), + db: AsyncSession = Depends(get_db) ) -> Dict[str, Any]: """ Get all Google accounts for a specific identity. + + SECURITY (CRITICAL-1): requires auth; the identity must be owned by the + caller (404 otherwise). The account lookup is additionally scoped through the + owned identity so no cross-tenant account data is returned. """ try: - # Get Google accounts for this identity - accounts = db.query(Account).filter( - Account.identity_id == identity_id, - Account.platform == "google" - ).all() - + # Ownership-scoped identity lookup first (404 if not owned / not found). + await _get_owned_identity(identity_id, current_user, db) + + # Get Google accounts for this (now confirmed owned) identity. + result = await db.execute( + select(Account).where( + (Account.identity_id == identity_id) + & (Account.platform == "google") + ) + ) + accounts = result.scalars().all() + return { "success": True, "accounts": [ @@ -189,7 +254,9 @@ async def get_google_accounts( for account in accounts ] } - + + except HTTPException: + raise except Exception as e: logger.error(f"Error getting Google accounts: {str(e)}") - raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}") \ No newline at end of file + raise HTTPException(status_code=500, detail="Internal server error") diff --git a/backend/app/routers/llm_scraper.py b/backend/app/routers/llm_scraper.py index a1ba219..71a5f03 100644 --- a/backend/app/routers/llm_scraper.py +++ b/backend/app/routers/llm_scraper.py @@ -1,14 +1,28 @@ -from fastapi import APIRouter, HTTPException, BackgroundTasks +from fastapi import APIRouter, HTTPException, BackgroundTasks, Depends from typing import Dict, List, Optional, Any import logging from pydantic import BaseModel from ..llm_scraper_service.llm_integration.code_generator import code_generator, ScraperCode from ..llm_scraper_service.llm_integration.prompt_templates import SiteInfo +from app.models.user import User +from app.routers.auth import get_current_user logger = logging.getLogger(__name__) -router = APIRouter(prefix="/api/llm-scraper", tags=["LLM Scraper"]) +# SECURITY (CRITICAL-2 / appsec #18): this router was public-by-default. Every +# route invoked the LLM (cost/abuse, prompt-injection) or exposed/deleted +# generated scraper source with NO auth. We now require the application JWT +# (`get_current_user`) on EVERY route in this router via a router-level +# dependency, so an unauthenticated caller gets 401 before any handler runs. The +# destructive DELETE and the LLM-invoking POSTs are therefore all gated. There is +# no public `/health` route here; if one is added later it can be exempted by +# declaring it on a separate, unauthenticated router. +router = APIRouter( + prefix="/api/llm-scraper", + tags=["LLM Scraper"], + dependencies=[Depends(get_current_user)], +) # Request/Response Models class GenerateScraperRequest(BaseModel): diff --git a/backend/app/routers/site_integrations.py b/backend/app/routers/site_integrations.py index 3fb4065..00d9aa2 100644 --- a/backend/app/routers/site_integrations.py +++ b/backend/app/routers/site_integrations.py @@ -5,7 +5,7 @@ including signup, signin, and API key creation for various platforms. """ -from fastapi import APIRouter, HTTPException, BackgroundTasks +from fastapi import APIRouter, HTTPException, BackgroundTasks, Depends from pydantic import BaseModel, EmailStr from typing import Dict, Any, List, Optional import logging @@ -13,6 +13,8 @@ from app.integrations.site import get_available_sites, get_site_capabilities from app.integrations.site.permit_io import PermitIOIntegration from app.integrations.site.permit_io.models import PermitIOCredentials, PermitIOResult +from app.models.user import User +from app.routers.auth import get_current_user logger = logging.getLogger(__name__) @@ -79,12 +81,20 @@ async def get_site_capabilities_endpoint(site_name: str): # Site Integration Operations @router.post("/signup", response_model=IntegrationResponse) -async def create_account(request: SignupRequest, background_tasks: BackgroundTasks): +async def create_account( + request: SignupRequest, + background_tasks: BackgroundTasks, + current_user: User = Depends(get_current_user), +): """ Create a new account on the specified site. - + This endpoint handles automated account signup for supported sites. The operation runs in the background for long-running automations. + + SECURITY (HIGH-1 / appsec #18): requires auth. This drives credential-bearing + browser automation against external sites; an unauthenticated caller must not + be able to trigger it (abuse / resource exhaustion). """ try: if request.site.lower() == "permit.io": @@ -101,11 +111,19 @@ async def create_account(request: SignupRequest, background_tasks: BackgroundTas raise HTTPException(status_code=500, detail="Account creation failed") @router.post("/signin", response_model=IntegrationResponse) -async def authenticate_account(request: SigninRequest): +async def authenticate_account( + request: SigninRequest, + current_user: User = Depends(get_current_user), +): """ Authenticate with an existing account on the specified site. - + This endpoint handles automated authentication for supported sites. + + SECURITY (HIGH-1 / appsec #18): requires auth. This accepts raw email+password + and drives credential-bearing browser automation against external sites; an + unauthenticated caller must not be able to trigger it (abuse / resource + exhaustion / credential-stuffing surface). """ try: if request.site.lower() == "permit.io": @@ -122,11 +140,18 @@ async def authenticate_account(request: SigninRequest): raise HTTPException(status_code=500, detail="Authentication failed") @router.post("/apikey", response_model=IntegrationResponse) -async def create_api_key(request: ApiKeyRequest): +async def create_api_key( + request: ApiKeyRequest, + current_user: User = Depends(get_current_user), +): """ Create an API key for the specified site account. - + This endpoint handles automated API key creation for supported sites. + + SECURITY (HIGH-1 / appsec #18): requires auth. This accepts raw email+password + and drives credential-bearing browser automation against external sites; an + unauthenticated caller must not be able to trigger it. """ try: if request.site.lower() == "permit.io": diff --git a/backend/tests/test_security_regressions.py b/backend/tests/test_security_regressions.py index d0b265d..865bda7 100644 --- a/backend/tests/test_security_regressions.py +++ b/backend/tests/test_security_regressions.py @@ -172,6 +172,19 @@ def test_loaded_key_round_trips_through_verify_api_key(self, monkeypatch): # filter (i.e. looking up by account_id alone), which is the IDOR. # =========================================================================== class TestIdorOwnershipScoping: + @pytest.fixture(autouse=True) + def _grant_full_identity_scope(self): + """These tests isolate the per-IDENTITY IDOR layer (Account.identity_id + scoping). The newer per-KEY scope gate (HIGH-2, require_identity_scope) + sits in FRONT of it, so we grant the calling service a wildcard scope here + and restore it afterwards; otherwise the request would 403 on key-scope + before ever reaching the IDOR check under test. The key-scope behaviour is + covered independently in TestServiceKeyIdentityScoping.""" + original = credentials_mod.SERVICE_IDENTITY_SCOPES + credentials_mod.SERVICE_IDENTITY_SCOPES = {"scraper-service": "*"} + yield + credentials_mod.SERVICE_IDENTITY_SCOPES = original + @pytest.fixture def db_session(self): """A synchronous in-memory SQLite session with the User/Identity/Account @@ -535,3 +548,197 @@ def _spy(n): assert sms_mod.registered_device_keys["dev-new"] == issued assert sms_mod._verify_device("dev-new", issued) is True assert sms_mod._verify_device("dev-new", "not-the-key") is False + + +# =========================================================================== +# AREA 4: Per-tenant service-key scoping (HIGH-2 / appsec #18) +# +# Fix under test (credentials.require_identity_scope + _load_service_identity_scopes +# + its enforcement on get_identity_accounts / request_account_credentials / +# store_account_credentials / request_identity_credentials): +# - A service key may only act for identities it is explicitly scoped to via +# _ALLOWED_IDENTITY_IDS. Unset/blank => NO identities (fail closed). +# - "*" is an explicit operator opt-in to act for ANY identity. +# - A scoped list ("1,2") admits only those ids; anything else -> 403. +# - get_identity_accounts now calls require_identity_scope BEFORE any lookup, so +# a valid key can no longer enumerate an arbitrary identity's accounts (the +# cross-tenant BOLA the old TODO left open). +# +# Reversion this would catch: removing require_identity_scope, defaulting an +# unconfigured service to "allow all", or dropping the scope check on +# get_identity_accounts (re-opening cross-tenant enumeration). +# +# Same cv2-free style as the rest of this file: drive the real functions directly. +# =========================================================================== +class TestServiceKeyIdentityScoping: + @pytest.fixture(autouse=True) + def _isolate_scopes(self): + """Snapshot/restore the module-level SERVICE_IDENTITY_SCOPES around each + test so we never leak scope config between tests.""" + original = credentials_mod.SERVICE_IDENTITY_SCOPES + yield + credentials_mod.SERVICE_IDENTITY_SCOPES = original + + # --- require_identity_scope unit behaviour -------------------------------- + def test_unconfigured_service_denies_all_identities_403(self): + """Fail closed: a service with NO configured scope may act for no identity.""" + credentials_mod.SERVICE_IDENTITY_SCOPES = {} + with pytest.raises(HTTPException) as exc: + credentials_mod.require_identity_scope("scraper-service", 1) + assert exc.value.status_code == 403 + + def test_identity_outside_scope_denied_403(self): + """A scoped key naming an identity NOT in its allow-list -> 403.""" + credentials_mod.SERVICE_IDENTITY_SCOPES = {"scraper-service": {1, 2}} + with pytest.raises(HTTPException) as exc: + credentials_mod.require_identity_scope("scraper-service", 99) + assert exc.value.status_code == 403 + + def test_identity_in_scope_allowed(self): + """An identity within the key's allow-list is permitted (no raise).""" + credentials_mod.SERVICE_IDENTITY_SCOPES = {"scraper-service": {1, 2}} + assert credentials_mod.require_identity_scope("scraper-service", 2) is None + + def test_wildcard_scope_allows_any_identity(self): + """Explicit "*" opt-in permits any identity.""" + credentials_mod.SERVICE_IDENTITY_SCOPES = {"mobile-service": "*"} + assert credentials_mod.require_identity_scope("mobile-service", 12345) is None + + # --- _load_service_identity_scopes env parsing ---------------------------- + def test_loader_fail_closed_and_wildcard_and_list(self, monkeypatch): + monkeypatch.delenv("SCRAPER_ALLOWED_IDENTITY_IDS", raising=False) + monkeypatch.delenv("MOBILE_ALLOWED_IDENTITY_IDS", raising=False) + monkeypatch.delenv("AUTOMATION_ALLOWED_IDENTITY_IDS", raising=False) + # All unset -> empty map (every service denied at request time). + assert credentials_mod._load_service_identity_scopes() == {} + + # Blank -> still omitted (fail closed). + monkeypatch.setenv("SCRAPER_ALLOWED_IDENTITY_IDS", " ") + assert credentials_mod._load_service_identity_scopes() == {} + + # Wildcard + explicit list, with junk ids ignored. + monkeypatch.setenv("SCRAPER_ALLOWED_IDENTITY_IDS", "*") + monkeypatch.setenv("MOBILE_ALLOWED_IDENTITY_IDS", "1, 2 ,bad, 3") + loaded = credentials_mod._load_service_identity_scopes() + assert loaded["scraper-service"] == "*" + assert loaded["mobile-service"] == {1, 2, 3} + + # --- get_identity_accounts enforces the scope BEFORE any DB lookup -------- + def test_get_identity_accounts_out_of_scope_denied_403(self): + """The endpoint must 403 an out-of-scope identity WITHOUT touching the DB + (so a bad db would not even be queried). Closes the enumeration BOLA.""" + credentials_mod.SERVICE_IDENTITY_SCOPES = {"scraper-service": {1}} + + class _ExplodingDb: + def query(self, *a, **k): + raise AssertionError("DB must not be queried for an out-of-scope identity") + + with pytest.raises(HTTPException) as exc: + _run(credentials_mod.get_identity_accounts( + identity_id=99, # not in {1} + service_name="scraper-service", + db=_ExplodingDb(), + )) + assert exc.value.status_code == 403 + + def test_request_account_credentials_out_of_scope_denied_403(self): + """The IDOR-hardened retrieval path now ALSO fails closed on key scope: + an out-of-scope identity is rejected 403 before any account lookup.""" + credentials_mod.SERVICE_IDENTITY_SCOPES = {"scraper-service": {1}} + + class _ExplodingDb: + def query(self, *a, **k): + raise AssertionError("DB must not be queried for an out-of-scope identity") + + req = credentials_mod.AccountCredentialRequest( + identity_id=99, account_id=5, credential_types=[], + ) + with pytest.raises(HTTPException) as exc: + _run(credentials_mod.request_account_credentials(req, "scraper-service", _ExplodingDb())) + assert exc.value.status_code == 403 + + +# =========================================================================== +# AREA 5: Auth coverage on site_integrations + llm_scraper routes +# (HIGH-1 / CRITICAL-2 — appsec #18) +# +# Rather than spin up the app (cv2-broken locally), we introspect the registered +# FastAPI routes/dependencies of the specific routers — importing these router +# modules is cv2-free (verified). We assert that get_current_user gates the routes +# the audit flagged. A reversion that drops the dependency makes these fail. +# +# - site_integrations: POST /signup, /signin, /apikey each depend on +# get_current_user (the HIGH-1 abuse/credential-stuffing surface). +# - llm_scraper: get_current_user is a ROUTER-LEVEL dependency, so EVERY route +# (incl. the destructive DELETE and the LLM-invoking POSTs) is gated. +# =========================================================================== +import app.routers.site_integrations as site_mod # cv2-free +import app.routers.llm_scraper as llm_mod # cv2-free +from app.routers.auth import get_current_user + + +def _route_dependency_calls(route): + """Return the set of dependency callables attached to a route (its own + dependant + nested sub-dependencies).""" + calls = set() + dependant = getattr(route, "dependant", None) + if dependant is None: + return calls + if getattr(dependant, "call", None) is not None: + calls.add(dependant.call) + for sub in getattr(dependant, "dependencies", []): + if getattr(sub, "call", None) is not None: + calls.add(sub.call) + for subsub in getattr(sub, "dependencies", []): + if getattr(subsub, "call", None) is not None: + calls.add(subsub.call) + return calls + + +class TestRouteAuthCoverage: + @pytest.mark.parametrize( + "method,path", + [ + ("POST", "/api/v1/integrations/signup"), + ("POST", "/api/v1/integrations/signin"), + ("POST", "/api/v1/integrations/apikey"), + ], + ) + def test_site_integrations_operational_routes_require_auth(self, method, path): + """HIGH-1: signup/signin/apikey must be gated by get_current_user.""" + matched = [ + r for r in site_mod.router.routes + if getattr(r, "path", None) == path and method in getattr(r, "methods", set()) + ] + assert matched, f"route {method} {path} not found" + for route in matched: + assert get_current_user in _route_dependency_calls(route), ( + f"{method} {path} is not gated by get_current_user" + ) + + def test_llm_scraper_router_level_auth_gates_every_route(self): + """CRITICAL-2: get_current_user is a router-level dependency, so every + route (DELETE + LLM POSTs + reads) inherits it.""" + # Router-level dependency present. + router_dep_calls = { + d.dependency for d in llm_mod.router.dependencies + if getattr(d, "dependency", None) is not None + } + assert get_current_user in router_dep_calls, ( + "llm_scraper router is missing the router-level get_current_user dependency" + ) + # And it actually propagates to the routes (spot-check the destructive DELETE + # and an LLM-invoking POST). + for method, path in [ + ("DELETE", "/api/llm-scraper/scrapers/{site_name}/{action_type}"), + ("POST", "/api/llm-scraper/generate"), + ]: + matched = [ + r for r in llm_mod.router.routes + if getattr(r, "path", None) == path and method in getattr(r, "methods", set()) + ] + assert matched, f"route {method} {path} not found" + for route in matched: + assert get_current_user in _route_dependency_calls(route), ( + f"{method} {path} is not gated by get_current_user" + )