I just stumbled over this log entry:

Location	http://example.com/user/register
Message		Incorrect CAPTCHA
Data:

  session_id = ''
  captcha_result = '...'
  author_ip = '...'
  author_id = '51347'
  author_name = 'some-username'
  author_mail = 'some-mailaddress'

Obviously, someone submitting the user_register form can never have an author_id already.

No idea why the code tries to look up an existing account for author_name, but I can only guess this must have been wrongly migrated from old code in the original patch that introduced the form value mapping.

This is not a security issue. It only affects Mollom's spam analysis performance and is quite unfortunate, to say the least.

CommentFileSizeAuthor
#8 mollom.author-mapping.8.patch7.32 KBsun
#1 mollom.author-mapping.1.patch3.98 KBsun
mollom.author-mapping.0.patch3.92 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom.author-mapping.1.patch3.98 KB

Forgot to copy one comment.

Status: Needs review » Needs work

The last submitted patch, mollom.author-mapping.1.patch, failed testing.

dries’s picture

dries commented

Mmm, yeah. Good catch. Not sure why the tests failed but we should get this committed once green.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

This patch will pass tests as soon as #1198192: "field_xyz][0][value" sometimes contained in post_body sent to Mollom has landed.

killua99’s picture

killua99 commented

We should change this lines in mollon.module

<?php
  // User name.
  if (!empty($mapping['author_name'])) {
    $data['author_name'] = $mapping['author_name'];
    // Try to inherit user from author name.
    $account = user_load(array('name' => $data['author_name']));
  }
  elseif (!empty($user->name)) {
    $data['author_name'] = $user->name;
  }
?>

to this

<?php
  // User name.
  if (!empty($mapping['author_name'])) {
    $data['author_name'] = $mapping['author_name'];
    // Try to inherit user from author name.
   //  $account = user_load(array('name' => $data['author_name']));
   $account = user_load(array('uid' => $user->uid)); 
  }
  elseif (!empty($user->name)) {
    $data['author_name'] = $user->name;
  }
?>

We did this change and is working very well. I post this same issue #1238060: user_load through 'name' is a good practice? because in my search didn't show me this topic.

Sorry for the duplicate content.

I'm impresive that this kind of module that have 30296+ sites in use, didn't fix this bug ASAP.

rodricels’s picture

rodricels commented

#1: mollom.author-mapping.1.patch queued for re-testing.

rodricels’s picture

rodricels commented
Status: Needs review » Reviewed & tested by the community

The patch are green now! (well, since august...).

I use it on some production sites without problems, it can be reviewed by someone more?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 6.x-1.x-dev » 7.x-2.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new mollom.author-mapping.8.patch7.32 KB

Re-rolled against 7.x-2.x.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

This looks ready to fly for me.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-2.x-dev » 6.x-2.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed to 7.x-2.x.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Patch (to be ported) » Fixed

Merged into 6.x-2.x via #1379136: Backport recent changes in 7.x-2.x.

Status: Fixed » Closed (fixed)

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

  • Commit b46e732 on 7.x-2.x, fai6, 8.x-2.x, fbajs, actions by sun: 
    - #1168604 by sun: Fixed author_id is wrongly associated for anonymous...

  • Commit b46e732 on 7.x-2.x, fai6, 8.x-2.x, fbajs, actions by sun: 
    - #1168604 by sun: Fixed author_id is wrongly associated for anonymous...