The title saved by Drupal core in {blocks}.title is supposed to be treated as plain text, i.e. it should be passed through check_plain() on output.

The block content type passes it through filter_xss_admin() or check_plain(). This is safe due to the filter_xss_admin() call in ctools_content_render(), but it does not give the expected results.

CommentFileSizeAuthor
#6 block-check-plain-4.patch4.38 KBc960657
#2 block-check-plain-2.patch4.05 KBc960657
block-check-plain-1.patch3.6 KBc960657

Comments

merlinofchaos’s picture

merlinofchaos commented
Status: Needs review » Closed (won't fix)

No, the choice to use filter_xss_admin is intentional so that administrators can put HTML in their block titles. Likewise, there is even a comment explaining why strip_tags is used over check_plain(). Brazenly deleting that comment without any explanation of why that comment is inaccurate is not going to be accepted.

c960657’s picture

c960657 commented
Status: Closed (won't fix) » Needs review
StatusFileSize
new block-check-plain-2.patch4.05 KB

Brazenly deleting that comment without any explanation of why that comment is inaccurate is not going to be accepted.

I guess my initial bug report wasn't very elaborate :-) I'll try to explain the problem in more detail. Also, I have updated the patch with some more comments.

HTML in block titles is not supported by Drupal core. What is saved to {block}.title is considered plain text and core always passes the string through check_plain() when it is displayed.

If you create a block and add it to a region on the page using admin/build/block, the block title and block description is passed through check_plain() when the block is displayed by core. E.g. if the block title and block description are set to "About the <br> tag" (i.e. the block contains a short HTML tutorial), this text is what users will see in the list on admin/build/block and on the frontend site.

Now, if you add the same block to a page using Panels, the title is not escaped, so the users will see text "About the tag" with a line-break in it. I.e. the block will have a different title depending on whether it is inserted as a regular block or via Panels.

Using blocks and panels on the same page may be unusual, so - strictly speaking - you could say that Ctools could redefine the meaning of block titles and block descriptions (though I wouldn't recommend this approach). However, this does not apply to blocks created by other modules than core's block module. hook_block() is defined so that $block['info'] is considered to be a plain-text string. Modules expect this to be passed through check_plain() on display. Modules implementing hook_block('list') are not required to be aware of Ctools, so they don't know that Ctools expects them to pass the string through check_plain() themselves.

Even with the patch, the user can still override the title from block defined by the block module using the override_title feature in Ctools. Also, the patch doesn't prevent modules (other than the block module) from returning HTML in $block['subject'] in hook_block('view').

In conclusion, I think there are two related problems:
1. Modules expect $block['info'] to be plain text, so this should always be passed through check_plain() by Ctools.
2. The block module itself considers {block}.title to be plain text, though Ctools could redefine the meaning of this, though with some surprising result (the displayed title depends on how the block is inserted).

I think no. 1 is a bug, but I guess you could argue that no. 2 is by design.

merlinofchaos’s picture

merlinofchaos commented

I'm a bit dubious. I know I had a reason for that strip_tags, but my research suggests it goes all the way back to the very original implementation of Panels and I think that there was a double check_plain() going on. So with some good testing, maybe.

However:

-  if ($module == 'block' && empty($conf['override_title'])) {
-    $block->subject = db_result(db_query("SELECT title FROM {blocks} WHERE module = '%s' AND delta = '%s'", 'block', $delta));
+  if ($module == 'block') {
+    // {block}.title is plain text, but the title returned by this hook should
+    // be HTML.
+    $block->subject = check_plain(db_result(db_query("SELECT title FROM {blocks} WHERE module = '%s' AND delta = '%s'", 'block', $delta)));
   }

That changes behavior and causes $conf['override_title'] to be ignored. This is bad behavior. override_title is an override and should always be respected.

c960657’s picture

c960657 commented

That changes behavior and causes $conf['override_title'] to be ignored.

Sorry, that was actually a separate issue I discovered while testing the patch. AFAICT override_title is handled in ctools_content_render(). The change seems necessary in order to use the %title placeholder in the overridden title.

merlinofchaos’s picture

merlinofchaos commented

Hmm. But that ends up forcing a query when it may not be needed. Panels prides itself on adding as few extra queries as possible.

c960657’s picture

c960657 commented
StatusFileSize
new block-check-plain-4.patch4.38 KB

Now the title look-up is skipped, if the title is overridden and does not use the %title placeholder.

sdboyer’s picture

sdboyer commented
Status: Needs review » Fixed

Latest patch has it all covered, so committed. Thanks!

Status: Fixed » Closed (fixed)

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