diff --git a/CHANGELOG.md b/CHANGELOG.md index ff1d8c62..7726965f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,10 @@ # Changelog +### Added +- Documented current security review findings and recommendations + ### Fixed +- Escape stored names when rendering and persisting to prevent injection vectors in UI lists - php 8.5 compatibility fix ## 6.0.2 - 2025-09-19 diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md new file mode 100644 index 00000000..3ebf1009 --- /dev/null +++ b/SECURITY_REVIEW.md @@ -0,0 +1,24 @@ +# Security Review + +## Scope and approach +- Reviewed server-side controllers and supporting services for access control gaps and data handling. +- Audited front-end rendering code for unsafe DOM mutations and reflected/stored injection risks. + +## Findings + +### 1) Stored XSS via report names in dataset status sidebar +- The dataset status view concatenates report names into HTML and injects them with `innerHTML` without escaping. A report name containing markup will be rendered as executable HTML when the dataset status loads. 【F:js/dataset.js†L618-L623】 +- Report names are persisted without sanitization during creation, so hostile markup can be stored and reused across sessions. 【F:lib/Service/ReportService.php†L200-L203】 +- Impact: Any user who views the dataset sidebar could have their session hijacked or their data exfiltrated if an attacker can create or rename a report tied to that dataset (e.g., via shared accounts or compromised credentials). +- Suggested mitigation: escape report names before injecting them (e.g., use `textContent`/`innerText`) and sanitize/validate names on the server. + +### 2) Stored XSS via dataload names in the dataload list +- Dataload names are written directly into the DOM with `innerHTML` after updates, allowing markup in the name field to execute on subsequent renders. 【F:js/dataset.js†L360-L388】 +- The update service stores the provided name without escaping, so injected HTML persists. 【F:lib/Service/DataloadService.php†L101-L110】 +- Impact: A user with dataload edit access can persistently inject scripts that execute for anyone viewing the dataset’s dataload tab, leading to session compromise or data theft. +- Suggested mitigation: write names with `textContent` and normalize/sanitize the name server-side before storage. + +## Recommendations +- Enforce output encoding in the front-end for any user-controlled text inserted into the DOM. +- Add server-side validation/encoding for display names (reports, datasets, dataloads) to prevent storing HTML/JS payloads. +- Add regression tests that cover the identified rendering paths to ensure unsafe HTML is neutralized. diff --git a/js/dataset.js b/js/dataset.js index c2d3142a..146b1ca1 100644 --- a/js/dataset.js +++ b/js/dataset.js @@ -384,7 +384,7 @@ Object.assign(OCA.Analytics.Dataset.Dataload = { OCA.Analytics.Dataset.Dataload.dataloadArray.find(x => x.id === dataloadId)['schedule'] = document.getElementById('dataloadSchedule').value; OCA.Analytics.Dataset.Dataload.dataloadArray.find(x => x.id === dataloadId)['name'] = document.getElementById('dataloadName').value; OCA.Analytics.Dataset.Dataload.dataloadArray.find(x => x.id === dataloadId)['option'] = option; - document.querySelector('[data-dataload-id="' + dataloadId + '"]').innerHTML = document.getElementById('dataloadName').value; + document.querySelector('[data-dataload-id="' + dataloadId + '"]').textContent = document.getElementById('dataloadName').value; }); }, @@ -615,12 +615,18 @@ Object.assign(OCA.Analytics.Dataset.Dataset = { .then(data => { document.getElementById('sidebarDatasetStatusRecords').innerText = parseInt(data['data']['count']).toLocaleString(); - let text = ''; - for (let report of data['reports']) { - text = text + '- ' + report['name'] + '
'; + const statusContainer = document.getElementById('sidebarDatasetStatusReports'); + statusContainer.innerHTML = ''; + + if (data['reports'].length === 0) { + statusContainer.textContent = t('analytics', 'This dataset is not used!'); + } else { + for (let report of data['reports']) { + const item = document.createElement('div'); + item.textContent = '- ' + report['name']; + statusContainer.appendChild(item); + } } - if (text === '') text = t('analytics', 'This dataset is not used!'); - document.getElementById('sidebarDatasetStatusReports').innerHTML = text; }); }, @@ -754,4 +760,4 @@ Object.assign(OCA.Analytics.Dataset.Dataset = { OCA.Analytics.Notification.notification('success', t('analytics', 'Saved')); }); }, -}); \ No newline at end of file +}); diff --git a/lib/Service/DataloadService.php b/lib/Service/DataloadService.php index 08e0f9ad..911a9052 100644 --- a/lib/Service/DataloadService.php +++ b/lib/Service/DataloadService.php @@ -100,6 +100,8 @@ public function read($datasetId) */ public function update(int $dataloadId, $name, $option, $schedule) { + $name = htmlspecialchars($name, ENT_NOQUOTES, 'UTF-8'); + $array = json_decode($option, true); foreach ($array as $key => $value) { $array[$key] = htmlspecialchars($value, ENT_NOQUOTES, 'UTF-8'); @@ -559,4 +561,4 @@ private function floatvalue($val) } } -} \ No newline at end of file +} diff --git a/lib/Service/ReportService.php b/lib/Service/ReportService.php index 3bb8ff82..6e83c863 100644 --- a/lib/Service/ReportService.php +++ b/lib/Service/ReportService.php @@ -185,6 +185,9 @@ public function create( $value, $addReport = null ): int { + $name = htmlspecialchars($name, ENT_NOQUOTES, 'UTF-8'); + $subheader = htmlspecialchars($subheader, ENT_NOQUOTES, 'UTF-8'); + $array = json_decode($link, true); if (is_array($array)) { foreach ($array as $key => $value) { @@ -222,6 +225,8 @@ public function create( public function createCopy(int $reportId, $chartoptions, $dataoptions, $filteroptions, $tableoptions) { $template = $this->ReportMapper->readOwn($reportId); + $template['name'] = htmlspecialchars($template['name'], ENT_NOQUOTES, 'UTF-8'); + $template['subheader'] = htmlspecialchars($template['subheader'], ENT_NOQUOTES, 'UTF-8'); $newId = $this->ReportMapper->create(// TRANSLATORS Noun $template['name'] . ' - ' . $this->l10n->t('copy'), $template['subheader'], $template['parent'], $template['type'], $template['dataset'], $template['link'], $template['visualization'], $template['chart'], $template['dimension1'], $template['dimension2'], $template['value']); $this->ReportMapper->updateOptions($newId, $chartoptions, $dataoptions, $filteroptions, $tableoptions); @@ -263,12 +268,12 @@ public function createFromDataFile($file = '') { } } - $name = explode('.', end(explode('/', $file)))[0]; - $subheader = $file; - $parent = 0; - $dataset = 0; - $type = DatasourceController::DATASET_TYPE_LOCAL_CSV; - $link = $file; + $name = htmlspecialchars(explode('.', end(explode('/', $file)))[0], ENT_NOQUOTES, 'UTF-8'); + $subheader = htmlspecialchars($file, ENT_NOQUOTES, 'UTF-8'); + $parent = 0; + $dataset = 0; + $type = DatasourceController::DATASET_TYPE_LOCAL_CSV; + $link = $file; $visualization = 'table'; $chart = 'line'; $reportId = $this->ReportMapper->create($name, $subheader, $parent, $type, $dataset, $link, $visualization, $chart, '', '', ''); @@ -308,6 +313,9 @@ public function update( $dimension2 = null, $value = null ) { + $name = htmlspecialchars($name, ENT_NOQUOTES, 'UTF-8'); + $subheader = htmlspecialchars($subheader, ENT_NOQUOTES, 'UTF-8'); + $array = json_decode($options, true); foreach ($array as $key => $tmpValue) { $array[$key] = htmlspecialchars($tmpValue, ENT_NOQUOTES, 'UTF-8'); @@ -555,6 +563,7 @@ public function updateGroup(int $reportId, $groupId) { */ public function rename(int $reportId, string $name) { if ($this->isOwn($reportId)) { + $name = htmlspecialchars($name, ENT_NOQUOTES, 'UTF-8'); return $this->ReportMapper->updateName($reportId, $name); } return false;