-
Notifications
You must be signed in to change notification settings - Fork 145
IDOR Vulnerability documentation ready to commit #598
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
IDOR Vulnerability documentation ready to commit #598
Conversation
|
Hi @atharv02-git , Strengths of the report:
Possible improvement:
Overall, this is a well-thought-out report. |
theiris6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your RBAC implementation fixes the security vulnerability but breaks authentication, as shown by the user-icon component errors in your screenshot. The frontend clearly depends on certain staff data fields that are now being filtered out.
I recommend:
- Preserving essential fields needed for UI components while still removing truly sensitive data
- Using a more surgical approach that filters specific sensitive fields rather than entire objects
- Adding conditional logic to return different data structures based on authentication context vs general data access
ibi420
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following your provided documentation, I have confirmed the presence of an Insecure Direct Object Reference (IDOR) vulnerability. This weakness permits unauthorized access to sensitive staff data via the API due to inadequate security measures. Using Postman, I successfully replicated this vulnerability by authenticating a student user, obtaining an auth_token, and subsequently executing a GET request to the vulnerable endpoint "/api/units/:id". Although the implemented Role-Based Access Control (RBAC) system mitigated the issue, it inadvertently disrupted frontend login functionality. While RBAC offers utility, migrating to Attribute-Based Access Control (ABAC) is recommended. ABAC's ability to assign attributes such as "is_student" and "is_tutor" would provide more granular access control and long-term maintainability by directly linking access permissions to specific attributes. Your documentation effectively outlines the vulnerability and its exploitation as an authentication mechanism, highlighting a significant security risk. Thank you for the opportunity to review your work.
Hey @ibi420, This documentation that I outlined was a week before my actual implementation and it highlights my approach to what my initial plan was to fix the vulnerability, but later as I found out that the changes I made in multiple files (which I have highlighted in the document) to implement RBAC it disrupted the functionality of making changes at the backend broke the connnectivity with frontend and hence I had to come up with a new solution to fix the issue with as many minimal changes as I can, then I realised the data exposure occurred in the serializer layer, particularly in the Entities::UnitEntity class. |
Hey @atharv02-git, I completely see my error; following our conversation, I reviewed your fix. You have implemented an attribute "is_staff" that addresses this issue instead of the RBAC I had initially thought and followed. This is what I initially referred to when I was reviewing your documentation. |
|
@atharv02-git Please raise this changes in a PR against doubtfire-astro |

Description
Fixes # (issue)
Type of change
The implementation introduces Role-Based Access Control (RBAC) to ensure sensitive staff information is only exposed to authorized users (Admins and Staff).
The backend files have been updated to apply conditional serialization of sensitive data based on the user’s role.
Bug fix (breaking change which fixes an issue)
Documentation (update or new)
How Has This Been Tested?
/api/units/:idTesting Checklist: