Remove $check argument from check_markup() [#446518]

modules/block/block.module(228):   $data['content'] = check_markup($block->body, $block->format, '', FALSE);
modules/book/book.test(143):     $this->assertRaw(check_markup($node->body, $node->format), t('Printer friendly body found.'));
modules/comment/comment.module(712):     $text .= '<h2>' . check_plain($comment->subject) . '</h2>' . check_markup($comment->comment, $comment->format, '', FALSE);
modules/comment/comment.module(1755):     $comment_values['subject'] = truncate_utf8(trim(decode_entities(strip_tags(check_markup($comment_values['comment'], $comment_values['comment_format'])))), 29, TRUE);
modules/comment/comment.module(1808):     $comment->comment = check_markup($comment->comment, $comment->format, '', FALSE);
modules/field/modules/text/text.module(114):       $text = isset($item['value']) ? check_markup($item['value'], $item['format'], isset($object->language) ? $object->language : $language->language, $check) : '';
modules/filter/filter.module(427): function check_markup($text, $format = FILTER_FORMAT_DEFAULT, $langcode = '', $check = TRUE) {
modules/node/node.api.php(375):     $text .= '<h2>' . check_plain($comment->subject) . '</h2>' . check_markup($comment->comment, $comment->format, '', FALSE);
modules/node/node.module(1233):     $node->body = check_markup($node->body, $node->format, $node->language, FALSE);
modules/node/node.module(1236):     $node->teaser = check_markup($node->teaser, $node->format, $node->language, FALSE);
modules/profile/profile.module(302):         return check_markup($value);
modules/user/user.module(2511):     $comment->signature = check_markup($comment->signature, $comment->format, '', FALSE);

Thank god we love to type cruft.

Comment	File	Size	Author
#19	drupal.check-markup-access-text.patch	3.78 KB	sun
#19
#12	drupal.check-markup-access.patch	8.82 KB	sun
#12
	drupal.check-markup.patch	6.91 KB	sun

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

chx CreditAttribution: chx commented 27 April 2009 at 19:47

Status:

Needs review

» Closed (won't fix)

Yes we do. This was deliberately made so for security reasons. If you omit that param hten it's secure.

Comment #2

sun

German

Karlsruhe

CreditAttribution: sun commented 27 April 2009 at 20:51

Status:

Closed (won't fix)

» Needs review

Checking which profile can be used and who used it is done on submission, not on viewing, so no permissions checks have to be done when filtering takes place.

Source: http://acko.net/blog/drupal-filter-formats

Initial commit of input formats: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/filter.module?hide...
Commit that added $check argument: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/filter.module?hide...

ok. I will accept any reasoning (if it turns out to [still] be valid). However, if I'm not able to understand why we need this argument (default value), who will?

So, there are 3 possible end-points this issue can have:

1) Documentation on $check is improved, clarifying when to use it, and why, and why the rest of core doesn't make it obsolete.

2) We change the default value, making calls to check_markup() more sane, still leaving the option to set $check = TRUE when developers need it.

3) We remove $check entirely, because it's no longer needed.

But simply marking this issue won't fix won't fix.

Comment #3

chx CreditAttribution: chx commented 27 April 2009 at 21:26

Title:	Negate $check argument to check_markup()	» Remove $check argument from check_markup()
Status:	Needs review	» Needs work

scor dug out the 2005 security archives so now I know why it is there and why it's TRUE. It's there because there was this horrible bug where you could tamper the filter form and execute PHP all over. We fixed this by adding this new parameter and deliberately made it so that stuff breaks, however that in 2005 June. By the time the deliberately made incompatible release got out, it contained the real fix: form API. I am calling now Gerhard and Heine to decide -- do we still need this check? I think not. Let's just get rid of it.

Comment #4

chx CreditAttribution: chx commented 27 April 2009 at 21:31

Also. Steven, I know you still read everything, so I would be very grateful if you would break your silence and say just one word "yes" if you agree with removing this and "no" if you don't.

Comment #5

Gerhard Killesreiter CreditAttribution: Gerhard Killesreiter commented 28 April 2009 at 07:17

I've looked at the cases in core which do not specify the 3rd param to be FALSE. I believe that the check for access is not needed in all three cases. We should do some checks in contrib to see if the situation is different there.

1) comment.module

    $comment_values['subject'] = trim(truncate_utf8(decode_entities(strip_tags(check_markup($comment_values['comment'], $comment_values['format']))), 29, TRUE));

The comment format is protected by FAPI.

2) user.module

      $comment->signature = check_markup($comment->signature, $comment->format);

The format is chosen by the signature owner and checked by FAPI.

3) profile.module

uses check_markup for ensuring html in textarea profile elements. I do not think the 3rd parameter makes sense here since the input format cannot be changed. In case it could be changed, this would be per-profike-owner, so also no concern here.

Comment #6

sun

German

Karlsruhe

CreditAttribution: sun commented 28 April 2009 at 12:07

1) Yes, agreed, was already changed accordingly in the patch.

2) The signature needs to be a separate field with a separate format (as mentioned as @todo in the patch), because the signature does not belong to the comment. To be discussed and fixed in #444090: Unable to read signatures in comments whose input format is not accessible to the current user.

3) Exactly, profile textarea fields need an own format, the current code that checks access for the viewing user makes absolutely no sense at all. However, given that users are fieldable now, I guess and hope that Profile will die anyway.

The only occurrences I could think of in contrib are #after_build functions that may work similar to comment.module's comment preview, but might not invoke FAPI validation manually before outputting the preview. From my understanding, #after_build content previews are the only the case where $check would be required when the code does not ensure that FAPI validation returned no errors.

I bet we can count modules implementing #after_build previews on one hand.

Comment #7

David_Rothstein CreditAttribution: David_Rothstein commented 29 April 2009 at 03:33

In the case of profile.module, it should be OK either way (i.e., the existing code does not need to change), since it's using the default format which everyone already has access to anyway, right? But yes, switching profile.module to use the Field API is the real solution in the long term regardless.... :)

I agree it would be great to get rid of the access parameter entirely. If we wanted to support the (extremely) rare use case where it's needed, we could at least provide a wrapper function that does the access check, rather than leaving this code in check_markup() where it's virtually never going to be used.

Comment #8

sun

German

Karlsruhe

CreditAttribution: sun commented 23 May 2009 at 01:50

We are in luck!

#455724: Rename "check_markup()" tries to change the function name (for good reason) :)

Comment #9

Dries CreditAttribution: Dries commented 28 May 2009 at 09:27

Issue tags:

+Favorite-of-Dries

It would be nice if this could land -- it affects (but doesn't block) #369011: Performance: do check_markup() in text_field_load().

Comment #10

catch

he/him

English

CreditAttribution: catch commented 28 May 2009 at 09:40

Status:

Needs work

» Needs review

This still needs more discussion, but I don't think it actually needs work for anything specific yet. Nudging the test bot.

Comment #11

28 May 2009 at 09:50

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #12

sun

German

Karlsruhe

CreditAttribution: sun commented 20 August 2009 at 02:39

Status:

Needs work

» Needs review

File	Size
drupal.check-markup-access.patch	8.82 KB

New patch removing $check argument from check_markup().

When this has been committed, #455724: Rename "check_markup()" must be bumped to critical, but that's no big deal, because we just have to find a valid, new function name.

Comment #13

20 August 2009 at 07:38

Status:

Needs review

» Needs work

The last submitted patch failed testing.

Comment #14

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 20 August 2009 at 07:41

Status:

Needs work

» Needs review

Oh, testing bot...

Comment #15

catch

he/him

English

CreditAttribution: catch commented 20 August 2009 at 09:19

Status:

Needs review

» Reviewed & tested by the community

Still a great patch, chx, scor and killes have checked there's no security issue with removing it, and the forum signatures issue got fixed with an SA already.

Comment #16

Dries CreditAttribution: Dries commented 20 August 2009 at 10:57

Status:

Reviewed & tested by the community

» Fixed

Not sure how I lost track of this one. Committed!

Let's document this in the upgrade guide, and mark this issue as 'fixed' once documented.

Thanks sun.

Comment #17

sun

German

Karlsruhe

CreditAttribution: sun commented 20 August 2009 at 14:30

Status:	Fixed	» Needs work
Issue tags:		+Needs documentation

Adding tag for documentation.

Also - unless someone withdraws the statement that this security/access check argument for check_markup() needs a rename of the function, #455724: Rename "check_markup()" is bumped to critical.

Comment #18

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 20 August 2009 at 23:06

Let's wait on doing any docs on this one until we see what happens with #455724: Rename "check_markup()".

Comment #19

sun

German

Karlsruhe

CreditAttribution: sun commented 24 August 2009 at 06:07

Status:

Needs work

» Reviewed & tested by the community

File	Size
drupal.check-markup-access-text.patch	3.78 KB

Sorry, looking at the patch and the resulting function signature once again, it's obvious that I've been quite silly. Pretty silly.

Comment #20

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 24 August 2009 at 14:56

Status:

Reviewed & tested by the community

» Needs review

sun, please do not set your own patches to RTBC.

Comment #21

pwolanin CreditAttribution: pwolanin commented 25 August 2009 at 02:20

Status:

Needs review

» Reviewed & tested by the community

this just removes the rather meaningless if/else but doesn't change the functional code. $text might be empty, but I can't see why it would ever be NULL.

Comment #22

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 25 August 2009 at 03:04

Status:

Reviewed & tested by the community

» Needs work

Cool, thanks for the sanity-check.

I *totally* think I remember that branch is there for a reason, but when I tried to look it up in CVS annotate I was directed to: http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/filter/filter.modu... which says "Fixed the filter module." CVS commit message FAIL. :)

Oh, well. I've committed this. I guess if it breaks something else we'll have to ensure we write a test for it. ;)

Back to needs work for docs.

Comment #23

sun

German

Karlsruhe

CreditAttribution: sun commented 7 September 2009 at 02:46

Apparently, no one in here replied to:

The only occurrences I could think of in contrib are #after_build functions that may work similar to comment.module's comment preview, but might not invoke FAPI validation manually before outputting the preview. From my understanding, #after_build content previews are the only the case where $check would be required when the code does not ensure that FAPI validation returned no errors.

I bet we can count modules implementing #after_build previews on one hand.

http://api.drupal.org/api/function/comment_preview/7 still manually checks for form_get_errors() before rendering a preview, so. Well. Actually, I don't even know of any module in contrib that provides previews like Comment module, and I'm also not sure whether the "Preview" button submit handler shouldn't rather set a flag in $form_state along with 'rebuild' => TRUE to trigger a form rebuild from cache that in turn will add a preview in #after_build. That current kind of oh-come-on-I-just-check-errors approach actually feels a bit scary.