[aur-dev] [PATCH] Redirect back after login
Fixes FS#32481 --- web/html/login.php | 2 +- web/lib/acctfuncs.inc.php | 8 +++++++- web/lib/aur.inc.php | 44 ++++++++++++++++++++++++++++++++++++++++++++ web/template/header.php | 4 ++-- 4 files changed, 54 insertions(+), 4 deletions(-) diff --git a/web/html/login.php b/web/html/login.php index e458fec..48fda29 100644 --- a/web/html/login.php +++ b/web/html/login.php @@ -20,7 +20,7 @@ html_header('AUR ' . __("Login")); <a href="<?= get_uri('/logout/'); ?>">[<?= __("Logout"); ?>]</a> </p> <?php elseif (!$DISABLE_HTTP_LOGIN || (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'])): ?> - <form method="post" action="<?= get_uri('/login') ?>"> + <form method="post" action="<?= get_uri('/login') . redirect_post() ?>"> <fieldset> <legend><?= __('Enter login credentials') ?></legend> <?php if (!empty($login_error)): ?> diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index 3fd23ae..ee19511 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -460,7 +460,13 @@ function try_login($dbh=NULL) { $cookie_time = 0; setcookie("AURSID", $new_sid, $cookie_time, "/", null, !empty($_SERVER['HTTPS']), true); - header("Location: " . get_uri('/')); + + if (isset($_GET['redirect'])) { + header("Location: " . $_GET['redirect']); + } else { + header("Location: " . get_uri('/')); + } + $login_error = ""; } diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 018d5c8..653cf55 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -310,6 +310,50 @@ function html_header($title="") { } /** + * Add redirect URL parameter when appropriate + * @return string Query string + **/ +function redirect_string() { + global $USE_VIRTUAL_URLS; + + /* get the request URI without the optional query string */ + $uri_parts = explode('?', $_SERVER['REQUEST_URI']); + + /* remove leading slash if get_route() is used */ + if (!$USE_VIRTUAL_URLS) { + $uri_parts[0] = ltrim($uri_parts[0], '/'); + } + + /* don't add a redirect string to /login/ to prevent looping; + don't add a redirect string to / and /register/: it's useless */ + switch ($uri_parts[0]) { + case get_uri('/'): + case get_uri('/login/'): + case get_uri('/register/'): + $querystring = ''; + break; + default: + $querystring = '?redirect=' . urlencode($_SERVER["REQUEST_URI"]); + } + + return htmlentities($querystring); +} + +/** + * Add redirect URL parameter to form action + * @return string Query string + **/ +function redirect_post() { + if ( isset($_GET['redirect']) ) { + $querystring = '?redirect=' . urlencode($_GET['redirect']); + } else { + $querystring = ''; + } + + return htmlentities($querystring); +} + +/** * Common AUR footer displayed on all pages * * @param string $ver The AUR version diff --git a/web/template/header.php b/web/template/header.php index 92cb2ff..e073df5 100644 --- a/web/template/header.php +++ b/web/template/header.php @@ -64,9 +64,9 @@ <?php else: ?> <li><a href="<?= get_uri('/register/'); ?>"><?= __("Register"); ?></a></li> <?php if ($DISABLE_HTTP_LOGIN && empty($_SERVER['HTTPS'])): ?> - <li><a href="<?= $AUR_LOCATION . get_uri('/login/'); ?>"><?= __("Login"); ?></a></li> + <li><a href="<?= $AUR_LOCATION . get_uri('/login/') . redirect_string(); ?>"><?= __("Login"); ?></a></li> <?php else: ?> - <li><a href="<?= get_uri('/login/'); ?>"><?= __("Login"); ?></a></li> + <li><a href="<?= get_uri('/login/') . redirect_string(); ?>"><?= __("Login"); ?></a></li> <?php endif; ?> <?php endif; ?> </ul> -- 1.8.0.2
On Fri, Dec 14, 2012 at 5:51 PM, Marcel Korpel <marcel.lists@gmail.com> wrote:
This implementation is susceptible to HTTP header injection. Also note the usage of $_SERVER['REQUEST_URI'] had previously been eliminated with commit 630f1cbae8473fb05e5f5af7244eccc60fe93812.
On Sun, Dec 16, 2012 at 7:12 PM, canyonknight <canyonknight@gmail.com> wrote:
This implementation is susceptible to HTTP header injection.
Ok. You mean in the current 'Location:' line without filtering 0x0a and 0x0d?
If we can't trust $_SERVER['REQUEST_URI'], then how should we determine the current URL? Using $_SERVER['PATH_INFO'] and $_SERVER['QUERY_STRING']? Or are these also susceptible to manipulation? Regards, Marcel
On Mon, Dec 17, 2012 at 1:10 PM, Marcel Korpel <marcel.lists@gmail.com> wrote:
Response splitting shouldn't be an issue. PHP prevents multiple headers from being sent at once in the header() function. I was referring to the fact that it is an unsanitized $_GET variable being used as a header. It can be manipulated and could redirect to a website outside the AUR or some other clever attack. That is one of the nice things about using a $_SESSION variable in this case. The server could directly set the redirect location in a $_SESSION variable without the user being able to tamper with it.
Briefly, I always thought the following could be a decent solution: - User is on a page and the route is saved as a $_SESSION variable - User navigates to login page and logs in - Login page uses the routing code to redirect to page saved in the $_SESSION variable I realize it isn't a GET parameter solution, but it is easy to do securely. The only downside is if a user has multiple tabs open, it will redirect to the last page opened. That and to implement properly it would require a bit of work. Regards, Jason
participants (2)
-
canyonknight
-
Marcel Korpel