⚔️ Vanguard: [Advanced Security Enhancement/Fix] Prevent privilege escalation in updateRole#253
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughA security vulnerability fix for the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/data-ops/src/queries/school-admin/roles.ts`:
- Around line 129-132: The current check uses role.isSystemRole before
performing the UPDATE, which creates a TOCTOU window; instead make the
system-role protection atomic by adding a condition to the UPDATE itself (e.g.,
include "WHERE id = ... AND isSystemRole = false" or equivalent in the mutation
used by the updateRole function), and after executing the UPDATE check the
affected-rows count and throw DatabaseError('VALIDATION_ERROR', 'Cannot update
system roles') if no rows were updated; update the code paths that reference
role.isSystemRole to rely on the atomic WHERE and the affected-rows check to
enforce the invariant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7323053b-eaa1-4313-8a94-c65c92c4148a
📒 Files selected for processing (2)
.jules/vanguard.mdpackages/data-ops/src/queries/school-admin/roles.ts
| // Phase 11: Prevent updating system roles to avoid privilege escalation | ||
| if (role.isSystemRole) { | ||
| throw new DatabaseError('VALIDATION_ERROR', 'Cannot update system roles') | ||
| } |
There was a problem hiding this comment.
Make system-role protection atomic in the UPDATE itself.
Line 130 checks isSystemRole before the write, but the actual mutation at Line 140 only filters by id. This leaves a TOCTOU window where a system role could still be updated if state changes between read and write.
🔧 Proposed fix
- const [updated] = await db
+ const [updated] = await db
.update(roles)
.set({
...data,
updatedAt: new Date(),
})
- .where(eq(roles.id, roleId))
+ .where(and(eq(roles.id, roleId), eq(roles.isSystemRole, false)))
.returning()
+
+ if (!updated) {
+ throw new DatabaseError('VALIDATION_ERROR', 'Cannot update system roles')
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/data-ops/src/queries/school-admin/roles.ts` around lines 129 - 132,
The current check uses role.isSystemRole before performing the UPDATE, which
creates a TOCTOU window; instead make the system-role protection atomic by
adding a condition to the UPDATE itself (e.g., include "WHERE id = ... AND
isSystemRole = false" or equivalent in the mutation used by the updateRole
function), and after executing the UPDATE check the affected-rows count and
throw DatabaseError('VALIDATION_ERROR', 'Cannot update system roles') if no rows
were updated; update the code paths that reference role.isSystemRole to rely on
the atomic WHERE and the affected-rows check to enforce the invariant.
1 participant
Vulnerability Path
The
updateRoledatabase mutation inpackages/data-ops/src/queries/school-admin/roles.tslacked a validation check forisSystemRole. WhiledeleteRolewas correctly protected, this missing check allowed any authenticated user with sufficient standard role-management permissions within their tenant to potentially modify the permissions of global system-level roles (e.g. elevating a standard teacher role to have global admin permissions), bypassing tenant isolation.Action Taken
Added a
role.isSystemRolecheck toupdateRolethat immediately throws aDatabaseError('VALIDATION_ERROR', 'Cannot update system roles'), preventing modifications to critical system-level roles by tenant admins.PR created automatically by Jules for task 15928544180577891934 started by @ldsgroups225
Summary by CodeRabbit
Bug Fixes