fix: Prevent IDOR in Workspaces::Update (Closes #951)#977
fix: Prevent IDOR in Workspaces::Update (Closes #951)#977MuhammadIbtisam wants to merge 1 commit intoMultiwoven:mainfrom
Conversation
…thenticated user update any workspace.
📝 WalkthroughWalkthroughThis PR fixes an Insecure Direct Object Reference (IDOR) vulnerability in the workspace update interactor by scoping the workspace lookup to the current user's workspaces instead of globally, with early failure handling for missing workspaces. Test coverage is added to verify the authorization check works correctly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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 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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/spec/interactors/workspaces/update_spec.rb (1)
52-58: Optionally assert failure payload to lock branch behavior.You can make this test stricter by asserting the interactor returns no workspace in this unauthorized path (Line 56 onward), which better pins the
fail!(workspace: nil)branch.Optional test tightening
expect(result).to be_a_failure + expect(result.workspace).to be_nil expect(workspace.reload.name).to eq(original_name)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/spec/interactors/workspaces/update_spec.rb` around lines 52 - 58, Test currently verifies the interactor fails but doesn't assert the failure payload; update the spec after calling Workspaces::Update.call to also assert the interactor returned no workspace (i.e. the failure payload contains workspace: nil) to lock the fail!(workspace: nil) branch—for example add an expectation that the failure payload's workspace is nil (checking whatever shape your interactor exposes, e.g. result.failure[:workspace] or result.failure.workspace).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/spec/interactors/workspaces/update_spec.rb`:
- Around line 52-58: Test currently verifies the interactor fails but doesn't
assert the failure payload; update the spec after calling
Workspaces::Update.call to also assert the interactor returned no workspace
(i.e. the failure payload contains workspace: nil) to lock the fail!(workspace:
nil) branch—for example add an expectation that the failure payload's workspace
is nil (checking whatever shape your interactor exposes, e.g.
result.failure[:workspace] or result.failure.workspace).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b6c34f49-52ac-4a0e-b934-d6a4d3ba8539
📒 Files selected for processing (2)
server/app/interactors/workspaces/update.rbserver/spec/interactors/workspaces/update_spec.rb
1 participant
Description
Fixes an IDOR (Insecure Direct Object Reference) in Workspaces::Update. The interactor previously used Workspace.find_by(id:), so any authenticated user could update any workspace. It now uses context.user.workspaces.find_by(id:) so updates are limited to workspaces the user can access. A guard clause ensures the interactor fails when the workspace is not found or not owned.
Related Issue
Relates to issue #951 – Security: Cross-Workspace Update IDOR in Workspaces::Update Interactor.
Type of Change
How Has This Been Tested?
bundle exec rspec spec/interactors/workspaces/update_spec.rb
Existing specs were updated to associate the user with the workspace.
A new spec asserts that a user cannot update another user’s workspace (IDOR case).
Summary by CodeRabbit