Posted by dawehner on October 19, 2012 at 1:00pm
11 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | user.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Needs change notification, VDC |
Issue Summary
Currently user_autocomplete() just executes a query that filters by name, but this will not find the user with uid 0,
because it doesn't have the name in the database.
Views has a workaround for that http://drupalcode.org/project/views.git/blob/refs/heads/8.x-3.x:/include...
but i think it would be also helpful for core itself.
Comments
#1
Wrong component.
#2
One general question, should we remove the views specific implementation in this patch?
#3
#2: drupal-1817656-2.patch queued for re-testing.
#4
Rerolled.
#5
If it goes green looks good to go.
#6
The last submitted patch, drupal-1817656-4.patch, failed testing.
#7
Oh, the autocomplete now looks on the q GET parameter.
#8
#7: drupal-1817656-7.patch queued for re-testing.
#9
Let's see how the bot gets on, this looks ok though.
#10
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..."?
#11
Thank you for the review!
#12
This looks good to me, do we need a user module maintainer to give this an eye too?
#13
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.
#14
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 :)
#15
I agree that we need this flexibility.
#16
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.
#17
Thanks for your review!
I totally agree with your suggested naming of the variable, good point!
Regarding the optional part, what about using a query parameter instead? So the router path just determines the controller/page callback and all other control-parameters are passed via $_GET. (That's not part of the new patch).
#18
The last submitted patch, drupal-1817656-17.patch, failed testing.
#19
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.
#20
Let's keep the new parameter as short as possible.
#21
The last submitted patch, drupal-1817656-20.patch, failed testing.
#22
Totally failed on the patch.
This patch seems to be the perfect candidate for converting to DrupalUnitTests later.
#23
The last submitted patch, drupal-1817656-22.patch, failed testing.
#24
#22: drupal-1817656-22.patch queued for re-testing.
#25
+++ 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.
#26
Good improvement of the documentation! Thank you.
I usually try to avoid randomString() to avoid random test failures, but yeah for these kind of cases it is certainly better.
Sure we could do something like 'a', or 'anon' or something similar, though it seems to be that all of them do make it more complicated.
#27
Do we have a follow up for #2 to remove the workaround?
#28
No, but here is one: #1881450: Replace views user autocomplete function with the one from core, once #1817656 is in. Believe me, I will take care of that :)
#29
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.
#30
#31
No where else in core do we check_plain() the anonymous name. This is causing random test failures.
#32
Committed/pushed that one.
#33
Just opened a follow up for the removal of that in views: #1892836: Use user/autocomplete instead of views own autocomplete function
#34
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 '/'?
#35
Committed the rollback to stop random test failures. Sorry, folks. :\
Back to needs work.
#36
There are just reasons why you should avoid randomString() if possible. Let's use randomName().
#37
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 :)#38
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)
#39
You know there are million of places in which randomString() is not used so we might should better fix them as well?
This is just a guess, could it be that certain random characters cause problems with some escaping?
#40
The last submitted patch, drupal-1817656-39.patch, failed testing.
#41
Here are a couple of values for the anonymous name that will always fail:
*NlLi"97'and''M=%[d-kAttached 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.
#42
Go bot go
#43
Added a comment to explain... thanks @dawehner
#44
RTBC if it's green and tim had a look at it.
#45
+++ 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.
#46
Oh damnit.
#47
Thanks! Good to go if it comes back green.
#48
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()
#49
Can I get one more +1 on amateescu's changes? Super nervous to commit another randomly failing test. :\
#50
+1
#51
And by amateescu, I of course meant alexpott. :P~
Awesome, thanks!!
Committed and pushed to 8.x. Crossing fingers this time. :)
#52
Is that json_encode + assertRaw necessary or could one do drupalGetAJAX + assert as usual?
Back to active per #29.
#53
@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.
#54
Not critical but maybe worth changing if that works too.
#55
This change looks pretty good, thanks for pointing that out olli.
#56
And here's the patch does:
Bumping back to critical since views can't use the currently committed solution.
#57
Removing stray @
#58
It has never been critical in the first place, so I think major might be better.
This clearly lacked a real world implementation, I'm sorry for that.
This is looking really good!
+++ b/core/modules/user/user.pages.incundefined@@ -15,22 +15,19 @@
+ * @param @boolean $include_anonymous
+ * Include the anonymous user name in the list of suggestions.
I think it should be look more like "@param bool $include_anonymous \n
TRUE if the anonymous user should be autocompleted. Defaults to FALSE."
#59
Updated comment
#60
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.
#61
< 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 "Anonymousalert('XSS!')" 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.
#62
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).
#63
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.).
#64
Okay
check_plain()is back and improved comment in response to #61Thanks for all the reviews!
#65
Good to know.
I don't think adding possibilities like query arguments to autocomplete paths is worth the effort.
#66
+++ 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
<?php$this->assertEqual($users[$anonymous_name], check_plain($anonymous_name), ...);
?>
#67
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.
#68
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 :-)
#69
Now that is a good point!
Here's a patch to address that.
#70
Assuming this comes back green, this is RTBC.
#71
Looks good, seems like paranoid tests, but I like that :)
#72
Eeek! Thanks for testing, folks.
Committed and pushed to 8.x. Back to active for change notification?
#73
Here we go: http://drupal.org/node/1896862
EDIT: The [ # ] filter obviously doesn't work here...
#74
This looks great! Thanks for writing a change record.
#75
Automatically closed -- issue fixed for 2 weeks with no activity.