From d7c6cbc6bf633767f4058fe2df0eecb3a80cced9 Mon Sep 17 00:00:00 2001 From: Nathan Vegdahl Date: Tue, 22 Jul 2025 14:47:56 +0200 Subject: [PATCH] Core: Sanitize ID/struct names when used in path templates In #139438 we added three path template variables that get their values from user-specified names: - `scene_name` - `camera_name` - `node_name` These names can contain characters that are illegal in filenames on some systems, and also characters like "/" that would be interpreted as path separators in a filepath. This can lead to unexpected outcomes. For example, if a node's name is "Hue/Saturation" and the `{node_name}` variable is used in a path, it would result in a "Hue" directory which in turn contains a "Saturation" file. This is almost certainly not what the user intended, because the slash in that name is not semantically supposed to be a path separator. This PR addresses this problem by splitting string variables into two types: strings and filepaths. The contents of string variables are sanitized to be valid filenames when substituted into a path template, but the contents of filepath variables are left as-is. Variables such as `blend_name`, `blend_dir`, etc. that actually represent path components are then added as filepath variables, while variables such as `scene_name` that represent plain strings without path semantics are added as string variables. Pull Request: https://projects.blender.org/blender/blender/pulls/142679 --- .../blender/blenkernel/BKE_path_templates.hh | 43 ++++++- .../blenkernel/intern/path_templates.cc | 66 ++++++++-- .../blenkernel/intern/path_templates_test.cc | 116 +++++++++++------- 3 files changed, 162 insertions(+), 63 deletions(-) diff --git a/source/blender/blenkernel/BKE_path_templates.hh b/source/blender/blenkernel/BKE_path_templates.hh index 6e97325a1f8..c1d018a3d50 100644 --- a/source/blender/blenkernel/BKE_path_templates.hh +++ b/source/blender/blenkernel/BKE_path_templates.hh @@ -91,12 +91,26 @@ namespace blender::bke::path_templates { * transient for collecting data that is relevant/available in a given * templating context. * - * There are currently three supported variable types: string, integer, and - * float. Names must be unique across all types: you can't have a string *and* + * There are currently four supported variable types: + * + * - String + * - Filepath + * - Integer + * - Float + * + * Names must be unique across all variable types: you can't have a string *and* * integer both with the name "bob". + * + * A filepath variable can contain either a full or partial filepath. The + * distinction between string and filepath variables exists because non-path + * strings may include phrases like "and/or" or "A:Left", which shouldn't be + * interpreted with path semantics. When used in path templating, the contents + * of string variables are therefore sanitized (replacing "/", etc.), but the + * contents of filepath variables are left as-is. */ class VariableMap { blender::Map strings_; + blender::Map filepaths_; blender::Map integers_; blender::Map floats_; @@ -125,6 +139,17 @@ class VariableMap { */ bool add_string(blender::StringRef name, blender::StringRef value); + /** + * Add a filepath variable with the given name and value. + * + * If there is already a variable with that name, regardless of type, the new + * variable is *not* added (no overwriting). + * + * \return True if the variable was successfully added, false if there was + * already a variable with that name. + */ + bool add_filepath(blender::StringRef name, blender::StringRef value); + /** * Add an integer variable with the given name and value. * @@ -155,6 +180,14 @@ class VariableMap { */ std::optional get_string(blender::StringRef name) const; + /** + * Fetch the value of the filepath variable with the given name. + * + * \return The value if a filepath variable with that name exists, + * #std::nullopt otherwise. + */ + std::optional get_filepath(blender::StringRef name) const; + /** * Fetch the value of the integer variable with the given name. * @@ -189,9 +222,9 @@ class VariableMap { * \return True if the variable was successfully added, false if there was * already a variable with that name. */ - bool add_filename(blender::StringRef var_name, - blender::StringRefNull full_path, - blender::StringRef fallback); + bool add_filename_only(blender::StringRef var_name, + blender::StringRefNull full_path, + blender::StringRef fallback); /** * Add the path up-to-but-not-including the filename as a variable. diff --git a/source/blender/blenkernel/intern/path_templates.cc b/source/blender/blenkernel/intern/path_templates.cc index 4bd8345ea2d..9cb98c8896a 100644 --- a/source/blender/blenkernel/intern/path_templates.cc +++ b/source/blender/blenkernel/intern/path_templates.cc @@ -6,6 +6,8 @@ #include "BLT_translation.hh" +#include "BLI_math_base.hh" +#include "BLI_path_utils.hh" #include "BLI_span.hh" #include "BKE_context.hh" @@ -27,6 +29,9 @@ bool VariableMap::contains(blender::StringRef name) const if (this->strings_.contains(name)) { return true; } + if (this->filepaths_.contains(name)) { + return true; + } if (this->integers_.contains(name)) { return true; } @@ -41,6 +46,9 @@ bool VariableMap::remove(blender::StringRef name) if (this->strings_.remove(name)) { return true; } + if (this->filepaths_.remove(name)) { + return true; + } if (this->integers_.remove(name)) { return true; } @@ -59,6 +67,15 @@ bool VariableMap::add_string(blender::StringRef name, blender::StringRef value) return true; } +bool VariableMap::add_filepath(blender::StringRef name, blender::StringRef value) +{ + if (this->contains(name)) { + return false; + } + this->filepaths_.add_new(name, value); + return true; +} + bool VariableMap::add_integer(blender::StringRef name, const int64_t value) { if (this->contains(name)) { @@ -86,6 +103,15 @@ std::optional VariableMap::get_string(blender::StringRef return blender::StringRefNull(*value); } +std::optional VariableMap::get_filepath(blender::StringRef name) const +{ + const std::string *value = this->filepaths_.lookup_ptr(name); + if (value == nullptr) { + return std::nullopt; + } + return blender::StringRefNull(*value); +} + std::optional VariableMap::get_integer(blender::StringRef name) const { const int64_t *value = this->integers_.lookup_ptr(name); @@ -104,22 +130,24 @@ std::optional VariableMap::get_float(blender::StringRef name) const return *value; } -bool VariableMap::add_filename(StringRef var_name, StringRefNull full_path, StringRef fallback) +bool VariableMap::add_filename_only(StringRef var_name, + StringRefNull full_path, + StringRef fallback) { const char *file_name = BLI_path_basename(full_path.c_str()); const char *file_name_end = BLI_path_extension_or_end(file_name); if (file_name[0] == '\0') { /* If there is no file name, default to the fallback. */ - return this->add_string(var_name, fallback); + return this->add_filepath(var_name, fallback); } else if (file_name_end == file_name) { /* When the filename has no extension, but starts with a period. */ - return this->add_string(var_name, StringRef(file_name)); + return this->add_filepath(var_name, StringRef(file_name)); } else { /* Normal case. */ - return this->add_string(var_name, StringRef(file_name, file_name_end)); + return this->add_filepath(var_name, StringRef(file_name, file_name_end)); } } @@ -129,12 +157,12 @@ bool VariableMap::add_path_up_to_file(StringRef var_name, { /* Empty path. */ if (full_path.is_empty()) { - return this->add_string(var_name, fallback); + return this->add_filepath(var_name, fallback); } /* No filename at the end. */ if (BLI_path_basename(full_path.c_str()) == full_path.end()) { - return this->add_string(var_name, full_path); + return this->add_filepath(var_name, full_path); } Vector dir_path(full_path.size() + 1); @@ -144,10 +172,10 @@ bool VariableMap::add_path_up_to_file(StringRef var_name, if (!success || dir_path[0] == '\0') { /* If no path before the filename, default to the fallback. */ - return this->add_string(var_name, fallback); + return this->add_filepath(var_name, fallback); } - return this->add_string(var_name, dir_path.data()); + return this->add_filepath(var_name, dir_path.data()); } bool operator==(const Error &left, const Error &right) @@ -249,7 +277,8 @@ void BKE_add_template_variables_general(VariableMap &variables, const ID *path_o { const char *g_blend_file_path = BKE_main_blendfile_path_from_global(); - variables.add_filename("blend_name", g_blend_file_path, blender::StringRef(DATA_("Unsaved"))); + variables.add_filename_only( + "blend_name", g_blend_file_path, blender::StringRef(DATA_("Unsaved"))); /* Note: fallback to "./" for unsaved files, which if used at the start of a * path is equivalent to the current working directory. This is consistent @@ -260,7 +289,7 @@ void BKE_add_template_variables_general(VariableMap &variables, const ID *path_o /* Library blend filepath (a.k.a. path to the blend file that actually owns the ID). */ if (path_owner_id) { const char *lib_blend_file_path = ID_BLEND_PATH_FROM_GLOBAL(path_owner_id); - variables.add_filename( + variables.add_filename_only( "blend_name_lib", lib_blend_file_path, blender::StringRef(DATA_("Unsaved"))); /* Note: fallback to "./" for unsaved files, which if used at the start of a @@ -306,7 +335,7 @@ void BKE_add_template_variables_for_node(blender::bke::path_templates::VariableM /* -------------------------------------------------------------------- */ -#define FORMAT_BUFFER_SIZE 128 +#define FORMAT_BUFFER_SIZE 512 namespace { @@ -859,14 +888,25 @@ static blender::Vector eval_template(char *out_path, if (std::optional string_value = template_variables.get_string( token.variable_name)) { - /* String variable found, but we only process it if there's no format - * specifier: string variables do not support format specifiers. */ if (token.format.type != FormatSpecifierType::NONE) { /* String variables don't take format specifiers: error. */ errors.append({ErrorType::FORMAT_SPECIFIER, token.byte_range}); continue; } STRNCPY(replacement_string, string_value->c_str()); + BLI_path_make_safe_filename(replacement_string); + break; + } + + if (std::optional path_value = template_variables.get_filepath( + token.variable_name)) + { + if (token.format.type != FormatSpecifierType::NONE) { + /* Path variables don't take format specifiers: error. */ + errors.append({ErrorType::FORMAT_SPECIFIER, token.byte_range}); + continue; + } + STRNCPY(replacement_string, path_value->c_str()); break; } diff --git a/source/blender/blenkernel/intern/path_templates_test.cc b/source/blender/blenkernel/intern/path_templates_test.cc index 079a6757522..d8fe99771d1 100644 --- a/source/blender/blenkernel/intern/path_templates_test.cc +++ b/source/blender/blenkernel/intern/path_templates_test.cc @@ -68,98 +68,119 @@ TEST(path_templates, VariableMap) EXPECT_FALSE(map.contains("hello")); EXPECT_FALSE(map.remove("hello")); EXPECT_EQ(std::nullopt, map.get_string("hello")); + EXPECT_EQ(std::nullopt, map.get_filepath("hello")); EXPECT_EQ(std::nullopt, map.get_integer("hello")); EXPECT_EQ(std::nullopt, map.get_float("hello")); /* Populate the map. */ EXPECT_TRUE(map.add_string("hello", "What a wonderful world.")); + EXPECT_TRUE(map.add_filepath("where", "/my/path")); EXPECT_TRUE(map.add_integer("bye", 42)); EXPECT_TRUE(map.add_float("what", 3.14159)); /* Attempting to add variables with those names again should fail, since they * already exist now. */ EXPECT_FALSE(map.add_string("hello", "Sup.")); + EXPECT_FALSE(map.add_string("where", "Sup.")); EXPECT_FALSE(map.add_string("bye", "Sup.")); EXPECT_FALSE(map.add_string("what", "Sup.")); + EXPECT_FALSE(map.add_filepath("hello", "/place")); + EXPECT_FALSE(map.add_filepath("where", "/place")); + EXPECT_FALSE(map.add_filepath("bye", "/place")); + EXPECT_FALSE(map.add_filepath("what", "/place")); EXPECT_FALSE(map.add_integer("hello", 2)); + EXPECT_FALSE(map.add_integer("where", 2)); EXPECT_FALSE(map.add_integer("bye", 2)); EXPECT_FALSE(map.add_integer("what", 2)); EXPECT_FALSE(map.add_float("hello", 2.71828)); + EXPECT_FALSE(map.add_float("where", 2.71828)); EXPECT_FALSE(map.add_float("bye", 2.71828)); EXPECT_FALSE(map.add_float("what", 2.71828)); /* Confirm that the right variables exist. */ EXPECT_TRUE(map.contains("hello")); + EXPECT_TRUE(map.contains("where")); EXPECT_TRUE(map.contains("bye")); EXPECT_TRUE(map.contains("what")); EXPECT_FALSE(map.contains("not here")); /* Fetch the variables we added. */ EXPECT_EQ("What a wonderful world.", map.get_string("hello")); + EXPECT_EQ("/my/path", map.get_filepath("where")); EXPECT_EQ(42, map.get_integer("bye")); EXPECT_EQ(3.14159, map.get_float("what")); /* The same variables shouldn't exist for the other types, despite our attempt * to add them earlier. */ + EXPECT_EQ(std::nullopt, map.get_filepath("hello")); EXPECT_EQ(std::nullopt, map.get_integer("hello")); EXPECT_EQ(std::nullopt, map.get_float("hello")); + EXPECT_EQ(std::nullopt, map.get_string("where")); + EXPECT_EQ(std::nullopt, map.get_integer("where")); + EXPECT_EQ(std::nullopt, map.get_float("where")); EXPECT_EQ(std::nullopt, map.get_string("bye")); + EXPECT_EQ(std::nullopt, map.get_filepath("bye")); EXPECT_EQ(std::nullopt, map.get_float("bye")); EXPECT_EQ(std::nullopt, map.get_string("what")); + EXPECT_EQ(std::nullopt, map.get_filepath("what")); EXPECT_EQ(std::nullopt, map.get_integer("what")); /* Remove the variables. */ EXPECT_TRUE(map.remove("hello")); + EXPECT_TRUE(map.remove("where")); EXPECT_TRUE(map.remove("bye")); EXPECT_TRUE(map.remove("what")); /* The variables shouldn't exist anymore. */ EXPECT_FALSE(map.contains("hello")); + EXPECT_FALSE(map.contains("where")); EXPECT_FALSE(map.contains("bye")); EXPECT_FALSE(map.contains("what")); EXPECT_EQ(std::nullopt, map.get_string("hello")); + EXPECT_EQ(std::nullopt, map.get_filepath("where")); EXPECT_EQ(std::nullopt, map.get_integer("bye")); EXPECT_EQ(std::nullopt, map.get_float("what")); EXPECT_FALSE(map.remove("hello")); + EXPECT_FALSE(map.remove("where")); EXPECT_FALSE(map.remove("bye")); EXPECT_FALSE(map.remove("what")); } -TEST(path_templates, VariableMap_add_filename) +TEST(path_templates, VariableMap_add_filename_only) { VariableMap map; - EXPECT_TRUE(map.add_filename("a", "/home/bob/project_joe/scene_3.blend", "fallback")); - EXPECT_EQ("scene_3", map.get_string("a")); + EXPECT_TRUE(map.add_filename_only("a", "/home/bob/project_joe/scene_3.blend", "fallback")); + EXPECT_EQ("scene_3", map.get_filepath("a")); - EXPECT_TRUE(map.add_filename("b", "/home/bob/project_joe/scene_3", "fallback")); - EXPECT_EQ("scene_3", map.get_string("b")); + EXPECT_TRUE(map.add_filename_only("b", "/home/bob/project_joe/scene_3", "fallback")); + EXPECT_EQ("scene_3", map.get_filepath("b")); - EXPECT_TRUE(map.add_filename("c", "/home/bob/project_joe/scene.03.blend", "fallback")); - EXPECT_EQ("scene.03", map.get_string("c")); + EXPECT_TRUE(map.add_filename_only("c", "/home/bob/project_joe/scene.03.blend", "fallback")); + EXPECT_EQ("scene.03", map.get_filepath("c")); - EXPECT_TRUE(map.add_filename("d", "/home/bob/project_joe/.scene_3.blend", "fallback")); - EXPECT_EQ(".scene_3", map.get_string("d")); + EXPECT_TRUE(map.add_filename_only("d", "/home/bob/project_joe/.scene_3.blend", "fallback")); + EXPECT_EQ(".scene_3", map.get_filepath("d")); - EXPECT_TRUE(map.add_filename("e", "/home/bob/project_joe/.scene_3", "fallback")); - EXPECT_EQ(".scene_3", map.get_string("e")); + EXPECT_TRUE(map.add_filename_only("e", "/home/bob/project_joe/.scene_3", "fallback")); + EXPECT_EQ(".scene_3", map.get_filepath("e")); - EXPECT_TRUE(map.add_filename("f", "scene_3.blend", "fallback")); - EXPECT_EQ("scene_3", map.get_string("f")); + EXPECT_TRUE(map.add_filename_only("f", "scene_3.blend", "fallback")); + EXPECT_EQ("scene_3", map.get_filepath("f")); - EXPECT_TRUE(map.add_filename("g", "scene_3", "fallback")); - EXPECT_EQ("scene_3", map.get_string("g")); + EXPECT_TRUE(map.add_filename_only("g", "scene_3", "fallback")); + EXPECT_EQ("scene_3", map.get_filepath("g")); /* No filename in path (ending slash means it's a directory). */ - EXPECT_TRUE(map.add_filename("h", "/home/bob/project_joe/", "fallback")); - EXPECT_EQ("fallback", map.get_string("h")); + EXPECT_TRUE(map.add_filename_only("h", "/home/bob/project_joe/", "fallback")); + EXPECT_EQ("fallback", map.get_filepath("h")); /* Empty path. */ - EXPECT_TRUE(map.add_filename("i", "", "fallback")); - EXPECT_EQ("fallback", map.get_string("i")); + EXPECT_TRUE(map.add_filename_only("i", "", "fallback")); + EXPECT_EQ("fallback", map.get_filepath("i")); /* Attempt to add already-added variable. */ - EXPECT_FALSE(map.add_filename("i", "", "fallback")); + EXPECT_FALSE(map.add_filename_only("i", "", "fallback")); } TEST(path_templates, VariableMap_add_path_up_to_file) @@ -167,31 +188,31 @@ TEST(path_templates, VariableMap_add_path_up_to_file) VariableMap map; EXPECT_TRUE(map.add_path_up_to_file("a", "/home/bob/project_joe/scene_3.blend", "fallback")); - EXPECT_EQ("/home/bob/project_joe/", map.get_string("a")); + EXPECT_EQ("/home/bob/project_joe/", map.get_filepath("a")); EXPECT_TRUE(map.add_path_up_to_file("b", "project_joe/scene_3.blend", "fallback")); - EXPECT_EQ("project_joe/", map.get_string("b")); + EXPECT_EQ("project_joe/", map.get_filepath("b")); EXPECT_TRUE(map.add_path_up_to_file("c", "/scene_3.blend", "fallback")); - EXPECT_EQ("/", map.get_string("c")); + EXPECT_EQ("/", map.get_filepath("c")); /* No filename in path (ending slash means it's a directory). */ EXPECT_TRUE(map.add_path_up_to_file("e", "/home/bob/project_joe/", "fallback")); - EXPECT_EQ("/home/bob/project_joe/", map.get_string("e")); + EXPECT_EQ("/home/bob/project_joe/", map.get_filepath("e")); EXPECT_TRUE(map.add_path_up_to_file("f", "/", "fallback")); - EXPECT_EQ("/", map.get_string("f")); + EXPECT_EQ("/", map.get_filepath("f")); /* No leading path. */ EXPECT_TRUE(map.add_path_up_to_file("d", "scene_3.blend", "fallback")); - EXPECT_EQ("fallback", map.get_string("d")); + EXPECT_EQ("fallback", map.get_filepath("d")); /* Empty path. */ EXPECT_TRUE(map.add_path_up_to_file("g", "", "fallback")); - EXPECT_EQ("fallback", map.get_string("g")); + EXPECT_EQ("fallback", map.get_filepath("g")); /* Attempt to add already-added variable. */ - EXPECT_FALSE(map.add_filename("g", "", "fallback")); + EXPECT_FALSE(map.add_filename_only("g", "", "fallback")); } struct PathTemplateTestCase { @@ -206,7 +227,10 @@ TEST(path_templates, validate_and_apply_template) { variables.add_string("hi", "hello"); variables.add_string("bye", "goodbye"); - variables.add_string("long", "This string is exactly 32 bytes."); + variables.add_string("empty", ""); + variables.add_string("sanitize", "./\\?*:|\"<>"); + variables.add_string("long", "This string is exactly 32 bytes_"); + variables.add_filepath("path", "C:\\and/or/../nor/"); variables.add_integer("the_answer", 42); variables.add_integer("prime", 7); variables.add_integer("i_negative", -7); @@ -222,10 +246,12 @@ TEST(path_templates, validate_and_apply_template) const Vector test_cases = { /* Simple case, testing all variables. */ { - "{hi}_{bye}_{the_answer}_{prime}_{i_negative}_{pi}_{e}_{ntsc}_{two}_{f_negative}_{huge}_" - "{tiny}", - "hello_goodbye_42_7_-7_3.141592653589793_2.718281828459045_29.970029970029973_2.0_-3." - "141592653589793_2e+32_2e-33", + "{hi}_{bye}_{empty}_{sanitize}_{path}" + "_{the_answer}_{prime}_{i_negative}_{pi}_{e}_{ntsc}_{two}" + "_{f_negative}_{huge}_{tiny}", + "hello_goodbye__.__________C:\\and/or/../nor/" + "_42_7_-7_3.141592653589793_2.718281828459045_29.970029970029973_2.0" + "_-3.141592653589793_2e+32_2e-33", {}, }, @@ -379,18 +405,18 @@ TEST(path_templates, validate_and_apply_template) "long}{long}{long}{long}{long}{long}{long}{long}{long}{long}{long}{long}{long}{long}{" "long}{long}", - "___This string is exactly 32 bytes.This string is exactly 32 bytes.This string is " - "exactly 32 bytes.This string is exactly 32 bytes.This string is exactly 32 bytes.This " - "string is exactly 32 bytes.This string is exactly 32 bytes.This string is exactly 32 " - "bytes.This string is exactly 32 bytes.This string is exactly 32 bytes.This string is " - "exactly 32 bytes.This string is exactly 32 bytes.This string is exactly 32 bytes.This " - "string is exactly 32 bytes.This string is exactly 32 bytes.This string is exactly 32 " - "bytes.This string is exactly 32 bytes.This string is exactly 32 bytes.This string is " - "exactly 32 bytes.This string is exactly 32 bytes.This string is exactly 32 bytes.This " - "string is exactly 32 bytes.This string is exactly 32 bytes.This string is exactly 32 " - "bytes.This string is exactly 32 bytes.This string is exactly 32 bytes.This string is " - "exactly 32 bytes.This string is exactly 32 bytes.This string is exactly 32 bytes.This " - "string is exactly 32 bytes.This string is exactly 32 bytes.This string is exactly 32 " + "___This string is exactly 32 bytes_This string is exactly 32 bytes_This string is " + "exactly 32 bytes_This string is exactly 32 bytes_This string is exactly 32 bytes_This " + "string is exactly 32 bytes_This string is exactly 32 bytes_This string is exactly 32 " + "bytes_This string is exactly 32 bytes_This string is exactly 32 bytes_This string is " + "exactly 32 bytes_This string is exactly 32 bytes_This string is exactly 32 bytes_This " + "string is exactly 32 bytes_This string is exactly 32 bytes_This string is exactly 32 " + "bytes_This string is exactly 32 bytes_This string is exactly 32 bytes_This string is " + "exactly 32 bytes_This string is exactly 32 bytes_This string is exactly 32 bytes_This " + "string is exactly 32 bytes_This string is exactly 32 bytes_This string is exactly 32 " + "bytes_This string is exactly 32 bytes_This string is exactly 32 bytes_This string is " + "exactly 32 bytes_This string is exactly 32 bytes_This string is exactly 32 bytes_This " + "string is exactly 32 bytes_This string is exactly 32 bytes_This string is exactly 32 " "by", {},