SA-CORE-2009-007 user signature format

+    // Set the format of existing signatures to the current default input format.
+    if ($current_default_filter = variable_get('filter_default_format', 0)) {
+      $ret[] = update_sql("UPDATE {users} SET signature_format = ". $current_default_filter);
+    }

I was wondering why the update function does use 0 as the default value. 0 means it will always use the format that is the default when displaying the signature while 1/filter_resolve_format() would stick to the format that was the default when the update has been executed.

I'm not saying it is wrong, I'm just interested in the reason as we are doing something similiar (adding format column in a update function) in privatemsg.module.

Could someone please take a look at this so we can ship alpha 1 with no known security holes? :)

Let's try this, I suppose we don't need an upgrade function and someone from the security team needs to look over this. I have *not* tested the patch.

What confuses me a bit is that we have a http://api.drupal.org/api/function/theme_user_signature/7 but I can't see where we would be using that one.

Additionally, the whole logic is a bit strange, since we load the signature and signature_format hardcoded in comment.module, but we call check_markup on it in user_comment_view().

Ooopsie. Added a missing ,.

Replaced default filter format constant with function...

Two more fixes:

- load filter format before using filter_access
- I'm not sure why, but in some cases, $account->signature_format is unset. Added an additional check.

For people upgrading from earlier versions of 6.x we need to add an update function.

Don't we require that people upgrade to the latest version of D6 before going to D7? Just wondering, that's what I thought.

+++ modules/user/user.module	13 Jan 2010 18:44:07 -0000
@@ -1009,11 +1009,20 @@ function user_account_form(&$form, &$for
+    drupal_set_message(t("The signature input format has been set to a format you don't have access to. It will be changed to a format you have access to when you save this page."));

I believe we're referring to input formats as text formats in the UI now.

Edit: Also, the last sentence isn't quite clear, "It will be changed to a format you have access to when you save this page." I don't have a recommendation yet though.

Subscribing.

Note that http://api.drupal.org/api/function/system_update_6051/7 is still sitting in the D7 codebase (but with D6 code living in it), and tries to do the same update as the function here.

So it seems this should either be removed (or fixed) as part of this patch?

Also, http://api.drupal.org/api/function/filter_update_7005/7 is relevant here. It seems we'd need to do the same kind of update on the {users} table as that function is already doing on other core tables that store text formats in them.

Crosspost.

tests for signatures have been started at: #276582: Tests needed: user.module

.

Fixing component.

#760014: Stop supporting direct updates from Drupal < 6.17 would remove system_update_6050() from D7, and require upgrading from a supported version of D6 before making the move to D7.

This will need updating for the #text_format type.

Updated, cleaned up, and whatnot. Reviews welcome.

Looks straightforward, we don't technically need the update once the issue from #20 is in but it doesn't do any harm either. If we have any tests for user signatures at all then that's probably sufficient but don't have time to look right now.

>90% of this patch doesn't come from me as I merely re-rolled and updated the code. Since it's D6 where this module/security update ultimately happened, and this patch merely applies the same changes, I'm calling this RTBC.

Agreed.

1. See #15 - we don't want to store 0 in the database for existing user signatures, but need to make sure they all get updated to use an actual format.

2. +    '#format' => isset($account->signature_format) ? $account->signature_format : filter_default_format(),
   

   Should this be NULL instead of filter_default_format() - seems to be what other places are using?

3. +    // @todo Use $comment->signature_safe or similar here.
   +    $comment->signature = check_markup($comment->signature, $comment->signature_format, '', TRUE);
   

   I'm not sure I understand the @todo - is this referring to the idea that $comment->signature could become a real field someday? I think the @todo should either be removed or expanded upon...

Not sure I understood your issue with http://api.drupal.org/api/function/filter_update_7005/7 and http://api.drupal.org/api/function/system_update_6051/6 -- effectively, I'd almost rather say that we can drop the module update entirely. If the D6 update needs to be corrected in any way, then that sounds like a separate issue (and too late?).

Therefore, removed that module update.

Also, I reconsidered and realized that the logic was not correct: It only took into account an inaccessible format, but not an invalid format. Furthermore, it tested the permissions of the current user, not the permissions of the user account that is edited.

Lastly, User module doesn't implement hook_filter_format_delete() yet, so deleting an text format leads to the same issue all over again. Since D6 does not provide this hook, the assigned format of existing user signatures after upgrading can still be invalid or inaccessible. The new logic accounts for all of that, and this also means that this code likely as to stay... longer. (I'm still searching someone to discuss and shape a total filter system revamp for D8+...)

Also clarified that @todo in user_comment_view().

Not sure I understood your issue with http://api.drupal.org/api/function/filter_update_7005/7 and http://api.drupal.org/api/function/system_update_6051/6 -- effectively, I'd almost rather say that we can drop the module update entirely. If the D6 update needs to be corrected in any way, then that sounds like a separate issue (and too late?).

The update function was OK for Drupal 6, but we need to correct it for D7 since '0' (FILTER_FORMAT_DEFAULT) is no longer interpreted as anything by Drupal. I think you are right that dropping the module update entirely from the D7 codebase is OK, but we still need to update D6 sites to ensure that the actual format is always stored, rather than '0'.

See the code for the {block_custom} table in http://api.drupal.org/api/function/filter_update_7005/7 -- we essentially need to do the same thing for the {users} table here in the user.module... and hm, come to think of it, that code for {block_custom} should probably be in the block module, shouldn't it :)

By updating the values, the user won't be notified that he needs to update the signature.

WTF? Why does this pass at all?

This means we have zero tests for user signatures.

This should fail.

Good.

But. WTF is that?

      'signature' => array(
        'type' => 'varchar',
        'length' => 255,
        'not null' => TRUE,
        'default' => '',
        'description' => "User's signature.",
      ),

Well, I can see that User module even contains a conditional database schema-based constraint + validation on the signature form value, so as to not exceed the 255 chars. I have no idea why this was done, but it seems the intention was to allow a contrib module to replace the schema column type...

So, I think this patch is ready to fly.

@David: Not sure whether my reply above was understandable -- if we update any values on top of the updates that already ran for D6, then no user will get the message that he needs to update the signature. A signature_format value of 0 will reset the format to NULL and therefore preselect the user's default format, but still needs updating anyway. Therefore I think we do not have to update anything additionally.

if we update any values on top of the updates that already ran for D6, then no user will get the message that he needs to update the signature. A signature_format value of 0 will reset the format to NULL and therefore preselect the user's default format, but still needs updating anyway. Therefore I think we do not have to update anything additionally.

Hm, OK, I see that, but on the other hand, if anyone ever views that user's signature before the user has gone to their account page and reset it, then that will result in a call to check_markup() with "0" passed in as the format ID, which is basically undefined behavior in Drupal 7 right now.

I guess #562694: Text formats should throw an error and refuse to process if a plugin goes missing is the issue that deals with what to do in that undefined situation, but in general I think the consensus is we never want to intentionally pass "0" in for this because it doesn't really mean anything in Drupal 7 (and never really had an intelligent meaning before D7, although at least it had a defined one). For example, your proposal in #562694: Text formats should throw an error and refuse to process if a plugin goes missing involves returning an empty string in this case, which would basically mean that user's signature will never be displayed until the next time they bother to save their user account info (which might be never).

I don't think we want to treat this differently than any of the other formats in core - for all the others we have a database update to replace the undefined "0" format with the actual sitewide default format at the moment of the D6->D7 upgrade, so it seems like we should do the same thing here...

Standardizing security tags.

sounds like David said in #35 that this needs work to remove the 0 and do it The Right Way. (my guess at summarizing this).

The text_format field has changed the way it stores value/format in $form_values: #735662: hook_form_alter() can easily clobber a text format

Updated the patch from #33 to reflect the changes.

#39: drupal.user-signature-format.39.patch queued for re-testing.

The Drupal 6 upgrade does this:

if ($current_default_filter = variable_get('filter_default_format', 0)) {
      $ret[] = update_sql("UPDATE {users} SET signature_format = ". $current_default_filter);
    }

We also decided to stop supporting updates from before this was fixed in Drupal 6, see #760014: Stop supporting direct updates from Drupal < 6.17.

So, we should remove the drupal_set_message() hunk from here, and update filter_update_7005() to include the {users}.signature_format column.

Untested re-roll with that change.

+++ modules/user/user.module	10 Jan 2010 15:00:01 -0000
@@ -1000,11 +1000,20 @@ function user_account_form(&$form, &$for
+    drupal_set_message(t("The signature input format has been set to a format you don't have access to. It will be changed to a format you have access to when you save this page."));

Is this something that mere mortals can make sense of? I don't think my mother would be able to understand what an 'input format' is.

By the way, I looked at this patch and I'm happy with it. It is an important patch to get in so my comment in #45 is not a show-stopper. I'll give this a bit more time for discussion, if not, I'll commit the patch as is.

Re #45 ...How bout:

"You do not have permission to save your signature in that format. A different format will be used when you save this page."

Dries's mom??

Uh, what patch are you all looking at?

That message about input formats is D6 code that has been in D6 for a while, not D7. The patch in #43 doesn't have a drupal_set_message() in it at all.

1. +    $this->filtered_html_format = db_query_range('SELECT * FROM {filter_format} WHERE name = :name', 0, 1, array(':name' => 'Filtered HTML'))->fetchObject();
   

   This is never used in the patch.

2. +    $this->assertNoText(t('Your signature needs to be updated.'));
   

   This is outdated - we no longer have that message in the patch so shouldn't be looking for it in the test.

3. Overall, the test seems like it would be better if it tested something specifically related to this security issue - i.e., view a comment and make sure the signature was filtered with the signature format, rather than the comment format. However, given that we have no signature tests now, this is at least a good start :)

Other than that, everything looks good.

New patch based on #43 that actually tests the page content on a user's comment to be sure that HTML is stripped out when the user's signature format is set to plain text.

Using assertRaw() and assertNoRaw(), and some cleaner test messages suggested by chx.

Nice!

Let's make it even a tiny bit more robust by having the administrator edit the comment after it's created to change the comment itself to Full HTML. That way we direct test the security issue by making sure that the <h1> tags in the signature don't get displayed even when the comment itself has a text format that would allow them.

I also removed the $this->assertNoText(t('Your signature needs to be updated.') line, as per above.

Is this the last beta blocker, I wonder.

Looks great, fixes a critical bug, a critical security bug, and adds tests for user signatures too! Bonus! :D

I was terribly confused by this sentence:

" Consider to use $comment->signature_safe or similar here
+ // and elsewhere."

After consulting with chx, I changed it to:

"Consider renaming to $comment->signature_safe or similar
// here and elsewhere in Drupal 8."

and... committed to HEAD! Thanks! :D

Looks like this wasn't actually committed?

Because of that, here's a reroll with the change from #54 included as part of the patch.

Committed to CVS HEAD. Thanks all.

I'm also moving this one to the 'HEAD to HEAD' project.

+ $this->assertNoText(t('Your signature needs to be updated.'));

This is outdated - we no longer have that message in the patch so shouldn't be looking for it in the test.

Why did we remove that text format check and warning message on user profile view? The bogus text format situation can technically happen anytime...? The innocent site member/user won't have any clue that the user profile needs to be updated to make the user signature re-appear properly formatted!

The patch contained an update function fix to ensure that all signatures updated from Drupal 6 had an actual format assigned, so there shouldn't be any legitimate way to get a bogus text format (or at least no more legitimate than any other format selector in Drupal, and the general case is being handled elsewhere).

The code that was removed looked like this:

+    // If there is a format assigned, but it doesn't exist or is not accessible
+    // for the user account that is edited, unset it, so the user can select a
+    // new one.
+    if (!($format = filter_format_load($account->signature_format)) || !filter_access($format, $account)) {
+      drupal_set_message(t('Your signature needs to be updated.'), 'warning');
+      $account->signature_format = NULL;
+    }

A format that the user does not have access to (the second condition) definitely can happen legitimately if the administrator edits the user's signature, but that just gets handled the standard way now (they get a message in the textarea saying that they don't have permission to edit it), right? Is there a reason signatures should be handled differently from everything else?

By the way, Head2Head issue is here: #824932: Update for user signature_format

Please explain to me why we don't do hook_update_N() to add columns in cases like this. I understand why Head2Head is required in more complex manipulations, but I just don't understand why we don't add the hook_update_N() here.

rfay: Basically, we are trying to get beta 1 out the door as quickly as possible. In order for that to happen, the upgrade path has to work. We were wasting a whole bunch of time trying to support updating from any old arbitrary D6 version to D7, and made a judgment call that from now on, we'd only support upgrading from the latest D6 version (atm, 6.17), which is consistent with what D5 and D6 did as well. This update already exists in the latest version of D6, so is therefore not needed in D7.

The good news is that once beta 1 comes out, we won't need a HEAD to HEAD project, because D7 will support HEAD to HEAD upgrades from then on. Or so is the plan, anyway.

ok, let's see whether we can omit that message. Could basically form-altered in through a contrib/custom module even, if I'm not mistaken.

@rfay: This patch did not contain an update, because the update was already added, committed, and released for Drupal 6, so all existing sites should already have it.

Off topic, but important: It's too early for beta, we still have major API changing issues in the queue, which cannot be solved without API changes. Let's not call something beta just for the sake of the word beta, please.

sun: Add 'em to http://drupal.org/community-initiatives/drupal-core.

subscribe