Access /drupal/comment/1blah (any comment-id starting with a number, but containing non-numeric chars)

With default installation and MySQL, I get a 404 page that also includes PHP undefined index notices:

Notice: Undefined index: 1blah in comment_load() (line 1685 of /var/www/drupal/modules/comment/comment.module).
Notice: Undefined index: 1blah in comment_load() (line 1685 of /var/www/drupal/modules/comment/comment.module).
Notice: Trying to get property of non-object in comment_access() (line 1443 of /var/www/drupal/modules/comment/comment.module).

An erroneous request shouldn't make it this far, as such unexpected conditions may lead to attack vectors (apart from the annoyance of PHP notices). Failfast and all that.

Steps to reproduce

  1. Clean install of Drupal 7.x
  2. Log in as admin
  3. Add Article
  4. Add Comment
  5. Navigate to /comment/1blah

With PostgreSQL, I get database errors:

PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR: invalid input syntax for integer: "1blah" at character 609: SELECT base.cid AS cid, base.pid AS pid, base.nid AS nid, base.uid AS uid, base.subject AS subject, base.hostname AS hostname, base.created AS created, base.changed AS changed, base.status AS status, base.thread AS thread, base.name AS name, base.mail AS mail, base.homepage AS homepage, base.language AS language, n.type AS node_type, u.name AS registered_name, u.uid AS u_uid, u.signature AS signature, u.signature_format AS signature_format, u.picture AS picture FROM {comment} base INNER JOIN {node} n ON base.nid = n.nid INNER JOIN {users} u ON base.uid = u.uid WHERE (base.cid IN (:db_condition_placeholder_0)) ; Array ( [:db_condition_placeholder_0] => 1blah ) in DrupalDefaultEntityController->load() (line 196 of /var/www/.../news/includes/entity.inc).

(presumably because MySQL silently, "cleverly", ignores conversion errors, while PG does not).

In fact for PostgreSQL it's not even /comment/blah (no leading digit) yields the same error, and comment/reply/blah as well.

I'm seeing the same problem with non-numeric node ID's and PG, but that is presumably a separate issue.

CommentFileSizeAuthor
#16 comment-fix-cid-1838338-16.patch649 bytesmoertle
#10 drupal-fix_errors_when_comment_argument_is_not_integer-1838338-9.patch1.33 KBparavibe
#7 drupal-fix_errors_when_comment_argument_is_not_integer-1838338-7.patch1.28 KBparavibe
#5 drupal-fix_errors_when_comment_argument_is_not_integer-1838338-5.patch1.3 KBparavibe
#2 drupal-fix_errors_when_comment_argument_is_not_integer-1838338-2.patch1.35 KBparavibe
#1 Screen Shot 2013-03-09 at 10.18.24 AM.png95.47 KBlbainbridge
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lbainbridge’s picture

lbainbridge CreditAttribution: lbainbridge commented
Version: 7.17 » 7.x-dev
FileSize
Screen Shot 2013-03-09 at 10.18.24 AM.png95.47 KB

Not an issue in Drupal 8.x

Steps to reproduce:

  1. Clean install of Drupal 7.x
  2. Log in as admin
  3. Add Article
  4. Add Comment
  5. Navigate to /comment/1blah
paravibe’s picture

paravibe CreditAttribution: paravibe commented
Status: Active » Needs review
FileSize
drupal-fix_errors_when_comment_argument_is_not_integer-1838338-2.patch1.35 KB

Maybe should be like this?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn commented 
+++ b/modules/comment/comment.module
@@ -1681,8 +1681,10 @@ function comment_load_multiple($cids = array(), $conditions = array(), $reset =
+  if (is_numeric($cid)) {

Probably should use is_int().
http://php.net/manual/en/function.is-numeric.php

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn commented
Issue summary: View changes
Status: Needs review » Needs work
paravibe’s picture

paravibe CreditAttribution: paravibe commented
Status: Needs work » Needs review
FileSize
drupal-fix_errors_when_comment_argument_is_not_integer-1838338-5.patch1.3 KB

OK, I think now it's better.

Status: Needs review » Needs work

The last submitted patch, 5: drupal-fix_errors_when_comment_argument_is_not_integer-1838338-5.patch, failed testing.

paravibe’s picture

paravibe CreditAttribution: paravibe commented
FileSize
drupal-fix_errors_when_comment_argument_is_not_integer-1838338-7.patch1.28 KB

Second try

paravibe’s picture

paravibe CreditAttribution: paravibe commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 7: drupal-fix_errors_when_comment_argument_is_not_integer-1838338-7.patch, failed testing.

paravibe’s picture

paravibe CreditAttribution: paravibe commented
Status: Needs work » Needs review
FileSize
drupal-fix_errors_when_comment_argument_is_not_integer-1838338-9.patch1.33 KB

Status: Needs review » Needs work

The last submitted patch, 10: drupal-fix_errors_when_comment_argument_is_not_integer-1838338-9.patch, failed testing.

rakeshfalke’s picture

rakeshfalke CreditAttribution: rakeshfalke commented

Tested @drupalrv code working good :).

rakeshfalke’s picture

rakeshfalke CreditAttribution: rakeshfalke commented
Status: Needs work » Fixed

Steps to Followed:

  1. Clean install of Drupal 7.x
  2. Log in as admin
  3. Add Article
  4. Add Comment
  5. Navigate to /comment/1blah

Working properly so closing this issue.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes
Status: Fixed » Needs work

I'm still able to reproduce this using the steps to reproduce, adding them to the issue summary.

@rakeshfalke - for future reference if this wasn't reproducible we'd use the status "Closed (cannot reproduce)" :)

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue tags: +Needs tests

Oh maybe I misunderstood - I missed comment #12.

So @rakeshfalke maybe you tested with the patch and it worked? That still means the issue needs to stay open for further reviews and adding tests to cover this bug would be a good idea.

moertle’s picture

moertle CreditAttribution: moertle commented
FileSize
comment-fix-cid-1838338-16.patch649 bytes

Another try with is_numeric

moertle’s picture

moertle CreditAttribution: moertle commented
Status: Needs work » Needs review
poker10’s picture

poker10 CreditAttribution: poker10 at ActivIT s.r.o. commented
Status: Needs review » Closed (outdated)

I have tried this on clean D7.97 install and accessing such link /comment/1blah does not throw notices/warnings for me (on PHP8). This is probably outdated, but feel free to reopen, if this is still an issue. Thanks!