PHP Errors on certain spam content

Confirming, my watchdog is filled with these.

Hm. Are you able to test the attached patch?

Looks like that fixed it. I was getting these errors every 10-20 minutes, and since I applied the patch it's been an hour without a single one, and Mollom seems to be operating properly. I should probably check again in the morning to be sure, but so far this looks great.

Thanks, sun!

Beautiful. No more php errors since I installed the patch, and Mollom is still catching all the spam so it doesn't seem to have any adverse effects.

I don't know if you want to call this RTBC on the basis of one person, but this gets a thumbs-up from me.

I really wonder why the Mollom module needs to handle this. Core shouldn't allow the rest of Drupal core or any contributed module to operate on invalid UTF-8 data. If not, we'll end up sprinkling drupal_validate_utf8() all over the place ... it feels like this kind of validation should happen early in the page handling request, or as part of the form API to help make sure we get valid input data.

Leaving it up to the contributed modules seems like a recipe for problems because we could end up storing invalid UTF-8 data in the database and what not. I don't know but in certain scenarios it might have security implications too.

mmm, yes. We're slightly circling back into the existing core issue #882298: XML-RPC request (string) values are not safe for UTF-8

So this bug should actually be fixed in xmlrpc.inc and/or check_plain(). However, I'm not sure whether Form API is guilty here, since Drupal always validates UTF-8 and escapes user input on output, which means that we are accepting and storing any kind of data. When form elements get rendered, the user input is sanitized. Likewise, when the stored data is displayed. What bites us here is that the XML request output is not properly sanitized currently.

Granted, it's debatable whether Form API shouldn't throw a form validation error in case the user input contains invalid UTF-8. That would likely catch and prevent most bogus user input, but would not remove the requirement to sanitize the user input on output, since data can always be submitted/stored through other means than form submissions.

Ultimately, however, I think we need to move forward with this patch, since it is not clear yet, when and how Drupal core 7 and 6 can or will be fixed. I'll try to move the existing core patch forward, of course.

What remains for this patch is to write a test that ensures we're catching the bug. Doesn't have to be as precise as the tests for Drupal core.

I'm willing to be pragmatic about it, especially because the utf8 checks weren't introduced in this patch. It would be nice to add a @todo so we don't forget about this in the Drupal 8 development cycle.

Having said that, the utf8 handling seems to be seriously flawed.

Whenever an UTF-8 validation error is found, Mollom simply discards the data. This effectively gives spammers an easy way to by-pass Mollom.

Furthermore, with the proposed patch, the Mollom module will silently ignore these validation errors. According to the people that report this bug above, their watchdog is filled with these PHP warnings meaning this happens a lot.

In summary: by silently discarding spam data we might have given spammers a way to by-pass Mollom, and with the proposed patch no one will ever notice. This may lead people to incorrectly believe that Mollom doesn't work.

Oh. Yes, that makes sense. So an alternative is to remove all of the existing UTF-8 validation checks, and instead, iterate over all keys in our generated $data and invoke drupal_validate_utf8() on them. If any key fails, we throw a form validation error and empty out the $data array. In turn, this would mean that the form cannot be submitted and we also do not query Mollom's backend with the invalid data.

The proposed solution in #8 is much better. It's actually what I had in mind.

Wouldn't it be better to move that code to the top of mollom_form_get_values() and iterate on $form_values instead of $data? In fact, I even wonder if that code belongs in mollom_form_get_values(). By putting it in mollom_form_get_values() we need those extra if-check elsewhere in the code.

If we can iterate over $form_values we could put this code in a separate function, and register it as a form validation callback that runs before any of the other validation callbacks. If that callback can prevent the other callbacks from running, than we have isolated this 'hack' nicely. We could add a @todo to that callback and hopefully move the code to form API at some point in the future.

Just an idea -- I haven't tried it yet so I'm not even sure it can be done elegantly. What do you think, sun?

It would also be great to have a test for this. It seems important enough.

Looking at the surrounding code, I noticed the following:

  $all_data = mollom_form_get_values($form_state['values'], $form_state['mollom']['enabled_fields'], $form_state['mollom']['mapping']);
  $data = $all_data;

We should be able to simplify that to one line. :)

Created a patch for Drupal core: #943722: Form API accepts values containing invalid UTF-8

Managed to talk to Damien Tournoud (who has a great UTF-8 expertise), and it looks like we're going to fix Drupal core in a way that halts the entire server request at the beginning of a request, in case we encounter invalid UTF-8 in the server input variables (PHP's superglobals). See #943722: Form API accepts values containing invalid UTF-8 for details.

Given that, I'd suggest to do a temporary stop-gap solution for the Mollom module only, which does not have to be pretty. We'll remove that code entirely after core has been fixed and a new version of Drupal core has been released.

Therefore, I think we can move forward with this patch as-is. Happy to discuss though.

For Drupal 7, I think we can wait for #943722: Form API accepts values containing invalid UTF-8 to land. I just reviewed it and it looks very close. It can probably be committed in the next day or so.

For Drupal 6, we might want to backport your patch in #8, unless we plan to back port that core patch in #943722.

Patch still applies cleanly against D6. I just ran all tests with it applied. Ready to fly.

Committed to DRUPAL-6--1. Marking as 'needs to be ported' so we can keep an eye on the core patch for D7.

Re-rolled against HEAD.

#943722: Form API accepts values containing invalid UTF-8 didn't land for D7.0, so we have at least one version of D7 core that needs hand-holding (this patch).

Thanks for reporting, reviewing, and testing! Committed to HEAD.

A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.