Complete security audit of Flextensioons, identifying key vulnerabilities and areas for improvement.#413
Open
cycomachead wants to merge 4 commits into
Open
Complete security audit of Flextensioons, identifying key vulnerabilities and areas for improvement.#413cycomachead wants to merge 4 commits into
cycomachead wants to merge 4 commits into
Conversation
…e_enabled The role check trusted a client-supplied params[:role], which the Stimulus controller passed through from a data-* attribute. Any authenticated user could PATCH /assignments/:id/toggle_enabled with role=instructor and toggle assignments in courses where they were a student or had no enrollment at all. Now the role is looked up exclusively via course.user_role(current_user), the JS no longer sends role/user_id, and the view drops the data-role / data-user-id attributes that fed the bypass. Adds spec coverage for students, foreign-course instructors, and the no-enrollment case all attempting role=instructor. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ders The previous .env.example shipped a plausible-looking Gradescope password (KoH-z92-oVJ-yqr) and a real-looking service-account email. Even though those values were not the live credential, anything that looks like a secret in a sample file invites confusion (and gets indexed by secret scanners as a leak). Replace both with obvious placeholders and add a warning not to commit live values. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…per login The find_or_create_user logic always matched first by email and silently re-keyed the existing user's canvas_uid. That is intentional for the developer omniauth strategy (whose whole point is "log in as an existing user without Canvas"), but for the real Canvas provider it would let any auth response carrying a victim's email rewrite that user's identity. Split the two paths: - Canvas (and any non-developer provider) now matches strictly by canvas_uid, updates email/name on that user, and refuses to merge two identities when the email is already taken by a different account. - Developer keeps the email-first masquerade behavior, but only runs when Rails.env is development or test. The omniauth_callback action now hard-fails developer-provider responses in any other environment, backing up the existing initializer-level guard. Adds spec coverage for: canvas-provider email collision (refused), canvas-provider canvas_uid match (still updates email), developer masquerade (still works), and developer-provider rejection in production. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
request_params permitted :user_id, which on PATCH #update would let a student reassign their request to another user (the create flow merged user: @user so it was incidentally safe; update did not). The assignment_id param was also never checked against @Course, so a request could be filed against an assignment from a course the submitter doesn't belong to. - Drop :user_id from request_params. Both create paths already set the owner explicitly (`user: @user` for self-service, `user: student` in the instructor `create_for_student` flow), so this only removes the mass-assignment escape hatch. - Add assignment_in_course? and reject create/update/create_for_student when the assignment_id resolves to a different course's CourseToLms. - Add student_enrolled_in_course for create_for_student so an instructor cannot file requests on behalf of someone outside their course roster. Adds spec coverage for the user_id mass-assignment attempt on both create and update, and for the cross-course assignment_id attempt on both. Also bumps a few hardcoded canvas_uids in pre-existing tests to string-prefixed values so they do not collide with the user factory's numeric sequence as more tests are added. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security Audit Remediation: Critical & High Vulnerability Fixes
Resolves four security vulnerabilities identified in the Flextensions security audit, covering privilege escalation, credential exposure, account takeover, and mass-assignment/IDOR issues.
Changes
C1 — Authorization bypass in
AssignmentsController(privilege escalation + IDOR)The
toggle_enabledaction previously acceptedparams[:role]from the client, allowing any authenticated user to passrole=instructorand toggle assignments in courses they don't belong to. The role is now derived exclusively server-side viacourse.user_role(@user). The Stimulus controller no longer sendsroleoruser_idin the request body, and the view no longer embeds those values indata-*attributes.C3 — Hard-coded Gradescope credentials in
.env.exampleReplaced a real-looking service account email and password with obvious placeholder values and added a comment explicitly warning against committing live credentials.
C4 — Account-linking logic enabling unauthorized account takeover
Split
find_or_create_userinto two distinct paths:canvas_lookup_or_create: canonical production path — matches bycanvas_uid, updates email on match, and refuses to silently re-key an existing account when a differentcanvas_uidpresents a matching email (returnsnil, surfacing an error to the user).developer_lookup_or_create: intentional dev/test masquerade path — preserves the legacy email-match behavior so developers can log in as existing users without Canvas. Gated bydeveloper_login_allowed?in the callback and the existing env check inomniauth.rb, with comments explaining the contract.H2 — Mass-assignment and IDOR in
RequestsController:user_idfromrequest_paramspermit list; ownership is now set explicitly by the controller (user: @userfor students,user: studentfor the instructorcreate_for_studentflow).assignment_in_course?helper that scopes assignment lookups to@course'sCourseToLmsrecords, preventing cross-course assignment references increate,update, andcreate_for_student.student_enrolled_in_coursehelper to verify the target student is actually enrolled in@coursebefore an instructor can file on their behalf.Testing
Added regression specs for each fix:
assignments_controller_spec.rb: Covers student-in-course rejection, instructor-in-different-course rejection, and unenrolled user rejection — all with and without a spoofedrole=instructorparam.requests_controller_spec.rb: Covers cross-course assignment rejection oncreateandupdate, client-supplieduser_idbeing ignored oncreateandupdate, and cross-course assignment rejection increate_for_student.session_controller_spec.rb: Covers canvas-provider email-collision refusal, canvas-providercanvas_uidmatch, developer masquerade (intentional), and developer-provider rejection whenRails.envis production.All 163 existing controller specs continue to pass.
Documentation
No external documentation required. Code comments in
session_controller.rb,requests_controller.rb, andconfig/initializers/omniauth.rbexplain the security contracts inline for future maintainers.Checklist
Superconductor Ticket Implementation | App Preview | Guided Review