Skip to content

fix: security hardening, macOS aria2 lookup, aria2 stability and UI bugs - #5

Merged
mogurt merged 8 commits into
mainfrom
fix/review-issues
Sep 28, 2026
Merged

mogurt merged 8 commits into
mainfrom
fix/review-issues

Conversation

@mogurt

@mogurt mogurt commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

Summary

Fixes from a full review of the app, one commit per area:

  • Security
    • Remote filenames (URL path, Content-Disposition, Civitai metadata) are reduced to a safe basename, so they can no longer escape the target directory via ../, \, drive prefixes or reserved names.
    • create_download / check_file_exists reject unsafe names.
    • Civitai / Hugging Face hosts are matched exactly (or by subdomain) instead of by substring, so API tokens are never sent to look-alike domains such as civitai.com.evil.net.
    • Token-like query params are redacted from backend logs and forwarded aria2 output.
  • macOS
    • The bundled aria2 sidecar is looked up next to the main executable (where Tauri puts externalBin) instead of only in Resources/binaries.
    • Hardcoded \ path joins are replaced by joinPath / getModelBaseDir.
    • The Workflow page now uses the default <comfyui_root>/models dir like the other pages.
  • UI bugs
    • The Civitai model type is no longer always empty (serde rename mismatch).
    • Changing the URL no longer downloads under the previous URL's filename and folder; stale parse results are discarded.
    • Settings inputs no longer drop characters or push half-typed proxy URLs to aria2 (state updates synchronously; DB writes and aria2 sync are debounced).
  • Dependencies
    • Removed the unused react-router-dom.
    • Moved shadcn to devDependencies. npm audit now reports 0 vulnerabilities, down from 17.
  • aria2 stability
    • Startup retries with a fresh port and retries the RPC connection with backoff.
    • Websocket connections are versioned, so a stale socket can't tear down the current one and events aren't emitted twice after reconnects.
    • After an automatic restart, the app reconnects and re-applies runtime settings, and the crash counter resets after stable running.
    • Polling reconciles tasks aria2 no longer reports, so they can't get stuck in "downloading".
    • --no-conf is set, --rpc-allow-origin-all is dropped, and exit waits at most 2s for aria2.
  • Build: aria2 from pinned official release
    • The macOS binaries came from AnInsomniacy/aria2-builder, which now returns 404 and broke npm ci on macOS. That breaks all CI, including main.
    • aria2 is pinned to 1.37.0 and every download is SHA256-verified. The hashes are cross-checked against the Homebrew and Scoop package definitions.
    • macOS builds the official source using system frameworks only (AppleTLS), then checks with otool that it links nothing from Homebrew.
    • Windows uses the official GitHub release zip.
    • Removed the silent fallback that bundled a PATH aria2c (Homebrew-linked, broken on users' machines).
    • ARIA2C_PATH can override the binary, and CI caches it.
  • Release automation
    • release-please ran only on the stale dev branch; it now runs on main, with bootstrap-sha set because the v0.1.0 tag is not in main's history.
    • When release-please creates a release, it calls release.yml (now workflow_call-able) to build the installers. This is needed because tags pushed with GITHUB_TOKEN don't trigger workflows.
  • Page title changed from temp_scaffold to ComfyDownloader.

Testing

  • npm run lint (0 errors; 1 existing warning in TaskList.tsx)
  • npm run typecheck
  • cd src-tauri && cargo test: 18 passed, including new tests for filename sanitizing, host matching and URL redaction
  • cargo fmt --check, npm run build, npm audit

Manual end-to-end test on Windows (tauri dev, driven through the real UI):

  • Downloaded 4 models from Hugging Face and Civitai (24 KB to 319 MB). All were auto-routed to the right folder, and all hashes match upstream (SHA256 / git blob).
  • Pausing a 319 MB download at 125 MB and resuming continued from the partial file and completed.
  • Re-downloading an existing file was skipped under the skip strategy.
  • Switching URLs while a parse was in flight showed only the new URL's result.
  • Fast typing into settings dropped no characters, and the value persisted.
  • Killing aria2c triggered an automatic restart and reconnect with no duplicate events.
  • Traversal filenames (../../x, ..\x.dll, C:\x) were rejected via real IPC, and nothing was written outside the target dir.

… in logs

- Add safety module: sanitize remote filenames to a bare basename (blocks
  ../ and \ traversal, drive prefixes, illegal chars, reserved names);
  create_download / check_file_exists reject unsafe names.
- Match civitai.com / huggingface.co / hf-mirror.com by exact host or
  subdomain instead of substring, in both backend and frontend, so tokens
  are never sent to look-alike domains.
- Redact token-like query params from URLs in backend logs and forwarded
  aria2 output.
- Look for the aria2c sidecar next to the main executable (where Tauri
  places externalBin, e.g. Contents/MacOS) before resource_dir, and warn
  when falling back to PATH.
- Add joinPath/getModelBaseDir helpers that follow the base path's
  separator style; replace hardcoded backslash joins in TaskInput,
  Search and Settings.
- Workflow page now uses the same default `<comfyui_root>/models` base dir
  as the other pages instead of reporting every model missing.
… inputs

- Serialize CivitaiModelInfo.model_type under its own name; the
  `rename = "type"` meant the frontend always read undefined.
- TaskInput: changing the URL clears the previous filename, parse result
  and auto-selected folder; late results from an older URL are discarded;
  paste no longer triggers a duplicate parse; checkFileExists errors are
  surfaced instead of rejecting unhandled.
- Settings store updates state synchronously and debounces DB writes and
  aria2 runtime sync, so typing no longer drops characters or pushes
  half-typed proxy URLs to aria2.
…cies

react-router-dom was never imported and carried several advisories.
shadcn is a CLI whose only runtime use is its Tailwind CSS import, which
is resolved at build time, so it belongs in devDependencies; this also
removes its server-side transitive deps (hono, express, MCP SDK) from the
production tree. `npm audit` now reports 0 vulnerabilities.
Backend:
- Retry aria2 startup with a fresh port (3 attempts) and retry the RPC
  connection with backoff instead of a single attempt after fixed sleeps;
  detect aria2 exiting immediately after spawn.
- Give each websocket connection an id: tasks of a superseded socket can no
  longer clear the current connection, and old reader/writer tasks are
  aborted so notifications are not emitted twice after a reconnect.
- A timed-out request only removes its own pending entry.
- Reset the crash counter after 2 minutes of stable running; after an
  automatic restart, reconnect and re-emit `aria2://ready` so the frontend
  re-applies runtime settings.
- Pass --no-conf and drop --rpc-allow-origin-all; cap the shutdown RPC at
  2s so a hung aria2 cannot block app exit.

Frontend:
- Polling also looks up tracked gids missing from the active/waiting list,
  so completion/errors are not lost when a notification is missed; gids
  aria2 no longer knows (or `removed`) are marked paused so they can be
  resumed. Task status is re-read before writing to avoid overwriting a
  concurrent pause/cancel.
- Reset aria2Ready on `aria2://error`.
…rty feed

The macOS binaries came from AnInsomniacy/aria2-builder, which no longer
exists (404), breaking `npm ci` on macOS and therefore all CI.

- Pin aria2 1.37.0 and verify SHA256 of every download (hashes
  cross-checked against the Homebrew and Scoop package definitions).
- macOS: aria2 ships no macOS binaries, so build the official source
  tarball using only system frameworks (AppleTLS, no Homebrew libs) with
  a 10.15 deployment target, and verify via otool that the result links
  only system libraries. Drop the silent fallback to a PATH aria2c, which
  would bundle a Homebrew-linked binary that breaks on users' machines.
- Windows: use the official GitHub release zip instead of a SourceForge
  mirror resolved from "latest".
- Allow ARIA2C_PATH to supply a local binary.
- Cache src-tauri/binaries in CI keyed on the script hash.
- Update THIRD_PARTY_NOTICES and READMEs accordingly.
release-please only ran on `dev`, which is no longer used (9 commits
behind main), so no release PR was ever opened.

- Run release-please on pushes to main, targeting main.
- The existing v0.1.0 tag is not on main's history, so set bootstrap-sha
  to the current main tip; release-please ignores it after the first
  release PR, so it needs no cleanup.
- Tags created with GITHUB_TOKEN do not trigger other workflows, so make
  release.yml callable (workflow_call) and invoke it from release-please
  when a release is created; release.yml checks out the release tag.
@mogurt
mogurt merged commit 8ee33ed into main Sep 28, 2026
4 checks passed
mogurt pushed a commit that referenced this pull request Sep 28, 2026
##
[0.1.1](comfy-downloader-v0.1.0...comfy-downloader-v0.1.1)
(2026-09-28)


### Bug Fixes

* security hardening, macOS aria2 lookup, aria2 stability and UI bugs
([#5](#5))
([8ee33ed](8ee33ed))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@mogurt
mogurt deleted the fix/review-issues branch September 28, 2026 09:15
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.

1 participant