Uh oh!
There was an error while loading. Please reload this page.
Fix cross-store IDOR gaps in Grand.Web.Store - #783
Merged
Conversation
Audit of the store-owner/store-manager panel (scoped by WorkContext.CurrentCustomer.StaffStoreId) found several places where a loaded entity, or an entity referenced by id in a request, was mutated without verifying it belonged to the current store manager's store. - ProductController: ~20 sub-resource mutation actions (category/ collection/related/similar/bundle/cross-sell/recommended/associated product mappings, pictures, spec attributes, prices, tier prices, attribute mappings/values) had no ownership check at all, even though the paired GET/list action next to each of them did. Added AccessToEntityByStore checks before every mutation, and filter SelectedProductIds down to store-owned products where the shared ProductViewModelService mutates each selected entity by id. - CategoryController/CollectionController: ProductUpdate/ProductDelete/ ProductAddPopup mutate the product's category/collection list, not the category/collection entity - the missing check was on the product. - MerchandiseReturnController.MerchandiseReturnNoteAdd: an unchecked orderId parameter was used to build a customer-facing notification email, letting a manager forge/misdirect it with another store's order data. Now validated against the merchandise return's own OrderId. - NewsController.List: scoped by request host (StoreContext.CurrentStore) instead of StaffStoreId, letting the result set diverge from the manager's actual store depending on which host the panel is reached through. - ShippingController.RestrictionSave: mutated restricted-countries/ restricted-groups on global (store-independent) shipping methods returned alongside store-owned ones, not just store-owned methods. - MessageTemplateController.Edit/Delete and SettingController's IsStoreOwnerAccessAllowed (MerchandiseReturnReason/Action) used a non-exclusive "is my store one of the assigned stores" check instead of requiring exclusive ownership, letting one store's manager edit-and- reassign-away or delete an entity another store still depends on. Updated ShippingControllerTests fixtures that encoded the old (vulnerable) behavior and added a regression test for the fix. Verified with dotnet build (Store, Admin, Vendor - all reference the touched shared AdminShared extension) and dotnet test on Grand.Web.Store.Tests.
…b.Store Of 183 ViewBag occurrences in Grand.Web.Store, 152 are ViewBag.Title, set only in views (the standard Razor <title> idiom) - left untouched. Of the rest, 17 were dead: ViewBag.AllLanguages (Blog/Page/News controllers) and ViewBag.productIdsInput (ProductController) are set but never read anywhere in the solution. Removed them (two of the AllLanguages assignments in NewsController were also missing `await`, an unobserved-task bug hidden inside otherwise-dead code). The remaining 4 keys actually cross the controller-to-view boundary and were promoted to typed model properties, following the two patterns already used in this area for extending a shared Grand.Web.AdminShared model from the store panel: - MessageTemplateController.IsReadOnly: new MessageTemplateStoreModel : MessageTemplateModel + MessageTemplateStoreProfile AutoMapper profile, mirroring the existing ContactAttributeStoreModel/ CustomerAttributeStoreModel/AddressAttributeStoreModel pattern. - OrderController/PaymentTransactionController.RefreshPage: added directly to the shared OrderModel.UploadLicenseModel and PaymentTransactionModel, mirroring the existing CustomerAttributeModel.IsReadOnly pattern (harmless if unused by Admin/Vendor). - PageController.ShowCopyButton: same, added to the shared PageModel. - BlogController.Comments had no model at all; added a small Store-only BlogCommentListModel. Verified with dotnet build on Store, Admin, and Vendor (all three reference the touched shared AdminShared models) - 0 errors, only a pre-existing unrelated warning in Grand.Business.Common. dotnet test on Grand.Web.Store.Tests stays green (no behavior touched by this change is covered by unit tests, but the build is the safety net for Razor view compilation here).
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Resolves: no tracked issue - internal security audit of the store-owner/store-manager panel (
Grand.Web.Store), analogous to the earlierGrand.Web.Vendoraudit (#782).Type: bugfix
Issue
Audited every controller in
src/Web/Grand.Web.Store(the store-owner/store-manager panel, scoped byWorkContext.CurrentCustomer.StaffStoreId) for IDOR-style gaps: entities loaded by an id from the request and mutated without verifying they belong to the current store manager's store.Found:
ProductUpdate/ProductDelete/ProductAddPopupmutate the product's category/collection list, not the category/collection entity - the missing check was on the product, not the category/collection.orderIdparameter was used to build a customer-facing notification email, letting a manager forge/misdirect it using another store's order data.StoreContext.CurrentStore) instead ofStaffStoreId, letting the result set diverge from the manager's actual store depending on which host the panel is reached through.IsStoreOwnerAccessAllowed(used by MerchandiseReturnReason/Action) used a non-exclusive "is my store one of the assigned stores" check instead of requiring exclusive ownership, letting one store's manager edit-and-reassign-away or delete an entity another store still depends on.Along the way, also found and cleaned up a related code-quality issue the user flagged:
ViewBagused as an untyped data carrier between controller and view in this area.Solution
entity.AccessToEntityByStore(StaffStoreId)/entity.StoreId == StaffStoreIdchecks on the loaded entity immediately before every mutation identified above, matching the pattern already used consistently elsewhere in each file.*ViewModelServicemethod (also used by Admin) mutates entities referenced by an id list (e.g.InsertAssociatedProductModel/InsertCategoryProductModel/InsertCollectionProductModel), the Store controller now pre-filters that list to store-owned entities before calling the shared service.MerchandiseReturnController.MerchandiseReturnNoteAddnow validatesorder.Id == merchandiseReturn.OrderId.SettingController.IsStoreOwnerAccessAllowednow additionally requires exclusive ownership (Stores.Count == 1), fixing all fourMerchandiseReturnReason/Actioncall sites at once;MessageTemplateControllernow callsAccessToEntityByStoredirectly instead of its own looser check.ShippingControllerTestsfixtures that encoded the old (vulnerable) behavior and addedRestrictionSave_GlobalShippingMethod_NotUpdatedas a regression test.ViewBagdata carriers that actually cross the controller-to-view boundary (IsReadOnly,RefreshPage,ShowCopyButton,FilterByBlogPostId) with typed model properties, following the two patterns already established in this area (XxxStoreModel : XxxModelsubclass + AutoMapper profile, or a property added directly to the shared model when it's a generic UI flag). Removed 17 deadViewBag.AllLanguages/ViewBag.productIdsInputassignments that nothing ever read (also fixing two missing-awaitcalls hidden inside that dead code). LeftViewBag.Title(152 of the original 183 occurrences) untouched - it's the standard Razor<title>idiom, set only in views.Breaking changes
None. All changes are internal to
Grand.Web.Storerequest handling, plus small additive (non-breaking) properties on sharedGrand.Web.AdminSharedview models (PageModel.ShowCopyButton,OrderModel.UploadLicenseModel.RefreshPage,PaymentTransactionModel.RefreshPage) that default tofalseand are unused by Admin/Vendor.Testing
dotnet build src/Web/Grand.Web.Store/Grand.Web.Store.csproj- 0 errors, 0 warnings.dotnet build src/Web/Grand.Web.Admin/Grand.Web.Admin.csprojandsrc/Web/Grand.Web.Vendor/Grand.Web.Vendor.csproj- 0 errors, 0 warnings (both reference the touched sharedGrand.Web.AdminSharedmodels).dotnet test src/Tests/Grand.Web.Store.Tests/Grand.Web.Store.Tests.csproj- 18/18 passing.ProductCategoryUpdate/RelatedProductUpdate/etc. with aproductIdbelonging to another store, orRestrictionSaveagainst a global shipping method) now redirect/no-op instead of mutating the foreign entity.