Skip to content

BUGFIX: Compare hosts when checking if target and source are the same #106

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robinroloff wants to merge 4 commits into
base: main
Choose a base branch
from
Open

BUGFIX: Compare hosts when checking if target and source are the same #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0050ae1
BUGFIX: Compare hosts when checking if target and source are the same
robinroloff Feb 3, 2026
348ab86
task: compare parsedSourceUrl.host instead of location host
robinroloff Feb 10, 2026
638c49b
task: compare parsedSourceUrl.host instead of location host
robinroloff Feb 10, 2026
88a8607
build js
robinroloff Feb 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions Classes/Controller/ModuleController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,12 +474,14 @@ protected function addRedirect(
$sourceUriPath = trim($sourceUriPath);
$targetUriPath = trim($targetUriPath);

$targetHost = parse_url($targetUriPath, PHP_URL_HOST);

if (!$this->validateRedirectAttributes($host, $sourceUriPath, $targetUriPath)) {
return [];
}

$redirect = $this->redirectStorage->getOneBySourceUriPathAndHost($sourceUriPath, $host ?: null, false);
$isSame = $this->isSame($sourceUriPath, $targetUriPath, $host, $statusCode, $redirect);
$isSame = $this->isSame($sourceUriPath, $targetUriPath, $targetHost, $statusCode, $redirect);
$go = true;

if ($redirect !== null && $isSame === false && $force === false) {
Expand All @@ -491,6 +493,7 @@ protected function addRedirect(
$go = false; // Ignore.. Not valid.
}


if ($go) {
$creator = $this->securityContext->getAccount()->getAccountIdentifier();

Expand All @@ -505,6 +508,7 @@ protected function addRedirect(
return [];
}


/**
* @param string $originalSourceUriPath
* @param string|null $originalHost
Expand Down Expand Up @@ -596,10 +600,10 @@ protected function validateRedirectAttributes(?string $host, string $sourceUriPa
}

protected function isSame(
string $sourceUriPath,
string $targetUriPath,
?string $host,
int $statusCode,
string $sourceUriPath,
string $targetUriPath,
?string $targetHost,
int $statusCode,
?RedirectInterface $redirect = null
): bool
{
Expand All @@ -609,7 +613,7 @@ protected function isSame(

return $redirect->getSourceUriPath() === $sourceUriPath
&& $redirect->getTargetUriPath() === $targetUriPath
&& $redirect->getHost() === $host
&& $redirect->getHost() === $targetHost
Copy link
Member

@Sebobo Sebobo Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host is used to determine whether a redirect is triggered in the middleware. So it has no relation to the target. IMO this part of the change is incorrect.
I think rather the targetUriPath should be checked twice in case one time it was given without host and one time with a host.

Copy link
Author

@robinroloff robinroloff Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. $redirect->getHost() === $host will always be true, so I'm not sure if it's the correct check in this case.

Can you tell me how you would've solved it?

Copy link
Member

@Sebobo Sebobo Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case the host comparison compares to equal, which is true and fine.

$targetUriPath can be a full url or just a path.
So you have to turn the existing and the new $targetUriPath into an absolute url for comparison and then check.

&& $redirect->getStatusCode() === $statusCode;
}

Expand Down
17 changes: 8 additions & 9 deletions Resources/Private/JavaScript/components/RedirectForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,15 +90,14 @@
// Replace a single asterisk with an empty value to match any domain
host = host && host.trim() === '*' ? '' : host;

if (!host || host === location.host) {
const parsedSourceUrl: URL = UrlUtil.parseURL(sourceUriPath, location.origin);
const parsedTargetUrl: URL = UrlUtil.parseURL(targetUriPath, location.origin);
if (parsedSourceUrl.pathname === parsedTargetUrl.pathname) {
notificationHelper.warning(
translate('error.sameSourceAndTarget', 'The source and target paths cannot be the same')
);
return;
}
const parsedSourceUrl: URL = UrlUtil.parseURL(sourceUriPath, location.origin);
const parsedTargetUrl: URL = UrlUtil.parseURL(targetUriPath, location.origin);

if ((!parsedSourceUrl.hostname || parsedSourceUrl.hostname === parsedTargetUrl.hostname) && parsedSourceUrl.pathname === parsedTargetUrl.pathname) {

Check failure on line 96 in Resources/Private/JavaScript/components/RedirectForm.tsx

View workflow job for this annotation

GitHub Actions / Neos CMS redirecthandler build 

Replace `(!parsedSourceUrl.hostname·||·parsedSourceUrl.hostname·===·parsedTargetUrl.hostname)·&&·parsedSourceUrl.pathname·===·parsedTargetUrl.pathname` with `⏎············(!parsedSourceUrl.hostname·||·parsedSourceUrl.hostname·===·parsedTargetUrl.hostname)·&&⏎············parsedSourceUrl.pathname·===·parsedTargetUrl.pathname⏎········`
notificationHelper.warning(
translate('error.sameSourceAndTarget', 'The source and target paths cannot be the same')
);
return;
}

const validStartDateTimeString =
Expand Down
Loading
Loading