N°9392 - wizard helper linkset data validation improvement (#1100)

- fix cs fixer

AGENTS.md

@@ -0,0 +1,133 @@
+# AGENTS.md: iTop
+
+Instructions for AI coding agents and AI code reviewers working on this repository.
+iTop (by Combodo) is an open-source ITSM/CMDB web application: PHP backend, JS/HTML/SCSS front-end, XML datamodel.
+Write all code, identifiers, comments and review comments in English.
+
+## Critical rules (security first)
+
+These rules apply to every change. In code review, a violation is a P1 issue.
+
+### Input: sanitize with an explicit filter
+- Read request parameters only with `utils::ReadParam()` or `utils::ReadPostedParam()`, never from `$_GET`, `$_POST`, `$_REQUEST` or `$_COOKIE`.
+- Always pass the filter explicitly as a `utils::ENUM_SANITIZATION_FILTER_*` constant, the most restrictive that fits (`INTEGER`, `CLASS`, `FIELD_NAME`, `TRANSACTION_ID`, `URL`...).
+- `utils::ENUM_SANITIZATION_FILTER_RAW_DATA` requires a comment explaining why no other filter fits.
+
+### Output: escape at consumption time, in the right context
+- Sanitizing input is not escaping output. Escape where the value is rendered, never upstream in the backend, and never store escaped values.
+- PHP to HTML: `utils::EscapeHtml()`. Not `htmlentities()` nor `utils::HtmlEntities()` (they escape every non-ASCII char).
+- Twig: rely on auto-escaping; use `|escape('js')` in JS contexts and `|sanitize(constant('utils::ENUM_SANITIZATION_FILTER_VARIABLE_NAME'))` (or `..._ELEMENT_IDENTIFIER`) for generated identifiers. `|raw` only for trusted HTML, with a comment.
+- JS to DOM: escape untrusted values with `CombodoSanitizer.EscapeHtml()` before building HTML; prefer `.text()` to `.html()` or `innerHTML`.
+- User-provided rich HTML goes through `HTMLSanitizer::Sanitize()`.
+
+### CSRF
+- Never change state on a GET request.
+- A form that changes state embeds a transaction id from `utils::GetNewTransactionId()`; its handler calls `utils::IsTransactionValid()` before any modification.
+- Ajax calls use jQuery or `CombodoHTTP.Fetch()`, which send the `X-Combodo-Ajax` header checked by `pages/ajax.render.php`. Never use raw `fetch()` or `XMLHttpRequest`.
+
+### Data access and permissions
+- OQL: use placeholders (`WHERE name = :name`) and pass values in the `$aParams` argument of `DBObjectSearch::FromOQL()`. Never concatenate input into OQL or SQL; raw SQL values go through `CMDBSource::Quote()`.
+- Every new entry point (page, ajax operation, controller route, webservice) enforces authentication (`LoginWebPage::DoLogin()` / `DoLoginEx()`) and rights (`UserRights::IsActionAllowed()`, `UserRights::IsAdministrator()`...).
+- `AllowAllData()` bypasses user rights and scopes: only when strictly needed, with a comment.
+
+### Secrets and logs
+- Never log passwords, tokens, API keys, session ids or personal data. Log security events on `LogChannels::SECURITY`.
+
+### Security configuration parameters: secure by default
+- The default value of a configuration parameter is always the secure one.
+- An option that weakens a protection is opt-in: a `bool` named `security.disable_<protection>`, default `false`, whose description states the risk when set to `true` (model: `security.disable_inline_documents_sandbox` in `core/config.class.inc.php`). Do not add new `security.enable_*` parameters.
+
+## Project overview
+
+- Supported PHP versions: the `require.php` constraint in `composer.json`. Never use a language feature above its minimum version.
+- Branches: `develop` (next version), `support/x.y` (maintained versions).
+- Layout:
+    - `core/`: ORM, `MetaModel`, OQL, configuration (`core/config.class.inc.php`), logs. Legacy structure, meant to be replaced by `sources/` in the long term.
+    - `application/`: legacy UI layer, `utils` (`application/utils.inc.php`), exceptions (`application/exceptions/`). Legacy structure, meant to be replaced by `sources/` in the long term.
+    - `sources/`: namespaced code (`Combodo\iTop\...`, namespace matches the folder): services, controllers, UI blocks, forms.
+    - `pages/`: legacy entry points (`UI.php`, `ajax.render.php`).
+    - `datamodels/2.x/<module>/`: modules (`module.<module>.php`, `datamodel.<module>.xml`, `dictionaries/`).
+    - `dictionaries/`: core translations. `templates/`: Twig. `js/`: iTop JS. `css/`: SCSS and compiled CSS.
+    - `tests/`: PHPUnit (`php-unit-tests/`), PHP-CS-Fixer (`php-code-style/`), PHPStan (`php-static-analysis/`).
+- Never edit:
+    - `lib/` (Composer `vendor-dir`) and `node_modules/` (npm): third-party code.
+    - `env-*/`: generated by the datamodel compilation, not versioned.
+    - `data/`: temporary or cached files, not versioned.
+
+## Commands
+
+- Unit tests (needs an installed iTop instance, see `tests/php-unit-tests/README.md`): run `composer install` in `tests/php-unit-tests/`, then `tests/php-unit-tests/vendor/bin/phpunit --configuration tests/php-unit-tests/phpunit.xml.dist <path>`.
+- PHP code style: run `composer install` in `tests/php-code-style/`, then `tests/php-code-style/vendor/bin/php-cs-fixer fix --config tests/php-code-style/.php-cs-fixer.dist.php <path>`.
+- Static analysis: PHPStan, see `tests/php-static-analysis/README.md` (analyses `env-production`, needs a setup).
+- After adding, renaming or moving a PHP class in `core/`, `application/` or `sources/`: run `composer dump-autoload` (the classmap is authoritative: an unlisted class is not found) and commit `lib/composer/autoload_classmap.php` and `lib/composer/autoload_static.php`.
+- After changing SCSS: regenerate and commit the compiled CSS (the pre-commit hook rejects SCSS without CSS).
+- After changing datamodel XML: the datamodel must be recompiled by the iTop setup or toolkit. An agent cannot do it: tell the user.
+
+## Coding conventions
+
+### PHP
+- Formatting is enforced by PHP-CS-Fixer (PSR-12, tabs): do not hand-format.
+- Variables: mixed case with a type prefix: `$s` string, `$i` int, `$b` bool, `$a` array, `$o` object, `$f` float, no prefix for mixed types. Loop counters (`$i`, `$j`) are fine.
+- Functions and methods: UpperCamelCase starting with a verb (`GetInstance()`, `ReadParam()`). Classes: UpperCamelCase.
+- Array keys: snake_case, documented with PHPDoc array shapes (`array{key: type}`) or generics (`array<string, \CheckResult>`).
+- New code uses strict comparison (`===`). `==` only with a comment explaining why.
+- Test empty strings with `utils::IsNullOrEmptyString()`, not `empty()` (`"0"` is empty).
+- Type hints in new code only. Do not add them to existing signatures (it breaks extensions) nor return types to fluent methods.
+- PHPDoc: `@since` is mandatory on new classes and on new or changed methods and properties, e.g. `@since 3.3.0 N°1234 - Short description`. Use `bool`, not `boolean`.
+- Do not modify existing copyright headers.
+- Configuration parameters: lowercase, domain prefix, underscores (`email_transport_smtp.host`), declared in `core/config.class.inc.php`.
+
+### Services
+- A new service is a singleton: private constructor, `private static $oInstance`, `public static function GetInstance()`. Model: `sources/Service/Module/ModuleService.php`.
+
+### Exceptions and logs
+- Throw iTop exceptions (`CoreException` and its subclasses in `application/exceptions/`) rather than a bare `\Exception`.
+- Log a caught exception with `ExceptionLog::LogException($oException)`, not `IssueLog::Error($oException->getMessage())`.
+- Other logs: `IssueLog` with a `LogChannels::*` channel.
+- Never swallow an exception: an empty `catch` needs a comment explaining why.
+
+### JavaScript, Twig, SCSS
+- JS functions: UpperCamelCase starting with a verb. Follow the escaping and Ajax rules above.
+- Indentation follows `.editorconfig` (PHP, JS: tabs; SCSS, XML: 2 spaces; Twig: 4 spaces).
+
+### Translations
+- No hard-coded UI text: use `Dict::S()`, `Dict::Format()`, or `|dict_s` / `|dict_format` in Twig.
+- Add new entries in English in the `en` dictionary file (`dictionaries/en.dictionary.itop.*.php` or `datamodels/2.x/<module>/dictionaries/en.dict.<module>.php`).
+- Add new entries in other existing languages as well.
+- In other languages, a ~~ suffix marks an untranslated entry: never remove it without translating.
+
+### Commits
+- Start either with the ticket reference (`N°*`) or with one of the supported Gitmoji code (`:white_check_mark:`, `:green_heart:`, `:memo:`, `:bookmark:`), imperative present tense, first line 72 characters max, describe the functional change. More details in the body if needed.
+
+## Dependencies and licenses
+
+- Dependencies are versioned in the repository: Composer in `lib/`, npm in `node_modules/`. Change versions in `composer.json` / `package.json` and run Composer or npm; never edit the files directly.
+- Every new dependency must have an entry in `setup/licenses/community-licenses.xml`, with a license compatible with AGPL-3.0. For Composer packages, propose to the user to run `php .make/license/updateLicenses.php` (needs bash, composer and jq) and review its diff; npm packages and other bundled assets are added by hand.
+
+## Datamodel
+
+- The datamodel ("DM") is compiled from the XML of the modules (`datamodels/2.x/<module>/datamodel.<module>.xml`) and of installed extensions into `env-<environment>/` (usually `env-production/`), accessed at runtime through `MetaModel`.
+- Changing an existing class or attribute impacts every customer upgrade and their extensions (which alter the datamodel through XML deltas): treat it as high-impact and flag it.
+
+## Tests
+
+- Location: `tests/php-unit-tests/unitary-tests/`, mirroring the source path (tests for `application/foo.php` go in `unitary-tests/application/`). Cross-application data consistency: `integration-tests/`.
+- Naming: `MyClass` is tested by `MyClassTest`; `MyMethod()` by `testMyMethod()`.
+- Use the lightest base class that works:
+    1. `PHPUnit\Framework\TestCase`: pure logic.
+    2. `Combodo\iTop\Test\UnitTest\ItopTestCase`: iTop helpers, no datamodel.
+    3. `Combodo\iTop\Test\UnitTest\ItopDataTestCase`: starts the datamodel (`MetaModel`) and rolls back CRUD operations. Slow: only when the test needs the datamodel or iTop objects.
+    4. `Combodo\iTop\Test\UnitTest\ItopCustomDatamodelTestCase`: a non-standard datamodel.
+- Process isolation (`@runInSeparateProcess`, about 800 ms per `ItopDataTestCase` test) only when needed, on the test, never globally.
+- Do not leave static state behind (`ContextTag::AddContext()`, `EventService::RegisterListener()`, `Dict::Add()`): checked by `integration-tests/DetectStaticPollutionTest.php`.
+- `markTestSkipped()` only for a temporarily disabled test.
+- More details: `tests/php-unit-tests/README.md`.
+
+## Glossary
+
+- **DM / datamodel**: the application data model (classes, attributes, lifecycles, menus), compiled from XML and PHP into `env-*/`.
+- **Module**: a unit of iTop code under `datamodels/2.x/<module>/`. **Extension**: a packaged set of modules installed on top of iTop.
+- **OQL**: iTop's object query language.
+- **Transaction id**: iTop's CSRF token.
+- **Console / back-office**: the main UI for agents. **Portal**: the end-user UI (`datamodels/2.x/itop-portal-base/`).
+- **N°xxxx**: Combodo internal ticket number, used in commits and `@since`.

REVIEW.md

@@ -0,0 +1,30 @@
+# REVIEW.md: AI code review instructions for iTop
+
+For automated reviewers (GitHub Copilot code review, Greptile, ...). Project conventions and security rules are in `AGENTS.md`; this file says what to comment on and how.
+
+## Output
+- Write every comment in English.
+- Start each comment with its priority: `[P1-Security]`, `[P2-Regression]`, `[P2-Performance]`, `[P2-Architecture]` or `[P3]`.
+- Report every P1 and P2 issue. Report at most 3 `[P3]` comments per PR: keep the most useful ones.
+- Put test suggestions in a single `[Tests]` comment; it does not count toward the P3 limit.
+- If nothing is above P3, say so in one sentence. No praise, no file-by-file summary.
+
+## Priorities
+1. **P1 Security**: XSS, CSRF, OQL/SQL injection, missing authentication or rights check, sensitive data in logs, unsafe file or path handling, any breach of the "Critical rules" in `AGENTS.md`, a security option that is not secure by default or not named `security.disable_*`.
+2. **P2 Regression**: behavior change in an API extensions rely on (public classes and methods, extension interfaces, events, configuration parameters, REST/webservices); change to an existing datamodel class or attribute; removed or renamed dictionary entry.
+3. **P2 Performance**: queries inside loops, loading whole object sets when a count or a subset is enough, loading all attributes of an object in a loop instead of calling `\DBObjectSet::OptimizeColumnLoad`, repeated expensive calls.
+4. **P2 Architecture**: logic in the wrong layer, a new service that is not a singleton, a new core production PHP class outside of the `sources/` folder (excluding test classes and module-specific code), a new PHP class in `core/`, `application/` or `sources/` without the updated `lib/composer/autoload_classmap.php`, a new dependency missing from `setup/licenses/community-licenses.xml`, SCSS changed without the compiled CSS.
+5. **P3 Other**: unclear names (variables, functions, classes), deviations from `AGENTS.md` conventions, typos and grammar in identifiers, comments and dictionary entries, UI labels that could be clearer or more consistent with existing ones.
+
+## Tests
+- When the PR changes behavior, propose the missing test cases: what to assert, in which file of `tests/php-unit-tests/unitary-tests/`.
+- Propose missing test cases to ensure maximum coverage of the new code, and that it is not broken by future changes.
+- Check readability, and that the base class is the lightest that works: `ItopDataTestCase` only if the test needs the datamodel. Flag unjustified process isolation and static state left behind.
+
+## Do not comment on
+- Formatting: indentation, spacing, line breaks, brace placement. PHP-CS-Fixer and `.editorconfig` handle it.
+- Third-party code in `lib/` and `node_modules/`, and generated code in `env-*/`. Only check whether `lib/composer/autoload_*.php` should have changed.
+- Code the PR does not touch, unless the change breaks it or it needs to be adjusted accordingly.
+
+## Vulnerability fixes
+This repository is public. If a change seems to fix an existing vulnerability, never describe the vulnerability, how to exploit it, or which versions are affected: neither in comments nor in the PR summary. Review it as an ordinary change. If the fix looks incomplete, describe only the missing check in neutral technical terms (for example "this value is not escaped here").

application/wizardhelper.class.inc.php

@@ -35,10 +35,14 @@ class WizardHelper
 	public function __construct()
 	{
 	}
+
 	/**
 	 * Constructs the PHP target object from the parameters sent to the web page by the wizard
+	 *
 	 * @param boolean $bReadUploadedFiles True to also read any uploaded file (for blob/document fields)
+	 *
 	 * @return object
+	 * @throws CoreException
 	 */
 	public function GetTargetObject($bReadUploadedFiles = false)
 	{
@@ -57,7 +61,14 @@ class WizardHelper
 					// assumes this is handled as an array of objects
 					// thus encoded in json like: [ { name:'link1', 'id': 123}, { name:'link2', 'id': 124}...]
 					if (!is_array($value)) {
-						$aData = json_decode($value, true); // true means decode as a hash array (not an object)
+						try {
+							$aData = json_decode($value, associative: true, flags: JSON_THROW_ON_ERROR); // true means decode as a hash array (not an object)
+						} catch (JsonException $e) {
+							throw new CoreException('WizardHelper GetTargetObject: invalid data for linked set attribute '.$sAttCode.' of class '.$this->m_aData['m_sClass'], [
+								'att_code' => $sAttCode,
+								'linked_class' => $this->m_aData['m_sClass'],
+							], 'Invalid JSON data', $e);
+						}
 					} else {
 						$aData = $value;
 					}
@@ -66,9 +77,6 @@ class WizardHelper
 					$aFields = $this->GetLinkedWizardStructure($oAttDef);
 					$sLinkedClass = $oAttDef->GetLinkedClass();
 					$aLinkedObjectsArray = [];
-					if (!is_array($aData)) {
-						echo("aData: '$aData' (value: '$value')\n");
-					}
 					foreach ($aData as $aLinkedObject) {
 						$oLinkedObj = MetaModel::NewObject($sLinkedClass);
 						foreach ($aFields as $sLinkedAttCode) {

datamodels/2.x/itop-backup/ajax.backup.php

@@ -29,6 +29,24 @@ require_once(APPROOT.'/application/application.inc.php');
 
 require_once(APPROOT.'core/mutex.class.inc.php');
 
+const BACKUP_DIR = APPROOT.'data/backups/';
+
+class AjaxBackupException extends CoreException
+{
+	public function __construct(array $aContextData = null, Exception $oPreviousException = null)
+	{
+		parent::__construct('Backup/Restore exception occurred', $aContextData, oPrevious: $oPreviousException);
+	}
+}
+
+class AjaxBackupInvalidFileException extends AjaxBackupException
+{
+	public function __construct(array $aContextData = null, Exception $oPreviousException = null)
+	{
+		parent::__construct($aContextData, oPreviousException: $oPreviousException);
+	}
+}
+
 /**
  * @param WebPage $oPage
  * @param string $sHtmlErrorMessage the whole HTML error, cinluding div/p/...
@@ -51,73 +69,79 @@ $sOperation = utils::ReadParam('operation', '');
 $oPage = new AjaxPage('');
 $oPage->SetContentType('text/html');
 
-/**
- * Check security
- */
-switch ($sOperation) {
-	/**
-	 * Can't use normal check methods (DoLogin for ex) as the datamodel can't be loaded here
-	 * So we're only using a token generated in the restore_token operation
-	 */
-	case 'restore_exec':
-		IssueLog::Enable(APPROOT.'log/error.log');
-		if (utils::GetConfig()->Get('demo_mode')) {
-			DisplayErrorAndDie(
-				$oPage,
-				'<div data-error-stimulus="Error">Sorry, '.ITOP_APPLICATION_SHORT.' is in <b>demonstration mode</b>: the feature is disabled.</div>'
-			);
-		}
-
-		$sToken = utils::ReadParam('token', '', false, 'raw_data');
-		$sBasePath = APPROOT.'/data/';
-		$sTokenFile = $sBasePath.'restore.'.$sToken.'.tok';
-		$tokenRealPath = utils::RealPath($sTokenFile, $sBasePath);
-		if (($tokenRealPath === false) || (!is_file($tokenRealPath))) {
-			IssueLog::Error("ajax.backup.php operation=$sOperation ERROR = inexisting token $sToken");
-			$sEscapedToken = utils::HtmlEntities($sToken);
-			DisplayErrorAndDie($oPage, "<p>Error: missing token file: '$sEscapedToken'</p>");
-		}
-
-		break;
-
-	default:
-		require_once(APPROOT.'/application/startup.inc.php');
-		require_once(APPROOT.'/application/loginwebpage.class.inc.php');
-
-		LoginWebPage::DoLogin();
-
-		$sTransactionId = utils::ReadParam('transaction_id', '', true, 'transaction_id');
-		// the consumer page is not reloaded after download, we need to keep the transaction_id
-		$bRemoveTransactionId = ($sOperation !== 'download');
-		if (!utils::IsTransactionValid($sTransactionId, $bRemoveTransactionId)) {
-			$sEscapedOperation = utils::HtmlEntities($sOperation);
-			DisplayErrorAndDie($oPage, "<div data-error-stimulus=\"Error\">Error: invalid Transaction ID. The operation '$sEscapedOperation' was NOT performed!</div>");
-		}
-
-		ApplicationMenu::CheckMenuIdEnabled('BackupStatus');
-
-		if (utils::GetConfig()->Get('demo_mode')) {
-			DisplayErrorAndDie($oPage, '<div data-error-stimulus="Error">Sorry, '.ITOP_APPLICATION_SHORT.' is in <b>demonstration mode</b>: the feature is disabled.</div>');
-		}
-		break;
-}
-
-/**
- * Backup from an interactive session
- */
 try {
+
+	/**
+	 * Check security
+	 */
+	switch ($sOperation) {
+		/**
+		 * Can't use normal check methods (DoLogin for ex) as the datamodel can't be loaded here
+		 * So we're only using a token generated in the restore_token operation
+		 */
+		case 'restore_exec':
+			IssueLog::Enable(APPROOT.'log/error.log');
+			if (utils::GetConfig()->Get('demo_mode')) {
+				DisplayErrorAndDie(
+					$oPage,
+					'<div data-error-stimulus="Error">Sorry, '.ITOP_APPLICATION_SHORT.' is in <b>demonstration mode</b>: the feature is disabled.</div>'
+				);
+			}
+
+			$sToken = utils::ReadParam('token', '', false, 'raw_data');
+			$sBasePath = APPROOT.'/data/';
+			$sTokenFile = $sBasePath.'restore.'.$sToken.'.tok';
+			$tokenRealPath = utils::RealPath($sTokenFile, $sBasePath);
+			if (($tokenRealPath === false) || (!is_file($tokenRealPath))) {
+				throw new AjaxBackupInvalidFileException([
+					'backup_operation' => $sOperation,
+					'message'          => 'Could not restore backup, invalid token file.',
+					'token'            => $sToken,
+				]);
+			}
+			break;
+
+		default:
+			require_once(APPROOT.'/application/startup.inc.php');
+			require_once(APPROOT.'/application/loginwebpage.class.inc.php');
+
+			LoginWebPage::DoLogin();
+
+			$sTransactionId = utils::ReadParam('transaction_id', '', true, 'transaction_id');
+			// the consumer page is not reloaded after download, we need to keep the transaction_id
+			$bRemoveTransactionId = ($sOperation !== 'download');
+			if (!utils::IsTransactionValid($sTransactionId, $bRemoveTransactionId)) {
+				throw new AjaxBackupException([
+					'backup_operation' => $sOperation,
+					'message'          => 'Invalid transaction id',
+					'transaction_id'   => $sTransactionId,
+				]);
+			}
+
+			ApplicationMenu::CheckMenuIdEnabled('BackupStatus');
+
+			if (utils::GetConfig()->Get('demo_mode')) {
+				DisplayErrorAndDie($oPage, '<div data-error-stimulus="Error">Sorry, '.ITOP_APPLICATION_SHORT.' is in <b>demonstration mode</b>: the feature is disabled.</div>');
+			}
+			break;
+	}
+
+	/**
+	 * Backup from an interactive session
+	 */
 	switch ($sOperation) {
 		case 'backup':
 			try {
 				set_time_limit(0);
 				$oBB = new BackupExec(APPROOT.'data/backups/manual/', 0 /*iRetentionCount*/);
 				$sRes = $oBB->Process(time() + 36000); // 10 hours to complete should be sufficient!
+				$oPage->output();
 			} catch (Exception $e) {
-				$oPage->p('Error: '.$e->getMessage());
-				IssueLog::Error($sOperation.' - '.$e->getMessage());
+				throw new AjaxBackupException([
+					'backup_operation' => $sOperation,
+					'message'          => $e->getMessage(),
+				], $e);
 			}
-
-			$oPage->output();
 			break;
 
 			/*
@@ -138,6 +162,15 @@ try {
 			}
 
 			$sFile = utils::ReadParam('file', '', false, 'raw_data');
+			$sBackupFilePath = utils::RealPath(BACKUP_DIR.$sFile, BACKUP_DIR);
+			if ($sBackupFilePath === false) {
+				throw new AjaxBackupInvalidFileException([
+					'backup_operation' => $sOperation,
+					'message'          => 'Could not restore backup, invalid file given.',
+					'backup_file'      => $sFile,
+				]);
+			}
+
 			$sToken = str_replace(' ', '', (string)microtime()).$sTransactionId;
 			$sTokenFile = APPROOT.'/data/restore.'.$sToken.'.tok';
 			file_put_contents($sTokenFile, $sFile);
@@ -171,14 +204,15 @@ try {
 				$oDBRS = new DBRestore($oItopConfig);
 				$oDBRS->SetMySQLBinDir($sMySQLBinDir);
 
-				$sBackupDir = APPROOT.'data/backups/';
-				$sBackupFile = $sBackupDir.$sFile;
+				$sBackupFile = BACKUP_DIR.$sFile;
 				$sRes = $oDBRS->RestoreFromCompressedBackup($sBackupFile, $sEnvironment);
 
 				IssueLog::Info('Backup Restore - Done, releasing the LOCK');
 			} catch (Exception $e) {
-				$oPage->p('Error: '.$e->getMessage());
-				IssueLog::Error($sOperation.' - '.$e->getMessage());
+				throw new AjaxBackupException([
+					'backup_operation' => $sOperation,
+					'message'          => $e->getMessage(),
+				], $e);
 			} finally {
 				unlink($tokenRealPath);
 			}
@@ -192,14 +226,33 @@ try {
 			}
 			$sFile = utils::ReadParam('file', '', false, 'raw_data');
 			$oBackup = new DBBackupScheduled();
-			$sBackupDir = APPROOT.'data/backups/';
-			$sBackupFilePath = utils::RealPath($sBackupDir.$sFile, $sBackupDir);
+			$sBackupFilePath = utils::RealPath(BACKUP_DIR.$sFile, BACKUP_DIR);
 			if ($sBackupFilePath === false) {
-				throw new CoreUnexpectedValue('Invalid file path');
+				throw new AjaxBackupInvalidFileException([
+					'backup_operation' => $sOperation,
+					'message'          => 'Could not download backup, invalid file given.',
+					'backup_file'      => $sFile,
+				]);
 			}
 			$oBackup->DownloadBackup($sBackupFilePath);
 			break;
 	}
+} catch (AjaxBackupInvalidFileException $e) {
+	IssueLog::Warning('Unable to perform backup `'.$e->getContextData()['backup_operation'].'` : '.$e->getContextData()['message'], LogChannels::SECURITY, [
+		'backup_operation' => $e->getContextData()['backup_operation'] ?? null,
+		'backup_file'      => $e->getContextData()['backup_file'] ?? null,
+	]);
+	http_response_code(422);
+	die($e->getContextData()['message']);
+} catch (CoreException $e) {
+	IssueLog::Error('Unable to perform backup `'.$e->getContextData()['backup_operation'].'` : '.$e->getContextData()['message'], LogChannels::BACKUP, [
+		'backup_operation' => $e->getContextData()['backup_operation'] ?? null,
+		'backup_file'      => $e->getContextData()['backup_file'] ?? null,
+	]);
+	http_response_code(500);
+	die(Dict::S('UI:PageTitle:FatalError'));
 } catch (Exception $e) {
-	IssueLog::Error($sOperation.' - '.$e->getMessage());
+	IssueLog::Error($e->getMessage());
+	http_response_code(500);
+	die(Dict::S('UI:PageTitle:FatalError'));
 }

datamodels/2.x/itop-backup/status.php

@@ -443,6 +443,13 @@ function LaunchBackupNow()
 				window.location.reload();
 			}
 			oModal.dialog('close');
+		}).fail(function(xhr){
+			var sErrorMessage = xhr.status === 500
+				? (xhr.responseText || 'Erreur serveur (HTTP 500).')
+				: (xhr.responseText || 'Erreur HTTP ' + xhr.status + '.');
+			$('#backup_errors').text(sErrorMessage);
+			$('#backup_errors').removeClass('ibo-is-hidden');
+			oModal.dialog('close');
 		});
 	}
 }
@@ -487,11 +494,25 @@ function LaunchRestoreNow(sBackupFile, sConfirmationMessage)
 					$('#backup_success').removeClass('ibo-is-hidden');
 				}
 				oModal.dialog('close');
+			}).fail(function(xhr){
+				var sErrorMessage = xhr.status === 500
+					? (xhr.responseText || 'Erreur serveur (HTTP 500).')
+					: (xhr.responseText || 'Erreur HTTP ' + xhr.status + '.');
+				$('#backup_errors').text(sErrorMessage);
+				$('#backup_errors').removeClass('ibo-is-hidden');
+				oModal.dialog('close');
 			});
 		} else {
 			$('button.restore').prop('disabled', true);
 			oModal.dialog('close');
 		}
+	}).fail(function(xhr){
+		var sErrorMessage = xhr.status === 500
+			? (xhr.responseText || 'Erreur serveur (HTTP 500).')
+			: (xhr.responseText || 'Erreur HTTP ' + xhr.status + '.');
+		$('#backup_errors').text(sErrorMessage);
+		$('#backup_errors').removeClass('ibo-is-hidden');
+		oModal.dialog('close');
 	});
 }
 JS

sources/Controller/Links/LinkSetController.php

@@ -17,11 +17,9 @@ use Combodo\iTop\Service\Base\ObjectRepository;
 use Combodo\iTop\Service\Router\Router;
 use CoreException;
 use DBObject;
-use DBObjectSet;
 use Dict;
 use Exception;
 use MetaModel;
-use SecurityException;
 use UserRights;
 use utils;
 
@@ -60,11 +58,7 @@ class LinkSetController extends AbstractController
 		// check transaction id
 		if (utils::IsTransactionValid($sTransactionId, false)) {
 			try {
-				$oLinkedObject = MetaModel::GetObject($sLinkedObjectClass, $sLinkedObjectObjectKey);
-				if (UserRights::IsActionAllowed(get_class($oLinkedObject), UR_ACTION_DELETE, DBObjectSet::FromObject($oLinkedObject)) !== UR_ALLOWED_YES) {
-					throw new SecurityException('You are not allowed to delete this object.');
-				}
-				$oDeletionPlan = $oLinkedObject->DBDelete();
+				$oDeletionPlan = MetaModel::GetObject($sLinkedObjectClass, $sLinkedObjectObjectKey)->DBDelete();
 				$bOperationSuccess = (count($oDeletionPlan->GetIssues()) === 0);
 				if (!$bOperationSuccess) {
 					$sErrorMessage = json_encode($oDeletionPlan->GetIssues());
@@ -109,9 +103,6 @@ class LinkSetController extends AbstractController
 		if (utils::IsTransactionValid($sTransactionId, false)) {
 			try {
 				$oLinkedObject = MetaModel::GetObject($sLinkedObjectClass, $sLinkedObjectKey);
-				if (UserRights::IsActionAllowed($sLinkedObjectClass, UR_ACTION_MODIFY, DBObjectSet::FromObject($oLinkedObject)) !== UR_ALLOWED_YES) {
-					throw new SecurityException('User not allowed to modify this object');
-				}
 				$oLinkedObject->Set($sExternalKeyAttCode, null);
 				$oLinkedObject->DBWrite();
 				$bOperationSuccess = true;

tests/php-unit-tests/src/BaseTestCase/ItopTestCase.php

@@ -722,4 +722,16 @@ abstract class ItopTestCase extends KernelTestCase
 
 		return $this->CallUrl($sUrl, $aPostFields, $aCurlOptions, $bXDebugEnabled);
 	}
+
+	/**
+	 * @since 3.2.4 3.3.0
+	 * @return string Temporary file path, that will be cleaned up by tearDown()
+	 */
+	public function GetTemporaryFilePath(string $sPrefix = "test"): string
+	{
+
+		$sPath = tempnam(sys_get_temp_dir(), $sPrefix);
+		$this->aFileToClean[] = $sPath;
+		return $sPath;
+	}
 }

tests/php-unit-tests/unitary-tests/application/wizardHelperTest.php

@@ -0,0 +1,44 @@
+<?php
+
+/**
+ * Copyright (C) 2018 Dennis Lassiter
+ *
+ * This file is part of iTop.
+ *
+ *  iTop is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * iTop is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with iTop. If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace Combodo\iTop\Test\UnitTest\Application;
+
+use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
+use WizardHelper;
+use CoreException;
+
+/**
+ * @covers WizardHelper
+ */
+class wizardHelperTest extends ItopDataTestCase
+{
+	public function testGetTargetObjectWithInvalidLinkSetDataValue()
+	{
+		// test case from N°9392 - [SECU] Stored XSS in subnets_list Parameter
+		$sWizardHelperJSON = '{"m_sClass":"VLAN","m_oFieldsMap":{"vlan_tag":"2_vlan_tag","org_id":"2_org_id","description":"2_description","subnets_list":"2_subnets_list","physicalinterfaces_list":"2_physicalinterfaces_list"},"m_oCurrentValues":{"vlan_tag":null,"org_id":null,"description":null,"subnets_list":"[]oszzt<img src=a onerror=alert(1)>l053a","physicalinterfaces_list":"[]"},"m_aDefaultValueRequested":[],"m_aAllowedValuesRequested":[],"m_oDefaultValue":{},"m_oAllowedValues":{},"m_aStaticValues":[],"m_iFieldsCount":5,"m_sFormPrefix":"","m_sState":"","m_bReturnNotEditableFields":false,"m_sWizHelperJsVarName":null}';
+		$this->expectException(CoreException::class);
+		$this->expectExceptionMessageMatches('/^WizardHelper GetTargetObject: invalid data for linked set attribute/');
+		$oWizardHelper = WizardHelper::FromJSON($sWizardHelperJSON);
+		$oWizardHelper->GetTargetObject();
+	}
+
+}

tests/php-unit-tests/unitary-tests/datamodels/2.x/itop-backup/BackupAjaxEndpointTest.php

@@ -0,0 +1,121 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Combodo\iTop\Test\UnitTest\Module\ItopBackup;
+
+use Combodo\iTop\Test\UnitTest\ItopDataTestCase;
+use MetaModel;
+use UserRights;
+use utils;
+
+class BackupAjaxEndpointTest extends ItopDataTestCase
+{
+	public const  USE_TRANSACTION         = false;
+	private const AUTHENTICATION_PASSWORD = 'tagada-Secret,007';
+
+	protected function setUp(): void
+	{
+		parent::setUp();
+
+		$this->BackupConfiguration();
+		$this->AddLoginModeAndSaveConfiguration('url');
+	}
+
+	public function testDownloadInvalidDownloadPathReturnsHttp500(): void
+	{
+		$sLogin = 'test-backup-endpoint-'.uniqid();
+		$this->CreateUser($sLogin, self::$aURP_Profiles['Administrator'], self::AUTHENTICATION_PASSWORD);
+
+		UserRights::Login($sLogin);
+
+		$sData = $this->CallItopUri(
+			'env-production/itop-backup/ajax.backup.php?'.http_build_query([
+				'operation'      => 'download',
+				'file'           => 'missing-'.uniqid().'.zip',
+				'auth_user'      => $sLogin,
+				'auth_pwd'       => self::AUTHENTICATION_PASSWORD,
+				'transaction_id' => utils::GetNewTransactionId(),
+			]),
+			[],
+			[
+				CURLOPT_HTTPHEADER => ['X-Combodo-Ajax:1'],
+				CURLOPT_POST       => 0,
+			],
+			true
+		);
+
+		$this->assertSame(422, $this->aLastCurlGetInfo['http_code'], __METHOD__.':'.$sData);
+	}
+
+	public function testRestoreGetTokenInvalidTransactionIdReturnsHttp500(): void
+	{
+		$sLogin = 'test-backup-endpoint-'.uniqid();
+		$this->CreateUser($sLogin, self::$aURP_Profiles['Administrator'], self::AUTHENTICATION_PASSWORD);
+
+		UserRights::Login($sLogin);
+
+		$sData = $this->CallItopUri(
+			'env-production/itop-backup/ajax.backup.php?'.http_build_query([
+				'operation'      => 'restore_get_token',
+				'auth_user'      => $sLogin,
+				'auth_pwd'       => self::AUTHENTICATION_PASSWORD,
+				'transaction_id' => 'unknown-transaction-id-'.uniqid(),
+			]),
+			[],
+			[
+				CURLOPT_HTTPHEADER => ['X-Combodo-Ajax:1'],
+				CURLOPT_POST       => 0,
+			],
+			true
+		);
+
+		$this->assertSame(500, $this->aLastCurlGetInfo['http_code'], __METHOD__.':'.$sData);
+	}
+
+	public function testRestoreGetToken(): void
+	{
+		$sLogin = 'test-backup-endpoint-'.uniqid();
+		$this->CreateUser($sLogin, self::$aURP_Profiles['Administrator'], self::AUTHENTICATION_PASSWORD);
+
+		UserRights::Login($sLogin);
+
+		$sBackupDirectory = APPROOT.'data/backups/';
+		if (!is_dir($sBackupDirectory)) {
+			$this->assertTrue(mkdir($sBackupDirectory, 0770, true) || is_dir($sBackupDirectory));
+		}
+		$sBackupFile = 'backup-'.uniqid().'.zip';
+		$sBackupPath = $sBackupDirectory.$sBackupFile;
+		$oArchive = new \ZipArchive();
+
+		$bArchiveOpen = $oArchive->open($sBackupPath, \ZipArchive::CREATE | \ZipArchive::OVERWRITE);
+		$this->assertTrue($bArchiveOpen);
+		$this->assertTrue($oArchive->addFromString('itop-dump.sql', '-- Test backup'));
+		$sConfigContents = file_get_contents(MetaModel::GetConfig()->GetLoadedFile());
+		$this->assertNotFalse($sConfigContents);
+		$this->assertTrue($oArchive->addFromString('config-itop.php', $sConfigContents));
+		$this->assertTrue($oArchive->close());
+		$this->aFileToClean[] = $sBackupPath;
+
+		$sData = $this->CallItopUri(
+			'env-production/itop-backup/ajax.backup.php?'.http_build_query([
+				'operation'      => 'restore_get_token',
+				'auth_user'      => $sLogin,
+				'auth_pwd'       => self::AUTHENTICATION_PASSWORD,
+				'file'           => $sBackupFile,
+				'transaction_id' => utils::GetNewTransactionId(),
+			]),
+			[],
+			[
+				CURLOPT_HTTPHEADER => ['X-Combodo-Ajax:1'],
+				CURLOPT_POST       => 0,
+			],
+			true
+		);
+
+		$aData = json_decode($sData, true);
+		$this->assertSame(200, $this->aLastCurlGetInfo['http_code'], __METHOD__.':'.$sData);
+		$this->assertArrayHasKey('token', $aData);
+
+	}
+}