Reproducible with clean Drupal (core-only) installed in a /drupal subdir, MySQL, no modules enabled, single article as content. Visiting rss.xml/whatever triggers

Warning: array_flip() expects parameter 1 to be array, string given in DrupalDefaultEntityController->load() (line 178 of /var/www/drupal/includes/entity.inc).

... followed by SQL error shortly. The single parameter is indeed a string, the part after rss.xml. Backtrace (after adding Devel module):

Warning: array_flip() expects parameter 1 to be array, string given in array_flip() (line 178 of /var/www/drupal/includes/entity.inc). =>
... (Array, 7 elements)
6: array_flip (Array, 4 elements)
5: load (Array, 7 elements)
4: entity_load (Array, 4 elements)
3: node_load_multiple (Array, 4 elements)
2: node_feed (Array, 2 elements)
1: call_user_func_array (Array, 4 elements)
0: menu_execute_active_handler (Array, 4 elements)
Krumo version 0.2.1a | http://krumo.sourceforge.net

Especially nasty to Wordpress-migrated sites, as users come looking for /feed/rss, /feed/atom etc (which after Path URL aliasing translate to /rss.xml/rss etc)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Status: Active » Postponed (maintainer needs more info)

What's the string?

droplet’s picture

Version: 7.10 » 8.x-dev
Status: Postponed (maintainer needs more info) » Active

@catch,

example.com/rss.xml/whatever
example.com/rss.xml/fewfewff
example.com/rss.xml/354yghtrhrgtr

drupdan3’s picture

As the original report states,

"The single parameter is indeed a string, the part after rss.xml"

Any value works: rss.xml/blah, rss.xml/bar, whatever.

But in any case, this is extremely easy to reproduce, and it seems to work regardless of what modules are (or are not) installed. Just try it.

drupdan3’s picture

Here (seems to be) an example, complete with SQL error shown to anonymous user:

http://d7.drupalexamples.info/rss.xml/sada

I should mention that if you alias /feed/* (using Pathauto or some other module) then of course the bug won't happen

swentel’s picture

Status: Active » Needs review
FileSize
530 bytes

The node_feed() function has $nids as an optional parameter. That string is passed as the first parameter (oh so lovely drupal menu system). Since $nids now is not false, node_load_multiple() receives a string and not an array of nids. Patch attaches changes the $nids === false check with an extra check to see if $nids is an array.

catch’s picture

Status: Needs review » Active

Now I get it.

This is very similar to #1359710: taxonomy_menu passes invalid arguments into taxonomy_form_term() and will need a similar test and fix.

swentel’s picture

Status: Active » Needs review
FileSize
1.18 KB

Makes sense to fix it that way. Here's the patch + test.

swentel’s picture

FileSize
630 bytes

Other patch with test only that should fail.

Status: Needs review » Needs work

The last submitted patch, 1410260-8-fail.patch, failed testing.

catch’s picture

Status: Needs work » Reviewed & tested by the community

Looks great. Marking RTBC, will commit in a few days if no objections.

drupdan3’s picture

Thanks, as a new Drupal user I have to say I'm impressed by the prompt response.

Will this apply to 7.x too?

catch’s picture

Issue tags: +Needs backport to D7

Yep, but it does need the tag.

droplet’s picture

Status: Reviewed & tested by the community » Needs work

I'm not well understand Feed specs. why not return "NOT FOUND" ??

swentel’s picture

Status: Needs work » Reviewed & tested by the community

@droplet, see the issue mentioned in #6 also.

droplet’s picture

Status: Reviewed & tested by the community » Needs work

@swentel,
#6 issue returns error 404. rss.xml/whatever should returns 404 by default. (drupal doesn't support rss.xml/nid)

if not, I suppose it returns empty items instead of all promoted articles

http://api.drupal.org/api/drupal/core--modules--node--node.module/functi...

xjm’s picture

Priority: Major » Normal

I don't think this is major, since we are adding gunky stuff to the end of the URL. It might even be minor except for the wordpress thing.

Edit: So I guess the question is whether we want to override the page callback to return a 404, or whether we want to ignore the additional arguments?

drupdan3’s picture

Throwing revealing SQL errors at the anonymous user, in a default installation, with a simple URI, isn't major? Oh well. I thought it might even be close to a security issue.

xjm’s picture

Exposed file system paths in error messages are not really a security issue. People should turn off error reporting to screen anyway on live sites. Besides, the paths are easily guessed anyway because they are the same for, well, most LAMP sites on the internet.

Also, a number of slightly similar issues were won't fixed previously. E.g. #1253858: Invalid type of $_POST['foo'] variables causes PHP warnings and notices when treated as strings. I'm not saying not to fix this particular case, but I don't think it's a major bug.

swentel’s picture

@droplet that issue does not return a 404. This patch simply uses the same technique. And with an invalid argument it simply returns the default feed.

Adding extra logic in the node_feed to validate is not the right place. An access callback would be a more gentle solution but this seems more overhead than what we need to fix.

xjm’s picture

If it's needed, my thought would be to add a separate page callback that either returns the 404 or calls node_feed(). Returning a 403 would be more convenient, but kinda incorrect.

swentel’s picture

You're right, a 403 would be weird here. However, if we introduce a page callback (not in favor btw), we should revert the other issue as well and also introduce a callback which on its turn calls drupal_get_form, otherwhise we're not being consistent in fixing these kind of bugs (which are not that a big deal anyway).

Also take for instance this: node/2/edit/sdaf or node/add/non-existing-node-type: these are an invalid path if we'd be consistent and should return 404 as well, but instead it returns the node edit screen, but nobody bothers because it's not throwing errors at you (see #1091068: Drupal returns 200 OK and content on expected 404 Not Found for the issue about that, because it is a potential SEO issue). So in the end, it's a broader problem.

So there are 2 options:

- either won't fix this and revert the other issue and open an issue against core menu.inc that should handle an invalid number arguments and return 404's. I know pwolalin once brought this up on IRC, but can't remember if there's already an issue for this.
- commit this one and go to sleep.

droplet’s picture

Status: Needs work » Needs review
FileSize
4.21 KB

wow. never known it. I'm expected ALL-NON-EXIST-PAGE return 404.

I'm working on the patch before your comment, Ignore me if not suitable for this case.

- return 404
- do not execute extra code when passing empty array() (it can be a separated issue)

droplet’s picture

The main reason I don't want it happen is....

rss.xml/apple << ~~~ user may expected that return apple stuff. on a busy site, every time you reload rss.xml/whatever it gives different results.

swentel’s picture

As I've said, I'm opposed against that solution with the page callback because it doesn't fix this consistently. If we want additional arguments to 404, we should fix it menu.inc, otherwhise we should open a couple of hundred issues against core. With that said, I'm unfollowing this issue.

swentel’s picture

As I've said, I'm opposed against that solution with the page callback because it doesn't fix this consistently. If we want additional arguments to 404, we should fix it menu.inc, otherwhise we should open a couple of hundred issues against core. With that said, I'm unfollowing this issue.

droplet’s picture

I'm not oppose against to @swentel patch this moment but can't do it case by case ?

similar function exist in Drupal core:

/**
 * Page callback wrapper for user_view().
 */
function user_view_page($account) {
  // An administrator may try to view a non-existent account,
  // so we give them a 404 (versus a 403 for non-admins).
  return is_object($account) ? user_view($account) : MENU_NOT_FOUND;
}

/user/24323243423433 returns 404 now

node/2/edit/fefe << it seems like an issue, return user theme instead admin theme

swentel’s picture

Note, there's actually another older issue about this, see #622426: rss.xml path throws errors if rss.xml/a - either this or the other needs to be marked as a dupe.

xjm’s picture

Closing #622426: rss.xml path throws errors if rss.xml/a as duplicate of this issue.

Let's reroll #7 to apply to HEAD. Let's also add an additional assertion after the existing one that tests for the expected content. When creating a new patch, upload a test-only patch first followed by the combined patch in the same comment.

mojzis’s picture

adding reroll and a test with an assertion

Status: Needs review » Needs work

The last submitted patch, 1410260-combined.patch, failed testing.

mojzis’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
901 bytes

error in diff, another attempt :)

pingers’s picture

FileSize
1.42 KB
+++ b/core/modules/node/lib/Drupal/node/Tests/NodeRSSContentTest.php
@@ -74,5 +74,9 @@ class NodeRSSContentTest extends NodeTestBase {
+    $this->assertText($rss_only_content, t('With an adress that doesnt make sense lets return default content.'));

"Ignore page arguments when delivering rss.xml." may be better and fixes spelling mistakes.

+++ b/core/modules/node/node.module
@@ -1898,6 +1898,9 @@ function node_menu() {
+    'page arguments' => array(FALSE, array()),

I expected just setting an empty array to resolve this bug, but this doesn't prevent path arguments being passed to the callback. Approach looks good to me.

valthebald’s picture

Patch does the trick (I get a bunch of errors with rss.xml/blabla without a patch, and feed page with applied patch), but hence the question - should rss.xml/blabla return 404 or 200?
If status 200 OK, I kinda vote for RTBC

Dave Reid’s picture

It should in fact return a 200 response.

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

I dare do this

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Hm. Why a 200, exactly? rss.xml/kajsdhkjashdkaksd is a bad path.

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

Because it's consistent with everything else in core.

Because node/1/kajsdhkjashdkaksd returns a 200.
Because admin/kajsdhkjashdkaksd returns a 200.

It's how our menus work by design and we shouldn't change that assumption here in this small bug report. That is an issue for #63390: Secure argument passing

greggles’s picture

If this were just for 7.x I could see going for a 200. But for 8.x I think a 404 makes way more sense. If #63390 makes this obsolete then this issue should be moved back to 7.x (though even in that case I think a 404 makes more sense).

You say it's how our menus work by design, but...what's the use case for that design? And is there some evidence that shows this was on purpose and not a byproduct of something else?

Dave Reid’s picture

Ok, let's start special casing this specific callback without any kind of plan. This is crazy.

mojzis’s picture

I dont want to be harsh or anything but ...
Dont we need to speed up ? With quite a few open issues, the initiatives not exactly ready and feature freeze round the corner ...
This is quite a simple problem. Currently its broken. The standard thus far has been returning 200. Thats what @swentel has done back in January. So we should probably just take it, be happy to have one error less and issue less.
And if we decide we want 404 in these cases, we can start a new issue, or keep working on #63390: Secure argument passing (its been around for 6 years and got into a state where apparently chx who started working on it doesnt want it anymore).

BTW :
i went to the "Get Involved with Core Sprint" in Munich and got this issue there. I enjoyed working on it, i enjoyed the atmosphere, but most of the issues i attempted to work on were similar - you follow the sprint instructions and than someone comes and says it should have been done otherwise. Was this meant as a test whether the newbies survive the postponing so usual in drupal queues ? This looks like a "from first sprint to burnout" cookbook :).
I think thats the "drupal philosophy" someone conveyed to me during the sprint : "repair it quickly and if there is another issue, then start a new one". Wouldnt it make sense ?

With respect to all the dubious work and hard decisions :) lets please commit this one and start a new thread on whether there should be 404 in such cases in general.

thanks :)

pingers’s picture

Can't agree more with the above - this patch solves the bug, not the general approach to handling bad requests in Drupal.

greggles’s picture

@Dave Reid - I've obviously upset you and I'm sorry. I'm trying to understand what the plan is (if we have a plan or not). I'm not trying to waste your time or derail progress, just trying to make sure of what the plan is and that we are heading in that direction consistently.

@mojzis - I hope you will continue to contribute. It's the great part and the sad part that issues in core get a lot of discussion about their merits.

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

@greggles the new router in Drupal 8 will very likely require explicit use of %menu_tail and not just pass the extra arguments to whichever is the last good known router path - mainly because the current behaviour is near impossible to emulate with the Symfony routing system, and because no-one's that keen on trying to maintain the current behaviour anyway.

Taxonomy module in D6 is an example of a module that used the extra arguments for things like the 1+2/all handling etc. fwiw I agree it's weird but also completely agree it should be handled separately.

This looks fine, so I've committed/pushed to 8.x, moving to 7.x for backport.

mojzis’s picture

Status: Patch (to be ported) » Needs review
FileSize
1.38 KB
763 bytes

backport attached

valthebald’s picture

Status: Needs review » Reviewed & tested by the community

Works in D7 as expected

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok. Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

Issue tags: +7.17 release notes

Adding this to the release notes and CHANGELOG.txt (since it touches a hook_menu() entry which people might be altering).