Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
52 changes: 28 additions & 24 deletions classes/core/PKPPageRouter.php
Original file line number Diff line number Diff line change
Expand Up @@ -500,40 +500,44 @@ private function _getLocaleForUrl(PKPRequest $request, ?Context $context, array
*/
private function _setLocale(PKPRequest $request, ?string $setLocale): void
{
$contextPath = $this->_getRequestedUrlParts(['Core', 'getContextPath'], $request);
$urlLocale = $this->_getRequestedUrlParts(['Core', 'getLocalization'], $request);
$contextPath = $this->_getRequestedUrlParts(Core::getContextPath(...), $request);
$urlLocale = $this->_getRequestedUrlParts(Core::getLocalization(...), $request);
$multiLingual = count($this->_getContextAndLocales($request, $contextPath)[1]) > 1;

if (!$multiLingual && !$urlLocale && !$setLocale || $multiLingual && !$setLocale && $urlLocale === Locale::getLocale()) {
Copy link
Contributor Author

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.

// Quit if there's no new locale to be set and the request URL is already well-formed
if (!$setLocale && ($multiLingual ? $urlLocale === Locale::getLocale() : !$urlLocale)) {
return;
}

$sessionLocale = (function (string $l) use ($request): string {
Copy link
Contributor Author

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.

$session = $request->getSession();
if (Locale::isSupported($l) && $l !== $session->get('currentLocale')) {
$session->put('currentLocale', $l);
$request->setCookieVar('currentLocale', $l);
}
// In case session current locale has been set to non-supported locale, or is null, somewhere else
if (!Locale::isSupported($session->get('currentLocale') ?? '')) {
$session->put('currentLocale', Locale::getLocale());
$request->setCookieVar('currentLocale', Locale::getLocale());
}
return $session->get('currentLocale');
})($setLocale ?? $urlLocale);
$session = $request->getSession();
$currentLocale = $session->get('currentLocale') ?? '';
if (!Locale::isSupported($currentLocale ?? '')) {
$currentLocale = null;
}

$newLocale = $setLocale ?? $urlLocale;
if (!Locale::isSupported($newLocale)) {
$newLocale = $currentLocale ?? Locale::getLocale();
}

if ($newLocale !== $currentLocale) {
$session->put('currentLocale', $newLocale);
$request->setCookieVar('currentLocale', $newLocale);
}

// Do not permit basic auth strings (user:password@url) in source parameter for redirects
if (preg_match('#^/\w#', $source = str_replace('@', '', $request->getUserVar('source') ?? ''))) {
$request->redirectUrl($source);
}

$newUrlLocale = $multiLingual ? "/{$newLocale}" : '';
$indexUrl = $this->getIndexUrl($request);
$uri = preg_replace("#^{$indexUrl}#", '', $setLocale ? ($_SERVER['HTTP_REFERER'] ?? '') : $request->getCompleteUrl(), 1);
Copy link
Contributor Author

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).

$newUrlLocale = $multiLingual ? "/{$sessionLocale}" : '';
$pathInfo = ($uri)
Copy link
Contributor Author

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.

? preg_replace("#^/{$contextPath}" . ($urlLocale ? "/{$urlLocale}" : '') . '(?=[/?\\#]|$)#', "/{$contextPath}{$newUrlLocale}", $uri, 1)
: "/index{$newUrlLocale}";

$request->redirectUrl($indexUrl . $pathInfo);
$pathInfo = preg_replace('/^' . preg_quote($indexUrl, '/') . '/', '', $setLocale ? ($_SERVER['HTTP_REFERER'] ?? '') : $request->getCompleteUrl(), 1);
$newPathInfo = preg_replace('/^' . preg_quote("/{$contextPath}" . ($urlLocale ? "/{$urlLocale}" : ''), '/') . '(?=[\\/?#])\b/', "/{$contextPath}{$newUrlLocale}", $pathInfo, 1, $replaceCount);
// Failed to setup the new URL, fallback to the default initial URL
if (!$replaceCount) {
$newPathInfo = "/index{$newUrlLocale}";
}
$request->redirectUrl($indexUrl . $newPathInfo);
}
}

Expand Down