Scope example item to the tenant to close cross-tenant IDOR - #4
Merged
Conversation
update() and destroy() authorise with a capability check alone, which every tenant's staff passes, so a route-model-bound id from another company resolved and was edited or deleted. Add App\Models\Concerns\TenantScoped to the model so a foreign id 404s at route binding before the controller runs. Every module is generated from this template, so the hole reproduced in each one. Also drop the stray empty ->with() left before the flash message.
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.
Problem
update()anddestroy()inExamplePackageItemControllerauthorise with a capability check alone (can('edit-example-package')), which every tenant's staff passes. With a route-model-boundExamplePackageItem $item, an id belonging to another company resolved fine and was then edited or deleted.index()scopes bycreated_by/creator_id, but the mutations do not.Because every module is generated from this template, the hole reproduces in each new module.
Fix
Put the boundary on the model, not each action: add
App\Models\Concerns\TenantScopedtoExamplePackageItem. The row owns itself viacreated_by(the tenant id), so a foreign id now resolves to null and route-model binding 404s before the controller runs. This is the same trait the other modules use to close the same class of IDOR.Also removes a stray empty
->with()call left before the flash message inupdate()/destroy().Verification
Scaffold a module from the template and hit
update/destroywith another tenant's id, expect a 404 rather than an edit/delete.