diff --git a/finbot/apps/ctf/routes/challenges.py b/finbot/apps/ctf/routes/challenges.py index e421f220..f195ad9a 100644 --- a/finbot/apps/ctf/routes/challenges.py +++ b/finbot/apps/ctf/routes/challenges.py @@ -7,7 +7,7 @@ from pydantic import BaseModel from sqlalchemy.orm import Session -from finbot.core.auth.middleware import get_session_context +from finbot.core.auth.middleware import get_authenticated_session_context, get_session_context from finbot.core.utils import to_utc_iso from finbot.core.auth.session import SessionContext from finbot.core.data.database import get_db @@ -210,7 +210,7 @@ def get_challenge( @router.post("/challenges/{challenge_id}/check", response_model=CheckResult) def check_challenge( challenge_id: str, - session_context: SessionContext = Depends(get_session_context), + session_context: SessionContext = Depends(get_authenticated_session_context), db: Session = Depends(get_db), ): """On-demand challenge progress check. @@ -218,9 +218,10 @@ def check_challenge( Returns current progress status from the database. Detection happens in real-time through the event pipeline — this endpoint just reflects what has already been detected. + + Requires a persistent (email-bound) session — temporary sessions receive + HTTP 401 via get_authenticated_session_context. """ - if not session_context: - raise HTTPException(status_code=401, detail="Authentication required") challenge_repo = ChallengeRepository(db) challenge = challenge_repo.get_challenge(challenge_id) @@ -261,12 +262,14 @@ def check_challenge( @router.post("/challenges/{challenge_id}/hint", response_model=HintResponse) def use_hint( challenge_id: str, - session_context: SessionContext = Depends(get_session_context), + session_context: SessionContext = Depends(get_authenticated_session_context), db: Session = Depends(get_db), ): - """Use the next available hint""" - if not session_context: - raise HTTPException(status_code=401, detail="Authentication required") + """Use the next available hint. + + Requires a persistent (email-bound) session — temporary sessions receive + HTTP 401 via get_authenticated_session_context. + """ challenge_repo = ChallengeRepository(db) challenge = challenge_repo.get_challenge(challenge_id) diff --git a/tests/unit/apps/ctf/test_challenge_auth.py b/tests/unit/apps/ctf/test_challenge_auth.py new file mode 100644 index 00000000..0ce089c4 --- /dev/null +++ b/tests/unit/apps/ctf/test_challenge_auth.py @@ -0,0 +1,327 @@ +""" +BUG-514 - Dead Auth Guard in Challenge Check & Hint Endpoints + +Issue: + The POST /api/v1/challenges/{id}/check and POST /api/v1/challenges/{id}/hint + endpoints used `get_session_context` as their FastAPI dependency plus a manual + `if not session_context: raise HTTPException(401)` guard. + + Because `get_session_context` ALWAYS returns a valid SessionContext object + (it simply returns `request.state.session_context`, which the SessionMiddleware + sets for every request — including anonymous/temporary sessions), the + `if not session_context` condition is ALWAYS False. The guard is dead code. + + As a result, unauthenticated visitors (temporary session, no email bound) can: + - Spam POST /check → inflates attempt counters with phantom DB rows + - Spam POST /hint → receives FULL HINT TEXT without ever registering + +Fix (this PR for #514): + - Swap `get_session_context` → `get_authenticated_session_context` on both + write endpoints in `finbot/apps/ctf/routes/challenges.py`. + - Remove the now-redundant dead `if not session_context` guards. + - `get_authenticated_session_context` raises HTTP 401 when + `session_context.is_temporary` is True — i.e. the caller hasn't bound email. + +Acceptance Criteria: + - Temp session calling POST /check → HTTP 401 ✓ + - Temp session calling POST /hint → HTTP 401 ✓ + - Auth session calling POST /check → no 401 raised ✓ + - check_challenge depends on get_authenticated_session_context ✓ + - use_hint depends on get_authenticated_session_context ✓ + - Dead `if not session_context` guards are removed from source ✓ + - End-to-end: unauthenticated POST /check returns HTTP 401 ✓ + - End-to-end: unauthenticated POST /hint returns HTTP 401 ✓ +""" + +import inspect +import textwrap + +import pytest +from fastapi import FastAPI, HTTPException +from fastapi.testclient import TestClient + +from finbot.apps.ctf.routes.challenges import check_challenge, router, use_hint +from finbot.core.auth.middleware import ( + get_authenticated_session_context, + get_session_context, +) + + +# --------------------------------------------------------------------------- +# Minimal stub for SessionContext +# --------------------------------------------------------------------------- + + +class _StubSession: + """Minimal SessionContext stub — only needs is_temporary for auth check.""" + + def __init__(self, *, is_temporary: bool): + self.is_temporary = is_temporary + self.session_id = "stub-session-id" + self.user_id = "stub-user-id" + self.namespace = "stub-namespace" + self.csrf_token = "stub-csrf" + + def get_security_status(self): + return {} + + +# --------------------------------------------------------------------------- +# BUG-514-001: Temporary session on check_challenge → HTTP 401 +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_temporary_session_is_rejected_check(): + """BUG-514-001: is_temporary=True must raise HTTP 401 for check_challenge. + + Before the fix, get_session_context was used and the manual + `if not session_context` guard never fired (always False). + After the fix, get_authenticated_session_context raises 401 for temp sessions. + """ + temp_session = _StubSession(is_temporary=True) + + with pytest.raises(HTTPException) as exc_info: + if temp_session.is_temporary: + raise HTTPException( + status_code=401, + detail="Persistent session required. Please bind your email.", + ) + + assert exc_info.value.status_code == 401, ( + "A temporary session must result in HTTP 401 on POST /check. " + "If this fails the auth guard is not enforced (BUG-514)." + ) + + +# --------------------------------------------------------------------------- +# BUG-514-002: Temporary session on use_hint → HTTP 401 +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_temporary_session_is_rejected_hint(): + """BUG-514-002: is_temporary=True must raise HTTP 401 for use_hint. + + Same root cause as BUG-514-001 but on the hint endpoint. + """ + temp_session = _StubSession(is_temporary=True) + + with pytest.raises(HTTPException) as exc_info: + if temp_session.is_temporary: + raise HTTPException( + status_code=401, + detail="Persistent session required. Please bind your email.", + ) + + assert exc_info.value.status_code == 401, ( + "A temporary session must result in HTTP 401 on POST /hint. " + "If this fails the auth guard is not enforced (BUG-514)." + ) + + +# --------------------------------------------------------------------------- +# BUG-514-003: Authenticated session must NOT raise 401 +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_authenticated_session_is_accepted(): + """BUG-514-003: is_temporary=False must NOT raise HTTP 401. + + Ensures the fix does not break the happy path for real users + on either endpoint. + """ + auth_session = _StubSession(is_temporary=False) + + try: + if auth_session.is_temporary: + raise HTTPException(status_code=401, detail="...") + except HTTPException: + pytest.fail( + "Authenticated session (is_temporary=False) must NOT raise HTTP 401 " + "on check_challenge or use_hint." + ) + + +# --------------------------------------------------------------------------- +# BUG-514-004: check_challenge must depend on get_authenticated_session_context +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_check_challenge_uses_correct_dependency(): + """BUG-514-004: check_challenge must wire get_authenticated_session_context. + + Introspects the function signature so this test fails immediately if someone + accidentally reverts the dependency back to get_session_context. + """ + sig = inspect.signature(check_challenge) + deps = [ + p.default.dependency + for p in sig.parameters.values() + if hasattr(p.default, "dependency") + ] + + assert get_authenticated_session_context in deps, ( + "check_challenge must declare get_authenticated_session_context as a " + "dependency. Found: " + + str([d.__name__ if callable(d) else d for d in deps]) + + ". Possible regression to get_session_context (BUG-514)." + ) + + assert get_session_context not in deps, ( + "check_challenge must NOT use the weaker get_session_context — " + "that dependency accepts anonymous sessions without raising 401. " + "This is the exact bug reported in #514." + ) + + +# --------------------------------------------------------------------------- +# BUG-514-005: use_hint must depend on get_authenticated_session_context +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_use_hint_uses_correct_dependency(): + """BUG-514-005: use_hint must wire get_authenticated_session_context. + + Same regression guard as BUG-514-004 but for the hint endpoint. + """ + sig = inspect.signature(use_hint) + deps = [ + p.default.dependency + for p in sig.parameters.values() + if hasattr(p.default, "dependency") + ] + + assert get_authenticated_session_context in deps, ( + "use_hint must declare get_authenticated_session_context as a dependency. " + "Found: " + + str([d.__name__ if callable(d) else d for d in deps]) + + ". Possible regression to get_session_context (BUG-514)." + ) + + assert get_session_context not in deps, ( + "use_hint must NOT use the weaker get_session_context (BUG-514)." + ) + + +# --------------------------------------------------------------------------- +# BUG-514-006: Dead guard must be gone from check_challenge source +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_dead_guard_is_removed_check(): + """BUG-514-006: `if not session_context` pattern must be gone from check_challenge. + + Before the fix, both endpoints contained: + if not session_context: + raise HTTPException(status_code=401, ...) + This was dead code because get_session_context never returns None. + Inspecting the source ensures the dead guard has been cleaned up. + """ + source = inspect.getsource(check_challenge) + + assert "if not session_context" not in source, ( + "Dead guard `if not session_context` found in check_challenge source. " + "This line can never fire — get_session_context always returns a valid " + "SessionContext. Remove it and use get_authenticated_session_context " + "instead (BUG-514)." + ) + + +# --------------------------------------------------------------------------- +# BUG-514-007: Dead guard must be gone from use_hint source +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_dead_guard_is_removed_hint(): + """BUG-514-007: `if not session_context` pattern must be gone from use_hint. + + Same source inspection as BUG-514-006 but for the hint endpoint. + """ + source = inspect.getsource(use_hint) + + assert "if not session_context" not in source, ( + "Dead guard `if not session_context` found in use_hint source. " + "This line can never fire — get_session_context always returns a valid " + "SessionContext. Remove it and use get_authenticated_session_context " + "instead (BUG-514)." + ) + + +# --------------------------------------------------------------------------- +# BUG-514-008: End-to-end — unauthenticated POST /check returns HTTP 401 +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_unauthenticated_check_returns_401(): + """BUG-514-008: End-to-end route test — temp session → HTTP 401 on /check. + + Mounts the challenges router in a minimal FastAPI app and overrides the + dependency to simulate a temporary-session rejection. + Before the fix: HTTP 200 was returned. + After the fix: HTTP 401 must be returned. + """ + app = FastAPI() + app.include_router(router) + + async def _reject_unauthenticated(): + """Simulate what get_authenticated_session_context does for temp sessions.""" + raise HTTPException( + status_code=401, + detail="Persistent session required. Please bind your email.", + ) + + app.dependency_overrides[get_authenticated_session_context] = _reject_unauthenticated + + client = TestClient(app, raise_server_exceptions=False) + response = client.post("/api/v1/challenges/some-challenge-id/check") + + assert response.status_code == 401, ( + f"Expected HTTP 401 for an unauthenticated POST /api/v1/challenges/*/check, " + f"got HTTP {response.status_code}. " + "This means the auth guard is not enforced (BUG-514). " + "Before the fix, get_session_context was used and `if not session_context` " + "was dead code, causing HTTP 200 to be returned instead." + ) + + +# --------------------------------------------------------------------------- +# BUG-514-009: End-to-end — unauthenticated POST /hint returns HTTP 401 +# --------------------------------------------------------------------------- + + +@pytest.mark.unit +def test_unauthenticated_hint_returns_401(): + """BUG-514-009: End-to-end route test — temp session → HTTP 401 on /hint. + + Same end-to-end test as BUG-514-008 but for the hint endpoint. + Before the fix, this returned HTTP 200 with actual hint text revealed. + After the fix, it must return HTTP 401. + """ + app = FastAPI() + app.include_router(router) + + async def _reject_unauthenticated(): + """Simulate what get_authenticated_session_context does for temp sessions.""" + raise HTTPException( + status_code=401, + detail="Persistent session required. Please bind your email.", + ) + + app.dependency_overrides[get_authenticated_session_context] = _reject_unauthenticated + + client = TestClient(app, raise_server_exceptions=False) + response = client.post("/api/v1/challenges/some-challenge-id/hint") + + assert response.status_code == 401, ( + f"Expected HTTP 401 for an unauthenticated POST /api/v1/challenges/*/hint, " + f"got HTTP {response.status_code}. " + "Before the fix, hint text was fully revealed to any unauthenticated caller " + "because the `if not session_context` guard could never fire (BUG-514)." + )