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.
This commit is contained in:
luisg0c
2026-07-28 09:04:52 -03:00
parent f8c0f3f3b0
commit 4ced15faf4
4 changed files with 100 additions and 15 deletions
+4 -1
View File
@@ -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.
+37 -11
View File
@@ -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<unsigned char>(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<std::string>(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<bool>(runtime, "turbo_loads");
+5 -3
View File
@@ -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
@@ -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() {