Merge lp:~tblue/quam-plures/bug13_fix_credits_disp into lp:quam-plures

[Tilman Blumenbach]

Fixes http://bugs.quamplures.net/cgi-bin/show_bug.cgi?id=13 and adds missing format_to_output() calls to all the _credits.disp.php files in order to make sure that the template does not get messed up by a bad encoding etc and/or bad HTML.

[lp:~tblue/quam-plures/bug13_fix_credits_disp]

7630. By Tilman Blumenbach on 2011-09-13

Merged trunk r7632

[EdB (edb)]

will get this done today (mtz)

[EdB (edb)]

Looks good, installs without issue, cures the common cold. Yay!

review: Approve

[Lee Turner (leeturner)]

I just have a few quick questions on this one:

Should we be formatting the output to allow all html or should we be restricting it to only allow a limited amount or none at all?

If it is to allow all html but not let it get messed up with bad encoding and/or bad html (as the description of the branch admittedly says) then all is cool and I can approve. However, I have just embedded a youtube video in the long description of a plugin which appears in the credits page via an iframe which seems a little wrong to me. I could embed anything in that iframe really.

Also, all of these things apply to the admin as well. None of the descriptions etc have the 'format_to_output' applied to them in the admin code. Admittedly it is not as bigger problem as the admin is limited in access but still - the youtube videos still display.

review: Needs Information

[Lee Turner (leeturner)]

I guess in summary, what I am asking is whether we should be doing:

format_to_output( $a_plugin->long_desc, 'entityencoded' )

so no html is allowed but just displayed as is?

L

review: Needs Information

[Tilman Blumenbach (tblue)]

The reason I chose 'htmlbody' for the output filter is that we use it in other places where we output plugin descriptions etc. as well, so I thought it was appropriate. Admittedly, it allows all HTML and only makes sure things like the charset are correct.

Of course it would be better to restrict the HTML tags that can be used -- not only on the credits page but in the admin interface as well. 'entityencoded' should work, let me check. I will change the code to use that filter if everything works.

[EdB (edb)]

What would be wrong with a youtube video in a template credit? Or a link or a list or an image? As long as it is properly formed and thus doesn't break the page, and as long as we're scrubbing for malicious intent I can't see why we would want to restrict. Well, other than restricting to what works inside a definition list since that is a current use of that info.

[Tilman Blumenbach (tblue)]

Hm. I figured that a template author can sneak in malicious content anyway. Same is true for plugin authors, just use the appropriate hook and output your bad HTML.

[Tilman Blumenbach (tblue)]

...so I guess using 'entityencoded' does not make that much sense after all...?

[EdB (edb)]

Hey wait. So ... what your saying is a bad person might take advantage of our hooks to do bad things? Like, they might just pop in through the wide open front door? And here I thought all we had to do was worry about then looking for sneaky back-door methods to do evil deeds.

:)

I'm quite happy with this branch as-is. We are sanitizing bits that should be, without worrying about OMG they might be evil ... not that concern is inappropriate.

OFF TOPIC: never mind I'll go off topic in a forum thread.

[Lee Turner (leeturner)]

Cool, just thought I would ask.

review: Approve

=== modified file 'qp_templates/_credits.disp.php'
--- qp_templates/_credits.disp.php	2011-08-29 14:22:24 +0000
+++ qp_templates/_credits.disp.php	2011-09-13 14:47:22 +0000
@@ -10,7 +10,6 @@
  * @copyright (c) 2010 by the Quam Plures developers - {@link http://quamplures.net/}
  * @license http://quamplures.net/license.html GNU General Public License (GPL)
  * @package templates
- *
  */
 if( !defined('QP_MAIN_INIT') ) die( 'Please, do not access this page directly.' );
 
@@ -33,11 +32,11 @@
 
 	foreach( $credits as $linkback )
 	{
-		echo '<dt>'.format_to_output( $linkback['pre'], 'htmlbody').'</dt>';
-		echo '<dd><a href="', $linkback['url'].'" title="';
+		echo '<dt>'.format_to_output( $linkback['pre'], 'htmlbody' ).'</dt>';
+		echo '<dd><a href="'.format_to_output( $linkback['url'], 'htmlattr' ).'" title="';
 		echo format_to_output( $linkback['title'], 'htmlattr' ).'">';
 		echo format_to_output( $linkback['text'], 'text' ).'</a></dd><dd>';
-		echo format_to_output( $linkback['post'], 'htmlbody')."</dd>\n";
+		echo format_to_output( $linkback['post'], 'htmlbody' )."</dd>\n";
 	}
 
 	echo '</dl></dd>'."\n";
@@ -59,19 +58,21 @@
 	$p_name = format_to_output( $a_plugin->name, 'htmlbody' );
 	if( $a_plugin->help_url != '' )
 	{
-		$p_name = '<a href="'.$a_plugin->help_url.'" title="'.T_('visit this plugins web page').'">'.$p_name.'</a>';
+		$p_name = '<a href="'.format_to_output( $a_plugin->help_url, 'htmlattr' ).'" title="'.format_to_output( T_('visit this plugins web page'), 'htmlattr' ).'">'.$p_name.'</a>';
 	}
 
 	$a_name = format_to_output( $a_plugin->author, 'htmlbody' );
 	if( $a_plugin->author_url != '' )
 	{
-		$a_name = '<a href="'.$a_plugin->author_url.'" title="'.T_('visit the authors website').'">'.$a_name.'</a>';
+		$a_name = '<a href="'.format_to_output( $a_plugin->author_url, 'htmlattr' ).'" title="'.format_to_output( T_('visit the authors website'), 'htmlattr' ).'">'.$a_name.'</a>';
 	}
 
 	$credits[ strtolower( $a_plugin->name ) ] = array(
 		'p_name' => $p_name,
 		'a_name' => $a_name,
-		'desc' => $a_plugin->long_desc,
+		// Looks like we usually allow HTML in plugin descriptions.
+		// We still need to make sure no bad chars mess up the template/encoding.
+		'desc' => format_to_output( $a_plugin->long_desc, 'htmlbody' ),
 	);
 }
 
@@ -103,7 +104,7 @@
 	echo '<dd><dl>'."\n";
 	foreach( $_hic_sunt_dracones as $dracone )
 	{
-		echo '<dt><a href="'.$dracone['homepage'].'" title="';
+		echo '<dt><a href="'.format_to_output( $dracone['homepage'], 'htmlattr' ).'" title="';
 		echo format_to_output( $dracone['homepage'], 'htmlattr' ).'">';
 		echo format_to_output( $dracone['name'], 'text' ).'</a></dt><dd>';
 		locale_flag( $dracone['locale'], 'h10px', 'leftmargin' );
@@ -115,9 +116,9 @@
 // Finally: QP's only linkback
 if( isset( $Template ) )
 {
-	echo '<dt>'.$Template->poweredby_linkback['pre'].':</dt>'."\n";
+	echo '<dt>'.format_to_output( $Template->poweredby_linkback['pre'], 'htmlbody' ).':</dt>'."\n";
 	echo '<dd><dl>', "\n";
-	echo '<dt><a href="', $Template->poweredby_linkback['url'], '" title="';
+	echo '<dt><a href="'.format_to_output( $Template->poweredby_linkback['url'], 'htmlattr' ).'" title="';
 	echo format_to_output( $Template->poweredby_linkback['title'], 'htmlattr' ), '">';
 	echo format_to_output( $Template->poweredby_linkback['text'], 'text' ), '</a></dt><dd>';
 	echo format_to_output( $Template->poweredby_linkback['post'], 'htmlbody'), "</dd>\n";

=== modified file 'qp_templates/basic/_credits.disp.php'
--- qp_templates/basic/_credits.disp.php	2011-08-29 14:22:24 +0000
+++ qp_templates/basic/_credits.disp.php	2011-09-13 14:47:22 +0000
@@ -38,11 +38,11 @@
 	echo '<dd><dl>'."\n";
 	foreach( $credits as $linkback )
 	{
-		echo '<dt>'.format_to_output( $linkback['pre'], 'htmlbody').'</dt>';
-		echo '<dd><a href="', $linkback['url'].'" title="';
+		echo '<dt>'.format_to_output( $linkback['pre'], 'htmlbody' ).'</dt>';
+		echo '<dd><a href="'.format_to_output( $linkback['url'], 'htmlattr' ).'" title="';
 		echo format_to_output( $linkback['title'], 'htmlattr' ).'">';
 		echo format_to_output( $linkback['text'], 'text' ).'</a></dd><dd>';
-		echo format_to_output( $linkback['post'], 'htmlbody')."</dd>\n";
+		echo format_to_output( $linkback['post'], 'htmlbody' )."</dd>\n";
 	}
 	echo '</dl></dd>'."\n";
 }
@@ -63,19 +63,21 @@
 	$p_name = format_to_output( $a_plugin->name, 'htmlbody' );
 	if( $a_plugin->help_url != '' )
 	{
-		$p_name = '<a href="'.$a_plugin->help_url.'" title="'.T_('visit this plugins web page').'">'.$p_name.'</a>';
+		$p_name = '<a href="'.format_to_output( $a_plugin->help_url, 'htmlattr' ).'" title="'.format_to_output( T_('visit this plugins web page'), 'htmlattr' ).'">'.$p_name.'</a>';
 	}
 
 	$a_name = format_to_output( $a_plugin->author, 'htmlbody' );
 	if( $a_plugin->author_url != '' )
 	{
-		$a_name = '<a href="'.$a_plugin->author_url.'" title="'.T_('visit the authors website').'">'.$a_name.'</a>';
+		$a_name = '<a href="'.format_to_output( $a_plugin->author_url, 'htmlattr' ).'" title="'.format_to_output( T_('visit the authors website'), 'htmlattr' ).'">'.$a_name.'</a>';
 	}
 
 	$credits[ strtolower( $a_plugin->name ) ] = array(
 		'p_name' => $p_name,
 		'a_name' => $a_name,
-		'desc' => $a_plugin->long_desc,
+		// Looks like we usually allow HTML in plugin descriptions.
+		// We still need to make sure no bad chars mess up the template/encoding.
+		'desc' => format_to_output( $a_plugin->long_desc, 'htmlbody' ),
 	);
 }
 
@@ -107,7 +109,7 @@
 	echo '<dd><dl>'."\n";
 	foreach( $_hic_sunt_dracones as $dracone )
 	{
-		echo '<dt><a href="'.$dracone['homepage'].'" title="';
+		echo '<dt><a href="'.format_to_output( $dracone['homepage'], 'htmlattr' ).'" title="';
 		echo format_to_output( $dracone['homepage'], 'htmlattr' ).'">';
 		echo format_to_output( $dracone['name'], 'text' ).'</a></dt><dd>';
 		locale_flag( $dracone['locale'], 'h10px', 'leftmargin' );
@@ -119,9 +121,9 @@
 // Finally: QP's only linkback
 if( isset( $Template ) )
 {
-	echo '<dt>'.$Template->poweredby_linkback['pre'].':</dt>'."\n";
+	echo '<dt>'.format_to_output( $Template->poweredby_linkback['pre'], 'htmlbody' ).':</dt>'."\n";
 	echo '<dd><dl>', "\n";
-	echo '<dt><a href="', $Template->poweredby_linkback['url'], '" title="';
+	echo '<dt><a href="'.format_to_output( $Template->poweredby_linkback['url'], 'htmlattr' ).'" title="';
 	echo format_to_output( $Template->poweredby_linkback['title'], 'htmlattr' ), '">';
 	echo format_to_output( $Template->poweredby_linkback['text'], 'text' ), '</a></dt><dd>';
 	echo format_to_output( $Template->poweredby_linkback['post'], 'htmlbody'), "</dd>\n";