Skip to content

Commit

Permalink
[BUGFIX] Mitigate misusing request 'id' as pages-uid in extbase BE
Browse files Browse the repository at this point in the history
The backend uses request GET/POST parameter 'id' as convention for
"pages uid" at various places: Especially the BackendModuleValidator
checks for this parameter early, to deny access to pages a BE user
has no access to.

This convention is a broken misuse: There is no such convention,
and for instance the filelist module uses 'id' to transfer a
selected "storage-uid:path".

The BackendModuleValidator mitigates this by calling
MU::canBeInterpretedAsInteger() before interpreting that
parameter as a pages-uid.

The extbase BackendConfigurationManager also uses 'id' to retrieve
the FE TypoScript configuration for this "pages-uid", it however
does not check with MU::canBeInterpretedAsInteger(), first.

The patch adds a MU::canBeInterpretedAsInteger() check to extbase
BackendConfigurationManager to be in-line with BackendModuleValidator,
and adds `@todo` comments outlining the general misuse of the argument.

Change-Id: I9b53a521bde4d3c145bfda2994d81dc4abf9c103
Resolves: #103540
Related: #96797
Releases: main, 12.4
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/83678
Reviewed-by: Christian Kuhn <lolli@schwarzbu.ch>
Tested-by: core-ci <typo3@b13.com>
Tested-by: Christian Kuhn <lolli@schwarzbu.ch>
  • Loading branch information
lolli42 committed Apr 7, 2024
1 parent b376629 commit 524267c
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 1 deletion.
Expand Up @@ -214,6 +214,11 @@ protected function validateModuleAccess(ServerRequestInterface $request, ModuleI
throw new ModuleAccessDeniedException('You don\'t have access to this module.', 1642450334);
}

// @todo: This misuses 'id' as a broken convention for pages-uid. The filelist module for instance
// uses 'id' as "storage-uid:path", which is only mitigated here by testing the argument
// with MU:canBeInterpretedAsInteger().
// Also see a similar misuse in extbase BackendConfigurationManager, which does this as well
// to guess a pages-uid for TypoScript retrieval.
$id = $request->getQueryParams()['id'] ?? $request->getParsedBody()['id'] ?? 0;
if (MathUtility::canBeInterpretedAsInteger($id) && $id > 0) {
$id = (int)$id;
Expand Down
Expand Up @@ -41,6 +41,7 @@
use TYPO3\CMS\Core\TypoScript\TypoScriptService;
use TYPO3\CMS\Core\Utility\ArrayUtility;
use TYPO3\CMS\Core\Utility\GeneralUtility;
use TYPO3\CMS\Core\Utility\MathUtility;
use TYPO3\CMS\Core\Utility\RootlineUtility;
use TYPO3\CMS\Extbase\Utility\FrontendSimulatorUtility;
use TYPO3\CMS\Frontend\ContentObject\ContentObjectRenderer;
Expand Down Expand Up @@ -377,7 +378,16 @@ protected function getCurrentPageId(?ServerRequestInterface $request = null): in
*/
protected function getCurrentPageIdFromRequest(ServerRequestInterface $request): int
{
return (int)($request->getParsedBody()['id'] ?? $request->getQueryParams()['id'] ?? 0);
// @todo: This misuses 'id' as a broken convention for pages-uid. The filelist module for instance
// uses 'id' as "storage-uid:path", which is only mitigated here by testing the argument
// with MU:canBeInterpretedAsInteger().
// This is in-line with a similar misuse in BackendModuleValidator.
$id = 0;
$potentialId = $request->getParsedBody()['id'] ?? $request->getQueryParams()['id'] ?? 0;
if (MathUtility::canBeInterpretedAsInteger($potentialId) && $potentialId > 0) {
$id = (int)$potentialId;
}
return $id;
}

/**
Expand Down

0 comments on commit 524267c

Please sign in to comment.