fix(security): Remove Dead Auth Guard in Challenge Check & Hint Endpoints

[prince-shakyaa]

fix(security): Remove Dead Auth Guard in Challenge Check & Hint Endpoints

Fixes #514

---

What's the Problem?

Two endpoints in finbot/apps/ctf/routes/challenges.py contain an authentication
guard that can never fire:

session_context: SessionContext = Depends(get_session_context)   # ← wrong dependency
...
if not session_context:                  # ← DEAD CODE — always False
    raise HTTPException(status_code=401, detail="Authentication required")

The guard reads if not session_context, but session_context is injected via
get_session_context which is defined as:

# finbot/core/auth/middleware.py  L156–161
async def get_session_context(request: Request) -> SessionContext:
    return request.state.session_context   # always a valid object, never None

The SessionMiddleware sets request.state.session_context for every request :
including anonymous visitors with no email bound (temporary sessions). So the value
is always a truthy SessionContext object. The if not session_context condition
evaluates to False 100% of the time, and the HTTPException(401) is never raised.

Result: Temporary/unauthenticated sessions bypass the guard on both:

1. POST /api/v1/challenges/{challenge_id}/check - attempt counters get inflated by throwaway sessions
2. POST /api/v1/challenges/{challenge_id}/hint - full hint text is leaked to anyone with a browser cookie, no registration required

This is the same class of bug as #511 (sidecar endpoint), but with a different root
cause: a dead if not X check instead of using the wrong dependency directly.

---

What This PR Does

1. Code Fix : finbot/apps/ctf/routes/challenges.py

Swap the weak get_session_context dependency to get_authenticated_session_context
on both write endpoints, and remove the now-redundant dead code guards.

get_authenticated_session_context raises HTTP 401 Unauthorized automatically when
session_context.is_temporary is True - exactly what the dead guard was supposed
to do but never could.

check_challenge endpoint

-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")
     ...

use_hint endpoint

 @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")
     ...

Note: GET /api/v1/challenges and GET /api/v1/challenges/{id} keep
get_session_context intentionally - they are publicly browsable. Only the
write operations (check and hint) require a persistent session.

---

2. New Test File : tests/unit/apps/ctf/test_challenge_auth.py

A dedicated unit test file following the exact same pattern as
test_sidecar_auth.py (from PR #512).

Approach

• Pure unit tests tagged @pytest.mark.unit
• No database required — sessions mocked with a minimal _StubSession stub
• Signature introspection guards against future regression back to get_session_context
• Source-code inspection tests confirm the dead if not session_context lines are gone
• End-to-end route tests via fastapi.testclient.TestClient

---

Files Changed

File	Type	Lines Changed
finbot/apps/ctf/routes/challenges.py	Modified	−4 (remove dead guards), ~2 (import swap)
tests/unit/apps/ctf/test_challenge_auth.py	New	~200

Total: 2 files, ~206 lines net change.

---

Before vs After

Scenario	Before (Buggy)	After (Fixed)
Unauthenticated / temp session calls POST /api/v1/challenges/{id}/check	HTTP 200 ❌ (attempt recorded)	HTTP 401 ✅
Unauthenticated / temp session calls POST /api/v1/challenges/{id}/hint	HTTP 200 ❌ (hint text revealed)	HTTP 401 ✅
Authenticated session calls POST /api/v1/challenges/{id}/check	HTTP 200 ✅	HTTP 200 ✅ (no change)
Authenticated session calls POST /api/v1/challenges/{id}/hint	HTTP 200 ✅	HTTP 200 ✅ (no change)

---

Root Cause vs. Issue #511

	Issue #511 (sidecar)	Issue #514 (challenges)
Symptom	Endpoint accepted temp sessions	Endpoint accepted temp sessions
Root cause	Wrong dependency (get_session_context instead of get_authenticated_session_context)	Dead code guard (if not session_context which is always False)
Fix	Swap dependency	Swap dependency + remove dead guard
Risk	Same — zero impact on authenticated users

---

How to Test

# Run only the new unit tests
pytest tests/unit/apps/ctf/test_challenge_auth.py -v

# Run the full CTF unit test suite
pytest tests/unit/apps/ctf/ -v

# Manual smoke test — get a temporary session cookie (no email bound),
# then hit the endpoints and confirm HTTP 401 is returned

# check endpoint
curl -X POST https://owasp-finbot-ctf.org/api/v1/challenges/<any-id>/check \
  -H "Cookie: session=<temp_session_cookie>"
# Expected: 401 Unauthorized

# hint endpoint
curl -X POST https://owasp-finbot-ctf.org/api/v1/challenges/<any-id>/hint \
  -H "Cookie: session=<temp_session_cookie>"
# Expected: 401 Unauthorized

---

Checklist

• Import swapped: get_session_context → get_authenticated_session_context in challenges.py
• Dead guards removed: if not session_context: raise HTTPException(401) deleted from both functions
• New test file test_challenge_auth.py created with 9 tests
• All 9 new tests pass locally
• No existing tests broken
• GET /api/v1/challenges and GET /api/v1/challenges/{id} are unchanged (still publicly browsable)
• PR references Closes #514

[prince-shakyaa]

Hey @saikishu @e2hln ,
I found a dead auth guard in the check and hint challenge endpoints where if not session_context could never fire, letting unauthenticated users freely access both endpoints and leak hint text. Fixed it by swapping to get_authenticated_session_context (same pattern as every other protected route) and added 9 unit tests to prevent regression.
Happy to make any changes based on your review!
Thank You.

[joshlochner]

Sai, do we want unauthenticated users to be able to get hints?

[stealthwhizz]

Sai, do we want unauthenticated users to be able to get hints?

Hey @joshlochner , i think there should be a difference between auth and unauth users..
People wont signup if its same for both Right...
and at EOD this is meant for learning
so i think providing unauth users with less hints would be a better option.