[aur-dev] [PATCH 1/3] Use username from the database if one is provided by the user
This fixes a bug where the new user name input by the user was invalid, causing the account deletion link and the form action to be wrong. Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- web/html/account.php | 2 +- web/lib/acctfuncs.inc.php | 29 ++++++++++++++++++++++++++--- web/template/account_edit_form.php | 4 ++-- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/web/html/account.php b/web/html/account.php index c447de3..8545478 100644 --- a/web/html/account.php +++ b/web/html/account.php @@ -100,7 +100,7 @@ if (isset($_COOKIE["AURSID"])) { in_request("E"), in_request("P"), in_request("C"), in_request("R"), in_request("L"), in_request("I"), in_request("K"), in_request("PK"), in_request("J"), - in_request("ID")); + in_request("ID"), $row["Username"]); } } else { if (has_credential(CRED_ACCOUNT_SEARCH)) { diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index edd38ee..ad907df 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -39,6 +39,25 @@ function html_format_pgp_fingerprint($fingerprint) { } /** + * If a user name from the database is provided, use that one, as the other + * one might be tampered by the end user + * + * @param string $U The username, possibly provided by the user + * @param string $N The username as present in the database + * + * @return string A username that's always safe to use in URLs + */ +function create_safe_url_username($U, $N) { + // if a display username is provided but not one extracted + // from the database, use the former + if (!empty($U) && empty($N)) { + $N = $U; + } + + return $N; +} + +/** * Loads the account editing form, with any values that are already saved * * @global array $SUPPORTED_LANGS Languages that are supported by the AUR @@ -56,13 +75,15 @@ function html_format_pgp_fingerprint($fingerprint) { * @param string $PK The list of SSH public keys * @param string $J The inactivity status of the displayed user * @param string $UID The user ID of the displayed user + * @param string $N The username as present in the database * * @return void */ function display_account_form($A,$U="",$T="",$S="",$E="",$P="",$C="",$R="", - $L="",$I="",$K="",$PK="",$J="", $UID=0) { + $L="",$I="",$K="",$PK="",$J="",$UID=0,$N="") { global $SUPPORTED_LANGS; + $N = create_safe_url_username($U, $N); include("account_edit_form.php"); return; } @@ -86,13 +107,15 @@ function display_account_form($A,$U="",$T="",$S="",$E="",$P="",$C="",$R="", * @param string $PK The list of public SSH keys * @param string $J The inactivity status of the user * @param string $UID The user ID of the modified account + * @param string $N The username as present in the database * * @return string|void Return void if successful, otherwise return error */ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$P="",$C="", - $R="",$L="",$I="",$K="",$PK="",$J="",$UID=0) { + $R="",$L="",$I="",$K="",$PK="",$J="",$UID=0,$N="") { global $SUPPORTED_LANGS; + $N = create_safe_url_username($U, $N); $error = ''; if (is_ipbanned()) { @@ -247,7 +270,7 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$P="",$C="", if ($error) { print "<ul class='errorlist'><li>".$error."</li></ul>\n"; display_account_form($A, $U, $T, $S, $E, "", "", - $R, $L, $I, $K, $PK, $J, $UID); + $R, $L, $I, $K, $PK, $J, $UID, $N); return; } diff --git a/web/template/account_edit_form.php b/web/template/account_edit_form.php index 56bdd45..0aadb9d 100644 --- a/web/template/account_edit_form.php +++ b/web/template/account_edit_form.php @@ -1,9 +1,9 @@ <?php if ($A == "UpdateAccount"): ?> <p> - <?= __('Click %shere%s if you want to permanently delete this account.', '<a href="' . get_user_uri($U) . 'delete/' . '">', '</a>') ?> + <?= __('Click %shere%s if you want to permanently delete this account.', '<a href="' . get_user_uri($N) . 'delete/' . '">', '</a>') ?> </p> -<form id="edit-profile-form" action="<?= get_user_uri($U) . 'update/'; ?>" method="post"> +<form id="edit-profile-form" action="<?= get_user_uri($N) . 'update/'; ?>" method="post"> <?php else: ?> <form id="edit-profile-form" action="<?= get_uri('/register/'); ?>" method="post"> <?php endif; ?> -- 2.4.5
Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- web/html/register.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/html/register.php b/web/html/register.php index 014d802..cb3e8dd 100644 --- a/web/html/register.php +++ b/web/html/register.php @@ -25,7 +25,7 @@ if (in_request("Action") == "NewAccount") { in_request("PK")); } else { - print __("Use this form to create an account."); + print '<p>' . __("Use this form to create an account.") . '</p>'; display_account_form("NewAccount", "", "", "", "", "", "", "", $LANG); } -- 2.4.5
When a user account is modified, the 'My Account' link should contain the new username, otherwise clicking it directly after editing a username will result in a 404. Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- web/html/account.php | 12 +++++++++--- web/html/register.php | 8 +++----- web/lib/acctfuncs.inc.php | 22 ++++++++++++++++++++++ 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/web/html/account.php b/web/html/account.php index 8545478..675e880 100644 --- a/web/html/account.php +++ b/web/html/account.php @@ -21,14 +21,20 @@ if (in_array($action, $need_userinfo)) { if ($action == "AccountInfo") { html_header(__('Account') . ' ' . $row['Username']); -} else { +} elseif ($action == "DisplayAccount" || $action == "DeleteAccount") { html_header(__('Accounts')); } +// don't show header in case of UpdateAccount yet, as the username might be +// changed and then the 'My Account' link has to contain the new username, +// too # Main page processing here # -echo "<div class=\"box\">\n"; -echo " <h2>".__("Accounts")."</h2>\n"; +if ($action != "UpdateAccount") { + // only show header when not processing a form + echo "<div class=\"box\">\n"; + echo " <h2>".__("Accounts")."</h2>\n"; +} if (isset($_COOKIE["AURSID"])) { if ($action == "SearchAccounts") { diff --git a/web/html/register.php b/web/html/register.php index cb3e8dd..746974c 100644 --- a/web/html/register.php +++ b/web/html/register.php @@ -13,11 +13,6 @@ if (isset($_COOKIE["AURSID"])) { exit(); } -html_header(__('Register')); - -echo '<div class="box">'; -echo '<h2>' . __('Register') . '</h2>'; - if (in_request("Action") == "NewAccount") { process_account_form("new", "NewAccount", in_request("U"), 1, 0, in_request("E"), '', '', in_request("R"), @@ -25,6 +20,9 @@ if (in_request("Action") == "NewAccount") { in_request("PK")); } else { + html_header(__('Register')); + echo '<div class="box">'; + echo '<h2>' . __('Register') . '</h2>'; print '<p>' . __("Use this form to create an account.") . '</p>'; display_account_form("NewAccount", "", "", "", "", "", "", "", $LANG); } diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index ad907df..e2ea547 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -89,6 +89,25 @@ function display_account_form($A,$U="",$T="",$S="",$E="",$P="",$C="",$R="", } /** + * Display header and open div of account page + * + * @param string $TYPE Either "edit" for editing or "new" for registering an account + * + * @return void + */ +function display_account_resultpage_header($TYPE) { + if ($TYPE == "edit") { + html_header(__('Accounts')); + echo '<div class="box">'; + echo '<h2>' . __('Accounts') . '</h2>'; + } else { + html_header(__('Register')); + echo '<div class="box">'; + echo '<h2>' . __('Register') . '</h2>'; + } +} + +/** * Process information given to new/edit account form * * @global array $SUPPORTED_LANGS Languages that are supported by the AUR @@ -268,6 +287,7 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$P="",$C="", } if ($error) { + display_account_resultpage_header($TYPE); print "<ul class='errorlist'><li>".$error."</li></ul>\n"; display_account_form($A, $U, $T, $S, $E, "", "", $R, $L, $I, $K, $PK, $J, $UID, $N); @@ -298,6 +318,7 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$P="",$C="", $q.= "VALUES (1, 0, 0, $U, $E, $P, $salt, $R, $L, "; $q.= "$I, $K)"; $result = $dbh->exec($q); + display_account_resultpage_header($TYPE); if (!$result) { print __("Error trying to create account, %s%s%s.", "<strong>", htmlspecialchars($U,ENT_QUOTES), "</strong>"); @@ -370,6 +391,7 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$P="",$C="", $q.= ", InactivityTS = " . $inactivity_ts; $q.= " WHERE ID = ".intval($UID); $result = $dbh->exec($q); + display_account_resultpage_header($TYPE); $ssh_key_result = account_set_ssh_keys($UID, $ssh_keys, $ssh_fingerprints); -- 2.4.5
On Sat, 11 Jul 2015 at 18:58:00, Marcel Korpel wrote:
When a user account is modified, the 'My Account' link should contain the new username, otherwise clicking it directly after editing a username will result in a 404.
Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- web/html/account.php | 12 +++++++++--- web/html/register.php | 8 +++----- web/lib/acctfuncs.inc.php | 22 ++++++++++++++++++++++ 3 files changed, 34 insertions(+), 8 deletions(-) [...]
Ugh, that's a lot of hacky code, just to get the page title right. Maybe we should rather use PRG here to avoid all that?
* Lukas Fleischer <lfleischer@archlinux.org> (Mon, 13 Jul 2015 09:15:49 +0200):
Ugh, that's a lot of hacky code, just to get the page title right. Maybe we should rather use PRG here to avoid all that?
Also a better solution! We will have to send error messages through, though, which is not trivial. Using a GET parameter? That doesn't feel like the best way.
* Marcel Korpel <marcel.korpel@gmail.com> (Mon, 13 Jul 2015 16:22:43 +0200):
Also a better solution! We will have to send error messages through, though, which is not trivial. Using a GET parameter? That doesn't feel like the best way.
Looking better at it, it *seems* easier, as the only time we need to use PRG is when the account modification succeeds. However, to send a Location header, we still have to suppress output until the point the change succeeds or errors out. Also, leaving it the way it is, i.e., keeping this bug present, is not nice for the user who first decides to change their username and then wants to change other info, too. Their first attempt to do the second step, by clicking on 'My Account' again, will fail with a 404. Best, Marcel
On Sat, 11 Jul 2015 at 18:57:58, Marcel Korpel wrote:
This fixes a bug where the new user name input by the user was invalid, causing the account deletion link and the form action to be wrong.
Signed-off-by: Marcel Korpel <marcel.korpel@gmail.com> --- web/html/account.php | 2 +- web/lib/acctfuncs.inc.php | 29 ++++++++++++++++++++++++++--- web/template/account_edit_form.php | 4 ++-- 3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/web/html/account.php b/web/html/account.php index c447de3..8545478 100644 --- a/web/html/account.php +++ b/web/html/account.php @@ -100,7 +100,7 @@ if (isset($_COOKIE["AURSID"])) { in_request("E"), in_request("P"), in_request("C"), in_request("R"), in_request("L"), in_request("I"), in_request("K"), in_request("PK"), in_request("J"), - in_request("ID")); + in_request("ID"), $row["Username"]); } } else { if (has_credential(CRED_ACCOUNT_SEARCH)) { diff --git a/web/lib/acctfuncs.inc.php b/web/lib/acctfuncs.inc.php index edd38ee..ad907df 100644 --- a/web/lib/acctfuncs.inc.php +++ b/web/lib/acctfuncs.inc.php @@ -39,6 +39,25 @@ function html_format_pgp_fingerprint($fingerprint) { }
/** + * If a user name from the database is provided, use that one, as the other + * one might be tampered by the end user + * + * @param string $U The username, possibly provided by the user + * @param string $N The username as present in the database + * + * @return string A username that's always safe to use in URLs + */ +function create_safe_url_username($U, $N) { + // if a display username is provided but not one extracted + // from the database, use the former + if (!empty($U) && empty($N)) { + $N = $U; + } + + return $N; +} + +/** * Loads the account editing form, with any values that are already saved * * @global array $SUPPORTED_LANGS Languages that are supported by the AUR @@ -56,13 +75,15 @@ function html_format_pgp_fingerprint($fingerprint) { * @param string $PK The list of SSH public keys * @param string $J The inactivity status of the displayed user * @param string $UID The user ID of the displayed user + * @param string $N The username as present in the database * * @return void */ function display_account_form($A,$U="",$T="",$S="",$E="",$P="",$C="",$R="", - $L="",$I="",$K="",$PK="",$J="", $UID=0) { + $L="",$I="",$K="",$PK="",$J="",$UID=0,$N="") { global $SUPPORTED_LANGS;
+ $N = create_safe_url_username($U, $N);
Could you explain why this is needed? Doesn't $N always refer to the correct user name? Related comment below.
include("account_edit_form.php"); return; } @@ -86,13 +107,15 @@ function display_account_form($A,$U="",$T="",$S="",$E="",$P="",$C="",$R="", * @param string $PK The list of public SSH keys * @param string $J The inactivity status of the user * @param string $UID The user ID of the modified account + * @param string $N The username as present in the database * * @return string|void Return void if successful, otherwise return error */ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$P="",$C="", - $R="",$L="",$I="",$K="",$PK="",$J="",$UID=0) { + $R="",$L="",$I="",$K="",$PK="",$J="",$UID=0,$N="") { global $SUPPORTED_LANGS;
+ $N = create_safe_url_username($U, $N); $error = '';
if (is_ipbanned()) { @@ -247,7 +270,7 @@ function process_account_form($TYPE,$A,$U="",$T="",$S="",$E="",$P="",$C="", if ($error) { print "<ul class='errorlist'><li>".$error."</li></ul>\n"; display_account_form($A, $U, $T, $S, $E, "", "", - $R, $L, $I, $K, $PK, $J, $UID); + $R, $L, $I, $K, $PK, $J, $UID, $N);
I am confused. We only call display_account_form() here if an error occurs *before* updating the account. Shouldn't we *always* use $N here, i.e. drop the create_safe_url_username() invocation above?
return; }
diff --git a/web/template/account_edit_form.php b/web/template/account_edit_form.php index 56bdd45..0aadb9d 100644 --- a/web/template/account_edit_form.php +++ b/web/template/account_edit_form.php @@ -1,9 +1,9 @@ <?php if ($A == "UpdateAccount"): ?> <p> - <?= __('Click %shere%s if you want to permanently delete this account.', '<a href="' . get_user_uri($U) . 'delete/' . '">', '</a>') ?> + <?= __('Click %shere%s if you want to permanently delete this account.', '<a href="' . get_user_uri($N) . 'delete/' . '">', '</a>') ?> </p>
-<form id="edit-profile-form" action="<?= get_user_uri($U) . 'update/'; ?>" method="post"> +<form id="edit-profile-form" action="<?= get_user_uri($N) . 'update/'; ?>" method="post"> <?php else: ?> <form id="edit-profile-form" action="<?= get_uri('/register/'); ?>" method="post"> <?php endif; ?> -- 2.4.5
* Lukas Fleischer <lfleischer@archlinux.org> (Mon, 13 Jul 2015 09:13:24 +0200):
Could you explain why this is needed? Doesn't $N always refer to the correct user name? Related comment below. […] I am confused. We only call display_account_form() here if an error occurs *before* updating the account. Shouldn't we *always* use $N here, i.e. drop the create_safe_url_username() invocation above?
That would be a better solution, as this one is quite hacky: I didn't want to change the invocation of display_account_form() in other places, but that would be a better solution so $N always contains the username from the database. Then we can drop the (indeed confusing) function create_safe_url_username(). I'll try to implement this better solution and resend my patches tomorrow. Best, Marcel
participants (2)
-
Lukas Fleischer
-
Marcel Korpel