Uh oh!
There was an error while loading. Please reload this page.
Fix vendor product IDORs and consolidate ownership checks in Grand.Web.Vendor - #782
Merged
Merged
Conversation
Two confirmed IDOR bugs in Grand.Web.Vendor product sub-resource mutations, plus a consolidation pass on how vendor ownership is checked across the area: - BundleProductModel field mismatch: Update/DeleteBundleProductModel loaded and mutated the parent product via ProductBundleId, but the global IProductValidVendor filter only validated ProductId (the bundled component). A vendor could pass their own product as ProductId to pass validation while mutating another vendor's ProductBundleId. Fixed with an explicit ownership check on the loaded parent. - ProductRelatedValidVendor OR-bypass: accepted ownership of either ProductId1 or ProductId2, but RelatedProductUpdate/Delete and SimilarProductUpdate/Delete only ever mutate ProductId1's mapping list. An attacker owning any product could supply it as ProductId2 to pass validation while editing someone else's ProductId1. Fixed by requiring ownership of ProductId1 only. - Consolidated ~50 inline `entity.VendorId != CurrentVendor.Id` checks (including a duplicate of HasAccessToProduct living as a private method on ProductController) onto the existing IWorkContext.HasAccessToX(entity) extensions in Extensions/HasAccess.cs, and added HasAccessToVendorReview / HasAccessToMerchandiseReturn to cover the two controllers that had no shared helper at all. Checking the loaded entity right before mutation (instead of a request DTO field) is what makes the first two bugs structurally impossible to reintroduce. - Documented the pattern and the failure mode with XML doc comments on HasAccess.cs and IProductValidVendor.cs. No test project exists yet for Grand.Web.Vendor; verified via `dotnet build src/Web/Grand.Web.Vendor/Grand.Web.Vendor.csproj`. Co-Authored-By: Claude Sonnet 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 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.
Type: bugfix
Issue
Two IDOR vulnerabilities were found in
Grand.Web.Vendorproduct sub-resource mutations:ProductViewModelService.UpdateBundleProductModel/DeleteBundleProductModel): the model carries two ids —ProductBundleId(the parent product actually loaded and mutated) andProductId(the bundled component). The globalIProductValidVendorownership filter only validatesProductId. A vendor could submit their own product asProductIdto pass validation while mutating another vendor's bundle-product list viaProductBundleId.ProductRelatedValidVendorOR-bypass: the validator behindIProductRelatedValidVendoraccepted ownership of eitherProductId1orProductId2, butRelatedProductUpdate/DeleteandSimilarProductUpdate/Deleteonly ever read/mutateProductId1's mapping list. A vendor owning any product could supply it asProductId2to pass validation while editing/deleting another vendor'sProductId1mapping entry.Separately, ownership checks across this area were duplicated ad hoc (~50 inline
entity.VendorId != CurrentVendor.Idcomparisons, plus a privateCheckAccessToProductonProductControllerthat reimplemented an existing shared helper), which is how issue #1 above stayed unnoticed — the check that matters is easy to miss or misplace when it isn't consolidated.Solution
UpdateBundleProductModel/DeleteBundleProductModel.ProductRelatedValidVendorto require ownership ofProductId1only, matching what the consuming actions actually mutate.IWorkContext.HasAccessToX(entity)extensions inExtensions/HasAccess.cs(checking the loaded entity right before mutation, not a request DTO field), removing the duplicated inline comparisons and the redundantCheckAccessToProduct. AddedHasAccessToVendorReview/HasAccessToMerchandiseReturnfor the two controllers that had no shared helper yet.HasAccess.csandIProductValidVendor.csso a future model with more than one product-id-shaped field doesn't reintroduce the same bug.Breaking changes
None.
ProductRelatedValidVendorbecomes stricter (was accepting an alternative ownership path that was never intentionally needed by any current caller); all other changes are behavior-preserving refactors of duplicated checks onto a single existing helper.Testing
dotnet build src/Web/Grand.Web.Vendor/Grand.Web.Vendor.csproj— builds clean, 0 warnings/errors.Id/ProductBundleIdfields via devtools on a page you have access to, or via direct POST), then POST toBundleProductUpdate/BundleProductDeletewithProductBundleId= vendor B's product andProductId= one of vendor A's own products. Before this fix: request succeeds and mutates vendor B's product. After: request throws/returns a permission error.RelatedProductUpdate/RelatedProductDelete(or the Similar-product equivalents) withProductId1= vendor B's product (and an existing mappingIdon it) andProductId2= one of vendor A's own products. Before: request succeeds. After: request is rejected byProductRelatedValidVendor.No dedicated
Grand.Web.Vendor.Testsproject exists yet, so this is verified via manual testing anddotnet buildrather than an automated test run.🤖 Generated with Claude Code