widelands-dev team mailing list archive

[Miroslav Remák <miroslavr256@xxxxxxxxx>]

Miroslav Remák has proposed merging lp:~widelands-dev/widelands/escaping-fixes into lp:widelands.

Commit message:
- Introduce '&' entity which translates to '&'
- Properly escape user messages
- Fix entity replacement in the old RT renderer
- Use a function for repeated code in chat_msg_layout.cc

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/escaping-fixes/+merge/290410

This allows the user to type e.g. '&lt;' literally, and fixes input fields getting messed up when such entity is typed in. It also prevents crashes (uncaught exceptions) with certain nicknames such as '<'.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/escaping-fixes into lp:widelands.

=== modified file 'src/graphic/richtext.cc'
--- src/graphic/richtext.cc	2016-03-02 17:11:16 +0000
+++ src/graphic/richtext.cc	2016-03-30 08:43:17 +0000
@@ -86,7 +86,6 @@
 			for (std::vector<std::string>::iterator source_it = words.begin();
 				  source_it != words.end(); ++source_it) {
 				std::string& word = *source_it;
-				replace_entities(&word);
 				if (source_it != words.end()) {
 					if (i18n::has_rtl_character(word.c_str()) || i18n::has_rtl_character(previous_word.c_str())) {
 						it = result_words.insert(result_words.begin(), word);
@@ -101,7 +100,6 @@
 			}
 		} else {
 			for (std::string& word: words) {
-				replace_entities(&word);
 				result_words.push_back(word);
 			}
 		}

=== modified file 'src/graphic/text_layout.cc'
--- src/graphic/text_layout.cc	2016-03-14 19:49:52 +0000
+++ src/graphic/text_layout.cc	2016-03-30 08:43:17 +0000
@@ -36,6 +36,7 @@
 	boost::replace_all(*text, "&gt;", ">");
 	boost::replace_all(*text, "&lt;", "<");
 	boost::replace_all(*text, "&nbsp;", " ");
+	boost::replace_all(*text, "&amp;", "&"); // Must be performed last
 }
 
 uint32_t text_width(const std::string& text, int ptsize) {
@@ -49,6 +50,7 @@
 
 std::string richtext_escape(const std::string& given_text) {
 	std::string text = given_text;
+	boost::replace_all(text, "&", "&amp;"); // Must be performed first
 	boost::replace_all(text, ">", "&gt;");
 	boost::replace_all(text, "<", "&lt;");
 	return text;

=== modified file 'src/graphic/text_parser.cc'
--- src/graphic/text_parser.cc	2016-03-14 19:49:52 +0000
+++ src/graphic/text_parser.cc	2016-03-30 08:43:17 +0000
@@ -28,6 +28,7 @@
 #include "graphic/font_handler1.h"
 #include "graphic/text/bidi.h"
 #include "graphic/text/font_set.h"
+#include "graphic/text_layout.h"
 #include "helper.h"
 
 namespace UI {
@@ -135,11 +136,17 @@
 			}
 
 			if (next_break == std::string::npos) {
-				if (line.size())
-					words.push_back(line);
+				if (line.size()) {
+					std::string word = line;
+					replace_entities(&word);
+					words.push_back(word);
+				}
 				break;
-			} else if (next_break)
-				words.push_back(line.substr(0, next_break));
+			} else if (next_break) {
+				std::string word = line.substr(0, next_break);
+				replace_entities(&word);
+				words.push_back(word);
+			}
 			line_breaks.push_back(words.size());
 			line.erase(0, next_break + 4);
 		}

=== modified file 'src/wui/chat_msg_layout.cc'
--- src/wui/chat_msg_layout.cc	2016-03-28 10:37:09 +0000
+++ src/wui/chat_msg_layout.cc	2016-03-30 08:43:17 +0000
@@ -23,6 +23,7 @@
 
 #include "chat/chat.h"
 #include "graphic/color.h"
+#include "graphic/text_layout.h"
 #include "logic/constants.h"
 #include "logic/player.h"
 
@@ -47,44 +48,7 @@
 	const std::string& font_face = "serif";
 	std::string message = "<p font-color=#33ff33 font-size=9>";
 
-	// Escape richtext characters
-	// The goal of this code is two-fold:
-	//  1. Assuming an honest game host, we want to prevent the ability of
-	//     clients to use richtext.
-	//  2. Assuming a malicious host or meta server, we want to reduce the
-	//     likelihood that a bug in the richtext renderer can be exploited,
-	//     by restricting the set of allowed richtext commands.
-	//     Most notably, images are not allowed in richtext at all.
-	//
-	// Note that we do want host and meta server to send some richtext code,
-	// as the ability to send formatted commands is nice for the usability
-	// of meta server so we're treading a bit of a fine line here.
-	std::string sanitized;
-	for (std::string::size_type pos = 0; pos < chat_message.msg.size(); ++pos) {
-		if (chat_message.msg[pos] == '<') {
-			if (chat_message.playern < 0) {
-				static const std::string good1 = "</p><p";
-				static const std::string good2 = "<br>";
-				if (!chat_message.msg.compare(pos, good1.size(), good1)) {
-					std::string::size_type nextclose = chat_message.msg.find('>', pos + good1.size());
-					if (nextclose != std::string::npos &&
-					    (nextclose == pos + good1.size() || chat_message.msg[pos + good1.size()] == ' ')) {
-						sanitized += good1;
-						pos += good1.size() - 1;
-						continue;
-					}
-				} else if (!chat_message.msg.compare(pos, good2.size(), good2)) {
-					sanitized += good2;
-					pos += good2.size() - 1;
-					continue;
-				}
-			}
-
-			sanitized += "&lt;";
-		} else {
-			sanitized += chat_message.msg[pos];
-		}
-	}
+	std::string sanitized = sanitize_message(chat_message);
 
 	// time calculation
 	char ts[13];
@@ -94,32 +58,35 @@
 	message = (boost::format("%s<p font-size=14 font-face=%s font-color=#%s")
 				  % message % font_face % color(chat_message.playern)).str();
 
+	std::string sender_escaped = richtext_escape(chat_message.sender);
+	std::string recipient_escaped = richtext_escape(chat_message.recipient);
+
 	if (chat_message.recipient.size() && chat_message.sender.size()) {
 		// Personal message handling
 		if (sanitized.compare(0, 3, "/me")) {
 
 			message = (boost::format("%s font-decoration=underline>%s @ %s:</p><p font-size=14 font-face=%s> %s")
 						  % message
-						  % chat_message.sender
-						  % chat_message.recipient
+						  % sender_escaped
+						  % recipient_escaped
 						  % font_face
 						  % sanitized).str();
 		} else {
 
 			message =
-				(boost::format("%s>@%s >> </p>"
+				(boost::format("%s>@%s &gt;&gt; </p>"
 									"<p font-size=14 font-face=%s font-color=#%s font-style=italic> %s%s")
 				 % message
-				 % chat_message.recipient
+				 % recipient_escaped
 				 % font_face
 				 % color(chat_message.playern)
-				 % chat_message.sender
+				 % sender_escaped
 				 % sanitized.substr(3)).str();
 		}
 	} else {
 		// Normal messages handling
 		if (!sanitized.compare(0, 3, "/me")) {
-			message = (boost::format("%s font-style=italic>-> %s%s")
+			message = (boost::format("%s font-style=italic>-&gt; %s%s")
 						  % message
 						  % (chat_message.sender.size() ? chat_message.sender.c_str() : "***")
 						  % sanitized.substr(3)).str();
@@ -127,7 +94,7 @@
 		} else if (chat_message.sender.size()) {
 			message = (boost::format("%s font-decoration=underline>%s:</p><p font-size=14 font-face=%s> %s")
 						  % message
-						  % chat_message.sender
+						  % sender_escaped
 						  % font_face
 						  % sanitized).str();
 		} else {
@@ -145,46 +112,7 @@
 	const std::string& font_face = "serif";
 	std::string message = "<p><font color=33ff33 size=9>";
 
-	// Escape richtext characters
-	// The goal of this code is two-fold:
-	//  1. Assuming an honest game host, we want to prevent the ability of
-	//     clients to use richtext.
-	//  2. Assuming a malicious host or meta server, we want to reduce the
-	//     likelihood that a bug in the richtext renderer can be exploited,
-	//     by restricting the set of allowed richtext commands.
-	//     Most notably, images are not allowed in richtext at all.
-	//
-	// Note that we do want host and meta server to send some richtext code,
-	// as the ability to send formatted commands is nice for the usability
-	// of meta server so we're treading a bit of a fine line here.
-	std::string sanitized;
-	for (std::string::size_type pos = 0; pos < chat_message.msg.size(); ++pos) {
-		if (chat_message.msg[pos] == '<') {
-			if (chat_message.playern < 0) {
-				static const std::string good1 = "</p><p";
-				static const std::string good2 = "<br>";
-				if (!chat_message.msg.compare(pos, good1.size(), good1)) {
-					std::string::size_type nextclose = chat_message.msg.find('>', pos + good1.size());
-					if
-						(nextclose != std::string::npos &&
-						(nextclose == pos + good1.size() || chat_message.msg[pos + good1.size()] == ' '))
-					{
-						sanitized += good1;
-						pos += good1.size() - 1;
-						continue;
-					}
-				} else if (!chat_message.msg.compare(pos, good2.size(), good2)) {
-					sanitized += good2;
-					pos += good2.size() - 1;
-					continue;
-				}
-			}
-
-			sanitized += "&lt;";
-		} else {
-			sanitized += chat_message.msg[pos];
-		}
-	}
+	std::string sanitized = sanitize_message(chat_message);
 
 	// time calculation
 	char ts[13];
@@ -194,23 +122,26 @@
 	message = (boost::format("%s</font><font size=14 face=%s color=%s")
 				  % message % font_face % color(chat_message.playern)).str();
 
+	std::string sender_escaped = richtext_escape(chat_message.sender);
+	std::string recipient_escaped = richtext_escape(chat_message.recipient);
+
 	if (chat_message.recipient.size() && chat_message.sender.size()) {
 		// Personal message handling
 		if (sanitized.compare(0, 3, "/me")) {
 			message = (boost::format("%s bold=1>%s @ %s:</font><font size=14 face=%s shadow=1 color=eeeeee> %s")
 					% message
-					% chat_message.sender
-					% chat_message.recipient
+					% sender_escaped
+					% recipient_escaped
 					% font_face
 					% sanitized).str();
 
 		} else {
 			message = (boost::format("%s>@%s &gt; </font><font size=14 face=%s color=%s italic=1 shadow=1> %s%s")
 					% message
-					% chat_message.recipient
+					% recipient_escaped
 					% font_face
 					% color(chat_message.playern)
-					% chat_message.sender
+					% sender_escaped
 					% sanitized.substr(3)).str();
 		}
 	} else {
@@ -218,14 +149,14 @@
 		if (!sanitized.compare(0, 3, "/me")) {
 			message += " italic=1>-&gt; ";
 			if (chat_message.sender.size())
-				message += chat_message.sender;
+				message += sender_escaped;
 			else
 				message += "***";
 			message += sanitized.substr(3);
 		} else if (chat_message.sender.size()) {
 			message = (boost::format("%s bold=1>%s:</font><font size=14 face=%s shadow=1 color=eeeeee> %s")
 						 % message
-						 % chat_message.sender
+						 % sender_escaped
 						 % font_face
 						 % sanitized).str();
 		} else {
@@ -237,3 +168,52 @@
 	// return the formated message
 	return message + "</font><br></p>";
 }
+
+std::string sanitize_message(const ChatMessage & chat_message)
+{
+	// Escape richtext characters
+	// The goal of this code is two-fold:
+	//  1. Assuming an honest game host, we want to prevent the ability of
+	//     clients to use richtext.
+	//  2. Assuming a malicious host or meta server, we want to reduce the
+	//     likelihood that a bug in the richtext renderer can be exploited,
+	//     by restricting the set of allowed richtext commands.
+	//     Most notably, images are not allowed in richtext at all.
+	//
+	// Note that we do want host and meta server to send some richtext code,
+	// as the ability to send formatted commands is nice for the usability
+	// of meta server so we're treading a bit of a fine line here.
+
+	if (chat_message.playern >= 0) {
+		return richtext_escape(chat_message.msg);
+	}
+
+	std::string sanitized;
+	for (std::string::size_type pos = 0; pos < chat_message.msg.size(); ++pos) {
+		if (chat_message.msg[pos] == '<') {
+			static const std::string good1 = "</p><p";
+			static const std::string good2 = "<br>";
+			if (!chat_message.msg.compare(pos, good1.size(), good1)) {
+				// TODO(MiroslavR): The logic here seems flawed.
+				std::string::size_type nextclose = chat_message.msg.find('>', pos + good1.size());
+				if
+					(nextclose != std::string::npos &&
+					(nextclose == pos + good1.size() || chat_message.msg[pos + good1.size()] == ' '))
+				{
+					sanitized += good1;
+					pos += good1.size() - 1;
+					continue;
+				}
+			} else if (!chat_message.msg.compare(pos, good2.size(), good2)) {
+				sanitized += good2;
+				pos += good2.size() - 1;
+				continue;
+			}
+
+			sanitized += "&lt;";
+		} else {
+			sanitized += chat_message.msg[pos];
+		}
+	}
+	return sanitized;
+}

=== modified file 'src/wui/chat_msg_layout.h'
--- src/wui/chat_msg_layout.h	2015-03-01 09:21:20 +0000
+++ src/wui/chat_msg_layout.h	2016-03-30 08:43:17 +0000
@@ -30,4 +30,6 @@
 // Formats 'chat_message' as richtext.
 std::string format_as_richtext(const ChatMessage& chat_message);
 
+std::string sanitize_message(const ChatMessage & chat_message);
+
 #endif  // end of include guard: WL_WUI_CHAT_MSG_LAYOUT_H