Return 404: Not found on illegal node url

I had this issue too.
I think the way drupal handles the second argument (node/$arg) is confusing for me, I tried fixing this in the node.module file but it's not an easy job, at least for me.
Dozens of dependancies I can't foresee.

looks good, moving to HEAD for more reviews.

Would like to see this backported to 4.7 as well--- once it gets committed.

Code looks good and makes sense to me.

This is a bug, if a page is not found, it should not redirect to another page claiming it still exists :-p

This should go into 5 and be backported to 4.7. However, this patch doesn't apply to HEAD because node_page() is gone now.

IMO, this is indeed a critical bug. It would be trivial to create a spamming solution using this. Easy enough to ruin a sites reputation in a search engine too.

Bumping this to 6 though, this would not be a trivial patch. This would be great to incorporate into the thinking going behind the new menu system proposed for 6 too.

i proposed a more generic solution for this at http://drupal.org/node/74347. i think this is a duplicate of that one, but i'll wait for someone else to change status.

i think this is a pretty good fix. this has to be applied to HEAD before it can be backported. moving ...

I think this might be a better general fix:

http://drupal.org/node/74347

So this might be a duplicate of that as Moshe pointed out above.

seems that i've made this too complicated. lets fix DRUPAL-5 with ted's patch and then 4.7 with gerd's patch. reassigning accordingly.

I agree with Moshe, lets get this applied for 4.7.x and 5.x and then concentrate on D6 maybe using the new menu system. Drupal is widely used for its good SEO features. This patch addresses a major bug with regards to SEO. I've seen Google and Yahoo! bots send out test checks on some of our client sites in the form of google+hash.html so they do carry out 404 tests on websites.

+1 to getting this committed asap.

I agree with the said comments :-)

However, I didn't write a patch yet :-p

Moshe, can you clarify *which* comments have the correct patches to be committed? I'm sure one of the core committers would ask this question, so might as well ask it for them :-p

oops again. gerd's most recent patches are the ones to go with. one for 5.0 and one for 4.7

gah, hate to do this, but http://drupal.org/files/issues/node404.5.0.patch from #15 contains drupalnot_found(). someone should re-roll that.

http://drupal.org/files/issues/node.module.404.4-7.patch is indeed RTBC.

oh, and +1 for fixing bugs in older versions, even if HEAD is in flux or bugs aren't present in newer versions. ;)

here you go. ;)

to repeat, http://drupal.org/files/issues/node.module.404.4-7.patch from #3 is also RTBC for 4.7.x.

cheers,
-derek

alas, whoops. previous patch (and therefore, my latest for 5.x) had syntax errors, too. hehe, guess i was a little hasty with that RTBC. ;)

i couldn't get empty() to play nicely with arg(). this seemed like the simplest approach. another pair of eyes, please. ;)

I'm not clear where this $validateargs comes from -- maybe a note about that in a comment? Should it be called $validate_args for clarity as well? Lowercase "true" should be "TRUE".

Wouldn't !isset(arg(1)) be faster than != as well?

FYI: i was just making a quick pass through the RTBC queue, and came upon this. i don't personally care much, and i have no time to work on it, which is why it's not Assigned to dww. ;) someone who cares will have to pick up the torch here. just wanted to make sure i hadn't created any false expectations...

cheers,
-derek

Should go on HEAD first and then be backported. Marking back to review to make sure this applies cleanly to HEAD.

Who is the everyone that agreed this is RTBC? The code is downright ugly. Why the attached patch is not enough?

I didn't have enough time to test chx's approach but once again,

lets get this applied for 4.7.x and 5.x and then concentrate on D6 maybe using the new menu system.

Someone please commit patch in #3 to 4.7.x branch. Then we'll move on to the 5.x discussions.

gerd, that's not the workflow we follow. We follow the sequence HEAD, 5, 4.7 in order to avoid inconsistencies.

When I try to access 'http://localhost/drupal-cvs/node/comment/reply/13' as per the original bug report, I get an access denied message. Could it be that this is no longer an issue for CVS HEAD?

Update: I also get an SQL error, I'm afraid. Not sure if that's related to the 'access denied' but I'm guessing it might be. We'll want to fix the 'access denied' message in CVS HEAD before we can properly test this. :/

"Yes Please" (subscribing)

I've found a bunch of hits from googlebot of the form:

/node/?page=&

because the default 'node' page has a pager at the bottom......

Err, that was supposed to say:
/node/[some string]?page=[large number]&[maybe other stuff]

This bug appears to be a duplicate of http://drupal.org/node/82764

To clarify the 5.x status:

• http://drupal.org/files/issues/node404.5.0.patch is fatal error broken
• http://drupal.org/files/issues/node404.5.0.patch_1.txt is fatal error broken
• http://drupal.org/files/issues/node404.5.0.patch_2.txt: relies on NULL == '' (non-triple-equals) and works fine for me.
• bug 82765's current patch http://drupal.org/files/issues/node-not-found.2.diff seems much more drupal-like to me. It also works for me.

Sorry. http://drupal.org/files/issues/node-not-found.2.diff doesn't work on non-numeric node paths.

So the only patch which works for me on 5.1 is http://drupal.org/files/issues/node404.5.0.patch_2.txt

(and to answer the question in #24, you could assign the return from arg() to a variable and test that variable with empty())

subscribing

This is a bit unwieldy. So, what is considered to be the best patch for CVS HEAD? :)

http://drupal.org/files/issues/node404.5.0.patch_2.txt

This patch appears to work for me as well. It appears to handle this problem on my site correctly (5.1, had to apply by hand and move the changes to line 2372). Will keep testing it, of course.

As per Dries in #35, I can't reproduce the original issue with today's HEAD.

If you type ?q=node/something, you get an access denied, plus the following errors:

    * warning: Invalid argument supplied for foreach() in /drupal/HEAD/drupal/modules/node/node.module on line 567.
    * notice: Undefined variable: cond in /drupal/HEAD/drupal/modules/node/node.module on line 571.
    * warning: implode() [function.implode]: Bad arguments. in /drupal/HEAD/drupal/modules/node/node.module on line 571.
    * user warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '' at line 1 query: SELECT n.nid, n.vid, n.type, n.status, n.created, n.changed, n.comment, n.promote, n.sticky, r.timestamp AS revision_timestamp, r.title, r.body, r.teaser, r.log, r.format, u.uid, u.name, u.picture, u.data FROM node n INNER JOIN users u ON u.uid = n.uid INNER JOIN node_revisions r ON r.vid = n.vid WHERE in /drupal/HEAD/drupal/includes/database.mysqli.inc on line 151.

If you type a URL that references a non existent node such as q=comment/reply/123456, you get a clean access denied.

If you type an invalid URL such as q=node/comment/reply/123456, you get the access denied plus the above error.

So, what is the problem we are trying to solve? Doesn't exist anymore?

I went ahead and reviewed and committed #30 to Drupal 5 at chx's advice. In the future, I think multiple issues for different versions of Drupal with significantly different patches is okay.

Gerhard, #3 is OK and HEAD needs a totally different patch (menu.inc level, I am thinking allowing callback arguments to be FALSE not an array in cases like this).

Applied #3 to 4.7.
moving back to HEAD

Where's the 6.x patch? Fixed in 4.7, 5.x but not 6.x? *doh*

As far as I can see this is not an issue in 6.x.

indeed. if node/dfghjk is not defined that any path beginning with that will come back with 404 thanks to node_load returning FALSE thus translate returning FALSE thus menu_get_item returning FALSE

ver 6.x got this issue as when the link as below

valid url: http://www.example.com/node/4
invalid url: http//www.example.com/node/4/invalid

where the invalid show the same content as valid url. it show direct to 404:page not found and will cause duplicate content

i search the solution for whole day but still cannot find it...anyone can help here? thanks!

I wonder why this issue is ignored.
On 6.17 I see the following:
example.com/"termalias"/"something-malformed" shows the content of
example.com/"termalias"
As a result we can have several urls pointing Google to the term's page if the url of some node of this term (which is example.com/"termalias"/"date"/"title") is changed and/or some other node gets deleted.
Why does Drupal in this case show the content of some url it has found while it should just redirect to this url?

I will keep bumping this issue till I find the solution.

@looney_toons: Please don't do that. It's just annoying and adds noise. Only bump the issue if you have something useful to contribute to it. Thanks.

With boost module enabled this can also lead to wasting disk space if a lot of malformed URLs are accessed forcing boost to generate static files for these URLs.

This is awkwardly from seo point it makes the frontpage looks as duplicated content
also the crawlers gets confused but keep coming back butt for no reason for pages that not exist anymore or never have been.
I know setup a disallow in robot.txt may help, but this not solves it.

This is the way the menu router works; some modules actually rely on this behavior via the arg() function. Given that, it probably can't be changed until Drupal 8. Also note this isn't the only way to create duplicate URLs, you can add arbitrary query strings like node/1?foo and the query string will be ignored.

The real solution is to avoid outputting incorrect links in the first place.