Implement XSS mitigations for report and dataload names

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

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.

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'] + '<br>';
+                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'));
             });
     },
-});
+});

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)
         }
     }
 
-}
+}

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;