From 4ced15faf4ae15fca77960698dc3f2fec1b58a09 Mon Sep 17 00:00:00 2001 From: luisg0c Date: Tue, 28 Jul 2026 09:04:52 -0300 Subject: [PATCH] fix(config): validate overlay_capture_persist_dir independent of host path grammar std::filesystem answers by the *host* path grammar, so this guard meant different things on different hosts. Under POSIX, "C:/outside" is a relative path whose first component is literally "C:", a backslash is an ordinary filename character, and "\\server\share" is a single relative component -- so Windows-authored absolute paths and backslash ".." traversals were accepted. Under Windows, a rooted "/outside" has no root name, so operator/ drops the project root's relative path and the result lands at the drive root; and a drive-relative "C:foo" is not absolute at all, yet replaces the whole path when the project sits on another drive. Scan the raw config bytes under both separator conventions instead, and reject an embedded NUL. TOML forbids a raw NUL byte but allows the \u0000 escape that produces one, and the runtime hands this value on as a C string (runtime/src/main.cpp:5236-5241 -> overlay_capture.c:230), so "..\u0000keep" validated as a four-byte component and then truncated to ".." at the point of use, escaping the project. Validating what the consumer actually sees closes that gap. recompiler/src/config_loader.h carries the contract next to the field and still described the old, narrower rule; docs/AOT_OVERLAY_PLAN.md carries the only prose statement of it. Both now match what is enforced. Note the doc claims identical accept/reject across hosts, not identical meaning: a value like "nested\dir" is accepted everywhere but still names different things. The registered recompiler_patch_test assertion "parser rejects absolute capture history directory" failed on POSIX before this; the suite is now 33/33 on macOS arm64. --- docs/AOT_OVERLAY_PLAN.md | 5 +- recompiler/src/config_loader.cpp | 48 ++++++++++++++----- recompiler/src/config_loader.h | 8 ++-- recompiler/tests/recompiler_patch_test.cpp | 54 ++++++++++++++++++++++ 4 files changed, 100 insertions(+), 15 deletions(-) diff --git a/docs/AOT_OVERLAY_PLAN.md b/docs/AOT_OVERLAY_PLAN.md index 77d51027..a6031d1e 100644 --- a/docs/AOT_OVERLAY_PLAN.md +++ b/docs/AOT_OVERLAY_PLAN.md @@ -331,7 +331,10 @@ additively merging every valid snapshot. Development configs may also set a project-relative `overlay_capture_persist_dir`. This creates one atomically published, immutable -JSON snapshot per changed capture. Absolute paths and `..` components are rejected. +JSON snapshot per changed capture. Rooted, UNC, drive-qualified and +drive-relative paths are rejected, as is any `..` component or an embedded NUL, +under both `/` and `\` separators, so a config is accepted or rejected +identically on every host. Production configs normally omit this key and retain only the naive executable- local addendum. Both history modes are off by default and contain game-derived bytes, so their outputs remain private/gitignored artifacts. diff --git a/recompiler/src/config_loader.cpp b/recompiler/src/config_loader.cpp index a83c6f0b..25fca050 100644 --- a/recompiler/src/config_loader.cpp +++ b/recompiler/src/config_loader.cpp @@ -229,6 +229,41 @@ static bool parse_aspect_ratio(const std::string& s, int* num, int* den) { return true; } +// Reject a config path that is not confined to the project directory. +// +// A game.toml is checked in and read on every host, so a path in it must mean +// the same thing everywhere; std::filesystem::path answers by the *host* +// grammar, so a Windows-authored escape passes on POSIX. Scan the raw bytes +// under both separator conventions instead. NUL is rejected outright because +// the runtime hands this value on as a C string — anything past a NUL would +// silently vanish between what is validated and what is used. +static void validate_project_relative(const std::string& value, + const char* field) { + if (value.find('\0') != std::string::npos) { + throw std::runtime_error( + fmt::format("{} must not contain a NUL byte", field)); + } + // Rooted ("/x", "\x"), UNC ("\\server\share"), drive-qualified ("C:/x", + // "C:\x") and drive-relative ("C:x") alike. + const bool rooted = !value.empty() && (value[0] == '/' || value[0] == '\\'); + const bool drive = value.size() >= 2 && value[1] == ':' && + std::isalpha(static_cast(value[0])); + if (rooted || drive) { + throw std::runtime_error( + fmt::format("{} must be project-relative", field)); + } + for (size_t start = 0;;) { + const size_t sep = value.find_first_of("/\\", start); + const size_t end = (sep == std::string::npos) ? value.size() : sep; + if (value.compare(start, end - start, "..") == 0) { + throw std::runtime_error( + fmt::format("{} must stay inside the project", field)); + } + if (sep == std::string::npos) break; + start = sep + 1; + } +} + // Parse the optional [runtime] block. All fields optional; absent fields // leave has_* = false on the returned struct. Paths are resolved relative // to `root` (project root). @@ -395,17 +430,8 @@ static RuntimeConfig parse_runtime_block(const toml::value& cfg, const fs::path& if (runtime.contains("overlay_capture_persist_dir")) { rt.overlay_capture_persist_dir = toml::find(runtime, "overlay_capture_persist_dir"); - const std::filesystem::path persist(rt.overlay_capture_persist_dir); - if (persist.is_absolute()) { - throw std::runtime_error( - "runtime.overlay_capture_persist_dir must be project-relative"); - } - for (const auto& component : persist) { - if (component == "..") { - throw std::runtime_error( - "runtime.overlay_capture_persist_dir must stay inside the project"); - } - } + validate_project_relative(rt.overlay_capture_persist_dir, + "runtime.overlay_capture_persist_dir"); } if (runtime.contains("turbo_loads")) { rt.turbo_loads = toml::find(runtime, "turbo_loads"); diff --git a/recompiler/src/config_loader.h b/recompiler/src/config_loader.h index 51d5d864..6c514fa5 100644 --- a/recompiler/src/config_loader.h +++ b/recompiler/src/config_loader.h @@ -235,9 +235,11 @@ struct RuntimeConfig { bool overlay_capture_history = false; // overlay_capture_persist_dir: optional DEV-only, project-relative safe - // directory for one immutable JSON file per changed snapshot. Absolute - // paths and `..` components are rejected. Production normally leaves this - // unset and retains only the addendum beside the executable. + // directory for one immutable JSON file per changed snapshot. Rooted, + // UNC, drive-qualified and drive-relative paths are rejected, as is any + // `..` component or an embedded NUL, under both `/` and `\` separators. + // Production normally leaves this unset and retains only the addendum + // beside the executable. std::string overlay_capture_persist_dir; // turbo_loads: OPT-IN per game. While the game is loading (CD data diff --git a/recompiler/tests/recompiler_patch_test.cpp b/recompiler/tests/recompiler_patch_test.cpp index 8975b6f0..b5c8d393 100644 --- a/recompiler/tests/recompiler_patch_test.cpp +++ b/recompiler/tests/recompiler_patch_test.cpp @@ -466,6 +466,60 @@ overlay_capture_persist_dir = "C:/outside" check_throws([&] { (void)PSXRecompV4::load_game_config(absolute); }, "must be project-relative", "parser rejects absolute capture history directory"); + + // A game.toml travels between hosts, so each of these must be rejected on + // every host, not only on the one whose path grammar it is written in. + // TOML literal strings ('...') are used because a backslash is an escape + // inside a TOML basic string. Cases marked POSIX-visible are the ones a + // host-native std::filesystem check silently accepted under POSIX. + struct RejectedDir { + const char* value; // as written in game.toml + const char* needle; // expected message fragment + const char* name; + }; + static constexpr RejectedDir rejected[] = { + {"'/outside'", "must be project-relative", + "parser rejects rooted capture history directory"}, + {R"('\outside')", "must be project-relative", + "parser rejects backslash-rooted capture history directory"}, + {R"('C:\outside')", "must be project-relative", + "parser rejects backslash drive-qualified capture history directory"}, + {"'C:outside'", "must be project-relative", + "parser rejects drive-relative capture history directory"}, + {R"('\\server\share\outside')", "must be project-relative", + "parser rejects UNC capture history directory"}, + {R"('..\outside')", "must stay inside the project", + "parser rejects backslash-escaping capture history directory"}, + {R"('nested\..\..\outside')", "must stay inside the project", + "parser rejects nested backslash-escaping capture history directory"}, + {"'nested/..'", "must stay inside the project", + "parser rejects a trailing .. component"}, + // A basic string, not a literal one: TOML forbids a raw NUL byte but + // allows the \u0000 escape that produces it. The runtime passes this + // value on as a C string, so a NUL would truncate it after validation. + {R"("..\u0000keep")", "must not contain a NUL byte", + "parser rejects an embedded NUL escape"}, + }; + for (size_t i = 0; i < std::size(rejected); ++i) { + const auto& c = rejected[i]; + const auto cfg_path = write_config( + root, fmt::format("capture-history-rejected-{}", i), + fmt::format("[runtime]\noverlay_capture_persist_dir = {}\n", + c.value)); + check_throws([&] { (void)PSXRecompV4::load_game_config(cfg_path); }, + c.needle, c.name); + } + + // A dotfile is not a `..` component; the guard must not over-reject. + const auto dotted = write_config(root, "capture-history-dotted", R"toml( +[runtime] +overlay_capture_history = true +overlay_capture_persist_dir = ".aot_capture_history/..keep/TEST-00000" +)toml"); + const auto dotted_cfg = PSXRecompV4::load_game_config(dotted); + check(dotted_cfg.runtime.overlay_capture_persist_dir == + ".aot_capture_history/..keep/TEST-00000", + "parser accepts a capture history component that merely starts with dots"); } void codegen_tests() {