If the current variable is empty, then it takes the path from the $_GET['q'], which is not good.
It should check before, that the $_GET['current'] exists or not, and if so it should be set as current instead.
If we dont do it this way, the checks will be incorrect.

Files: 
CommentFileSizeAuthor
#1 issue-2231269-1.patch495 bytesmr.york

Comments

mr.york’s picture

StatusFileSize
new495 bytes

Attached the patch.

mithy’s picture

Priority:Critical» Normal
Status:Active» Closed (won't fix)

I wonder whether current can be set but NULL under normal operation. By no means this is critical.

mpdonadio’s picture

Status:Closed (won't fix)» Needs review

@mithy, I think this is a real bug.

Consider setting the destination based on the page you are on, but coming to the login form from a menu link:

1. Place 'user/login' in a menu.
2. Create a destination rule. Set the destination to be an arbitrary place, and set the path to match against to be somewhere else
3. Browser to the path you set above on the site and click this menu entry
4. You should be redirected to user/menu?current=somewhere
5. Login, and you should not goto the somewhere path you defined above. _login_destination_match_rule() will move on to the next rule().

login_destination_translated_menu_link_alter() tacks current= to the query options to the $item['user/login'], but login_destination_get_destination() does not check this.

Personally, I think this is better

@@ -342,7 +342,12 @@ function login_destination_get_destination($trigger = '', $current = NULL) {
     ->fetchAll();

   if ($current == NULL) {
-    $current = $_GET['q'];
+    if (!empty( $_GET['current'])) {
+      $current =  $_GET['current'];
+    }
+    else {
+      $current = $_GET['q'];
+    }

which also addresses the concern about current being NULL under normal operations. This fixes the scenarios I describe.

awolfey’s picture

This is a needed fix and the patch works.

DrCord’s picture

This patch worked for us as well.

SkidNCrashwell’s picture

I don't know why you didn't just use the ternary operator to save on a few lines of code :) And wouldn't the line above be better as

if ($current === NULL) {

(The argument about whether this way is quicker than is_null() rages on elsewhere so I'm not commenting on that!)

But if this patch has been reviewed and works, then status should be set to RTBC, so a new release can be done. Which would be awesome!