[aur-dev] [PATCH 1/3] Use username from the database if one is provided by the user

[Lukas Fleischer]

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 at 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