Skip to content

Commit

Permalink
[CVE-2023-49783] Opt in to permission checks in bulk loaders
Browse files Browse the repository at this point in the history
  • Loading branch information
GuySartorelli committed Jan 22, 2024
1 parent 0443a72 commit 9b96bbd
Show file tree
Hide file tree
Showing 11 changed files with 148 additions and 10 deletions.
1 change: 1 addition & 0 deletions code/GroupImportForm.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ public function __construct($controller, $name, $fields = null, $actions = null,
public function doImport($data, $form)
{
$loader = new GroupCsvBulkLoader();
$loader->setCheckPermissions(true);

// load file
$result = $loader->load($data['CsvFile']['tmp_name']);
Expand Down
1 change: 1 addition & 0 deletions code/MemberImportForm.php
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ public function __construct($controller, $name, $fields = null, $actions = null,
public function doImport($data, $form)
{
$loader = new MemberCsvBulkLoader();
$loader->setCheckPermissions(true);

// optionally set group relation
if ($this->group) {
Expand Down
22 changes: 14 additions & 8 deletions code/ModelAdmin.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
use SilverStripe\Control\Controller;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Control\HTTPResponse_Exception;
use SilverStripe\Core\ClassInfo;
use SilverStripe\Core\Convert;
use SilverStripe\Core\Injector\Injector;
Expand Down Expand Up @@ -620,7 +621,11 @@ public function getModelImporters()

$importers = [];
foreach ($importerClasses as $modelClass => $importerClass) {
$importers[$modelClass] = new $importerClass($modelClass);
$importer = new $importerClass($modelClass);
if (ClassInfo::hasMethod($importer, 'setCheckPermissions')) {
$importer->setCheckPermissions(true);
}
$importers[$modelClass] = $importer;
}

return $importers;
Expand All @@ -647,19 +652,14 @@ public function ImportForm()
return false;
}

if (!$modelSNG->canCreate(Security::getCurrentUser())) {
return false;
}

$fields = new FieldList(
new HiddenField('ClassName', false, $this->modelClass),
new FileField('_CsvFile', false)
);

// get HTML specification for each import (column names etc.)
$importerClass = $importers[$this->modelTab];
/** @var BulkLoader $importer */
$importer = new $importerClass($this->modelClass);
$importer = $importers[$this->modelTab];
$spec = $importer->getImportSpec();
$specFields = new ArrayList();
foreach ($spec['fields'] as $name => $desc) {
Expand Down Expand Up @@ -744,7 +744,13 @@ public function import($data, $form, $request)
if (!empty($data['EmptyBeforeImport']) && $data['EmptyBeforeImport']) { //clear database before import
$loader->deleteExistingRecords = true;
}
$results = $loader->load($_FILES['_CsvFile']['tmp_name']);
try {
$results = $loader->load($_FILES['_CsvFile']['tmp_name']);
} catch (HTTPResponse_Exception $e) {
$form->sessionMessage($e->getMessage(), ValidationResult::TYPE_ERROR);
$this->redirectBack();
return false;
}

$message = '';

Expand Down
4 changes: 2 additions & 2 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
}
],
"require": {
"silverstripe/framework": "^4.10",
"silverstripe/framework": "^4.13.39",
"silverstripe/versioned": "^1@dev",
"silverstripe/vendor-plugin": "^1.0",
"php": "^7.4 || ^8.0"
Expand Down Expand Up @@ -54,4 +54,4 @@
},
"minimum-stability": "dev",
"prefer-stable": true
}
}
2 changes: 2 additions & 0 deletions tests/behat/features/files/import-company-create.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Name,Category,Revenue,CEO
"New Company",Retail,123,"New person"
1 change: 1 addition & 0 deletions tests/behat/features/files/import-company-delete.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ID,Name,Category,Revenue,CEO
2 changes: 2 additions & 0 deletions tests/behat/features/files/import-company-edit.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ID,Name,Category,Revenue,CEO
1,"Some other company",Retail,123,"some person"
83 changes: 83 additions & 0 deletions tests/behat/features/gridfield-import.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
Feature: Import in GridField
As a site owner
I want confidence that only users with permission can import records
So that I can sleep at night

Background:
Given the "Company" "Walmart" with "Category"="Retail"
And I am logged in with "ADMIN" permissions

Scenario: I can create new records via the import form
When I go to "/admin/test"
Then I should see 1 ".col-Name" elements
And I press the "Import CSV" button
Then I should see the "#Form_ImportForm" element
When I attach the file "import-company-create.csv" to the "_CsvFile" field
And I press the "Import from CSV" button
Then I should see "Imported 1 record" in the "#Form_ImportForm" element
And I should see 2 ".col-Name" elements

Scenario: I cannot create new records without permission
Given I add an extension "SilverStripe\Admin\Tests\Behat\Context\Extension\CannotCreateExtension" to the "SilverStripe\FrameworkTest\Model\Company" class
When I go to "/admin/test"
And I press the "Import CSV" button
Then I should see the "#Form_ImportForm" element
When I attach the file "import-company-create.csv" to the "_CsvFile" field
And I press the "Import from CSV" button
Then I should see "Not allowed to create 'Company' records" in the "#Form_ImportForm" element
And I should see 1 ".col-Name" elements
And I should see "Walmart" in the ".col-Name" element

Scenario: I can edit existing records via the import form
# To edit, we have to rely on IDs - so start by validating that a record with that ID exists at all.
When I go to "/admin/test/SilverStripe-FrameworkTest-Model-Company/EditForm/field/SilverStripe-FrameworkTest-Model-Company/item/1"
# Check we are viewing a record (we don't know or care what it's called)
Then I should see "Main" in the "div.cms-content-header-tabs" element
# Check we didn't get a not found error
And I should not see "Not Found"
When I go to "/admin/test"
And I press the "Import CSV" button
Then I should see the "#Form_ImportForm" element
When I attach the file "import-company-edit.csv" to the "_CsvFile" field
And I press the "Import from CSV" button
Then I should see "Updated 1 record" in the "#Form_ImportForm" element
And I should see 1 ".col-Name" elements
And I should not see "Walmart" in the ".col-Name" element
And I should see "Some other company" in the ".col-Name" element

Scenario: I cannot edit existing records without permission
Given I add an extension "SilverStripe\Admin\Tests\Behat\Context\Extension\CannotEditExtension" to the "SilverStripe\FrameworkTest\Model\Company" class
# We can't check if the record exists here because we don't have permission! But if it doesn't, this test will fail anyway.
When I go to "/admin/test"
And I press the "Import CSV" button
Then I should see the "#Form_ImportForm" element
When I attach the file "import-company-edit.csv" to the "_CsvFile" field
And I press the "Import from CSV" button
Then I should see "Not allowed to edit 'Company' records" in the "#Form_ImportForm" element
And I should see 1 ".col-Name" elements
And I should see "Walmart" in the ".col-Name" element
And I should not see "Some other company" in the ".col-Name" element

Scenario: I can delete records via the import form
When I go to "/admin/test"
And I press the "Import CSV" button
Then I should see the "#Form_ImportForm" element
When I attach the file "import-company-delete.csv" to the "_CsvFile" field
And I check "Replace data"
And I press the "Import from CSV" button
Then I should see "Nothing to import" in the "#Form_ImportForm" element
And I should see "No items found" in the ".ss-gridfield-items" element
And I should not see the ".col-Name" element

Scenario: I cannot delete records without permission
Given I add an extension "SilverStripe\Admin\Tests\Behat\Context\Extension\CannotDeleteExtension" to the "SilverStripe\FrameworkTest\Model\Company" class
When I go to "/admin/test"
And I press the "Import CSV" button
Then I should see the "#Form_ImportForm" element
When I attach the file "import-company-delete.csv" to the "_CsvFile" field
And I check "Replace data"
And I press the "Import from CSV" button
Then I should see "Not allowed to delete 'Company' records" in the "#Form_ImportForm" element
And I should not see "No items found" in the ".ss-gridfield-items" element
And I should see 1 ".col-Name" elements
And I should see "Walmart" in the ".col-Name" element
14 changes: 14 additions & 0 deletions tests/behat/src/Extension/CannotCreateExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

namespace SilverStripe\Admin\Tests\Behat\Context\Extension;

use SilverStripe\Core\Extension;
use SilverStripe\Dev\TestOnly;

class CannotCreateExtension extends Extension implements TestOnly
{
public function canCreate()
{
return false;
}
}
14 changes: 14 additions & 0 deletions tests/behat/src/Extension/CannotDeleteExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

namespace SilverStripe\Admin\Tests\Behat\Context\Extension;

use SilverStripe\Core\Extension;
use SilverStripe\Dev\TestOnly;

class CannotDeleteExtension extends Extension implements TestOnly
{
public function canDelete()
{
return false;
}
}
14 changes: 14 additions & 0 deletions tests/behat/src/Extension/CannotEditExtension.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?php

namespace SilverStripe\Admin\Tests\Behat\Context\Extension;

use SilverStripe\Core\Extension;
use SilverStripe\Dev\TestOnly;

class CannotEditExtension extends Extension implements TestOnly
{
public function canEdit()
{
return false;
}
}

0 comments on commit 9b96bbd

Please sign in to comment.