If someone sets 'Default 403 (access denied) page' / 'Default 404 (not found) page' to a path that doesn't exist or user doesn't have permissions to view, the body/content will be '2' or '3' which is a bit confusing.

This patch fixes those issues.

CommentFileSizeAuthor
#23 20080112.D5-core-40x.patch1.4 KBbart jansens
#20 40x-loop_1-84754.patch1.03 KBpasqualle
#18 84754-40x-loop_0.patch1019 bytesforngren
#17 84754-40x-loop.patch866 bytesforngren
#15 40x_bug.patch813 bytesmsameer
#10 40x_loop_bug.patch971 bytesforngren
#7 40x_loop_bug.patch_1.txt1.03 KBforngren
#3 40x_loop_bug.patch_0.txt1.08 KBforngren
40x_loop_bug.patch.txt1.03 KBforngren

Comments

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Needs review

Is it possible to hit the third case where these are used, MENU_SITE_OFFLINE?

Please leave as 'code needs review' and wait until a second person has given a positive review before setting 'ready to commit.'

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: x.y.z » 6.x-dev
forngren’s picture

forngren commented
Version: 6.x-dev » 5.0-rc1
StatusFileSize
new 40x_loop_bug.patch_0.txt1.08 KB

Howto reproduce:

* Go to /admin/settings/error-reporting
* Set Default 403 (access denied) page and/or Default 404 (not found) page to path does not exist (e.g. /idonotexist) or is unaccessible by visitors (e.g. /admin).
* Then visit an url that does not exit or is denied by the current user.
* It will display a blank, templated page containing either "2" or "3".

Site maintenance mode (i.e. MENU_SITE_OFFLINE) does not affect this behavior.

dries’s picture

dries commented
Status: Needs review » Needs work

Strange patch. I'd like us to ponder on this some more to see if we can come up with something more elegant.

forngren’s picture

forngren commented

@Dries: Sure, but I'm not sure how to accomplish something slimmer with the current menu system.

If someone has an idea; please post it even if you think it's rubbish. Thanks.

forngren’s picture

forngren commented
Status: Needs work » Needs review

This goes pretty deep in the hierarchy:

index.php

// $Id: index.php,v 1.91 2006/12/12 09:32:18 unconed Exp $

/**
 * @file
 * The PHP page that serves all page requests on a Drupal installation.
 *
 * The routines here dispatch control to the appropriate handler, which then
 * prints the appropriate page.
 */

require_once './includes/bootstrap.inc';
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);

$return = menu_execute_active_handler();

// Menu status constants are integers; page content is a string.
if (is_int($return)) {
  switch ($return) {
    case MENU_NOT_FOUND:
      drupal_not_found();
      break;
    case MENU_ACCESS_DENIED:
      drupal_access_denied();
      break;
    case MENU_SITE_OFFLINE:
      drupal_site_offline();
      break;
  }
}
elseif (isset($return)) {
  // Print any value (including an empty string) except NULL or undefined:
  print theme('page', $return);

}

drupal_page_footer();

It looks like we'll have to do it in drupal_not_found() and drupal_access_denied(). Actually I want to remove those functions and rewrite them from bottom up, but that's another story.

Still, I'm not sure how to polish this patch even more. In 6.x I will try to rewrite the 40x-handling, but that will maybe require some API-changes. For now a just want to squeeze this bug and make 5.x as stable as possible.

Setting this PCNR to get some more attention. The patch works (for me atleast), the issue is how to implement a/this solution.

forngren’s picture

forngren commented
StatusFileSize
new 40x_loop_bug.patch_1.txt1.03 KB

Fixed missing t(). I don't think it will get any slimmer/more drupalish.

drumm’s picture

drumm
he/him
Location NY, US
commented
Version: 5.0-rc1 » 5.x-dev
forngren’s picture

forngren commented
Version: 5.x-dev » 6.x-dev

Updated version, same file still applies.

forngren’s picture

forngren commented
StatusFileSize
new 40x_loop_bug.patch971 bytes

Updated to better match chxs menu patch.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

The index.php fix predates the menu patch -- this even needs to be ported back to 5.x I believe. It takes a MENU_NOT_FOUND (or denied) from the menu_execute_active_handler and tells the user. Nice, very small change.

chx’s picture

chx commented
Version: 6.x-dev » 5.x-dev

I forgot to set version. 6.x needs something similar but not exactly -- 40x is a bit broken in HEAD now, that needs to be fixed, in general.

chx’s picture

chx commented

I forgot to set version. 6.x needs something similar but not exactly -- 40x is a bit broken in HEAD now, that needs to be fixed, in general.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Reviewed & tested by the community » Needs work

Looks okay. Chx, if you have any review of this code that would be great.

I would like to see a comment explaining what is happening in the code.

msameer’s picture

msameer commented
Status: Needs work » Needs review
StatusFileSize
new 40x_bug.patch813 bytes

I'd rather handle it in drupal_access_denied() and drupal_not_found()

Patch against the latest drupal6 tarball

msameer’s picture

msameer commented
Version: 5.x-dev » 6.x-dev
forngren’s picture

forngren commented
StatusFileSize
new 84754-40x-loop.patch866 bytes

Updated/tested patch.

@drumm: I don't think this patch requires any additional comments.

forngren’s picture

forngren commented
StatusFileSize
new 84754-40x-loop_0.patch1019 bytes

Just a little bit friendlier.

forngren’s picture

forngren commented
Assigned: forngren » Unassigned

Free issue! $0

(please, push this into core)

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
StatusFileSize
new 40x-loop_1-84754.patch1.03 KB

reroll

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Excellent, ran into this a couple of times and it was very annoying. Tested, works great.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Version: 6.x-dev » 5.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Indeed, I also ran into this quite a few times. Thanks for the patch. Using the constants it does show what happens, so committed.

Seems like this was not fixed in 5.x yet, so setting for backport.

bart jansens’s picture

bart jansens commented
Status: Patch (to be ported) » Needs review
StatusFileSize
new 20080112.D5-core-40x.patch1.4 KB

Backport. Works in my tests.

drumm’s picture

drumm
he/him
Location NY, US
commented
Status: Needs review » Fixed

Committed to 5.x.

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

davidsimpson’s picture

davidsimpson commented
Version: 5.x-dev » 6.1
Status: Closed (fixed) » Needs review

Hello,

I just downloaded the latest stable release and it seems to have this patch already applied to it, however I still have the problem that is described here. Meaning that if I restrict access to content for anonymous users, and add a customized 403 page, it still writes the typical "Access Denied" page.

Any ideas as to why this may be?

Thank you all,

David

catch’s picture

catch
he/him
Primary language English
commented

davidsimpson: I'm pretty sure that's exactly what the original patch was supposed to do - you used to get '2' instead of 'Access Denied', now you get 'Access Denied' instead of the node which the user doesn't have access to - because it'd be really hacky to bypass the node_access system for this.

catch’s picture

catch
he/him
Primary language English
commented
Version: 6.1 » 5.x-dev
Status: Needs review » Closed (fixed)

however, if you want to open a new feature request against 7.x for this that'd be fine. For now I'm setting this back to it's original status.

publicme’s picture

publicme commented
Version: 6.1 » 5.x-dev

I have this same '403' problem in 6.1. Disable content access for anonymous users, with reference to a custom "access denied" page, where I'd like to instruct visitors to registar an account to view the content, and the custom page does not appear. Reading this thread, I'm left clueless as to what to try next. It sounds like it's resolved, then it sounds like it's not resolved.

publicme’s picture

publicme commented
Version: 5.x-dev » 6.1

How is one to deal with this currently existing problem?

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

Meaning that if I restrict access to content for anonymous users, and add a customized 403 page, it still writes the typical "Access Denied" page.

that's a different issue: #238508: custom 403 page not displayed

Mattias-J’s picture

Mattias-J commented
Version: 5.x-dev » 6.1

I get the same confusion...

-  if (empty($return)) {
+  if (empty($return) || $return == MENU_NOT_FOUND || $return == MENU_ACCESS_DENIED) {
     drupal_set_title(t('Access denied'));
+    menu_set_active_item('');
     $return = t('You are not authorized to access this page.');
   }

This part of the patch is the culprit. With this part, if an anonymous user tries to access some restricted content they only get "You are not authorized to access this page."
without this patch they are redirected to the drupal 403 page. There has to be a mistake here...

Mattias-J’s picture

Mattias-J commented

Forgot to mention, this is on the 5.10 branch.