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)
Comment | File | Size | Author |
---|---|---|---|
#44 | 1410260-d7backport-test-only.patch | 763 bytes | mojzis |
#44 | 1410260-d7backport-combined.patch | 1.38 KB | mojzis |
#32 | 1410260-combined_32.patch | 1.42 KB | pingers |
#31 | 1410260-test-only.patch | 901 bytes | mojzis |
#31 | 1410260-combined.patch | 1.44 KB | mojzis |
Comments
Comment #1
catchWhat's the string?
Comment #2
droplet CreditAttribution: droplet commented@catch,
Comment #3
drupdan3 CreditAttribution: drupdan3 commentedAs 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.
Comment #4
drupdan3 CreditAttribution: drupdan3 commentedHere (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
Comment #5
swentel CreditAttribution: swentel commentedThe 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.
Comment #6
catchNow 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.
Comment #7
swentel CreditAttribution: swentel commentedMakes sense to fix it that way. Here's the patch + test.
Comment #8
swentel CreditAttribution: swentel commentedOther patch with test only that should fail.
Comment #10
catchLooks great. Marking RTBC, will commit in a few days if no objections.
Comment #11
drupdan3 CreditAttribution: drupdan3 commentedThanks, as a new Drupal user I have to say I'm impressed by the prompt response.
Will this apply to 7.x too?
Comment #12
catchYep, but it does need the tag.
Comment #13
droplet CreditAttribution: droplet commentedI'm not well understand Feed specs. why not return "NOT FOUND" ??
Comment #14
swentel CreditAttribution: swentel commented@droplet, see the issue mentioned in #6 also.
Comment #15
droplet CreditAttribution: droplet commented@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...
Comment #16
xjmI 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?
Comment #17
drupdan3 CreditAttribution: drupdan3 commentedThrowing 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.
Comment #18
xjmExposed 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.
Comment #19
swentel CreditAttribution: swentel commented@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.
Comment #20
xjmIf 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.Comment #21
swentel CreditAttribution: swentel commentedYou'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.
Comment #22
droplet CreditAttribution: droplet commentedwow. 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)
Comment #23
droplet CreditAttribution: droplet commentedThe 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.
Comment #24
swentel CreditAttribution: swentel commentedAs 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.
Comment #25
swentel CreditAttribution: swentel commentedAs 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.
Comment #26
droplet CreditAttribution: droplet commentedI'm not oppose against to @swentel patch this moment but can't do it case by case ?
similar function exist in Drupal core:
/user/24323243423433 returns 404 now
node/2/edit/fefe << it seems like an issue, return user theme instead admin theme
Comment #27
swentel CreditAttribution: swentel commentedNote, 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.
Comment #28
xjmClosing #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.
Comment #29
mojzis CreditAttribution: mojzis commentedadding reroll and a test with an assertion
Comment #31
mojzis CreditAttribution: mojzis commentederror in diff, another attempt :)
Comment #32
pingers CreditAttribution: pingers commented"Ignore page arguments when delivering rss.xml." may be better and fixes spelling mistakes.
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.
Comment #33
valthebaldPatch 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
Comment #34
Dave ReidIt should in fact return a 200 response.
Comment #35
valthebaldI dare do this
Comment #36
webchickHm. Why a 200, exactly? rss.xml/kajsdhkjashdkaksd is a bad path.
Comment #37
Dave ReidBecause 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
Comment #38
gregglesIf 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?
Comment #39
Dave ReidOk, let's start special casing this specific callback without any kind of plan. This is crazy.
Comment #40
mojzis CreditAttribution: mojzis commentedI 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 :)
Comment #41
pingers CreditAttribution: pingers commentedCan't agree more with the above - this patch solves the bug, not the general approach to handling bad requests in Drupal.
Comment #42
greggles@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.
Comment #43
catch@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.
Comment #44
mojzis CreditAttribution: mojzis commentedbackport attached
Comment #45
valthebaldWorks in D7 as expected
Comment #46
webchickOk. Committed and pushed to 7.x. Thanks!
Comment #48
David_Rothstein CreditAttribution: David_Rothstein commentedAdding this to the release notes and CHANGELOG.txt (since it touches a hook_menu() entry which people might be altering).