rss.xml path throws errors if rss.xml/a [#622426]

If we add an extra arg to rss.xml - e.g. ?q=rss.xml/a, we get big flaming errors:

# Warning: array_flip(): The argument should be an array in DrupalDefaultEntityController->load() (line 109 of /drupal-7/includes/entity.inc).
# Warning: Invalid argument supplied for foreach() in DatabaseCondition->compile() (line 1271 of /drupal-7/includes/database/query.inc).
PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 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 2: SELECT revision.timestamp AS revision_timestamp, revision.uid AS revision_uid, revision.vid AS vid, revision.title AS title, revision.log AS log, revision.status AS status, revision.comment AS comment, revision.promote AS promote, revision.sticky AS sticky, base.nid AS nid, base.type AS type, base.language AS language, base.uid AS uid, base.created AS created, base.changed AS changed, base.tnid AS tnid, base.translate AS translate FROM {node} base INNER JOIN {node_revision} revision ON revision.vid = base.vid WHERE (base.nid IN ()) ; Array ( )  in DrupalDefaultEntityController->load() (line 127 of /drupal-7/includes/entity.inc).

and no output.

Comment	File	Size	Author
#13	622426-optionA.patch	1.58 KB	mr.baileys
#13
#13	622426-optionB.patch	1.96 KB	mr.baileys
#13
#5	622426-node-rss-errors-5t.patch	1.55 KB	pwolanin
#5
#4	622426-drupal-node-feed-argument-validation.patch	477 bytes	voxpelli
#4
#3	622426-node-rss-errors-D7.patch	2.42 KB	Dave Reid
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

chx CreditAttribution: chx commented 4 December 2009 at 17:07

Status:

Active

» Closed (won't fix)

garbage in, garbage out. That's just good.

Comment #2

pwolanin CreditAttribution: pwolanin commented 4 December 2009 at 17:10

Status:

Closed (won't fix)

» Active

I disagree - user input triggering a SQL error is a form of SQL injection

Comment #3

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 4 December 2009 at 17:56

Status:

Active

» Needs review

File	Size
622426-node-rss-errors-D7.patch	2.42 KB

Comment #4

voxpelli CreditAttribution: voxpelli commented 4 December 2009 at 18:22

Priority:

Critical

» Normal

File	Size
622426-drupal-node-feed-argument-validation.patch	477 bytes

An alternate patch to Dave Reids - should we really return anything but a 404 for a subpath to rss.xml?

Also - can't see why this is a critical issue - no SQL injection possible.

Comment #5

pwolanin CreditAttribution: pwolanin commented 4 December 2009 at 21:53

File	Size
622426-node-rss-errors-5t.patch	1.55 KB

Many places in Drupal extra path parts are just ignored - I'd expect that instead of a 404.

Here's the no-WS change version of Dave's patch plus regression tests.

Comment #6

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 4 December 2009 at 21:56

Can someone explain why we need to fix this this way when we don't have to add those weird page arguments to any other menu callback? Should we not address this in node_feed()?

Comment #7

pwolanin CreditAttribution: pwolanin commented 4 December 2009 at 23:03

@webchick - the issue is that we are using an API function directly as the page callback without wrapping it - so we are essentially just provding correct/safe arguments rather than allowing the menu system to pass in arbitrary arguments. The alternative is to do more parameter checking in node_feed, but this seems simpler.

A couple other existing examples from core:

  $items['system/files'] = array(
    'title' => 'File download',
    'page callback' => 'file_download',
    'page arguments' => array('private'),
    'access callback' => TRUE,
    'type' => MENU_CALLBACK,
  );

  $items['admin/config/search/clean-urls/check'] = array(
    'title' => 'Clean URL check',
    'page callback' => 'drupal_json_output',
    'page arguments' => array(array('status' => TRUE)),
    'access callback' => TRUE,
    'type' => MENU_CALLBACK,
    'file' => 'system.admin.inc',
  );

Comment #8

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 5 December 2009 at 08:00

Hm. I think my preference would actually be for a "buffer" function between the two (node_feed_page?). That array(FALSE, array()) thing seems pretty weird.

Comment #9

pwolanin CreditAttribution: pwolanin commented 6 December 2009 at 16:30

@webchick - well, that array(FALSE, array()) is just doing exactly what the wrapper function would do....

Comment #10

ksenzee

she/her

Seattle area

CreditAttribution: ksenzee commented 12 December 2009 at 02:31

If we add a wrapper function, we can at least document the array(FALSE, array()) thing.

Comment #11

MichaelCole CreditAttribution: MichaelCole commented 1 May 2010 at 00:49

#5: 622426-node-rss-errors-5t.patch queued for re-testing.

Comment #12

2 May 2010 at 00:30

Status:

Needs review

» Needs work

The last submitted patch, 622426-node-rss-errors-5t.patch, failed testing.

Comment #13

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 15 May 2011 at 16:00

Version:	7.x-dev	» 8.x-dev
Status:	Needs work	» Needs review

File	Size
622426-optionB.patch	1.96 KB

622426-optionA.patch	1.58 KB

Re-rolled against HEAD. I think adding a wrapper function for this is overkill.

If we add a wrapper function, we can at least document the array(FALSE, array()) thing.

Well, we might just as well document the array(FALSE, array()) in node_menu, we don't need to add a function just for documentation purposes.

2 patches attached:

option A: pwolanin's patch with an additional doc line.
option B: alternate patch that introduces parameter checking in the API layer.

Comment #14

drbeaker CreditAttribution: drbeaker commented 29 June 2011 at 19:46

I've tried both of these patches and still get the issue. Any ideas?

Comment #15

mr.baileys

Dutch

🇧🇪 (Ghent)

CreditAttribution: mr.baileys commented 30 June 2011 at 13:09

@drbeaker: did you rebuild the menu after applying the patches? (Administer > Configuration > Performance > Clear All Caches should do the trick).

Comment #16

greggles

he/him

English

Denver, Colorado, USA

CreditAttribution: greggles commented 8 September 2011 at 13:35

I disagree - user input triggering a SQL error is a form of SQL injection

I don't know it's a form of "sql injection" but it is often flagged as such by automated scanners.

It's just an error, imo, and certainly one worth fixing.

Comment #17

Dave Reid

he/him

English

Nebraska USA

CreditAttribution: Dave Reid commented 8 September 2011 at 13:43

Status:

Needs review

» Needs work

Reviewing the option A patch as that's the one I prefer. People should always be in the habit of providing default parameters in their menu callbacks.

+++ b/modules/node/node.testundefined
@@ -813,6 +813,16 @@ class NodeRSSContentTestCase extends DrupalWebTestCase {
+    // Regression test for #622426.  Extra path parts should be ignored.
+    $this->drupalGet('rss.xml/a');
+    // Check that content added in 'rss' build mode appear in RSS feed.
+    $rss_only_content = t('Extra data that should appear only in the RSS feed for node !nid.', array('!nid' => $node->nid));
+    $this->assertText($rss_only_content, t('Node content designated for RSS appear in RSS feed.'));
+    $this->drupalGet('rss.xml/b/c');
+
+    // Check that content added in 'rss' build mode appear in RSS feed.
+    $rss_only_content = t('Extra data that should appear only in the RSS feed for node !nid.', array('!nid' => $node->nid));
+    $this->assertText($rss_only_content, t('Node content designated for RSS appear in RSS feed.'));
   }

I don't see why we are redeclaring $rss_only_content twice again when it is already defined earlier in the test and doesn't need to be changed.

We could probably simplify this by just doing $this->drupalGet('rss.xml/a/b'). Doing so tests $this->drupalGet('rss.xml/a') as well.

"Check that content added in 'rss' build mode ~~appear~~appears in RSS feed."