Merge lp:~widelands-dev/widelands/bug-1588063 into lp:widelands
- bug-1588063
- Merge into trunk
Status: | Merged | ||||||||
---|---|---|---|---|---|---|---|---|---|
Merged at revision: | 8051 | ||||||||
Proposed branch: | lp:~widelands-dev/widelands/bug-1588063 | ||||||||
Merge into: | lp:widelands | ||||||||
Diff against target: |
182 lines (+48/-12) 7 files modified
src/editor/ui_menus/main_menu_save_map.cc (+9/-3) src/editor/ui_menus/main_menu_save_map_make_directory.cc (+3/-1) src/io/filesystem/disk_filesystem.cc (+14/-6) src/io/filesystem/layered_filesystem.cc (+15/-0) src/io/filesystem/layered_filesystem.h (+2/-0) src/logic/save_handler.cc (+3/-1) src/wui/game_main_menu_save_game.cc (+2/-1) |
||||||||
To merge this branch: | bzr merge lp:~widelands-dev/widelands/bug-1588063 | ||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tino | Approve | ||
GunChleoc | Needs Resubmitting | ||
Klaus Halfmann | compile, review, test | Approve | |
Review via email:
|
Commit message
Fix automatic creation of the maps/My_Maps folder in Windows. Prevent the user from entering illegal filenames.
Description of the change
Use system dependant file seperator for the My_Maps subdirectory.
Now ensure_
I don't like the #ifdef __win32 solution, but i was not able to FileSystem:
Suggestions welcome.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
GunChleoc (gunchleoc) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tino (tino79) wrote : | # |
Found another bug in the current state of implementation: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 1222. State: failed. Details: https:/
Appveyor build 1064. State: success. Details: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 1222. State: passed. Details: https:/
Appveyor build 1064. State: success. Details: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Bunnybot encountered an error while working on this merge proposal:
The read operation timed out
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 1222. State: passed. Details: https:/
Appveyor build 1064. State: success. Details: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Bunnybot encountered an error while working on this merge proposal:
<urlopen error timed out>
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 1222. State: passed. Details: https:/
Appveyor build 1064. State: success. Details: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 1229. State: passed. Details: https:/
Appveyor build 1071. State: failed. Details: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 1229. State: errored. Details: https:/
Appveyor build 1071. State: failed. Details: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Bunnybot encountered an error while working on this merge proposal:
The read operation timed out
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 1231. State: errored. Details: https:/
Appveyor build 1073. State: success. Details: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Klaus Halfmann (klaus-halfmann) wrote : | # |
Testing on OSX:
* removed .widelands folder
* :bug-1588063$ ./widelands --editor
* Created some random map and saved it
* found ./widelands/
* 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
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
bunnybot (widelandsofficial) wrote : | # |
Continuous integration builds have changed state:
Travis build 1236. State: failed. Details: https:/
Appveyor build 1078. State: success. Details: https:/
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Klaus Halfmann (klaus-halfmann) wrote : | # |
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
GunChleoc (gunchleoc) wrote : | # |
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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Tino (tino79) wrote : | # |
Nice, thanks Gun.
I can compile fine and do not find those bugs active any longer.
@bunnybot merge
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
GunChleoc (gunchleoc) wrote : | # |
*happy* Thanks for testing, everyone!
Preview Diff
1 | === modified file 'src/editor/ui_menus/main_menu_save_map.cc' | |||
2 | --- src/editor/ui_menus/main_menu_save_map.cc 2016-08-04 15:49:05 +0000 | |||
3 | +++ src/editor/ui_menus/main_menu_save_map.cc 2016-08-04 19:19:13 +0000 | |||
4 | @@ -155,8 +155,10 @@ | |||
5 | 155 | MainMenuSaveMapMakeDirectory md(this, _("unnamed")); | 155 | MainMenuSaveMapMakeDirectory md(this, _("unnamed")); |
6 | 156 | if (md.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) { | 156 | if (md.run<UI::Panel::Returncodes>() == UI::Panel::Returncodes::kOk) { |
7 | 157 | g_fs->ensure_directory_exists(curdir_); | 157 | g_fs->ensure_directory_exists(curdir_); |
9 | 158 | // create directory | 158 | // Create directory. |
10 | 159 | std::string fullname = curdir_ + g_fs->file_separator() + md.get_dirname(); | 159 | std::string fullname = curdir_ + g_fs->file_separator() + md.get_dirname(); |
11 | 160 | // Trim it for preceding/trailing whitespaces in user input | ||
12 | 161 | boost::trim(fullname); | ||
13 | 160 | g_fs->make_directory(fullname); | 162 | g_fs->make_directory(fullname); |
14 | 161 | fill_table(); | 163 | fill_table(); |
15 | 162 | } | 164 | } |
16 | @@ -215,7 +217,8 @@ | |||
17 | 215 | * The editbox was changed. Enable ok button | 217 | * The editbox was changed. Enable ok button |
18 | 216 | */ | 218 | */ |
19 | 217 | void MainMenuSaveMap::edit_box_changed() { | 219 | void MainMenuSaveMap::edit_box_changed() { |
21 | 218 | ok_.set_enabled(!editbox_->text().empty()); | 220 | // Prevent the user from creating nonsense file names, like e.g. ".." or "...". |
22 | 221 | ok_.set_enabled(LayeredFileSystem::is_legal_filename(editbox_->text())); | ||
23 | 219 | } | 222 | } |
24 | 220 | 223 | ||
25 | 221 | void MainMenuSaveMap::set_current_directory(const std::string& filename) { | 224 | void MainMenuSaveMap::set_current_directory(const std::string& filename) { |
26 | @@ -237,11 +240,14 @@ | |||
27 | 237 | // Make sure that the current directory exists and is writeable. | 240 | // Make sure that the current directory exists and is writeable. |
28 | 238 | g_fs->ensure_directory_exists(curdir_); | 241 | g_fs->ensure_directory_exists(curdir_); |
29 | 239 | 242 | ||
30 | 243 | // Trim it for preceding/trailing whitespaces in user input | ||
31 | 244 | boost::trim(filename); | ||
32 | 245 | |||
33 | 240 | // OK, first check if the extension matches (ignoring case). | 246 | // OK, first check if the extension matches (ignoring case). |
34 | 241 | if (!boost::iends_with(filename, WLMF_SUFFIX)) | 247 | if (!boost::iends_with(filename, WLMF_SUFFIX)) |
35 | 242 | filename += WLMF_SUFFIX; | 248 | filename += WLMF_SUFFIX; |
36 | 243 | 249 | ||
38 | 244 | // append directory name | 250 | // Append directory name. |
39 | 245 | const std::string complete_filename = curdir_ + g_fs->file_separator() + filename; | 251 | const std::string complete_filename = curdir_ + g_fs->file_separator() + filename; |
40 | 246 | 252 | ||
41 | 247 | // Check if file exists. If so, show a warning. | 253 | // Check if file exists. If so, show a warning. |
42 | 248 | 254 | ||
43 | === modified file 'src/editor/ui_menus/main_menu_save_map_make_directory.cc' | |||
44 | --- src/editor/ui_menus/main_menu_save_map_make_directory.cc 2016-08-04 15:49:05 +0000 | |||
45 | +++ src/editor/ui_menus/main_menu_save_map_make_directory.cc 2016-08-04 19:19:13 +0000 | |||
46 | @@ -22,6 +22,7 @@ | |||
47 | 22 | #include "base/i18n.h" | 22 | #include "base/i18n.h" |
48 | 23 | #include "graphic/font_handler1.h" | 23 | #include "graphic/font_handler1.h" |
49 | 24 | #include "graphic/graphic.h" | 24 | #include "graphic/graphic.h" |
50 | 25 | #include "io/filesystem/layered_filesystem.h" | ||
51 | 25 | 26 | ||
52 | 26 | MainMenuSaveMapMakeDirectory::MainMenuSaveMapMakeDirectory(UI::Panel* const parent, | 27 | MainMenuSaveMapMakeDirectory::MainMenuSaveMapMakeDirectory(UI::Panel* const parent, |
53 | 27 | char const* dirname) | 28 | char const* dirname) |
54 | @@ -91,6 +92,7 @@ | |||
55 | 91 | */ | 92 | */ |
56 | 92 | void MainMenuSaveMapMakeDirectory::edit_changed() { | 93 | void MainMenuSaveMapMakeDirectory::edit_changed() { |
57 | 93 | const std::string& text = edit_.text(); | 94 | const std::string& text = edit_.text(); |
59 | 94 | ok_button_.set_enabled(!text.empty()); | 95 | // Prevent the user from creating nonsense directory names, like e.g. ".." or "...". |
60 | 96 | ok_button_.set_enabled(LayeredFileSystem::is_legal_filename(text)); | ||
61 | 95 | dirname_ = text; | 97 | dirname_ = text; |
62 | 96 | } | 98 | } |
63 | 97 | 99 | ||
64 | === modified file 'src/io/filesystem/disk_filesystem.cc' | |||
65 | --- src/io/filesystem/disk_filesystem.cc 2016-08-04 15:49:05 +0000 | |||
66 | +++ src/io/filesystem/disk_filesystem.cc 2016-08-04 19:19:13 +0000 | |||
67 | @@ -297,17 +297,25 @@ | |||
68 | 297 | * if the dir can't be created or if a file with this name exists | 297 | * if the dir can't be created or if a file with this name exists |
69 | 298 | */ | 298 | */ |
70 | 299 | void RealFSImpl::ensure_directory_exists(const std::string& dirname) { | 299 | void RealFSImpl::ensure_directory_exists(const std::string& dirname) { |
71 | 300 | std::string clean_dirname = dirname; | ||
72 | 301 | #ifdef _WIN32 | ||
73 | 302 | // Make sure we always use "/" for splitting the directory, because | ||
74 | 303 | // directory names might be hardcoded in C++ or come from the file system. | ||
75 | 304 | // Calling canonicalize_name will take care of this working for all file systems. | ||
76 | 305 | boost::replace_all(clean_dirname, "\\", "/"); | ||
77 | 306 | #endif | ||
78 | 307 | |||
79 | 300 | try { | 308 | try { |
80 | 301 | std::string::size_type it = 0; | 309 | std::string::size_type it = 0; |
83 | 302 | while (it < dirname.size()) { | 310 | while (it < clean_dirname.size()) { |
84 | 303 | it = dirname.find(file_separator(), it); | 311 | it = clean_dirname.find('/', it); |
85 | 304 | 312 | ||
87 | 305 | FileSystemPath fspath(canonicalize_name(dirname.substr(0, it))); | 313 | FileSystemPath fspath(canonicalize_name(clean_dirname.substr(0, it))); |
88 | 306 | if (fspath.exists_ && !fspath.is_directory_) | 314 | if (fspath.exists_ && !fspath.is_directory_) |
89 | 307 | throw wexception("'%s' in directory '%s' exists and is not a directory", | 315 | throw wexception("'%s' in directory '%s' exists and is not a directory", |
91 | 308 | dirname.substr(0, it).c_str(), directory_.c_str()); | 316 | clean_dirname.substr(0, it).c_str(), directory_.c_str()); |
92 | 309 | if (!fspath.exists_) | 317 | if (!fspath.exists_) |
94 | 310 | make_directory(dirname.substr(0, it)); | 318 | make_directory(clean_dirname.substr(0, it)); |
95 | 311 | 319 | ||
96 | 312 | if (it == std::string::npos) | 320 | if (it == std::string::npos) |
97 | 313 | break; | 321 | break; |
98 | @@ -315,7 +323,7 @@ | |||
99 | 315 | } | 323 | } |
100 | 316 | } catch (const std::exception& e) { | 324 | } catch (const std::exception& e) { |
101 | 317 | throw wexception("RealFSImpl::ensure_directory_exists(%s) in directory '%s': %s", | 325 | throw wexception("RealFSImpl::ensure_directory_exists(%s) in directory '%s': %s", |
103 | 318 | dirname.c_str(), directory_.c_str(), e.what()); | 326 | clean_dirname.c_str(), directory_.c_str(), e.what()); |
104 | 319 | } | 327 | } |
105 | 320 | } | 328 | } |
106 | 321 | 329 | ||
107 | 322 | 330 | ||
108 | === modified file 'src/io/filesystem/layered_filesystem.cc' | |||
109 | --- src/io/filesystem/layered_filesystem.cc 2016-08-04 15:49:05 +0000 | |||
110 | +++ src/io/filesystem/layered_filesystem.cc 2016-08-04 19:19:13 +0000 | |||
111 | @@ -22,6 +22,9 @@ | |||
112 | 22 | #include <cstdio> | 22 | #include <cstdio> |
113 | 23 | #include <memory> | 23 | #include <memory> |
114 | 24 | 24 | ||
115 | 25 | #include <boost/algorithm/string/predicate.hpp> | ||
116 | 26 | #include <boost/regex.hpp> | ||
117 | 27 | |||
118 | 25 | #include "base/log.h" | 28 | #include "base/log.h" |
119 | 26 | #include "base/wexception.h" | 29 | #include "base/wexception.h" |
120 | 27 | #include "io/fileread.h" | 30 | #include "io/fileread.h" |
121 | @@ -38,6 +41,18 @@ | |||
122 | 38 | LayeredFileSystem::~LayeredFileSystem() { | 41 | LayeredFileSystem::~LayeredFileSystem() { |
123 | 39 | } | 42 | } |
124 | 40 | 43 | ||
125 | 44 | bool LayeredFileSystem::is_legal_filename(const std::string& filename) { | ||
126 | 45 | // No potential file separators or other potentially illegal characters | ||
127 | 46 | // https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx | ||
128 | 47 | // http://www.linfo.org/file_name.html | ||
129 | 48 | // https://support.apple.com/en-us/HT202808 | ||
130 | 49 | // We can't just regex for word & digit characters here because of non-Latin scripts. | ||
131 | 50 | boost::regex re(".*[<>:\"|?*/\\\\].*"); | ||
132 | 51 | return !filename.empty() && !boost::starts_with(filename, ".") && | ||
133 | 52 | !boost::starts_with(filename, " ") && !boost::starts_with(filename, "-") && | ||
134 | 53 | !boost::regex_match(filename, re); | ||
135 | 54 | } | ||
136 | 55 | |||
137 | 41 | /** | 56 | /** |
138 | 42 | * Just assume that at least one of our child FSs is writable | 57 | * Just assume that at least one of our child FSs is writable |
139 | 43 | */ | 58 | */ |
140 | 44 | 59 | ||
141 | === modified file 'src/io/filesystem/layered_filesystem.h' | |||
142 | --- src/io/filesystem/layered_filesystem.h 2016-08-04 15:49:05 +0000 | |||
143 | +++ src/io/filesystem/layered_filesystem.h 2016-08-04 19:19:13 +0000 | |||
144 | @@ -60,6 +60,8 @@ | |||
145 | 60 | 60 | ||
146 | 61 | std::set<std::string> list_directory(const std::string& path) override; | 61 | std::set<std::string> list_directory(const std::string& path) override; |
147 | 62 | 62 | ||
148 | 63 | /// Returns true if the filename is legal in all operating systems | ||
149 | 64 | static bool is_legal_filename(const std::string& filename); | ||
150 | 63 | bool is_writable() const override; | 65 | bool is_writable() const override; |
151 | 64 | bool file_exists(const std::string& path) override; | 66 | bool file_exists(const std::string& path) override; |
152 | 65 | bool is_directory(const std::string& path) override; | 67 | bool is_directory(const std::string& path) override; |
153 | 66 | 68 | ||
154 | === modified file 'src/logic/save_handler.cc' | |||
155 | --- src/logic/save_handler.cc 2016-08-04 15:49:05 +0000 | |||
156 | +++ src/logic/save_handler.cc 2016-08-04 19:19:13 +0000 | |||
157 | @@ -174,8 +174,10 @@ | |||
158 | 174 | */ | 174 | */ |
159 | 175 | std::string SaveHandler::create_file_name(const std::string& dir, | 175 | std::string SaveHandler::create_file_name(const std::string& dir, |
160 | 176 | const std::string& filename) const { | 176 | const std::string& filename) const { |
162 | 177 | // Append directory name | 177 | // Append directory name. |
163 | 178 | std::string complete_filename = dir + g_fs->file_separator() + filename; | 178 | std::string complete_filename = dir + g_fs->file_separator() + filename; |
164 | 179 | // Trim it for preceding/trailing whitespaces in user input | ||
165 | 180 | boost::trim(complete_filename); | ||
166 | 179 | 181 | ||
167 | 180 | // Now check if the extension matches (ignoring case) | 182 | // Now check if the extension matches (ignoring case) |
168 | 181 | if (!boost::iends_with(filename, WLGF_SUFFIX)) { | 183 | if (!boost::iends_with(filename, WLGF_SUFFIX)) { |
169 | 182 | 184 | ||
170 | === modified file 'src/wui/game_main_menu_save_game.cc' | |||
171 | --- src/wui/game_main_menu_save_game.cc 2016-08-04 15:49:05 +0000 | |||
172 | +++ src/wui/game_main_menu_save_game.cc 2016-08-04 19:19:13 +0000 | |||
173 | @@ -207,7 +207,8 @@ | |||
174 | 207 | * The editbox was changed. Enable ok button | 207 | * The editbox was changed. Enable ok button |
175 | 208 | */ | 208 | */ |
176 | 209 | void GameMainMenuSaveGame::edit_box_changed() { | 209 | void GameMainMenuSaveGame::edit_box_changed() { |
178 | 210 | button_ok_->set_enabled(editbox_.text().size()); | 210 | // Prevent the user from creating nonsense directory names, like e.g. ".." or "...". |
179 | 211 | button_ok_->set_enabled(LayeredFileSystem::is_legal_filename(editbox_.text())); | ||
180 | 211 | } | 212 | } |
181 | 212 | 213 | ||
182 | 213 | static void dosave(InteractiveGameBase& igbase, const std::string& complete_filename) { | 214 | static void dosave(InteractiveGameBase& igbase, const std::string& complete_filename) { |
Ah, bl*** file separators, my bad.
I will look into concatenating this properly.