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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. The discussion already occurred in http://drupal.org/node/266488 and in http://drupal.org/node/275308 (sigh!).

Nothing more to add.

NaheemSays’s picture

FileSize
1.94 KB

And a port of the above patch for Drupal 6

Dries’s picture

Version: 7.x-dev » 6.x-dev

I've committed this to CVS HEAD.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

I 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.

NaheemSays’s picture

Is 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.

NaheemSays’s picture

I 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.

NaheemSays’s picture

Status: Needs work » Needs review

Patch 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?

John Morahan’s picture

Title: DO not check_plain() usernames more than once » Do not check_plain() usernames more than once

It is not escaped earlier but later, in l() and drupal_attributes().

NaheemSays’s picture

@#4 - can this be set back to rtbc as the question has been answered in comment 8?

John Morahan’s picture

Status: Needs review » Reviewed & tested by the community

Yes, it can. The patch is correct and still applies.

-      '#value' => l(t('View recent blog entries'), "blog/$user->uid", array('attributes' => array('title' => t("Read @username's latest blog entries.", array('@username' => $user->name))))),
+      '#value' => l(t('View recent blog entries'), "blog/$user->uid", array('attributes' => array('title' => t("Read !username's latest blog entries.", array('!username' => $user->name))))),

This is safe because l() passes $options['attributes'] through drupal_attributes, which runs check_plain on the attribute values.

-    drupal_set_breadcrumb(array(l(t('Home'), NULL), l(t('Blogs'), 'blog'), l(t("@name's blog", array('@name' => $node->name)), 'blog/'. $node->uid)));
+    drupal_set_breadcrumb(array(l(t('Home'), NULL), l(t('Blogs'), 'blog'), l(t("!name's blog", array('!name' => $node->name)), 'blog/'. $node->uid)));

This is safe because l() runs check_plain directly on the $text parameter unless $options['html'] is set, which it is not here.

-        'title' => t("@username's blog", array('@username' => $node->name)),
+        '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)))
+        'attributes' => array('title' => t("Read !username's latest blog entries.", array('!username' => $node->name)))

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.

John Morahan’s picture

On 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.

Sophia’s picture

Is 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 :)

Gábor Hojtsy’s picture

Version: 6.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
FileSize
2.35 KB

Ok, 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.

gpk’s picture

Component: user system » documentation

Changing to documentation component.

jhodgdon’s picture

Component: documentation » blog.module

These are code comments - I think this belongs in the blog module for consideration...

TR’s picture

Version: 7.x-dev » 8.x-dev
Status: Patch (to be ported) » Needs review
FileSize
1.57 KB

As 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.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Looks fine to me. 8.x/7.x - no code change, just some in-code comments to clarify what is going on.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x and 7.x. Thanks!

Status: Fixed » Closed (fixed)
Issue tags: -Needs backport to D7

Automatically closed -- issue fixed for 2 weeks with no activity.