Merge lp:~widelands-dev/widelands/bug-1588063 into lp:widelands

[Tino]

Fix automatic creation of the maps/My_Maps folder in Windows. Prevent the user from entering illegal filenames.

Use system dependant file seperator for the My_Maps subdirectory.
Now ensure_directory_exists() does work also on win32.

I don't like the #ifdef __win32 solution, but i was not able to FileSystem::file_separator() because i lack c++ knowledge to concatenate char/char*/char[].

Suggestions welcome.

[GunChleoc (gunchleoc)]

Ah, bl*** file separators, my bad.

I will look into concatenating this properly.

[Tino (tino79)]

Found another bug in the current state of implementation: https://bugs.launchpad.net/widelands/+bug/1608558

review: Needs Fixing

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1222. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/148893466.
Appveyor build 1064. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1588063-1064.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1222. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/148893466.
Appveyor build 1064. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1588063-1064.

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

The read operation timed out

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1222. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/148893466.
Appveyor build 1064. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1588063-1064.

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

<urlopen error timed out>

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1222. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/148893466.
Appveyor build 1064. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1588063-1064.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1229. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/149710458.
Appveyor build 1071. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1588063-1071.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1229. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/149710458.
Appveyor build 1071. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1588063-1071.

[bunnybot (widelandsofficial)]

Bunnybot encountered an error while working on this merge proposal:

The read operation timed out

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1231. State: errored. Details: https://travis-ci.org/widelands/widelands/builds/149798427.
Appveyor build 1073. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1588063-1073.

[Klaus Halfmann (klaus-halfmann)]

Testing on OSX:

* removed .widelands folder
* :bug-1588063$ ./widelands --editor
* Created some random map and saved it
* found ./widelands/maps/My_Maps, OK
* Copied some selfmade map into ./widelands/maps
ls -R maps
Crossriver.wmf My_Maps

maps/My_Maps:
Test1608558.wmf

* I can open Crossriver.wmf in the Editor, is merged into the list of Maps.

I now will restore my old .widelands folder an try some evil characters

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 1236. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/149854580.
Appveyor build 1078. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1588063-1078.

[Klaus Halfmann (klaus-halfmann)]

OK, Tested some "evil" charaters. On OSX '`´&$()[]{} are allowed while |/":* are not (incomplete).

Maybe we should filter these as well, to avoid Filenames with bad effects on the Commandline?
OTOH the average user will not not use such characters, will he/she?

Code LGTM, but I can test OSX only, so we need some Windows test, too.

review: Approve (compile, review, test)

[GunChleoc (gunchleoc)]

I have decided to only filter out the characters that can lead to potential crashes, not the "inconvenient" ones that look confusing on the command line (like e.g. $).

I have tested on Windows and all should be fine now - both bugs fixed.

review: Needs Resubmitting

[Tino (tino79)]

Nice, thanks Gun.

I can compile fine and do not find those bugs active any longer.

@bunnybot merge

review: Approve

[GunChleoc (gunchleoc)]

*happy* Thanks for testing, everyone!

=== modified file 'src/editor/ui_menus/main_menu_save_map.cc'
--- src/editor/ui_menus/main_menu_save_map.cc	2016-08-04 15:49:05 +0000
+++ src/editor/ui_menus/main_menu_save_map.cc	2016-08-04 19:19:13 +0000
@@ -155,8 +155,10 @@
 	MainMenuSaveMapMakeDirectory md(this, _("unnamed"));
 	if (md.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) {
 		g_fs->ensure_directory_exists(curdir_);
-		//  create directory
+		//  Create directory.
 		std::string fullname = curdir_ + g_fs->file_separator() + md.get_dirname();
+		// Trim it for preceding/trailing whitespaces in user input
+		boost::trim(fullname);
 		g_fs->make_directory(fullname);
 		fill_table();
 	}
@@ -215,7 +217,8 @@
  * The editbox was changed. Enable ok button
  */
 void MainMenuSaveMap::edit_box_changed() {
-	ok_.set_enabled(!editbox_->text().empty());
+	// Prevent the user from creating nonsense file names, like e.g. ".." or "...".
+	ok_.set_enabled(LayeredFileSystem::is_legal_filename(editbox_->text()));
 }
 
 void MainMenuSaveMap::set_current_directory(const std::string& filename) {
@@ -237,11 +240,14 @@
 	//  Make sure that the current directory exists and is writeable.
 	g_fs->ensure_directory_exists(curdir_);
 
+	// Trim it for preceding/trailing whitespaces in user input
+	boost::trim(filename);
+
 	//  OK, first check if the extension matches (ignoring case).
 	if (!boost::iends_with(filename, WLMF_SUFFIX))
 		filename += WLMF_SUFFIX;
 
-	//  append directory name
+	//  Append directory name.
 	const std::string complete_filename = curdir_ + g_fs->file_separator() + filename;
 
 	//  Check if file exists. If so, show a warning.

=== modified file 'src/editor/ui_menus/main_menu_save_map_make_directory.cc'
--- src/editor/ui_menus/main_menu_save_map_make_directory.cc	2016-08-04 15:49:05 +0000
+++ src/editor/ui_menus/main_menu_save_map_make_directory.cc	2016-08-04 19:19:13 +0000
@@ -22,6 +22,7 @@
 #include "base/i18n.h"
 #include "graphic/font_handler1.h"
 #include "graphic/graphic.h"
+#include "io/filesystem/layered_filesystem.h"
 
 MainMenuSaveMapMakeDirectory::MainMenuSaveMapMakeDirectory(UI::Panel* const parent,
                                                            char const* dirname)
@@ -91,6 +92,7 @@
  */
 void MainMenuSaveMapMakeDirectory::edit_changed() {
 	const std::string& text = edit_.text();
-	ok_button_.set_enabled(!text.empty());
+	// Prevent the user from creating nonsense directory names, like e.g. ".." or "...".
+	ok_button_.set_enabled(LayeredFileSystem::is_legal_filename(text));
 	dirname_ = text;
 }

=== modified file 'src/io/filesystem/disk_filesystem.cc'
--- src/io/filesystem/disk_filesystem.cc	2016-08-04 15:49:05 +0000
+++ src/io/filesystem/disk_filesystem.cc	2016-08-04 19:19:13 +0000
@@ -297,17 +297,25 @@
  * if the dir can't be created or if a file with this name exists
  */
 void RealFSImpl::ensure_directory_exists(const std::string& dirname) {
+	std::string clean_dirname = dirname;
+#ifdef _WIN32
+	// Make sure we always use "/" for splitting the directory, because
+	// directory names might be hardcoded in C++ or come from the file system.
+	// Calling canonicalize_name will take care of this working for all file systems.
+	boost::replace_all(clean_dirname, "\\", "/");
+#endif
+
 	try {
 		std::string::size_type it = 0;
-		while (it < dirname.size()) {
-			it = dirname.find(file_separator(), it);
+		while (it < clean_dirname.size()) {
+			it = clean_dirname.find('/', it);
 
-			FileSystemPath fspath(canonicalize_name(dirname.substr(0, it)));
+			FileSystemPath fspath(canonicalize_name(clean_dirname.substr(0, it)));
 			if (fspath.exists_ && !fspath.is_directory_)
 				throw wexception("'%s' in directory '%s' exists and is not a directory",
-				                 dirname.substr(0, it).c_str(), directory_.c_str());
+				                 clean_dirname.substr(0, it).c_str(), directory_.c_str());
 			if (!fspath.exists_)
-				make_directory(dirname.substr(0, it));
+				make_directory(clean_dirname.substr(0, it));
 
 			if (it == std::string::npos)
 				break;
@@ -315,7 +323,7 @@
 		}
 	} catch (const std::exception& e) {
 		throw wexception("RealFSImpl::ensure_directory_exists(%s) in directory '%s': %s",
-		                 dirname.c_str(), directory_.c_str(), e.what());
+		                 clean_dirname.c_str(), directory_.c_str(), e.what());
 	}
 }
 

=== modified file 'src/io/filesystem/layered_filesystem.cc'
--- src/io/filesystem/layered_filesystem.cc	2016-08-04 15:49:05 +0000
+++ src/io/filesystem/layered_filesystem.cc	2016-08-04 19:19:13 +0000
@@ -22,6 +22,9 @@
 #include <cstdio>
 #include <memory>
 
+#include <boost/algorithm/string/predicate.hpp>
+#include <boost/regex.hpp>
+
 #include "base/log.h"
 #include "base/wexception.h"
 #include "io/fileread.h"
@@ -38,6 +41,18 @@
 LayeredFileSystem::~LayeredFileSystem() {
 }
 
+bool LayeredFileSystem::is_legal_filename(const std::string& filename) {
+	// No potential file separators or other potentially illegal characters
+	// https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
+	// http://www.linfo.org/file_name.html
+	// https://support.apple.com/en-us/HT202808
+	// We can't just regex for word & digit characters here because of non-Latin scripts.
+	boost::regex re(".*[<>:\"|?*/\\\\].*");
+	return !filename.empty() && !boost::starts_with(filename, ".") &&
+	       !boost::starts_with(filename, " ") && !boost::starts_with(filename, "-") &&
+	       !boost::regex_match(filename, re);
+}
+
 /**
  * Just assume that at least one of our child FSs is writable
  */

=== modified file 'src/io/filesystem/layered_filesystem.h'
--- src/io/filesystem/layered_filesystem.h	2016-08-04 15:49:05 +0000
+++ src/io/filesystem/layered_filesystem.h	2016-08-04 19:19:13 +0000
@@ -60,6 +60,8 @@
 
 	std::set<std::string> list_directory(const std::string& path) override;
 
+	/// Returns true if the filename is legal in all operating systems
+	static bool is_legal_filename(const std::string& filename);
 	bool is_writable() const override;
 	bool file_exists(const std::string& path) override;
 	bool is_directory(const std::string& path) override;

=== modified file 'src/logic/save_handler.cc'
--- src/logic/save_handler.cc	2016-08-04 15:49:05 +0000
+++ src/logic/save_handler.cc	2016-08-04 19:19:13 +0000
@@ -174,8 +174,10 @@
  */
 std::string SaveHandler::create_file_name(const std::string& dir,
                                           const std::string& filename) const {
-	// Append directory name
+	// Append directory name.
 	std::string complete_filename = dir + g_fs->file_separator() + filename;
+	// Trim it for preceding/trailing whitespaces in user input
+	boost::trim(complete_filename);
 
 	// Now check if the extension matches (ignoring case)
 	if (!boost::iends_with(filename, WLGF_SUFFIX)) {

=== modified file 'src/wui/game_main_menu_save_game.cc'
--- src/wui/game_main_menu_save_game.cc	2016-08-04 15:49:05 +0000
+++ src/wui/game_main_menu_save_game.cc	2016-08-04 19:19:13 +0000
@@ -207,7 +207,8 @@
  * The editbox was changed. Enable ok button
  */
 void GameMainMenuSaveGame::edit_box_changed() {
-	button_ok_->set_enabled(editbox_.text().size());
+	// Prevent the user from creating nonsense directory names, like e.g. ".." or "...".
+	button_ok_->set_enabled(LayeredFileSystem::is_legal_filename(editbox_.text()));
 }
 
 static void dosave(InteractiveGameBase& igbase, const std::string& complete_filename) {