Deleted forum nodes return 403 instead of 404

It happens on any deleted forum node.

I think you want a URL, not a path, which would not help anything. I have sent you some information via your contact form as my client would prefer not to have this problem posted publicly.

Thanks for your help Salvis.

Yes, the site is using pathauto. Other relevant path-related modules are Path Redirect, Global Redirect, and Taxonomy Redirect.

There are no orphaned path aliases or redirections that have been left over after deleting the node.

The records in the node_access table are being deleted when the node is deleted. There were 3 forum_access records and 1 acl record on an example I tested. All were gone after deleting the node.

Clearing caches makes no difference to the results.

However I think you are onto something with the path aliases, because if I delete a node then try to access it using the path alias I get a 403, however if I use node/5678 as the path I get a 404 as you suggested. I am attempting to debug this again though I find that debugging tools for php generally suck and make things very difficult.

Ok, I have narrowed it to what I think is the problem.

The access check is definitely failing at _forum_access_forum_access_callback. Below is a backtrace showing that the function is being called not with a numerical $tid (topic id or nid I assume) but with the string 'container' which is the forum container string part of the deleted forum topic path (you can see it further down the call stack).

This is the path of the deleted node that is being called for this call stack:
/forum/container/forum-specifier/15187/forum-topic-name

Looking at the call stack it appeared to me to be triggered by the cache_exclude module in the _init phase. This is used by the site to stop a handful of pages from being cached - however the forums are not part of this. So I disabled the module but the problem persisted and was triggered by another module, this time special_menu_items_init. Basically it is happening with any module that triggers an access check in the init phase.

boolean false

array
  0 => 
    array
      'function' => string '_forum_access_forum_access_callback'
      'args' => 
        array
          0 => &string 'container'
  1 => 
    array
      'file' => string '/includes/menu.inc'
      'line' => int 454
      'function' => string 'call_user_func_array'
      'args' => 
        array
          0 => &string '_forum_access_forum_access_callback'
          1 => &
            array
              0 => string 'container'
  2 => 
    array
      'file' => string '/includes/menu.inc'
      'line' => int 583
      'function' => string '_menu_check_access'
      'args' => 
        array
          0 => &
            array
              'path' => string 'forum'
              'load_functions' => string ''
              'to_arg_functions' => string ''
              'access_callback' => string '_forum_access_forum_access_callback'
              'access_arguments' => string 'a:1:{i:0;i:1;}'
              'page_callback' => string 'advanced_forum_page'
              'page_arguments' => string 'a:0:{}'
              'fit' => string '1'
              'number_parts' => string '1'
              'tab_parent' => string ''
              'tab_root' => string 'forum'
              'title' => string 'Forums'
              'title_callback' => string 't'
              'title_arguments' => string ''
              'type' => string '20'
              'block_callback' => string ''
              'description' => string ''
              'position' => string ''
              'weight' => string '0'
              'file' => string 'modules/forum/forum.pages.inc'
              'href' => string 'forum'
              'options' => 
                array
                  empty
          1 => &
            array
              0 => string 'forum'
              1 => string 'container'
              2 => string 'forum-specifier'
              3 => string '15187'
              4 => string 'forum-topic-name'
  3 => 
    array
      'file' => string '/includes/menu.inc'
      'line' => int 318
      'function' => string '_menu_translate'
      'args' => 
        array
          0 => &
            array
              'path' => string 'forum'
              'load_functions' => string ''
              'to_arg_functions' => string ''
              'access_callback' => string '_forum_access_forum_access_callback'
              'access_arguments' => string 'a:1:{i:0;i:1;}'
              'page_callback' => string 'advanced_forum_page'
              'page_arguments' => string 'a:0:{}'
              'fit' => string '1'
              'number_parts' => string '1'
              'tab_parent' => string ''
              'tab_root' => string 'forum'
              'title' => string 'Forums'
              'title_callback' => string 't'
              'title_arguments' => string ''
              'type' => string '20'
              'block_callback' => string ''
              'description' => string ''
              'position' => string ''
              'weight' => string '0'
              'file' => string 'modules/forum/forum.pages.inc'
              'href' => string 'forum'
              'options' => 
                array
                  empty
          1 => &
            array
              0 => string 'forum'
              1 => string 'container'
              2 => string 'forum-specifier'
              3 => string '15187'
              4 => string 'forum-topic-name'
  4 => 
    array
      'file' => string '/includes/menu.inc'
      'line' => int 703
      'function' => string 'menu_get_item'
      'args' => 
        array
          0 => &null
  5 => 
    array
      'file' => string '/sites/all/modules/cacheexclude/cacheexclude.module'
      'line' => int 86
      'function' => string 'menu_get_object'
      'args' => 
        array
          0 => &string 'node'
  6 => 
    array
      'function' => string 'cacheexclude_init'
      'args' => 
        array
          empty
  7 => 
    array
      'file' => string '/includes/module.inc'
      'line' => int 497
      'function' => string 'call_user_func_array'
      'args' => 
        array
          0 => &string 'cacheexclude_init'
          1 => &
            array
              empty
  8 => 
    array
      'file' => string '/includes/common.inc'
      'line' => int 2730
      'function' => string 'module_invoke_all'
      'args' => 
        array
          0 => &string 'init'
  9 => 
    array
      'file' => string '/includes/bootstrap.inc'
      'line' => int 1206
      'function' => string '_drupal_bootstrap_full'
      'args' => 
        array
          empty
  10 => 
    array
      'file' => string '/includes/bootstrap.inc'
      'line' => int 1113
      'function' => string '_drupal_bootstrap'
      'args' => 
        array
          0 => &int 8
  11 => 
    array
      'file' => string '/index.php'
      'line' => int 15
      'function' => string 'drupal_bootstrap'
      'args' => 
        array
          0 => &int 8

We know for sure that as the node does not exist, _forum_access_forum_access_callback will never receive a valid $tid when called in this situation. I don't know how or why _menu_check_access ends up using the "container" part of the path but I assume it is because the callback is set to use arg[1] which is normally the nid in node/[1234]. However in the context of the invalid path when it is split into an array, "container" becomes the 2nd element. Again all assumption on that. However it does fit with the observation that the 404 is returned properly if the path alias is not used because the 2nd element in the path is then a node id.

If I log the contents of the $arguments passed to every access callback on a page load I can see that more often than not, it is called with a string parameter (usually an access permission name).

So changing this:

function _forum_access_forum_access_callback($tid = NULL) {
  return (!$tid && _forum_access_access_any_forum()) || forum_access_access($tid, 'view');
}

to this fixes the problem for me.

function _forum_access_forum_access_callback($tid = NULL) {
  if (is_numeric($tid)) {
    return forum_access_access($tid, 'view');
  }
  return _forum_access_access_any_forum();
}

I'll leave it up to your judgement as to whether that is going to cause some other problem elsewhere. I still have hidden admin-only forums that anonymous users and those without the required permissions cannot access, so the module seems to be functioning properly.

Sorry I should have made that clear in my post, rather than leaving you to interpret that from the stack trace. I have edited it to include the path I was testing on which was:

/forum/container/forum-specifier/15187/forum-topic-name

The forums are organised more or less like this, and pathauto generates a path alias accordingly.

forum
--> container 1
-->--> specific forum 1
-->--> specific forum 2
--> container 2
-->--> specific forum 3
-->--> specific forum 4

"15187" in the path happens to be a node id in the path which is generated by pathauto.

What happens if you comment out the entire forum_access_menu_alter() function and clear the caches? This will remove _forum_access_forum_access_callback() -- you can comment it out as well if you want.

The exact same behaviour as disabling the entire module.

If I comment it out, it returns a 404.
If I re-enable the original code, it returns a 403.
If I use the patched code with is_numeric, it returns a 404.

I think I'm beginning to understand what the problem is: your choice of the alias pattern
http://www.example.com/forum/discussion/general-chat/something-i-just-ma...
interferes with the expectation that forum is a path controlled by core.

When you write "container" I'm assuming you really mean "discussion". I don't want to know your domain, but we need to be specific about the paths that we talk about!

The choice of using forum in the alias pattern makes sense because when you use core forums, the forums are found at /forum. So I would imagine that most people would have their forum containers and topics in paths "underneath" that alias.

"Container" means the group of forums as explained above.

At this point I'm pretty sure that
http://www.example.com/forum/discussion/general-chat/deleted-node
and
http://www.example.com/forum/discussion/general-chat/never-existed-node
behave exactly the same.

Yes they do. With the current code, they both return a 403. With the patched code they return a 404 for me.

If the URL does not exist, then Pathalias steps back and passes the path on to core (and on to FA), which assumes that arg(1) is a TID. This is standard core behavior: you can take any existing core path and add additional components, and core will continue to give you the same page. So,
http://www.example.com/forum/discussion/general-chat/deleted-node
is the same as
http://www.example.com/forum/discussion
and _forum_access_forum_access_callback() says that you don't have access to the forum with the TID "discussion".

Yes, but the reason this fails is that the code is assuming that "discussion" is a number. See around line 540 in forum_access.module where it uses %d:

$result = db_result(db_query("SELECT tid FROM {forum_access} WHERE rid IN (". db_placeholders($roles) .") AND grant_". $type ." = 1 AND tid = %d", array_merge($roles, array($tid))));

Unfortunately you cannot assume that arg(1) is a number for reasons I explained above. It could be the string "container" from my path, or "discussion" in the example you provided. If it isn't a number, _forum_access_forum_access_callback returns false and prevents core from returning the 404 which it should. Technically it could even return true by accident if casting the string to a digit (which I guess would be 1) and there was a forum topic with node id of 1 that anonymous users had access to.

Checking is_numeric() will catch this, but it won't help if you decide to use a pathalias pattern such as
http://www.example.com/forum/2013/general-chat/something-i-just-made-up-now

It took me a while to see what you were getting at here, but yes if you had a digit in the arg(1) position you would be relying on that digit not existing as a forum node id. It seems that you need to know whether a numeric $tid refers to an actual node id or if it is just the 2nd segment of the path being requested.

I just did a quick check and if the topic node exists, arg(0) will always be "node", regardless of the path alias requested. However if the topic does not exist arg(0) will be the first segment in the path, such as "forum" in my example. Therefore this code should be more robust:

function _forum_access_forum_access_callback($tid = NULL) {
  if (is_numeric($tid) && arg(0) == 'node') {
    return forum_access_access($tid, 'view');
  }
  return _forum_access_access_any_forum();
}

However I can still see a couple of situations where this may not be good enough. If the user requests a non existent node path such as node/9876 (where 9876 is not a valid node id). This would happen all the time for deleted nodes on a site not using path aliases. Also it could be a problem if the site used some sort of weird path alias that overrides the default node/1234 situation. eg: /node/12345/forum/topic

The check for 'node' doesn't add any value. If arg(0) were 'node', _forum_access_forum_access_callback() would get called at all.

I think you meant to say it would NOT get called? I wrote the previous one as a suggestion without testing and I see what you mean now.

i think what you are saying is the bogus path being called that starts with /forum causes the access check to be called. When this happens Drupal passes on all the extraneous path information as "forum" is the only matching registered path handler, which in turn causes $tid to be non-numeric due to the path specification. The is_numeric check on my client's site works because failure causes _forum_access_access_any_forum() to be called which returns TRUE as anonymous users are allowed to browse the forums. This in turn allows Drupal to return a 404. If this is the case then if anonymous users were not permitted to view the forums it would still return a 403 for bogus paths as _forum_access_access_any_forum() would return FALSE. So clearly that is another reason why that solution isn't going to work for everyone.

I assume _forum_access_access_any_forum() was designed to check the base "/forum" path only.

This suggest that you must have the forum structure and paths set up something like this if you want both this module to work and don't want to have the 403 problem:

forum index -> /forum
- forum container1 -> /forum/container1
-- forum1 -> /forum/container1/forum1
--- forum topic 1 -> /some/other/path/that/doesnt/start/with/forum
-- forum2 -> /forum/container1/forum2
-- forum3 -> /forum/container1/forum3

Its a pretty serious problem that you cannot choose a path alias for your forum topics starting with "forum" without causing this problem as I expect lots of people to have done. The is_numeric has been working well in my situation for a while now so I'm happy to leave it there but I'm at a loss as to how to come up with a solution that works for everyone. Perhaps checking if the requested path exists before doing the access check?

This is an older issue from another module with a similar problem.
#152558: Acid free messes up 'Page not found'

Ok thanks for your help. I'm sticking with the patch I posted earlier as it works perfectly in my situation and it might help someone else if they come across this one.