Cleanup for user_validate_name() + tests

I don't pretend to understand that strange regexp, but it seems that there is no need to validate the server part because the authentication scheme don't use that anymore (it was used for Drupal Distributed Authentication, which is deprecated in favor of OpenID).

So feel free to drop that part.

One remark about the patch: the strlen() there should be a drupal_strlen(), because we are in an unicode context.

and various weirdo UTF8 chars... or even invalid UTF8 characters for that matter.

Patch is goods so far.

I'm wondering if we could incorporate allowing apostrophes into this cleanup, since it's currently just an omission. Patch and discussion here: http://drupal.org/node/165226

I don't understand at all why we are not using ENT_NOQUOTES in check_plain()?

It looks like this sort of things should not even be required. I guess some part of the code may call check_plain() two times, and it fails because that function is not reentrant.

Works for me. Will leave it at needs review for a second set of eyes but I think this is ready. Side note, patch isn't rolled from root of the Drupal directory, it's easier to apply that way.

@MadHarold, the patch is perfectly rolled from the root directory, there is nothing wrong about it.

whoops. Nothing wrong. user_100.patch wasn't rolled from root (or at least was prompting me for filenames), user_101.patch is completely fine. Sorry for the noise :(

Testing the patch in #8:

Applied the patch to todays dev tarball - applies cleanly.

created a number of accounts - those with or without apostrophe are created correctly. Those with quotation marks (") give an error of an illegal character.

All that is left is for a "policy decision" - either patch is comment or comment comment 8 should go in.

Moved to RTBC as I am the second pair of eyes as requested by Catch.

I've asked the security team to have a quick at this patch as well.

Well, there are some XSS vectors that succeed due to injected quotes closing an img path or attribute, so I'm not that comfortable with this. Clearly check_plain() should not be called multiple time on the same text - but that's a separate issue.

Using ENT_NOQUOTES allows values to 'run out' of attribute values:

Consider:

$output = '<tag attribute="'. check_plain($somevalue) .'" />';

Now, suppose $somevalue is 'foo" onerror="javascript:evil_js()" '

You'll get:

<tag attribute="foo" onerror="javascript:evil_js()" />

Would this analysis also apply for the original (otherwise almost identical) patch in comment 6 that uses ENT_COMPAT instead of ENT_NOQUOTES?

In HTML 4, attributes may be delimited by either single or double quotes. Single quotes may be used unencoded in attribute values when the value is surrounded by double quotes and vv. As check_plain has no information about which quotes are used to embed something we need to play it safe and cater to both delimiters.

Or in clear words; check_plain should not be modified.

Is there another sane approach that can be used to allow apostrophes in usernames?

(this issue does not deal directly with that issue, the the failed segments of the patch are those that have been imported from the other issue mentioned in comments 5 and 6.)

Of course there is. Investigate the causes of double encoding and fix them.

For instance: blog_link escapes the username

$links['blog_usernames_blog'] = array(
  'title' => t("@username's blog", array('@username' => $node->name)),
  'href' => "blog/$node->uid",
  'attributes' => array('title' => t("Read @username's latest blog entries.", array('@username' => $node->name)))
);

Then theme_links escapes the title and attributes again (in l()):

if (isset($link['href'])) {
  $output .= l($link['title'], $link['href'], $link['attributes'], $link['query'], $link['fragment'], FALSE, $html);
}
else if ($link['title']) {
  //Some links are actually not links, but we wrap these in <span> for adding title and class attributes
  if (!$html) {
    $link['title'] = check_plain($link['title']);
  }

I assume this patch is still useful without changing things to ENT_NOQUOTES, or to ENT_COMPAT, as they were added later on to the patch.

Attached patch is the same as above, but I have manually removed the bit for changing the ENT_QUOTES bit. I have kept in the bit allowing apostrophe's. I say let's assume the places where they do not work properly are bugs (they do work in most places).

The patch in #22 can go in this way. Making check_plain() reentrant will require a new issue.

I introduced a discussion about making check_plain() reentrant in #275308.

Committed to CVS HEAD. Thanks.

Some tests are failing. Please keep that issue open on 7.x so we can figured out why.

Dries: Looks like an incomplete version of the patch was committed. Only the user.test hunks were committed, not the user.module ones.

Bumping to critical by convention (tests are failing...).

While we are at it, here is the user.module hunk plus some small cleanups for the new username validation tests, that make test descriptions cleaner and remove a spurious '%s' in the test description.

Looks good, works good too.

Committed to CVS HEAD. Thanks Damien, and sorry for messing up.

Ok, let's consider inclusion in the Drupal 6.x branch.

Here's a port. I've excluded the user.test bit, as well as other things that actually changed functionality and/or strings.

+  if (substr($name, 0, 1) == ' ') {
+    return t('The username cannot begin with a space.');
+  }
+  if (substr($name, -1) == ' ') {
+    return t('The username cannot end with a space.');

Why can't we just trim() username? I realize this is already in the 7.x version; I didn't get a chance to review that patch before it was committed. It also shouldn't be a tab character or any other whitespace. trim() takes care of that.

Followup issue for fixing instances of the username being check_plain'ed twice:

http://drupal.org/node/276174

Patch in comment 34 applies cleanly, works as expected and is a port of what is in HEAD so setting to RTBC

As for comment 35, I would guess the proper place to trim would be upon input - before the validation function.

While I completely agree with Angie's comment in #35, this is another issue.

However, two hunks were excluded from the patch in #34, that we should consider including again:

• The dropping of the "authentication ID" check: I think this should be dropped also for D6, because it is not used anymore (remote authentication schemes are done differently, see my comment in #1)
• The addition of control characters in the 0x00-0x1f range, that we should probably include for sanity

I'm putting on review the opportunity to add those back in.

I have re-ported the patch from comment 30 - without any functional changes that were introduced to the port to drupal 6 in comment 34.

@nbz: You're implementing a new feature though: allowing apostrophes.

Drupal 6 is in feature and string freeze both. The patch in #34 didn't change alter or add or remove any feature, even if #38 thinks it should contain more of the 7.x code. I'm still not sure about this though, as it will change the behaviour. So I'm leaving it for Gábor/Dries to decide on how much of it should be back ported.

Not supporting a single quote in a username is a bug, not a lacking feature.

@ #40 - I see where you are coming from, but from the mailing lists it seems Drupal 6 may get an extended life cycle (by virtue of delaying Drupal 7 code freeze date), and IMO allowing apostrophe's is a vital functionality, the lack of which bordering close to being a bug.

Not having explicit approval (yet) from Dries or Gabor for this additional functionality should not be a blocker for RTBC status. IMO the code review should be to make sure that it all works as expected without any caveats. nce the patch has been set RTBC, the branch maintainer can make up his mind about whether this should be allowed or not.

EDIT - not having this feature in core means that me (and potentially others who have migrated users from other systems that allow apostrophes) will run a patched core install, which may have security vulnerabilities. That should be considered a bug, not in the released software, but procedure.

We have two patches here - comment 34 without the changes allowing apostrophes in usernames, and comment 39 with the changes allowing apostrophes.

Can somebody please review (and maybe help in the other issue (#276174: Do not check_plain() usernames more than once) too so that I can find out exactly where the other checkplain takes place)?

Once both of these patches are RTBC, the maintainer can decide if he wished to allow or not allow this bug fixing functionality.

Patch in comment #34 (without new features and string change) was RTBC'd (in comment 37), so we're just short of a review of the patch in comment #39 (adding features and changing a string) before we can let Gábor-Dries decide on what to go with.

Patch in comment 39 still works, but with an offset of 14 lines in hunk 3. Should I reroll?

(and is some kind soul willing to review?)

subscribe

updated patch from comment 39 - I just removed hunk three as all that did was change a string to inform that apostrophes are allowed.

In this patch, apostrophes are still allowed, but that is now "by stealth" so as not to break string freeze.

(This will need to be applied at the same time as #276174: Do not check_plain() usernames more than once to fix the theming errors for usernames in blog.module.)

Subscribe... we have lots of members imported from phpBB with an ' in their username. Would be lovely if that worked for them :)

Patch in comment 47 still applies, but with an offset of 2 lines.

For those subscribing - it would be helpful if the patch was reviewed and set to rtbc if its working as expected.

Well, we have been testing it for half a year now, and it works perfectly. How can I set it to rtbc? It works flawlessly... and we are about to patch the updated core again. Personally I think it is ready to be implemented :)

"How can I set it to rtbc"

you just choose the option. At that stage, the maintainer of the Drupal 6 branch can then agree or disagree with the change. til then it is nothing and will not be committed even if the maintainer likes it.

I have also been using this patch for a long time too and think it is ready, but since I rolled it, made some changes etc... it would not be good for me toset it to rtbc.

fwiw, the last version does not break any strings - it just adds the feature "silently" and this will be compatible with Drupal 7 since that already has the patch.

This issue has been floating around now for nearly two years.

user_validate_name in D7 has been fixed for more than 21 months already.

The backport to D6 has been around for 18 months. It's been tested fairly extensively as the last few posts show, and I've just re-checked it myself as well. As such, I'm marking RTBC.

Let's try to get it into the next D6 update, especially since this patch also fixes #526590: user_validate_name not allowing military email addresses as username. As noted there, a username that happens to be a military email address causes an outright rejection -- that's a pretty shocking bug at this point, so I'm also marking this as critical.

And finally, I'm re-rolling the same patch in #47 to help speed things along.

Let's get this done, please.

Great, thanks for all the testing, committed the patch with one small change. Left the initial strlen() intact (with the code style changes applied). The version in the patch did not allow for username "0" (the number zero) which is otherwise allowed before the patch. I'd suggest porting this change back to D7 as not to break backwards compatibility with D6 userbases.

When $user->name == "0", this causes problems elsewhere (which we could fix, of course - but I'd like some consensus on whether this should be a valid user name before going in either direction).

#845472: Robustly determine when strings are empty

Okay, I've asked around, and the general consensus seems to be that "0" should be a valid username. That being the case, here's a patch against D7 that implements the same check as D6 has. I'll deal with the impact of this in #845472 (for both D7 and D6).

-1 to patch in #55.

I'm in favor of not allowing 0 as a name and giving one less edge case to contrib authors to worry about. Core doesn't even handle this properly yet (#845472: Robustly determine when strings are empty), acknowledging we don't support this case is the easiest solution.

I know, technically, we could. Is it worth the pain to patch things in core and contrib to support this? Is there any site that actually wants a user named 0?

Here is what D7 does at the moment:

Enter nothing in the Username field and you get 'Password field is required.'.

The only way to get the 'You must enter a username.' string (from user_validate_name() ) is to enter a 0 in the Username field.

The forms layer is already validating that a string has been entered. user_validate_name() is attempting to repeat this logic and screwing it up.

The 'required' check should just be removed from user_validate_name().

'0' is a perfectly valid string. We shouldn't let the failings of PHP dictate what is and what isn't a valid username.

See also http://drupal.org/node/845472#comment-3276796

It seems that user_validate_name() is tested directly, instead of through the form.

Updated patch fixes the empty check in user_validate_name() and sets the username form item to validated so that user_validate_name() can report the error the way it wants.

Looks fine

+++ modules/user/user.module	2 Aug 2010 11:15:32 -0000
@@ -574,7 +574,7 @@
 function user_validate_name($name) {
-  if (!$name) {
+  if (strlen(trim($name)) == 0) {
     return t('You must enter a username.');

@@ -973,6 +973,7 @@
+    '#validated' => TRUE,   // This is so user_validate_name() can handle the 'required' validation.

+++ includes/install.core.inc	2 Aug 2010 11:15:33 -0000
@@ -1638,6 +1638,7 @@
     '#required' => TRUE,
...
+    '#validated' => TRUE,   // This is so user_validate_name() can handle the 'required' validation.

heh, that's a giant hack :)

If it's just for getting around the default #required error message, then postpone this change on #742344: Allow forms to set custom validation error messages on required fields.

Actually, #required should already handle exactly the case for that empty username, so I'm not sure why this code contains custom validation for the very same empty condition at all.

But again, if it's just to have a fancier validation error message, then aforementioned issue is required for this patch.

Powered by Dreditor.

I think the idea is that user_validate_name() should be the authority on what is a valid username. This function is tested directly, not through the form.

See #58

Reading through the entire issue, now I understand where you're coming from.

1. The follow-up fix that was already applied to D6 isn't contained in http://api.drupal.org/api/function/user_validate_name/7 yet: Drupal 7 does not allow a username of "0".
2. That quick follow-up fix for D6 should have used drupal_strlen() instead of strlen().
3. I see the point of user_validate_name() being invoked on its own outside Form API. Thus, we want to keep that first condition + error message.
4. Regardless of what you do (and #validate => TRUE won't fly ;), that very error message won't be output for the user registration form. That's only possible via #742344: Allow forms to set custom validation error messages on required fields, which, however, requires to duplicate this form validation error message elsewhere.

Agreed on all points!