Fix cross-store IDOR in store-panel customer product price/personalization#725
Merged
Merged
Conversation
…ation The store-panel CustomerController exposed four actions that mutated CustomerProductPrice / CustomerProduct entities by raw id without verifying the underlying customer belongs to the current store: UpdateProductPrice, DeleteProductPrice, UpdatePersonalizedProduct, DeletePersonalizedProduct. The AdminShared view-model service loads these entities by id and saves/deletes them with no store filter, so a store manager could update or delete another store customer's custom price or personalized products by supplying a foreign id. Resolve the entity, take its CustomerId and run it through the existing GetStoreCustomer() gate (store + registered check) before mutating - matching the pattern already used by UpdateCart/DeleteCart. No domain model or shared service changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a cross-store IDOR in Grand.Web.Store’s store-panel CustomerController by adding a store-ownership gate before mutating customer product price and personalization records, aligning these actions with the controller’s existing GetStoreCustomer() enforcement pattern.
Changes:
- Injects
ICustomerProductServiceto resolveCustomerProductPrice/CustomerProductby raw id. - Adds store-ownership validation (
GetStoreCustomer(entity.CustomerId)) before update/delete operations for product price and personalized products. - Ensures missing entities or cross-store access attempts no-op rather than mutating data.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add [HttpPost] to UpdateProductPrice/DeleteProductPrice/ UpdatePersonalizedProduct/DeletePersonalizedProduct so the [AutoValidateAntiforgeryToken] on BaseStoreController is enforced (the Kendo grids already call them via POST) - closes the CSRF gap Copilot flagged. - Delete the already-resolved entity via ICustomerProductService instead of re-loading by id in the view-model service, removing the redundant query and the TOCTOU exception on a concurrent delete. - Update CustomerControllerTests to supply the new ICustomerProductService constructor dependency (fixes the CI build break). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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.
Summary
Security fix found during a review of
Grand.Web.Store. The store-panelCustomerControllerhad four actions that mutated customer product data by raw id, with no store-ownership check:UpdateProductPrice(model)DeleteProductPrice(id)UpdatePersonalizedProduct(model)DeletePersonalizedProduct(id)Every other action in this controller funnels through
GetStoreCustomer()(which enforcescustomer.StoreId == CurrentStoreIdand registered-only), but these four bypassed it. The underlyingCustomerViewModelServiceloads theCustomerProductPrice/CustomerProductby id and saves/deletes it without any store filter, so a store manager could pass an id belonging to another store''s customer and update their custom price or delete their personalized products (cross-store write/delete IDOR).Fix
Resolve the entity first, take its
CustomerId, and run it through the existingGetStoreCustomer()gate before delegating to the service — the same pattern already used byUpdateCart/DeleteCartin this controller. If the entity is missing or its customer is not a registered customer of the current store, the action no-ops.AdminShared) changes.Grand.Web.Store/Controllers/CustomerController.cs(+20 lines).Note: this controller is gated behind
Customer:RegisterCustomersPerStore, so the issue only applies to per-store customer installations, but the missing authorization check is fixed regardless.Test plan
Grand.Web.Storebuilds clean🤖 Generated with Claude Code