Skip to content

Harden Cutline imports and test portability - #90

Merged
MyPrototypeWhat merged 1 commit into
MiniMax-AI:mainfrom
klarkxy:cutline-follow-up-hardening
Oct 27, 2026
Merged

MyPrototypeWhat merged 1 commit into
MiniMax-AI:mainfrom
klarkxy:cutline-follow-up-hardening

Conversation

@klarkxy

@klarkxy klarkxy commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Resolve real paths before validating media extensions in single-file and reference imports, so a media-looking symlink cannot import a non-media target.
  • Keep FABLECUT_DATA_DIR mandatory, document the fail-fast behavior, and add a regression test for loading without it.
  • Skip only the 21 media-dependent tests when FFmpeg is unavailable; all other tests continue to run.
  • Make the private-redirect test portable to macOS by listening on 127.0.0.1 while retaining the 127.0.0.2 target and the no-request assertions.

Validation

  • npm --prefix plugins/klarkxy/cutline test: 188 passed, 0 failed, 2 skipped (Windows symlink permission).
  • Full suite with FFmpeg hidden from PATH: 167 passed, 0 failed, 23 skipped (21 expected FFmpeg skips plus 2 Windows symlink skips).
  • node --test plugins/klarkxy/cutline/tests/import-url-security.test.mjs: 8 passed.
  • Installed in MiniMax Code and exercised all 21 Cutline MCP tools against a synthetic project. Both editor-tab and standalone exports succeeded; the MP4 outputs were 1280x720 at 30 fps, 4.5 seconds, and passed independent full-stream decoding.
  • npm run validate: Cutline: OK; git diff --check passed.

Environment notes

  • The repository-wide npm run check validation passes, but two existing repository symlink tests fail locally with Windows EPERM; the other 67 repository tests pass.
  • macOS was not run locally; the loopback fixture no longer needs to bind 127.0.0.2.

View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need.

@MyPrototypeWhat MyPrototypeWhat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 17931d6. Thanks for the follow-up to #76. Nothing needs to change before merge. Approving.

Checks run

  • npx -y -p node@22 npm run check at the repository root passes.
  • Plugin tests (node --test tests/*.test.mjs) with FFmpeg on PATH: 190 tests, 177 pass, 0 fail, 13 skipped (browser-only tests, no Chromium on the machine).
  • Same suite with only Node and Git on PATH, from a clean fixture folder: 144 pass, 0 fail, 46 skipped. The media tests skip with a clear reason instead of failing.

Verified

  • Single-file import and fablecut_analyze_reference now resolve the path with realpath before the extension check, and single-file import copies from the resolved path. A .mp4 symlink pointing at a .txt file is refused by both, and nothing lands in media/.
  • Extension checks are lower-cased. The isFile() check rejects directories, including symlinks to directories.
  • copyToMedia re-checks the extension of the source it actually copies, so the two layers agree.
  • paths.js still throws when FABLECUT_DATA_DIR is unset. The header comment now matches that behavior, and tests/paths.test.mjs covers it.
  • The private-redirect test listens on 127.0.0.1 while the redirect still names 127.0.0.2. The request stub still proves no request is sent, so the test keeps its meaning and no longer needs a second loopback address on macOS.
  • Everything stays inside plugins/klarkxy/cutline/. Repository CI, .github/, root scripts and the root test/ folder are untouched.

Optional

  1. The new symlink test fails on a second run in the same checkout. The fixture folder under .fablecut-acceptance/ persists, so symlinkSync throws EEXIST. Removing disguised.mp4 (for example with rmSync(link, { force: true })) before creating the link would make reruns pass.
  2. The resolved path is checked and then copied in two separate steps, so a process that can already write to the source folder could swap a path component in between. That attacker already has local write access, and the copy still lands only in media/, so this is defense in depth. Opening the file once and checking fstat on the handle would close the gap if you want it.
  3. The copy is named after the link (path.basename(src)), not the target. If a .png link points at a .mp4 file, the copy keeps the .png name with video content. Both are media, so this is harmless, but naming the copy after the resolved file would keep the extension consistent with its content.

@MyPrototypeWhat
MyPrototypeWhat merged commit 3ce4d3c into MiniMax-AI:main Oct 27, 2026
1 check passed
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