-
Notifications
You must be signed in to change notification settings - Fork 451
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.
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
Feature main 10818 fix redirect loop #10822
base: main
Are you sure you want to change the base?
Feature main 10818 fix redirect loop #10822
Conversation
$multiLingual = count($this->_getContextAndLocales($request, $contextPath)[1]) > 1; | ||
|
||
if (!$multiLingual && !$urlLocale && !$setLocale || $multiLingual && !$setLocale && $urlLocale === Locale::getLocale()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified the common parts, but the initial intention was just to drop the requirement of knowing the operator precedence.
return; | ||
} | ||
|
||
$sessionLocale = (function (string $l) use ($request): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see a reason to create a closure here, so I extracted the code out of it and tried to simplify.
$indexUrl = $this->getIndexUrl($request); | ||
$uri = preg_replace("#^{$indexUrl}#", '', $setLocale ? ($_SERVER['HTTP_REFERER'] ?? '') : $request->getCompleteUrl(), 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the line 525 there's a str_replace('@', '', $url)
, was it used to do some kind of abuse (redirect user to another system and pass credentials)?
If yes, then the HTTP_REFERER
here should receive the same treatment, as it's also unsafe, depending on what was the issue, perhaps we should check if the destiny URL belongs to us (e.g. same domain/base path).
$indexUrl = $this->getIndexUrl($request); | ||
$uri = preg_replace("#^{$indexUrl}#", '', $setLocale ? ($_SERVER['HTTP_REFERER'] ?? '') : $request->getCompleteUrl(), 1); | ||
$newUrlLocale = $multiLingual ? "/{$sessionLocale}" : ''; | ||
$pathInfo = ($uri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem was here, when the $uri
is /
, then the code attempts to do the replace, nothing is replaced and we get into a loop.
Hi @jyhein! I think you were the last person to work on this part of the code, if you can do a review, great :) |
No description provided.