Let user_autocomplete also find the anonymous user.

If it goes green looks good to go.

Let's see how the bot gets on, this looks ok though.

Sorry, forgot some standard nitpicks :)

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAutocompleteTest.phpundefined
@@ -46,5 +46,11 @@ function testUserAutocomplete() {
+    // Using anonymous user's name, make sure it's in the result.

"Test that anonymous username is in the result" ?

+++ b/core/modules/user/user.pages.incundefined
@@ -17,6 +17,11 @@
+    // Allow to autocomplete for the anonymous user.

"Allow autocompletion for..."?

This looks good to me, do we need a user module maintainer to give this an eye too?

I think this should be an optional feature, i.e. I would vote for an $include_anonymous option in the function. I would very much welcome this feature but in certain cases choosing the anonymous user is senseless. I'm thinking of private messaging where the "Recipient" field would use an autocomplete. If I'm writing a message to some Andrew, it would be weird to type "An" and then have "Anonymous" pop up. I would assume that would be an actual user with a weird nickname or something.

I'm not sure whether the option should default to TRUE or FALSE.

In the long run, once we have an entity reference field in core, user module could even allow to have "Include anonymous user" as an option on the autocomplete widget.

That's a pretty good idea. I would default to FALSE I think (also not 100%), and later, an option for this on field widgets would be cool too :)

Some nit-picks. Marking needs work for the last item, as that would be a security issue (in case I am correct). Also no one ping Crell about this, as he will crucify you for using the "automatic passing of extra arguments"-feature of the (current) menu system. :-) I thought about using a dedicated menu router for this, but that would be a lot of code for such a small thing, so I personally like the way you solved that.

+++ b/core/modules/user/user.pages.inc
@@ -12,11 +12,24 @@
+ * @param bool $autocomplete_anonymous

I think 'autocomplete' is redundant in the variable name. I proposed 'include_anonymous' above. I'm definitely open to other suggestions but I find the ...autocomplete($autocomplete... part weird-looking. Maybe, it's just me, though. :-)

+++ b/core/modules/user/user.pages.inc
@@ -12,11 +12,24 @@
+ *   A json response containing the autocomplete suggestions for existing users.

I think this should be JSON.

+++ b/core/modules/user/user.pages.inc
@@ -12,11 +12,24 @@
+        $matches[$anonymous_name] = $anonymous_name;
...
       $matches[$account->name] = check_plain($account->name);

I think the value should be check_plain()ed for anonymous as well.

I guess we could do that as well. I must say, I don't really care either way. I don't find the current solution bad either. I'm just pretty certain Crell would hate it :-)

Test fail looks unrelated, but not sending to re-test in case you want to re-roll with the other approach.

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAutocompleteTest.php
@@ -46,5 +46,13 @@ function testUserAutocomplete() {
+    $anonymous_name = $this->randomName();

Just noticed this now, sorry, but this should be randomString(). (Using that would have (potentially) caught the not using check_plain() in previous patches.)

Since I've been complaining a lot in this issue, I thought I'd quickly re-roll the patch myself. I also added some docs to user_autocomplete() on the 'anonymous' query parameter. It came out a bit verbose, so if you can shorten that, please feel free. Otherwise I think it's fine for you to RTBC this, because my "working on this" was not really substantial. This is RTBC from my perspective.

Do we have a follow up for #2 to remove the workaround?

It's not really a feature request if the feature is already in core hidden in Views. Committed/pushed to 8.x.

Could use a change notice.

No where else in core do we check_plain() the anonymous name. This is causing random test failures.

Committed/pushed that one.

This is still causing random test failures.
Let's roll it back and then deal with the special characters causing it to break.

Anyone want to be its '/'?

Committed the rollback to stop random test failures. Sorry, folks. :\

Back to needs work.

Ran the latest patch 100 times with no fails using #1825592: Add --repeat and --die-on-fail to run-tests.sh to make random test failure diagnosis easier no fails... This one is good to go (again)...

randomString() is one of the common causes of random fails :)

Well, I'm not sure that its a valid fix. After all; the anonymous name could have any characters it wants.

I think we need a real fix (which is why it was rolled back and not just patched)

Here are a couple of values for the anonymous name that will always fail: *NlLi"97' and ''M=%[d-k

Attached are three patches which prove that we need to use json_encode(check_plain($anonymous_name), JSON_HEX_TAG | JSON_HEX_APOS | JSON_HEX_AMP | JSON_HEX_QUOT);

The patches called *.testonly.pass_.patch and *.testonly.fail_.patch are to prove that we've fixed the random fail.

Go bot go

Added a comment to explain... thanks @dawehner

+++ b/core/modules/user/user.pages.incundefined
@@ -12,11 +12,31 @@
+        $matches[$anonymous_name] = check_plain($anonymous_name);

Why are we adding the check_plain() back in here? This is still wrong.

Thanks! Good to go if it comes back green.

Removing unnecessary check_plain() from test as well then :)

Also opened followed to remove the check_plain() from user account names as well - #1893514: Remove unnecessary check_plain() in user_autocomplete()

Can I get one more +1 on amateescu's changes? Super nervous to commit another randomly failing test. :\

And by amateescu, I of course meant alexpott. :P~

Awesome, thanks!!

Committed and pushed to 8.x. Crossing fingers this time. :)

Is that json_encode + assertRaw necessary or could one do drupalGetAJAX + assert as usual?

Back to active per #29.

@olli how right you are... the code looks much cleaner... nice one! And we're less coupled to the json_encode() options which has to be a good thing.

The patch attached changes the test to use this - i've tested it with the previously failing strings and repeated the test 30+ times - all pass.

Also it's apparent from #1892836: Use user/autocomplete instead of views own autocomplete function that the whole approach of adding to the query string is incompatible with FAPI's #autocomplete so I've created a new menu entry for user/autocomplete/anonymous

Removed the change notice till we've resolved this.

Not critical but maybe worth changing if that works too.

And here's the patch does:

Also it's apparent from #1892836: Use user/autocomplete instead of views own autocomplete function that the whole approach of adding to the query string is incompatible with FAPI's #autocomplete so I've created a new menu entry for user/autocomplete/anonymous

Bumping back to critical since views can't use the currently committed solution.

Removing stray @

Updated comment

Hmm I wondered about an extra router item but assumed there'd been an attempt to avoid creating one for whatever reason. Adding another one is fine with me.

< major nitpicking of one sentence, please don't kill me >

+++ b/core/modules/user/user.pages.inc
@@ -15,22 +15,19 @@
- * parameter for the string to use to search for suggestions. If the name used
- * to indicate anonymous users (e.g. "Anonymous") is to be included as a
...
+ *   Include the anonymous user name in the list of suggestions.

I agree with @dawehner's suggestion. We usually even do the super-verbose: "(optional) TRUE, if the anonymous user should be included in the list of suggestions, FALSE otherwise. Default to FALSE."
At least the "(optional)" should be at the beginning, though, that is a pretty well-enforced standard around core.

Also, I had "the name used to indicate anonymous users (e.g. "Anonymous")" before, where now you simply have "the anonymous user name". I know the audience here is developers but I personally find it weird to have a user name that is anonymous, i.e. if I know the name he/she is not anonymous anymore...

< / see above >

I would like to question the decision here to remove the check_plain(), I think that was wrong, or at least it should be optional. I realize that as a generic JsonResponse user_autocomplete() should have no business in deciding whether I want the names sanitized or not, but in the specific case where the autocomplete response is used directly in a form I was able to perform an XSS injection by simply applying this patch and then doing

     $form['author']['name'] = array(
       '#type' => 'textfield',
       '#title' => t('Authored by'),
       '#maxlength' => 60,
-      '#autocomplete_path' => 'user/autocomplete',
+      '#autocomplete_path' => 'user/autocomplete/anonymous',
      '#default_value' => !empty($node->name) ? $node->name : '',
      '#weight' => -1,
      '#description' => t('Leave blank for %anonymous.', array('%anonymous' => $user_config->get('anonymous'))),
    );

in NodeFormController::form(). If you enter "Anonymous

" as the anonymous user name, the script runs in your browser when typing "A" in the user autocomplete field on the node add/edit form.

I think ideally, we would do that processing (should be just a array_walk($names, 'check_plain') right?) on the form side, and leave the user_autocomplete() as it (currently) is. Again, I'm the last person who wants to steal the purity of JsonResponses, but IMO it would be foolish to inadvertently open the door to XSS injections both in core and in contrib.

Also as has been noted above, the user names themselves are being check_plain()ed also, so the current state could even be considered a regression. At the very least it's inconsistent and arguably a bug in its current form.

For these reasons, I think we should have that discussion here and not in a follow-up. Also because the form-side processing should be trivial, as noted.

I don't know what to set the status to, because the currently proposed path is an improvement over the status quo, and allows the (currently dangerous, see above) form API integration. I think we should commit the path (after the comment fix, see above) and then discuss the security/sanitization issue here as a follow-up.

I had already forgotten about #1893514: Remove unnecessary check_plain() in user_autocomplete(), but I personally still think we should handle this here (whether that now goes in or not).

You need to deliver HTML to autocomplete.js . This means that you must check_plain usernames and the anonymous name (if it isn't check_plained elsewhere in core, it's the elsewhere that is wrong.).

Okay check_plain() is back and improved comment in response to #61

Thanks for all the reviews!

+++ b/core/modules/user/lib/Drupal/user/Tests/UserAutocompleteTest.php
@@ -47,15 +47,12 @@ function testUserAutocomplete() {
+    $this->assertTrue(isset($users[$anonymous_name]), 'The anonymous name found in autocompletion results.');

Since we went so much back and forth on this I think it would be good to specifically test for the check_plain()ed value. I.e. I would change this to

$this->assertEqual($users[$anonymous_name], check_plain($anonymous_name), ...);

I don't think that's necessary as the json array is keyed by the non check_plain'ed value and we are testing the key.

Well, I just wanted to point out that that test will pass with or without the check_plain() in user_autocomplete(). Don't want to get into any status wars, though :-)

Now that is a good point!

Here's a patch to address that.

Assuming this comes back green, this is RTBC.

Looks good, seems like paranoid tests, but I like that :)

Eeek! Thanks for testing, folks.

Committed and pushed to 8.x. Back to active for change notification?

Here we go: http://drupal.org/node/1896862

EDIT: The [ # ] filter obviously doesn't work here...

Untagging. Please remove the "Needs change notification" tag when the change notice task is complete.