Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Following on from http://drupal.org/node/266488, treating cases where usernames are escaped twice as a bug.
Since check_plain cannot be made reentrant, I guess the instances where the username is checked twice should be fixed. From a quick view, this only seems to happen in the blog.module file.
I have changed the @username placeholder inside l() anf t() functions to !username. Patch is attached.
Comment | File | Size | Author |
---|---|---|---|
#17 | 276174-code-comments.patch | 1.57 KB | TR |
#13 | fixnames-drupal6-docs.patch | 2.35 KB | Gábor Hojtsy |
#2 | fixnames-drupal6.patch | 1.94 KB | NaheemSays |
fixnames.patch | 2.1 KB | NaheemSays | |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks good. The discussion already occurred in http://drupal.org/node/266488 and in http://drupal.org/node/275308 (sigh!).
Nothing more to add.
Comment #2
NaheemSays CreditAttribution: NaheemSays commentedAnd a port of the above patch for Drupal 6
Comment #3
Dries CreditAttribution: Dries commentedI've committed this to CVS HEAD.
Comment #4
Gábor HojtsyI briefly looked at the two issues referenced, and could not find references to these exact issues there. Where are usernames escaped in these cases? Just using the username without escaping seems like a bad idea to me, and the code just shows that. Wherever it is escaped earlier, it might need fixing IMHO.
So let's first define how do these end up escaped twice.
Comment #5
NaheemSays CreditAttribution: NaheemSays commentedIs there any way to check and follow how the strings are processed?
From what I can see, node_load gets the username through a db_fetch_object. blog_view uses it. I have no idea how to get the intermediate/following stages.
Comment #6
NaheemSays CreditAttribution: NaheemSays commentedI think this should be set back to RTBC as in node.module, theme_node_submitted (in HEAD around line 2534) already uses !username so that it is not checkplained twice.
I do not know where the original check_plain takes place, but drupal already works around that in other places.
Comment #7
NaheemSays CreditAttribution: NaheemSays commentedPatch still applies cleanly. Setting to CNR so that someone else may review and pick up on this as I cannot answer the asked questions, just note that node.module is the lead I am following with these changes.
EDIT - does this need a security review?
Comment #8
John Morahan CreditAttribution: John Morahan commentedIt is not escaped earlier but later, in l() and drupal_attributes().
Comment #9
NaheemSays CreditAttribution: NaheemSays commented@#4 - can this be set back to rtbc as the question has been answered in comment 8?
Comment #10
John Morahan CreditAttribution: John Morahan commentedYes, it can. The patch is correct and still applies.
This is safe because l() passes $options['attributes'] through drupal_attributes, which runs check_plain on the attribute values.
This is safe because l() runs check_plain directly on the $text parameter unless $options['html'] is set, which it is not here.
This is safe because template_preprocess_node passes these through theme('links') which calls l() and so is safe for the same reasons as above.
Comment #11
John Morahan CreditAttribution: John Morahan commentedOn the other hand, maybe you don't want to change strings in D6 for this, seeing as how the characters that would cause problems are not allowed in usernames anyway without some custom coding. I expect some more custom coding could get around the double escaping too.
Comment #12
Sophia CreditAttribution: Sophia commentedIs anything knows when this patch will be applied to the next release of D6? We imported hundreds of members from phpBB, and several of them have an ' in their name... thanks :)
Comment #13
Gábor HojtsyOk, I think it is important to document why can we use unescaped text, so I've committed this to Drupal 6. I'd like to request a port of the three doc lines to Drupal 7, so we keep this knowledge in the code, and people will not attempt to "fix" this by moving away from !name and !username.
Comment #14
gpk CreditAttribution: gpk commentedChanging to documentation component.
Comment #15
jhodgdonThese are code comments - I think this belongs in the blog module for consideration...
Comment #17
TR CreditAttribution: TR commentedAs requested by Gábor in #13, here's a port of the code comments to Drupal 8.x. Patch applies to Drupal 7.x as well.
Comment #18
jhodgdonLooks fine to me. 8.x/7.x - no code change, just some in-code comments to clarify what is going on.
Comment #19
webchickCommitted to 8.x and 7.x. Thanks!