campaignd: Send uploading clients a list of illegal names when any are found

(See issue #2043.)

It wouldn't be proper to stuff the full list into the conventional error
response, so in order to provide clients with the means to report it in
a better fashion in the future (a la WML load errors report dialog, with
a fancy scrolling box and an option to copy the report to clipboard and
stuff), we're attaching it as an extra_data attribute in the [error]
response. Naturally, only clients that are aware of its existence can
handle it, so this whole patch is completely unsuitable for 1.12.

Still, some level of backwards compatibility is retained -- the [error]
continues to be perfectly functional for incompatible clients (e.g.
1.13.10), and the details are simply missing for them.

The list would currently look like this:

  Test_Addon_420/~illegalname
  Test_Addon_420/testdir/illegal name 2
  Test_Addon_420/testdir/illegal~dir~name/

The required client-side changes for handling the extra_data attribute
and displaying its contents are included in this patch, but the GUI
changes are very rudimentary and should be considered more of a
proof-of-concept than a final solution, which I'll leave to someone who
actually groks GUI2 as it is right now and is an active developer in
the first place, which I am not.

Also note that the only reason the second parameter for
check_names_legal() is optional is that the function is also used
client-side by addons_client::install_addon(), which really doesn't need
to waste time building a full report of invalid names -- add-ons
containing those aren't supposed to make it to the server in the first
place, and the client-side check only exists as an extra safeguard.

src/addon/client.cpp

@@ -44,6 +44,7 @@ addons_client::addons_client(CVideo& v, const std::string& address)
 	, conn_(nullptr)
 	, stat_(nullptr)
 	, last_error_()
+	, last_error_data_()
 {
 	const std::vector<std::string>& address_components =
 		utils::split(addr_, ':');
@@ -439,10 +440,12 @@ bool addons_client::update_last_error(config& response_cfg)
 {
 	if(config const &error = response_cfg.child("error")) {
 		this->last_error_ = error["message"].str();
+		this->last_error_data_ = error["extra_data"].str();
 		ERR_ADDONS << "server error: " << error << '\n';
 		return true;
 	} else {
 		this->last_error_.clear();
+		this->last_error_data_.clear();
 		return false;
 	}
 }

src/addon/client.hpp

@@ -60,6 +60,9 @@ public:
 	/** Returns the last error message sent by the server, or an empty string. */
 	const std::string& get_last_server_error() const { return last_error_; }
 
+	/** Returns the last error message extra data sent by the server, or an empty string. */
+	const std::string& get_last_server_error_data() const { return last_error_data_; }
+
 	/**
 	 * Request the add-ons list from the server.
 	 *
@@ -123,6 +126,7 @@ private:
 	std::unique_ptr<network_asio::connection> conn_;
 	std::unique_ptr<gui2::dialogs::network_transmission> stat_;
 	std::string last_error_;
+	std::string last_error_data_;
 
 	/**
 	* Downloads the specified add-on from the server.

src/addon/validation.cpp

@@ -68,16 +68,56 @@ bool addon_filename_legal(const std::string& name)
 	}
 }
 
-bool check_names_legal(const config& dir)
+namespace {
+
+bool check_names_legal_internal(const config& dir, std::string current_prefix, std::vector<std::string>* badlist)
 {
-	for (const config &path : dir.child_range("file")) {
-		if (!addon_filename_legal(path["name"])) return false;
+	if (!current_prefix.empty()) {
+		current_prefix += '/';
 	}
-	for (const config &path : dir.child_range("dir")) {
-		if (!addon_filename_legal(path["name"])) return false;
-		if (!check_names_legal(path)) return false;
+
+	for(const config& path : dir.child_range("file")) {
+		const std::string& filename = path["name"];
+
+		if(!addon_filename_legal(filename)) {
+			if(badlist) {
+				badlist->push_back(current_prefix + filename);
+			} else {
+				return false;
+			}
+		}
 	}
-	return true;
+
+	for(const config& path : dir.child_range("dir")) {
+		const std::string& dirname = path["name"];
+		const std::string& new_prefix = current_prefix + dirname;
+
+		if(!addon_filename_legal(dirname)) {
+			if(badlist) {
+				badlist->push_back(new_prefix + "/");
+			} else {
+				return false;
+			}
+		}
+
+		// Recurse into subdir.
+		if(!check_names_legal_internal(path, new_prefix, badlist) && !badlist) {
+			return false;
+		}
+	}
+
+	return badlist ? badlist->empty() : true;
+}
+
+} // end unnamed namespace 3
+
+bool check_names_legal(const config& dir, std::vector<std::string>* badlist)
+{
+	// Usually our caller is passing us the root [dir] for an add-on, which
+	// shall contain a single subdir named after the add-on itself, so we can
+	// start with an empty display prefix and that'll reflect the addon
+	// structure correctly (e.g. "Addon_Name/~illegalfilename1").
+	return check_names_legal_internal(dir, "", badlist);
 }
 
 ADDON_TYPE get_addon_type(const std::string& str)

src/addon/validation.hpp

@@ -63,8 +63,20 @@ std::string get_addon_type_string(ADDON_TYPE type);
 bool addon_name_legal(const std::string& name);
 /** Checks whether an add-on file name is legal or not. */
 bool addon_filename_legal(const std::string& name);
-/** Probes an add-on archive for illegal names. */
-bool check_names_legal(const config& dir);
+
+/**
+ * Scans an add-on archive for illegal names.
+ *
+ * @param dir     The WML container for the root [dir] node where the search
+ *                should begin.
+ * @param badlist If provided and not null, any illegal names encountered will
+ *                be added to this list. This also makes the archive scan more
+ *                thorough instead of returning on the first illegal name
+ *                encountered.
+ *
+ * @returns True if no illegal names were found.
+ */
+bool check_names_legal(const config& dir, std::vector<std::string>* badlist = nullptr);
 
 std::string encode_binary(const std::string& str);
 std::string unencode_binary(const std::string& str);

src/campaign_server/campaign_server.cpp

@@ -437,6 +437,16 @@ void server::send_error(const std::string& msg, socket_ptr sock)
 	async_send_doc(sock, doc, std::bind(&server::handle_new_client, this, _1), null_handler);
 }
 
+void server::send_error(const std::string& msg, const std::string& extra_data, socket_ptr sock)
+{
+	ERR_CS << "[" << client_address(sock) << "]: " << msg << '\n';
+	simple_wml::document doc;
+	simple_wml::node& err_cfg = doc.root().add_child("error");
+	err_cfg.set_attr_dup("message", msg.c_str());
+	err_cfg.set_attr_dup("extra_data", extra_data.c_str());
+	async_send_doc(sock, doc, std::bind(&server::handle_new_client, this, _1), null_handler);
+}
+
 #define REGISTER_CAMPAIGND_HANDLER(req_id) \
 	handlers_[#req_id] = std::bind(&server::handle_##req_id, \
 		std::placeholders::_1, std::placeholders::_2)
@@ -640,6 +650,8 @@ void server::handle_upload(const server::request& req)
 		return;
 	}
 
+	std::vector<std::string> badnames;
+
 	if(read_only_) {
 		LOG_CS << "Upload aborted - uploads not permitted in read-only mode.\n";
 		send_error("Add-on rejected: The server is currently in read-only mode.", req.sock);
@@ -673,11 +685,13 @@ void server::handle_upload(const server::request& req)
 	} else if(upload["email"].empty()) {
 		LOG_CS << "Upload aborted - no add-on email specified.\n";
 		send_error("Add-on rejected: You did not specify your email address in the pbl file!", req.sock);
-	} else if(!check_names_legal(data)) {
-		LOG_CS << "Upload aborted - invalid file names in add-on data.\n";
-		send_error("Add-on rejected: The add-on contains an illegal file or directory name."
-				   " File or directory names may not contain whitespace or any of the following characters: '/ \\ : ~'",
-				   req.sock);
+	} else if(!check_names_legal(data, &badnames)) {
+		const std::string& filelist = utils::join(badnames, "\n");
+		LOG_CS << "Upload aborted - invalid file names in add-on data (" << badnames.size() << " entries).\n";
+		send_error(
+			"Add-on rejected: The add-on contains files or directories with illegal names. "
+			"File or directory names may not contain whitespace or any of the following characters: '/ \\ : ~'",
+			filelist, req.sock);
 	} else if(campaign && !authenticate(*campaign, upload["passphrase"])) {
 		LOG_CS << "Upload aborted - incorrect passphrase.\n";
 		send_error("Add-on rejected: The add-on already exists, and your passphrase was incorrect.", req.sock);

src/campaign_server/campaign_server.hpp

@@ -187,6 +187,17 @@ private:
 	 * is recorded to the server log.
 	 */
 	void send_error(const std::string& msg, socket_ptr sock);
+
+	/**
+	 * Send a client an error message.
+	 *
+	 * The WML sent consists of a document containing a single @p [error] child
+	 * with a @a message attribute holding the value of @a msg, and an
+	 * @a extra_data attribute holding the value of @a extra_data. In addition
+	 * to sending the error to the client, a line with the client IP and
+	 * message is recorded to the server log.
+	 */
+	void send_error(const std::string& msg, const std::string& extra_data, socket_ptr sock);
 };
 
 } // end namespace campaignd

src/gui/dialogs/addon/manager.cpp

@@ -684,8 +684,20 @@ void addon_manager::publish_addon(const addon_info& addon, window& window)
 			_("The server responded with an error:") + "\n" + client_.get_last_server_error());
 	} else if(gui2::show_message(window.video(), _("Terms"), server_msg, gui2::dialogs::message::ok_cancel_buttons, true) == gui2::window::OK) {
 		if(!client_.upload_addon(addon_id, server_msg, cfg)) {
-			gui2::show_error_message(window.video(),
-				_("The server responded with an error:") + "\n" + client_.get_last_server_error());
+			const std::string& msg = _("The server responded with an error:") +
+			                         "\n\n" + client_.get_last_server_error();
+			const std::string& extra_data = client_.get_last_server_error_data();
+			if (!extra_data.empty()) {
+				// TODO: Allow user to copy the extra data portion to clipboard
+				//       or something, maybe display it in a dialog similar to
+				//       the WML load errors report in a monospace font and
+				//       stuff (having a scroll container is especially
+				//       important since a long list can cause the dialog to
+				//       overflow).
+				gui2::show_error_message(window.video(), msg + "\n\n" + extra_data);
+			} else {
+				gui2::show_error_message(window.video(), msg);
+			}
 		} else {
 			gui2::show_transient_message(window.video(), _("Response"), server_msg);
 			fetch_addons_list(window);