Skip to content

fix(sec): remediate broken access control (BOLA/IDOR) across API + parent model (#59) - #66

Merged
franciszver merged 14 commits into
mainfrom
fix/p-sec-broken-access-control
Jul 25, 2026
Merged

fix(sec): remediate broken access control (BOLA/IDOR) across API + parent model (#59)#66
franciszver merged 14 commits into
mainfrom
fix/p-sec-broken-access-control

Conversation

@franciszver

@franciszver franciszver commented Jul 24, 2026

Copy link
Copy Markdown
Owner

Summary

Closes the systemic broken-access-control (BOLA/IDOR) cluster from the security review (epic #59). API routes authenticated the caller — or skipped auth entirely — but never checked that the requested object belonged to that caller. This PR centralizes the ownership check and applies it everywhere.

Implements the full remediation plan:

Review gates run

  • /security-review (adversarial) — found and fixed a BOLA where create_override authorized student_id but not the target_id's owner.
  • /simplify — flattened the jobs WS auth flow; direct 404 on malformed practice id.
  • /code-review (deep) — found and fixed a BLOCKER (/practice/assign/async was missed) and hardened the helper to fail closed (403, not 500) on a None/malformed target.

Tests

615 passed, 1 skipped, 2 xfailed (was 549 on main). ~66 new authz tests, red-first: cross-user access → 403, no-auth on the previously-open routes → 401, malformed ids → 422, owner/assigned-tutor/linked-parent/admin → 2xx. Includes a WebSocket owner-accept test for /jobs/{id}/ws and parent linked/unlinked coverage.

Service-to-service auth — investigated, resolved as docs-only (#67)

The docs claimed X-API-Key service-to-service auth (Rails calling these routes). Research across docs, callers, and full git history found: no Rails code ever existed, X-API-Key was never implemented server-side, and there is no scheduler/cron callerrender.yaml runs only web services, there's no scheduler library, and nudges fire in-process during the frontend's authenticated GET /nudges/users/{id} call. Every real caller is the JWT-sending React frontend. So no live caller breaks. This PR updates the docs to state JWT-only and removes the dead ai_service_api_key/rails_app_url config.

Follow-ups — all resolved in this PR

Every new/re-opened route routes through require_role + assert_can_access_student, both deny-by-default; a fresh adversarial review of the parent re-opening confirmed no fail-open path or cross-parent leak.

Closes #59, #60, #61, #62, #63, #64, #65, #67, #68, #69.

🤖 Generated with Claude Code

franciszver and others added 8 commits July 24, 2026 12:29
…per (#60, #61)

Phase 0: _ALLOWED_REGISTER_ROLES narrowed to {"student"}; tutor/parent are
now admin-provisioned. Phase 1: add src/api/middleware/authz.py
assert_can_access_student() encoding the access model (student->own,
tutor->TutorStudentAssignment, parent->denied, admin->all) plus unit tests
and reusable tests/_authz_utils.py for later phases.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
)

These routes used get_current_user_optional and never read the result, so
they were fully unauthenticated. Switch to mandatory get_current_user and
gate each on assert_can_access_student against the resolved student. The
/jobs WebSocket now authenticates on connect (token query param) and closes
1008 before streaming. Adds uuid coercion needed for the ownership lookups.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
summaries, enhancements conversation-context, and messaging threads now
authorize the target id via assert_can_access_student instead of trusting a
path/query id. Parent dashboards and advanced-analytics engagement are
locked to admin (parent<->student linkage deferred, #60); analytics also
drops the parent role and adds the ownership check for tutors.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
…ck (#64)

create_override no longer trusts the body-supplied tutor_id: it authorizes
the caller via assert_can_access_student(student_id) and persists the
authenticated caller's id as override.tutor_id. GET /overrides/{student_id}
gains the same ownership check.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
Adversarial review found create_override authorized the caller against
request.student_id but never checked that target_id (a Summary/
PracticeAssignment) belonged to that student, so an assigned tutor could
edit another student's record via target_id. Cross-check the fetched
target's student_id against the authorized student (normalized), coerce
the target lookups to UUID, and add a regression test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
)

/simplify cleanups on the remediation diff: flatten the jobs WebSocket
auth block to a single close-1008 exit and hoist SessionLocal to a
module import; make complete_practice raise 404 directly on a malformed
item_id instead of round-tripping a None filter. Behavior-preserving;
full suite green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
…, #61)

Deep-review caught two gaps in the remediation:
- POST /practice/assign/async still used get_current_user_optional with no
  ownership check (missed alongside its /assign siblings) -> now requires
  auth + assert_can_access_student on the body student_id.
- assert_can_access_student raised an uncaught ValueError (500) on a None or
  malformed target id -> now fails closed with 403 for every role. This also
  turns malformed ?student_id on assign/summary into 403 instead of 500.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
franciszver and others added 4 commits July 24, 2026 15:13
…t-path test (#63, #62)

get_conversation_history let any tutor read any student's Q&A history while
its sibling get_conversation_context (fixed earlier) restricts tutors to
assigned students - a cross-tutor IDOR and an inconsistency. Route it through
assert_can_access_student too (tutors now limited to their assigned students;
admins unchanged). Also add the previously-missing WebSocket owner-accept
test for /jobs/{id}/ws (monkeypatched session, no production change).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
…config (#67)

Research confirmed the X-API-Key service-to-service auth and Rails caller were
aspirational scaffolding never implemented in code; the only real caller is
the JWT-sending React frontend. Update API_CONTRACTS.md and siblings to state
JWT-only and mark the Rails/X-API-Key/webhook sections NOT IMPLEMENTED, and
remove the unused ai_service_api_key/rails_app_url settings.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
Retype student_id/item_id/target_id params on practice and overrides from
str to UUID so FastAPI returns 422 on malformed ids (was 403/500 via manual
coercion), and drop the now-redundant uuid.UUID() wrapping. Tests updated to
expect 422 on malformed ids.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
…to linked parents (#68)

Add ParentStudentAssignment (model + migration 004), extend
assert_can_access_student with a parent branch (parent may access only linked
students; fail-closed otherwise), and re-open the parent dashboards + engagement
routes from admin-only to [parent, admin] with the relationship enforced.
/parent/students now returns only linked students for a parent (all for admin).
Links are admin/seed-provisioned; no self-service linking endpoint (avoids a
new IDOR). Registration stays student-only.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
@franciszver franciszver changed the title fix(sec): remediate broken access control (BOLA/IDOR) across API (#59) fix(sec): remediate broken access control (BOLA/IDOR) across API + parent model (#59) Jul 24, 2026
franciszver and others added 2 commits July 24, 2026 17:42
A CI run stalled 83min on the test step (passes in ~50s locally), with no
signal of which test hung. Add pytest-timeout with a 120s per-test cap
(thread method: dumps stacks and aborts) so any hang becomes a fast, named
failure instead of an indefinite stall.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
#62)

The WebSocket auth block only caught InvalidTokenError/HTTPException, so a DB
error during the job lookup escaped and left the socket hanging (CI stalled
120s: the attacker test hit the empty CI Postgres via the handler's direct
SessionLocal, raising UndefinedTable). Catch any exception and close 1008 -
an internal error must never hang the handshake. Also monkeypatch
SessionLocal in the attacker WS test so it exercises the real deny path
against the in-memory engine instead of the configured DB.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgJgXbhqnBB89y4yL6ZJVk
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:high High priority security Security/authz

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[EPIC] Security: broken access control (BOLA/IDOR) remediation

1 participant