fix: enforce ownership check on /accounts/:id to close IDOR#103
Open
hacktron-app-stg[bot] wants to merge 1 commit into
Open
fix: enforce ownership check on /accounts/:id to close IDOR#103hacktron-app-stg[bot] wants to merge 1 commit into
hacktron-app-stg[bot] wants to merge 1 commit into
Conversation
The /accounts/:id endpoint returned any account by id without verifying ownership, allowing id enumeration to leak other users' account data. Reject requests with 401 when unauthenticated and 403 unless the requested id matches session[:user] before returning the account.
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
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.
Vulnerability
GET /accounts/:id(payments.rb:24-27) returned the JSON representation of any account addressed byid, with no check that the account belongs to the authenticated user. Since account ids are enumerable, an attacker could request/accounts/1,/accounts/2, … and read other users' balances and private details — a classic Insecure Direct Object Reference / broken access control (high severity).Fix
Add an object-level authorization check to the handler:
halt 401when there is no authenticatedsession[:user].halt 403unless the requestedparams[:id]matches the authenticated user.Only then is the account returned. This matches how the existing
/transferhandler already treatssession[:user]as the account key inACCOUNTS, so the ownership comparison is correct for this data model.Verification
Reviewed the surrounding code to confirm
ACCOUNTSis keyed by user id (see/transfer, which usesACCOUNTS[session[:user]]). Ruby is not installed in the sandbox, soruby -ccould not be run; the change uses only standard Sinatrahalt/sessionidioms already present in the file. No test infrastructure exists in the repo, so no regression test was added.Automated fix by Hacktron for finding: https://staging.hacktron.ai/testestesttest/findings/299ce0ed-6707-4305-9285-7963cd0ccf2d