Posted by forngren on September 18, 2006 at 1:12pm
12 followers
| Project: | Drupal core |
| Version: | 6.1 |
| Component: | base system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| 40x_loop_bug.patch.txt | 1.03 KB | Ignored: Check issue status. | None | None |
Comments
#1
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
#3
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.
#4
Strange patch. I'd like us to ponder on this some more to see if we can come up with something more elegant.
#5
@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
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
Fixed missing t(). I don't think it will get any slimmer/more drupalish.
#8
#9
Updated version, same file still applies.
#10
Updated to better match chxs menu patch.
#11
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
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
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
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
I'd rather handle it in drupal_access_denied() and drupal_not_found()
Patch against the latest drupal6 tarball
#16
#17
Updated/tested patch.
@drumm: I don't think this patch requires any additional comments.
#18
Just a little bit friendlier.
#19
Free issue! $0
(please, push this into core)
#20
reroll
#21
Excellent, ran into this a couple of times and it was very annoying. Tested, works great.
#22
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
Backport. Works in my tests.
#24
Committed to 5.x.
#25
Automatically closed -- issue fixed for two weeks with no activity.
#26
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
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
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
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
How is one to deal with this currently existing problem?
#31
that's a different issue: #238508: custom 403 page not displayed
#32
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
Forgot to mention, this is on the 5.10 branch.