Fix of non-existent/denied 403/404 pages

forngren - September 18, 2006 - 13:12
Project:Drupal
Version:6.1
Component:base system
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed
Description

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.

AttachmentSize
40x_loop_bug.patch.txt1.03 KB

#1

drumm - October 3, 2006 - 03:08
Status:patch (reviewed & tested by the community)» patch (code 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.'

#2

drumm - December 27, 2006 - 13:21
Version:x.y.z» 6.x-dev

#3

forngren - December 28, 2006 - 13:33
Version:6.x-dev» 5.0-rc1

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.

AttachmentSize
40x_loop_bug.patch_0.txt1.08 KB

#4

Dries - December 28, 2006 - 15:33
Status:patch (code needs review)» patch (code 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.

#5

forngren - December 28, 2006 - 15:43

@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.

#6

forngren - December 28, 2006 - 16:29
Status:patch (code needs work)» patch (code needs review)

This goes pretty deep in the hierarchy:

index.php

<?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.

#7

forngren - January 5, 2007 - 20:33

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

AttachmentSize
40x_loop_bug.patch_1.txt1.03 KB

#8

drumm - January 11, 2007 - 07:45
Version:5.0-rc1» 5.x-dev

#9

forngren - February 4, 2007 - 16:41
Version:5.x-dev» 6.x-dev

Updated version, same file still applies.

#10

forngren - February 4, 2007 - 18:08

Updated to better match chxs menu patch.

AttachmentSize
40x_loop_bug.patch971 bytes

#11

chx - February 4, 2007 - 18:14
Status:patch (code needs review)» patch (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.

#12

chx - February 4, 2007 - 18:26
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.

#13

chx - February 4, 2007 - 18:26

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.

#14

drumm - February 7, 2007 - 07:32
Status:patch (reviewed & tested by the community)» patch (code 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.

#15

msameer - May 9, 2007 - 14:00
Status:patch (code needs work)» patch (code needs review)

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

Patch against the latest drupal6 tarball

AttachmentSize
40x_bug.patch813 bytes

#16

msameer - May 9, 2007 - 14:02
Version:5.x-dev» 6.x-dev

#17

forngren - June 1, 2007 - 19:23

Updated/tested patch.

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

AttachmentSize
84754-40x-loop.patch866 bytes

#18

forngren - June 1, 2007 - 19:36

Just a little bit friendlier.

AttachmentSize
84754-40x-loop_0.patch1019 bytes

#19

forngren - September 20, 2007 - 12:36
Assigned to:forngren» Anonymous

Free issue! $0

(please, push this into core)

#20

Pasqualle - November 12, 2007 - 20:52

reroll

AttachmentSize
40x-loop_1-84754.patch1.03 KB

#21

catch - November 12, 2007 - 21:55
Status:patch (code needs review)» patch (reviewed & tested by the community)

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

#22

Gábor Hojtsy - November 12, 2007 - 22:13
Version:6.x-dev» 5.x-dev
Status:patch (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.

#23

Bart Jansens - January 12, 2008 - 16:17
Status:patch (to be ported)» patch (code needs review)

Backport. Works in my tests.

AttachmentSize
20080112.D5-core-40x.patch1.4 KB

#24

drumm - February 11, 2008 - 05:32
Status:patch (code needs review)» fixed

Committed to 5.x.

#25

Anonymous (not verified) - February 25, 2008 - 05:41
Status:fixed» closed

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

#26

davidsimpson - March 10, 2008 - 18:48
Version:5.x-dev» 6.1
Status:closed» patch (code 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

#27

catch - March 10, 2008 - 22:24

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.

#28

catch - March 10, 2008 - 22:25
Version:6.1» 5.x-dev
Status:patch (code needs review)» closed

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.

#29

publicme - March 25, 2008 - 16:15

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.

#30

publicme - March 24, 2008 - 15:31
Version:5.x-dev» 6.1

How is one to deal with this currently existing problem?

#31

Pasqualle - March 25, 2008 - 14:10

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

#32

Mattias-J - September 2, 2008 - 12:14

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...

#33

Mattias-J - September 2, 2008 - 12:15

Forgot to mention, this is on the 5.10 branch.

 
 

Drupal is a registered trademark of Dries Buytaert.