Skip to content

Commit

Permalink
new: [security] added functionality to tighten bookmark creation rules
Browse files Browse the repository at this point in the history
- site admins can now limit the baseurls of the provided bookmark URLs to a list of values via the server settings
  • Loading branch information
iglocska committed Nov 22, 2024
1 parent ab331dc commit 55cac2e
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 0 deletions.
8 changes: 8 additions & 0 deletions src/Controller/UserSettingsController.php
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ public function add($user_id=null)
if (empty($currentUser['role']['perm_community_admin'])) {
$data['user_id'] = $currentUser->id;
}
$validationResult = $this->UserSettings->validateUserSetting($data, $currentUser);
if (!$validationResult !== true) {
throw new MethodNotAllowedException(__('You cannot create the given user setting. Reason: {0}', $validationResult));
}
return $data;
}
]);
Expand Down Expand Up @@ -131,6 +135,10 @@ public function edit($id)
if ($data['user_id'] != $entity->user_id) {
throw new MethodNotAllowedException(__('You cannot assign the setting to a different user.'));
}
$validationResult = $this->UserSettings->validateUserSetting($data);
if ($validationResult !== true) {
throw new MethodNotAllowedException(__('Setting value: {0}', $validationResult));
}
return $data;
}
]);
Expand Down
11 changes: 11 additions & 0 deletions src/Model/Table/SettingProviders/CerebrateSettingsProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,17 @@ protected function generateSettingsConfiguration()
],
]
],
'Restrictions' => [
'Allowed bookmark domains' => [
'security.restrictions.allowed_bookmark_domains' => [
'name' => __('Allowed bookmark domains'),
'type' => 'string',
'severity' => 'info',
'description' => __('Comma separated list of allowed bookmark domains. Leave empty to allow all domains.'),
'default' => '',
],
],
],
'Development' => [
'Debugging' => [
'debug' => [
Expand Down
45 changes: 45 additions & 0 deletions src/Model/Table/UserSettingsTable.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use App\Model\Table\AppTable;
use Cake\ORM\Table;
use Cake\Validation\Validator;
use Cake\Core\Configure;

require_once(APP . 'Model' . DS . 'Table' . DS . 'SettingProviders' . DS . 'UserSettingsProvider.php');
use App\Settings\SettingsProvider\UserSettingsProvider;
Expand Down Expand Up @@ -102,6 +103,14 @@ public function saveBookmark($user, $data)
'name' => $data['bookmark_name'],
'url' => $data['bookmark_url'],
];
$restricted_domains = Configure::read('Security.restrictions.allowed_bookmark_domains');
if (!empty($restricted_domains)) {
$restricted_domains = explode(',', $restricted_domains);
$parsed = parse_url($bookmarkData['url']);
if (!in_array($parsed['host'], $restricted_domains)) {
return null;
}
}
if (is_null($setting)) { // setting not found, create it
$bookmarksData = json_encode([$bookmarkData]);
$result = $this->createSetting($user, $this->BOOKMARK_SETTING_NAME, $bookmarksData);
Expand Down Expand Up @@ -153,4 +162,40 @@ public function validURI(String $uri): bool
}
return $isLocalPath || $isValidURL;
}

public function validateUserSetting($data): bool|string
{
$errors = [];
$json_fields = ['ui.bookmarks'];
if (in_array($data['name'], $json_fields)) {
$decoded = json_decode($data['value'], true);
if (json_last_error() !== JSON_ERROR_NONE) {
return __('Invalid JSON data');
}
$value = $decoded;
} else {
$value = $data['value'];
}
if ($data['name'] === 'ui.bookmarks') {
if (array_values($value) !== $value) {
$value = [$value];
}
$restricted_domains = Configure::read('security.restrictions.allowed_bookmark_domains');
if (!empty($restricted_domains)) {
$restricted_domains = explode(',', $restricted_domains);
foreach ($restricted_domains as &$rd) {
$rd = trim($rd);
}
}
foreach ($value as $bookmark) {
if (!empty($restricted_domains)) {
$parsed = parse_url($bookmark['url']);
if (!in_array($parsed['host'], $restricted_domains)) {
return __('Invalid domain for bookmark. The only domains allowed are: {0}', implode(', ', $restricted_domains));
}
}
}
}
return true;
}
}

1 comment on commit 55cac2e

@Wachizungu
Copy link
Contributor

@Wachizungu Wachizungu commented on 55cac2e Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iglocska With current solution, only exact matches work. So if you add sld.tld to allowlist, and then someone tries to add bookmark www.sld.tld it doesn't work. Not sure if that was intended.

I also get internal error message for the case it's not in allowlist.

Small fix PR as well: #187

Please sign in to comment.