[aur-dev] [PATCH] False 'File Request' buttons for unconfirmed users
Displaying flag, notify, vote and file requet buttons for users which did not authenticate themselves and letting those fake buttons link to the login page. Signed-off-by: Gordian Edenhofer <gordian.edenhofer@gmail.com> --- web/template/pkg_details.php | 37 +++++++++++++++++++++++++++++++++++++ web/template/pkgbase_details.php | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+) diff --git a/web/template/pkg_details.php b/web/template/pkg_details.php index c1c07ba..fdb0a6c 100644 --- a/web/template/pkg_details.php +++ b/web/template/pkg_details.php @@ -88,6 +88,7 @@ $sources = pkg_sources($row["ID"]); <li><a href="<?= $cgit_uri . $row['BaseName'] . '.git' ?>/snapshot/master.tar.gz"><?= __('Download snapshot') ?></a></li> <li><a href="https://wiki.archlinux.org/index.php/Special:Search?search=<?= urlencode($row['Name']) ?>"><?= __('Search wiki') ?></a></li> <li><span class="flagged"><?php if ($row["OutOfDateTS"] !== NULL) { echo __('Flagged out-of-date')." (${out_of_date_time})"; } ?></span></li> + <?php if ($uid): ?> <?php if ($row["OutOfDateTS"] === NULL): ?> <li> @@ -143,6 +144,42 @@ $sources = pkg_sources($row["ID"]); <li><a href="<?= get_pkgbase_uri($row['BaseName']) . 'delete/'; ?>"><?= __('Delete Package'); ?></a></li> <li><a href="<?= get_pkgbase_uri($row['BaseName']) . 'merge/'; ?>"><?= __('Merge Package'); ?></a></li> <?php endif; ?> + <?php else: ?> + <?php if ($row["OutOfDateTS"] === NULL): ?> + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <form action="<?= get_uri('/login/', true); ?>" method="post"> + <?php else: ?> + <form action="<?= get_uri('/login/'); ?>" method="post"> + <?php endif; ?> + <input type="submit" class="button text-button" name="do_Flag" value="<?= __('Flag package out-of-date') ?>" /> + </form> + </li> + <?php endif; ?> + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <form action="<?= get_uri('/login/', true); ?>" method="post"> + <?php else: ?> + <form action="<?= get_uri('/login/'); ?>" method="post"> + <?php endif; ?> + <input type="submit" class="button text-button" name="do_Vote" value="<?= __('Vote for this package') ?>" /> + </form> + </li> + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <form action="<?= get_uri('/login/', true); ?>" method="post"> + <?php else: ?> + <form action="<?= get_uri('/login/'); ?>" method="post"> + <?php endif; ?> + <input type="submit" class="button text-button" name="do_Notify" value="<?= __('Notify of new comments') ?>" /> + </form> + </li> + <li><span class="flagged"><?php if ($row["RequestCount"] > 0) { echo _n('%d pending request', '%d pending requests', $row["RequestCount"]); } ?></span></li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <li><a href="<?= get_uri('/login/', true); ?>"><?= __('File Request'); ?></a></li> + <?php else: ?> + <li><a href="<?= get_uri('/login/'); ?>"><?= __('File Request'); ?></a></li> + <?php endif; ?> <?php endif; ?> <?php if ($uid && $row["MaintainerUID"] === NULL): ?> diff --git a/web/template/pkgbase_details.php b/web/template/pkgbase_details.php index 98a7219..d11e0e3 100644 --- a/web/template/pkgbase_details.php +++ b/web/template/pkgbase_details.php @@ -39,6 +39,7 @@ $pkgs = pkgbase_get_pkgnames($base_id); <li><a href="<?= $cgit_uri . $row['Name'] . '.git' ?>/snapshot/master.tar.gz"><?= __('Download snapshot') ?></a></li> <li><a href="https://wiki.archlinux.org/index.php/Special:Search?search=<?= urlencode($row['Name']) ?>"><?= __('Search wiki') ?></a></li> <li><span class="flagged"><?php if ($row["OutOfDateTS"] !== NULL) { echo __('Flagged out-of-date')." (${out_of_date_time})"; } ?></span></li> + <?php if ($uid): ?> <?php if ($row["OutOfDateTS"] === NULL): ?> <li> @@ -94,6 +95,42 @@ $pkgs = pkgbase_get_pkgnames($base_id); <li><a href="<?= get_pkgbase_uri($row['Name']) . 'delete/'; ?>"><?= __('Delete Package'); ?></a></li> <li><a href="<?= get_pkgbase_uri($row['Name']) . 'merge/'; ?>"><?= __('Merge Package'); ?></a></li> <?php endif; ?> + <?php else: ?> + <?php if ($row["OutOfDateTS"] === NULL): ?> + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <form action="<?= get_uri('/login/', true); ?>" method="post"> + <?php else: ?> + <form action="<?= get_uri('/login/'); ?>" method="post"> + <?php endif; ?> + <input type="submit" class="button text-button" name="do_Flag" value="<?= __('Flag package out-of-date') ?>" /> + </form> + </li> + <?php endif; ?> + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <form action="<?= get_uri('/login/', true); ?>" method="post"> + <?php else: ?> + <form action="<?= get_uri('/login/'); ?>" method="post"> + <?php endif; ?> + <input type="submit" class="button text-button" name="do_Vote" value="<?= __('Vote for this package') ?>" /> + </form> + </li> + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <form action="<?= get_uri('/login/', true); ?>" method="post"> + <?php else: ?> + <form action="<?= get_uri('/login/'); ?>" method="post"> + <?php endif; ?> + <input type="submit" class="button text-button" name="do_Notify" value="<?= __('Notify of new comments') ?>" /> + </form> + </li> + <li><span class="flagged"><?php if ($row["RequestCount"] > 0) { echo _n('%d pending request', '%d pending requests', $row["RequestCount"]); } ?></span></li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <li><a href="<?= get_uri('/login/', true); ?>"><?= __('File Request'); ?></a></li> + <?php else: ?> + <li><a href="<?= get_uri('/login/'); ?>"><?= __('File Request'); ?></a></li> + <?php endif; ?> <?php endif; ?> <?php if ($uid && $row["MaintainerUID"] === NULL): ?> -- 2.4.2
* Gordian Edenhofer <gordian.edenhofer@gmail.com> (Wed, 10 Jun 2015 19:56:12 +0200):
I'm not sure I like this user experience, especially as you get (back) to the home page after login yet, and not to the expected page. Best, Marcel
On Wed, 2015-06-10 at 20:45 +0200, Marcel Korpel wrote:
Good point, I will try to add this functionality pretty soon. However my main concern was to at least show the user what options the AUR offers, without having to dig through the wiki or contact the mailing list. Regards, Gordian
* Gordian Edenhofer <gordian.edenhofer@gmail.com> (Thu, 11 Jun 2015 00:35:59 +0200):
Note that I already attempted to implement this, but it was not secure enough (so, don't reinvent this wheel). [1] Best, Marcel [1]https://lists.archlinux.org/pipermail/aur-dev/2012-December/002309.html and follow-ups
On Fri, 2015-06-12 at 08:46 +0200, Lukas Fleischer wrote:
Since FS#32481 has been fixed, does this patch will be accepted as is or are there major objections? Best regards, Gordian
On Thu, 25 Jun 2015 at 17:47:34, Gordian Edenhofer wrote:
I like the idea but I do not like the implementation. The package base details and the package details pages are way too cluttered already. I just submitted two preparatory patches to clean up at least parts of the mess. Could you try to implement your feature on top of the pu branch? I didn't think of all the details but you probably don't even need to touch any templates. Amending the new html_action_link() and html_action_form() functions might be enough. Regards, Lukas
On Fri, 2015-06-26 at 12:48 +0200, Lukas Fleischer wrote:
The template called web/template/pkgbase_actions.php determine whether those fields should be displayed. A simple "<?php if ($uid): ?>" is used for this purpose. Therefore it would be necessary to alter this file in order for users with no $uid to at least see those buttons respectively links. I am currently working on the implementation and will submit a new patch pretty soon. Best Regards, Gordian
* Gordian Edenhofer <gordian.edenhofer@gmail.com> (Thu, 11 Jun 2015 00:35:59 +0200):
There's still one thing to bear in mind with these buttons: even if you click one of them and the redirection after login is implemented, you get back to the package page and you have to click the button again. I don't think this is a good user experience. A solution would be to * send the login page something that doesn't let it redirect back to the package page, but to the action page, or * don't use these false buttons at all, but always link to the correct pages, and let those decide whether a user has the permission to do this action and if not, redirect to the login page: this way the login page will redirect back to the action page. I'd go for the second approach. Please tell what you think about it. Best, Marcel
On Fri, 2015-06-19 at 22:48 +0200, Marcel Korpel wrote:
Those action pages assume to get incoming post requests. This comes down to the problem I tried to point out in my comment: Post requests are not redirected as post requests. Flagging or voting a package generates a post request, however my approach is not able to redirect an incoming post request to the login page back as an outgoing post request to the desired destination. For this redirect to work js would be needed to autosubmit an invisible form back to the original referer. Though without js this redirect would not work and the site would be crippled. Best regards, Gordian
The function html_action_link closes its link with </a> hence there is no need for closing them again after invoking the function. --- web/template/pkgbase_actions.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/web/template/pkgbase_actions.php b/web/template/pkgbase_actions.php index 757b063..a659c88 100644 --- a/web/template/pkgbase_actions.php +++ b/web/template/pkgbase_actions.php @@ -29,18 +29,18 @@ <?php endif; ?> <?php if (has_credential(CRED_PKGBASE_EDIT_COMAINTAINERS, array($row["MaintainerUID"]))): ?> - <li><?= html_action_link($base_uri . 'comaintainers/', __('Manage Co-Maintainers')) ?></a></li> + <li><?= html_action_link($base_uri . 'comaintainers/', __('Manage Co-Maintainers')) ?></li> <?php endif; ?> <li><span class="flagged"><?php if ($row["RequestCount"] > 0) { echo _n('%d pending request', '%d pending requests', $row["RequestCount"]); } ?></span></li> - <li><?= html_action_link($base_uri . 'request/', __('File Request')) ?></a></li> + <li><?= html_action_link($base_uri . 'request/', __('File Request')) ?></li> <?php if (has_credential(CRED_PKGBASE_DELETE)): ?> - <li><?= html_action_link($base_uri . 'delete/', __('Delete Package')) ?></a></li> - <li><?= html_action_link($base_uri . 'merge/', __('Merge Package')) ?></a></li> + <li><?= html_action_link($base_uri . 'delete/', __('Delete Package')) ?></li> + <li><?= html_action_link($base_uri . 'merge/', __('Merge Package')) ?></li> <?php endif; ?> - <?php if ($uid && $row["MaintainerUID"] === NULL): ?> + <?php if ($row["MaintainerUID"] === NULL): ?> <li><?= html_action_form($base_uri . 'adopt/', "do_Adopt", __('Adopt Package')) ?></li> <?php elseif (has_credential(CRED_PKGBASE_DISOWN, array($row["MaintainerUID"]))): ?> <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package')) ?></li> -- 2.4.4
On Fri, 26 Jun 2015 at 17:41:20, Gordian Edenhofer wrote:
Good catch. I'll squash this into my original patch and add your Signed-off-by if you're fine with that. Thanks!
Displaying flag, notify, vote, adopt and file requet buttons for users which did not authenticate themselves and letting those fake buttons link to the login page. --- web/template/pkgbase_actions.php | 47 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/web/template/pkgbase_actions.php b/web/template/pkgbase_actions.php index a659c88..cd55464 100644 --- a/web/template/pkgbase_actions.php +++ b/web/template/pkgbase_actions.php @@ -45,6 +45,53 @@ <?php elseif (has_credential(CRED_PKGBASE_DISOWN, array($row["MaintainerUID"]))): ?> <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package')) ?></li> <?php endif; ?> + + <?php else: ?> + <?php if ($row["OutOfDateTS"] === NULL): ?> + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <?= html_action_link(get_uri('/login/', true), __('Flag package out-of-date')) ?> + <?php else: ?> + <?= html_action_link(get_uri('/login/'), __('Flag package out-of-date')) ?> + <?php endif; ?> + </li> + <?php endif; ?> + + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <?= html_action_link(get_uri('/login/', true), __('Vote for this package')) ?> + <?php else: ?> + <?= html_action_link(get_uri('/login/'), __('Vote for this package')) ?> + <?php endif; ?> + </li> + + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <?= html_action_link(get_uri('/login/', true), __('Notify of new comments')) ?> + <?php else: ?> + <?= html_action_link(get_uri('/login/'), __('Notify of new comments')) ?> + <?php endif; ?> + </li> + + <li><span class="flagged"><?php if ($row["RequestCount"] > 0) { echo _n('%d pending request', '%d pending requests', $row["RequestCount"]); } ?></span></li> + + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <?= html_action_link(get_uri('/login/', true), __('File Request')) ?> + <?php else: ?> + <?= html_action_link(get_uri('/login/'), __('File Request')) ?> + <?php endif; ?> + </li> + + <?php if ($row["MaintainerUID"] === NULL): ?> + <li> + <?php if (config_get_bool('options', 'disable_http_login') && empty($_SERVER['HTTPS'])): ?> + <?= html_action_link(get_uri('/login/', true), __('Adopt Package')) ?> + <?php else: ?> + <?= html_action_link(get_uri('/login/'), __('Adopt Package')) ?> + <?php endif; ?> + </li> + <?php endif; ?> <?php endif; ?> </ul> </div> -- 2.4.4
On Fri, 26 Jun 2015 at 18:06:36, Gordian Edenhofer wrote:
Thanks for submitting a reworked version! I still think this is way too much duplicate code, though. First of all, you are repeating the same if-else statement over and over again which is an indication that there should be a wrapper function. But then again, I don't think we need two differentiate those cases at all. Redirecting to the HTTP page when the user chooses to use HTTP is fine. The official setup even uses HTTPs unconditionally, so this doesn't affect us at all. Maybe it is time to drop the disable_http_login setting... I also still think that it is possible to implement this without duplicating all the links. Is there a reason we cannot simply remove the $uid check from the pkgbase_actions template and make the html_action_link() helper generate either proper or fake links, based on whether you are logged in? Regards, Lukas
Displaying flag, notify, vote, adopt and file request links for users which did not authenticate themselves and letting those fake buttons link to the login page. --- Agreed, the statements were kind of redundant. I hope this patch is more straightforward. web/lib/aur.inc.php | 33 ++++++++++++++++++++++----------- web/template/pkgbase_actions.php | 26 ++++++++++++-------------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 95f72ce..98ebde4 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -226,11 +226,16 @@ function html_format_maintainers($maintainer, $comaintainers) { * * @param string $uri The link target * @param string $desc The link label + * @param string $uid The User ID * * @return string The generated HTML code for the action link */ -function html_action_link($uri, $desc) { - $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; +function html_action_link($uri, $desc, $uid="") { + if ($uid) { + $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '">'; + } $code .= htmlspecialchars($desc) . '</a>'; return $code; @@ -242,18 +247,24 @@ function html_action_link($uri, $desc) { * @param string $uri The link target * @param string $action The action name (passed as HTTP POST parameter) * @param string $desc The link label + * @param string $uid The User ID * * @return string The generated HTML code for the action link */ -function html_action_form($uri, $action, $desc) { - $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; - $code .= 'method="post">'; - $code .= '<input type="hidden" name="token" value="'; - $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; - $code .= '<input type="submit" class="button text-button" name="'; - $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; - $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; - $code .= '</form>'; +function html_action_form($uri, $action, $desc, $uid="") { + if ($uid) { + $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; + $code .= 'method="post">'; + $code .= '<input type="hidden" name="token" value="'; + $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; + $code .= '<input type="submit" class="button text-button" name="'; + $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; + $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; + $code .= '</form>'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '">'; + $code .= htmlspecialchars($desc) . '</a>'; + } return $code; } diff --git a/web/template/pkgbase_actions.php b/web/template/pkgbase_actions.php index a659c88..9675d3a 100644 --- a/web/template/pkgbase_actions.php +++ b/web/template/pkgbase_actions.php @@ -9,42 +9,40 @@ <li><a href="<?= $snapshot_uri ?>"><?= __('Download snapshot') ?></a> <li><a href="https://wiki.archlinux.org/index.php/Special:Search?search=<?= urlencode($row['Name']) ?>"><?= __('Search wiki') ?></a></li> <li><span class="flagged"><?php if ($row["OutOfDateTS"] !== NULL) { echo __('Flagged out-of-date')." (${out_of_date_time})"; } ?></span></li> - <?php if ($uid): ?> <?php if ($row["OutOfDateTS"] === NULL): ?> - <li><?= html_action_form($base_uri . 'flag/', "do_Flag", __('Flag package out-of-date')) ?></li> + <li><?= html_action_form($base_uri . 'flag/', "do_Flag", __('Flag package out-of-date'), $uid) ?></li> <?php elseif (($row["OutOfDateTS"] !== NULL) && has_credential(CRED_PKGBASE_UNFLAG, $maintainers)): ?> - <li><?= html_action_form($base_uri . 'unflag/', "do_UnFlag", __('Unflag package')) ?></li> + <li><?= html_action_form($base_uri . 'unflag/', "do_UnFlag", __('Unflag package'), $uid) ?></li> <?php endif; ?> <?php if (pkgbase_user_voted($uid, $base_id)): ?> - <li><?= html_action_form($base_uri . 'unvote/', "do_UnVote", __('Remove vote')) ?></li> + <li><?= html_action_form($base_uri . 'unvote/', "do_UnVote", __('Remove vote'), $uid) ?></li> <?php else: ?> - <li><?= html_action_form($base_uri . 'vote/', "do_Vote", __('Vote for this package')) ?></li> + <li><?= html_action_form($base_uri . 'vote/', "do_Vote", __('Vote for this package'), $uid) ?></li> <?php endif; ?> <?php if (pkgbase_user_notify($uid, $base_id)): ?> - <li><?= html_action_form($base_uri . 'unnotify/', "do_UnNotify", __('Disable notifications')) ?></li> + <li><?= html_action_form($base_uri . 'unnotify/', "do_UnNotify", __('Disable notifications'), $uid) ?></li> <?php else: ?> - <li><?= html_action_form($base_uri . 'notify/', "do_Notify", __('Notify of new comments')) ?></li> + <li><?= html_action_form($base_uri . 'notify/', "do_Notify", __('Notify of new comments'), $uid) ?></li> <?php endif; ?> <?php if (has_credential(CRED_PKGBASE_EDIT_COMAINTAINERS, array($row["MaintainerUID"]))): ?> - <li><?= html_action_link($base_uri . 'comaintainers/', __('Manage Co-Maintainers')) ?></li> + <li><?= html_action_link($base_uri . 'comaintainers/', __('Manage Co-Maintainers'), $uid) ?></li> <?php endif; ?> <li><span class="flagged"><?php if ($row["RequestCount"] > 0) { echo _n('%d pending request', '%d pending requests', $row["RequestCount"]); } ?></span></li> - <li><?= html_action_link($base_uri . 'request/', __('File Request')) ?></li> + <li><?= html_action_link($base_uri . 'request/', __('File Request'), $uid) ?></li> <?php if (has_credential(CRED_PKGBASE_DELETE)): ?> - <li><?= html_action_link($base_uri . 'delete/', __('Delete Package')) ?></li> - <li><?= html_action_link($base_uri . 'merge/', __('Merge Package')) ?></li> + <li><?= html_action_link($base_uri . 'delete/', __('Delete Package'), $uid) ?></li> + <li><?= html_action_link($base_uri . 'merge/', __('Merge Package'), $uid) ?></li> <?php endif; ?> <?php if ($row["MaintainerUID"] === NULL): ?> - <li><?= html_action_form($base_uri . 'adopt/', "do_Adopt", __('Adopt Package')) ?></li> + <li><?= html_action_form($base_uri . 'adopt/', "do_Adopt", __('Adopt Package'), $uid) ?></li> <?php elseif (has_credential(CRED_PKGBASE_DISOWN, array($row["MaintainerUID"]))): ?> - <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package')) ?></li> - <?php endif; ?> + <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package'), $uid) ?></li> <?php endif; ?> </ul> </div> -- 2.4.4
On Fri, 26 Jun 2015 at 21:03:17, Gordian Edenhofer wrote:
Forgot to mention it in your previous patches but could you add your sign-off, please (simply pass -s when running `git commit`)?
Looks much better indeed, thanks! Some comments below.
Do we really need to make the $uid parameter optional? Do we skip the parameter somewhere? If it makes sense, could we please use false as default value instead of an empty string?
Cool, this is kind of what I expected! I wonder whether we can directly set a referer here, though? Shouldn't something like get_uri('/login/', true) . '?referer=' . urlencode($uri) work? Having said that, I now see a potential problem with the GET parameter approach of implementing FS#32481. You could build a malicious login link that redirects to a certain page and send that link to a privileged user (i.e. a TU or a Developer). I am not aware of any action that cannot be undone and doesn't require any additional confirmation, so that probably isn't very critical. We should fix it anyway... Regards, Lukas
On Fri, 2015-06-26 at 21:26 +0200, Lukas Fleischer wrote:
I can not make sense out of the "?referer" parameter, since my fix of FS#32481 uses $_SERVER['HTTP_REFERER'] and does not rely on an additional parameter to a GET request.
Since most of the relevant package actions are perform through POST req uests (which are redirected as GET requests), I could not think of any potential security issues. Furthermore the referer is partially checked for outgoing links: if (strpos($referer, aur_location()) !== 0) { $referer = '/'; }
On Fri, 26 Jun 2015 at 22:14:20, Gordian Edenhofer wrote:
It uses $_SERVER['HTTP_REFERER'] unless $_REQUEST['referer'] is set. So we should be able to overwrite the referer by setting the HTTP GET parameter.
Yeah, however, it is already kind of annoying if you can make a TU flag a package out-of-date without noticing. And we might introduce something more critical that does not require a POST request in the future. Better safe than sorry. This is something to fix in a separate patch set, though.
On Fri, 2015-06-26 at 23:08 +0200, Lukas Fleischer wrote:
I forgot that I used $_REQUEST, I though that it was $_POST. My bad! Though if I think of it, it just might be a good idea to switch to $_POST since then $_GET parameters like "?refer" would not be concidered and only $_SERVER['HTTP_REFERER'] or a POST "referer" would be accepted. Shell I submit another patch for that or is the gain in security negligible?
Flagging, voting, notifying and adopting a package is all done through POST requests AFAIK. Deleting or merging a package is not even available for unauthenticated users. Hence a malicious URL would not flag a package since the corresponding variable is not set.
On Fri, 26 Jun 2015 at 23:40:26, Gordian Edenhofer wrote:
I would say it is negligible. Let's take advantage of this now to implement the redirection as I suggested. We need to fix the security issues properly in another patch series in any case.
Yeah, you're right. We also use a CSRF token in most places. It should be implemented properly at some point anyway.
Displaying flag, notify, vote, adopt and file request links for users which did not authenticate themselves and letting those fake buttons link to the login page. Signed-off-by: Gordian Edenhofer <gordian.edenhofer@gmail.com> --- Here you go! web/lib/aur.inc.php | 35 ++++++++++++++++++++++++----------- web/template/pkgbase_actions.php | 26 ++++++++++++-------------- 2 files changed, 36 insertions(+), 25 deletions(-) diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 95f72ce..8216d1c 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -226,11 +226,17 @@ function html_format_maintainers($maintainer, $comaintainers) { * * @param string $uri The link target * @param string $desc The link label + * @param string $uid The User ID * * @return string The generated HTML code for the action link */ -function html_action_link($uri, $desc) { - $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; +function html_action_link($uri, $desc, $uid) { + if ($uid) { + $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '?referer='; + $code .= urlencode($uri) . '">'; + } $code .= htmlspecialchars($desc) . '</a>'; return $code; @@ -242,18 +248,25 @@ function html_action_link($uri, $desc) { * @param string $uri The link target * @param string $action The action name (passed as HTTP POST parameter) * @param string $desc The link label + * @param string $uid The User ID * * @return string The generated HTML code for the action link */ -function html_action_form($uri, $action, $desc) { - $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; - $code .= 'method="post">'; - $code .= '<input type="hidden" name="token" value="'; - $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; - $code .= '<input type="submit" class="button text-button" name="'; - $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; - $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; - $code .= '</form>'; +function html_action_form($uri, $action, $desc, $uid) { + if ($uid) { + $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; + $code .= 'method="post">'; + $code .= '<input type="hidden" name="token" value="'; + $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; + $code .= '<input type="submit" class="button text-button" name="'; + $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; + $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; + $code .= '</form>'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '?referer='; + $code .= urlencode($uri) . '">'; + $code .= htmlspecialchars($desc) . '</a>'; + } return $code; } diff --git a/web/template/pkgbase_actions.php b/web/template/pkgbase_actions.php index a659c88..9675d3a 100644 --- a/web/template/pkgbase_actions.php +++ b/web/template/pkgbase_actions.php @@ -9,42 +9,40 @@ <li><a href="<?= $snapshot_uri ?>"><?= __('Download snapshot') ?></a> <li><a href="https://wiki.archlinux.org/index.php/Special:Search?search=<?= urlencode($row['Name']) ?>"><?= __('Search wiki') ?></a></li> <li><span class="flagged"><?php if ($row["OutOfDateTS"] !== NULL) { echo __('Flagged out-of-date')." (${out_of_date_time})"; } ?></span></li> - <?php if ($uid): ?> <?php if ($row["OutOfDateTS"] === NULL): ?> - <li><?= html_action_form($base_uri . 'flag/', "do_Flag", __('Flag package out-of-date')) ?></li> + <li><?= html_action_form($base_uri . 'flag/', "do_Flag", __('Flag package out-of-date'), $uid) ?></li> <?php elseif (($row["OutOfDateTS"] !== NULL) && has_credential(CRED_PKGBASE_UNFLAG, $maintainers)): ?> - <li><?= html_action_form($base_uri . 'unflag/', "do_UnFlag", __('Unflag package')) ?></li> + <li><?= html_action_form($base_uri . 'unflag/', "do_UnFlag", __('Unflag package'), $uid) ?></li> <?php endif; ?> <?php if (pkgbase_user_voted($uid, $base_id)): ?> - <li><?= html_action_form($base_uri . 'unvote/', "do_UnVote", __('Remove vote')) ?></li> + <li><?= html_action_form($base_uri . 'unvote/', "do_UnVote", __('Remove vote'), $uid) ?></li> <?php else: ?> - <li><?= html_action_form($base_uri . 'vote/', "do_Vote", __('Vote for this package')) ?></li> + <li><?= html_action_form($base_uri . 'vote/', "do_Vote", __('Vote for this package'), $uid) ?></li> <?php endif; ?> <?php if (pkgbase_user_notify($uid, $base_id)): ?> - <li><?= html_action_form($base_uri . 'unnotify/', "do_UnNotify", __('Disable notifications')) ?></li> + <li><?= html_action_form($base_uri . 'unnotify/', "do_UnNotify", __('Disable notifications'), $uid) ?></li> <?php else: ?> - <li><?= html_action_form($base_uri . 'notify/', "do_Notify", __('Notify of new comments')) ?></li> + <li><?= html_action_form($base_uri . 'notify/', "do_Notify", __('Notify of new comments'), $uid) ?></li> <?php endif; ?> <?php if (has_credential(CRED_PKGBASE_EDIT_COMAINTAINERS, array($row["MaintainerUID"]))): ?> - <li><?= html_action_link($base_uri . 'comaintainers/', __('Manage Co-Maintainers')) ?></li> + <li><?= html_action_link($base_uri . 'comaintainers/', __('Manage Co-Maintainers'), $uid) ?></li> <?php endif; ?> <li><span class="flagged"><?php if ($row["RequestCount"] > 0) { echo _n('%d pending request', '%d pending requests', $row["RequestCount"]); } ?></span></li> - <li><?= html_action_link($base_uri . 'request/', __('File Request')) ?></li> + <li><?= html_action_link($base_uri . 'request/', __('File Request'), $uid) ?></li> <?php if (has_credential(CRED_PKGBASE_DELETE)): ?> - <li><?= html_action_link($base_uri . 'delete/', __('Delete Package')) ?></li> - <li><?= html_action_link($base_uri . 'merge/', __('Merge Package')) ?></li> + <li><?= html_action_link($base_uri . 'delete/', __('Delete Package'), $uid) ?></li> + <li><?= html_action_link($base_uri . 'merge/', __('Merge Package'), $uid) ?></li> <?php endif; ?> <?php if ($row["MaintainerUID"] === NULL): ?> - <li><?= html_action_form($base_uri . 'adopt/', "do_Adopt", __('Adopt Package')) ?></li> + <li><?= html_action_form($base_uri . 'adopt/', "do_Adopt", __('Adopt Package'), $uid) ?></li> <?php elseif (has_credential(CRED_PKGBASE_DISOWN, array($row["MaintainerUID"]))): ?> - <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package')) ?></li> - <?php endif; ?> + <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package'), $uid) ?></li> <?php endif; ?> </ul> </div> -- 2.4.4
On Fri, 26 Jun 2015 at 21:03:17, Gordian Edenhofer wrote:
Oh, seems like this doesn't work... We need an absolute URI here.
I don't think we should set the referer here. These actions require an HTTP POST request.
The pkgbase_actions hunks did not apply cleanly for some reason. Instead of trying to fix that, I applied a slightly modified version. I will submit it for review in a minute. Thanks!
On Sat, 2015-06-27 at 11:11 +0200, Lukas Fleischer wrote:
My fault! Though I still don't get why one should set this parameter, espicially because it is contained in the $_SERVER['HTTP_REFERER'] and would be properly set either way.
Feel free to use your version. I am glad that this feature is being added.
Displaying flag, notify, vote, adopt and file request links for users which did not authenticate themselves and letting those fake buttons link to the login page. Signed-off-by: Gordian Edenhofer <gordian.edenhofer@gmail.com> --- web/lib/aur.inc.php | 33 ++++++++++++++++++++++----------- web/template/pkgbase_actions.php | 26 ++++++++++++-------------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 95f72ce..eb313e3 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -226,11 +226,16 @@ function html_format_maintainers($maintainer, $comaintainers) { * * @param string $uri The link target * @param string $desc The link label + * @param string $uid The User ID * * @return string The generated HTML code for the action link */ -function html_action_link($uri, $desc) { - $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; +function html_action_link($uri, $desc, $uid) { + if ($uid) { + $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '">'; + } $code .= htmlspecialchars($desc) . '</a>'; return $code; @@ -242,18 +247,24 @@ function html_action_link($uri, $desc) { * @param string $uri The link target * @param string $action The action name (passed as HTTP POST parameter) * @param string $desc The link label + * @param string $uid The User ID * * @return string The generated HTML code for the action link */ -function html_action_form($uri, $action, $desc) { - $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; - $code .= 'method="post">'; - $code .= '<input type="hidden" name="token" value="'; - $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; - $code .= '<input type="submit" class="button text-button" name="'; - $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; - $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; - $code .= '</form>'; +function html_action_form($uri, $action, $desc, $uid) { + if ($uid) { + $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; + $code .= 'method="post">'; + $code .= '<input type="hidden" name="token" value="'; + $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; + $code .= '<input type="submit" class="button text-button" name="'; + $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; + $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; + $code .= '</form>'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '">'; + $code .= htmlspecialchars($desc) . '</a>'; + } return $code; } diff --git a/web/template/pkgbase_actions.php b/web/template/pkgbase_actions.php index a659c88..9675d3a 100644 --- a/web/template/pkgbase_actions.php +++ b/web/template/pkgbase_actions.php @@ -9,42 +9,40 @@ <li><a href="<?= $snapshot_uri ?>"><?= __('Download snapshot') ?></a> <li><a href="https://wiki.archlinux.org/index.php/Special:Search?search=<?= urlencode($row['Name']) ?>"><?= __('Search wiki') ?></a></li> <li><span class="flagged"><?php if ($row["OutOfDateTS"] !== NULL) { echo __('Flagged out-of-date')." (${out_of_date_time})"; } ?></span></li> - <?php if ($uid): ?> <?php if ($row["OutOfDateTS"] === NULL): ?> - <li><?= html_action_form($base_uri . 'flag/', "do_Flag", __('Flag package out-of-date')) ?></li> + <li><?= html_action_form($base_uri . 'flag/', "do_Flag", __('Flag package out-of-date'), $uid) ?></li> <?php elseif (($row["OutOfDateTS"] !== NULL) && has_credential(CRED_PKGBASE_UNFLAG, $maintainers)): ?> - <li><?= html_action_form($base_uri . 'unflag/', "do_UnFlag", __('Unflag package')) ?></li> + <li><?= html_action_form($base_uri . 'unflag/', "do_UnFlag", __('Unflag package'), $uid) ?></li> <?php endif; ?> <?php if (pkgbase_user_voted($uid, $base_id)): ?> - <li><?= html_action_form($base_uri . 'unvote/', "do_UnVote", __('Remove vote')) ?></li> + <li><?= html_action_form($base_uri . 'unvote/', "do_UnVote", __('Remove vote'), $uid) ?></li> <?php else: ?> - <li><?= html_action_form($base_uri . 'vote/', "do_Vote", __('Vote for this package')) ?></li> + <li><?= html_action_form($base_uri . 'vote/', "do_Vote", __('Vote for this package'), $uid) ?></li> <?php endif; ?> <?php if (pkgbase_user_notify($uid, $base_id)): ?> - <li><?= html_action_form($base_uri . 'unnotify/', "do_UnNotify", __('Disable notifications')) ?></li> + <li><?= html_action_form($base_uri . 'unnotify/', "do_UnNotify", __('Disable notifications'), $uid) ?></li> <?php else: ?> - <li><?= html_action_form($base_uri . 'notify/', "do_Notify", __('Notify of new comments')) ?></li> + <li><?= html_action_form($base_uri . 'notify/', "do_Notify", __('Notify of new comments'), $uid) ?></li> <?php endif; ?> <?php if (has_credential(CRED_PKGBASE_EDIT_COMAINTAINERS, array($row["MaintainerUID"]))): ?> - <li><?= html_action_link($base_uri . 'comaintainers/', __('Manage Co-Maintainers')) ?></li> + <li><?= html_action_link($base_uri . 'comaintainers/', __('Manage Co-Maintainers'), $uid) ?></li> <?php endif; ?> <li><span class="flagged"><?php if ($row["RequestCount"] > 0) { echo _n('%d pending request', '%d pending requests', $row["RequestCount"]); } ?></span></li> - <li><?= html_action_link($base_uri . 'request/', __('File Request')) ?></li> + <li><?= html_action_link($base_uri . 'request/', __('File Request'), $uid) ?></li> <?php if (has_credential(CRED_PKGBASE_DELETE)): ?> - <li><?= html_action_link($base_uri . 'delete/', __('Delete Package')) ?></li> - <li><?= html_action_link($base_uri . 'merge/', __('Merge Package')) ?></li> + <li><?= html_action_link($base_uri . 'delete/', __('Delete Package'), $uid) ?></li> + <li><?= html_action_link($base_uri . 'merge/', __('Merge Package'), $uid) ?></li> <?php endif; ?> <?php if ($row["MaintainerUID"] === NULL): ?> - <li><?= html_action_form($base_uri . 'adopt/', "do_Adopt", __('Adopt Package')) ?></li> + <li><?= html_action_form($base_uri . 'adopt/', "do_Adopt", __('Adopt Package'), $uid) ?></li> <?php elseif (has_credential(CRED_PKGBASE_DISOWN, array($row["MaintainerUID"]))): ?> - <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package')) ?></li> - <?php endif; ?> + <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package'), $uid) ?></li> <?php endif; ?> </ul> </div> -- 2.4.4
From: Gordian Edenhofer <gordian.edenhofer@gmail.com> Displaying flag, notify, vote, adopt and file request links for users which did not authenticate themselves and letting those fake buttons link to the login page. Signed-off-by: Gordian Edenhofer <gordian.edenhofer@gmail.com> Signed-off-by: Lukas Fleischer <lfleischer@archlinux.org> --- Changes since v4: * Use absolute URIs when setting the referer. * Do not set the referer GET parameter in html_action_form(). * Simplify the patch such that the $uid argument isn't needed at all. web/lib/aur.inc.php | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 95f72ce..06d604d 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -230,7 +230,12 @@ function html_format_maintainers($maintainer, $comaintainers) { * @return string The generated HTML code for the action link */ function html_action_link($uri, $desc) { - $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; + if (isset($_COOKIE["AURSID"])) { + $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '?referer='; + $code .= urlencode($uri) . '">'; + } $code .= htmlspecialchars($desc) . '</a>'; return $code; @@ -246,14 +251,19 @@ function html_action_link($uri, $desc) { * @return string The generated HTML code for the action link */ function html_action_form($uri, $action, $desc) { - $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; - $code .= 'method="post">'; - $code .= '<input type="hidden" name="token" value="'; - $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; - $code .= '<input type="submit" class="button text-button" name="'; - $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; - $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; - $code .= '</form>'; + if (isset($_COOKIE["AURSID"])) { + $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; + $code .= 'method="post">'; + $code .= '<input type="hidden" name="token" value="'; + $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; + $code .= '<input type="submit" class="button text-button" name="'; + $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; + $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; + $code .= '</form>'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '">'; + $code .= htmlspecialchars($desc) . '</a>'; + } return $code; } -- 2.4.4
From: Gordian Edenhofer <gordian.edenhofer@gmail.com> Displaying flag, notify, vote, adopt and file request links for users which did not authenticate themselves and letting those fake buttons link to the login page. Signed-off-by: Gordian Edenhofer <gordian.edenhofer@gmail.com> Signed-off-by: Lukas Fleischer <lfleischer@archlinux.org> --- Accidentally submitted an old version of the v5 patch. This one actually does what the v5 changelog says. web/lib/aur.inc.php | 28 +++++++++++++++++++--------- web/template/pkgbase_actions.php | 2 -- 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/web/lib/aur.inc.php b/web/lib/aur.inc.php index 95f72ce..7a455c6 100644 --- a/web/lib/aur.inc.php +++ b/web/lib/aur.inc.php @@ -230,7 +230,12 @@ function html_format_maintainers($maintainer, $comaintainers) { * @return string The generated HTML code for the action link */ function html_action_link($uri, $desc) { - $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; + if (isset($_COOKIE["AURSID"])) { + $code = '<a href="' . htmlspecialchars($uri, ENT_QUOTES) . '">'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '?referer='; + $code .= urlencode(rtrim(aur_location(), '/') . $uri) . '">'; + } $code .= htmlspecialchars($desc) . '</a>'; return $code; @@ -246,14 +251,19 @@ function html_action_link($uri, $desc) { * @return string The generated HTML code for the action link */ function html_action_form($uri, $action, $desc) { - $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; - $code .= 'method="post">'; - $code .= '<input type="hidden" name="token" value="'; - $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; - $code .= '<input type="submit" class="button text-button" name="'; - $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; - $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; - $code .= '</form>'; + if (isset($_COOKIE["AURSID"])) { + $code = '<form action="' . htmlspecialchars($uri, ENT_QUOTES) . '" '; + $code .= 'method="post">'; + $code .= '<input type="hidden" name="token" value="'; + $code .= htmlspecialchars($_COOKIE['AURSID'], ENT_QUOTES) . '" />'; + $code .= '<input type="submit" class="button text-button" name="'; + $code .= htmlspecialchars($action, ENT_QUOTES) . '" '; + $code .= 'value="' . htmlspecialchars($desc, ENT_QUOTES) . '" />'; + $code .= '</form>'; + } else { + $code = '<a href="' . get_uri('/login/', true) . '">'; + $code .= htmlspecialchars($desc) . '</a>'; + } return $code; } diff --git a/web/template/pkgbase_actions.php b/web/template/pkgbase_actions.php index 757b063..61ad18f 100644 --- a/web/template/pkgbase_actions.php +++ b/web/template/pkgbase_actions.php @@ -9,7 +9,6 @@ <li><a href="<?= $snapshot_uri ?>"><?= __('Download snapshot') ?></a> <li><a href="https://wiki.archlinux.org/index.php/Special:Search?search=<?= urlencode($row['Name']) ?>"><?= __('Search wiki') ?></a></li> <li><span class="flagged"><?php if ($row["OutOfDateTS"] !== NULL) { echo __('Flagged out-of-date')." (${out_of_date_time})"; } ?></span></li> - <?php if ($uid): ?> <?php if ($row["OutOfDateTS"] === NULL): ?> <li><?= html_action_form($base_uri . 'flag/', "do_Flag", __('Flag package out-of-date')) ?></li> <?php elseif (($row["OutOfDateTS"] !== NULL) && has_credential(CRED_PKGBASE_UNFLAG, $maintainers)): ?> @@ -45,7 +44,6 @@ <?php elseif (has_credential(CRED_PKGBASE_DISOWN, array($row["MaintainerUID"]))): ?> <li><?= html_action_form($base_uri . 'disown/', "do_Disown", __('Disown Package')) ?></li> <?php endif; ?> - <?php endif; ?> </ul> </div> </div> -- 2.4.4
participants (3)
-
Gordian Edenhofer
-
Lukas Fleischer
-
Marcel Korpel