[Bug / Security] Dead Auth Guard in Challenge Check & Hint Endpoints: Anonymous Sessions Bypass Attempt Tracking and Hint Consumption

[prince-shakyaa]

[Bug / Security] Dead Auth Guard in Challenge Check & Hint Endpoints: Anonymous Sessions Bypass Attempt Tracking and Hint Consumption

Summary

Two challenge-interaction endpoints : POST /api/v1/challenges/{challenge_id}/check and
POST /api/v1/challenges/{challenge_id}/hint :contain an authentication guard that
can never fire. The guard reads:

if not session_context:
    raise HTTPException(status_code=401, detail="Authentication required")

But session_context is injected via the get_session_context FastAPI dependency
(finbot/core/auth/middleware.py L156–161), which always returns a valid
SessionContext object : it simply returns request.state.session_context, which the
SessionMiddleware sets for every request including anonymous/temporary sessions.

Because session_context is never None or falsy, the if not session_context
condition always evaluates to False. The guard is dead code. Unauthenticated
visitors (temporary session, no email bound) can call both endpoints freely.

---

Affected Files & Lines

File	Line(s)	Dead Guard
finbot/apps/ctf/routes/challenges.py	222–223	check_challenge endpoint
finbot/apps/ctf/routes/challenges.py	268–269	use_hint endpoint

---

Root Cause : Why the Guard Can Never Fire

get_session_context is defined in finbot/core/auth/middleware.py:

# middleware.py: L156–161
async def get_session_context(request: Request) -> SessionContext:
    """FastAPI dependency to get normal session context
    - This is used for routes that don't explicitly require authentication
    - May or may not be bound to an email address (temporary vs persistent)
    """
    return request.state.session_context

And SessionMiddleware.dispatch unconditionally sets request.state.session_context
for every request (temporary or not) before the route handler runs:

# middleware.py: L116–125
new_session = session_manager.create_session(...)
return new_session, "session_created"

There is no code path in the middleware where request.state.session_context is left
unset or set to None. Therefore, get_session_context always returns a truthy
SessionContext, and the guard if not session_context evaluates to False 100% of
the time.

---

Vulnerable Code

check_challenge (challenges.py L210–258)

@router.post("/challenges/{challenge_id}/check", response_model=CheckResult)
def check_challenge(
    challenge_id: str,
    session_context: SessionContext = Depends(get_session_context),   # ← weaker dep
    db: Session = Depends(get_db),
):
    if not session_context:                 # ← DEAD CODE: always False
        raise HTTPException(status_code=401, detail="Authentication required")
    ...
    progress_repo.record_attempt(challenge_id)    # ← runs for temporary sessions
    ...

use_hint (challenges.py L261–303)

@router.post("/challenges/{challenge_id}/hint", response_model=HintResponse)
def use_hint(
    challenge_id: str,
    session_context: SessionContext = Depends(get_session_context),   # ← weaker dep
    db: Session = Depends(get_db),
):
    if not session_context:                 # ← DEAD CODE: always False
        raise HTTPException(status_code=401, detail="Authentication required")
    ...
    updated_progress = progress_repo.use_hint(challenge_id, cost)   # ← runs for temp sessions
    ...

---

Comparison with Other Endpoints

Every other endpoint that guards personal-data writes uses the correct dependency:

Endpoint	Auth Dependency
GET /api/v1/profile	get_authenticated_session_context ✅
PUT /api/v1/profile	get_authenticated_session_context ✅
PUT /api/v1/profile/featured-badges	get_authenticated_session_context ✅
GET /api/v1/sidecar	get_authenticated_session_context ✅ (fixed in #511/#512)
POST /api/v1/challenges/{id}/check	get_session_context + dead if not check ❌
POST /api/v1/challenges/{id}/hint	get_session_context + dead if not check ❌

The correct dependency, get_authenticated_session_context, raises HTTP 401 whenever
session_context.is_temporary is True. The manual if not session_context guard was
clearly intended as a substitute, but it is structurally broken because the dependency
never yields None.

---

Impact

1. Unauthenticated Attempt Inflation (check_challenge)

progress_repo.record_attempt(challenge_id) is called inside check_challenge for any
caller that has no prior progress record. Because temporary sessions are accepted:

• A bot or automated script can spam POST /api/v1/challenges/{id}/check without
  ever binding an email address.
• Each call increments the attempt counter stored in UserChallengeProgress under the
  temporary session's user_id.
• While this doesn't affect other users' progress, it creates phantom progress rows
  in the database for throwaway sessions, polluting the UserChallengeProgress table
  indefinitely.
• The intended design is that only authenticated (email-bound) users have meaningful
  challenge progress tracked.

2. Unauthenticated Hint Consumption (use_hint)

progress_repo.use_hint(challenge_id, cost) is called inside use_hint. For a
temporary session:

• A temporary user can call POST /api/v1/challenges/{id}/hint repeatedly.
• The server returns real hint text (hint["text"]) for each hint that hasn't been used
  under that temporary session.
• The hint text is fully revealed : there is no sanitization for unauthenticated
  callers.
• The hints_cost deduction is recorded against the temporary session's progress row
  (which is meaningless, but the hint content itself is exposed without authentication).
• This means all challenge hints can be scraped without ever registering or binding
  an email address.

Summary of Effects

Scenario	Effect
Temporary session calls POST /api/v1/challenges/{id}/check	Returns 200 OK with challenge status instead of 401 Unauthorized. Attempt counter is incremented for throwaway session.
Temporary session calls POST /api/v1/challenges/{id}/hint	Returns 200 OK with full hint text instead of 401 Unauthorized. Hints are revealed to unauthenticated callers.
Automated scraper with no email	Can extract every hint for every challenge by cycling through challenge IDs without authentication.

---

Steps to Reproduce

Reproduce the Dead Guard (check_challenge)

# Step 1: Get a fresh session cookie (no email bound — just visit the app once)
#         The middleware auto-creates a temporary session.

# Step 2: Call check on any valid challenge ID with the temporary session cookie
curl -X POST https://owasp-finbot-ctf.org/api/v1/challenges/some-challenge-id/check \
  -H "Cookie: session=<your_temporary_session_cookie>"

# Expected: HTTP 401 Unauthorized
# Actual:   HTTP 200 OK  →  {"status": "available", "detected": false, ...}

Reproduce the Hint Leak (use_hint)

# Step 1: Same : get a temporary session cookie with no email bound.

# Step 2: Call hint endpoint to retrieve hint text
curl -X POST https://owasp-finbot-ctf.org/api/v1/challenges/some-challenge-id/hint \
  -H "Cookie: session=<your_temporary_session_cookie>"

# Expected: HTTP 401 Unauthorized
# Actual:   HTTP 200 OK  →  {"hint_index": 0, "hint_text": "<actual hint revealed>", ...}

---

Proposed Fix

Replace the broken get_session_context + dead if not pattern with
get_authenticated_session_context - the same dependency used by every other
personal-data endpoint.

challenges.py : check_challenge

-from finbot.core.auth.middleware import get_session_context
+from finbot.core.auth.middleware import get_authenticated_session_context

 @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),
 ):
-    if not session_context:
-        raise HTTPException(status_code=401, detail="Authentication required")
     ...

challenges.py : use_hint

 @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),
 ):
-    if not session_context:
-        raise HTTPException(status_code=401, detail="Authentication required")
     ...

The dead if not session_context checks can be removed entirely since
get_authenticated_session_context raises HTTP 401 on its own when the session is
temporary.

Note: GET /api/v1/challenges and GET /api/v1/challenges/{id} intentionally use
get_session_context because they are designed to be publicly browsable (they show
locked hints and anonymised status for unauthenticated visitors). Only the write
operations — check and hint — should require a persistent session.

---

Additional Scope : Wider Audit Note

As noted in #511's "Additional Notes" section, activity.py, stats.py,
badges.py, and toolkit.py also use get_session_context. Unlike the endpoints
above, those routes do not contain a dead guard — they either intentionally support
anonymous browsing or handle the unauthenticated case inline. However, toolkit.py's
dead-drop and exfil endpoints expose intercepted emails and network captures without
requiring a persistent session. A follow-up review of whether those routes should also
enforce get_authenticated_session_context may be warranted.

The immediate priority is check_challenge and use_hint because they:

1. Contain dead code that gives a false sense of security.
2. Leak full hint text to unauthenticated callers.
3. Write phantom rows to the database for throwaway sessions.

---

Affected Files

File	Lines	Issue
finbot/apps/ctf/routes/challenges.py	210–258	check_challenge uses get_session_context; dead if not guard at L222–223
finbot/apps/ctf/routes/challenges.py	261–303	use_hint uses get_session_context; dead if not guard at L268–269

---

Type of Issue

• Bug (dead code : the if not session_context guard can never fire)
• Security (hint text leaked to unauthenticated/temporary sessions)
• Data Integrity (phantom attempt rows written by throwaway sessions)

[c-thiam]

Since the CTF is designed to be playable without authentication, I don't think this should be treated as a Broken Access Control/security issue. Anonymous/temporary sessions calling /check and /hint appear to be intended behavior — hints are meant to be available to all players, so exposing them to unauthenticated sessions isn't a leak, it's the intended experience.
The valid part is the dead guard: if not session_context is misleading because get_session_context always returns a session context. I'd suggest fixing the dead code and commenting the intended anonymous flow, rather than switching these endpoints to get_authenticated_session_context.
If there is concern about abuse, it should be handled separately with rate limiting, cleanup of temporary-session progress, or differentiated hint limits — not by requiring authentication.