drupal_access_denied() in page callback displays access denied page twice

I can confirm this bug exists in drupal 7 as well as the current stable release 6.17.

To clarify the steps to reproduce this error, you must visit a uid that does not exist!
Since I believe uid 1 will always exist, the url mentioned above will not display the error.
Try visiting user/reset/0/1/1 instead.

I've attached a screenshot taken in a fresh install of 6.17 (The page looks similar in Drupal 7, of course).

I manually checked all the places where drupal_access_denied() is called in core, and I have confirmed that this is the only affected page. I've attached a patch... it simply adds a drupal_exit() after the offending function call.

This issue is also reported in the drupal 6.x queue here:
#653404: Use of drupal_access_denied inside drupal_get_form is not correct

When I replace the call with "return MENU_ACCESS_DENIED", I get the error:
Unsupported operand types in /var/shared/sites/drupal7/site/includes/form.inc on line 855.

I think this is because the page callback is drupal_get_form(), which calls user_pass_reset() indirectly. Other places in core use drupal_access_denied() followed by drupal_exit() to avoid this problem, as per the comments in common.inc:710:

 /**
 * Page callback functions wanting to report an "access denied" message should
 * return MENU_ACCESS_DENIED instead of calling drupal_access_denied(). However,
 * functions that are invoked in contexts where that return value might not
 * bubble up to menu_execute_active_handler() should call drupal_access_denied().
 */

Whoops, I made a small mistake generating the patch. Lets see if this does it.

Okay, now it should really work...I was making my patch from the module directory instead of the root.

Queued for testing.

er, if we are denying access we need to generate a 403 header and say so. this patch does neither, AFAICT

i see. thx.

The fix should be ok for Drupal 6, but for D7, could we please fix that properly?

A form callback should not contain anything that has a side effect. The form part of this function (which is buried down below a pile of action logic) should be moved to a separate callback. The main callback should use return MENU_ACCESS_DENIED, problem solved.

@Damien Tournoud: That is a good point. I think I have done what you wanted, could you please verify?

*Shameless bump* Or anyone else for that matter. It should simple to follow.

#1: add-drupal-exit-after-access-denied.patch queued for re-testing.

I'm very sorry for the delay. This felt completely out of my radar.

The patch looks pretty close to me. Two small concerns:

 /**
+ * Form builder; Allow the user to log in and change their password
+ * @ingroup forms
+ * @see user_pass_reset()
+ */

There should be a new line after the function summary.

+function user_pass_reset_form($form, $form_state, $uid, $timestamp, $hashed_pass, $action = NULL) {
+  $user = reset(user_load_multiple(array($uid), array('status' => '1')));

You can pass the fully loaded $user object from the menu callback instead of reloading it.

hi

i was creating a custom page using the same function and got the same double page display.

but..

function foo () {
...
   return drupal_access_denied (); <= do this works ok
}

function foo () {
...
   drupal_access_denied (); <= do this, gives double page display
}

@halmsx: Please do not change the version information.

This issue is to solve a bug in a specific form of Drupal core, that does not use the API properly.

If you want to return an Access denied from your menu callback, in Drupal 6, use either:

drupal_access_denied();
exit;

Or (preferably, because compatible with Drupal 7):

return MENU_ACCESS_DENIED;

None of the code examples you provided are actually correct.

apology. my bad. didnt noticed the version.

Damien, thanks for your note. I like how simple return MENU_ACCESS_DENIED; works, but I don't understand why drupal_access_denied() function even exists if it doesn't handle the return itself. Can you explain?

I am still experiencing this issue in drupal 7.16.

I think this is not a problem anymore. This commit: http://cgit.drupalcode.org/drupal/commit/modules/user/user.pages.inc?h=7..., solves it by calling "drupal_exit() after access denied.

drupal_access_denied();
drupal_exit();

.