Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CVE-2023-49783] Opt in to permission checks in bulk loaders #1654

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
}
}
Loading