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.

#1 issue-2231269-1.patch495 bytesmr.york


mr.york’s picture

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) {

   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.