Skip to content

fix: reject invalid output filenames to prevent path traversal (V-002) - #209

Open
anupamme wants to merge 2 commits into
Candlestickers:developmentfrom
anupamme:fix-repo-candlestick-v-002-ffmpeg-name-sanitization
Open

fix: reject invalid output filenames to prevent path traversal (V-002)#209
anupamme wants to merge 2 commits into
Candlestickers:developmentfrom
anupamme:fix-repo-candlestick-v-002-ffmpeg-name-sanitization

Conversation

@anupamme

@anupamme anupamme commented Sep 5, 2026

Copy link
Copy Markdown

Summary

Fixes a path-traversal issue (CWE-22) in render_video_ffmpeg
(src-tauri/src/main.rs): the user-supplied name parameter was used
to build output_path without validation, so a crafted project file
(e.g. name = "../../../Users/me/Desktop/evil") could cause the
rendered .mp4 to be written outside the intended temp render
directory.

Scope correction from the original automated report

The original scanner report labeled this CRITICAL and described
"shell metacharacter injection" affecting both name and fps. That
framing doesn't hold up:

  • Command::new("ffmpeg") in Rust does not invoke a shell, so
    there is no shell metacharacter injection here.
  • fps is only ever used as &fps.to_string() passed as a single
    Command argument; it isn't used to construct any path.

The real, demonstrable issue is narrower: name feeding
output_path allows path traversal / unintended output file
placement
(CWE-22). This PR fixes that specific issue.

Fix

The previous fix in this PR stripped invalid characters from name
before use (silently turning "../../foo" into "foo"). This revision
replaces that with an explicit allow-list validator that rejects
invalid names outright instead of transforming them, so the security
boundary is explicit and a caller isn't surprised by a silently
different filename than the one they chose.

Tests

Added #[cfg(test)] mod tests (first test in src-tauri) covering
valid names, empty names, and traversal/separator patterns. Run
locally with cd src-tauri && cargo test (no CI workflow exists yet
in this repo to run this automatically).

Files changed

  • src-tauri/src/main.rs

Originally opened as an automated security fix by OrbisAI
Security
; revised by the PR author to narrow
scope to the actual verifiable issue and strengthen the fix.

Automated security fix generated by OrbisAI Security
@InternetAstronaut

Copy link
Copy Markdown
Contributor

Even with this being a real security issue, I haven't seen any malware that uses Candlestick. It's way easier to create malware for Blender and Animate than Candlestick, and the mentioned softwares also have way more users than Candlestick.

Narrow the V-002 fix to the actual reachable issue: path traversal via
the name parameter feeding output_path in render_video_ffmpeg (CWE-22).
Replace the character-stripping sanitizer with an explicit allow-list
validator that rejects invalid names outright, so inputs like
"../../foo" fail loudly instead of being silently rewritten to "foo".
Adds unit tests for the validator.
@anupamme anupamme changed the title fix: add path validation in main.rs fix: reject invalid output filenames to prevent path traversal (V-002) Sep 7, 2026
@anupamme

anupamme commented Sep 7, 2026

Copy link
Copy Markdown
Author

@InternetAstronaut fair pushback — I've updated the PR to reflect that.

You're right that the original report overstated this: Command::new()
in Rust doesn't go through a shell, so "shell metacharacter injection"
was never accurate, and fps isn't involved at all (it's just a number
converted to a string argument, never used to build a path). I've
dropped the CRITICAL/shell-injection framing and both of those claims
from the PR description.

What's left, and worth fixing regardless of Candlestick's current
threat profile, is narrower: the name parameter was used to build the
output file path without validation, so a crafted project file could
cause the rendered video to be written outside the intended temp
directory (path traversal, CWE-22). Low severity given Candlestick's
exposure, but cheap and low-risk to fix correctly, so I tightened it up:

  • Replaced the previous character-stripping sanitizer with an explicit
    validate_output_name function that rejects invalid names (e.g.
    ../../foo) instead of silently rewriting them.
  • Added unit tests covering valid names, empty names, and traversal /
    separator patterns.

Updated title/description are pushed now. Let me know if you'd still
rather close this out instead given the low real-world impact — happy
to go either way.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants