Merge lp:~quam-plures-core/quam-plures/posting_xhtml-fixes into lp:quam-plures

[Tilman Blumenbach]

Discussion: http://forums.quamplures.net/viewtopic.php?f=11&t=809
---------------------

This branch cleans up some of the mess that posting and validating XHTML in QP currently is.

For example, this branch allows using HTML comments (<!-- ... -->) when posting (that was not possible until now). However, blog comments may not contain HTML comments -- they are converted to visible HTML (i. e. &lt;-- ... --&gt;). While HTML comments make sense in posts, I don't think that's the case for blog comments. It would just confuse users not familiar with HTML.

An important fix I incorporated into this branch is that all XHTML needs to be well-formed: Unclosed comments etc. are rejected. In the past, you could post blog comments simply containing "<!--", which would comment out everything after the blog comment (i. e. the rest of the page). Not good at all (shhh, I didn't tell you that!).

Finally, remember bug 736035? The fix we committed back then certainly works, but it's ugly. This branch has a better solution: Processing instructions (<?foo ... ?>) in posts are allowed as long as they contain valid XHTML (however, comments cannot contain processing instructions). Let me explain:

Something like "<?<a><script>alert(0)</script>" (from the POC) would fail because it's not well-formed. But even if you make it well-formed ("<?foo <script>alert(0)</script> ?>") our XHTML parser would detect the forbidden <script> tag. Hah! Gotcha!

In other words, something like "<?foo <a href="http://google.com">Goooooooogle</a> ?>" is fine, but dangerous processing instructions like that one above are not. After all, processing instructions *could* be useful for some users.

Of course, that violates the XML spec. Processing instructions contain application-specific data so strictly speaking, we cannot assume that data to be XHTML. Well, if we don't, we make it possible to perform XSS attacks.

A clean solution would be to disallow processing instructions completely, but what if somebody finds a legitimate use for them? I don't want to restrict users.

[EdB (edb)]

Possiubly unrelated to this branch, and excuse me for not checking, but there are some really simple things I wanted to fix regarding xhtml trans and strict. I wonder if you'd mind adding them if they are not in this branch?

Simple stuff, and I'll find a file. Attributes get added to an image that are not xhtml strict even though 10 lines above a check is made for "if strict do it this way", and then the image tags get closed with "/>" instead of " />". It involves a minor style change and (again) I can't recall exactly why.

Anyway if you're up for adding a potentially unrelated yet slightly related bit I'd be happy to try to find the right file and bits for you.

[Tilman Blumenbach (tblue)]

Sure, why not. Go ahead. :)

[EdB (edb)]

will do a forum thread due to it is easy to put code bits into that system than here ... although technically it is just copy/paste but the end result is easier to read is the thing :)

[EdB (edb)]

Finally downloading this branch. If I can remember how to make stuff run on localhost I'll get it tested today. If not I'll figure that stuff out today and get this branch tested tomorrow.

[EdB (edb)]

Finally tried a test on this and I don't see where it fails, but I don't see what gain we get from it. Specifically, I tried posting some php code and nothing happened. Which sort of makes sense given that spitting out the item's content is part of what the server does, but it doesn't revisit the content to see if new instructions exist.

<?php echo "http://localhost/branches_from_others/posting_xhtml-fixes/media/blogs/id_1/waterfall.jpg"; ?> doesn't post the image - it just gets ignored.

Adding some text then a comment then some text worked, but each bit of text got wrapped in it's own P tag.

hello world! <!-- this is a comment -->how's it going? created 2 paragraphs is what I mean. I'm not sure that is what a user would expect when adding an inline comment ya know?

As I said I don't see a problem, but I'm also not sure I see the gain. Other than the "shhh!" issue, which clearly needed to be fixed. Can you show me a case where enabling code in posts actually works? Even if it isn't very realistic (like php to pop in an image), just something that shows how it is a gain.

Gonna leave this as a comment instead of approving because I'm a bit worried that somehow we might have bit ourselves in the ass. Hopefully Yabs can look at it from a security angle cuz I pretty much can't. Malicious mean-spirited code is much more a Yabs thing ;) And if Yabs is okay with this branch then I have no objection and will gladly do the merge thing.

[EdB (edb)]

OT: you tinkered in an area I have a heavy tinker in, so it is going to kill me to make it be "mine" again. The html_sanity_check function is where I added a bit for one day going to xhtml(trans,strict) and/or html5. Such is life eh? Hopefully I'll be able to deal with it smartly later on :)

[Tilman Blumenbach (tblue)]

> <?php echo "http://localhost/branches_from_others/posting_xhtml-fixes/media/blogs/id_1/waterfall.jpg"; ?> doesn't post the image - it just gets ignored.

Yes, but that's not my fault. The PHP code isn't actually parsed, it's treated as HTML and since that's invalid HTML, you don't see an image. You'd have to use an ordinary <img> tag for that, as usual. It's always been this way.

So what's the purpose of this branch? As you said, for one, it allows us to use comments. Fine, the plugin that makes the <p> tags seems to have its problems with that, I guess I'll have to fix it: Comments should be ignored completely.

Additionally, this branch implements a cleaner fix for the security vulnerability issue we had a while ago. Back then, we basically disallowed everything looking like "<? ... ?>". I think that's bad as such constructs may -- theoretically -- be of use to somebody out there. So what I did is to allow such constructs, but only if they are safe.

Okay, let yabs look at it, I wanted him to do that anyway. ;)

[EdB (edb)]

The P tag thing isn't mission-critical but if it seems an easy fix that'd be cool.

My only concern is that I don't know how to look for security issues that might (but probably haven't) been opened up. I got now in my head a great idea: if Yabs can't get to this before I go back out on the road I'll do the merge thing. That's a couple of days unless I find something that doesn't involve going out on the road, but in that case it'll still be a couple of days cuz it's easier than actually trying to plan :)

[Tilman Blumenbach (tblue)]

I will look at the <p> tag issue now.

[EdB (edb)]

I just remembered that the original creative commons plugin used an html comment to trigger the renderer, so I did a v000 install and added a comment inline. It allowed the comment AND broke the paragraph around it. So this P tag behavior is not a new thing.

In the CC plugin the licensing went at the end so no one noticed that it split content around it.

[Tilman Blumenbach (tblue)]

I think I have fixed the <p> tag issue. Maybe it's not a perfect solution, but it seems to work. I hope it works together with other plugins, too.

review: Needs Resubmitting

[Tilman Blumenbach (tblue)]

Well, now it's messing up HTML comments spanning multiple lines.

I feel that the Auto-P plugin needs an overhaul, it's messy. :(

[EdB (edb)]

For what it's worth, I'd say leave it as-is for now since the observed behavior (undesirable though it may be) is not induced by this branch. I just happened to notice it when testing.

I may be wrong, but there have been problems observed with Auto-P ever since forever. Especially when it comes time to interact with other rendering plugins. Hoo boy does that ever get messy, and you can often change symptoms by changing plugin priority numbers.

[Tilman Blumenbach (tblue)]

Good, then let's merge this, it kinda works anyway. Let's hope nobody uses multiline HTML comments in the near future.

[Tilman Blumenbach (tblue)]

I will look at making the Auto-P plugin work correctly soon... So many things to do: Anti-CSRF tokens (aka nonces), fixing Auto-P... *cough*

[lp:~quam-plures-core/quam-plures/posting_xhtml-fixes]

7617. By Tilman Blumenbach on 2011-06-20

Reverted changes to Auto-P plugin, will fix it in another branch

7618. By Tilman Blumenbach on 2011-06-20

Merged trunk

[EdB (edb)]

24 hour warning!

If anyone has any desire to further test this branch please do so asap! I doubt they have a truck for me but they might, so I might be on the road again soon. That means I need to get this merge done even though I personally don't know how to really really REALLY test it for possible undesirable side effects. It works and Tblue knows what he's doing, but heck we've burned ourselves before so ... anyway I'd love to see someone else test this branch but if not it'll get merged in 24 hours (more or less).

[Yabs (yabs)]

Bugger, I'll attempt to check this within 24 hours .. tad hectic here atm though

If I don't make the deadline then take my (cursory) glance at code as "approved" .. tblue usually knows what he's on about

¥

[EdB (edb)]

I thought for sure I put an approved on this, but obviously not.

I'll do the dracones branch now, and wait the rest of today for Yabs to take a look. Else I'll merge this one this evening (MTZ).

review: Approve

=== modified file 'qp_inc/_core/_misc.funcs.php'
--- qp_inc/_core/_misc.funcs.php	2011-01-31 22:13:02 +0000
+++ qp_inc/_core/_misc.funcs.php	2011-06-20 11:14:51 +0000
@@ -2645,11 +2645,6 @@
 		case 'imgtag':
 			$r = '<img src="'.$rsc_url.$icon['file'].'" ';
 
-			if( !$use_strict )
-			{	// Include non CSS fallbacks - transitional only:
-				$r .= 'border="0" align="top" ';
-			}
-
 			// Include class (will default to "icon"):
 			if( ! isset( $params['class'] ) )
 			{
@@ -2659,7 +2654,7 @@
 				}
 				else
 				{
-					$params['class'] = '';
+					$params['class'] = 'middle';
 				}
 			}
 
@@ -2686,7 +2681,7 @@
 			$r .= get_field_attribs_as_string( $params, false );
 
 			// Close tag:
-			$r .= '/>';
+			$r .= ' />';
 
 
 			if( $include_in_legend && ( $IconLegend = & get_IconLegend() ) )
@@ -2702,11 +2697,7 @@
 
 			$r = '<img src="'.$rsc_url.$blank_icon['file'].'" ';
 
-			// TODO: dh> add this only for !$use_strict, like above?
-			// Include non CSS fallbacks (needed by bozos... and basic template):
-			$r .= 'border="0" align="top" ';
-
-			// Include class (will default to "noicon"):
+			// Include class (will default to "no_icon"):
 			if( ! isset( $params['class'] ) )
 			{
 				if( isset($icon['class']) )
@@ -2735,7 +2726,7 @@
 			$r .= get_field_attribs_as_string( $params, false );
 
 			// Close tag:
-			$r .= '/>';
+			$r .= ' />';
 
 			return $r;
 			/* BREAK */

=== modified file 'qp_inc/_core/_param.funcs.php'
--- qp_inc/_core/_param.funcs.php	2011-03-16 14:56:32 +0000
+++ qp_inc/_core/_param.funcs.php	2011-06-20 11:14:51 +0000
@@ -1626,12 +1626,15 @@
  * It is NOT (necessarily) safe to use the output.
  *
  * @param string The content to format
- * @param string
+ * @param string Context: "posting", "xmlrpc_posting" or "commenting"
  * @param integer Create automated <br /> tags?
  * @param string Encoding (used for XHTML_Validator only!); defaults to $io_charset
+ * @param boolean When "posting" or "xmlrpc_posting", should comments be converted to
+ *                visible HTML? Forced to true when "commenting".
  * @return boolean|string
  */
-function check_html_sanity( $content, $context = 'posting', $autobr = false, $encoding = NULL )
+function check_html_sanity( $content, $context = 'posting', $autobr = false,
+							$encoding = NULL, $escape_comments = false )
 {
 	global $use_balanceTags, $admin_url;
 	global $io_charset, $use_xhtmlvalidation_for_comments, $comment_allowed_tags, $comments_allow_css_tweaks;
@@ -1679,10 +1682,12 @@
 
 	$error = false;
 
+	// NOTE: These RegExps match NameStartChar and NameChar, but exclude
+	//       higher characters (i. e. UTF-8). That's pure laziness: FIXME.
 	// Replace any & that is not a character or entity reference with &amp;
 	$content = preg_replace( '/&(?!#[0-9]+;|#x[0-9a-fA-F]+;|[a-zA-Z_:][a-zA-Z0-9._:-]*;)/', '&amp;', $content );
-	// Fix for smileys like LOVE <3 (and other situations with '<' before a number).
-	$content = preg_replace( '#<([0-9])#', '&lt;$1', $content );
+	// Escape any '<'s that are not used in a valid manner:
+	$content = preg_replace( '#<(?![!?/]|[a-zA-Z_:][a-zA-Z0-9._:-]*)#', '&lt;', $content );
 
 	// ANTISPAM check:
 	if( ! $bypass_antispam
@@ -1719,6 +1724,30 @@
 		$content = balance_tags( $content );
 	}
 
+	if( $context == 'commenting' || $escape_comments )
+	{	// Comments, processing instructions etc. are not allowed when commenting.
+		$content = str_replace( array(
+				// Comments, CDATA sections and various declarations.
+				// Declarations end with a single '>' which makes it hard
+				// to correctly detect their closing tags. Thus, we don't
+				// even try to do that (FIXME).
+				'<!',
+				// Closing comments
+				'-->',
+				// Closing CDATA sections
+				']]>',
+				// Processing instructions
+				'<?',
+				'?>',
+			), array(
+				'&lt;!',
+				'--&gt;',
+				']]&gt;',
+				'&lt;?',
+				'?&gt;',
+			), $content );
+	}
+
 	if( $xhtmlvalidation )
 	{ // We want to validate XHTML:
 		load_class( 'xhtml_validator/_xhtml_validator.class.php' );
@@ -1739,49 +1768,80 @@
 		{	// DEPRECATED but still...
 			$content = strip_tags( $content, $comment_allowed_tags );
 		}
-
-		// Security checking:
-		$check = $content;
-		// Open comments or '<![CDATA[' are dangerous
-		$check = str_replace('<!', '<', $check);
-		// # # are delimiters
-		// i modifier at the end means caseless
-
-		// CHECK Styling restictions:
-		if( ! $allow_css_tweaks
-			&& preg_match( '#\s((style|class|id)\s*=)#i', $check, $matches) )
-		{
-			$Messages->add( T_('Illegal CSS markup found: ').htmlspecialchars($matches[1]), 'error' );
-			$error = true;
-		}
-
-		// CHECK JAVASCRIPT:
-		if( ! $allow_javascript
-			&& ( preg_match( '¤( < \s* //? \s* (script|noscript) )¤xi', $check, $matches )
-				|| preg_match( '#\s((on[a-z]+)\s*=)#i', $check, $matches )
-				// action=, background=, cite=, classid=, codebase=, data=, href=, longdesc=, profile=, src=, usemap=
-				|| preg_match( '#=["\'\s]*((javascript|vbscript|about):)#i', $check, $matches ) ) )
-		{
-			$Messages->add( T_('Illegal javascript markup found: ').htmlspecialchars($matches[1]), 'error' );
-			$error = true;
-		}
-
-		// CHECK IFRAMES:
-		if( ! $allow_iframes
-			&& preg_match( '¤( < \s* //? \s* (frame|iframe) )¤xi', $check, $matches) )
-		{
-			$Messages->add( T_('Illegal frame markup found: ').htmlspecialchars($matches[1]), 'error' );
-			$error = true;
-		}
-
-		// CHECK OBJECTS:
-		if( ! $allow_objects
-			&& preg_match( '¤( < \s* //? \s* (applet|object|param|embed) )¤xi', $check, $matches) )
-		{
-			$Messages->add( T_('Illegal object markup found: ').htmlspecialchars($matches[1]), 'error' );
-			$error = true;
-		}
-
+		else if( ! $escape_comments
+				&& preg_match_all( '#(?:<!--|<!\[CDATA\[|<\?)#i', $content, $matches, PREG_OFFSET_CAPTURE ) )
+		{	// We are posting an item, try an el-cheapo check for unclosed
+			// comments, processing instructions etc.
+			// Declarations end with a single '>' which is hard to detect
+			// correctly, so we don't try to validate them here. In theory,
+			// this may be an issue, but apparently, it's not a big threat.
+			static $search = array(
+				'<!--'      => '-->',
+				'<![CDATA[' => ']]>',
+				'<?'        => '?>',
+			);
+
+			$last_offset = 0;
+			foreach( $matches[0] as $m )
+			{
+				list( $text, $this_offset ) = $m;
+				if( $this_offset < $last_offset )
+				{
+					continue;
+				}
+
+				$last_offset = strpos( $content, $search[$text], $this_offset + strlen( $text ) );
+				if( $last_offset === false )
+				{
+					$Messages->add( T_( 'XML document not well-formed, are there unclosed comments or processing instructions?' ), 'error' );
+					$error = true;
+					break;
+				}
+
+				$last_offset += strlen( $search[$text] );
+			}
+		}
+
+		if( ! $error )
+		{	// No error yet?
+			// TODO: Remove useless assignment.
+			$check = $content;
+
+			// CHECK Styling restictions:
+			if( ! $allow_css_tweaks
+				&& preg_match( '#\s((style|class|id)\s*=)#i', $check, $matches) )
+			{
+				$Messages->add( T_('Illegal CSS markup found: ').htmlspecialchars($matches[1]), 'error' );
+				$error = true;
+			}
+
+			// CHECK JAVASCRIPT:
+			if( ! $allow_javascript
+				&& ( preg_match( '¤( < \s* //? \s* (script|noscript) )¤xi', $check, $matches )
+					|| preg_match( '#\s((on[a-z]+)\s*=)#i', $check, $matches )
+					// action=, background=, cite=, classid=, codebase=, data=, href=, longdesc=, profile=, src=, usemap=
+					|| preg_match( '#=["\'\s]*((javascript|vbscript|about):)#i', $check, $matches ) ) )
+			{
+				$Messages->add( T_('Illegal javascript markup found: ').htmlspecialchars($matches[1]), 'error' );
+				$error = true;
+			}
+
+			// CHECK IFRAMES:
+			if( ! $allow_iframes
+				&& preg_match( '¤( < \s* //? \s* (frame|iframe) )¤xi', $check, $matches) )
+			{
+				$Messages->add( T_('Illegal frame markup found: ').htmlspecialchars($matches[1]), 'error' );
+				$error = true;
+			}
+
+			// CHECK OBJECTS:
+			if( ! $allow_objects
+				&& preg_match( '¤( < \s* //? \s* (applet|object|param|embed) )¤xi', $check, $matches) )
+			{
+				$Messages->add( T_('Illegal object markup found: ').htmlspecialchars($matches[1]), 'error' );
+				$error = true;
+			}
+		}
 	}
 
 	if( $error )

=== modified file 'qp_inc/xhtml_validator/_xhtml_validator.class.php'
--- qp_inc/xhtml_validator/_xhtml_validator.class.php	2011-03-16 14:56:32 +0000
+++ qp_inc/xhtml_validator/_xhtml_validator.class.php	2011-06-20 11:14:51 +0000
@@ -55,14 +55,24 @@
 	var $stack = array();
 	var $last_checked_pos;
 	var $error;
+	
+	var $allow_css_tweaks;
+	var $allow_iframes;
+	var $allow_javascript;
+	var $allow_objects;
 
 	/**
 	 * Constructor
 	 *
 	 * {@internal This gets tested in _libs.misc.simpletest.php}}
 	 *
-	 * @param string
-	 * @param string Input encoding to use ('ISO-8859-1', 'UTF-8', 'US-ASCII' or '' for auto-detect)
+	 * @param string Context: "posting", "xmlrpc_posting" or "commenting"
+	 * @param boolean Allow the "style" attribute to be used?
+	 * @param boolean Allow using the <iframe> tag?
+	 * @param boolean Allow using the <script> and <noscript> tags?
+	 * @param boolean Allow using the <object> and <embed> tags?
+	 * @param string Input encoding to use ('ISO-8859-1', 'UTF-8', 'US-ASCII' or ''/NULL for auto-detect)
+	 * @param string Message category for error messages
 	 */
 	function XHTML_Validator( $context = 'posting', $allow_css_tweaks = false, $allow_iframes = false, $allow_javascript = false, $allow_objects = false, $encoding = NULL, $msg_type = 'error' )
 	{
@@ -71,6 +81,10 @@
 		require $inc_path.'xhtml_validator/_xhtml_dtd.inc.php';
 
 		$this->context = $context;
+		$this->allow_css_tweaks = $allow_css_tweaks;
+		$this->allow_iframes    = $allow_iframes;
+		$this->allow_javascript = $allow_javascript;
+		$this->allow_objects    = $allow_objects;
 
 		switch( $context )
 		{
@@ -137,6 +151,7 @@
 		xml_set_element_handler($this->parser, 'tag_open', 'tag_close');
 		// set function to call for the actual data
 		xml_set_character_data_handler($this->parser, 'cdata');
+		xml_set_processing_instruction_handler( $this->parser, 'pi_handler' );
 
 		xml_parser_set_option($this->parser, XML_OPTION_CASE_FOLDING, false);
 	}
@@ -166,12 +181,6 @@
 			}
 		}
 
-		// Remove special tokens:
-		// "<!": Open comments or '<![CDATA[' are dangerous.
-		// "<?": Processing instructions allow an attacker to inject arbitrary HTML.
-		//       (LP bug #736035).
-		$xhtml = str_replace( array( '<!', '<?' ), '', $xhtml );
-
 		$xhtml_head = '<?xml version="1.0"';
 		if( ! empty($this->encoding) )
 		{
@@ -207,9 +216,30 @@
 			$xml_error_string .= ' near <code>'.htmlspecialchars( evo_substr( $xhtml, $this->last_checked_pos, $pos-$this->last_checked_pos+20 ) ).'</code>';
 
 			$this->html_error( T_('Parser error: ').$xml_error_string );
-		}
-
-		return $this->isOK();
+			return false;
+		}
+
+		if( ! $this->isOK() )
+		{	// Error occurred while parsing, abort early.
+			return false;
+		}
+
+		// Everything seems to be okay, but the document may not have been
+		// parsed completely (caused by unclosed comments and/or processing
+		// instructions). We detect this by checking whether the tags are balanced;
+		// this is the case only if the whole document has been parsed.
+		// Note that we enclose the user-supplied data in <body> tags so
+		// that nothing malicious can be injected after the closing <body>
+		// tag -- if that would be possible, all tags would be balanced
+		// but there could still be malicious data...
+		if( ! empty( $this->stack ) )
+		{
+			$this->html_error( T_( 'XML document not well-formed, are there unclosed comments or processing instructions?' ) );
+			return false;
+		}
+
+		// This is a valid XML document! Congratulations!
+		return true;
 	}
 
 
@@ -306,9 +336,37 @@
 		array_pop($this->stack);
 	}
 
-/**
- * dummy docblock makes error-free autodocs
- */
+	/**
+	 * Called when the parser finds a processing instruction.
+	 *
+	 * We try to check the contents of the instruction for safe XHTML.
+	 */
+	function pi_handler( $parser, $target, $data )
+	{
+		$this->last_checked_pos = xml_get_current_byte_index( $this->parser );
+
+		// If previous tag is illegal, no point in running tests
+		$previous = $this->stack[count( $this->stack ) - 1];
+		if( ! in_array( $previous, array_keys( $this->tags ) ) )
+		{
+			return;
+		}
+
+		// This is ugly but seems to be the best way. Just parse the data as
+		// it were XHTML.
+		$validator = new XHTML_Validator( $this->context, $this->allow_css_tweaks,
+							$this->allow_iframes, $this->allow_javascript,
+							$this->allow_objects, $this->encoding, $this->msg_type );
+		if( ! $validator->check( $data ) )
+		{
+			$this->html_error( sprintf( T_( 'Invalid data in processing instruction <code>&lt;?%s</code> (see above)' ),
+									htmlspecialchars( $target ) ) );
+		}
+	}
+
+	/**
+	 * dummy docblock makes error-free autodocs
+	 */
 	function html_error( $string )
 	{
 		global $Messages;

=== modified file 'qp_srvc/comment_post.php'
--- qp_srvc/comment_post.php	2010-12-31 12:12:03 +0000
+++ qp_srvc/comment_post.php	2011-06-20 11:14:51 +0000
@@ -165,7 +165,8 @@
 // TODO: AutoBR should really be a "comment renderer" (like with Items)
 // OLD stub: $comment = format_to_post( $comment, $comment_autobr, 1 ); // includes antispam
 $saved_comment = $comment;
-$comment = check_html_sanity( $comment, $perm_comment_edit ? 'posting' : 'commenting', $comment_autobr );
+$comment = check_html_sanity( $comment, $perm_comment_edit ? 'posting' : 'commenting',
+				$comment_autobr, NULL, true /* escape comments */ );
 if( $comment === false )
 {	// ERROR
 	$comment = $saved_comment;
@@ -399,4 +400,4 @@
 header_redirect(); // Will save $Messages into Session
 
 
-?>
\ No newline at end of file
+?>