Return 404: Not found on illegal node url

Silly me, the case of /node needs to be handled. Thats what the code should look like:

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 don't know about 5 and 6, but in the meantime: can somebody just apply the existing patch to the 4.7 branch? ;-)

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.

Yes, your patch handles the same problem, but I doubt it will be incorporated in the foreseeable future, since it proposes a rather big change. At least it won't be incorporated into 4.7.

Since everybody commenting here actually

• acknowledged the current behavior is a bug and
• my patch does solve this for the 4.7 branch,

I'd like to ask again for someone to apply it to this branch - please.

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

As m3avrck pointed out in #7, this patch cannot be applied to HEAD, since HEAD menu system works different in many ways. So what's the sense of porting this patch to HEAD, which means to completely rewrite it, just to port it back afterwards?

My point of view is that this patch solves a 4.7-specific critical bug in a 4.7-specific way. If there's are similar unsolved problem in HEAD or 5.0, so it may be. But this cannot be an argument to not fix the original bug!

I've investigated 5.0 and the patch does not apply to this, either, since a URL like http://drupal.org/node/ispamdrupalorg gets routed to node_default_page() without further argument checking. URLs with patter node/id however are mapped to node_page_view().

In 4.7 node_page() does handle both cases and does internal argument checking - and that is where my patch comes in: It modifies the argument checking case statement.

Therefore, for 5 or HEAD a completely different solution needs to be found.

So, to end moving responsibilities around, here is a proposal to fix this for 5.0. In HEAD however, menu system has changed again and seems to be in the flow, so I second the patch proposed by moshe in #10.

I suggest somebody applies the 4.7-fix, and afterwards we discuss the 5.0 patch ;-). Agree?

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

Since everyone agrees that 4.7-patch is RTBC, I marked this bug accordingly. I suggest to set it to 5.x.dev and status "code needs review" after the 4.7-patch has been applied.

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

Aargh! Please do not start moving responsibilities around, again! There is a solution of a critical bug for 4.7 around for 4 months, and it doesn't get applied because everyones wants it to be ported to higher versions first. This is not good.

Rob, please read a thread before changing status: porting this to 5.0, or HEAD means complete rewriting, backporting therefore obviously means rewriting the rewritten - or applying the actual patch, that everyone already agrees on.

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.

Oooookay. Let's get things straight.

1. There is a patch for 4.7 given in #3. This patch is ready to be committed
2. There are several patches available for 5.0, which need to be reviewed
3. There is a patch by chx that I don't know what version it applies to (6.4?) - but it is sparkling and shiny. This patch needs to be reviewed, too

However, since menu system has changed from 4.7 to 5.0 patches to either version are completely different. This means that talking about "backporting" is downright nonsense, since "porting" in this special case means "rewriting".

Now, since 5.0 and HEAD still need to be discussed but 4.7 is ready, and unfortunately the bug reporting system doesn't allow us to express this situation properly, I proposed in #27 to mark this bug as RTBC for 4.7, wait till it gets committed, and afterwards mark it as 5.0, "code needs review".

Can we agree on this, please? I fear this bug else will never get fixed for 4.7 but endlessly discussed.

Or maybe two bug should be filed, one for 4.7 and one for 5.0?

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:

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