Skip to content

Commit e07e8f7

Browse files
KrzysztofPajakKrzysztofPajak
authored
fix: close cross-store IDOR in Category/Collection/Brand ImportFromXlsx (#830)
1 parent 3f49a5b commit e07e8f7

6 files changed

Lines changed: 141 additions & 0 deletions

File tree

‎src/Tests/Grand.Web.Admin.Tests/Controllers/BaseBrandControllerTests.cs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -451,4 +451,40 @@ public async Task ImportFromXlsx_NoFile_ShowsErrorAndRedirectsToList()
451451
Assert.AreEqual("List", redirect.ActionName);
452452
importManagerMock.Verify(i => i.Import(It.IsAny<Stream>()), Times.Never);
453453
}
454+
455+
// --- ImportFromXlsx (security fix: ImportManager<T> upserts by an attacker-supplied Id with no
456+
// ownership check - store-scoped callers must be denied outright) ------------------------------
457+
458+
[TestMethod]
459+
public async Task ImportFromXlsx_StoreScope_DeniesAndNeverCallsImportManager()
460+
{
461+
_scopeMock.Setup(s => s.DefaultStoreId).Returns("store-1");
462+
var importManagerMock = new Mock<Grand.Business.Core.Interfaces.ExportImport.IImportManager<Grand.Business.Core.Dto.BrandDto>>();
463+
var fileMock = new Mock<IFormFile>();
464+
fileMock.Setup(f => f.Length).Returns(100);
465+
466+
var result = await _controller.ImportFromXlsx(fileMock.Object, importManagerMock.Object);
467+
468+
var redirect = result as RedirectToActionResult;
469+
Assert.IsNotNull(redirect);
470+
Assert.AreEqual("List", redirect.ActionName);
471+
importManagerMock.Verify(m => m.Import(It.IsAny<Stream>()), Times.Never);
472+
}
473+
474+
[TestMethod]
475+
public async Task ImportFromXlsx_GlobalScope_CallsImportManager()
476+
{
477+
_scopeMock.Setup(s => s.DefaultStoreId).Returns((string)null);
478+
var importManagerMock = new Mock<Grand.Business.Core.Interfaces.ExportImport.IImportManager<Grand.Business.Core.Dto.BrandDto>>();
479+
var fileMock = new Mock<IFormFile>();
480+
fileMock.Setup(f => f.Length).Returns(100);
481+
fileMock.Setup(f => f.OpenReadStream()).Returns(Stream.Null);
482+
483+
var result = await _controller.ImportFromXlsx(fileMock.Object, importManagerMock.Object);
484+
485+
var redirect = result as RedirectToActionResult;
486+
Assert.IsNotNull(redirect);
487+
Assert.AreEqual("List", redirect.ActionName);
488+
importManagerMock.Verify(m => m.Import(It.IsAny<Stream>()), Times.Once);
489+
}
454490
}

‎src/Tests/Grand.Web.Admin.Tests/Controllers/BaseCategoryControllerTests.cs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -544,4 +544,40 @@ public async Task ProductList_ScopeGrantsAccess_ReturnsData()
544544
Assert.AreEqual(1, gridModel.Total);
545545
Assert.IsNotNull(gridModel.Data);
546546
}
547+
548+
// --- ImportFromXlsx (security fix: ImportManager<T> upserts by an attacker-supplied Id with no
549+
// ownership check - store-scoped callers must be denied outright) ------------------------------
550+
551+
[TestMethod]
552+
public async Task ImportFromXlsx_StoreScope_DeniesAndNeverCallsImportManager()
553+
{
554+
_scopeMock.Setup(s => s.DefaultStoreId).Returns("store-1");
555+
var importManagerMock = new Mock<Grand.Business.Core.Interfaces.ExportImport.IImportManager<Grand.Business.Core.Dto.CategoryDto>>();
556+
var fileMock = new Mock<IFormFile>();
557+
fileMock.Setup(f => f.Length).Returns(100);
558+
559+
var result = await _controller.ImportFromXlsx(fileMock.Object, importManagerMock.Object);
560+
561+
var redirect = result as RedirectToActionResult;
562+
Assert.IsNotNull(redirect);
563+
Assert.AreEqual("List", redirect.ActionName);
564+
importManagerMock.Verify(m => m.Import(It.IsAny<Stream>()), Times.Never);
565+
}
566+
567+
[TestMethod]
568+
public async Task ImportFromXlsx_GlobalScope_CallsImportManager()
569+
{
570+
_scopeMock.Setup(s => s.DefaultStoreId).Returns((string)null);
571+
var importManagerMock = new Mock<Grand.Business.Core.Interfaces.ExportImport.IImportManager<Grand.Business.Core.Dto.CategoryDto>>();
572+
var fileMock = new Mock<IFormFile>();
573+
fileMock.Setup(f => f.Length).Returns(100);
574+
fileMock.Setup(f => f.OpenReadStream()).Returns(Stream.Null);
575+
576+
var result = await _controller.ImportFromXlsx(fileMock.Object, importManagerMock.Object);
577+
578+
var redirect = result as RedirectToActionResult;
579+
Assert.IsNotNull(redirect);
580+
Assert.AreEqual("List", redirect.ActionName);
581+
importManagerMock.Verify(m => m.Import(It.IsAny<Stream>()), Times.Once);
582+
}
547583
}

‎src/Tests/Grand.Web.Admin.Tests/Controllers/BaseCollectionControllerTests.cs‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -604,4 +604,40 @@ public async Task ProductAddPopupInsert_StoreScope_AllProductsForeign_SkipsInser
604604

605605
_collectionViewModelServiceMock.Verify(v => v.InsertCollectionProductModel(It.IsAny<CollectionModel.AddCollectionProductModel>()), Times.Never);
606606
}
607+
608+
// --- ImportFromXlsx (security fix: ImportManager<T> upserts by an attacker-supplied Id with no
609+
// ownership check - store-scoped callers must be denied outright) ------------------------------
610+
611+
[TestMethod]
612+
public async Task ImportFromXlsx_StoreScope_DeniesAndNeverCallsImportManager()
613+
{
614+
_scopeMock.Setup(s => s.DefaultStoreId).Returns("store-1");
615+
var importManagerMock = new Mock<Grand.Business.Core.Interfaces.ExportImport.IImportManager<Grand.Business.Core.Dto.CollectionDto>>();
616+
var fileMock = new Mock<IFormFile>();
617+
fileMock.Setup(f => f.Length).Returns(100);
618+
619+
var result = await _controller.ImportFromXlsx(fileMock.Object, importManagerMock.Object);
620+
621+
var redirect = result as RedirectToActionResult;
622+
Assert.IsNotNull(redirect);
623+
Assert.AreEqual("List", redirect.ActionName);
624+
importManagerMock.Verify(m => m.Import(It.IsAny<Stream>()), Times.Never);
625+
}
626+
627+
[TestMethod]
628+
public async Task ImportFromXlsx_GlobalScope_CallsImportManager()
629+
{
630+
_scopeMock.Setup(s => s.DefaultStoreId).Returns((string)null);
631+
var importManagerMock = new Mock<Grand.Business.Core.Interfaces.ExportImport.IImportManager<Grand.Business.Core.Dto.CollectionDto>>();
632+
var fileMock = new Mock<IFormFile>();
633+
fileMock.Setup(f => f.Length).Returns(100);
634+
fileMock.Setup(f => f.OpenReadStream()).Returns(Stream.Null);
635+
636+
var result = await _controller.ImportFromXlsx(fileMock.Object, importManagerMock.Object);
637+
638+
var redirect = result as RedirectToActionResult;
639+
Assert.IsNotNull(redirect);
640+
Assert.AreEqual("List", redirect.ActionName);
641+
importManagerMock.Verify(m => m.Import(It.IsAny<Stream>()), Times.Once);
642+
}
607643
}

‎src/Web/Grand.Web.AdminShared/Controllers/BaseBrandController.cs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,17 @@ public async Task<IActionResult> ImportFromXlsx(IFormFile importexcelfile,
268268
{
269269
try
270270
{
271+
// ImportManager<T>/BrandImportDataObject match rows by an attacker-supplied Id and
272+
// upsert with no ownership check at all (see project security-fix history) - a
273+
// store-scoped caller could otherwise create or silently overwrite ANY store's brand.
274+
// Deny outright for non-global scope rather than threading scope through the shared,
275+
// multi-entity import engine.
276+
if (scope.DefaultStoreId is not null)
277+
{
278+
Error(translationService.GetResource("Admin.AccessDenied.Title"));
279+
return RedirectToAction("List");
280+
}
281+
271282
if (importexcelfile is { Length: > 0 })
272283
{
273284
await importManager.Import(importexcelfile.OpenReadStream());

‎src/Web/Grand.Web.AdminShared/Controllers/BaseCategoryController.cs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -236,6 +236,17 @@ public async Task<IActionResult> ImportFromXlsx(IFormFile importexcelfile,
236236
{
237237
try
238238
{
239+
// ImportManager<T>/CategoryImportDataObject match rows by an attacker-supplied Id and
240+
// upsert with no ownership check at all (see project security-fix history) - a
241+
// store-scoped caller could otherwise create or silently overwrite ANY store's category.
242+
// Deny outright for non-global scope rather than threading scope through the shared,
243+
// multi-entity import engine.
244+
if (scope.DefaultStoreId is not null)
245+
{
246+
Error(translationService.GetResource("Admin.AccessDenied.Title"));
247+
return RedirectToAction("List");
248+
}
249+
239250
if (importexcelfile is { Length: > 0 })
240251
{
241252
await importManager.Import(importexcelfile.OpenReadStream());

‎src/Web/Grand.Web.AdminShared/Controllers/BaseCollectionController.cs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,17 @@ public async Task<IActionResult> ImportFromXlsx(IFormFile importexcelfile,
269269
{
270270
try
271271
{
272+
// ImportManager<T>/CollectionImportDataObject match rows by an attacker-supplied Id and
273+
// upsert with no ownership check at all (see project security-fix history) - a
274+
// store-scoped caller could otherwise create or silently overwrite ANY store's
275+
// collection. Deny outright for non-global scope rather than threading scope through the
276+
// shared, multi-entity import engine.
277+
if (scope.DefaultStoreId is not null)
278+
{
279+
Error(translationService.GetResource("Admin.AccessDenied.Title"));
280+
return RedirectToAction("List");
281+
}
282+
272283
if (importexcelfile is { Length: > 0 })
273284
{
274285
await importManager.Import(importexcelfile.OpenReadStream());

0 commit comments

Comments
 (0)