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 1 commit
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 @@ export class RedirectForm extends PureComponent<RedirectFormProps, RedirectFormS
// 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 ((!host || host === parsedTargetUrl.host) && parsedSourceUrl.pathname === parsedTargetUrl.pathname) {
Copy link
Copy Markdown
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.

Shouldn't it be instead:

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

Copy link
Copy Markdown
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.

You're right, this probably got lost when I rebased something. Changed it

notificationHelper.warning(
translate('error.sameSourceAndTarget', 'The source and target paths cannot be the same')
);
return;
}

const validStartDateTimeString =
Expand Down
2 changes: 1 addition & 1 deletion Resources/Public/Assets/main.bundle.js
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Resources/Public/Assets/main.bundle.js.map
View file
Open in desktop

Large diffs are not rendered by default.

Loading