Skip to content

fix(security): only remove a student from a run#333

Open
Isaries wants to merge 1 commit into
WISE-Community:developfrom
Isaries:fix/remove-student-run-role
Open

fix(security): only remove a student from a run#333
Isaries wants to merge 1 commit into
WISE-Community:developfrom
Isaries:fix/remove-student-run-role

Conversation

@Isaries

@Isaries Isaries commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

DELETE /api/teacher/run/{runId}/student/{studentId}/remove has the same shape as #332: it checks write permission on the run from the path, then loads the target user straight from studentId without checking the two are related. This one is much less serious, and I want to be precise about why rather than overstate it.

There is no cross-run problem here. Both steps of removeStudentFromRun are already scoped to the run:

  • removeStudentFromWorkgroupsInRun uses getWorkgroupListByRunAndUser(run, user), filtered by run.
  • removeStudentFromPeriodInRun uses run.getPeriodOfStudent(user), which returns null for a non-member and is guarded.

So passing the id of a student from another teacher's run does nothing at all.

The gap is the missing role constraint. getListByRunAndUser does not filter by role, and teachers do have a workgroup in a run. So a teacher who shares a run can pass a co-owner's user id to this endpoint and remove that co-owner from their own workgroup in the run. The target has to already be associated with the run, so this is a lateral action between people who share it rather than an escalation, but the endpoint is documented as removing a student and should hold to that.

Changes

  • Check the target is a student before removing them.
  • Add tests for this controller, which had none.

Why this differs from #332

ChangeStudentPasswordController needed a run membership check because updateUserPassword is not scoped to the run at all. Here the underlying operations already are, so a membership check would add nothing against the real gap while risking two behaviour changes I did not want to make blind:

  • A call for a student who is not in the run currently succeeds as a no-op. Turning that into a 403 could break a client that treats removal as idempotent.
  • A student who is in a workgroup but no longer in a period would become unremovable, so that inconsistent state could no longer be cleaned up through this endpoint.

Happy to switch to run.isStudentAssociatedToThisRun for symmetry with #332 if you would rather have the stricter check and are not worried about either of those.

Testing

  • mvnw test -Dtest=RemoveStudentRunControllerTest passes (3 tests).
  • Reverting only the controller change makes exactly the teacher-target test fail, and it fails by removeStudentFromRun being called on the teacher rather than the request being rejected.

The remove student endpoint took the target user straight from the student id
and never checked it was a student.

Both steps of removeStudentFromRun are already scoped to the run, so passing
the id of a user outside the run does nothing. A teacher is a different
matter: getListByRunAndUser does not filter by role and teachers do have a
workgroup in a run, so passing the id of a teacher who shares the run removed
that teacher from their own workgroup in it.

Check that the target is a student before removing them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant