Quam Plures

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

  1. bug13_fix_credits_disp
  2. Merge into trunk
Proposed by Tilman Blumenbach
Status: Merged
Merged at revision: 7634
Proposed branch: lp:~tblue/quam-plures/bug13_fix_credits_disp
Merge into: lp:quam-plures
Diff against target: 137 lines (+22/-19)
2 files modified
qp_templates/_credits.disp.php (+11/-10)
qp_templates/basic/_credits.disp.php (+11/-9)
To merge this branch: bzr merge lp:~tblue/quam-plures/bug13_fix_credits_disp
Reviewer Review Type Date Requested Status
Lee Turner (community) Approve
EdB Approve
Review via email: mp+74485@code.launchpad.net

Description of the change

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.

To post a comment you must log in.
lp:~tblue/quam-plures/bug13_fix_credits_disp updated
7630. By Tilman Blumenbach

Merged trunk r7632

Revision history for this message
EdB (edb) wrote :

will get this done today (mtz)

Revision history for this message
EdB (edb) wrote :

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

review: Approve
Revision history for this message
Lee Turner (leeturner) wrote :

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
Revision history for this message
Lee Turner (leeturner) wrote :

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
Revision history for this message
Tilman Blumenbach (tblue) wrote :

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.

Revision history for this message
EdB (edb) wrote :

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.

Revision history for this message
Tilman Blumenbach (tblue) wrote :

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.

Revision history for this message
Tilman Blumenbach (tblue) wrote :

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

Revision history for this message
EdB (edb) wrote :

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.

Revision history for this message
Lee Turner (leeturner) wrote :

Cool, just thought I would ask.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
10 * @copyright (c) 2010 by the Quam Plures developers - {@link http://quamplures.net/}10 * @copyright (c) 2010 by the Quam Plures developers - {@link http://quamplures.net/}
11 * @license http://quamplures.net/license.html GNU General Public License (GPL)11 * @license http://quamplures.net/license.html GNU General Public License (GPL)
12 * @package templates12 * @package templates
13 *
14 */13 */
15if( !defined('QP_MAIN_INIT') ) die( 'Please, do not access this page directly.' );14if( !defined('QP_MAIN_INIT') ) die( 'Please, do not access this page directly.' );
1615
@@ -33,11 +32,11 @@
3332
34 foreach( $credits as $linkback )33 foreach( $credits as $linkback )
35 {34 {
36 echo '<dt>'.format_to_output( $linkback['pre'], 'htmlbody').'</dt>';35 echo '<dt>'.format_to_output( $linkback['pre'], 'htmlbody' ).'</dt>';
37 echo '<dd><a href="', $linkback['url'].'" title="';36 echo '<dd><a href="'.format_to_output( $linkback['url'], 'htmlattr' ).'" title="';
38 echo format_to_output( $linkback['title'], 'htmlattr' ).'">';37 echo format_to_output( $linkback['title'], 'htmlattr' ).'">';
39 echo format_to_output( $linkback['text'], 'text' ).'</a></dd><dd>';38 echo format_to_output( $linkback['text'], 'text' ).'</a></dd><dd>';
40 echo format_to_output( $linkback['post'], 'htmlbody')."</dd>\n";39 echo format_to_output( $linkback['post'], 'htmlbody' )."</dd>\n";
41 }40 }
4241
43 echo '</dl></dd>'."\n";42 echo '</dl></dd>'."\n";
@@ -59,19 +58,21 @@
59 $p_name = format_to_output( $a_plugin->name, 'htmlbody' );58 $p_name = format_to_output( $a_plugin->name, 'htmlbody' );
60 if( $a_plugin->help_url != '' )59 if( $a_plugin->help_url != '' )
61 {60 {
62 $p_name = '<a href="'.$a_plugin->help_url.'" title="'.T_('visit this plugins web page').'">'.$p_name.'</a>';61 $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>';
63 }62 }
6463
65 $a_name = format_to_output( $a_plugin->author, 'htmlbody' );64 $a_name = format_to_output( $a_plugin->author, 'htmlbody' );
66 if( $a_plugin->author_url != '' )65 if( $a_plugin->author_url != '' )
67 {66 {
68 $a_name = '<a href="'.$a_plugin->author_url.'" title="'.T_('visit the authors website').'">'.$a_name.'</a>';67 $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>';
69 }68 }
7069
71 $credits[ strtolower( $a_plugin->name ) ] = array(70 $credits[ strtolower( $a_plugin->name ) ] = array(
72 'p_name' => $p_name,71 'p_name' => $p_name,
73 'a_name' => $a_name,72 'a_name' => $a_name,
74 'desc' => $a_plugin->long_desc,73 // Looks like we usually allow HTML in plugin descriptions.
74 // We still need to make sure no bad chars mess up the template/encoding.
75 'desc' => format_to_output( $a_plugin->long_desc, 'htmlbody' ),
75 );76 );
76}77}
7778
@@ -103,7 +104,7 @@
103 echo '<dd><dl>'."\n";104 echo '<dd><dl>'."\n";
104 foreach( $_hic_sunt_dracones as $dracone )105 foreach( $_hic_sunt_dracones as $dracone )
105 {106 {
106 echo '<dt><a href="'.$dracone['homepage'].'" title="';107 echo '<dt><a href="'.format_to_output( $dracone['homepage'], 'htmlattr' ).'" title="';
107 echo format_to_output( $dracone['homepage'], 'htmlattr' ).'">';108 echo format_to_output( $dracone['homepage'], 'htmlattr' ).'">';
108 echo format_to_output( $dracone['name'], 'text' ).'</a></dt><dd>';109 echo format_to_output( $dracone['name'], 'text' ).'</a></dt><dd>';
109 locale_flag( $dracone['locale'], 'h10px', 'leftmargin' );110 locale_flag( $dracone['locale'], 'h10px', 'leftmargin' );
@@ -115,9 +116,9 @@
115// Finally: QP's only linkback116// Finally: QP's only linkback
116if( isset( $Template ) )117if( isset( $Template ) )
117{118{
118 echo '<dt>'.$Template->poweredby_linkback['pre'].':</dt>'."\n";119 echo '<dt>'.format_to_output( $Template->poweredby_linkback['pre'], 'htmlbody' ).':</dt>'."\n";
119 echo '<dd><dl>', "\n";120 echo '<dd><dl>', "\n";
120 echo '<dt><a href="', $Template->poweredby_linkback['url'], '" title="';121 echo '<dt><a href="'.format_to_output( $Template->poweredby_linkback['url'], 'htmlattr' ).'" title="';
121 echo format_to_output( $Template->poweredby_linkback['title'], 'htmlattr' ), '">';122 echo format_to_output( $Template->poweredby_linkback['title'], 'htmlattr' ), '">';
122 echo format_to_output( $Template->poweredby_linkback['text'], 'text' ), '</a></dt><dd>';123 echo format_to_output( $Template->poweredby_linkback['text'], 'text' ), '</a></dt><dd>';
123 echo format_to_output( $Template->poweredby_linkback['post'], 'htmlbody'), "</dd>\n";124 echo format_to_output( $Template->poweredby_linkback['post'], 'htmlbody'), "</dd>\n";
124125
=== 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 @@
38 echo '<dd><dl>'."\n";38 echo '<dd><dl>'."\n";
39 foreach( $credits as $linkback )39 foreach( $credits as $linkback )
40 {40 {
41 echo '<dt>'.format_to_output( $linkback['pre'], 'htmlbody').'</dt>';41 echo '<dt>'.format_to_output( $linkback['pre'], 'htmlbody' ).'</dt>';
42 echo '<dd><a href="', $linkback['url'].'" title="';42 echo '<dd><a href="'.format_to_output( $linkback['url'], 'htmlattr' ).'" title="';
43 echo format_to_output( $linkback['title'], 'htmlattr' ).'">';43 echo format_to_output( $linkback['title'], 'htmlattr' ).'">';
44 echo format_to_output( $linkback['text'], 'text' ).'</a></dd><dd>';44 echo format_to_output( $linkback['text'], 'text' ).'</a></dd><dd>';
45 echo format_to_output( $linkback['post'], 'htmlbody')."</dd>\n";45 echo format_to_output( $linkback['post'], 'htmlbody' )."</dd>\n";
46 }46 }
47 echo '</dl></dd>'."\n";47 echo '</dl></dd>'."\n";
48}48}
@@ -63,19 +63,21 @@
63 $p_name = format_to_output( $a_plugin->name, 'htmlbody' );63 $p_name = format_to_output( $a_plugin->name, 'htmlbody' );
64 if( $a_plugin->help_url != '' )64 if( $a_plugin->help_url != '' )
65 {65 {
66 $p_name = '<a href="'.$a_plugin->help_url.'" title="'.T_('visit this plugins web page').'">'.$p_name.'</a>';66 $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>';
67 }67 }
6868
69 $a_name = format_to_output( $a_plugin->author, 'htmlbody' );69 $a_name = format_to_output( $a_plugin->author, 'htmlbody' );
70 if( $a_plugin->author_url != '' )70 if( $a_plugin->author_url != '' )
71 {71 {
72 $a_name = '<a href="'.$a_plugin->author_url.'" title="'.T_('visit the authors website').'">'.$a_name.'</a>';72 $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>';
73 }73 }
7474
75 $credits[ strtolower( $a_plugin->name ) ] = array(75 $credits[ strtolower( $a_plugin->name ) ] = array(
76 'p_name' => $p_name,76 'p_name' => $p_name,
77 'a_name' => $a_name,77 'a_name' => $a_name,
78 'desc' => $a_plugin->long_desc,78 // Looks like we usually allow HTML in plugin descriptions.
79 // We still need to make sure no bad chars mess up the template/encoding.
80 'desc' => format_to_output( $a_plugin->long_desc, 'htmlbody' ),
79 );81 );
80}82}
8183
@@ -107,7 +109,7 @@
107 echo '<dd><dl>'."\n";109 echo '<dd><dl>'."\n";
108 foreach( $_hic_sunt_dracones as $dracone )110 foreach( $_hic_sunt_dracones as $dracone )
109 {111 {
110 echo '<dt><a href="'.$dracone['homepage'].'" title="';112 echo '<dt><a href="'.format_to_output( $dracone['homepage'], 'htmlattr' ).'" title="';
111 echo format_to_output( $dracone['homepage'], 'htmlattr' ).'">';113 echo format_to_output( $dracone['homepage'], 'htmlattr' ).'">';
112 echo format_to_output( $dracone['name'], 'text' ).'</a></dt><dd>';114 echo format_to_output( $dracone['name'], 'text' ).'</a></dt><dd>';
113 locale_flag( $dracone['locale'], 'h10px', 'leftmargin' );115 locale_flag( $dracone['locale'], 'h10px', 'leftmargin' );
@@ -119,9 +121,9 @@
119// Finally: QP's only linkback121// Finally: QP's only linkback
120if( isset( $Template ) )122if( isset( $Template ) )
121{123{
122 echo '<dt>'.$Template->poweredby_linkback['pre'].':</dt>'."\n";124 echo '<dt>'.format_to_output( $Template->poweredby_linkback['pre'], 'htmlbody' ).':</dt>'."\n";
123 echo '<dd><dl>', "\n";125 echo '<dd><dl>', "\n";
124 echo '<dt><a href="', $Template->poweredby_linkback['url'], '" title="';126 echo '<dt><a href="'.format_to_output( $Template->poweredby_linkback['url'], 'htmlattr' ).'" title="';
125 echo format_to_output( $Template->poweredby_linkback['title'], 'htmlattr' ), '">';127 echo format_to_output( $Template->poweredby_linkback['title'], 'htmlattr' ), '">';
126 echo format_to_output( $Template->poweredby_linkback['text'], 'text' ), '</a></dt><dd>';128 echo format_to_output( $Template->poweredby_linkback['text'], 'text' ), '</a></dt><dd>';
127 echo format_to_output( $Template->poweredby_linkback['post'], 'htmlbody'), "</dd>\n";129 echo format_to_output( $Template->poweredby_linkback['post'], 'htmlbody'), "</dd>\n";

Subscribers

People subscribed via source and target branches