Never allow '..' in OS.get_safe_dir_name(), fixes minor security conc…

…ern, add unit tests for it

Previous behavior: '..' is allowed if 'allow_dir_separator' is false, which doesn't really make sense, I think it was a mistake in original implementation.

core/os/os.cpp

@@ -211,11 +211,12 @@ uint64_t OS::get_embedded_pck_offset() const {
 
 // Helper function to ensure that a dir name/path will be valid on the OS
 String OS::get_safe_dir_name(const String &p_dir_name, bool p_allow_dir_separator) const {
-	Vector<String> invalid_chars = String(": * ? \" < > |").split(" ");
-	if (p_allow_dir_separator) {
-		// Dir separators are allowed, but disallow ".." to avoid going up the filesystem
-		invalid_chars.push_back("..");
-	} else {
+	// Disallow dangerous directory characters, especially '..' which can be used to traverse the filesystem.
+	Vector<String> invalid_chars = String(": * ? \" < > | ..").split(" ");
+
+	// Optionally, allow the directory separator so users can create subdirectories.
+	if (!p_allow_dir_separator) {
+		// Disallowed, so add it to invalid characters.
 		invalid_chars.push_back("/");
 	}
 

tests/core/os/test_os.h

@@ -153,6 +153,32 @@ TEST_CASE("[OS] Execute") {
 #endif
 }
 
+TEST_CASE("[OS] Ensure get_safe_dir_name returns safe results") {
+	// Path separators ('/', '\') are only allowed if the second argument is true.
+	CHECK_MESSAGE(
+			OS::get_singleton()->get_safe_dir_name("a/b/c") == "a-b-c",
+			"Safe dir names should not contain directory separators, unless specified.");
+	CHECK_MESSAGE(
+			OS::get_singleton()->get_safe_dir_name("a/b/c", true) == "a/b/c",
+			"Directory separators should be allowed, when specified.");
+	CHECK_MESSAGE(
+			OS::get_singleton()->get_safe_dir_name("a\\b\\c") == "a-b-c",
+			"Safe dir names should not contain directory separators, unless specified.");
+	CHECK_MESSAGE(
+			OS::get_singleton()->get_safe_dir_name("a\\b\\c", true) == "a/b/c",
+			"Legacy path separators should be replaced with slashes, when specified.");
+
+	// Never allow '..', it is dangerous! (whether that is an overstatement depends on the use case, but we should always be careful with user input).
+	CHECK_MESSAGE(
+			OS::get_singleton()->get_safe_dir_name("../../secret/files.txt") == "----secret-files.txt",
+			"Navigational path was not properly handled, the file system could have been traversed.");
+
+	// Allow '.' because it is still valid in dir/file names.
+	CHECK_MESSAGE(
+			OS::get_singleton()->get_safe_dir_name("some_file.txt") == "some_file.txt",
+			"Individual full stops '.' should be allowed, they are harmless in this context.");
+}
+
 } // namespace TestOS
 
 #endif // TEST_OS_H