Skip to content

fix(validate): keep asset paths relative when rewriting a scenario - #229

Merged
LeadcodeDev merged 1 commit into
chantier/audit-2026-09from
fix/validate-relative-paths
Sep 22, 2026
Merged

LeadcodeDev merged 1 commit into
chantier/audit-2026-09from
fix/validate-relative-paths

Conversation

@LeadcodeDev

Copy link
Copy Markdown
Owner

Severity Medium, category correctness. Location: crates/rustmotion/src/cli/commands/validate.rs:146

Impact

loaded.raw is captured AFTER rustmotion::assets::rebase_relative_paths(&mut json_value, dir) runs (crates/rustmotion/src/cli/commands/validation.rs:194-196), which rewrites every src/track string that names an existing file next to the scenario into a canonicalized ABSOLUTE path (crates/rustmotion/src/assets.rs:62-78). refuse_fix (validate.rs:81-109) only guards HTML, config/$, include, and for-each/use — nothing stops this. So rustmotion validate --fix scene.json on a scenario containing "src": "assets/logo.png" silently overwrites the source with "src": "/Users/alice/proj/assets/logo.png", which no longer resolves on any other machine or in CI, and pollutes the diff with paths the author never typed. This is the same class of unfaithful write-back that FixRefusal exists to prevent. Secondary hazard from the same line: rustmotion-studio enables serde_json's preserve_order (crates/rustmotion-studio/Cargo.toml:21) while crates/rustmotion/Cargo.toml:29 does not, so under resolver-2 feature unification a workspace build preserves key order but a standalone cargo install rustmotion build alphabetises every object in the rewritten file.

Fix

Keep an un-rebased copy of the post-expansion JSON in LoadedScenario (e.g. raw_unrebased) and serialise that in --fix; or re-read the file from disk, apply apply_fixes to those bytes, and write that back. Either way, also pin serde_json with features = ["preserve_order"] in the rustmotion crate so --fix never reorders keys.

Evidence the audit read

let mut json_value = loaded.raw.clone();
applied_fixes = apply_fixes(&mut json_value, &report_out.geom_violations);
if applied_fixes > 0 {
    let pretty = serde_json::to_string_pretty(&json_value)
        .map_err(|e| RustmotionError::Generic(format!("serialize fixes: {}", e)))?;
    std::fs::write(input, pretty)

Stacked on fix/geometry-transform-lengths, which carries the previous finding of this workstream. GitHub shows only this finding's diff; merge in order.

Part of the September 2026 audit remediation chantier. Refs #220 (RM-16).

@LeadcodeDev LeadcodeDev added the bug Something isn't working label Sep 21, 2026
@LeadcodeDev LeadcodeDev self-assigned this Sep 21, 2026
@LeadcodeDev
LeadcodeDev force-pushed the fix/geometry-transform-lengths branch from e52dc82 to f478e84 Compare September 21, 2026 23:39
@LeadcodeDev
LeadcodeDev force-pushed the fix/validate-relative-paths branch from 95e7b59 to 1ac44d6 Compare September 21, 2026 23:39
@LeadcodeDev
LeadcodeDev force-pushed the fix/geometry-transform-lengths branch from 9f00928 to 6753586 Compare September 22, 2026 06:09
@LeadcodeDev
LeadcodeDev force-pushed the fix/validate-relative-paths branch from bef57d4 to b62a1cc Compare September 22, 2026 06:09
@LeadcodeDev
LeadcodeDev force-pushed the fix/geometry-transform-lengths branch from 6753586 to de249ff Compare September 22, 2026 08:33
@LeadcodeDev
LeadcodeDev force-pushed the fix/validate-relative-paths branch from b62a1cc to d585af3 Compare September 22, 2026 08:33
@LeadcodeDev
LeadcodeDev force-pushed the fix/geometry-transform-lengths branch from de249ff to 59ba406 Compare September 22, 2026 08:43
@LeadcodeDev
LeadcodeDev force-pushed the fix/validate-relative-paths branch from d585af3 to 4984fbc Compare September 22, 2026 08:43
@LeadcodeDev
LeadcodeDev changed the base branch from fix/geometry-transform-lengths to chantier/audit-2026-09 September 22, 2026 08:52
@LeadcodeDev
LeadcodeDev force-pushed the fix/validate-relative-paths branch from 4984fbc to 5c96bc8 Compare September 22, 2026 08:58
`loaded.raw` is captured AFTER `rustmotion::assets::rebase_relative_paths(&mut json_value, dir)` runs (crates/rustmotion/src/cli/commands/validation.rs:194-196), which rewrites every `src`/`track` string that names an existing file next to the scenario into a canonicalized ABSOLUTE path (crates/rustmotion/src/assets.rs:62-78). `refuse_fix` (validate.rs:81-109) only guards HTML, `config`/`$`, `include`, and `for-each`/`use` — nothing stops this. So `rustmotion validate --fix scene.json` on a scenario containing `"src": "assets/logo.png"` silently overwrites the source with `"src": "/Users/alice/proj/assets/logo.png"`, which no longer resolves on any other machine or in CI, and pollutes the diff with paths the author never typed. This is the same class of unfaithful write-back that `FixRefusal` exists to prevent. Secondary hazard from the same line: `rustmotion-studio` enables serde_json's `preserve_order` (crates/rustmotion-studio/Cargo.toml:21) while `crates/rustmotion/Cargo.toml:29` does not, so under resolver-2 feature unification a workspace build preserves key order but a standalone `cargo install rustmotion` build alphabetises every object in the rewritten file.

Refs #220
@LeadcodeDev
LeadcodeDev force-pushed the fix/validate-relative-paths branch from 5c96bc8 to b52ca6c Compare September 22, 2026 09:04
@LeadcodeDev
LeadcodeDev merged commit 0e47992 into chantier/audit-2026-09 Sep 22, 2026
LeadcodeDev added a commit that referenced this pull request Sep 22, 2026
)

`loaded.raw` is captured AFTER `rustmotion::assets::rebase_relative_paths(&mut json_value, dir)` runs (crates/rustmotion/src/cli/commands/validation.rs:194-196), which rewrites every `src`/`track` string that names an existing file next to the scenario into a canonicalized ABSOLUTE path (crates/rustmotion/src/assets.rs:62-78). `refuse_fix` (validate.rs:81-109) only guards HTML, `config`/`$`, `include`, and `for-each`/`use` — nothing stops this. So `rustmotion validate --fix scene.json` on a scenario containing `"src": "assets/logo.png"` silently overwrites the source with `"src": "/Users/alice/proj/assets/logo.png"`, which no longer resolves on any other machine or in CI, and pollutes the diff with paths the author never typed. This is the same class of unfaithful write-back that `FixRefusal` exists to prevent. Secondary hazard from the same line: `rustmotion-studio` enables serde_json's `preserve_order` (crates/rustmotion-studio/Cargo.toml:21) while `crates/rustmotion/Cargo.toml:29` does not, so under resolver-2 feature unification a workspace build preserves key order but a standalone `cargo install rustmotion` build alphabetises every object in the rewritten file.

Refs #220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant