From dfb8d73a9282f0c6108eda596287343b884d44d7 Mon Sep 17 00:00:00 2001 From: Sami Mokaddem Date: Mon, 7 Feb 2022 10:37:03 +0100 Subject: [PATCH 1/5] fix: [userSettings] Renamed template to match the controller endpoint --- .../UserSettings/{delete_bookmark.php => delete_my_bookmark.php} | 0 .../UserSettings/{save_bookmark.php => save_my_bookmark.php} | 0 templates/UserSettings/{set_setting.php => set_my_setting.php} | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename templates/UserSettings/{delete_bookmark.php => delete_my_bookmark.php} (100%) rename templates/UserSettings/{save_bookmark.php => save_my_bookmark.php} (100%) rename templates/UserSettings/{set_setting.php => set_my_setting.php} (100%) diff --git a/templates/UserSettings/delete_bookmark.php b/templates/UserSettings/delete_my_bookmark.php similarity index 100% rename from templates/UserSettings/delete_bookmark.php rename to templates/UserSettings/delete_my_bookmark.php diff --git a/templates/UserSettings/save_bookmark.php b/templates/UserSettings/save_my_bookmark.php similarity index 100% rename from templates/UserSettings/save_bookmark.php rename to templates/UserSettings/save_my_bookmark.php diff --git a/templates/UserSettings/set_setting.php b/templates/UserSettings/set_my_setting.php similarity index 100% rename from templates/UserSettings/set_setting.php rename to templates/UserSettings/set_my_setting.php From 14ec995c2bd618b181197dc6b64e63fd966b4860 Mon Sep 17 00:00:00 2001 From: Sami Mokaddem Date: Mon, 7 Feb 2022 10:48:55 +0100 Subject: [PATCH 2/5] fix: [userSettings] Perform URI validation for bookmarks - As reported by Dawid Czarnecki from Zigrin Security --- src/Model/Table/UserSettingsTable.php | 14 +++++++++++++ templates/Instance/home.php | 20 ++++++++++++++----- .../layouts/sidebar/bookmark-entry.php | 9 +++++++-- 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/Model/Table/UserSettingsTable.php b/src/Model/Table/UserSettingsTable.php index bdfe535..cc4b5db 100644 --- a/src/Model/Table/UserSettingsTable.php +++ b/src/Model/Table/UserSettingsTable.php @@ -135,4 +135,18 @@ class UserSettingsTable extends AppTable } return $result; } + + /** + * validURI - Ensure the provided URI can be safely put as a link + * + * @param String $uri + * @return bool if the URI is safe to be put as a link + */ + public function validURI(String $uri): bool + { + $parsed = parse_url($uri); + $isLocalPath = empty($parsed['scheme']) && empty($parsed['domain']) && !empty($parsed['path']); + $isValidURL = !empty($parsed['scheme']) && in_array($parsed['scheme'], ['http', 'https']) && filter_var($uri, FILTER_SANITIZE_URL); + return $isLocalPath || $isValidURL; + } } diff --git a/templates/Instance/home.php b/templates/Instance/home.php index 6d91266..e5803c2 100644 --- a/templates/Instance/home.php +++ b/templates/Instance/home.php @@ -1,5 +1,9 @@ user_settings_by_name['ui.bookmarks']['value']) ? json_decode($loggedUser->user_settings_by_name['ui.bookmarks']['value'], true) : []; +$this->userSettingsTable = TableRegistry::getTableLocator()->get('UserSettings'); ?>

@@ -9,18 +13,24 @@ $bookmarks = !empty($loggedUser->user_settings_by_name['ui.bookmarks']['value'])

- +
  • - - - + userSettingsTable->validURI($bookmark['url'])): ?> + + + + + + + +
- +

diff --git a/templates/element/layouts/sidebar/bookmark-entry.php b/templates/element/layouts/sidebar/bookmark-entry.php index f2d01c1..a33ffe4 100644 --- a/templates/element/layouts/sidebar/bookmark-entry.php +++ b/templates/element/layouts/sidebar/bookmark-entry.php @@ -1,5 +1,8 @@ userSettingsTable = TableRegistry::getTableLocator()->get('UserSettings'); $seed = 'sb-' . mt_rand(); $icon = $entry['icon'] ?? ''; @@ -14,6 +17,8 @@ $active = true; } + $validURI = $this->userSettingsTable->validURI($url); + echo $this->Bootstrap->button([ 'nodeType' => 'a', 'text' => h($label), @@ -22,9 +27,9 @@ 'outline' => !$active, 'size' => 'sm', 'icon' => h($icon), - 'class' => ['mb-1'], + 'class' => ['mb-1', !$validURI ? 'disabled' : ''], 'params' => [ - 'href' => h($url), + 'href' => $validURI ? h($url) : '#', ] ]); ?> From 336dfb091c6f5e298c6db2d5290ebbf4fa817d01 Mon Sep 17 00:00:00 2001 From: Sami Mokaddem Date: Mon, 7 Feb 2022 11:11:25 +0100 Subject: [PATCH 3/5] chg: [settingTable] Gracefully handle if file not writeable --- src/Model/Table/SettingsTable.php | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/Model/Table/SettingsTable.php b/src/Model/Table/SettingsTable.php index 7fe18ac..cbd14c2 100644 --- a/src/Model/Table/SettingsTable.php +++ b/src/Model/Table/SettingsTable.php @@ -3,6 +3,7 @@ namespace App\Model\Table; use App\Model\Table\AppTable; use Cake\ORM\Table; +use Cake\Filesystem\File; use Cake\Core\Configure; use Cake\Error\Debugger; @@ -85,6 +86,8 @@ class SettingsTable extends AppTable if (!empty($setting['afterSave'])) { $this->SettingsProvider->evaluateFunctionForSetting($setting['afterSave'], $setting); } + } else { + $errors[] = __('Could not save settings on disk'); } } return $errors; @@ -129,8 +132,16 @@ class SettingsTable extends AppTable $settings = $this->readSettings(); $settings[$name] = $value; $settings = json_encode($settings, JSON_PRETTY_PRINT); - file_put_contents(CONFIG . 'config.json', $settings); - $this->loadSettings(); - return true; + $path = CONFIG . 'config.json'; + $file = new File($path); + if ($file->writable()) { + $success = file_put_contents($path, $settings); + if ($success) { + $this->loadSettings(); + } + } else { + $success = false; + } + return $success; } } From e13b4e7bc5f1a0ff59b52162cc99405e89c0544a Mon Sep 17 00:00:00 2001 From: Sami Mokaddem Date: Mon, 7 Feb 2022 11:43:09 +0100 Subject: [PATCH 4/5] fix: [settings:settingField] Enforce sanitization of input fields - As reported by Dawid Czarnecki from Zigrin Security --- templates/element/Settings/field.php | 32 ++++++++++++++-------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/templates/element/Settings/field.php b/templates/element/Settings/field.php index 34bdde8..d52bc14 100644 --- a/templates/element/Settings/field.php +++ b/templates/element/Settings/field.php @@ -13,11 +13,11 @@ (!empty($setting['error']) ? $appView->get('variantFromSeverity')[$setting['severity']] : ''), ], ($setting['type'] == 'textarea' ? '' : 'type') => ($setting['type'] == 'textarea' ? '' : 'text'), - 'id' => $settingId, - 'data-setting-name' => $settingName, - 'value' => isset($setting['value']) ? $setting['value'] : "", - 'placeholder' => $setting['default'] ?? '', - 'aria-describedby' => "{$settingId}Help" + 'id' => h($settingId), + 'data-setting-name' => h($settingName), + 'value' => isset($setting['value']) ? h($setting['value']) : "", + 'placeholder' => empty($setting['default']) ? '' : h($setting['default']), + 'aria-describedby' => h("{$settingId}Help") ] ); })($settingName, $setting, $this); @@ -28,13 +28,13 @@ return $this->Bootstrap->switch([ 'label' => h($setting['description']), 'checked' => !empty($setting['value']), - 'id' => $settingId, + 'id' => h($settingId), 'class' => [ (!empty($setting['error']) ? 'is-invalid' : ''), (!empty($setting['error']) ? $appView->get('variantFromSeverity')[$setting['severity']] : ''), ], 'attrs' => [ - 'data-setting-name' => $settingName + 'data-setting-name' => h($settingName) ] ]); })($settingName, $setting, $this); @@ -53,16 +53,16 @@ 'type' => 'number', 'min' => '0', 'step' => 1, - 'id' => $settingId, - 'data-setting-name' => $settingName, - 'aria-describedby' => "{$settingId}Help" + 'id' => h($settingId), + 'data-setting-name' => h($settingName), + 'aria-describedby' => h("{$settingId}Help") ]); })($settingName, $setting, $this); } elseif ($setting['type'] == 'select' || $setting['type'] == 'multi-select') { $input = (function ($settingName, $setting, $appView) { $settingId = str_replace('.', '_', $settingName); - $setting['value'] = $setting['value'] ?? ''; + $setting['value'] = empty($setting['value']) ? '' : h($setting['value']); if ($setting['type'] == 'multi-select') { if (!is_array($setting['value'])) { $firstChar = substr($setting['value'], 0, 1); @@ -77,7 +77,7 @@ foreach ($setting['options'] as $key => $value) { $optionParam = [ 'class' => [], - 'value' => $key, + 'value' => h($key), ]; if ($setting['type'] == 'multi-select') { if (in_array($key, $setting['value'])) { @@ -100,10 +100,10 @@ (!empty($setting['error']) ? $appView->get('variantFromSeverity')[$setting['severity']] : ''), ], ($setting['type'] == 'multi-select' ? 'multiple' : '') => ($setting['type'] == 'multi-select' ? 'multiple' : ''), - 'id' => $settingId, - 'data-setting-name' => $settingName, - 'placeholder' => $setting['default'] ?? '', - 'aria-describedby' => "{$settingId}Help" + 'id' => h($settingId), + 'data-setting-name' => h($settingName), + 'placeholder' => empty($setting['default']) ? '' : h($setting['default']), + 'aria-describedby' => h("{$settingId}Help") ], $options); })($settingName, $setting, $this); } From ad3e89199bb6243cdffd6029ef1d620d5e8009b7 Mon Sep 17 00:00:00 2001 From: Sami Mokaddem Date: Mon, 7 Feb 2022 12:01:07 +0100 Subject: [PATCH 5/5] chg: [settingTable] Added value validation before saving the setting --- src/Model/Table/SettingsTable.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/Model/Table/SettingsTable.php b/src/Model/Table/SettingsTable.php index cbd14c2..2eb941c 100644 --- a/src/Model/Table/SettingsTable.php +++ b/src/Model/Table/SettingsTable.php @@ -74,10 +74,18 @@ class SettingsTable extends AppTable } } $setting['value'] = $value ?? ''; + if (isset($setting['test'])) { + $validationResult = $this->SettingsProvider->evaluateFunctionForSetting($setting['test'], $setting); + if ($validationResult !== true) { + $errors[] = $validationResult; + $setting['errorMessage'] = $validationResult; + } + } if (empty($errors) && !empty($setting['beforeSave'])) { $beforeSaveResult = $this->SettingsProvider->evaluateFunctionForSetting($setting['beforeSave'], $setting); if ($beforeSaveResult !== true) { $errors[] = $beforeSaveResult; + $setting['errorMessage'] = $validationResult; } } if (empty($errors)) {