Refactor is_safe_filename and add test

utility/CMakeLists.txt

@@ -82,3 +82,8 @@ if(FREECIV_ENABLE_NLS)
   target_include_directories(utility PUBLIC ${Intl_INCLUDE_DIRS})
   target_link_libraries(utility PUBLIC ${Intl_LIBRARIES})
 endif()
+
+# Tests
+if (BUILD_TESTING)
+  add_subdirectory(tests)
+endif()

utility/shared.cpp

@@ -33,6 +33,7 @@
 #include <QCoreApplication>
 #include <QDateTime>
 #include <QDir>
+#include <QRegularExpression>
 #include <QStandardPaths>
 #include <QString>
 #include <QtGlobal>
@@ -208,29 +209,11 @@ static bool is_ascii(char ch)
    Check if the name is safe security-wise.  This is intended to be used to
    make sure an untrusted filename is safe to be used.
  */
-bool is_safe_filename(const char *name)
+bool is_safe_filename(const QString &name)
 {
-  int i = 0;
-
-  // must not be nullptr or empty
-  if (!name || *name == '\0') {
-    return false;
-  }
-
-  for (; '\0' != name[i]; i++) {
-    if (nullptr == strchr(".@", name[i])
-        && nullptr == strchr(base64url, name[i])) {
-      return false;
-    }
-  }
-
-  // we don't allow the filename to ascend directories
-  if (strstr(name, PARENT_DIR_OPERATOR)) {
-    return false;
-  }
-
-  // Otherwise, it is okay...
-  return true;
+  const QRegularExpression regex(QLatin1String("^[\\w_\\-.@]+$"));
+  return regex.match(name).hasMatch()
+         && !name.contains(QLatin1String(PARENT_DIR_OPERATOR));
 }
 
 /**

utility/shared.h

@@ -107,7 +107,7 @@ const char *int_to_text(unsigned int number);
 
 bool is_ascii_name(const char *name);
 bool is_base64url(const char *s);
-bool is_safe_filename(const char *name);
+bool is_safe_filename(const QString &name);
 void randomize_base64url_string(char *s, size_t n);
 
 char *skip_leading_spaces(char *s);

utility/tests/CMakeLists.txt

@@ -0,0 +1,5 @@
+set(CMAKE_AUTOMOC ON)
+
+add_executable(test_utility_paths test_paths.cpp)
+target_link_libraries(test_utility_paths PRIVATE Qt5::Test utility)
+add_test(NAME test_utility_paths COMMAND test_utility_paths)

utility/tests/test_paths.cpp

@@ -0,0 +1,27 @@
+#include "shared.h"
+
+#include <QtTest>
+
+/**
+ * Tests functions acting on paths
+ */
+class test_paths : public QObject {
+  Q_OBJECT
+
+private slots:
+  void is_safe_filename();
+};
+
+/**
+ * Tests \ref ::is_safe_filename
+ */
+void test_paths::is_safe_filename()
+{
+  QCOMPARE(::is_safe_filename(QLatin1String("")), false);
+  QCOMPARE(::is_safe_filename(QLatin1String("abcABC_-._")), true);
+  QCOMPARE(::is_safe_filename(QLatin1String("a/b")), false);
+  QCOMPARE(::is_safe_filename(QLatin1String("..")), false);
+}
+
+QTEST_MAIN(test_paths)
+#include "test_paths.moc"