fix: scope ContactTypeGroup lookup to current organization to prevent cross-org access#6888
Open
PhutiCee wants to merge 4 commits intorubyforgood:mainfrom
Open
fix: scope ContactTypeGroup lookup to current organization to prevent cross-org access#6888PhutiCee wants to merge 4 commits intorubyforgood:mainfrom
PhutiCee wants to merge 4 commits intorubyforgood:mainfrom
Conversation
… cross-org access
… cross-org access Admins could access and modify ContactTypeGroups belonging to other organizations by directly visiting the edit URL with a foreign record ID. Fix scopes the find query to current_organization so cross-org access raises ActiveRecord::RecordNotFound (404) instead of returning the record. Also adds a rescue_from ActiveRecord::RecordNotFound handler in ApplicationController to return a proper 404 response. Adds request specs covering the cross-organization access scenario for both GET edit and PUT update endpoints.
PhutiCee
requested review from
FireLemons,
compwron and
elasticspoon
as code owners
Moving rescue_from ActiveRecord::RecordNotFound from ApplicationController to ContactTypeGroupsController to avoid interfering with other controllers that expect the exception to bubble up. Also fixes Rails/FilePath linting issue and trailing newline in spec file.
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.
Summary
Fixes #6351
Problem
An admin from Organization A could visit a direct URL containing the ID
of a ContactTypeGroup belonging to Organization B and successfully edit
or update it. This is a multi-tenancy data isolation bug.
Root Cause
set_contact_type_groupusedContactTypeGroup.find(params[:id])which searches all records globally, ignoring the current user's org.
Fix
Scoped the lookup to the current organization:
Also added
rescue_from ActiveRecord::RecordNotFoundinApplicationController to return a proper 404 response instead of
raising an unhandled exception.
Testing
Notes
As @FireLemons noted, similar patterns may exist in HearingTypes and
Judge controllers and would be worth a follow-up fix.