Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's security posture by addressing Insecure Direct Object Reference (IDOR) vulnerabilities. It introduces a new plug that centralizes resource access validation, ensuring that all operations on specific entities (like assessments, questions, submissions, and teams) are strictly confined to the user's current course context. This change moves resource validation from individual controller actions into a reusable and robust plug, making the application more secure and maintainable by preventing unauthorized access to data across different courses. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new EnsureResourceScope Plug to centralize resource validation and fetching within the correct course context, enhancing security and reducing boilerplate in controllers. This involved adding new get_..._in_course functions in various modules and updating numerous controllers to utilize this plug. Corresponding test cases were also updated to reflect the new error handling. The review suggests an improvement in the get_answer_in_course function to combine two database queries into a single, more efficient one, avoiding an extra database roundtrip.
| def get_answer_in_course(answer_id, course_id) | ||
| when is_ecto_id(answer_id) and is_ecto_id(course_id) do | ||
| answer_query = | ||
| Answer | ||
| |> where(id: ^answer_id) | ||
| |> join(:inner, [a], q in assoc(a, :question)) | ||
| |> join(:inner, [_, q], ast in assoc(q, :assessment)) | ||
| |> where([_, _, ast], ast.course_id == ^course_id) | ||
|
|
||
| case Repo.exists?(answer_query) do | ||
| true -> get_answer(answer_id) | ||
| false -> {:error, {:forbidden, "Forbidden"}} | ||
| end | ||
| end |
There was a problem hiding this comment.
This function currently performs two separate database queries: one with Repo.exists? to check for the answer's existence within the course, and then another one inside get_answer to fetch the answer. This is inefficient.
You can combine these into a single, more efficient query that both verifies the scope and fetches the answer. This avoids the extra database roundtrip.
Assuming get_answer preloads common associations, the combined query should also include them. I've included some common preloads based on other functions in this file, but you should verify if get_answer preloaded others.
def get_answer_in_course(answer_id, course_id)
when is_ecto_id(answer_id) and is_ecto_id(course_id) do
answer_query =
Answer
|> where(id: ^answer_id)
|> join(:inner, [a], q in assoc(a, :question))
|> join(:inner, [_, q], ast in assoc(q, :assessment))
|> where([_, _, ast], ast.course_id == ^course_id)
|> preload([:question, :submission, :history, :grading, :comments])
case Repo.one(answer_query) do
nil -> {:error, {:forbidden, "Forbidden"}}
answer -> {:ok, answer}
end
end
There was a problem hiding this comment.
Pull request overview
This PR addresses an IDOR class of vulnerabilities by enforcing that resource IDs in requests (assessment/question/submission/answer/team/course_reg) belong to the current course scope, and updates controllers/tests to reflect the new authorization outcomes.
Changes:
- Introduces
CadetWeb.Plug.EnsureResourceScopeto validate scoped access for common resource IDs. - Adds course-scoped getters in
Cadet.Assessments,Cadet.Accounts.Teams, andCadet.Accounts.CourseRegistrations. - Updates multiple controllers to apply scoping plugs and adjusts tests/error expectations (notably shifting some cases to 403/400).
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/cadet_web/controllers/teams_controller_test.exs | Adds a cross-course assessment access test expecting 403. |
| test/cadet_web/controllers/assessments_controller_test.exs | Adjusts expected status codes and adds scoped-access test for assessment. |
| test/cadet_web/controllers/answer_controller_test.exs | Updates setup to reuse course from conn scope and adjusts expected error statuses. |
| test/cadet_web/controllers/ai_code_analysis_controller_test.exs | Updates invalid-answer-id behavior expectation (but currently contains a failing assertion). |
| test/cadet_web/admin_controllers/admin_user_controller_test.exs | Updates expected responses to 403 for cross-course / missing scoped course_reg cases. |
| test/cadet_web/admin_controllers/admin_teams_controller_test.exs | Updates fixtures to ensure team belongs to course; adjusts invalid-id expectations. |
| test/cadet_web/admin_controllers/admin_grading_controller_test.exs | Updates expected status codes for scoped access behavior. |
| lib/cadet_web/plug/ensure_resource_scope.ex | New plug to enforce course scoping for resource IDs. |
| lib/cadet_web/controllers/team_controller.ex | Applies scoping to assessment ID for team index. |
| lib/cadet_web/controllers/generate_ai_comments.ex | Applies scoping to answer access for AI comment generation/finalization. |
| lib/cadet_web/controllers/assessments_controller.ex | Applies scoping to assessment access for several actions and uses scoped ID for contest endpoints. |
| lib/cadet_web/controllers/answer_controller.ex | Applies scoping to question access for submit/last-modified endpoints. |
| lib/cadet_web/admin_controllers/admin_user_controller.ex | Applies course_reg scoping for admin actions on course registrations. |
| lib/cadet_web/admin_controllers/admin_teams_controller.ex | Applies team scoping for update/delete and refactors usage to rely on scoped team. |
| lib/cadet_web/admin_controllers/admin_grading_controller.ex | Applies scoping plugs for submission/question/assessment IDs for grading actions. |
| lib/cadet_web/admin_controllers/admin_goals_controller.ex | Applies course_reg scoping for goals-with-progress/progress updates. |
| lib/cadet_web/admin_controllers/admin_assessments_controller.ex | Applies scoping for course_reg and assessment IDs across admin assessment endpoints. |
| lib/cadet/assessments/assessments.ex | Adds course-scoped retrieval helpers and scopes certain grading/regrade operations by course. |
| lib/cadet/accounts/teams.ex | Adds get_team_in_course/2 for scoped team retrieval. |
| lib/cadet/accounts/course_registrations.ex | Adds get_course_reg_in_course/2 for scoped course registration retrieval. |
Comments suppressed due to low confidence (1)
lib/cadet_web/admin_controllers/admin_teams_controller.ex:62
EnsureResourceScopehere only scopes theteamid, butassessmentIdandstudent_idsin the update payload are not validated to belong to the current course. As written, a staff user in course A could update a team in course A to reference an assessment from course B and/or attach course_reg IDs from other courses. Add scoping/validation for the target assessment and for each student/course_reg ID (either via additional EnsureResourceScope plugs or checks insideTeams.update_team/3) and reject cross-course IDs with 403.
def update(conn, %{"assessmentId" => assessmentId, "student_ids" => student_ids}) do
team =
conn.assigns.scoped_team
|> Repo.preload(assessment: [:config], team_members: [student: [:user]])
case Teams.update_team(team, assessmentId, student_ids) do
{:ok, _updated_team} ->
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| response = | ||
| conn | ||
| |> sign_in(admin_user.user) | ||
| |> post(build_url_generate_ai_comments(course_with_llm.id, random_answer_id)) | ||
| |> text_response(400) | ||
|
|
||
| assert response(response, 403) == "Forbidden" | ||
| end |
There was a problem hiding this comment.
In this test, response is a %Plug.Conn{} variable, but the assertion calls response(response, 403), which will fail (and also shadows the function name). Use the conn returned by post/3 and assert with response(conn, 403) (or text_response/2) instead.
| plug( | ||
| CadetWeb.Plug.EnsureResourceScope, | ||
| [resource: :question, param: "questionid", assign: :scoped_question] | ||
| when action in [:submit, :check_last_modified] | ||
| ) |
There was a problem hiding this comment.
With EnsureResourceScope now handling missing/out-of-scope questions before the action runs, this controller no longer returns the documented 404 "Question not found" response from submit/check_last_modified. The Swagger spec below still advertises a 404 for this endpoint; update the Swagger responses to reflect the new 400/403 behavior (or reintroduce a 404 if that's still required).
| plug( | ||
| CadetWeb.Plug.EnsureResourceScope, | ||
| [resource: :submission, param: "submissionid", assign: :scoped_submission] | ||
| when action in [ | ||
| :show, | ||
| :update, | ||
| :unsubmit, | ||
| :unpublish_grades, | ||
| :publish_grades, | ||
| :autograde_submission, | ||
| :autograde_answer | ||
| ] | ||
| ) | ||
|
|
||
| plug( | ||
| CadetWeb.Plug.EnsureResourceScope, | ||
| [resource: :question, param: "questionid", assign: :scoped_question] | ||
| when action in [:update, :autograde_answer] | ||
| ) |
There was a problem hiding this comment.
The new scope plugs (and function heads) expect path params named submissionid/questionid/assessmentid, matching the router (/grading/:submissionid/...). However, the Swagger paths/params in this controller still use {submissionId} / {questionId} etc. This will produce confusing API docs; align the Swagger path placeholders and parameter names with the actual route param keys.
|
|
||
| case Repo.exists?(answer_query) do | ||
| true -> get_answer(answer_id) | ||
| false -> {:error, {:forbidden, "Forbidden"}} |
There was a problem hiding this comment.
get_answer_in_course/2 currently issues two DB queries for the success case (Repo.exists? and then get_answer/1). Since this is used in a request plug, it adds avoidable overhead on every call. Consider fetching the scoped answer directly in a single query (e.g., join + preload and Repo.one) and returning nil/{:error, :forbidden} when not found, instead of exists? + re-fetch.
| case Repo.exists?(answer_query) do | |
| true -> get_answer(answer_id) | |
| false -> {:error, {:forbidden, "Forbidden"}} | |
| |> preload([a, q, _ast], question: q) | |
| case Repo.one(answer_query) do | |
| nil -> | |
| {:error, {:forbidden, "Forbidden"}} | |
| answer -> | |
| # Mirror the voting-question sanitization in get_answer/1 | |
| answer = | |
| if answer.question.type == :voting do | |
| empty_contest_entries = | |
| Map.put(answer.question.question, :contest_entries, []) | |
| empty_popular_leaderboard = | |
| Map.put(empty_contest_entries, :popular_leaderboard, []) | |
| empty_contest_leaderboard = | |
| Map.put(empty_popular_leaderboard, :contest_leaderboard, []) | |
| question = Map.put(answer.question, :question, empty_contest_leaderboard) | |
| Map.put(answer, :question, question) | |
| else | |
| answer | |
| end | |
| {:ok, answer} |
| plug( | ||
| CadetWeb.Plug.EnsureResourceScope, | ||
| [resource: :submission, param: "submissionid", assign: :scoped_submission] | ||
| when action in [ | ||
| :show, | ||
| :update, | ||
| :unsubmit, | ||
| :unpublish_grades, | ||
| :publish_grades, | ||
| :autograde_submission, | ||
| :autograde_answer | ||
| ] | ||
| ) | ||
|
|
||
| plug( | ||
| CadetWeb.Plug.EnsureResourceScope, | ||
| [resource: :question, param: "questionid", assign: :scoped_question] | ||
| when action in [:update, :autograde_answer] | ||
| ) | ||
|
|
||
| plug( | ||
| CadetWeb.Plug.EnsureResourceScope, | ||
| [resource: :assessment, param: "assessmentid", assign: :scoped_assessment] | ||
| when action in [:publish_all_grades, :unpublish_all_grades] | ||
| ) |
There was a problem hiding this comment.
These EnsureResourceScope plugs load and assign %Submission{} / %Question{} / %Assessment{} records, but the actions in this controller still use the raw IDs and re-query (e.g., get_answers_in_submission/1, update_grading_info/3, etc.). This makes each request do extra DB work just for scoping. Either use the assigned scoped records in the actions (where possible) or add an option to the plug to validate scope without fetching the full record.
| @tag authenticate: :staff | ||
| test "missing parameter", %{conn: conn} do | ||
| course_id = conn.assigns.course_id | ||
| conn = post(conn, build_url(course_id, 1, 3), %{}) | ||
| assert response(conn, 400) =~ "Missing parameter" | ||
| assert response(conn, 403) == "Forbidden" | ||
| end |
There was a problem hiding this comment.
This test no longer exercises the controller's "missing parameter" branch. With the new EnsureResourceScope plugs, hardcoded IDs (1, 3) will be rejected as out-of-scope/not found before the action runs, so the 403 is coming from the plug rather than from the missing-parameter validation. Seed a valid submission/question in the current course and omit the required body field so the action returns the intended 400 "Missing parameter".
| plug( | ||
| CadetWeb.Plug.EnsureResourceScope, | ||
| [resource: :team, param: "teamid", assign: :scoped_team] | ||
| when action in [:update, :delete] | ||
| ) |
There was a problem hiding this comment.
The controller now relies on the path param key teamid (router uses /teams/:teamid and this plug reads "teamid"), but the Swagger paths/parameters in this file still use {teamId} / teamId. This mismatch will generate misleading API docs; update the swagger path placeholders/parameter names to match the actual route param key.
| def delete(conn, %{"teamid" => _team_id}) do | ||
| team = conn.assigns.scoped_team | ||
|
|
||
| if team do | ||
| case Teams.delete_team(team) do |
There was a problem hiding this comment.
Because EnsureResourceScope runs for :delete and returns 403 when the team is not found/in another course, conn.assigns.scoped_team will never be nil here. The if team do ... else 404 "Team not found!" branch is now effectively dead code and the documented/expected behavior is 403 instead of 404; consider removing the nil-check branch (or adjusting the plug behavior if 404 is still desired).
Adds checks to ensure enforce valid associations among accessed route parameter tuples.
Commits with
aiscope are generated by AI; I will manually vet everything later before merging.