From e9edb3b1b60cd795f03cf1e5acba7d6b3731f753 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:27:59 +0200 Subject: [PATCH 01/16] feat(plugins): secure yt-dlp and centralize CI --- .github/workflows/plugin-ci.yml | 301 ++++++++++++++++++ CHANGELOG.md | 12 + README.md | 2 +- src-tauri/Cargo.lock | 62 ++-- src-tauri/Cargo.toml | 5 +- .../adapters/driven/plugin/capabilities.rs | 70 +++- .../adapters/driven/plugin/host_functions.rs | 279 ++++------------ src-tauri/src/adapters/driven/plugin/mod.rs | 1 + .../adapters/driven/plugin/ytdlp_broker.rs | 121 +++++++ .../driven/plugin/ytdlp_broker/legacy.rs | 124 ++++++++ .../plugin/ytdlp_broker/legacy_download.rs | 143 +++++++++ .../driven/plugin/ytdlp_broker/output.rs | 79 +++++ .../driven/plugin/ytdlp_broker/platform.rs | 145 +++++++++ .../driven/plugin/ytdlp_broker/process.rs | 118 +++++++ .../driven/plugin/ytdlp_broker/request.rs | 136 ++++++++ .../driven/plugin/ytdlp_broker/selectors.rs | 120 +++++++ .../driven/plugin/ytdlp_broker/tests/audio.rs | 23 ++ .../plugin/ytdlp_broker/tests/legacy.rs | 45 +++ .../driven/plugin/ytdlp_broker/tests/mod.rs | 5 + .../plugin/ytdlp_broker/tests/platform.rs | 36 +++ .../plugin/ytdlp_broker/tests/process.rs | 58 ++++ .../plugin/ytdlp_broker/tests/request.rs | 132 ++++++++ .../driven/plugin/ytdlp_broker/validation.rs | 131 ++++++++ src-tauri/src/adapters/driving/tauri_ipc.rs | 111 ++++--- 24 files changed, 1950 insertions(+), 309 deletions(-) create mode 100644 .github/workflows/plugin-ci.yml create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy_download.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/output.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/platform.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/process.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/request.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/selectors.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/audio.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/legacy.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/mod.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/process.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs create mode 100644 src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs diff --git a/.github/workflows/plugin-ci.yml b/.github/workflows/plugin-ci.yml new file mode 100644 index 00000000..0d37c98e --- /dev/null +++ b/.github/workflows/plugin-ci.yml @@ -0,0 +1,301 @@ +name: Reusable Plugin CI + +on: + workflow_call: + +permissions: {} + +concurrency: + group: plugin-ci-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + +env: + CARGO_TERM_COLOR: always + RUST_BACKTRACE: "1" + +jobs: + build-and-test: + name: Build, test, and package + runs-on: ubuntu-24.04 + timeout-minutes: 45 + permissions: + contents: read + + steps: + - name: Check out plugin repository + uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + + - name: Activate repository Rust toolchain + shell: bash + run: | + test -f rust-toolchain.toml + rustc --version --verbose + cargo --version + rustup component list --installed | grep -q '^clippy-' + rustup component list --installed | grep -q '^rustfmt-' + rustup component list --installed | grep -q '^llvm-tools-' + rustup target list --installed | grep -qx 'wasm32-wasip1' + + - name: Cache Cargo build data + uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2 + + - name: Verify plugin manifests + shell: bash + run: | + python3 - <<'PY' + import os + import re + import tomllib + + numeric = r"(?:0|[1-9][0-9]*)" + prerelease = ( + r"(?:0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*)" + r"(?:\.(?:0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*))*" + ) + build = r"[0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*" + semver = re.compile( + rf"{numeric}\.{numeric}\.{numeric}" + rf"(?:-{prerelease})?(?:\+{build})?" + ) + + with open("Cargo.toml", "rb") as file: + cargo = tomllib.load(file) + with open("plugin.toml", "rb") as file: + manifest = tomllib.load(file)["plugin"] + + package = cargo["package"] + if package["name"] != manifest["name"]: + raise SystemExit("Cargo.toml and plugin.toml names differ") + if package["version"] != manifest["version"]: + raise SystemExit("Cargo.toml and plugin.toml versions differ") + for field, version in ( + ("version", manifest["version"]), + ("min_vortex_version", manifest["min_vortex_version"]), + ): + if not isinstance(version, str) or semver.fullmatch(version) is None: + raise SystemExit(f"plugin.{field} is not valid SemVer 2.0.0") + + package_name = package["name"] + crate_name = cargo.get("lib", {}).get("name", package_name.replace("-", "_")) + if re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", crate_name) is None: + raise SystemExit("library name cannot be mapped to a WASM artifact") + + artifact_name = f"{crate_name}.wasm" + wasm_path = f"target/wasm32-wasip1/release/{artifact_name}" + with open(os.environ["GITHUB_ENV"], "a", encoding="utf-8") as env: + env.write(f"PACKAGE_NAME={package_name}\n") + env.write(f"ARTIFACT_NAME={artifact_name}\n") + env.write(f"WASM_PATH={wasm_path}\n") + PY + + - name: Check formatting + run: cargo fmt --all -- --check + + - name: Lint native targets + run: cargo clippy --locked --all-targets --target x86_64-unknown-linux-gnu -- -D warnings + + - name: Lint WASM library + run: cargo clippy --locked --lib --target wasm32-wasip1 -- -D warnings + + - name: Build release WASM + run: cargo build --locked --lib --target wasm32-wasip1 --release + + - name: Verify WASM artifact and ABI exports + shell: bash + run: | + test -s "${WASM_PATH}" + + llvm_nm="$(find "$(rustc --print sysroot)" -type f -name llvm-nm -print -quit)" + test -x "${llvm_nm}" + symbols="$("${llvm_nm}" --defined-only "${WASM_PATH}" | awk '{print $3}')" + + case "${PACKAGE_NAME}" in + vortex-mod-containers) + required="can_decrypt detect decrypt" + ;; + vortex-mod-gallery) + required="can_handle supports_playlist extract_links extract_generic is_http_url" + ;; + vortex-mod-soundcloud) + required="can_handle supports_playlist extract_links extract_playlist extract_track resolve_stream_url download_to_file" + ;; + vortex-mod-vimeo) + required="can_handle supports_playlist extract_links get_media_variants resolve_stream_url download_to_file extract_playlist" + ;; + vortex-mod-youtube) + required="can_handle supports_playlist extract_links get_media_variants extract_playlist resolve_stream_url download_to_file" + ;; + *) + required="can_handle supports_playlist extract_links resolve_stream_url" + ;; + esac + + for export in ${required}; do + grep -qx "${export}" <<<"${symbols}" || { + echo "missing required WASM export: ${export}" >&2 + exit 1 + } + done + + - name: Run native tests including ABI smoke + run: cargo test --locked --all-targets --target x86_64-unknown-linux-gnu + + - name: Build verified release bundle + shell: bash + run: | + mkdir -p dist + cp "${WASM_PATH}" "dist/${ARTIFACT_NAME}" + cp "${WASM_PATH}" dist/plugin.wasm + cp plugin.toml dist/plugin.toml + ( + cd dist + sha256sum plugin.wasm plugin.toml > SHA256SUMS + sha256sum --check SHA256SUMS + ) + + - name: Upload verified release bundle + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: plugin-release-${{ github.sha }} + path: dist/ + if-no-files-found: error + retention-days: 14 + + release: + name: Publish immutable GitHub release + if: startsWith(github.ref, 'refs/tags/v') + needs: build-and-test + runs-on: ubuntu-24.04 + timeout-minutes: 10 + permissions: + contents: write + + steps: + - name: Download verified bundle + uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 + with: + name: plugin-release-${{ github.sha }} + path: dist + + - name: Verify release metadata + env: + RELEASE_TAG: ${{ github.ref_name }} + shell: bash + run: | + ( + cd dist + sha256sum --check SHA256SUMS + ) + python3 - <<'PY' + import os + import tomllib + + with open("dist/plugin.toml", "rb") as file: + manifest = tomllib.load(file)["plugin"] + + tag = os.environ["RELEASE_TAG"] + if tag != f"v{manifest['version']}": + raise SystemExit( + f"tag {tag!r} does not match plugin version {manifest['version']!r}" + ) + PY + + - name: Create or verify immutable release + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + RELEASE_TAG: ${{ github.ref_name }} + shell: bash + run: | + if ! gh release view "${RELEASE_TAG}" >/dev/null 2>&1; then + gh release create "${RELEASE_TAG}" dist/* \ + --verify-tag \ + --generate-notes \ + --title "${RELEASE_TAG}" + fi + + expected_assets="$(find dist -maxdepth 1 -type f -printf '%f\n' | LC_ALL=C sort)" + actual_assets="$(gh release view "${RELEASE_TAG}" \ + --json assets --jq '.assets[].name' | LC_ALL=C sort)" + if [[ "${actual_assets}" != "${expected_assets}" ]]; then + echo "release asset set differs from the verified bundle" >&2 + diff -u <(printf '%s\n' "${expected_assets}") \ + <(printf '%s\n' "${actual_assets}") || true + exit 1 + fi + + published="$(mktemp -d)" + gh release download "${RELEASE_TAG}" --dir "${published}" + while IFS= read -r asset; do + if ! cmp --silent "dist/${asset}" "${published}/${asset}"; then + echo "release asset content differs: ${asset}" >&2 + exit 1 + fi + done <<<"${expected_assets}" + + registry-consistency: + name: Check central registry after release + if: startsWith(github.ref, 'refs/tags/v') + needs: release + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: + contents: read + + steps: + - name: Download released bundle + uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 + with: + name: plugin-release-${{ github.sha }} + path: dist + + - name: Compare release with central registry + id: registry-check + continue-on-error: true + shell: bash + run: | + registry="${RUNNER_TEMP}/registry.toml" + curl --fail --silent --show-error --location \ + --proto '=https' --tlsv1.2 \ + https://raw.githubusercontent.com/mpiton/vortex/main/registry/registry.toml \ + --output "${registry}" + REGISTRY_PATH="${registry}" python3 - <<'PY' + import hashlib + import os + import pathlib + import tomllib + + dist = pathlib.Path("dist") + with (dist / "plugin.toml").open("rb") as file: + manifest = tomllib.load(file)["plugin"] + with open(os.environ["REGISTRY_PATH"], "rb") as file: + entries = tomllib.load(file)["plugin"] + + entry = next( + (item for item in entries if item["name"] == manifest["name"]), + None, + ) + if entry is None: + raise SystemExit("plugin is missing from the central registry") + for key in ("version", "min_vortex_version"): + if entry[key] != manifest[key]: + raise SystemExit(f"registry {key} differs from plugin.toml") + + wasm_sha = hashlib.sha256((dist / "plugin.wasm").read_bytes()).hexdigest() + manifest_sha = hashlib.sha256((dist / "plugin.toml").read_bytes()).hexdigest() + if entry["checksum_sha256"] != wasm_sha: + raise SystemExit("registry WASM checksum differs from release artifact") + if entry["checksum_sha256_toml"] != manifest_sha: + raise SystemExit("registry manifest checksum differs from plugin.toml") + PY + + - name: Report registry update requirement + if: steps.registry-check.outcome == 'failure' + shell: bash + run: | + echo "::warning title=Registry update required::The release is published, but vortex/registry/registry.toml does not match its assets." + { + echo "### Registry update required" + echo + echo "The release is published and immutable. Update \`vortex/registry/registry.toml\` with its version and checksums." + } >> "${GITHUB_STEP_SUMMARY}" diff --git a/CHANGELOG.md b/CHANGELOG.md index a6fc14d2..5cc623bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Security + +- **Typed yt-dlp broker (MAT-131)**: replaced the plugin-facing generic + `run_subprocess(binary, args, timeout)` capability with a closed + `run_ytdlp` contract. Vortex now selects an approved executable, builds all + arguments, ignores external yt-dlp configuration/plugins/remote components, + uses fixed timeouts and a cleared environment, confines download output to + the Vortex temporary directory, and caps stdout/stderr. A strict legacy shim + keeps the exact profiles of already-published official plugins loadable + during the migration; arbitrary subprocess capabilities are no longer + registered. The Tauri metadata fallback uses the same broker. + ### Fixed - **Lot 0 maintenance baseline (MAT-126–MAT-128)**: restored CI by allowing non-secret `.npmrc` configuration through the shared redacted scanner, updated compatible npm and Rust dependencies to clear security audits, pinned CI actions on their Node 24 releases, kept the codebase clean under the current Rust stable Clippy, and added multi-OS Tauri build diagnostics. diff --git a/README.md b/README.md index 22cda1a2..08149287 100644 --- a/README.md +++ b/README.md @@ -106,7 +106,7 @@ A Nix flake is provided for a reproducible toolchain (`nix develop`). Plugins are independent crates compiled to `wasm32-wasip1`. The four official plugins live in sibling repos: -- [`vortex-mod-youtube`](https://github.com/mpiton/vortex-mod-youtube) — YouTube + yt-dlp subprocess integration +- [`vortex-mod-youtube`](https://github.com/mpiton/vortex-mod-youtube) — YouTube via the host's typed yt-dlp broker - [`vortex-mod-vimeo`](https://github.com/mpiton/vortex-mod-vimeo) — Vimeo crawler - [`vortex-mod-soundcloud`](https://github.com/mpiton/vortex-mod-soundcloud) — SoundCloud crawler - [`vortex-mod-gallery`](https://github.com/mpiton/vortex-mod-gallery) — generic image gallery extractor diff --git a/src-tauri/Cargo.lock b/src-tauri/Cargo.lock index a176f142..cd4aa2e8 100644 --- a/src-tauri/Cargo.lock +++ b/src-tauri/Cargo.lock @@ -1135,7 +1135,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "117725a109d387c937a1533ce01b450cbde6b88abceea8473c4d7a85853cda3c" dependencies = [ "lazy_static", - "windows-sys 0.59.0", + "windows-sys 0.48.0", ] [[package]] @@ -1148,6 +1148,16 @@ dependencies = [ "memchr", ] +[[package]] +name = "command-group" +version = "5.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a68fa787550392a9d58f44c21a3022cfb3ea3e2458b7f85d3b399d0ceeccf409" +dependencies = [ + "nix 0.27.1", + "winapi", +] + [[package]] name = "concurrent-queue" version = "2.5.0" @@ -1815,7 +1825,7 @@ dependencies = [ "libc", "option-ext", "redox_users 0.5.2", - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -2064,7 +2074,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -2107,8 +2117,9 @@ dependencies = [ [[package]] name = "extism" -version = "0.0.0+replaced-by-ci" -source = "git+https://github.com/mpiton/extism.git?rev=f14e56ddf0f994f04f363545d3d01880b7876f82#f14e56ddf0f994f04f363545d3d01880b7876f82" +version = "1.30.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4b66cd9ac5c64b49c9bac69db3d1b10d8f9386e7caab73489c2f197ba43d5e05" dependencies = [ "anyhow", "async-trait", @@ -2133,8 +2144,9 @@ dependencies = [ [[package]] name = "extism-convert" -version = "0.0.0+replaced-by-ci" -source = "git+https://github.com/mpiton/extism.git?rev=f14e56ddf0f994f04f363545d3d01880b7876f82#f14e56ddf0f994f04f363545d3d01880b7876f82" +version = "1.30.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ad19858c4c462309a8f3a20abec53e8603bda1eefda26c8bfab51d5516b40cbb" dependencies = [ "anyhow", "base64 0.22.1", @@ -2148,8 +2160,9 @@ dependencies = [ [[package]] name = "extism-convert-macros" -version = "0.0.0+replaced-by-ci" -source = "git+https://github.com/mpiton/extism.git?rev=f14e56ddf0f994f04f363545d3d01880b7876f82#f14e56ddf0f994f04f363545d3d01880b7876f82" +version = "1.30.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5f2932799f6d9f9646f97b65287f6bb2addc75a0ee61e40fb24559a7540dd928" dependencies = [ "manyhow", "proc-macro-crate 3.5.0", @@ -2160,8 +2173,9 @@ dependencies = [ [[package]] name = "extism-manifest" -version = "0.0.0+replaced-by-ci" -source = "git+https://github.com/mpiton/extism.git?rev=f14e56ddf0f994f04f363545d3d01880b7876f82#f14e56ddf0f994f04f363545d3d01880b7876f82" +version = "1.30.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e2f59c8dadb5e0bde9a48c6ed45312e6ef625cbcd5f67c28459dbc8fe8bc0383" dependencies = [ "base64 0.22.1", "serde", @@ -3435,7 +3449,7 @@ checksum = "3640c1c38b8e4e43584d8df18be5fc6b0aa314ce6ebf51b53313d4306cca8e46" dependencies = [ "hermit-abi", "libc", - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -4096,6 +4110,17 @@ version = "1.0.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "650eef8c711430f1a879fdd01d4745a7deea475becfb90269c06775983bbf086" +[[package]] +name = "nix" +version = "0.27.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2eb04e9c688eff1c89d72b407f168cf79bb9e867a9d3323ed6c01519eb9cc053" +dependencies = [ + "bitflags 2.11.0", + "cfg-if", + "libc", +] + [[package]] name = "nix" version = "0.29.0" @@ -4183,7 +4208,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -4274,7 +4299,7 @@ version = "0.7.6" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "680998035259dcfcafe653688bf2aa6d3e2dc05e98be6ab46afb089dc84f1df8" dependencies = [ - "proc-macro-crate 3.5.0", + "proc-macro-crate 1.3.1", "proc-macro2", "quote", "syn 2.0.117", @@ -5207,7 +5232,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "27c6023962132f4b30eb4c172c91ce92d933da334c59c23cddee82358ddafb0b" dependencies = [ "anyhow", - "itertools 0.14.0", + "itertools 0.10.5", "proc-macro2", "quote", "syn 2.0.117", @@ -5863,7 +5888,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.12.1", - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -5932,7 +5957,7 @@ dependencies = [ "security-framework", "security-framework-sys", "webpki-root-certs", - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -7554,7 +7579,7 @@ dependencies = [ "getrandom 0.4.2", "once_cell", "rustix 1.1.4", - "windows-sys 0.59.0", + "windows-sys 0.60.2", ] [[package]] @@ -8342,6 +8367,7 @@ dependencies = [ "bytes", "bzip2", "codspeed-criterion-compat", + "command-group", "dashmap", "digest 0.11.2", "dirs", diff --git a/src-tauri/Cargo.toml b/src-tauri/Cargo.toml index 1e910982..84438f9c 100644 --- a/src-tauri/Cargo.toml +++ b/src-tauri/Cargo.toml @@ -30,9 +30,8 @@ tokio-util = { version = "0.7.18", features = ["rt"] } futures-util = "0.3.32" bytes = "1.11.1" bincode = "2" -# Temporary: direct git dep until extism upstream ships with wasmtime >=43. -# Clears GHSA-jhxm-h53p-jm7w (aarch64 Cranelift) and GHSA-xx5w-cvp6-jv83 (Winch). -extism = { git = "https://github.com/mpiton/extism.git", rev = "f14e56ddf0f994f04f363545d3d01880b7876f82" } +command-group = "=5.0.1" +extism = "=1.30.0" notify = "8.2.0" toml = "1.1.2" dashmap = "6.1.0" diff --git a/src-tauri/src/adapters/driven/plugin/capabilities.rs b/src-tauri/src/adapters/driven/plugin/capabilities.rs index 7823c085..96455cd7 100644 --- a/src-tauri/src/adapters/driven/plugin/capabilities.rs +++ b/src-tauri/src/adapters/driven/plugin/capabilities.rs @@ -97,7 +97,7 @@ pub struct PluginHostContext { /// Build host functions based on manifest capabilities. /// /// Always registers the six base functions (log, get/set config, get/set state, get credential). -/// Conditionally registers http_request and run_subprocess based on declared capabilities. +/// Conditionally registers HTTP and the constrained yt-dlp broker based on capabilities. pub fn build_host_functions( manifest: &PluginManifest, shared: &Arc, @@ -131,6 +131,15 @@ pub fn build_host_functions( } shared.plugin_states.entry(name.clone()).or_default(); + let declares_ytdlp = manifest.has_capability("subprocess:yt-dlp"); + let supports_ytdlp = super::ytdlp_broker::supports_plugin(&name); + if declares_ytdlp && !supports_ytdlp { + tracing::warn!( + plugin = %name, + "ignoring yt-dlp capability for an unapproved plugin" + ); + } + let ctx = PluginHostContext { plugin_name: name, capabilities: manifest.capabilities().to_vec(), @@ -153,12 +162,11 @@ pub fn build_host_functions( )); } - if manifest - .capabilities() - .iter() - .any(|c| c.starts_with("subprocess:")) - { - functions.push(super::host_functions::make_run_subprocess_function( + if declares_ytdlp && supports_ytdlp { + functions.push(super::host_functions::make_run_ytdlp_function( + user_data.clone(), + )); + functions.push(super::host_functions::make_legacy_run_subprocess_function( user_data.clone(), )); } @@ -172,8 +180,12 @@ mod tests { use crate::domain::model::plugin::{PluginCategory, PluginInfo, PluginManifest}; fn make_manifest_with_caps(caps: Vec<&str>) -> PluginManifest { + make_named_manifest_with_caps("test-plugin", caps) + } + + fn make_named_manifest_with_caps(name: &str, caps: Vec<&str>) -> PluginManifest { let info = PluginInfo::new( - "test-plugin".to_string(), + name.to_string(), "1.0.0".to_string(), "Test plugin".to_string(), "tester".to_string(), @@ -185,13 +197,17 @@ mod tests { #[test] fn test_build_host_functions_all_capabilities() { let shared = Arc::new(SharedHostResources::new()); - let manifest = - make_manifest_with_caps(vec!["http", "subprocess:ffmpeg", "subprocess:yt-dlp"]); + let manifest = make_named_manifest_with_caps( + "vortex-mod-youtube", + vec!["http", "subprocess:ffmpeg", "subprocess:yt-dlp"], + ); let functions = build_host_functions(&manifest, &shared); - // 6 base + http + subprocess = 8 - assert_eq!(functions.len(), 8); + // 6 base + http + typed yt-dlp + legacy compatibility = 9 + assert_eq!(functions.len(), 9); + assert!(functions.iter().any(|f| f.name() == "run_ytdlp")); + assert!(functions.iter().any(|f| f.name() == "run_subprocess")); } #[test] @@ -286,17 +302,41 @@ mod tests { } #[test] - fn test_build_host_functions_subprocess_only() { + fn test_build_host_functions_rejects_generic_subprocess_capability() { let shared = Arc::new(SharedHostResources::new()); let manifest = make_manifest_with_caps(vec!["subprocess:ffmpeg"]); let functions = build_host_functions(&manifest, &shared); - // 6 base + run_subprocess = 7 - assert_eq!(functions.len(), 7); + assert_eq!(functions.len(), 6); + assert!(!functions.iter().any(|f| f.name() == "run_ytdlp")); + assert!(!functions.iter().any(|f| f.name() == "run_subprocess")); + } + + #[test] + fn test_build_host_functions_ytdlp_only() { + let shared = Arc::new(SharedHostResources::new()); + let manifest = + make_named_manifest_with_caps("vortex-mod-youtube", vec!["subprocess:yt-dlp"]); + + let functions = build_host_functions(&manifest, &shared); + + assert_eq!(functions.len(), 8); + assert!(functions.iter().any(|f| f.name() == "run_ytdlp")); assert!(functions.iter().any(|f| f.name() == "run_subprocess")); } + #[test] + fn test_unapproved_plugin_cannot_register_ytdlp() { + let shared = Arc::new(SharedHostResources::new()); + let manifest = make_manifest_with_caps(vec!["subprocess:yt-dlp"]); + + let functions = build_host_functions(&manifest, &shared); + + assert_eq!(functions.len(), 6); + assert!(!functions.iter().any(|f| f.name() == "run_ytdlp")); + } + #[test] fn test_get_credential_returns_credential() { use crate::domain::model::credential::Credential; diff --git a/src-tauri/src/adapters/driven/plugin/host_functions.rs b/src-tauri/src/adapters/driven/plugin/host_functions.rs index a4c0f8d1..e68eea95 100644 --- a/src-tauri/src/adapters/driven/plugin/host_functions.rs +++ b/src-tauri/src/adapters/driven/plugin/host_functions.rs @@ -3,12 +3,13 @@ use std::collections::HashMap; use std::io::Read; use std::net::{IpAddr, SocketAddr, ToSocketAddrs}; -use std::process::Child; -use std::time::{Duration, Instant}; use serde::{Deserialize, Serialize}; use super::capabilities::PluginHostContext; +use super::ytdlp_broker::{ + LegacySubprocessRequest, PluginYtDlpRequest, run_legacy_request, run_plugin_request, +}; // ── JSON types ──────────────────────────────────────────────────────────────── @@ -34,21 +35,6 @@ struct LogRequest { message: String, } -#[derive(Deserialize)] -struct SubprocessRequest { - binary: String, - #[serde(default)] - args: Vec, - timeout_ms: Option, -} - -#[derive(Serialize)] -struct SubprocessResponse { - exit_code: i32, - stdout: String, - stderr: String, -} - #[derive(Deserialize)] struct ConfigEntry { key: String, @@ -61,14 +47,7 @@ struct CredentialResponse { password: String, } -struct CapturedOutput { - bytes: Vec, - truncated: bool, -} - const MAX_HTTP_BODY_BYTES: u64 = 100 * 1024 * 1024; -const MAX_SUBPROCESS_OUTPUT_BYTES: usize = 1024 * 1024; -const SUBPROCESS_POLL_INTERVAL: Duration = Duration::from_millis(25); // ── Helpers ─────────────────────────────────────────────────────────────────── @@ -191,21 +170,6 @@ fn is_link_local(ip: &IpAddr) -> bool { } } -/// Validate subprocess binary name has no path components. -fn validate_binary_name(binary: &str) -> Result<(), extism::Error> { - if binary.is_empty() - || binary.contains('/') - || binary.contains('\\') - || binary.contains("..") - || binary.contains('\0') - { - return Err(anyhow::anyhow!( - "run_subprocess: invalid binary name '{binary}'" - )); - } - Ok(()) -} - fn read_http_body_capped( response: &mut reqwest::blocking::Response, ) -> Result, extism::Error> { @@ -224,101 +188,6 @@ fn read_http_body_capped( Ok(body_bytes) } -fn read_stream_capped(mut reader: R, max_bytes: usize) -> std::io::Result { - let mut bytes = Vec::new(); - let mut truncated = false; - let mut chunk = [0_u8; 8192]; - - loop { - let read = reader.read(&mut chunk)?; - if read == 0 { - break; - } - - let remaining = max_bytes.saturating_sub(bytes.len()); - let to_copy = remaining.min(read); - if to_copy > 0 { - bytes.extend_from_slice(&chunk[..to_copy]); - } - if to_copy < read { - truncated = true; - break; - } - } - - Ok(CapturedOutput { bytes, truncated }) -} - -fn spawn_output_reader( - stream: Option, -) -> std::thread::JoinHandle> -where - T: Read + Send + 'static, -{ - std::thread::spawn(move || match stream { - Some(stream) => read_stream_capped(stream, MAX_SUBPROCESS_OUTPUT_BYTES), - None => Ok(CapturedOutput { - bytes: Vec::new(), - truncated: false, - }), - }) -} - -fn decode_captured_output(output: CapturedOutput) -> String { - let mut text = String::from_utf8_lossy(&output.bytes).into_owned(); - if output.truncated { - text.push_str("\n[truncated]"); - } - text -} - -fn collect_subprocess_output( - stdout_handle: std::thread::JoinHandle>, - stderr_handle: std::thread::JoinHandle>, -) -> Result<(String, String), extism::Error> { - let stdout = stdout_handle - .join() - .map_err(|_| anyhow::anyhow!("run_subprocess: stdout reader thread panicked"))? - .map_err(|e| anyhow::anyhow!("run_subprocess: failed to read stdout: {e}"))?; - let stderr = stderr_handle - .join() - .map_err(|_| anyhow::anyhow!("run_subprocess: stderr reader thread panicked"))? - .map_err(|e| anyhow::anyhow!("run_subprocess: failed to read stderr: {e}"))?; - - Ok(( - decode_captured_output(stdout), - decode_captured_output(stderr), - )) -} - -fn wait_for_child_with_timeout( - child: &mut Child, - timeout: Duration, -) -> Result<(std::process::ExitStatus, bool), extism::Error> { - let started_at = Instant::now(); - - loop { - if let Some(status) = child - .try_wait() - .map_err(|e| anyhow::anyhow!("run_subprocess: failed to poll child status: {e}"))? - { - return Ok((status, false)); - } - - if started_at.elapsed() >= timeout { - child.kill().map_err(|e| { - anyhow::anyhow!("run_subprocess: failed to kill timed out process: {e}") - })?; - let status = child.wait().map_err(|e| { - anyhow::anyhow!("run_subprocess: failed to reap timed out process: {e}") - })?; - return Ok((status, true)); - } - - std::thread::sleep(SUBPROCESS_POLL_INTERVAL); - } -} - // ── Host functions ──────────────────────────────────────────────────────────── /// Route plugin log messages through the `tracing` framework. @@ -595,8 +464,44 @@ pub fn make_get_credential_function( ) } -/// Run a subprocess binary declared in the plugin's `subprocess:` capabilities. -pub fn make_run_subprocess_function( +/// Run an approved yt-dlp operation built entirely by the host. +pub fn make_run_ytdlp_function(user_data: extism::UserData) -> extism::Function { + extism::Function::new( + "run_ytdlp", + [extism::ValType::I64], + [extism::ValType::I64], + user_data, + |plugin, inputs, outputs, ud| { + let input = read_input_string(plugin, inputs)?; + let request: PluginYtDlpRequest = serde_json::from_str(&input) + .map_err(|e| anyhow::anyhow!("run_ytdlp: invalid JSON: {e}"))?; + let plugin_name = { + let guard = ud.get()?; + let ctx = guard + .lock() + .map_err(|_| anyhow::anyhow!("run_ytdlp: mutex poisoned"))?; + if !ctx + .capabilities + .iter() + .any(|cap| cap == "subprocess:yt-dlp") + { + return Err(anyhow::anyhow!("run_ytdlp: capability is not declared")); + } + ctx.plugin_name.clone() + }; + let response = run_plugin_request(&plugin_name, request)?; + let json = serde_json::to_string(&response) + .map_err(|e| anyhow::anyhow!("run_ytdlp: failed to serialize response: {e}"))?; + write_output_string(plugin, outputs, &json) + }, + ) +} + +/// Compatibility shim for already-published plugins using the former ABI. +/// +/// The broker accepts only the exact historical yt-dlp profiles of official +/// plugins and rebuilds them with the same host-side controls as `run_ytdlp`. +pub fn make_legacy_run_subprocess_function( user_data: extism::UserData, ) -> extism::Function { extism::Function::new( @@ -606,63 +511,28 @@ pub fn make_run_subprocess_function( user_data, |plugin, inputs, outputs, ud| { let input = read_input_string(plugin, inputs)?; - let req: SubprocessRequest = serde_json::from_str(&input) - .map_err(|e| anyhow::anyhow!("run_subprocess: invalid JSON: {e}"))?; - - // F4: Validate binary name has no path components - validate_binary_name(&req.binary)?; - - // F6: Minimize mutex scope — check capability then release - { + let request: LegacySubprocessRequest = serde_json::from_str(&input) + .map_err(|e| anyhow::anyhow!("run_subprocess compatibility: invalid JSON: {e}"))?; + let plugin_name = { let guard = ud.get()?; let ctx = guard .lock() - .map_err(|_| anyhow::anyhow!("run_subprocess: mutex poisoned"))?; - - let required_cap = format!("subprocess:{}", req.binary); - if !ctx.capabilities.iter().any(|c| c == &required_cap) { + .map_err(|_| anyhow::anyhow!("run_subprocess compatibility: mutex poisoned"))?; + if !ctx + .capabilities + .iter() + .any(|cap| cap == "subprocess:yt-dlp") + { return Err(anyhow::anyhow!( - "run_subprocess: binary '{}' not listed in plugin capabilities", - req.binary + "run_subprocess compatibility: capability is not declared" )); } - } // Mutex released here — subprocess runs without holding the lock - - let timeout = std::time::Duration::from_millis(req.timeout_ms.unwrap_or(60_000)); - let binary = req.binary.clone(); - - let mut child = std::process::Command::new(&req.binary) - .args(&req.args) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - .map_err(|e| { - anyhow::anyhow!("run_subprocess: failed to spawn '{}': {e}", req.binary) - })?; - - let stdout_handle = spawn_output_reader(child.stdout.take()); - let stderr_handle = spawn_output_reader(child.stderr.take()); - - let (status, timed_out) = wait_for_child_with_timeout(&mut child, timeout)?; - if timed_out { - return Err(anyhow::anyhow!( - "run_subprocess: '{}' timed out after {}ms", - binary, - timeout.as_millis() - )); - } - - let (stdout, stderr) = collect_subprocess_output(stdout_handle, stderr_handle)?; - - let resp = SubprocessResponse { - exit_code: status.code().unwrap_or(-1), - stdout, - stderr, + ctx.plugin_name.clone() }; - let json = serde_json::to_string(&resp).map_err(|e| { - anyhow::anyhow!("run_subprocess: failed to serialize response: {e}") + let response = run_legacy_request(&plugin_name, request)?; + let json = serde_json::to_string(&response).map_err(|e| { + anyhow::anyhow!("run_subprocess compatibility: failed to serialize response: {e}") })?; - write_output_string(plugin, outputs, &json) }, ) @@ -696,15 +566,6 @@ mod tests { assert!(req.body.is_none()); } - #[test] - fn test_subprocess_request_deserialization() { - let json = r#"{"binary":"yt-dlp","args":["--no-playlist","https://example.com"],"timeout_ms":5000}"#; - let req: SubprocessRequest = serde_json::from_str(json).unwrap(); - assert_eq!(req.binary, "yt-dlp"); - assert_eq!(req.args, vec!["--no-playlist", "https://example.com"]); - assert_eq!(req.timeout_ms, Some(5000)); - } - #[test] fn test_get_set_config_round_trip() { let shared = std::sync::Arc::new(SharedHostResources::new()); @@ -745,20 +606,6 @@ mod tests { assert_eq!(value, "abc123"); } - #[test] - fn test_subprocess_binary_validation() { - let ctx_caps: Vec = vec!["subprocess:ffmpeg".to_string()]; - let required = "yt-dlp"; - let cap_key = format!("subprocess:{required}"); - assert!(!ctx_caps.iter().any(|c| c == &cap_key)); - - let ctx_caps2: Vec = vec![ - "subprocess:ffmpeg".to_string(), - "subprocess:yt-dlp".to_string(), - ]; - assert!(ctx_caps2.iter().any(|c| c == &cap_key)); - } - #[test] fn test_ipv4_mapped_ipv6_is_forbidden() { let loopback = "::ffff:127.0.0.1".parse::().unwrap(); @@ -769,20 +616,4 @@ mod tests { assert!(is_forbidden_ip(&private)); assert!(is_forbidden_ip(&link_local)); } - - #[test] - fn test_read_stream_capped_only_marks_truncated_when_extra_bytes_exist() { - let exact = vec![b'a'; MAX_SUBPROCESS_OUTPUT_BYTES]; - let exact_output = - read_stream_capped(std::io::Cursor::new(exact), MAX_SUBPROCESS_OUTPUT_BYTES).unwrap(); - assert!(!exact_output.truncated); - assert_eq!(exact_output.bytes.len(), MAX_SUBPROCESS_OUTPUT_BYTES); - - let too_long = vec![b'a'; MAX_SUBPROCESS_OUTPUT_BYTES + 1]; - let too_long_output = - read_stream_capped(std::io::Cursor::new(too_long), MAX_SUBPROCESS_OUTPUT_BYTES) - .unwrap(); - assert!(too_long_output.truncated); - assert_eq!(too_long_output.bytes.len(), MAX_SUBPROCESS_OUTPUT_BYTES); - } } diff --git a/src-tauri/src/adapters/driven/plugin/mod.rs b/src-tauri/src/adapters/driven/plugin/mod.rs index 523357b1..1f7d65e9 100644 --- a/src-tauri/src/adapters/driven/plugin/mod.rs +++ b/src-tauri/src/adapters/driven/plugin/mod.rs @@ -6,6 +6,7 @@ pub mod host_functions; pub mod manifest; pub mod registry; pub mod watcher; +pub(crate) mod ytdlp_broker; pub use extism_loader::ExtismPluginLoader; pub use github_store_client::GithubStoreClient; diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker.rs new file mode 100644 index 00000000..9c974e60 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker.rs @@ -0,0 +1,121 @@ +//! Constrained yt-dlp broker for official media plugins. + +mod legacy; +mod legacy_download; +mod output; +mod platform; +mod process; +mod request; +mod selectors; +mod validation; + +#[cfg(test)] +mod tests; + +use std::path::PathBuf; +use std::time::Duration; + +use anyhow::bail; +use serde::{Deserialize, Serialize}; + +pub(super) const DEFAULT_TIMEOUT: Duration = Duration::from_secs(60); +pub(super) const DOWNLOAD_TIMEOUT: Duration = Duration::from_secs(30 * 60); + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum YtDlpProvider { + Youtube, + Vimeo, + Soundcloud, + Generic, +} + +#[derive(Debug, Deserialize)] +#[serde(tag = "action", rename_all = "snake_case", deny_unknown_fields)] +pub(crate) enum PluginYtDlpRequest { + Metadata { + url: String, + #[serde(default)] + playlist: bool, + }, + Resolve { + url: String, + quality: Option, + format: Option, + #[serde(default)] + audio_only: bool, + }, + Download { + url: String, + quality: Option, + format: Option, + output_dir: String, + #[serde(default)] + audio_only: bool, + }, +} + +#[derive(Debug, Deserialize)] +pub(crate) struct LegacySubprocessRequest { + pub(crate) binary: String, + #[serde(default)] + pub(crate) args: Vec, + pub(crate) timeout_ms: Option, +} + +#[derive(Debug, Serialize, Deserialize)] +pub(crate) struct YtDlpResponse { + pub(crate) exit_code: i32, + pub(crate) stdout: String, + pub(crate) stderr: String, +} + +#[derive(Debug)] +pub(super) struct PreparedCommand { + pub(super) args: Vec, + pub(super) working_dir: PathBuf, + pub(super) timeout: Duration, +} + +pub(crate) fn supports_plugin(plugin_name: &str) -> bool { + provider_for_plugin(plugin_name).is_ok() +} + +pub(crate) fn run_plugin_request( + plugin_name: &str, + request: PluginYtDlpRequest, +) -> anyhow::Result { + let provider = provider_for_plugin(plugin_name)?; + process::execute(request::prepare(provider, request, &std::env::temp_dir())?) +} + +pub(crate) fn run_legacy_request( + plugin_name: &str, + request: LegacySubprocessRequest, +) -> anyhow::Result { + process::execute(legacy::prepare( + plugin_name, + request, + &std::env::temp_dir(), + )?) +} + +pub(crate) fn run_generic_metadata(url: String) -> anyhow::Result { + let request = PluginYtDlpRequest::Metadata { + url, + playlist: true, + }; + process::execute(request::prepare( + YtDlpProvider::Generic, + request, + &std::env::temp_dir(), + )?) +} + +pub(super) fn provider_for_plugin(plugin_name: &str) -> anyhow::Result { + match plugin_name { + "vortex-mod-youtube" => Ok(YtDlpProvider::Youtube), + "vortex-mod-vimeo" => Ok(YtDlpProvider::Vimeo), + "vortex-mod-soundcloud" => Ok(YtDlpProvider::Soundcloud), + _ => bail!("run_ytdlp: plugin '{plugin_name}' is not approved for yt-dlp"), + } +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy.rs new file mode 100644 index 00000000..970d9f27 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy.rs @@ -0,0 +1,124 @@ +use std::path::Path; + +use anyhow::bail; + +use super::request::{append_url, prepare as prepare_typed, secure_prefix, strings}; +use super::validation::validate_url; +use super::{ + DEFAULT_TIMEOUT, LegacySubprocessRequest, PluginYtDlpRequest, PreparedCommand, YtDlpProvider, + legacy_download, provider_for_plugin, +}; + +pub(super) fn prepare( + plugin_name: &str, + request: LegacySubprocessRequest, + temp_root: &Path, +) -> anyhow::Result { + if request.binary != "yt-dlp" { + bail!("run_subprocess compatibility: binary replacement is forbidden"); + } + let provider = provider_for_plugin(plugin_name)?; + if contains_dangerous_option(&request.args) { + bail!("run_subprocess compatibility: arguments are not approved"); + } + if let Some(prepared) = metadata_profile(provider, &request.args, temp_root) { + return prepared; + } + if let Some(prepared) = resolve_profile(provider, &request.args, temp_root) { + return prepared; + } + let _ignored_timeout = request.timeout_ms; + legacy_download::prepare(provider, &request.args, temp_root) +} + +fn metadata_profile( + provider: YtDlpProvider, + args: &[String], + temp_root: &Path, +) -> Option> { + let matches = provider == YtDlpProvider::Youtube + && args.len() == 5 + && args[0] == "--dump-json" + && matches!(args[1].as_str(), "--no-playlist" | "--flat-playlist") + && args[2] == "--no-warnings" + && args[3] == "--"; + matches.then(|| { + prepare_typed( + provider, + PluginYtDlpRequest::Metadata { + url: args[4].clone(), + playlist: args[1] == "--flat-playlist", + }, + temp_root, + ) + }) +} + +fn resolve_profile( + provider: YtDlpProvider, + args: &[String], + temp_root: &Path, +) -> Option> { + let matches = provider == YtDlpProvider::Youtube + && args.len() == 7 + && args[0] == "--get-url" + && args[1] == "--no-playlist" + && args[2] == "--no-warnings" + && args[3] == "--format" + && args[5] == "--"; + matches.then(|| rebuild_resolve(provider, args, temp_root)) +} + +fn rebuild_resolve( + provider: YtDlpProvider, + args: &[String], + temp_root: &Path, +) -> anyhow::Result { + validate_selector(&args[4])?; + let mut rebuilt = secure_prefix(); + rebuilt.extend(strings([ + "--get-url", + "--no-playlist", + "--no-warnings", + "--format", + ])); + rebuilt.push(args[4].clone()); + append_url(&mut rebuilt, validate_url(provider, &args[6])?); + Ok(PreparedCommand { + args: rebuilt, + working_dir: temp_root.join("vortex-ytdlp"), + timeout: DEFAULT_TIMEOUT, + }) +} + +fn contains_dangerous_option(args: &[String]) -> bool { + const BLOCKED: [&str; 6] = [ + "--exec", + "--config-location", + "--config-locations", + "--plugin-dirs", + "--netrc-cmd", + "--use-postprocessor", + ]; + args.iter().any(|arg| { + BLOCKED + .iter() + .any(|blocked| arg == blocked || arg.starts_with(&format!("{blocked}="))) + }) +} + +pub(super) fn validate_selector(selector: &str) -> anyhow::Result<()> { + let safe = !selector.is_empty() + && selector.len() <= 1024 + && selector.chars().all(|character| { + character.is_ascii_alphanumeric() + || matches!( + character, + '[' | ']' | '<' | '>' | '=' | '+' | '/' | '_' | '.' | ',' | '-' + ) + }); + if !safe { + bail!("run_subprocess compatibility: format selector is not approved"); + } + Ok(()) +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy_download.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy_download.rs new file mode 100644 index 00000000..89b0bbf9 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy_download.rs @@ -0,0 +1,143 @@ +use std::path::{Path, PathBuf}; + +use anyhow::bail; + +use super::legacy::validate_selector; +use super::request::secure_prefix; +use super::validation::{private_output_dir, validate_format, validate_url}; +use super::{DOWNLOAD_TIMEOUT, PreparedCommand, YtDlpProvider}; + +struct LegacyProfile<'a> { + selector: Option<&'a str>, + output_template: &'a str, + output_index: usize, + url: &'a str, +} + +pub(super) fn prepare( + provider: YtDlpProvider, + args: &[String], + temp_root: &Path, +) -> anyhow::Result { + let profile = match_profile(provider, args)?; + if let Some(selector) = profile.selector { + validate_selector(selector)?; + } + let output_dir = parse_output_dir(profile.output_template, temp_root)?; + let url = validate_url(provider, profile.url)?; + let mut rebuilt = secure_prefix(); + let offset = rebuilt.len(); + rebuilt.extend_from_slice(args); + rebuilt[offset + profile.output_index] = output_template(&output_dir); + let last = rebuilt.len() - 1; + rebuilt[last] = url; + Ok(PreparedCommand { + args: rebuilt, + working_dir: output_dir, + timeout: DOWNLOAD_TIMEOUT, + }) +} + +fn match_profile(provider: YtDlpProvider, args: &[String]) -> anyhow::Result> { + match provider { + YtDlpProvider::Youtube if youtube_profile(args) => { + validate_merge_format(&args[3])?; + Ok(LegacyProfile { + selector: Some(&args[1]), + output_template: &args[5], + output_index: 5, + url: &args[18], + }) + } + YtDlpProvider::Vimeo if vimeo_profile(args) => { + validate_merge_format(&args[3])?; + Ok(LegacyProfile { + selector: Some(&args[1]), + output_template: &args[5], + output_index: 5, + url: &args[16], + }) + } + YtDlpProvider::Soundcloud if soundcloud_profile(args) => { + validate_format(Some(args[2].clone()))?; + Ok(LegacyProfile { + selector: None, + output_template: &args[4], + output_index: 4, + url: &args[11], + }) + } + _ => bail!("run_subprocess compatibility: arguments are not an approved yt-dlp profile"), + } +} + +fn youtube_profile(args: &[String]) -> bool { + args.len() == 19 + && args[0] == "--format" + && args[2] == "--merge-output-format" + && args[4] == "--output" + && common_tail(&args[6..]) + && args[11] == "--extractor-args" + && args[12] == "youtube:player_client=default,web_safari,android_vr,tv" + && retry_tail(&args[13..], 5) +} + +fn vimeo_profile(args: &[String]) -> bool { + args.len() == 17 + && args[0] == "--format" + && args[2] == "--merge-output-format" + && args[4] == "--output" + && common_tail(&args[6..]) + && retry_tail(&args[11..], 5) +} + +fn soundcloud_profile(args: &[String]) -> bool { + args.len() == 12 + && args[0] == "--extract-audio" + && args[1] == "--audio-format" + && args[3] == "--output" + && common_tail(&args[5..]) + && args[10] == "--" +} + +fn common_tail(args: &[String]) -> bool { + args.first().is_some_and(|arg| arg == "--print") + && args + .get(1) + .is_some_and(|arg| arg == "after_move:%(filepath)s") + && args.get(2).is_some_and(|arg| arg == "--no-playlist") + && args.get(3).is_some_and(|arg| arg == "--no-warnings") + && args.get(4).is_some_and(|arg| arg == "--quiet") +} + +fn retry_tail(args: &[String], sentinel: usize) -> bool { + args.first().is_some_and(|arg| arg == "--retries") + && args.get(1).is_some_and(|arg| arg == "3") + && args.get(2).is_some_and(|arg| arg == "--fragment-retries") + && args.get(3).is_some_and(|arg| arg == "3") + && args.get(sentinel - 1).is_some_and(|arg| arg == "--") +} + +fn validate_merge_format(format: &str) -> anyhow::Result<()> { + if !matches!(format, "mkv" | "mov" | "mp4" | "webm") { + bail!("run_subprocess compatibility: merge format is not approved"); + } + Ok(()) +} + +fn parse_output_dir(template: &str, temp_root: &Path) -> anyhow::Result { + let raw = template.strip_suffix("/%(id)s.%(ext)s").ok_or_else(|| { + anyhow::anyhow!("run_subprocess compatibility: output template is not approved") + })?; + if raw.replace("%%", "").contains('%') { + bail!("run_subprocess compatibility: output template is not approved"); + } + private_output_dir(&raw.replace("%%", "%"), temp_root) +} + +fn output_template(output_dir: &Path) -> String { + format!( + "{}/%(id)s.%(ext)s", + output_dir.to_string_lossy().replace('%', "%%") + ) +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/output.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/output.rs new file mode 100644 index 00000000..82daa9f9 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/output.rs @@ -0,0 +1,79 @@ +use std::io::Read; +use std::thread::JoinHandle; + +use anyhow::bail; + +const MAX_OUTPUT_BYTES: usize = 1024 * 1024; + +pub(super) struct CapturedOutput { + pub(super) bytes: Vec, + pub(super) truncated: bool, +} + +pub(super) type ReaderHandle = JoinHandle>; + +pub(super) fn read_stream_capped( + mut reader: R, + max_bytes: usize, +) -> std::io::Result { + let mut output = CapturedOutput { + bytes: Vec::new(), + truncated: false, + }; + let mut chunk = [0_u8; 8192]; + loop { + let read = reader.read(&mut chunk)?; + if read == 0 { + return Ok(output); + } + let copied = max_bytes.saturating_sub(output.bytes.len()).min(read); + output.bytes.extend_from_slice(&chunk[..copied]); + output.truncated |= copied < read; + } +} + +pub(super) fn spawn_reader(stream: Option) -> ReaderHandle +where + T: Read + Send + 'static, +{ + std::thread::spawn(move || match stream { + Some(stream) => read_stream_capped(stream, MAX_OUTPUT_BYTES), + None => Ok(CapturedOutput { + bytes: Vec::new(), + truncated: false, + }), + }) +} + +pub(super) fn collect( + stdout: ReaderHandle, + stderr: ReaderHandle, +) -> anyhow::Result<(String, String)> { + let stdout = stdout.join(); + let stderr = stderr.join(); + let stdout = + stdout.map_err(|_| anyhow::anyhow!("run_ytdlp: stdout reader thread panicked"))??; + let stderr = + stderr.map_err(|_| anyhow::anyhow!("run_ytdlp: stderr reader thread panicked"))??; + decode("stdout", stdout, "stderr", stderr) +} + +fn decode( + first_name: &str, + first: CapturedOutput, + second_name: &str, + second: CapturedOutput, +) -> anyhow::Result<(String, String)> { + if first.truncated || second.truncated { + let stream = if first.truncated { + first_name + } else { + second_name + }; + bail!("run_ytdlp: {stream} exceeded the {MAX_OUTPUT_BYTES}-byte limit"); + } + Ok(( + String::from_utf8_lossy(&first.bytes).into_owned(), + String::from_utf8_lossy(&second.bytes).into_owned(), + )) +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/platform.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/platform.rs new file mode 100644 index 00000000..7cfea2be --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/platform.rs @@ -0,0 +1,145 @@ +use std::ffi::OsString; +use std::path::{Path, PathBuf}; +use std::process::Command; + +use anyhow::{Context, bail}; + +pub(super) fn discover_ytdlp() -> anyhow::Result { + find_approved_binary(&candidates(), &approved_roots()) +} + +fn candidates() -> Vec { + let mut paths = Vec::new(); + if let Some(home) = dirs::home_dir() { + paths.push(home.join(".local/bin/yt-dlp")); + paths.push(home.join(".nix-profile/bin/yt-dlp")); + } + #[cfg(unix)] + paths.extend([ + PathBuf::from("/opt/homebrew/bin/yt-dlp"), + PathBuf::from("/usr/local/bin/yt-dlp"), + PathBuf::from("/usr/bin/yt-dlp"), + PathBuf::from("/run/current-system/sw/bin/yt-dlp"), + PathBuf::from("/nix/var/nix/profiles/default/bin/yt-dlp"), + ]); + #[cfg(windows)] + add_windows_candidates(&mut paths); + paths +} + +#[cfg(windows)] +fn add_windows_candidates(paths: &mut Vec) { + let Some(local) = std::env::var_os("LOCALAPPDATA").map(PathBuf::from) else { + return; + }; + paths.push(local.join("Programs/yt-dlp/yt-dlp.exe")); + let packages = local.join("Microsoft/WinGet/Packages"); + let Ok(entries) = std::fs::read_dir(packages) else { + return; + }; + paths.extend(entries.filter_map(Result::ok).filter_map(|entry| { + entry + .file_name() + .to_string_lossy() + .starts_with("yt-dlp.yt-dlp_") + .then(|| entry.path().join("yt-dlp.exe")) + })); +} + +fn approved_roots() -> Vec { + let mut roots = Vec::new(); + if let Some(home) = dirs::home_dir() { + roots.push(home); + } + #[cfg(unix)] + roots.extend([ + PathBuf::from("/opt/homebrew"), + PathBuf::from("/usr/local"), + PathBuf::from("/usr"), + PathBuf::from("/run/current-system"), + PathBuf::from("/nix/store"), + PathBuf::from("/nix/var/nix/profiles"), + ]); + #[cfg(windows)] + if let Some(local) = std::env::var_os("LOCALAPPDATA") { + roots.push(PathBuf::from(local)); + } + roots +} + +pub(super) fn find_approved_binary( + candidates: &[PathBuf], + roots: &[PathBuf], +) -> anyhow::Result { + for candidate in candidates { + let Ok(canonical) = std::fs::canonicalize(candidate) else { + continue; + }; + if valid_binary(&canonical, roots)? { + return Ok(canonical); + } + } + bail!("yt-dlp not found in approved locations; install it with: pip install yt-dlp") +} + +fn valid_binary(path: &Path, roots: &[PathBuf]) -> anyhow::Result { + let expected = if cfg!(windows) { + "yt-dlp.exe" + } else { + "yt-dlp" + }; + if path.file_name().and_then(|name| name.to_str()) != Some(expected) { + return Ok(false); + } + let metadata = std::fs::metadata(path)?; + if !metadata.is_file() || !inside_approved_root(path, roots) { + return Ok(false); + } + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let mode = metadata.permissions().mode(); + if mode & 0o111 == 0 || mode & 0o022 != 0 { + return Ok(false); + } + } + Ok(true) +} + +fn inside_approved_root(path: &Path, roots: &[PathBuf]) -> bool { + roots.iter().any(|root| { + let canonical = std::fs::canonicalize(root).unwrap_or_else(|_| root.clone()); + path.starts_with(canonical) + }) +} + +pub(super) fn controlled_path(binary: &Path) -> anyhow::Result { + let mut paths = Vec::new(); + if let Some(parent) = binary.parent() { + paths.push(parent.to_path_buf()); + } + paths.extend( + candidates() + .into_iter() + .filter_map(|path| path.parent().map(Path::to_path_buf)), + ); + #[cfg(windows)] + if let Some(root) = std::env::var_os("SystemRoot").map(PathBuf::from) { + paths.extend([root.join("System32"), root]); + } + paths.sort(); + paths.dedup(); + std::env::join_paths(paths).context("run_ytdlp: approved PATH cannot be encoded") +} + +#[cfg(windows)] +pub(super) fn copy_required_environment(command: &mut Command) { + for name in ["SystemRoot", "WINDIR"] { + if let Some(value) = std::env::var_os(name) { + command.env(name, value); + } + } +} + +#[cfg(not(windows))] +pub(super) fn copy_required_environment(_command: &mut Command) {} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/process.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/process.rs new file mode 100644 index 00000000..c6d15a11 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/process.rs @@ -0,0 +1,118 @@ +use std::path::Path; +use std::process::{Command, ExitStatus, Stdio}; +use std::time::{Duration, Instant}; + +use anyhow::{Context, bail}; +use command_group::{CommandGroup, GroupChild}; + +use super::output::{self, ReaderHandle}; +use super::platform; +use super::{PreparedCommand, YtDlpResponse}; + +const PROCESS_POLL_INTERVAL: Duration = Duration::from_millis(25); + +pub(super) fn execute(prepared: PreparedCommand) -> anyhow::Result { + std::fs::create_dir_all(&prepared.working_dir).with_context(|| { + format!( + "run_ytdlp: failed to create working directory '{}'", + prepared.working_dir.display() + ) + })?; + run_process( + &platform::discover_ytdlp()?, + &prepared.args, + &prepared.working_dir, + prepared.timeout, + ) +} + +pub(super) fn run_process( + binary: &Path, + args: &[String], + working_dir: &Path, + timeout: Duration, +) -> anyhow::Result { + let mut child = spawn_group(binary, args, working_dir)?; + let stdout = output::spawn_reader(child.inner().stdout.take()); + let stderr = output::spawn_reader(child.inner().stderr.take()); + let status = wait_for_group(&mut child, &stdout, &stderr, timeout); + let output = output::collect(stdout, stderr); + let status = status?; + let (stdout, stderr) = output?; + Ok(YtDlpResponse { + exit_code: status.code().unwrap_or(-1), + stdout, + stderr, + }) +} + +fn spawn_group(binary: &Path, args: &[String], cwd: &Path) -> anyhow::Result { + if !binary.is_absolute() { + bail!("run_ytdlp: approved binary path must be absolute"); + } + let mut command = Command::new(binary); + command + .args(args) + .current_dir(cwd) + .env_clear() + .env("PATH", platform::controlled_path(binary)?) + .env("LANG", "C.UTF-8") + .env("LC_ALL", "C.UTF-8") + .env("PYTHONIOENCODING", "utf-8") + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()); + platform::copy_required_environment(&mut command); + command + .group_spawn() + .with_context(|| format!("run_ytdlp: failed to spawn '{}'", binary.display())) +} + +fn wait_for_group( + child: &mut GroupChild, + stdout: &ReaderHandle, + stderr: &ReaderHandle, + timeout: Duration, +) -> anyhow::Result { + let started = Instant::now(); + let mut status = None; + loop { + if status.is_none() { + match child.try_wait() { + Ok(result) => status = result, + Err(error) => { + terminate_group(child) + .context("run_ytdlp: failed to clean up process after poll error")?; + return Err(error).context("run_ytdlp: failed to poll process group"); + } + } + } + if let Some(status) = status + && stdout.is_finished() + && stderr.is_finished() + { + return Ok(status); + } + if started.elapsed() >= timeout { + terminate_group(child)?; + bail!( + "run_ytdlp: process timed out after {}ms", + timeout.as_millis() + ); + } + std::thread::sleep(PROCESS_POLL_INTERVAL); + } +} + +fn terminate_group(child: &mut GroupChild) -> anyhow::Result<()> { + let kill_error = child.kill().err(); + child + .wait() + .context("run_ytdlp: failed to reap process group")?; + if let Some(error) = kill_error + && error.kind() != std::io::ErrorKind::InvalidInput + { + return Err(error).context("run_ytdlp: failed to kill process group"); + } + Ok(()) +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/request.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/request.rs new file mode 100644 index 00000000..558cbd37 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/request.rs @@ -0,0 +1,136 @@ +use std::path::Path; + +use anyhow::bail; + +use super::selectors::{build_direct_selector, build_download_args}; +use super::validation::{private_output_dir, validate_format, validate_quality, validate_url}; +use super::{ + DEFAULT_TIMEOUT, DOWNLOAD_TIMEOUT, PluginYtDlpRequest, PreparedCommand, YtDlpProvider, +}; + +pub(super) fn prepare( + provider: YtDlpProvider, + request: PluginYtDlpRequest, + temp_root: &Path, +) -> anyhow::Result { + match request { + PluginYtDlpRequest::Metadata { url, playlist } => { + prepare_metadata(provider, url, playlist, temp_root) + } + PluginYtDlpRequest::Resolve { + url, + quality, + format, + audio_only, + } => prepare_resolve(provider, url, quality, format, audio_only, temp_root), + PluginYtDlpRequest::Download { + url, + quality, + format, + output_dir, + audio_only, + } => prepare_download( + provider, url, quality, format, output_dir, audio_only, temp_root, + ), + } +} + +fn prepare_metadata( + provider: YtDlpProvider, + url: String, + playlist: bool, + temp_root: &Path, +) -> anyhow::Result { + if !matches!(provider, YtDlpProvider::Youtube | YtDlpProvider::Generic) { + bail!("run_ytdlp: metadata action is not available for this provider"); + } + let mut args = secure_prefix(); + args.extend(metadata_args(provider, playlist)); + append_url(&mut args, validate_url(provider, &url)?); + Ok(prepared(args, temp_root, DEFAULT_TIMEOUT)) +} + +fn metadata_args(provider: YtDlpProvider, playlist: bool) -> Vec { + if provider == YtDlpProvider::Generic { + strings(["--dump-single-json", "--flat-playlist", "--no-warnings"]) + } else if playlist { + strings(["--dump-json", "--flat-playlist", "--no-warnings"]) + } else { + strings(["--dump-json", "--no-playlist", "--no-warnings"]) + } +} + +fn prepare_resolve( + provider: YtDlpProvider, + url: String, + quality: Option, + format: Option, + audio_only: bool, + temp_root: &Path, +) -> anyhow::Result { + if provider != YtDlpProvider::Youtube { + bail!("run_ytdlp: resolve action is only available for YouTube"); + } + let selector = build_direct_selector( + validate_quality(quality)?, + validate_format(format)?.as_deref(), + audio_only, + ); + let mut args = secure_prefix(); + args.extend(strings(["--get-url", "--no-playlist", "--no-warnings"])); + args.extend(["--format".to_string(), selector]); + append_url(&mut args, validate_url(provider, &url)?); + Ok(prepared(args, temp_root, DEFAULT_TIMEOUT)) +} + +fn prepare_download( + provider: YtDlpProvider, + url: String, + quality: Option, + format: Option, + output_dir: String, + audio_only: bool, + temp_root: &Path, +) -> anyhow::Result { + let format = validate_format(format)?; + let output_dir = private_output_dir(&output_dir, temp_root)?; + let args = build_download_args( + provider, + &validate_url(provider, &url)?, + validate_quality(quality)?, + format.as_deref(), + &output_dir, + audio_only, + )?; + Ok(PreparedCommand { + args, + working_dir: output_dir, + timeout: DOWNLOAD_TIMEOUT, + }) +} + +fn prepared(args: Vec, temp_root: &Path, timeout: std::time::Duration) -> PreparedCommand { + PreparedCommand { + args, + working_dir: temp_root.join("vortex-ytdlp"), + timeout, + } +} + +pub(super) fn secure_prefix() -> Vec { + strings([ + "--ignore-config", + "--no-plugin-dirs", + "--no-remote-components", + "--no-exec", + "--no-cache-dir", + ]) +} + +pub(super) fn strings(values: [&str; N]) -> Vec { + values.into_iter().map(str::to_string).collect() +} + +pub(super) fn append_url(args: &mut Vec, url: String) { + args.extend(["--".to_string(), url]); +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/selectors.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/selectors.rs new file mode 100644 index 00000000..7be2ae07 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/selectors.rs @@ -0,0 +1,120 @@ +use std::path::Path; + +use anyhow::bail; + +use super::YtDlpProvider; +use super::request::{append_url, secure_prefix, strings}; + +pub(super) fn build_direct_selector( + quality: Option, + format: Option<&str>, + audio_only: bool, +) -> String { + if audio_only { + return format + .map(|ext| { + format!( + "bestaudio[ext={ext}][protocol=https]/bestaudio[protocol=https]/bestaudio[ext={ext}]/bestaudio" + ) + }) + .unwrap_or_else(|| "bestaudio[protocol=https]/bestaudio".to_string()); + } + match (quality, format) { + (Some(height), Some(ext)) => format!( + "best[height<={height}][ext={ext}][protocol=https]/best[height<={height}][protocol=https]" + ), + (Some(height), None) => format!("best[height<={height}][protocol=https]"), + (None, Some(ext)) => format!("best[ext={ext}][protocol=https]/best[protocol=https]"), + (None, None) => "best[protocol=https]".to_string(), + } +} + +fn build_download_selector(quality: Option, format: Option<&str>, audio: bool) -> String { + if audio { + return format + .map(|ext| format!("bestaudio[ext={ext}]/bestaudio")) + .unwrap_or_else(|| "bestaudio".to_string()); + } + quality + .map(|height| { + format!( + "bestvideo[height<={height}]+bestaudio/bestvideo[height<={height}]+bestaudio[ext=m4a]/best[height<={height}]" + ) + }) + .unwrap_or_else(|| "bestvideo+bestaudio/best".to_string()) +} + +pub(super) fn build_download_args( + provider: YtDlpProvider, + url: &str, + quality: Option, + format: Option<&str>, + output_dir: &Path, + audio_only: bool, +) -> anyhow::Result> { + if provider == YtDlpProvider::Generic { + bail!("run_ytdlp: generic provider cannot download files"); + } + let audio = audio_only || provider == YtDlpProvider::Soundcloud; + let mut args = secure_prefix(); + args.extend([ + "--format".to_string(), + build_download_selector(quality, format, audio), + ]); + if audio { + args.extend(strings(["--extract-audio", "--audio-format"])); + args.push(normalize_audio_format(format).to_string()); + } else { + args.extend(strings([ + "--merge-output-format", + merge_format(provider, format), + ])); + } + append_output_args(&mut args, output_dir); + append_provider_args(&mut args, provider); + append_url(&mut args, url.to_string()); + Ok(args) +} + +fn merge_format(provider: YtDlpProvider, format: Option<&str>) -> &str { + if provider == YtDlpProvider::Vimeo { + "mp4" + } else { + format + .filter(|value| matches!(*value, "mkv" | "mov" | "mp4" | "webm")) + .unwrap_or("mp4") + } +} + +fn append_output_args(args: &mut Vec, output_dir: &Path) { + let escaped = output_dir.to_string_lossy().replace('%', "%%"); + args.extend(["--output".to_string(), format!("{escaped}/%(id)s.%(ext)s")]); + args.extend(strings([ + "--print", + "after_move:%(filepath)s", + "--no-playlist", + "--no-warnings", + "--quiet", + ])); +} + +fn append_provider_args(args: &mut Vec, provider: YtDlpProvider) { + if provider == YtDlpProvider::Youtube { + args.extend(strings([ + "--extractor-args", + "youtube:player_client=default,web_safari,android_vr,tv", + ])); + } + if matches!(provider, YtDlpProvider::Youtube | YtDlpProvider::Vimeo) { + args.extend(strings(["--retries", "3", "--fragment-retries", "3"])); + } +} + +fn normalize_audio_format(format: Option<&str>) -> &str { + match format { + Some("aac") => "m4a", + Some("ogg") => "vorbis", + Some(value @ ("best" | "flac" | "m4a" | "mp3" | "opus" | "vorbis" | "wav")) => value, + _ => "mp3", + } +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/audio.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/audio.rs new file mode 100644 index 00000000..3872dd65 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/audio.rs @@ -0,0 +1,23 @@ +use super::super::{PluginYtDlpRequest, YtDlpProvider, request}; + +#[test] +fn vimeo_audio_download_uses_audio_extraction() { + let temp = tempfile::tempdir().unwrap(); + let managed = temp.path().join("vortex-downloads"); + std::fs::create_dir_all(&managed).unwrap(); + let prepared = request::prepare( + YtDlpProvider::Vimeo, + PluginYtDlpRequest::Download { + url: "https://vimeo.com/123456789".to_string(), + quality: None, + format: Some("m4a".to_string()), + output_dir: managed.to_string_lossy().into_owned(), + audio_only: true, + }, + temp.path(), + ) + .unwrap(); + + assert!(prepared.args.contains(&"--extract-audio".to_string())); + assert!(!prepared.args.contains(&"--merge-output-format".to_string())); +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/legacy.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/legacy.rs new file mode 100644 index 00000000..8d70c9a6 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/legacy.rs @@ -0,0 +1,45 @@ +use std::path::Path; + +use super::super::{LegacySubprocessRequest, legacy}; + +#[test] +fn rejects_binary_replacement() { + let request = LegacySubprocessRequest { + binary: "/tmp/yt-dlp".to_string(), + args: vec!["--version".to_string()], + timeout_ms: Some(1_000), + }; + let error = legacy::prepare("vortex-mod-youtube", request, Path::new("/tmp")) + .expect_err("binary replacement must fail"); + assert!(error.to_string().contains("binary")); +} + +#[test] +fn rejects_exec_and_config_arguments() { + for dangerous in ["--exec", "--config-locations", "--plugin-dirs"] { + let request = LegacySubprocessRequest { + binary: "yt-dlp".to_string(), + args: vec![dangerous.to_string(), "payload".to_string()], + timeout_ms: Some(1_000), + }; + let error = legacy::prepare("vortex-mod-youtube", request, Path::new("/tmp")) + .expect_err("dangerous argument must fail"); + assert!(error.to_string().contains("arguments")); + } +} + +#[test] +fn rebuilds_known_youtube_metadata_profile() { + let url = "https://www.youtube.com/watch?v=abcdefghijk"; + let request = LegacySubprocessRequest { + binary: "yt-dlp".to_string(), + args: ["--dump-json", "--no-playlist", "--no-warnings", "--", url] + .map(str::to_string) + .to_vec(), + timeout_ms: Some(60_000), + }; + let prepared = legacy::prepare("vortex-mod-youtube", request, Path::new("/tmp")) + .expect("published profile remains compatible"); + assert_eq!(prepared.args.last().map(String::as_str), Some(url)); + assert!(prepared.args.contains(&"--ignore-config".to_string())); +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/mod.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/mod.rs new file mode 100644 index 00000000..6ac8335e --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/mod.rs @@ -0,0 +1,5 @@ +mod audio; +mod legacy; +mod platform; +mod process; +mod request; diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs new file mode 100644 index 00000000..5bfedb94 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs @@ -0,0 +1,36 @@ +#[cfg(unix)] +mod unix { + use std::os::unix::fs::{PermissionsExt, symlink}; + + use super::super::super::platform::find_approved_binary; + + #[test] + fn binary_must_resolve_inside_root_with_safe_permissions() { + let approved = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let valid = executable(approved.path(), "yt-dlp", 0o700); + let unsafe_binary = executable(outside.path(), "yt-dlp", 0o700); + let link = approved.path().join("linked-yt-dlp"); + symlink(&unsafe_binary, &link).unwrap(); + + let found = + find_approved_binary(&[link, valid.clone()], &[approved.path().to_path_buf()]).unwrap(); + + assert_eq!(found, valid); + } + + #[test] + fn group_writable_binary_is_rejected() { + let approved = tempfile::tempdir().unwrap(); + let binary = executable(approved.path(), "yt-dlp", 0o720); + + assert!(find_approved_binary(&[binary], &[approved.path().to_path_buf()]).is_err()); + } + + fn executable(directory: &std::path::Path, name: &str, mode: u32) -> std::path::PathBuf { + let path = directory.join(name); + std::fs::write(&path, "#!/bin/sh\n").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode)).unwrap(); + path + } +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/process.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/process.rs new file mode 100644 index 00000000..71752c1a --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/process.rs @@ -0,0 +1,58 @@ +use std::io::{Cursor, Seek}; +use std::time::{Duration, Instant}; + +use super::super::output::{collect, read_stream_capped, spawn_reader}; +use super::super::process::run_process; + +#[test] +fn output_reader_caps_but_drains_the_stream() { + let mut cursor = Cursor::new(vec![b'x'; 17]); + let output = read_stream_capped(&mut cursor, 16).expect("read output"); + assert_eq!(output.bytes.len(), 16); + assert!(output.truncated); + assert_eq!(cursor.stream_position().unwrap(), 17); +} + +#[test] +fn oversized_output_is_reported_instead_of_returning_truncated_data() { + let stdout = spawn_reader(Some(Cursor::new(vec![b'x'; 1024 * 1024 + 1]))); + let stderr = spawn_reader(Some(Cursor::new(Vec::new()))); + + let error = collect(stdout, stderr).expect_err("oversized output must fail"); + + assert!(error.to_string().contains("stdout exceeded")); +} + +#[cfg(unix)] +#[test] +fn process_uses_controlled_environment_and_working_directory() { + let temp = tempfile::tempdir().unwrap(); + let binary = fake_binary(temp.path(), "printf '%s\\n' \"${HOME-unset}\"\npwd\n"); + let response = run_process(&binary, &[], temp.path(), Duration::from_secs(1)).unwrap(); + let mut lines = response.stdout.lines(); + assert_eq!(lines.next(), Some("unset")); + assert_eq!(lines.next(), temp.path().to_str()); +} + +#[cfg(unix)] +#[test] +fn timeout_kills_descendants_that_keep_output_pipes_open() { + let temp = tempfile::tempdir().unwrap(); + let binary = fake_binary(temp.path(), "sleep 5 &\n"); + let started = Instant::now(); + let error = run_process(&binary, &[], temp.path(), Duration::from_millis(50)) + .expect_err("process group must time out"); + assert!(error.to_string().contains("timed out")); + assert!(started.elapsed() < Duration::from_secs(2)); +} + +#[cfg(unix)] +fn fake_binary(directory: &std::path::Path, body: &str) -> std::path::PathBuf { + use std::os::unix::fs::PermissionsExt; + let binary = directory.join("yt-dlp"); + std::fs::write(&binary, format!("#!/bin/sh\n{body}")).unwrap(); + let mut permissions = std::fs::metadata(&binary).unwrap().permissions(); + permissions.set_mode(0o700); + std::fs::set_permissions(&binary, permissions).unwrap(); + binary +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs new file mode 100644 index 00000000..bef486c4 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs @@ -0,0 +1,132 @@ +use std::path::Path; + +use super::super::{PluginYtDlpRequest, YtDlpProvider, request}; + +#[test] +fn typed_contract_rejects_binary_and_raw_arguments() { + let with_binary = r#"{ + "action":"metadata", + "url":"https://www.youtube.com/watch?v=abcdefghijk", + "playlist":false, + "binary":"/tmp/yt-dlp" + }"#; + let with_args = r#"{ + "action":"metadata", + "url":"https://www.youtube.com/watch?v=abcdefghijk", + "playlist":false, + "args":["--exec","sh"] + }"#; + assert!(serde_json::from_str::(with_binary).is_err()); + assert!(serde_json::from_str::(with_args).is_err()); +} + +#[test] +fn metadata_disables_external_configuration_and_code_loading() { + let prepared = request::prepare( + YtDlpProvider::Youtube, + metadata("https://www.youtube.com/watch?v=abcdefghijk"), + Path::new("/tmp"), + ) + .expect("valid request"); + assert!(prepared.args.starts_with(&[ + "--ignore-config".to_string(), + "--no-plugin-dirs".to_string(), + "--no-remote-components".to_string(), + "--no-exec".to_string(), + "--no-cache-dir".to_string(), + ])); +} + +#[test] +fn url_is_validated_and_appended_after_option_sentinel() { + let url = "https://www.youtube.com/watch?v=abcdefghijk&next=--exec"; + let prepared = request::prepare(YtDlpProvider::Youtube, metadata(url), Path::new("/tmp")) + .expect("valid request"); + let sentinel = prepared.args.iter().position(|arg| arg == "--").unwrap(); + assert_eq!( + prepared.args.get(sentinel + 1).map(String::as_str), + Some(url) + ); + assert_eq!(sentinel + 2, prepared.args.len()); +} + +#[test] +fn provider_host_mismatch_is_rejected() { + let error = request::prepare( + YtDlpProvider::Youtube, + metadata("https://attacker.example/video"), + Path::new("/tmp"), + ) + .expect_err("host must be rejected"); + assert!(error.to_string().contains("YouTube URL")); +} + +#[test] +fn dangerous_format_value_is_rejected() { + let error = request::prepare( + YtDlpProvider::Youtube, + download("/tmp/vortex-downloads", Some("mp4 --exec sh")), + Path::new("/tmp"), + ) + .expect_err("format must be rejected"); + assert!(error.to_string().contains("format")); +} + +#[test] +fn output_directory_must_be_managed_root() { + let temp = tempfile::tempdir().unwrap(); + let managed = temp.path().join("vortex-downloads"); + let outside = temp.path().join("outside"); + std::fs::create_dir_all(&managed).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + let error = request::prepare( + YtDlpProvider::Youtube, + download(&outside.to_string_lossy(), Some("mp4")), + temp.path(), + ) + .expect_err("working directory must be rejected"); + assert!( + error + .to_string() + .contains("must be the Vortex download root") + ); +} + +#[test] +fn each_download_gets_a_private_output_directory() { + let temp = tempfile::tempdir().unwrap(); + let managed = temp.path().join("vortex-downloads"); + std::fs::create_dir_all(&managed).unwrap(); + let output = managed.to_string_lossy(); + let first = request::prepare( + YtDlpProvider::Youtube, + download(&output, Some("mp4")), + temp.path(), + ) + .unwrap(); + let second = request::prepare( + YtDlpProvider::Youtube, + download(&output, Some("mp4")), + temp.path(), + ) + .unwrap(); + assert_ne!(first.working_dir, second.working_dir); + assert_eq!(first.working_dir.parent(), Some(managed.as_path())); +} + +fn metadata(url: &str) -> PluginYtDlpRequest { + PluginYtDlpRequest::Metadata { + url: url.to_string(), + playlist: false, + } +} + +fn download(output_dir: &str, format: Option<&str>) -> PluginYtDlpRequest { + PluginYtDlpRequest::Download { + url: "https://www.youtube.com/watch?v=abcdefghijk".to_string(), + quality: Some(1080), + format: format.map(str::to_string), + output_dir: output_dir.to_string(), + audio_only: false, + } +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs new file mode 100644 index 00000000..8557c4af --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs @@ -0,0 +1,131 @@ +use std::path::{Path, PathBuf}; + +use anyhow::{Context, bail}; + +use super::YtDlpProvider; + +const MAX_URL_BYTES: usize = 8 * 1024; + +pub(super) fn validate_url(provider: YtDlpProvider, url: &str) -> anyhow::Result { + let url = url.trim(); + if url.is_empty() || url.len() > MAX_URL_BYTES { + bail!("run_ytdlp: URL length is invalid"); + } + let parsed = reqwest::Url::parse(url).context("run_ytdlp: invalid URL")?; + if !matches!(parsed.scheme(), "http" | "https") { + bail!("run_ytdlp: URL must use HTTP or HTTPS"); + } + let host = parsed + .host_str() + .ok_or_else(|| anyhow::anyhow!("run_ytdlp: URL has no host"))? + .to_ascii_lowercase(); + if !host_is_approved(provider, &host) { + bail!("run_ytdlp: expected a {}", provider_label(provider)); + } + Ok(url.to_string()) +} + +fn host_is_approved(provider: YtDlpProvider, host: &str) -> bool { + match provider { + YtDlpProvider::Youtube => matches!( + host, + "youtube.com" + | "www.youtube.com" + | "m.youtube.com" + | "music.youtube.com" + | "youtube-nocookie.com" + | "www.youtube-nocookie.com" + | "youtu.be" + ), + YtDlpProvider::Vimeo => matches!( + host, + "vimeo.com" | "www.vimeo.com" | "player.vimeo.com" | "m.vimeo.com" + ), + YtDlpProvider::Soundcloud => matches!( + host, + "soundcloud.com" | "www.soundcloud.com" | "m.soundcloud.com" | "on.soundcloud.com" + ), + YtDlpProvider::Generic => true, + } +} + +fn provider_label(provider: YtDlpProvider) -> &'static str { + match provider { + YtDlpProvider::Youtube => "YouTube URL", + YtDlpProvider::Vimeo => "Vimeo URL", + YtDlpProvider::Soundcloud => "SoundCloud URL", + YtDlpProvider::Generic => "HTTP(S) URL", + } +} + +pub(super) fn validate_quality(quality: Option) -> anyhow::Result> { + if quality.is_some_and(|value| value == 0 || value > 4320) { + bail!("run_ytdlp: quality must be between 1 and 4320"); + } + Ok(quality) +} + +pub(super) fn validate_format(format: Option) -> anyhow::Result> { + let Some(format) = format else { + return Ok(None); + }; + let format = format.trim().to_ascii_lowercase(); + if format.is_empty() { + return Ok(None); + } + if !is_approved_format(&format) { + bail!("run_ytdlp: unsupported format '{format}'"); + } + Ok(Some(format)) +} + +fn is_approved_format(format: &str) -> bool { + matches!( + format, + "aac" + | "best" + | "flac" + | "m4a" + | "mkv" + | "mov" + | "mp3" + | "mp4" + | "ogg" + | "opus" + | "vorbis" + | "wav" + | "webm" + ) +} + +pub(super) fn private_output_dir(output_dir: &str, temp_root: &Path) -> anyhow::Result { + let allowed_root = temp_root.join("vortex-downloads"); + let canonical_root = std::fs::canonicalize(&allowed_root).with_context(|| { + format!( + "run_ytdlp: output root '{}' is unavailable", + allowed_root.display() + ) + })?; + let requested = std::fs::canonicalize(output_dir) + .with_context(|| format!("run_ytdlp: output directory '{output_dir}' is unavailable"))?; + if requested != canonical_root { + bail!("run_ytdlp: output directory must be the Vortex download root"); + } + create_private_dir(&canonical_root) +} + +fn create_private_dir(root: &Path) -> anyhow::Result { + let path = root.join(format!("job-{}", uuid::Uuid::new_v4().simple())); + std::fs::create_dir(&path).with_context(|| { + format!( + "run_ytdlp: failed to create private output directory '{}'", + path.display() + ) + })?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o700))?; + } + Ok(path) +} diff --git a/src-tauri/src/adapters/driving/tauri_ipc.rs b/src-tauri/src/adapters/driving/tauri_ipc.rs index cfb01b3a..887fc17c 100644 --- a/src-tauri/src/adapters/driving/tauri_ipc.rs +++ b/src-tauri/src/adapters/driving/tauri_ipc.rs @@ -1773,6 +1773,10 @@ fn resolve_media_stream( file_info.path.display() )); } + let private_output_dir = produced_canonical + .parent() + .filter(|parent| parent.parent() == Some(temp_dir_canonical.as_path())) + .map(Path::to_path_buf); let filename = title .as_deref() @@ -1815,6 +1819,9 @@ fn resolve_media_stream( ); } } + if let Some(path) = private_output_dir { + cleanup_private_output_dir(&path); + } Ok(StreamResolution::LocalFile { path: dest_path, @@ -1837,6 +1844,20 @@ fn resolve_media_stream( } } +fn cleanup_private_output_dir(path: &Path) { + // ponytail: only delete an empty job directory; add a bounded stale-job + // janitor if failed partial downloads cause measurable disk growth. + if let Err(error) = std::fs::remove_dir(path) + && error.kind() != std::io::ErrorKind::NotFound + { + tracing::warn!( + path = %path.display(), + error = %error, + "failed to remove private yt-dlp output directory" + ); + } +} + fn configured_download_destination(download_dir: Option<&str>) -> Option { download_dir .map(str::trim) @@ -1911,27 +1932,18 @@ pub async fn command_get_media_metadata( } } - let output = tokio::task::spawn_blocking(move || -> Result { - let binary = find_ytdlp()?; - std::process::Command::new(&binary) - .args([ - "--dump-single-json", - "--flat-playlist", - "--no-warnings", - &url, - ]) - .output() - .map_err(|e| format!("Failed to run yt-dlp: {e}")) + let output = tokio::task::spawn_blocking(move || { + crate::adapters::driven::plugin::ytdlp_broker::run_generic_metadata(url) + .map_err(|error| format!("Failed to run yt-dlp: {error}")) }) .await .map_err(|e| format!("Task join error: {e}"))??; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - return Err(format!("yt-dlp error: {stderr}")); + if output.exit_code != 0 { + return Err(format!("yt-dlp error: {}", output.stderr)); } - let json: serde_json::Value = serde_json::from_slice(&output.stdout) + let json: serde_json::Value = serde_json::from_str(&output.stdout) .map_err(|e| format!("Failed to parse yt-dlp output: {e}"))?; parse_ytdlp_json(&json) @@ -1975,9 +1987,13 @@ struct PluginMediaVariant { #[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Deserialize)] #[serde(rename_all = "snake_case")] enum PluginVariantKind { + #[serde(alias = "muxed", alias = "video_only")] Video, + #[serde(alias = "audio_only")] Audio, Adaptive, + #[serde(other)] + Unknown, } #[derive(Debug, Clone, serde::Deserialize)] @@ -2090,7 +2106,10 @@ fn parse_plugin_video_metadata( }); } - if !variant.ext.is_empty() && seen_video_exts.insert(variant.ext.clone()) { + if matches!(kind, PluginVariantKind::Video) + && !variant.ext.is_empty() + && seen_video_exts.insert(variant.ext.clone()) + { available_formats.push(variant.ext); } } @@ -2099,6 +2118,7 @@ fn parse_plugin_video_metadata( available_audio_formats.push(variant.ext); } } + PluginVariantKind::Unknown => {} } } @@ -2266,35 +2286,6 @@ fn millis_to_seconds(duration_ms: Option) -> u64 { duration_ms.unwrap_or_default() / 1000 } -fn find_ytdlp() -> Result { - // Try PATH via `which` equivalent — just attempt running `yt-dlp --version` - if std::process::Command::new("yt-dlp") - .arg("--version") - .output() - .map(|o| o.status.success()) - .unwrap_or(false) - { - return Ok(std::path::PathBuf::from("yt-dlp")); - } - - // Known fallback locations - let mut candidates = vec![ - std::path::PathBuf::from("/usr/local/bin/yt-dlp"), - std::path::PathBuf::from("/usr/bin/yt-dlp"), - ]; - if let Some(home) = dirs::home_dir() { - candidates.insert(0, home.join(".local/bin/yt-dlp")); - } - - for path in &candidates { - if path.exists() { - return Ok(path.clone()); - } - } - - Err("yt-dlp not found — install it with: pip install yt-dlp".to_string()) -} - /// Canonical YouTube vertical-resolution ladder supported by /// `vortex-mod-youtube`. Kept in sync with the `default_quality.options` array /// in the plugin's `plugin.toml`. Anything off this list either has no @@ -3880,7 +3871,7 @@ mod tests { } #[test] - fn test_parse_plugin_video_metadata_keeps_adaptive_variants_available() { + fn test_parse_plugin_video_metadata_keeps_adaptive_quality_not_transport_format() { let extract_links = serde_json::json!({ "kind": "video", "videos": [{ @@ -3893,7 +3884,7 @@ mod tests { "variants": [ { "kind": "adaptive", - "ext": "mp4", + "ext": "m3u8", "width": 1280, "height": 720, "fps": 30.0, @@ -3909,7 +3900,31 @@ mod tests { assert_eq!(metadata.default_quality.as_deref(), Some("720p")); assert_eq!(metadata.available_qualities.len(), 1); assert_eq!(metadata.available_qualities[0].quality, "720p"); - assert_eq!(metadata.available_formats, vec!["mp4"]); + assert!(metadata.available_formats.is_empty()); + } + + #[test] + fn test_parse_plugin_video_metadata_accepts_youtube_variant_kinds() { + let extract_links = serde_json::json!({ + "kind": "video", + "videos": [{"title": "YouTube", "duration": 1, "thumbnail": null}] + }); + let variants = serde_json::json!({ + "variants": [ + {"kind": "muxed", "ext": "mp4", "height": 720}, + {"kind": "video_only", "ext": "webm", "height": 1080}, + {"kind": "audio_only", "ext": "m4a", "abr": 128.0}, + {"kind": "unknown", "ext": "bin"} + ] + }); + + let metadata = + parse_plugin_video_metadata(&extract_links.to_string(), &variants.to_string()) + .expect("YouTube variants should parse"); + + assert_eq!(metadata.available_formats, vec!["mp4", "webm"]); + assert_eq!(metadata.available_audio_formats, vec!["m4a"]); + assert_eq!(metadata.default_quality.as_deref(), Some("1080p")); } #[test] From 4fcc61cd0231aae2c6288c9a79c59c7f5e121b0a Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:31:45 +0200 Subject: [PATCH 02/16] chore(deps): remove obsolete Extism fork allowlist --- deny.toml | 3 --- 1 file changed, 3 deletions(-) diff --git a/deny.toml b/deny.toml index eba42c07..f4924f8e 100644 --- a/deny.toml +++ b/deny.toml @@ -108,6 +108,3 @@ skip-tree = [] unknown-registry = "deny" unknown-git = "deny" allow-registry = ["https://github.com/rust-lang/crates.io-index"] -# Temporary: extism git fork pinned in src-tauri/Cargo.toml until upstream -# ships wasmtime >=43 (clears GHSA-jhxm-h53p-jm7w + GHSA-xx5w-cvp6-jv83). -allow-git = ["https://github.com/mpiton/extism.git"] From b70495f246944921b0781328a0139c404e775214 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:53:57 +0200 Subject: [PATCH 03/16] refactor(ci): isolate read-only plugin verification --- .github/workflows/plugin-ci.yml | 141 +------------------------------- 1 file changed, 1 insertion(+), 140 deletions(-) diff --git a/.github/workflows/plugin-ci.yml b/.github/workflows/plugin-ci.yml index 0d37c98e..142ab82a 100644 --- a/.github/workflows/plugin-ci.yml +++ b/.github/workflows/plugin-ci.yml @@ -1,4 +1,4 @@ -name: Reusable Plugin CI +name: Reusable Plugin Verification on: workflow_call: @@ -160,142 +160,3 @@ jobs: path: dist/ if-no-files-found: error retention-days: 14 - - release: - name: Publish immutable GitHub release - if: startsWith(github.ref, 'refs/tags/v') - needs: build-and-test - runs-on: ubuntu-24.04 - timeout-minutes: 10 - permissions: - contents: write - - steps: - - name: Download verified bundle - uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 - with: - name: plugin-release-${{ github.sha }} - path: dist - - - name: Verify release metadata - env: - RELEASE_TAG: ${{ github.ref_name }} - shell: bash - run: | - ( - cd dist - sha256sum --check SHA256SUMS - ) - python3 - <<'PY' - import os - import tomllib - - with open("dist/plugin.toml", "rb") as file: - manifest = tomllib.load(file)["plugin"] - - tag = os.environ["RELEASE_TAG"] - if tag != f"v{manifest['version']}": - raise SystemExit( - f"tag {tag!r} does not match plugin version {manifest['version']!r}" - ) - PY - - - name: Create or verify immutable release - env: - GH_TOKEN: ${{ github.token }} - GH_REPO: ${{ github.repository }} - RELEASE_TAG: ${{ github.ref_name }} - shell: bash - run: | - if ! gh release view "${RELEASE_TAG}" >/dev/null 2>&1; then - gh release create "${RELEASE_TAG}" dist/* \ - --verify-tag \ - --generate-notes \ - --title "${RELEASE_TAG}" - fi - - expected_assets="$(find dist -maxdepth 1 -type f -printf '%f\n' | LC_ALL=C sort)" - actual_assets="$(gh release view "${RELEASE_TAG}" \ - --json assets --jq '.assets[].name' | LC_ALL=C sort)" - if [[ "${actual_assets}" != "${expected_assets}" ]]; then - echo "release asset set differs from the verified bundle" >&2 - diff -u <(printf '%s\n' "${expected_assets}") \ - <(printf '%s\n' "${actual_assets}") || true - exit 1 - fi - - published="$(mktemp -d)" - gh release download "${RELEASE_TAG}" --dir "${published}" - while IFS= read -r asset; do - if ! cmp --silent "dist/${asset}" "${published}/${asset}"; then - echo "release asset content differs: ${asset}" >&2 - exit 1 - fi - done <<<"${expected_assets}" - - registry-consistency: - name: Check central registry after release - if: startsWith(github.ref, 'refs/tags/v') - needs: release - runs-on: ubuntu-24.04 - timeout-minutes: 5 - permissions: - contents: read - - steps: - - name: Download released bundle - uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 - with: - name: plugin-release-${{ github.sha }} - path: dist - - - name: Compare release with central registry - id: registry-check - continue-on-error: true - shell: bash - run: | - registry="${RUNNER_TEMP}/registry.toml" - curl --fail --silent --show-error --location \ - --proto '=https' --tlsv1.2 \ - https://raw.githubusercontent.com/mpiton/vortex/main/registry/registry.toml \ - --output "${registry}" - REGISTRY_PATH="${registry}" python3 - <<'PY' - import hashlib - import os - import pathlib - import tomllib - - dist = pathlib.Path("dist") - with (dist / "plugin.toml").open("rb") as file: - manifest = tomllib.load(file)["plugin"] - with open(os.environ["REGISTRY_PATH"], "rb") as file: - entries = tomllib.load(file)["plugin"] - - entry = next( - (item for item in entries if item["name"] == manifest["name"]), - None, - ) - if entry is None: - raise SystemExit("plugin is missing from the central registry") - for key in ("version", "min_vortex_version"): - if entry[key] != manifest[key]: - raise SystemExit(f"registry {key} differs from plugin.toml") - - wasm_sha = hashlib.sha256((dist / "plugin.wasm").read_bytes()).hexdigest() - manifest_sha = hashlib.sha256((dist / "plugin.toml").read_bytes()).hexdigest() - if entry["checksum_sha256"] != wasm_sha: - raise SystemExit("registry WASM checksum differs from release artifact") - if entry["checksum_sha256_toml"] != manifest_sha: - raise SystemExit("registry manifest checksum differs from plugin.toml") - PY - - - name: Report registry update requirement - if: steps.registry-check.outcome == 'failure' - shell: bash - run: | - echo "::warning title=Registry update required::The release is published, but vortex/registry/registry.toml does not match its assets." - { - echo "### Registry update required" - echo - echo "The release is published and immutable. Update \`vortex/registry/registry.toml\` with its version and checksums." - } >> "${GITHUB_STEP_SUMMARY}" From 3abf4058eb1fa946b196b1a87cf459c4ffe73a4e Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 16:54:49 +0200 Subject: [PATCH 04/16] feat(ci): add tag-only plugin release workflow --- .github/workflows/plugin-release.yml | 156 +++++++++++++++++++++++++++ 1 file changed, 156 insertions(+) create mode 100644 .github/workflows/plugin-release.yml diff --git a/.github/workflows/plugin-release.yml b/.github/workflows/plugin-release.yml new file mode 100644 index 00000000..cb70b754 --- /dev/null +++ b/.github/workflows/plugin-release.yml @@ -0,0 +1,156 @@ +name: Reusable Plugin Release + +on: + workflow_call: + +permissions: {} + +concurrency: + group: plugin-release-${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: false + +jobs: + verify: + name: Verify release bundle + permissions: + contents: read + uses: mpiton/vortex/.github/workflows/plugin-ci.yml@b70495f246944921b0781328a0139c404e775214 + + release: + name: Publish immutable GitHub release + if: startsWith(github.ref, 'refs/tags/v') + needs: verify + runs-on: ubuntu-24.04 + timeout-minutes: 10 + permissions: + contents: write + + steps: + - name: Download verified bundle + uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 + with: + name: plugin-release-${{ github.sha }} + path: dist + + - name: Verify release metadata + env: + RELEASE_TAG: ${{ github.ref_name }} + shell: bash + run: | + ( + cd dist + sha256sum --check SHA256SUMS + ) + python3 - <<'PY' + import os + import tomllib + + with open("dist/plugin.toml", "rb") as file: + manifest = tomllib.load(file)["plugin"] + + tag = os.environ["RELEASE_TAG"] + if tag != f"v{manifest['version']}": + raise SystemExit( + f"tag {tag!r} does not match plugin version {manifest['version']!r}" + ) + PY + + - name: Create or verify immutable release + env: + GH_TOKEN: ${{ github.token }} + GH_REPO: ${{ github.repository }} + RELEASE_TAG: ${{ github.ref_name }} + shell: bash + run: | + if ! gh release view "${RELEASE_TAG}" >/dev/null 2>&1; then + gh release create "${RELEASE_TAG}" dist/* \ + --verify-tag \ + --generate-notes \ + --title "${RELEASE_TAG}" + fi + + expected_assets="$(find dist -maxdepth 1 -type f -printf '%f\n' | LC_ALL=C sort)" + actual_assets="$(gh release view "${RELEASE_TAG}" \ + --json assets --jq '.assets[].name' | LC_ALL=C sort)" + if [[ "${actual_assets}" != "${expected_assets}" ]]; then + echo "release asset set differs from the verified bundle" >&2 + diff -u <(printf '%s\n' "${expected_assets}") \ + <(printf '%s\n' "${actual_assets}") || true + exit 1 + fi + + published="$(mktemp -d)" + gh release download "${RELEASE_TAG}" --dir "${published}" + while IFS= read -r asset; do + if ! cmp --silent "dist/${asset}" "${published}/${asset}"; then + echo "release asset content differs: ${asset}" >&2 + exit 1 + fi + done <<<"${expected_assets}" + + registry-consistency: + name: Check central registry after release + if: startsWith(github.ref, 'refs/tags/v') + needs: release + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: + contents: read + + steps: + - name: Download released bundle + uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 + with: + name: plugin-release-${{ github.sha }} + path: dist + + - name: Compare release with central registry + id: registry-check + continue-on-error: true + shell: bash + run: | + registry="${RUNNER_TEMP}/registry.toml" + curl --fail --silent --show-error --location \ + --proto '=https' --tlsv1.2 \ + https://raw.githubusercontent.com/mpiton/vortex/main/registry/registry.toml \ + --output "${registry}" + REGISTRY_PATH="${registry}" python3 - <<'PY' + import hashlib + import os + import pathlib + import tomllib + + dist = pathlib.Path("dist") + with (dist / "plugin.toml").open("rb") as file: + manifest = tomllib.load(file)["plugin"] + with open(os.environ["REGISTRY_PATH"], "rb") as file: + entries = tomllib.load(file)["plugin"] + + entry = next( + (item for item in entries if item["name"] == manifest["name"]), + None, + ) + if entry is None: + raise SystemExit("plugin is missing from the central registry") + for key in ("version", "min_vortex_version"): + if entry[key] != manifest[key]: + raise SystemExit(f"registry {key} differs from plugin.toml") + + wasm_sha = hashlib.sha256((dist / "plugin.wasm").read_bytes()).hexdigest() + manifest_sha = hashlib.sha256((dist / "plugin.toml").read_bytes()).hexdigest() + if entry["checksum_sha256"] != wasm_sha: + raise SystemExit("registry WASM checksum differs from release artifact") + if entry["checksum_sha256_toml"] != manifest_sha: + raise SystemExit("registry manifest checksum differs from plugin.toml") + PY + + - name: Report registry update requirement + if: steps.registry-check.outcome == 'failure' + shell: bash + run: | + echo "::warning title=Registry update required::The release is published, but vortex/registry/registry.toml does not match its assets." + { + echo "### Registry update required" + echo + echo "The release is published and immutable. Update \`vortex/registry/registry.toml\` with its version and checksums." + } >> "${GITHUB_STEP_SUMMARY}" From 676ce0e2bc3d11ad7e77eb7035772183d8c337a3 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 17:29:04 +0200 Subject: [PATCH 05/16] fix(ci): whitelist verified release assets --- .github/workflows/plugin-release.yml | 35 +++++++++++++++++++++++++--- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/.github/workflows/plugin-release.yml b/.github/workflows/plugin-release.yml index cb70b754..aa0d6187 100644 --- a/.github/workflows/plugin-release.yml +++ b/.github/workflows/plugin-release.yml @@ -43,6 +43,7 @@ jobs: ) python3 - <<'PY' import os + import re import tomllib with open("dist/plugin.toml", "rb") as file: @@ -53,6 +54,15 @@ jobs: raise SystemExit( f"tag {tag!r} does not match plugin version {manifest['version']!r}" ) + + plugin_name = manifest.get("name") + if not isinstance(plugin_name, str): + raise SystemExit("plugin name is missing from plugin.toml") + release_wasm_asset = f"{plugin_name.replace('-', '_')}.wasm" + if re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*\.wasm", release_wasm_asset) is None: + raise SystemExit("plugin name cannot be mapped to a release WASM asset") + with open(os.environ["GITHUB_ENV"], "a", encoding="utf-8") as env: + env.write(f"PLUGIN_WASM_ASSET={release_wasm_asset}\n") PY - name: Create or verify immutable release @@ -62,14 +72,32 @@ jobs: RELEASE_TAG: ${{ github.ref_name }} shell: bash run: | + release_assets=( + "plugin.wasm" + "plugin.toml" + "SHA256SUMS" + "${PLUGIN_WASM_ASSET}" + ) + release_paths=() + download_patterns=() + for asset in "${release_assets[@]}"; do + test -s "dist/${asset}" + release_paths+=("dist/${asset}") + download_patterns+=(--pattern "${asset}") + done + if ! cmp --silent dist/plugin.wasm "dist/${PLUGIN_WASM_ASSET}"; then + echo "release WASM asset differs from verified plugin.wasm" >&2 + exit 1 + fi + if ! gh release view "${RELEASE_TAG}" >/dev/null 2>&1; then - gh release create "${RELEASE_TAG}" dist/* \ + gh release create "${RELEASE_TAG}" "${release_paths[@]}" \ --verify-tag \ --generate-notes \ --title "${RELEASE_TAG}" fi - expected_assets="$(find dist -maxdepth 1 -type f -printf '%f\n' | LC_ALL=C sort)" + expected_assets="$(printf '%s\n' "${release_assets[@]}" | LC_ALL=C sort)" actual_assets="$(gh release view "${RELEASE_TAG}" \ --json assets --jq '.assets[].name' | LC_ALL=C sort)" if [[ "${actual_assets}" != "${expected_assets}" ]]; then @@ -80,7 +108,8 @@ jobs: fi published="$(mktemp -d)" - gh release download "${RELEASE_TAG}" --dir "${published}" + gh release download "${RELEASE_TAG}" --dir "${published}" \ + "${download_patterns[@]}" while IFS= read -r asset; do if ! cmp --silent "dist/${asset}" "${published}/${asset}"; then echo "release asset content differs: ${asset}" >&2 From 158b042d664d86995f1ea793a2fe434c0ea581cb Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 17:54:35 +0200 Subject: [PATCH 06/16] fix(plugins): harden trusted yt-dlp execution --- CHANGELOG.md | 6 + .../adapters/driven/plugin/capabilities.rs | 49 ++- .../adapters/driven/plugin/extism_loader.rs | 355 ++++++++++++++---- .../src/adapters/driven/plugin/manifest.rs | 18 +- src-tauri/src/adapters/driven/plugin/mod.rs | 1 + .../src/adapters/driven/plugin/provenance.rs | 272 ++++++++++++++ .../adapters/driven/plugin/ytdlp_broker.rs | 55 ++- .../driven/plugin/ytdlp_broker/legacy.rs | 9 +- .../plugin/ytdlp_broker/legacy_download.rs | 21 +- .../driven/plugin/ytdlp_broker/output.rs | 27 +- .../driven/plugin/ytdlp_broker/platform.rs | 108 +++++- .../driven/plugin/ytdlp_broker/process.rs | 84 ++++- .../driven/plugin/ytdlp_broker/request.rs | 41 +- .../driven/plugin/ytdlp_broker/selectors.rs | 7 +- .../driven/plugin/ytdlp_broker/tests/audio.rs | 35 +- .../plugin/ytdlp_broker/tests/legacy.rs | 40 +- .../driven/plugin/ytdlp_broker/tests/mod.rs | 13 + .../plugin/ytdlp_broker/tests/platform.rs | 29 +- .../plugin/ytdlp_broker/tests/process.rs | 61 ++- .../plugin/ytdlp_broker/tests/request.rs | 141 ++++++- .../driven/plugin/ytdlp_broker/validation.rs | 249 +++++++++++- src-tauri/src/adapters/driving/tauri_ipc.rs | 36 +- .../src/application/commands/store_install.rs | 61 ++- .../src/domain/ports/driven/plugin_loader.rs | 31 ++ .../ports/driven/plugin_store_client.rs | 121 ++++++ 25 files changed, 1638 insertions(+), 232 deletions(-) create mode 100644 src-tauri/src/adapters/driven/plugin/provenance.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 5cc623bd..c2b440c6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 keeps the exact profiles of already-published official plugins loadable during the migration; arbitrary subprocess capabilities are no longer registered. The Tauri metadata fallback uses the same broker. +- **Official plugin provenance (MAT-131)**: yt-dlp host functions are now + granted only to Store-installed official plugins whose manifest and WASM + still match freshly fetched registry checksums. The grant is persisted + outside plugin directories and revalidated on every load; local installs, + hot-reloaded modifications, unsafe executable paths, oversized process + output, and failed partial downloads all fail closed. ### Fixed diff --git a/src-tauri/src/adapters/driven/plugin/capabilities.rs b/src-tauri/src/adapters/driven/plugin/capabilities.rs index 96455cd7..289a488e 100644 --- a/src-tauri/src/adapters/driven/plugin/capabilities.rs +++ b/src-tauri/src/adapters/driven/plugin/capabilities.rs @@ -94,13 +94,32 @@ pub struct PluginHostContext { pub(crate) shared: Arc, } +/// Host-owned privileges established outside the plugin manifest. +/// +/// A manifest can request a capability, but it cannot grant itself access to +/// a native broker. The loader only sets these grants after verifying the +/// installed files against trusted Store provenance. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(super) struct HostFunctionGrants { + pub(super) ytdlp: bool, +} + /// Build host functions based on manifest capabilities. /// /// Always registers the six base functions (log, get/set config, get/set state, get credential). /// Conditionally registers HTTP and the constrained yt-dlp broker based on capabilities. +#[cfg(test)] pub fn build_host_functions( manifest: &PluginManifest, shared: &Arc, +) -> Vec { + build_host_functions_with_grants(manifest, shared, HostFunctionGrants::default()) +} + +pub(super) fn build_host_functions_with_grants( + manifest: &PluginManifest, + shared: &Arc, + grants: HostFunctionGrants, ) -> Vec { let name = manifest.info().name().to_string(); @@ -133,10 +152,10 @@ pub fn build_host_functions( let declares_ytdlp = manifest.has_capability("subprocess:yt-dlp"); let supports_ytdlp = super::ytdlp_broker::supports_plugin(&name); - if declares_ytdlp && !supports_ytdlp { + if declares_ytdlp && (!supports_ytdlp || !grants.ytdlp) { tracing::warn!( plugin = %name, - "ignoring yt-dlp capability for an unapproved plugin" + "ignoring yt-dlp capability without verified official provenance" ); } @@ -162,7 +181,7 @@ pub fn build_host_functions( )); } - if declares_ytdlp && supports_ytdlp { + if declares_ytdlp && supports_ytdlp && grants.ytdlp { functions.push(super::host_functions::make_run_ytdlp_function( user_data.clone(), )); @@ -202,7 +221,11 @@ mod tests { vec!["http", "subprocess:ffmpeg", "subprocess:yt-dlp"], ); - let functions = build_host_functions(&manifest, &shared); + let functions = build_host_functions_with_grants( + &manifest, + &shared, + HostFunctionGrants { ytdlp: true }, + ); // 6 base + http + typed yt-dlp + legacy compatibility = 9 assert_eq!(functions.len(), 9); @@ -319,13 +342,29 @@ mod tests { let manifest = make_named_manifest_with_caps("vortex-mod-youtube", vec!["subprocess:yt-dlp"]); - let functions = build_host_functions(&manifest, &shared); + let functions = build_host_functions_with_grants( + &manifest, + &shared, + HostFunctionGrants { ytdlp: true }, + ); assert_eq!(functions.len(), 8); assert!(functions.iter().any(|f| f.name() == "run_ytdlp")); assert!(functions.iter().any(|f| f.name() == "run_subprocess")); } + #[test] + fn test_official_name_without_verified_provenance_cannot_register_ytdlp() { + let shared = Arc::new(SharedHostResources::new()); + let manifest = + make_named_manifest_with_caps("vortex-mod-youtube", vec!["subprocess:yt-dlp"]); + + let functions = build_host_functions(&manifest, &shared); + + assert!(!functions.iter().any(|f| f.name() == "run_ytdlp")); + assert!(!functions.iter().any(|f| f.name() == "run_subprocess")); + } + #[test] fn test_unapproved_plugin_cannot_register_ytdlp() { let shared = Arc::new(SharedHostResources::new()); diff --git a/src-tauri/src/adapters/driven/plugin/extism_loader.rs b/src-tauri/src/adapters/driven/plugin/extism_loader.rs index fb3a1d4e..a1dca797 100644 --- a/src-tauri/src/adapters/driven/plugin/extism_loader.rs +++ b/src-tauri/src/adapters/driven/plugin/extism_loader.rs @@ -9,10 +9,14 @@ use crate::domain::error::DomainError; use crate::domain::model::plugin::{PluginInfo, PluginManifest}; use crate::domain::ports::driven::PluginLoader; use crate::domain::ports::driven::plugin_loader::DownloadedFileInfo; +use crate::domain::ports::driven::plugin_store_client::OfficialPluginProvenance; use super::builtin::HttpModule; -use super::capabilities::{SharedHostResources, build_host_functions}; -use super::manifest::{find_wasm_file, parse_manifest, parse_manifest_metadata}; +use super::capabilities::{SharedHostResources, build_host_functions_with_grants}; +use super::manifest::{ + find_wasm_file, parse_manifest, parse_manifest_metadata, parse_manifest_metadata_bytes, +}; +use super::provenance::OfficialProvenanceStore; use super::registry::{LoadedPlugin, PluginRegistry}; /// Per-plugin install coordination. @@ -48,6 +52,7 @@ pub struct ExtismPluginLoader { /// Per-plugin install coordination. See [`InstallState`] for the /// two pieces of state it carries (serializer + refcount). installs: Arc>>>, + provenance: OfficialProvenanceStore, } /// RAII guard: decrements the install refcount when dropped, so the @@ -68,12 +73,27 @@ impl ExtismPluginLoader { plugins_dir: PathBuf, shared_resources: Arc, ) -> Result { + let provenance_path = plugins_dir.with_extension("provenance.json"); + Self::new_with_provenance_path(plugins_dir, shared_resources, provenance_path) + } + + pub fn new_with_provenance_path( + plugins_dir: PathBuf, + shared_resources: Arc, + provenance_path: PathBuf, + ) -> Result { + if provenance_path.starts_with(&plugins_dir) { + return Err(DomainError::ValidationError( + "plugin provenance must live outside the plugin directory".into(), + )); + } Ok(Self { registry: Arc::new(PluginRegistry::new()), plugins_dir, shared_resources, builtin_http: HttpModule::new()?, installs: Arc::new(Mutex::new(HashMap::new())), + provenance: OfficialProvenanceStore::new(provenance_path)?, }) } @@ -161,6 +181,77 @@ impl ExtismPluginLoader { DomainError::PluginError(format!("plugin '{}' {func} failed: {e}", info.name())) }) } + + fn install_from_dir( + &self, + dir: &Path, + provenance: Option<&OfficialPluginProvenance>, + ) -> Result<(), DomainError> { + let (manifest, _) = parse_manifest(dir)?; + let name = manifest.info().name().to_string(); + if let Some(provenance) = provenance { + verify_provenance(dir, &manifest, provenance)?; + } + + let state = self.get_or_create_install_state(&name); + state.count.fetch_add(1, Ordering::SeqCst); + let _in_flight = InstallInFlight { + state: state.clone(), + }; + let _serializer_guard = state + .serializer + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + + match self.unload(&name) { + Ok(()) | Err(DomainError::NotFound(_)) => {} + Err(error) => return Err(error), + } + if provenance.is_none() { + self.provenance.revoke(&name)?; + } + + let dest_dir = self.plugins_dir.join(&name); + if dest_dir.exists() { + std::fs::remove_dir_all(&dest_dir).map_err(|error| { + DomainError::PluginError(format!( + "failed to remove existing plugin dir '{}': {error}", + dest_dir.display() + )) + })?; + } + std::fs::create_dir_all(&dest_dir).map_err(|error| { + DomainError::PluginError(format!("failed to create plugin dir: {error}")) + })?; + for entry in std::fs::read_dir(dir).map_err(|error| { + DomainError::PluginError(format!("failed to read staging dir: {error}")) + })? { + let entry = entry.map_err(|error| { + DomainError::PluginError(format!("staging dir entry error: {error}")) + })?; + let source = entry.path(); + if source.is_file() { + let destination = dest_dir.join(entry.file_name()); + std::fs::copy(&source, &destination).map_err(|error| { + DomainError::PluginError(format!( + "failed to copy {} → {}: {error}", + source.display(), + destination.display() + )) + })?; + } + } + + if let Some(provenance) = provenance { + verify_provenance(&dest_dir, &manifest, provenance)?; + self.provenance.record(provenance)?; + } + let result = self.load(&manifest); + if result.is_err() && provenance.is_some() { + self.provenance.revoke(&name)?; + } + result + } } impl PluginLoader for ExtismPluginLoader { @@ -176,6 +267,10 @@ impl PluginLoader for ExtismPluginLoader { // Derive wasm path directly from convention: plugins_dir//.wasm let plugin_dir = self.plugins_dir.join(&name); + let manifest_bytes = std::fs::read(plugin_dir.join("plugin.toml")).map_err(|error| { + DomainError::PluginError(format!("failed to read plugin.toml for '{name}': {error}")) + })?; + let disk_manifest = parse_manifest_metadata_bytes(&plugin_dir, &manifest_bytes)?; let wasm_path = find_wasm_file(&plugin_dir)?; const MAX_WASM_SIZE: u64 = 100 * 1024 * 1024; // 100 MB @@ -193,13 +288,20 @@ impl PluginLoader for ExtismPluginLoader { DomainError::PluginError(format!("failed to read wasm {}: {e}", wasm_path.display())) })?; + let grants = self.provenance.grants_for( + &name, + disk_manifest.info().version(), + &wasm_bytes, + &manifest_bytes, + ); let extism_manifest = extism::Manifest::new([extism::Wasm::data(wasm_bytes)]); - let host_functions = build_host_functions(manifest, &self.shared_resources); + let host_functions = + build_host_functions_with_grants(&disk_manifest, &self.shared_resources, grants); let plugin = extism::Plugin::new(&extism_manifest, host_functions, true) .map_err(|e| DomainError::PluginError(format!("failed to load plugin: {e}")))?; let loaded = LoadedPlugin { - manifest: manifest.clone(), + manifest: disk_manifest, plugin: std::sync::Arc::new(std::sync::Mutex::new(plugin)), enabled: true, }; @@ -455,78 +557,47 @@ impl PluginLoader for ExtismPluginLoader { } fn load_from_dir(&self, dir: &std::path::Path) -> Result<(), DomainError> { - let (manifest, _wasm_path) = parse_manifest(dir)?; - let name = manifest.info().name().to_string(); - - // Per-plugin install coordination: - // - // 1. Bump the refcount up-front so the watcher starts skipping - // events *immediately*, before any filesystem mutation. - // 2. Take the per-plugin serializer lock so concurrent installs - // of the same plugin name can't interleave on the staging - // and destination directories. - // - // The `InstallInFlight` guard decrements the refcount on drop; the - // local `_serializer_guard` releases the lock on drop. Both run - // even on error or panic, so the state never leaks. - let state = self.get_or_create_install_state(&name); - state.count.fetch_add(1, Ordering::SeqCst); - let _in_flight = InstallInFlight { - state: state.clone(), - }; - let _serializer_guard = state - .serializer - .lock() - .unwrap_or_else(|poisoned| poisoned.into_inner()); - - // Ensure any prior in-memory instance is cleared before we touch - // the filesystem. Doing this inside the suppression window (after - // the refcount bump) closes the gap that would otherwise let a - // delayed watcher event re-insert the plugin between an external - // `unload()` call and the start of this function, causing the - // final `self.load()` below to fail with `AlreadyExists`. - // - // Only swallow `NotFound` — other errors (poisoned mutex, etc.) - // must abort the install to avoid leaving the in-memory state - // half-mutated. - match self.unload(&name) { - Ok(()) | Err(DomainError::NotFound(_)) => {} - Err(e) => return Err(e), - } - - // Copy staged files to the permanent plugins directory - let dest_dir = self.plugins_dir.join(&name); - if dest_dir.exists() { - std::fs::remove_dir_all(&dest_dir).map_err(|e| { - DomainError::PluginError(format!( - "failed to remove existing plugin dir '{}': {e}", - dest_dir.display() - )) - })?; - } - std::fs::create_dir_all(&dest_dir) - .map_err(|e| DomainError::PluginError(format!("failed to create plugin dir: {e}")))?; + self.install_from_dir(dir, None) + } - for entry in std::fs::read_dir(dir) - .map_err(|e| DomainError::PluginError(format!("failed to read staging dir: {e}")))? - { - let entry = entry - .map_err(|e| DomainError::PluginError(format!("staging dir entry error: {e}")))?; - let src = entry.path(); - if src.is_file() { - let dest = dest_dir.join(entry.file_name()); - std::fs::copy(&src, &dest).map_err(|e| { - DomainError::PluginError(format!( - "failed to copy {} → {}: {e}", - src.display(), - dest.display() - )) - })?; - } - } + fn load_official_from_dir( + &self, + dir: &Path, + provenance: &OfficialPluginProvenance, + ) -> Result<(), DomainError> { + self.install_from_dir(dir, Some(provenance)) + } +} - self.load(&manifest) +fn verify_provenance( + dir: &Path, + manifest: &PluginManifest, + provenance: &OfficialPluginProvenance, +) -> Result<(), DomainError> { + use sha2::{Digest, Sha256}; + + if manifest.info().name() != provenance.name || manifest.info().version() != provenance.version + { + return Err(DomainError::ValidationError( + "official plugin provenance does not match manifest identity".into(), + )); + } + let wasm = std::fs::read(find_wasm_file(dir)?).map_err(|error| { + DomainError::PluginError(format!("failed to read verified plugin wasm: {error}")) + })?; + let manifest_bytes = std::fs::read(dir.join("plugin.toml")).map_err(|error| { + DomainError::PluginError(format!("failed to read verified plugin manifest: {error}")) + })?; + let wasm_digest = hex::encode(Sha256::digest(&wasm)); + let manifest_digest = hex::encode(Sha256::digest(&manifest_bytes)); + if !wasm_digest.eq_ignore_ascii_case(&provenance.wasm_sha256) + || !manifest_digest.eq_ignore_ascii_case(&provenance.manifest_sha256) + { + return Err(DomainError::PluginError( + "official plugin checksum changed before load".into(), + )); } + Ok(()) } /// Returns `true` if the plugin error message indicates an adaptive-only stream. @@ -588,6 +659,30 @@ description = "Test plugin" wf.write_all(wasm_bytes).unwrap(); } + fn setup_ytdlp_importing_plugin(dir: &Path, name: &str) -> (Vec, Vec) { + std::fs::create_dir_all(dir).unwrap(); + let manifest = format!( + r#"[plugin] +name = "{name}" +version = "1.0.0" +category = "crawler" +author = "tester" +description = "Test plugin" + +[capabilities] +subprocess = ["yt-dlp"] +"# + ) + .into_bytes(); + let wasm = br#"(module + (import "extism:host/user" "run_ytdlp" (func (param i64) (result i64))) +)"# + .to_vec(); + std::fs::write(dir.join("plugin.toml"), &manifest).unwrap(); + std::fs::write(dir.join(format!("{name}.wasm")), &wasm).unwrap(); + (wasm, manifest) + } + #[test] fn test_overlapping_installs_keep_suppression_active_until_last_drop() { // The reason we track a refcount rather than a boolean flag: two @@ -799,4 +894,118 @@ description = "Test plugin" .exists() ); } + + #[test] + fn test_load_official_from_dir_grants_ytdlp_for_verified_files() { + use sha2::{Digest, Sha256}; + + let tmp = TempDir::new().unwrap(); + let plugins_dir = tmp.path().join("plugins"); + let staged = tmp.path().join("staging").join("vortex-mod-youtube"); + let (wasm, manifest) = setup_ytdlp_importing_plugin(&staged, "vortex-mod-youtube"); + let loader = + ExtismPluginLoader::new(plugins_dir, Arc::new(SharedHostResources::new())).unwrap(); + let provenance = + crate::domain::ports::driven::plugin_store_client::OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: hex::encode(Sha256::digest(&wasm)), + manifest_sha256: hex::encode(Sha256::digest(&manifest)), + }; + + let result = loader.load_official_from_dir(&staged, &provenance); + + assert!( + result.is_ok(), + "official verified plugin should load: {result:?}" + ); + assert!(loader.registry().contains("vortex-mod-youtube")); + } + + #[test] + fn test_local_official_name_does_not_receive_ytdlp_grant() { + let tmp = TempDir::new().unwrap(); + let plugin_dir = tmp.path().join("vortex-mod-youtube"); + setup_ytdlp_importing_plugin(&plugin_dir, "vortex-mod-youtube"); + let loader = ExtismPluginLoader::new( + tmp.path().to_path_buf(), + Arc::new(SharedHostResources::new()), + ) + .unwrap(); + let (manifest, _) = parse_manifest(&plugin_dir).unwrap(); + + let result = loader.load(&manifest); + + assert!(result.is_err()); + assert!(!loader.registry().contains("vortex-mod-youtube")); + } + + #[test] + fn test_reload_revalidates_wasm_before_restoring_ytdlp_grant() { + use sha2::{Digest, Sha256}; + + let tmp = TempDir::new().unwrap(); + let plugins_dir = tmp.path().join("plugins"); + let staged = tmp.path().join("staging").join("vortex-mod-youtube"); + let (wasm, manifest_bytes) = setup_ytdlp_importing_plugin(&staged, "vortex-mod-youtube"); + let loader = + ExtismPluginLoader::new(plugins_dir.clone(), Arc::new(SharedHostResources::new())) + .unwrap(); + let provenance = + crate::domain::ports::driven::plugin_store_client::OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: hex::encode(Sha256::digest(&wasm)), + manifest_sha256: hex::encode(Sha256::digest(&manifest_bytes)), + }; + loader.load_official_from_dir(&staged, &provenance).unwrap(); + + let installed = plugins_dir + .join("vortex-mod-youtube") + .join("vortex-mod-youtube.wasm"); + let mut tampered = wasm; + tampered.push(b'\n'); + std::fs::write(installed, tampered).unwrap(); + loader.unload("vortex-mod-youtube").unwrap(); + let (manifest, _) = parse_manifest(&plugins_dir.join("vortex-mod-youtube")).unwrap(); + + let result = loader.load(&manifest); + + assert!(result.is_err()); + assert!(!loader.registry().contains("vortex-mod-youtube")); + } + + #[test] + fn test_startup_load_reuses_persisted_provenance_after_checksum_revalidation() { + use sha2::{Digest, Sha256}; + + let tmp = TempDir::new().unwrap(); + let plugins_dir = tmp.path().join("plugins"); + let staged = tmp.path().join("staging").join("vortex-mod-youtube"); + let (wasm, manifest_bytes) = setup_ytdlp_importing_plugin(&staged, "vortex-mod-youtube"); + let provenance = + crate::domain::ports::driven::plugin_store_client::OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: hex::encode(Sha256::digest(&wasm)), + manifest_sha256: hex::encode(Sha256::digest(&manifest_bytes)), + }; + { + let loader = + ExtismPluginLoader::new(plugins_dir.clone(), Arc::new(SharedHostResources::new())) + .unwrap(); + loader.load_official_from_dir(&staged, &provenance).unwrap(); + } + let loader = + ExtismPluginLoader::new(plugins_dir.clone(), Arc::new(SharedHostResources::new())) + .unwrap(); + let (manifest, _) = parse_manifest(&plugins_dir.join("vortex-mod-youtube")).unwrap(); + + let result = loader.load(&manifest); + + assert!( + result.is_ok(), + "startup load should retain grant: {result:?}" + ); + } } diff --git a/src-tauri/src/adapters/driven/plugin/manifest.rs b/src-tauri/src/adapters/driven/plugin/manifest.rs index c46f4e98..160b4b6f 100644 --- a/src-tauri/src/adapters/driven/plugin/manifest.rs +++ b/src-tauri/src/adapters/driven/plugin/manifest.rs @@ -69,14 +69,28 @@ pub fn parse_manifest(dir: &Path) -> Result<(PluginManifest, PathBuf), DomainErr /// and `repository` even when the binary is unusable. pub fn parse_manifest_metadata(dir: &Path) -> Result { let toml_path = dir.join("plugin.toml"); - let content = std::fs::read_to_string(&toml_path).map_err(|e| { + let content = std::fs::read(&toml_path).map_err(|e| { DomainError::PluginError(format!( "failed to read plugin.toml at {}: {e}", toml_path.display() )) })?; + parse_manifest_metadata_bytes(dir, &content) +} + +pub(super) fn parse_manifest_metadata_bytes( + dir: &Path, + content: &[u8], +) -> Result { + let toml_path = dir.join("plugin.toml"); + let content = std::str::from_utf8(content).map_err(|e| { + DomainError::PluginError(format!( + "plugin.toml at {} is not valid UTF-8: {e}", + toml_path.display() + )) + })?; - let raw: RawManifest = toml::from_str(&content).map_err(|e| { + let raw: RawManifest = toml::from_str(content).map_err(|e| { DomainError::PluginError(format!( "invalid plugin.toml at {}: {e}", toml_path.display() diff --git a/src-tauri/src/adapters/driven/plugin/mod.rs b/src-tauri/src/adapters/driven/plugin/mod.rs index 1f7d65e9..dc5d7a02 100644 --- a/src-tauri/src/adapters/driven/plugin/mod.rs +++ b/src-tauri/src/adapters/driven/plugin/mod.rs @@ -4,6 +4,7 @@ pub mod extism_loader; pub mod github_store_client; pub mod host_functions; pub mod manifest; +mod provenance; pub mod registry; pub mod watcher; pub(crate) mod ytdlp_broker; diff --git a/src-tauri/src/adapters/driven/plugin/provenance.rs b/src-tauri/src/adapters/driven/plugin/provenance.rs new file mode 100644 index 00000000..3a8a7f31 --- /dev/null +++ b/src-tauri/src/adapters/driven/plugin/provenance.rs @@ -0,0 +1,272 @@ +use std::collections::HashMap; +use std::io::Write; +use std::path::PathBuf; +use std::sync::Mutex; + +use serde::{Deserialize, Serialize}; +use sha2::{Digest, Sha256}; + +use crate::domain::error::DomainError; +use crate::domain::ports::driven::plugin_store_client::OfficialPluginProvenance; + +use super::capabilities::HostFunctionGrants; + +#[derive(Clone, Debug, Deserialize, Serialize)] +struct PersistedProvenance { + version: String, + wasm_sha256: String, + manifest_sha256: String, +} + +#[derive(Default, Deserialize, Serialize)] +struct ProvenanceFile { + plugins: HashMap, +} + +/// Host-owned trust state. Its path is supplied by the loader and must live +/// outside the directory writable through plugin installation/hot reload. +pub(super) struct OfficialProvenanceStore { + path: PathBuf, + entries: Mutex>, +} + +impl OfficialProvenanceStore { + pub(super) fn new(path: PathBuf) -> Result { + let entries = match std::fs::read(&path) { + Ok(bytes) => match serde_json::from_slice::(&bytes) { + Ok(file) => file.plugins, + Err(error) => { + tracing::warn!( + path = %path.display(), + %error, + "ignoring invalid plugin provenance state" + ); + HashMap::new() + } + }, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => HashMap::new(), + Err(error) => { + return Err(DomainError::PluginError(format!( + "failed to read plugin provenance '{}': {error}", + path.display() + ))); + } + }; + Ok(Self { + path, + entries: Mutex::new(entries), + }) + } + + pub(super) fn record(&self, provenance: &OfficialPluginProvenance) -> Result<(), DomainError> { + validate_digest(&provenance.wasm_sha256)?; + validate_digest(&provenance.manifest_sha256)?; + let mut entries = self + .entries + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let mut updated_entries = entries.clone(); + updated_entries.insert( + provenance.name.clone(), + PersistedProvenance { + version: provenance.version.clone(), + wasm_sha256: provenance.wasm_sha256.to_ascii_lowercase(), + manifest_sha256: provenance.manifest_sha256.to_ascii_lowercase(), + }, + ); + self.persist(&updated_entries)?; + *entries = updated_entries; + Ok(()) + } + + pub(super) fn grants_for( + &self, + name: &str, + version: &str, + wasm_bytes: &[u8], + manifest_bytes: &[u8], + ) -> HostFunctionGrants { + let entries = self + .entries + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let verified = entries.get(name).is_some_and(|entry| { + entry.version == version + && entry.wasm_sha256 == digest(wasm_bytes) + && entry.manifest_sha256 == digest(manifest_bytes) + }); + HostFunctionGrants { ytdlp: verified } + } + + pub(super) fn revoke(&self, name: &str) -> Result<(), DomainError> { + let mut entries = self + .entries + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + if entries.remove(name).is_some() { + self.persist(&entries)?; + } + Ok(()) + } + + fn persist(&self, entries: &HashMap) -> Result<(), DomainError> { + if let Some(parent) = self.path.parent() { + std::fs::create_dir_all(parent).map_err(|error| { + DomainError::PluginError(format!( + "failed to create plugin provenance directory '{}': {error}", + parent.display() + )) + })?; + } + let payload = serde_json::to_vec_pretty(&ProvenanceFile { + plugins: entries.clone(), + }) + .map_err(|error| DomainError::PluginError(format!("provenance encode failed: {error}")))?; + let mut options = std::fs::OpenOptions::new(); + options.create(true).truncate(true).write(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.mode(0o600); + } + let mut file = options.open(&self.path).map_err(|error| { + DomainError::PluginError(format!( + "failed to write plugin provenance '{}': {error}", + self.path.display() + )) + })?; + file.write_all(&payload).map_err(|error| { + DomainError::PluginError(format!( + "failed to write plugin provenance '{}': {error}", + self.path.display() + )) + })?; + #[cfg(unix)] + std::fs::set_permissions(&self.path, std::fs::Permissions::from_mode(0o600)).map_err( + |error| { + DomainError::PluginError(format!( + "failed to secure plugin provenance '{}': {error}", + self.path.display() + )) + }, + )?; + Ok(()) + } +} + +#[cfg(unix)] +use std::os::unix::fs::PermissionsExt; + +fn digest(bytes: &[u8]) -> String { + hex::encode(Sha256::digest(bytes)) +} + +fn validate_digest(value: &str) -> Result<(), DomainError> { + if value.len() == 64 && value.bytes().all(|byte| byte.is_ascii_hexdigit()) { + Ok(()) + } else { + Err(DomainError::ValidationError( + "official plugin provenance requires a 64-character SHA-256 digest".into(), + )) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use tempfile::TempDir; + + #[test] + fn test_verified_provenance_persists_and_grants_unchanged_plugin() { + let temp = TempDir::new().unwrap(); + let state_path = temp + .path() + .join("host-state") + .join("plugin-provenance.json"); + let wasm = b"official wasm"; + let manifest = b"official manifest"; + let provenance = OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: digest(wasm), + manifest_sha256: digest(manifest), + }; + + OfficialProvenanceStore::new(state_path.clone()) + .unwrap() + .record(&provenance) + .unwrap(); + let reloaded = OfficialProvenanceStore::new(state_path).unwrap(); + + let grants = reloaded.grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest); + assert!(grants.ytdlp); + } + + #[test] + fn test_verified_provenance_denies_tampered_wasm() { + let temp = TempDir::new().unwrap(); + let state_path = temp.path().join("plugin-provenance.json"); + let wasm = b"official wasm"; + let manifest = b"official manifest"; + let provenance = OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: digest(wasm), + manifest_sha256: digest(manifest), + }; + let store = OfficialProvenanceStore::new(state_path).unwrap(); + store.record(&provenance).unwrap(); + + let grants = store.grants_for("vortex-mod-youtube", "1.0.0", b"tampered wasm", manifest); + + assert!(!grants.ytdlp); + } + + #[test] + fn test_verified_provenance_denies_tampered_manifest() { + let temp = TempDir::new().unwrap(); + let state_path = temp.path().join("plugin-provenance.json"); + let wasm = b"official wasm"; + let manifest = b"official manifest"; + let provenance = OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: digest(wasm), + manifest_sha256: digest(manifest), + }; + let store = OfficialProvenanceStore::new(state_path).unwrap(); + store.record(&provenance).unwrap(); + + let grants = store.grants_for("vortex-mod-youtube", "1.0.0", wasm, b"tampered manifest"); + + assert!(!grants.ytdlp); + } + + #[test] + fn test_failed_persistence_does_not_leave_an_in_memory_grant() { + let temp = TempDir::new().unwrap(); + let state_dir = temp.path().join("host-state"); + std::fs::create_dir(&state_dir).unwrap(); + let state_path = state_dir.join("plugin-provenance.json"); + let wasm = b"official wasm"; + let manifest = b"official manifest"; + let provenance = OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: digest(wasm), + manifest_sha256: digest(manifest), + }; + let store = OfficialProvenanceStore::new(state_path).unwrap(); + std::fs::rename(&state_dir, temp.path().join("moved-host-state")).unwrap(); + std::fs::write(&state_dir, b"file").unwrap(); + + let result = store.record(&provenance); + + assert!(result.is_err()); + assert!( + !store + .grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest) + .ytdlp + ); + } +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker.rs index 9c974e60..a4933bd7 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker.rs @@ -12,7 +12,7 @@ mod validation; #[cfg(test)] mod tests; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::time::Duration; use anyhow::bail; @@ -20,6 +20,8 @@ use serde::{Deserialize, Serialize}; pub(super) const DEFAULT_TIMEOUT: Duration = Duration::from_secs(60); pub(super) const DOWNLOAD_TIMEOUT: Duration = Duration::from_secs(30 * 60); +pub(super) const DEFAULT_OUTPUT_LIMIT: usize = 1024 * 1024; +pub(super) const METADATA_OUTPUT_LIMIT: usize = 16 * 1024 * 1024; #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(super) enum YtDlpProvider { @@ -74,6 +76,31 @@ pub(super) struct PreparedCommand { pub(super) args: Vec, pub(super) working_dir: PathBuf, pub(super) timeout: Duration, + pub(super) stdout_limit: usize, + pub(super) stderr_limit: usize, + pub(super) cleanup_working_dir_on_failure: bool, +} + +pub(crate) struct ManagedOutputRequest { + path: PathBuf, +} + +impl ManagedOutputRequest { + pub(crate) fn path(&self) -> &Path { + &self.path + } +} + +impl Drop for ManagedOutputRequest { + fn drop(&mut self) { + if let Err(error) = validation::cleanup_private_request_dir(&self.path) { + tracing::warn!( + path = %self.path.display(), + error = %error, + "failed to clean private yt-dlp request directory" + ); + } + } } pub(crate) fn supports_plugin(plugin_name: &str) -> bool { @@ -85,7 +112,7 @@ pub(crate) fn run_plugin_request( request: PluginYtDlpRequest, ) -> anyhow::Result { let provider = provider_for_plugin(plugin_name)?; - process::execute(request::prepare(provider, request, &std::env::temp_dir())?) + process::execute(request::prepare(provider, request, &managed_temp_root()?)?) } pub(crate) fn run_legacy_request( @@ -95,7 +122,7 @@ pub(crate) fn run_legacy_request( process::execute(legacy::prepare( plugin_name, request, - &std::env::temp_dir(), + &managed_temp_root()?, )?) } @@ -107,10 +134,30 @@ pub(crate) fn run_generic_metadata(url: String) -> anyhow::Result process::execute(request::prepare( YtDlpProvider::Generic, request, - &std::env::temp_dir(), + &managed_temp_root()?, )?) } +pub(crate) fn managed_output_request() -> anyhow::Result { + let temp_root = managed_temp_root()?; + let output_root = temp_root.join("vortex-downloads"); + validation::ensure_private_root(&output_root)?; + Ok(ManagedOutputRequest { + path: validation::create_private_child(&output_root, "request-")?, + }) +} + +fn managed_temp_root() -> anyhow::Result { + let cache = dirs::cache_dir() + .ok_or_else(|| anyhow::anyhow!("run_ytdlp: user cache directory is unavailable"))?; + let cache = validation::ensure_owned_cache_directory(&cache)?; + let app_root = cache.join("vortex"); + validation::ensure_private_root(&app_root)?; + let broker_root = app_root.join("ytdlp"); + validation::ensure_private_root(&broker_root)?; + Ok(broker_root) +} + pub(super) fn provider_for_plugin(plugin_name: &str) -> anyhow::Result { match plugin_name { "vortex-mod-youtube" => Ok(YtDlpProvider::Youtube), diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy.rs index 970d9f27..3a7c7a06 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy.rs @@ -5,8 +5,8 @@ use anyhow::bail; use super::request::{append_url, prepare as prepare_typed, secure_prefix, strings}; use super::validation::validate_url; use super::{ - DEFAULT_TIMEOUT, LegacySubprocessRequest, PluginYtDlpRequest, PreparedCommand, YtDlpProvider, - legacy_download, provider_for_plugin, + DEFAULT_OUTPUT_LIMIT, DEFAULT_TIMEOUT, LegacySubprocessRequest, PluginYtDlpRequest, + PreparedCommand, YtDlpProvider, legacy_download, provider_for_plugin, }; pub(super) fn prepare( @@ -86,8 +86,11 @@ fn rebuild_resolve( append_url(&mut rebuilt, validate_url(provider, &args[6])?); Ok(PreparedCommand { args: rebuilt, - working_dir: temp_root.join("vortex-ytdlp"), + working_dir: temp_root.to_path_buf(), timeout: DEFAULT_TIMEOUT, + stdout_limit: DEFAULT_OUTPUT_LIMIT, + stderr_limit: DEFAULT_OUTPUT_LIMIT, + cleanup_working_dir_on_failure: false, }) } diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy_download.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy_download.rs index 89b0bbf9..b781eb36 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy_download.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/legacy_download.rs @@ -4,8 +4,8 @@ use anyhow::bail; use super::legacy::validate_selector; use super::request::secure_prefix; -use super::validation::{private_output_dir, validate_format, validate_url}; -use super::{DOWNLOAD_TIMEOUT, PreparedCommand, YtDlpProvider}; +use super::validation::{private_output_dir, validate_url}; +use super::{DEFAULT_OUTPUT_LIMIT, DOWNLOAD_TIMEOUT, PreparedCommand, YtDlpProvider}; struct LegacyProfile<'a> { selector: Option<&'a str>, @@ -23,8 +23,8 @@ pub(super) fn prepare( if let Some(selector) = profile.selector { validate_selector(selector)?; } - let output_dir = parse_output_dir(profile.output_template, temp_root)?; let url = validate_url(provider, profile.url)?; + let output_dir = parse_output_dir(profile.output_template, temp_root)?; let mut rebuilt = secure_prefix(); let offset = rebuilt.len(); rebuilt.extend_from_slice(args); @@ -35,6 +35,9 @@ pub(super) fn prepare( args: rebuilt, working_dir: output_dir, timeout: DOWNLOAD_TIMEOUT, + stdout_limit: DEFAULT_OUTPUT_LIMIT, + stderr_limit: DEFAULT_OUTPUT_LIMIT, + cleanup_working_dir_on_failure: true, }) } @@ -59,7 +62,7 @@ fn match_profile(provider: YtDlpProvider, args: &[String]) -> anyhow::Result { - validate_format(Some(args[2].clone()))?; + validate_audio_format(&args[2])?; Ok(LegacyProfile { selector: None, output_template: &args[4], @@ -71,6 +74,16 @@ fn match_profile(provider: YtDlpProvider, args: &[String]) -> anyhow::Result anyhow::Result<()> { + if !matches!( + format, + "aac" | "alac" | "best" | "flac" | "m4a" | "mp3" | "opus" | "vorbis" | "wav" + ) { + bail!("run_subprocess compatibility: audio format is not approved"); + } + Ok(()) +} + fn youtube_profile(args: &[String]) -> bool { args.len() == 19 && args[0] == "--format" diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/output.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/output.rs index 82daa9f9..a2a279c6 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/output.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/output.rs @@ -3,8 +3,6 @@ use std::thread::JoinHandle; use anyhow::bail; -const MAX_OUTPUT_BYTES: usize = 1024 * 1024; - pub(super) struct CapturedOutput { pub(super) bytes: Vec, pub(super) truncated: bool, @@ -32,12 +30,12 @@ pub(super) fn read_stream_capped( } } -pub(super) fn spawn_reader(stream: Option) -> ReaderHandle +pub(super) fn spawn_reader(stream: Option, max_bytes: usize) -> ReaderHandle where T: Read + Send + 'static, { std::thread::spawn(move || match stream { - Some(stream) => read_stream_capped(stream, MAX_OUTPUT_BYTES), + Some(stream) => read_stream_capped(stream, max_bytes), None => Ok(CapturedOutput { bytes: Vec::new(), truncated: false, @@ -48,6 +46,8 @@ where pub(super) fn collect( stdout: ReaderHandle, stderr: ReaderHandle, + stdout_limit: usize, + stderr_limit: usize, ) -> anyhow::Result<(String, String)> { let stdout = stdout.join(); let stderr = stderr.join(); @@ -55,22 +55,31 @@ pub(super) fn collect( stdout.map_err(|_| anyhow::anyhow!("run_ytdlp: stdout reader thread panicked"))??; let stderr = stderr.map_err(|_| anyhow::anyhow!("run_ytdlp: stderr reader thread panicked"))??; - decode("stdout", stdout, "stderr", stderr) + decode( + "stdout", + stdout, + stdout_limit, + "stderr", + stderr, + stderr_limit, + ) } fn decode( first_name: &str, first: CapturedOutput, + first_limit: usize, second_name: &str, second: CapturedOutput, + second_limit: usize, ) -> anyhow::Result<(String, String)> { if first.truncated || second.truncated { - let stream = if first.truncated { - first_name + let (stream, limit) = if first.truncated { + (first_name, first_limit) } else { - second_name + (second_name, second_limit) }; - bail!("run_ytdlp: {stream} exceeded the {MAX_OUTPUT_BYTES}-byte limit"); + bail!("run_ytdlp: {stream} exceeded the {limit}-byte limit"); } Ok(( String::from_utf8_lossy(&first.bytes).into_owned(), diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/platform.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/platform.rs index 7cfea2be..5a74caf7 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/platform.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/platform.rs @@ -92,46 +92,126 @@ fn valid_binary(path: &Path, roots: &[PathBuf]) -> anyhow::Result { return Ok(false); } let metadata = std::fs::metadata(path)?; - if !metadata.is_file() || !inside_approved_root(path, roots) { + let Some(root) = approved_root_for(path, roots) else { + return Ok(false); + }; + if !metadata.is_file() { return Ok(false); } #[cfg(unix)] { - use std::os::unix::fs::PermissionsExt; + use std::os::unix::fs::{MetadataExt, PermissionsExt}; let mode = metadata.permissions().mode(); - if mode & 0o111 == 0 || mode & 0o022 != 0 { + if mode & 0o111 == 0 + || mode & 0o022 != 0 + || !trusted_owner(metadata.uid()) + || !trusted_ancestor_chain(path, &root)? + { return Ok(false); } } Ok(true) } -fn inside_approved_root(path: &Path, roots: &[PathBuf]) -> bool { - roots.iter().any(|root| { - let canonical = std::fs::canonicalize(root).unwrap_or_else(|_| root.clone()); - path.starts_with(canonical) - }) +fn approved_root_for(path: &Path, roots: &[PathBuf]) -> Option { + roots + .iter() + .filter_map(|root| std::fs::canonicalize(root).ok()) + .filter(|root| path.starts_with(root)) + .max_by_key(|root| root.components().count()) +} + +#[cfg(unix)] +fn trusted_ancestor_chain(path: &Path, root: &Path) -> anyhow::Result { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let Some(mut current) = path.parent() else { + return Ok(false); + }; + loop { + let metadata = std::fs::metadata(current)?; + if !metadata.is_dir() + || !trusted_owner(metadata.uid()) + || metadata.permissions().mode() & 0o022 != 0 + { + return Ok(false); + } + if current == root { + return Ok(true); + } + let Some(parent) = current.parent() else { + return Ok(false); + }; + current = parent; + } +} + +#[cfg(unix)] +fn trusted_owner(uid: u32) -> bool { + uid == 0 || uid == unsafe { libc::geteuid() } } pub(super) fn controlled_path(binary: &Path) -> anyhow::Result { + controlled_path_with_roots(binary, &approved_roots()) +} + +pub(super) fn controlled_path_with_roots( + binary: &Path, + roots: &[PathBuf], +) -> anyhow::Result { let mut paths = Vec::new(); - if let Some(parent) = binary.parent() { - paths.push(parent.to_path_buf()); + if let Some(parent) = binary.parent() + && let Some(parent) = trusted_directory(parent, roots) + { + paths.push(parent); } + #[cfg(unix)] paths.extend( - candidates() - .into_iter() - .filter_map(|path| path.parent().map(Path::to_path_buf)), + [ + "/opt/homebrew/bin", + "/usr/local/bin", + "/usr/bin", + "/run/current-system/sw/bin", + "/nix/var/nix/profiles/default/bin", + ] + .into_iter() + .filter_map(|path| trusted_directory(Path::new(path), roots)), ); #[cfg(windows)] if let Some(root) = std::env::var_os("SystemRoot").map(PathBuf::from) { - paths.extend([root.join("System32"), root]); + paths.extend( + [root.join("System32"), root] + .into_iter() + .filter_map(|path| trusted_directory(&path, roots)), + ); } paths.sort(); paths.dedup(); std::env::join_paths(paths).context("run_ytdlp: approved PATH cannot be encoded") } +fn trusted_directory(path: &Path, roots: &[PathBuf]) -> Option { + let canonical = std::fs::canonicalize(path).ok()?; + let metadata = std::fs::metadata(&canonical).ok()?; + if !metadata.is_dir() { + return None; + } + #[cfg(unix)] + { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + let root = approved_root_for(&canonical, roots)?; + if !trusted_owner(metadata.uid()) + || metadata.permissions().mode() & 0o022 != 0 + || !trusted_ancestor_chain(&canonical.join(".vortex-helper-check"), &root).ok()? + { + return None; + } + } + #[cfg(not(unix))] + let _ = roots; + Some(canonical) +} + #[cfg(windows)] pub(super) fn copy_required_environment(command: &mut Command) { for name in ["SystemRoot", "WINDIR"] { diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/process.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/process.rs index c6d15a11..b94e76ce 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/process.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/process.rs @@ -5,38 +5,94 @@ use std::time::{Duration, Instant}; use anyhow::{Context, bail}; use command_group::{CommandGroup, GroupChild}; +#[cfg(test)] +use super::DEFAULT_OUTPUT_LIMIT; use super::output::{self, ReaderHandle}; use super::platform; +use super::validation; use super::{PreparedCommand, YtDlpResponse}; const PROCESS_POLL_INTERVAL: Duration = Duration::from_millis(25); pub(super) fn execute(prepared: PreparedCommand) -> anyhow::Result { - std::fs::create_dir_all(&prepared.working_dir).with_context(|| { - format!( - "run_ytdlp: failed to create working directory '{}'", - prepared.working_dir.display() + execute_with_discovery(prepared, platform::discover_ytdlp) +} + +fn execute_with_discovery( + prepared: PreparedCommand, + discover: impl FnOnce() -> anyhow::Result, +) -> anyhow::Result { + let result = (|| { + std::fs::create_dir_all(&prepared.working_dir).with_context(|| { + format!( + "run_ytdlp: failed to create working directory '{}'", + prepared.working_dir.display() + ) + })?; + run_process_capped( + &discover()?, + &prepared.args, + &prepared.working_dir, + prepared.timeout, + prepared.stdout_limit, + prepared.stderr_limit, ) - })?; - run_process( - &platform::discover_ytdlp()?, - &prepared.args, - &prepared.working_dir, - prepared.timeout, - ) + })(); + + let failed = result + .as_ref() + .map_or(true, |response| response.exit_code != 0); + if prepared.cleanup_working_dir_on_failure + && failed + && let Err(error) = validation::cleanup_private_job_dir(&prepared.working_dir) + { + tracing::warn!( + path = %prepared.working_dir.display(), + error = %error, + "failed to clean unsuccessful yt-dlp job directory" + ); + } + result +} + +#[cfg(test)] +pub(super) fn execute_with_test_discovery( + prepared: PreparedCommand, + discover: impl FnOnce() -> anyhow::Result, +) -> anyhow::Result { + execute_with_discovery(prepared, discover) } +#[cfg(test)] pub(super) fn run_process( binary: &Path, args: &[String], working_dir: &Path, timeout: Duration, +) -> anyhow::Result { + run_process_capped( + binary, + args, + working_dir, + timeout, + DEFAULT_OUTPUT_LIMIT, + DEFAULT_OUTPUT_LIMIT, + ) +} + +fn run_process_capped( + binary: &Path, + args: &[String], + working_dir: &Path, + timeout: Duration, + stdout_limit: usize, + stderr_limit: usize, ) -> anyhow::Result { let mut child = spawn_group(binary, args, working_dir)?; - let stdout = output::spawn_reader(child.inner().stdout.take()); - let stderr = output::spawn_reader(child.inner().stderr.take()); + let stdout = output::spawn_reader(child.inner().stdout.take(), stdout_limit); + let stderr = output::spawn_reader(child.inner().stderr.take(), stderr_limit); let status = wait_for_group(&mut child, &stdout, &stderr, timeout); - let output = output::collect(stdout, stderr); + let output = output::collect(stdout, stderr, stdout_limit, stderr_limit); let status = status?; let (stdout, stderr) = output?; Ok(YtDlpResponse { diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/request.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/request.rs index 558cbd37..e5e758b8 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/request.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/request.rs @@ -5,7 +5,8 @@ use anyhow::bail; use super::selectors::{build_direct_selector, build_download_args}; use super::validation::{private_output_dir, validate_format, validate_quality, validate_url}; use super::{ - DEFAULT_TIMEOUT, DOWNLOAD_TIMEOUT, PluginYtDlpRequest, PreparedCommand, YtDlpProvider, + DEFAULT_OUTPUT_LIMIT, DEFAULT_TIMEOUT, DOWNLOAD_TIMEOUT, METADATA_OUTPUT_LIMIT, + PluginYtDlpRequest, PreparedCommand, YtDlpProvider, }; pub(super) fn prepare( @@ -47,7 +48,12 @@ fn prepare_metadata( let mut args = secure_prefix(); args.extend(metadata_args(provider, playlist)); append_url(&mut args, validate_url(provider, &url)?); - Ok(prepared(args, temp_root, DEFAULT_TIMEOUT)) + Ok(prepared( + args, + temp_root, + DEFAULT_TIMEOUT, + METADATA_OUTPUT_LIMIT, + )) } fn metadata_args(provider: YtDlpProvider, playlist: bool) -> Vec { @@ -80,7 +86,12 @@ fn prepare_resolve( args.extend(strings(["--get-url", "--no-playlist", "--no-warnings"])); args.extend(["--format".to_string(), selector]); append_url(&mut args, validate_url(provider, &url)?); - Ok(prepared(args, temp_root, DEFAULT_TIMEOUT)) + Ok(prepared( + args, + temp_root, + DEFAULT_TIMEOUT, + DEFAULT_OUTPUT_LIMIT, + )) } fn prepare_download( @@ -92,12 +103,17 @@ fn prepare_download( audio_only: bool, temp_root: &Path, ) -> anyhow::Result { + if provider == YtDlpProvider::Generic { + bail!("run_ytdlp: generic provider cannot download files"); + } let format = validate_format(format)?; + let url = validate_url(provider, &url)?; + let quality = validate_quality(quality)?; let output_dir = private_output_dir(&output_dir, temp_root)?; let args = build_download_args( provider, - &validate_url(provider, &url)?, - validate_quality(quality)?, + &url, + quality, format.as_deref(), &output_dir, audio_only, @@ -106,14 +122,25 @@ fn prepare_download( args, working_dir: output_dir, timeout: DOWNLOAD_TIMEOUT, + stdout_limit: DEFAULT_OUTPUT_LIMIT, + stderr_limit: DEFAULT_OUTPUT_LIMIT, + cleanup_working_dir_on_failure: true, }) } -fn prepared(args: Vec, temp_root: &Path, timeout: std::time::Duration) -> PreparedCommand { +fn prepared( + args: Vec, + temp_root: &Path, + timeout: std::time::Duration, + stdout_limit: usize, +) -> PreparedCommand { PreparedCommand { args, - working_dir: temp_root.join("vortex-ytdlp"), + working_dir: temp_root.to_path_buf(), timeout, + stdout_limit, + stderr_limit: DEFAULT_OUTPUT_LIMIT, + cleanup_working_dir_on_failure: false, } } diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/selectors.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/selectors.rs index 7be2ae07..1ea05815 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/selectors.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/selectors.rs @@ -65,10 +65,9 @@ pub(super) fn build_download_args( args.extend(strings(["--extract-audio", "--audio-format"])); args.push(normalize_audio_format(format).to_string()); } else { - args.extend(strings([ - "--merge-output-format", - merge_format(provider, format), - ])); + let container = merge_format(provider, format); + args.extend(strings(["--merge-output-format", container])); + args.extend(strings(["--remux-video", container])); } append_output_args(&mut args, output_dir); append_provider_args(&mut args, provider); diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/audio.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/audio.rs index 3872dd65..bc1cc2f3 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/audio.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/audio.rs @@ -1,10 +1,10 @@ use super::super::{PluginYtDlpRequest, YtDlpProvider, request}; +use super::private_root; #[test] fn vimeo_audio_download_uses_audio_extraction() { let temp = tempfile::tempdir().unwrap(); - let managed = temp.path().join("vortex-downloads"); - std::fs::create_dir_all(&managed).unwrap(); + let managed = private_root(temp.path()); let prepared = request::prepare( YtDlpProvider::Vimeo, PluginYtDlpRequest::Download { @@ -19,5 +19,36 @@ fn vimeo_audio_download_uses_audio_extraction() { .unwrap(); assert!(prepared.args.contains(&"--extract-audio".to_string())); + let audio_format = prepared + .args + .windows(2) + .find(|pair| pair[0] == "--audio-format") + .expect("audio format argument"); + assert_eq!(audio_format[1], "m4a"); assert!(!prepared.args.contains(&"--merge-output-format".to_string())); } + +#[test] +fn video_download_remuxes_premuxed_fallback_to_requested_container() { + let temp = tempfile::tempdir().unwrap(); + let managed = private_root(temp.path()); + let prepared = request::prepare( + YtDlpProvider::Youtube, + PluginYtDlpRequest::Download { + url: "https://www.youtube.com/watch?v=abcdefghijk".to_string(), + quality: Some(1080), + format: Some("mkv".to_string()), + output_dir: managed.to_string_lossy().into_owned(), + audio_only: false, + }, + temp.path(), + ) + .unwrap(); + + assert!( + prepared + .args + .windows(2) + .any(|pair| pair == ["--remux-video", "mkv"]) + ); +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/legacy.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/legacy.rs index 8d70c9a6..fda838ac 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/legacy.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/legacy.rs @@ -1,6 +1,7 @@ use std::path::Path; use super::super::{LegacySubprocessRequest, legacy}; +use super::private_root; #[test] fn rejects_binary_replacement() { @@ -16,7 +17,12 @@ fn rejects_binary_replacement() { #[test] fn rejects_exec_and_config_arguments() { - for dangerous in ["--exec", "--config-locations", "--plugin-dirs"] { + for dangerous in [ + "--exec", + "--config-location", + "--config-locations", + "--plugin-dirs", + ] { let request = LegacySubprocessRequest { binary: "yt-dlp".to_string(), args: vec![dangerous.to_string(), "payload".to_string()], @@ -28,6 +34,38 @@ fn rejects_exec_and_config_arguments() { } } +#[test] +fn rejects_non_audio_soundcloud_format_before_allocating_output() { + let temp = tempfile::tempdir().unwrap(); + let managed = private_root(temp.path()); + let template = format!("{}/%(id)s.%(ext)s", managed.display()); + let request = LegacySubprocessRequest { + binary: "yt-dlp".to_string(), + args: [ + "--extract-audio", + "--audio-format", + "webm", + "--output", + &template, + "--print", + "after_move:%(filepath)s", + "--no-playlist", + "--no-warnings", + "--quiet", + "--", + "https://soundcloud.com/artist/track", + ] + .map(str::to_string) + .to_vec(), + timeout_ms: Some(60_000), + }; + + let error = legacy::prepare("vortex-mod-soundcloud", request, temp.path()) + .expect_err("non-audio format must fail"); + assert!(error.to_string().contains("audio format")); + assert_eq!(std::fs::read_dir(&managed).unwrap().count(), 0); +} + #[test] fn rebuilds_known_youtube_metadata_profile() { let url = "https://www.youtube.com/watch?v=abcdefghijk"; diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/mod.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/mod.rs index 6ac8335e..c26201a8 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/mod.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/mod.rs @@ -3,3 +3,16 @@ mod legacy; mod platform; mod process; mod request; + +fn private_root(temp: &std::path::Path) -> std::path::PathBuf { + let managed = temp.join("vortex-downloads"); + let request = managed.join("request-test"); + std::fs::create_dir_all(&request).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&managed, std::fs::Permissions::from_mode(0o700)).unwrap(); + std::fs::set_permissions(&request, std::fs::Permissions::from_mode(0o700)).unwrap(); + } + request +} diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs index 5bfedb94..4257d2a1 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs @@ -2,11 +2,12 @@ mod unix { use std::os::unix::fs::{PermissionsExt, symlink}; - use super::super::super::platform::find_approved_binary; + use super::super::super::platform::{controlled_path_with_roots, find_approved_binary}; #[test] fn binary_must_resolve_inside_root_with_safe_permissions() { let approved = tempfile::tempdir().unwrap(); + std::fs::set_permissions(approved.path(), std::fs::Permissions::from_mode(0o700)).unwrap(); let outside = tempfile::tempdir().unwrap(); let valid = executable(approved.path(), "yt-dlp", 0o700); let unsafe_binary = executable(outside.path(), "yt-dlp", 0o700); @@ -27,6 +28,32 @@ mod unix { assert!(find_approved_binary(&[binary], &[approved.path().to_path_buf()]).is_err()); } + #[test] + fn binary_below_group_writable_parent_is_rejected() { + let approved = tempfile::tempdir().unwrap(); + let writable_parent = approved.path().join("bin"); + std::fs::create_dir(&writable_parent).unwrap(); + std::fs::set_permissions(&writable_parent, std::fs::Permissions::from_mode(0o770)).unwrap(); + let binary = executable(&writable_parent, "yt-dlp", 0o700); + + assert!(find_approved_binary(&[binary], &[approved.path().to_path_buf()]).is_err()); + } + + #[test] + fn controlled_path_does_not_add_unselected_user_candidate_directories() { + let approved = tempfile::tempdir().unwrap(); + std::fs::set_permissions(approved.path(), std::fs::Permissions::from_mode(0o700)).unwrap(); + let binary = executable(approved.path(), "yt-dlp", 0o700); + let path = controlled_path_with_roots(&binary, &[approved.path().to_path_buf()]).unwrap(); + let entries = std::env::split_paths(&path).collect::>(); + + assert!(entries.contains(&approved.path().to_path_buf())); + if let Some(home) = dirs::home_dir() { + assert!(!entries.contains(&home.join(".local/bin"))); + assert!(!entries.contains(&home.join(".nix-profile/bin"))); + } + } + fn executable(directory: &std::path::Path, name: &str, mode: u32) -> std::path::PathBuf { let path = directory.join(name); std::fs::write(&path, "#!/bin/sh\n").unwrap(); diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/process.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/process.rs index 71752c1a..4f414c4a 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/process.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/process.rs @@ -2,7 +2,10 @@ use std::io::{Cursor, Seek}; use std::time::{Duration, Instant}; use super::super::output::{collect, read_stream_capped, spawn_reader}; -use super::super::process::run_process; +use super::super::process::{execute_with_test_discovery, run_process}; +use super::super::validation; +use super::super::{DEFAULT_OUTPUT_LIMIT, METADATA_OUTPUT_LIMIT, PreparedCommand}; +use super::private_root; #[test] fn output_reader_caps_but_drains_the_stream() { @@ -15,14 +18,64 @@ fn output_reader_caps_but_drains_the_stream() { #[test] fn oversized_output_is_reported_instead_of_returning_truncated_data() { - let stdout = spawn_reader(Some(Cursor::new(vec![b'x'; 1024 * 1024 + 1]))); - let stderr = spawn_reader(Some(Cursor::new(Vec::new()))); + let stdout = spawn_reader( + Some(Cursor::new(vec![b'x'; DEFAULT_OUTPUT_LIMIT + 1])), + DEFAULT_OUTPUT_LIMIT, + ); + let stderr = spawn_reader(Some(Cursor::new(Vec::new())), DEFAULT_OUTPUT_LIMIT); - let error = collect(stdout, stderr).expect_err("oversized output must fail"); + let error = collect(stdout, stderr, DEFAULT_OUTPUT_LIMIT, DEFAULT_OUTPUT_LIMIT) + .expect_err("oversized output must fail"); assert!(error.to_string().contains("stdout exceeded")); } +#[test] +fn metadata_output_accepts_large_playlist_below_its_bounded_cap() { + let stdout = spawn_reader( + Some(Cursor::new(vec![b'x'; DEFAULT_OUTPUT_LIMIT + 1])), + METADATA_OUTPUT_LIMIT, + ); + let stderr = spawn_reader(Some(Cursor::new(Vec::new())), DEFAULT_OUTPUT_LIMIT); + + let (stdout, _) = collect(stdout, stderr, METADATA_OUTPUT_LIMIT, DEFAULT_OUTPUT_LIMIT) + .expect("metadata has a larger bounded allowance"); + assert_eq!(stdout.len(), DEFAULT_OUTPUT_LIMIT + 1); +} + +#[test] +fn metadata_output_still_rejects_data_above_its_cap() { + let stdout = spawn_reader( + Some(Cursor::new(vec![b'x'; METADATA_OUTPUT_LIMIT + 1])), + METADATA_OUTPUT_LIMIT, + ); + let stderr = spawn_reader(Some(Cursor::new(Vec::new())), DEFAULT_OUTPUT_LIMIT); + + collect(stdout, stderr, METADATA_OUTPUT_LIMIT, DEFAULT_OUTPUT_LIMIT) + .expect_err("metadata output must remain bounded"); +} + +#[test] +fn discovery_failure_cleans_partial_download_directory() { + let temp = tempfile::tempdir().unwrap(); + let request = private_root(temp.path()); + let job = validation::create_private_child(&request, "job-").unwrap(); + std::fs::write(job.join("partial.part"), b"partial").unwrap(); + let prepared = PreparedCommand { + args: Vec::new(), + working_dir: job.clone(), + timeout: Duration::from_secs(1), + stdout_limit: DEFAULT_OUTPUT_LIMIT, + stderr_limit: DEFAULT_OUTPUT_LIMIT, + cleanup_working_dir_on_failure: true, + }; + + execute_with_test_discovery(prepared, || Err(anyhow::anyhow!("yt-dlp is missing"))) + .expect_err("discovery must fail"); + + assert!(!job.exists()); +} + #[cfg(unix)] #[test] fn process_uses_controlled_environment_and_working_directory() { diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs index bef486c4..41c86284 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs @@ -1,6 +1,8 @@ use std::path::Path; -use super::super::{PluginYtDlpRequest, YtDlpProvider, request}; +use super::super::{DEFAULT_OUTPUT_LIMIT, METADATA_OUTPUT_LIMIT}; +use super::super::{PluginYtDlpRequest, YtDlpProvider, request, validation}; +use super::private_root; #[test] fn typed_contract_rejects_binary_and_raw_arguments() { @@ -35,6 +37,8 @@ fn metadata_disables_external_configuration_and_code_loading() { "--no-exec".to_string(), "--no-cache-dir".to_string(), ])); + assert_eq!(prepared.stdout_limit, METADATA_OUTPUT_LIMIT); + assert_eq!(prepared.stderr_limit, DEFAULT_OUTPUT_LIMIT); } #[test] @@ -61,6 +65,21 @@ fn provider_host_mismatch_is_rejected() { assert!(error.to_string().contains("YouTube URL")); } +#[test] +fn generic_metadata_rejects_loopback_and_untrusted_hosts() { + for url in [ + "http://127.0.0.1/admin", + "http://[::1]/admin", + "https://attacker.example/video", + "http://youtube.com/watch?v=abcdefghijk", + "https://user:pass@youtube.com/watch?v=abcdefghijk", + "https://youtube.com:8443/watch?v=abcdefghijk", + ] { + request::prepare(YtDlpProvider::Generic, metadata(url), Path::new("/tmp")) + .expect_err("generic metadata must not reach an attacker-controlled host"); + } +} + #[test] fn dangerous_format_value_is_rejected() { let error = request::prepare( @@ -75,9 +94,8 @@ fn dangerous_format_value_is_rejected() { #[test] fn output_directory_must_be_managed_root() { let temp = tempfile::tempdir().unwrap(); - let managed = temp.path().join("vortex-downloads"); + private_root(temp.path()); let outside = temp.path().join("outside"); - std::fs::create_dir_all(&managed).unwrap(); std::fs::create_dir_all(&outside).unwrap(); let error = request::prepare( YtDlpProvider::Youtube, @@ -88,15 +106,14 @@ fn output_directory_must_be_managed_root() { assert!( error .to_string() - .contains("must be the Vortex download root") + .contains("must be a Vortex-managed request root") ); } #[test] fn each_download_gets_a_private_output_directory() { let temp = tempfile::tempdir().unwrap(); - let managed = temp.path().join("vortex-downloads"); - std::fs::create_dir_all(&managed).unwrap(); + let managed = private_root(temp.path()); let output = managed.to_string_lossy(); let first = request::prepare( YtDlpProvider::Youtube, @@ -114,6 +131,118 @@ fn each_download_gets_a_private_output_directory() { assert_eq!(first.working_dir.parent(), Some(managed.as_path())); } +#[test] +fn invalid_download_does_not_allocate_a_private_output_directory() { + let temp = tempfile::tempdir().unwrap(); + let managed = private_root(temp.path()); + let request = PluginYtDlpRequest::Download { + url: "https://attacker.example/video".to_string(), + quality: Some(1080), + format: Some("mp4".to_string()), + output_dir: managed.to_string_lossy().into_owned(), + audio_only: false, + }; + + request::prepare(YtDlpProvider::Youtube, request, temp.path()) + .expect_err("invalid URL must fail"); + assert_eq!(std::fs::read_dir(&managed).unwrap().count(), 0); +} + +#[cfg(unix)] +#[test] +fn group_readable_output_root_is_rejected() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + let managed = temp.path().join("vortex-downloads"); + std::fs::create_dir(&managed).unwrap(); + std::fs::set_permissions(&managed, std::fs::Permissions::from_mode(0o750)).unwrap(); + + request::prepare( + YtDlpProvider::Youtube, + download(&managed.to_string_lossy(), Some("mp4")), + temp.path(), + ) + .expect_err("non-private root must fail"); +} + +#[cfg(unix)] +#[test] +fn host_created_output_root_is_private() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("private-root"); + + validation::ensure_private_root(&root).unwrap(); + + let metadata = std::fs::symlink_metadata(root).unwrap(); + assert_eq!(metadata.uid(), unsafe { libc::geteuid() }); + assert_eq!(metadata.permissions().mode() & 0o777, 0o700); +} + +#[cfg(unix)] +#[test] +fn group_writable_private_root_parent_is_rejected() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + std::fs::set_permissions(temp.path(), std::fs::Permissions::from_mode(0o770)).unwrap(); + let cache = temp.path().join("cache"); + + validation::ensure_owned_directory_until(&cache, temp.path()) + .expect_err("group-writable parent must not protect broker output"); +} + +#[cfg(unix)] +#[test] +fn missing_cache_directory_is_created_privately() { + use std::os::unix::fs::PermissionsExt; + + let temp = tempfile::tempdir().unwrap(); + std::fs::set_permissions(temp.path(), std::fs::Permissions::from_mode(0o700)).unwrap(); + let cache = temp.path().join("new").join("cache"); + + let created = validation::ensure_owned_directory_until(&cache, temp.path()).unwrap(); + + assert_eq!(created, cache); + assert_eq!( + std::fs::metadata(&created).unwrap().permissions().mode() & 0o777, + 0o700 + ); +} + +#[test] +fn request_cleanup_removes_partial_job_trees() { + let temp = tempfile::tempdir().unwrap(); + let request = private_root(temp.path()); + let job = validation::create_private_child(&request, "job-").unwrap(); + std::fs::write(job.join("partial.part"), b"partial").unwrap(); + + validation::cleanup_private_request_dir(&request).unwrap(); + + assert!(!request.exists()); +} + +#[cfg(unix)] +#[test] +fn symlinked_output_root_is_rejected() { + use std::os::unix::fs::symlink; + + let temp = tempfile::tempdir().unwrap(); + let outside = tempfile::tempdir().unwrap(); + let managed = temp.path().join("vortex-downloads"); + symlink(outside.path(), &managed).unwrap(); + + request::prepare( + YtDlpProvider::Youtube, + download(&managed.to_string_lossy(), Some("mp4")), + temp.path(), + ) + .expect_err("symlinked root must fail"); + assert_eq!(std::fs::read_dir(outside.path()).unwrap().count(), 0); +} + fn metadata(url: &str) -> PluginYtDlpRequest { PluginYtDlpRequest::Metadata { url: url.to_string(), diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs index 8557c4af..fa4eafe0 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use anyhow::{Context, bail}; @@ -15,6 +15,14 @@ pub(super) fn validate_url(provider: YtDlpProvider, url: &str) -> anyhow::Result if !matches!(parsed.scheme(), "http" | "https") { bail!("run_ytdlp: URL must use HTTP or HTTPS"); } + if provider == YtDlpProvider::Generic + && (parsed.scheme() != "https" + || !parsed.username().is_empty() + || parsed.password().is_some() + || parsed.port().is_some_and(|port| port != 443)) + { + bail!("run_ytdlp: metadata fallback requires a canonical HTTPS URL"); + } let host = parsed .host_str() .ok_or_else(|| anyhow::anyhow!("run_ytdlp: URL has no host"))? @@ -45,7 +53,11 @@ fn host_is_approved(provider: YtDlpProvider, host: &str) -> bool { host, "soundcloud.com" | "www.soundcloud.com" | "m.soundcloud.com" | "on.soundcloud.com" ), - YtDlpProvider::Generic => true, + YtDlpProvider::Generic => { + host_is_approved(YtDlpProvider::Youtube, host) + || host_is_approved(YtDlpProvider::Vimeo, host) + || host_is_approved(YtDlpProvider::Soundcloud, host) + } } } @@ -54,7 +66,7 @@ fn provider_label(provider: YtDlpProvider) -> &'static str { YtDlpProvider::Youtube => "YouTube URL", YtDlpProvider::Vimeo => "Vimeo URL", YtDlpProvider::Soundcloud => "SoundCloud URL", - YtDlpProvider::Generic => "HTTP(S) URL", + YtDlpProvider::Generic => "supported media platform URL", } } @@ -100,6 +112,12 @@ fn is_approved_format(format: &str) -> bool { pub(super) fn private_output_dir(output_dir: &str, temp_root: &Path) -> anyhow::Result { let allowed_root = temp_root.join("vortex-downloads"); + validate_private_root(&allowed_root)?; + let requested_metadata = std::fs::symlink_metadata(output_dir) + .with_context(|| format!("run_ytdlp: output directory '{output_dir}' is unavailable"))?; + if requested_metadata.file_type().is_symlink() { + bail!("run_ytdlp: output directory must not be a symlink"); + } let canonical_root = std::fs::canonicalize(&allowed_root).with_context(|| { format!( "run_ytdlp: output root '{}' is unavailable", @@ -108,24 +126,231 @@ pub(super) fn private_output_dir(output_dir: &str, temp_root: &Path) -> anyhow:: })?; let requested = std::fs::canonicalize(output_dir) .with_context(|| format!("run_ytdlp: output directory '{output_dir}' is unavailable"))?; - if requested != canonical_root { - bail!("run_ytdlp: output directory must be the Vortex download root"); + if requested.parent() != Some(canonical_root.as_path()) || !has_prefix(&requested, "request-") { + bail!("run_ytdlp: output directory must be a Vortex-managed request root"); } - create_private_dir(&canonical_root) + validate_private_root(&requested)?; + create_private_child(&requested, "job-") +} + +pub(super) fn ensure_private_root(root: &Path) -> anyhow::Result<()> { + match std::fs::symlink_metadata(root) { + Ok(_) => validate_private_root(root), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + let mut builder = std::fs::DirBuilder::new(); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } + builder.create(root).with_context(|| { + format!( + "run_ytdlp: failed to create private root '{}'", + root.display() + ) + })?; + validate_private_root(root) + } + Err(error) => Err(error) + .with_context(|| format!("run_ytdlp: failed to inspect root '{}'", root.display())), + } +} + +#[cfg(unix)] +pub(super) fn ensure_owned_cache_directory(path: &Path) -> anyhow::Result { + ensure_owned_directory_until(path, Path::new("/")) } -fn create_private_dir(root: &Path) -> anyhow::Result { - let path = root.join(format!("job-{}", uuid::Uuid::new_v4().simple())); - std::fs::create_dir(&path).with_context(|| { +#[cfg(not(unix))] +pub(super) fn ensure_owned_cache_directory(path: &Path) -> anyhow::Result { + std::fs::create_dir_all(path).with_context(|| { format!( - "run_ytdlp: failed to create private output directory '{}'", + "run_ytdlp: failed to create user cache directory '{}'", path.display() ) })?; + let canonical = std::fs::canonicalize(path)?; + if !canonical.is_dir() { + bail!("run_ytdlp: user cache path must be a directory"); + } + Ok(canonical) +} + +#[cfg(unix)] +pub(super) fn ensure_owned_directory_until(path: &Path, anchor: &Path) -> anyhow::Result { + if !path.is_absolute() + || path + .components() + .any(|component| matches!(component, Component::ParentDir)) + { + bail!("run_ytdlp: user cache path must be absolute and normalized"); + } + + let canonical_anchor = std::fs::canonicalize(anchor).with_context(|| { + format!( + "run_ytdlp: cache trust anchor '{}' is unavailable", + anchor.display() + ) + })?; + let mut existing = path.to_path_buf(); + let mut missing = Vec::new(); + loop { + match std::fs::symlink_metadata(&existing) { + Ok(_) => break, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + let name = existing.file_name().ok_or_else(|| { + anyhow::anyhow!("run_ytdlp: user cache path has no existing ancestor") + })?; + missing.push(name.to_os_string()); + existing = existing + .parent() + .ok_or_else(|| anyhow::anyhow!("run_ytdlp: invalid user cache path"))? + .to_path_buf(); + } + Err(error) => return Err(error).context("run_ytdlp: failed to inspect user cache"), + } + } + + let mut current = std::fs::canonicalize(&existing)?; + if !current.starts_with(&canonical_anchor) { + bail!("run_ytdlp: user cache escapes its trust anchor"); + } + validate_directory_chain(¤t, &canonical_anchor)?; + + for component in missing.into_iter().rev() { + current.push(component); + let mut builder = std::fs::DirBuilder::new(); + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + builder.create(¤t).with_context(|| { + format!( + "run_ytdlp: failed to create user cache directory '{}'", + current.display() + ) + })?; + validate_private_root(¤t)?; + } + Ok(current) +} + +#[cfg(unix)] +fn validate_directory_chain(path: &Path, anchor: &Path) -> anyhow::Result<()> { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let mut current = path; + let mut first = true; + loop { + let metadata = std::fs::metadata(current)?; + let owner_is_trusted = if first { + metadata.uid() == unsafe { libc::geteuid() } + } else { + matches!(metadata.uid(), 0) || metadata.uid() == unsafe { libc::geteuid() } + }; + if !metadata.is_dir() || !owner_is_trusted || metadata.permissions().mode() & 0o022 != 0 { + bail!( + "run_ytdlp: cache ancestor '{}' is not safely owned (uid {}, mode {:o})", + current.display(), + metadata.uid(), + metadata.permissions().mode() & 0o777, + ); + } + if current == anchor { + return Ok(()); + } + current = current.parent().ok_or_else(|| { + anyhow::anyhow!("run_ytdlp: cache path does not reach its trust anchor") + })?; + first = false; + } +} + +fn validate_private_root(root: &Path) -> anyhow::Result<()> { + let metadata = std::fs::symlink_metadata(root) + .with_context(|| format!("run_ytdlp: output root '{}' is unavailable", root.display()))?; + if metadata.file_type().is_symlink() || !metadata.is_dir() { + bail!("run_ytdlp: output root must be a private directory, not a symlink"); + } #[cfg(unix)] { - use std::os::unix::fs::PermissionsExt; - std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o700))?; + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + if metadata.uid() != unsafe { libc::geteuid() } { + bail!("run_ytdlp: output root is not owned by the current user"); + } + if metadata.permissions().mode() & 0o077 != 0 { + bail!("run_ytdlp: output root permissions must be 0700"); + } + } + Ok(()) +} + +pub(super) fn create_private_child(root: &Path, prefix: &str) -> anyhow::Result { + let path = root.join(format!("{prefix}{}", uuid::Uuid::new_v4().simple())); + let mut builder = std::fs::DirBuilder::new(); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); } + builder.create(&path).with_context(|| { + format!( + "run_ytdlp: failed to create private output directory '{}'", + path.display() + ) + })?; Ok(path) } + +pub(super) fn cleanup_private_job_dir(path: &Path) -> anyhow::Result<()> { + if !has_prefix(path, "job-") + || !path + .parent() + .is_some_and(|parent| has_prefix(parent, "request-")) + { + bail!("run_ytdlp: refusing to clean an unmanaged job directory"); + } + cleanup_private_tree(path) +} + +pub(super) fn cleanup_private_request_dir(path: &Path) -> anyhow::Result<()> { + if !has_prefix(path, "request-") + || path + .parent() + .and_then(Path::file_name) + .and_then(|name| name.to_str()) + != Some("vortex-downloads") + { + bail!("run_ytdlp: refusing to clean an unmanaged request directory"); + } + cleanup_private_tree(path) +} + +fn cleanup_private_tree(path: &Path) -> anyhow::Result<()> { + let metadata = match std::fs::symlink_metadata(path) { + Ok(metadata) => metadata, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(error) => return Err(error).context("run_ytdlp: failed to inspect cleanup path"), + }; + if metadata.file_type().is_symlink() || !metadata.is_dir() { + bail!("run_ytdlp: refusing to clean a non-directory or symlink"); + } + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + if metadata.uid() != unsafe { libc::geteuid() } { + bail!("run_ytdlp: refusing to clean a directory owned by another user"); + } + } + std::fs::remove_dir_all(path).with_context(|| { + format!( + "run_ytdlp: failed to clean private directory '{}'", + path.display() + ) + }) +} + +fn has_prefix(path: &Path, prefix: &str) -> bool { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with(prefix)) +} diff --git a/src-tauri/src/adapters/driving/tauri_ipc.rs b/src-tauri/src/adapters/driving/tauri_ipc.rs index 887fc17c..98cd359c 100644 --- a/src-tauri/src/adapters/driving/tauri_ipc.rs +++ b/src-tauri/src/adapters/driving/tauri_ipc.rs @@ -1744,9 +1744,10 @@ fn resolve_media_stream( match plugin_loader.resolve_stream_url(url, quality, format, audio_only) { Ok(cdn_url) => Ok(StreamResolution::CdnUrl(cdn_url)), Err(crate::domain::error::DomainError::AdaptiveStreamOnly) => { - let temp_dir = std::env::temp_dir().join("vortex-downloads"); - std::fs::create_dir_all(&temp_dir) - .map_err(|e| format!("failed to create temp dir: {e}"))?; + let output_request = + crate::adapters::driven::plugin::ytdlp_broker::managed_output_request() + .map_err(|e| format!("failed to create private output request: {e}"))?; + let temp_dir = output_request.path(); let file_info = plugin_loader .download_to_file( @@ -1773,10 +1774,6 @@ fn resolve_media_stream( file_info.path.display() )); } - let private_output_dir = produced_canonical - .parent() - .filter(|parent| parent.parent() == Some(temp_dir_canonical.as_path())) - .map(Path::to_path_buf); let filename = title .as_deref() @@ -1808,20 +1805,17 @@ fn resolve_media_stream( let (dest_path, dest_filename) = unique_destination(&dest_dir, &filename) .map_err(|e| format!("failed to select unique destination: {e}"))?; - if std::fs::rename(&file_info.path, &dest_path).is_err() { - std::fs::copy(&file_info.path, &dest_path) + if std::fs::rename(&produced_canonical, &dest_path).is_err() { + std::fs::copy(&produced_canonical, &dest_path) .map_err(|e| format!("failed to copy merged file: {e}"))?; - if let Err(e) = std::fs::remove_file(&file_info.path) { + if let Err(e) = std::fs::remove_file(&produced_canonical) { tracing::warn!( - path = %file_info.path.display(), + path = %produced_canonical.display(), error = %e, "failed to remove temp file after copy" ); } } - if let Some(path) = private_output_dir { - cleanup_private_output_dir(&path); - } Ok(StreamResolution::LocalFile { path: dest_path, @@ -1844,20 +1838,6 @@ fn resolve_media_stream( } } -fn cleanup_private_output_dir(path: &Path) { - // ponytail: only delete an empty job directory; add a bounded stale-job - // janitor if failed partial downloads cause measurable disk growth. - if let Err(error) = std::fs::remove_dir(path) - && error.kind() != std::io::ErrorKind::NotFound - { - tracing::warn!( - path = %path.display(), - error = %error, - "failed to remove private yt-dlp output directory" - ); - } -} - fn configured_download_destination(download_dir: Option<&str>) -> Option { download_dir .map(str::trim) diff --git a/src-tauri/src/application/commands/store_install.rs b/src-tauri/src/application/commands/store_install.rs index 30551130..c4f1ff00 100644 --- a/src-tauri/src/application/commands/store_install.rs +++ b/src-tauri/src/application/commands/store_install.rs @@ -4,7 +4,6 @@ use crate::application::command_bus::CommandBus; use crate::application::commands::store_refresh::read_cache; use crate::application::error::AppError; use crate::application::read_models::plugin_store_view::PluginStoreEntryDto; -use crate::domain::model::plugin_store::{PluginStoreEntry, PluginStoreStatus}; pub struct StoreInstallCommand { pub name: String, @@ -44,7 +43,7 @@ impl CommandBus { // Find the entry in the cache let raw = read_cache(cache_path)?; - let entry_dto: PluginStoreEntryDto = raw + let _entry_dto: PluginStoreEntryDto = raw .into_iter() .filter_map(|v| { serde_json::from_value(v) @@ -54,10 +53,21 @@ impl CommandBus { .find(|dto: &PluginStoreEntryDto| dto.name == cmd.name) .ok_or_else(|| AppError::Plugin(format!("plugin '{}' not found in cache", cmd.name)))?; - // Check minimum Vortex version requirement - if let Some(ref min_ver) = entry_dto.min_vortex_version { + // Refetch the authoritative registry entry. The cache above is only a + // UI index and must never be the source of `official` or checksum + // trust decisions because it is user-writable. + let plugin_name = cmd.name.clone(); + let download = + tokio::task::spawn_blocking(move || client.download_store_plugin(&plugin_name)) + .await + .map_err(|e| AppError::Plugin(format!("download task failed: {e}")))? + .map_err(|e| AppError::Plugin(e.to_string()))?; + let plugin_dir = download.directory; + + if let Some(ref min_ver) = download.min_vortex_version { let app_ver = env!("CARGO_PKG_VERSION"); if !is_version_compatible(app_ver, min_ver) { + let _ = std::fs::remove_dir_all(&plugin_dir); return Err(AppError::Plugin(format!( "plugin '{}' requires Vortex >= {min_ver} (running {})", cmd.name, app_ver @@ -65,36 +75,6 @@ impl CommandBus { } } - // Reconstruct domain entry for the client using REAL values from cache - let category = entry_dto - .category - .parse::() - .map_err(|e| { - AppError::Plugin(format!( - "unknown plugin category '{}': {e}", - entry_dto.category - )) - })?; - let domain_entry = PluginStoreEntry { - name: entry_dto.name.clone(), - description: entry_dto.description.clone(), - author: entry_dto.author.clone(), - version: entry_dto.version.clone(), - category, - repository: entry_dto.repository.clone(), - checksum_sha256: entry_dto.checksum_sha256.clone(), - checksum_sha256_toml: entry_dto.checksum_sha256_toml.clone(), - official: entry_dto.official, - min_vortex_version: entry_dto.min_vortex_version.clone(), - status: PluginStoreStatus::NotInstalled, - installed_version: None, - }; - - let plugin_dir = tokio::task::spawn_blocking(move || client.download_plugin(&domain_entry)) - .await - .map_err(|e| AppError::Plugin(format!("download task failed: {e}")))? - .map_err(|e| AppError::Plugin(e.to_string()))?; - // Parse manifest from the downloaded directory and load via the // plugin loader. `load_from_dir` performs its own idempotent // unload inside the install-in-progress suppression window, so @@ -104,10 +84,13 @@ impl CommandBus { // final `load()` to fail with `AlreadyExists`. let loader = self.plugin_loader_arc(); let staging_for_cleanup = plugin_dir.clone(); - tokio::task::spawn_blocking(move || loader.load_from_dir(&plugin_dir)) - .await - .map_err(|e| AppError::Plugin(format!("plugin install task failed: {e}")))? - .map_err(AppError::from)?; + tokio::task::spawn_blocking(move || match download.provenance { + Some(provenance) => loader.load_official_from_dir(&plugin_dir, &provenance), + None => loader.load_from_dir(&plugin_dir), + }) + .await + .map_err(|e| AppError::Plugin(format!("plugin install task failed: {e}")))? + .map_err(AppError::from)?; // Staging is only meaningful until `load_from_dir` has copied the // files to the permanent plugins directory. After that the staged @@ -147,7 +130,7 @@ mod tests { use super::*; use crate::application::commands::store_refresh::write_cache; use crate::domain::model::plugin::PluginCategory; - use crate::domain::model::plugin_store::PluginStoreEntry; + use crate::domain::model::plugin_store::{PluginStoreEntry, PluginStoreStatus}; use tempfile::TempDir; fn make_entry(name: &str, version: &str) -> PluginStoreEntry { diff --git a/src-tauri/src/domain/ports/driven/plugin_loader.rs b/src-tauri/src/domain/ports/driven/plugin_loader.rs index 7eb4b030..f3be5d16 100644 --- a/src-tauri/src/domain/ports/driven/plugin_loader.rs +++ b/src-tauri/src/domain/ports/driven/plugin_loader.rs @@ -5,6 +5,7 @@ use crate::domain::error::DomainError; use crate::domain::model::plugin::{PluginInfo, PluginManifest}; +use crate::domain::ports::driven::plugin_store_client::OfficialPluginProvenance; /// Result of a `download_to_file` plugin call. pub struct DownloadedFileInfo { @@ -33,6 +34,21 @@ pub trait PluginLoader: Send + Sync { )) } + /// Install assets authenticated by the official Store and persist their + /// host-owned provenance before loading them. + /// + /// The default is deliberately fail-closed. Existing trait implementors + /// remain source-compatible but cannot silently discard the trust data. + fn load_official_from_dir( + &self, + _dir: &std::path::Path, + _provenance: &OfficialPluginProvenance, + ) -> Result<(), DomainError> { + Err(DomainError::PluginError( + "official Store installs not supported by this loader".into(), + )) + } + /// Unload a plugin by name. fn unload(&self, name: &str) -> Result<(), DomainError>; @@ -210,4 +226,19 @@ mod tests { let result = loader.decrypt_container(b"DLC\x00random"); assert!(matches!(result, Err(DomainError::NotFound(_)))); } + + #[test] + fn test_load_official_from_dir_default_is_fail_closed() { + let loader = MinimalLoader; + let provenance = OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: "a".repeat(64), + manifest_sha256: "b".repeat(64), + }; + + let result = loader.load_official_from_dir(std::path::Path::new("/tmp"), &provenance); + + assert!(matches!(result, Err(DomainError::PluginError(_)))); + } } diff --git a/src-tauri/src/domain/ports/driven/plugin_store_client.rs b/src-tauri/src/domain/ports/driven/plugin_store_client.rs index 1df08b57..1abbfedd 100644 --- a/src-tauri/src/domain/ports/driven/plugin_store_client.rs +++ b/src-tauri/src/domain/ports/driven/plugin_store_client.rs @@ -5,6 +5,27 @@ use std::path::PathBuf; use crate::domain::error::DomainError; use crate::domain::model::plugin_store::PluginStoreEntry; +/// Store-authenticated identity and digests handed to the plugin loader. +/// +/// These values originate from a fresh registry fetch, never from the +/// user-writable Store UI cache or from `plugin.toml` itself. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OfficialPluginProvenance { + pub name: String, + pub version: String, + pub wasm_sha256: String, + pub manifest_sha256: String, +} + +/// Files downloaded from a freshly fetched Store entry and checksum-verified. +/// Official entries additionally carry provenance eligible for native grants. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct StorePluginDownload { + pub directory: PathBuf, + pub provenance: Option, + pub min_vortex_version: Option, +} + /// Reads the remote plugin catalogue and downloads plugin assets. pub trait PluginStoreClient: Send + Sync { /// Fetch and parse the central `registry.toml` from GitHub Raw. @@ -21,4 +42,104 @@ pub trait PluginStoreClient: Send + Sync { /// - `DomainError::PluginError("checksum mismatch")` if sha256 does not match /// - `DomainError::PluginError("download failed: ...")` on network errors fn download_plugin(&self, entry: &PluginStoreEntry) -> Result; + + /// Refetch the authoritative registry entry and download both assets. + /// Only `official = true` entries receive provenance that can be persisted + /// by the loader and later unlock a native broker. + /// + /// This default keeps existing Store adapters source-compatible while + /// ensuring callers do not elevate data read from the local UI cache. + fn download_store_plugin(&self, name: &str) -> Result { + let entry = self + .fetch_registry()? + .into_iter() + .find(|entry| entry.name == name) + .ok_or_else(|| DomainError::NotFound(format!("Store plugin '{name}'")))?; + let provenance = if entry.official { + let manifest_sha256 = entry.checksum_sha256_toml.clone().ok_or_else(|| { + DomainError::PluginError(format!( + "official plugin '{name}' is missing checksum_sha256_toml" + )) + })?; + Some(OfficialPluginProvenance { + name: entry.name.clone(), + version: entry.version.clone(), + wasm_sha256: entry.checksum_sha256.clone(), + manifest_sha256, + }) + } else { + None + }; + let directory = self.download_plugin(&entry)?; + Ok(StorePluginDownload { + directory, + provenance, + min_vortex_version: entry.min_vortex_version, + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::domain::model::plugin::PluginCategory; + use crate::domain::model::plugin_store::PluginStoreStatus; + use std::sync::atomic::{AtomicBool, Ordering}; + + struct FakeStoreClient { + download_called: AtomicBool, + official: bool, + } + + impl PluginStoreClient for FakeStoreClient { + fn fetch_registry(&self) -> Result, DomainError> { + Ok(vec![PluginStoreEntry { + name: "community-plugin".into(), + description: "test".into(), + author: "tester".into(), + version: "1.0.0".into(), + category: PluginCategory::Utility, + repository: "https://example.test/community-plugin".into(), + checksum_sha256: "a".repeat(64), + checksum_sha256_toml: Some("b".repeat(64)), + official: self.official, + min_vortex_version: None, + status: PluginStoreStatus::NotInstalled, + installed_version: None, + }]) + } + + fn download_plugin(&self, _: &PluginStoreEntry) -> Result { + self.download_called.store(true, Ordering::SeqCst); + Ok(PathBuf::from("/tmp/community-plugin")) + } + } + + #[test] + fn test_download_store_plugin_installs_unofficial_without_provenance() { + let client = FakeStoreClient { + download_called: AtomicBool::new(false), + official: false, + }; + + let result = client.download_store_plugin("community-plugin").unwrap(); + + assert!(result.provenance.is_none()); + assert!(client.download_called.load(Ordering::SeqCst)); + } + + #[test] + fn test_download_store_plugin_returns_provenance_only_for_official_entry() { + let client = FakeStoreClient { + download_called: AtomicBool::new(false), + official: true, + }; + + let result = client.download_store_plugin("community-plugin").unwrap(); + + let provenance = result.provenance.expect("official provenance"); + assert_eq!(provenance.name, "community-plugin"); + assert_eq!(provenance.wasm_sha256, "a".repeat(64)); + assert_eq!(provenance.manifest_sha256, "b".repeat(64)); + } } From f91b43d62504522407e2e491aaf9d53dacd2726b Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 18:16:22 +0200 Subject: [PATCH 07/16] fix(ci): restrict plugin releases to owner --- .github/workflows/plugin-release.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/plugin-release.yml b/.github/workflows/plugin-release.yml index aa0d6187..6efb4c89 100644 --- a/.github/workflows/plugin-release.yml +++ b/.github/workflows/plugin-release.yml @@ -18,7 +18,9 @@ jobs: release: name: Publish immutable GitHub release - if: startsWith(github.ref, 'refs/tags/v') + if: >- + startsWith(github.ref, 'refs/tags/v') && + github.actor == github.repository_owner needs: verify runs-on: ubuntu-24.04 timeout-minutes: 10 From 9b3b7b3b7956150f7475ab409bb0eed424573bae Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 18:33:10 +0200 Subject: [PATCH 08/16] fix(ci): canonicalize plugin release assets --- .github/workflows/plugin-ci.yml | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/.github/workflows/plugin-ci.yml b/.github/workflows/plugin-ci.yml index 142ab82a..d9a3f243 100644 --- a/.github/workflows/plugin-ci.yml +++ b/.github/workflows/plugin-ci.yml @@ -82,9 +82,12 @@ jobs: artifact_name = f"{crate_name}.wasm" wasm_path = f"target/wasm32-wasip1/release/{artifact_name}" + plugin_wasm_asset = f"{package_name.replace('-', '_')}.wasm" + if re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*\.wasm", plugin_wasm_asset) is None: + raise SystemExit("plugin name cannot be mapped to a release WASM asset") with open(os.environ["GITHUB_ENV"], "a", encoding="utf-8") as env: env.write(f"PACKAGE_NAME={package_name}\n") - env.write(f"ARTIFACT_NAME={artifact_name}\n") + env.write(f"PLUGIN_WASM_ASSET={plugin_wasm_asset}\n") env.write(f"WASM_PATH={wasm_path}\n") PY @@ -144,8 +147,10 @@ jobs: shell: bash run: | mkdir -p dist - cp "${WASM_PATH}" "dist/${ARTIFACT_NAME}" cp "${WASM_PATH}" dist/plugin.wasm + if [[ "${PLUGIN_WASM_ASSET}" != "plugin.wasm" ]]; then + cp "${WASM_PATH}" "dist/${PLUGIN_WASM_ASSET}" + fi cp plugin.toml dist/plugin.toml ( cd dist @@ -153,6 +158,19 @@ jobs: sha256sum --check SHA256SUMS ) + expected_assets=(plugin.wasm plugin.toml SHA256SUMS) + if [[ "${PLUGIN_WASM_ASSET}" != "plugin.wasm" ]]; then + expected_assets+=("${PLUGIN_WASM_ASSET}") + fi + expected_assets="$(printf '%s\n' "${expected_assets[@]}" | LC_ALL=C sort)" + actual_assets="$(find dist -maxdepth 1 -type f -printf '%f\n' | LC_ALL=C sort)" + if [[ "${actual_assets}" != "${expected_assets}" ]]; then + echo "verified bundle asset set is not canonical" >&2 + diff -u <(printf '%s\n' "${expected_assets}") \ + <(printf '%s\n' "${actual_assets}") || true + exit 1 + fi + - name: Upload verified release bundle uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: From 1b0cc276086cfc87165d63f8c5624fa3960ad2e9 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 18:33:19 +0200 Subject: [PATCH 09/16] fix(ci): handle canonical release asset collisions --- .github/workflows/plugin-release.yml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/plugin-release.yml b/.github/workflows/plugin-release.yml index 6efb4c89..037e1cbe 100644 --- a/.github/workflows/plugin-release.yml +++ b/.github/workflows/plugin-release.yml @@ -14,7 +14,7 @@ jobs: name: Verify release bundle permissions: contents: read - uses: mpiton/vortex/.github/workflows/plugin-ci.yml@b70495f246944921b0781328a0139c404e775214 + uses: mpiton/vortex/.github/workflows/plugin-ci.yml@9b3b7b3b7956150f7475ab409bb0eed424573bae release: name: Publish immutable GitHub release @@ -78,8 +78,10 @@ jobs: "plugin.wasm" "plugin.toml" "SHA256SUMS" - "${PLUGIN_WASM_ASSET}" ) + if [[ "${PLUGIN_WASM_ASSET}" != "plugin.wasm" ]]; then + release_assets+=("${PLUGIN_WASM_ASSET}") + fi release_paths=() download_patterns=() for asset in "${release_assets[@]}"; do From 890d62f96ced7cbd3a711ff920009fb582c902c3 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 18:47:41 +0200 Subject: [PATCH 10/16] fix(plugins): harden provenance persistence --- CHANGELOG.md | 4 +- .../adapters/driven/plugin/extism_loader.rs | 116 +++++++++++- .../src/adapters/driven/plugin/provenance.rs | 177 +++++++++++++++--- 3 files changed, 267 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c2b440c6..64af16be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 still match freshly fetched registry checksums. The grant is persisted outside plugin directories and revalidated on every load; local installs, hot-reloaded modifications, unsafe executable paths, oversized process - output, and failed partial downloads all fail closed. + output, and failed partial downloads all fail closed. Provenance paths are + resolved before containment checks, state updates use an atomic synced + replacement, and failed revocations leave memory and disk consistent. ### Fixed diff --git a/src-tauri/src/adapters/driven/plugin/extism_loader.rs b/src-tauri/src/adapters/driven/plugin/extism_loader.rs index a1dca797..11237a41 100644 --- a/src-tauri/src/adapters/driven/plugin/extism_loader.rs +++ b/src-tauri/src/adapters/driven/plugin/extism_loader.rs @@ -1,7 +1,7 @@ //! Implements [`PluginLoader`] using Extism and [`PluginRegistry`]. use std::collections::HashMap; -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::{Arc, Mutex}; @@ -82,6 +82,8 @@ impl ExtismPluginLoader { shared_resources: Arc, provenance_path: PathBuf, ) -> Result { + let plugins_dir = resolve_path(&plugins_dir)?; + let provenance_path = resolve_path(&provenance_path)?; if provenance_path.starts_with(&plugins_dir) { return Err(DomainError::ValidationError( "plugin provenance must live outside the plugin directory".into(), @@ -254,6 +256,59 @@ impl ExtismPluginLoader { } } +fn resolve_path(path: &Path) -> Result { + let absolute = if path.is_absolute() { + path.to_path_buf() + } else { + std::env::current_dir() + .map_err(|error| { + DomainError::PluginError(format!("failed to resolve current directory: {error}")) + })? + .join(path) + }; + resolve_existing_ancestor(&absolute) +} + +fn resolve_existing_ancestor(path: &Path) -> Result { + match std::fs::canonicalize(path) { + Ok(resolved) => Ok(resolved), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + if path + .components() + .any(|component| component == Component::ParentDir) + { + return Err(DomainError::ValidationError(format!( + "path '{}' contains an unresolved parent traversal", + path.display() + ))); + } + if std::fs::symlink_metadata(path).is_ok() { + return Err(DomainError::ValidationError(format!( + "path '{}' contains an unresolved symbolic link", + path.display() + ))); + } + let parent = path.parent().ok_or_else(|| { + DomainError::ValidationError(format!( + "path '{}' has no resolvable parent", + path.display() + )) + })?; + let name = path.file_name().ok_or_else(|| { + DomainError::ValidationError(format!( + "path '{}' cannot be resolved safely", + path.display() + )) + })?; + Ok(resolve_existing_ancestor(parent)?.join(name)) + } + Err(error) => Err(DomainError::PluginError(format!( + "failed to resolve path '{}': {error}", + path.display() + ))), + } +} + impl PluginLoader for ExtismPluginLoader { fn load(&self, manifest: &PluginManifest) -> Result<(), DomainError> { let name = manifest.info().name().to_string(); @@ -683,6 +738,65 @@ subprocess = ["yt-dlp"] (wasm, manifest) } + #[test] + fn test_new_rejects_absolute_provenance_inside_relative_plugins_dir() { + let current_dir = std::env::current_dir().unwrap(); + let temp = tempfile::Builder::new() + .prefix("vortex-provenance-containment-") + .tempdir_in(¤t_dir) + .unwrap(); + let relative_root = temp.path().strip_prefix(¤t_dir).unwrap(); + let relative_plugins_dir = relative_root.join("plugins"); + let absolute_provenance_path = temp.path().join("plugins").join("provenance.json"); + + let result = ExtismPluginLoader::new_with_provenance_path( + relative_plugins_dir, + Arc::new(SharedHostResources::new()), + absolute_provenance_path, + ); + + assert!(matches!(result, Err(DomainError::ValidationError(_)))); + } + + #[test] + fn test_new_rejects_unresolved_parent_traversal_into_plugins_dir() { + let temp = TempDir::new().unwrap(); + let plugins_dir = temp.path().join("plugins"); + std::fs::create_dir(&plugins_dir).unwrap(); + let provenance_path = temp + .path() + .join("missing") + .join("..") + .join("plugins") + .join("provenance.json"); + + let result = ExtismPluginLoader::new_with_provenance_path( + plugins_dir, + Arc::new(SharedHostResources::new()), + provenance_path, + ); + + assert!(matches!(result, Err(DomainError::ValidationError(_)))); + } + + #[cfg(unix)] + #[test] + fn test_new_rejects_provenance_symlink_resolving_inside_plugins_dir() { + let temp = TempDir::new().unwrap(); + let plugins_dir = temp.path().join("plugins"); + std::fs::create_dir(&plugins_dir).unwrap(); + let plugins_alias = temp.path().join("plugins-alias"); + std::os::unix::fs::symlink(&plugins_dir, &plugins_alias).unwrap(); + + let result = ExtismPluginLoader::new_with_provenance_path( + plugins_dir, + Arc::new(SharedHostResources::new()), + plugins_alias.join("provenance.json"), + ); + + assert!(matches!(result, Err(DomainError::ValidationError(_)))); + } + #[test] fn test_overlapping_installs_keep_suppression_active_until_last_drop() { // The reason we track a refcount rather than a boolean flag: two diff --git a/src-tauri/src/adapters/driven/plugin/provenance.rs b/src-tauri/src/adapters/driven/plugin/provenance.rs index 3a8a7f31..76c4f4ca 100644 --- a/src-tauri/src/adapters/driven/plugin/provenance.rs +++ b/src-tauri/src/adapters/driven/plugin/provenance.rs @@ -5,6 +5,7 @@ use std::sync::Mutex; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha256}; +use uuid::Uuid; use crate::domain::error::DomainError; use crate::domain::ports::driven::plugin_store_client::OfficialPluginProvenance; @@ -103,60 +104,101 @@ impl OfficialProvenanceStore { .entries .lock() .unwrap_or_else(|poisoned| poisoned.into_inner()); - if entries.remove(name).is_some() { - self.persist(&entries)?; + let mut updated_entries = entries.clone(); + if updated_entries.remove(name).is_some() { + self.persist(&updated_entries)?; + *entries = updated_entries; } Ok(()) } fn persist(&self, entries: &HashMap) -> Result<(), DomainError> { - if let Some(parent) = self.path.parent() { - std::fs::create_dir_all(parent).map_err(|error| { - DomainError::PluginError(format!( - "failed to create plugin provenance directory '{}': {error}", - parent.display() - )) - })?; - } + let parent = self.path.parent().ok_or_else(|| { + DomainError::PluginError(format!( + "plugin provenance path '{}' has no parent", + self.path.display() + )) + })?; + std::fs::create_dir_all(parent).map_err(|error| { + DomainError::PluginError(format!( + "failed to create plugin provenance directory '{}': {error}", + parent.display() + )) + })?; let payload = serde_json::to_vec_pretty(&ProvenanceFile { plugins: entries.clone(), }) .map_err(|error| DomainError::PluginError(format!("provenance encode failed: {error}")))?; + let file_name = self.path.file_name().ok_or_else(|| { + DomainError::PluginError(format!( + "plugin provenance path '{}' has no file name", + self.path.display() + )) + })?; + let temp_path = parent.join(format!( + ".{}.{}.tmp", + file_name.to_string_lossy(), + Uuid::new_v4() + )); + let result = self.persist_atomically(&temp_path, parent, &payload); + if result.is_err() { + let _ = std::fs::remove_file(&temp_path); + } + result + } + + fn persist_atomically( + &self, + temp_path: &std::path::Path, + parent: &std::path::Path, + payload: &[u8], + ) -> Result<(), DomainError> { let mut options = std::fs::OpenOptions::new(); - options.create(true).truncate(true).write(true); + options.create_new(true).write(true); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; options.mode(0o600); } - let mut file = options.open(&self.path).map_err(|error| { - DomainError::PluginError(format!( - "failed to write plugin provenance '{}': {error}", - self.path.display() - )) - })?; - file.write_all(&payload).map_err(|error| { + { + let mut file = options.open(temp_path).map_err(|error| { + DomainError::PluginError(format!( + "failed to create plugin provenance temp file '{}': {error}", + temp_path.display() + )) + })?; + file.write_all(payload).map_err(|error| { + DomainError::PluginError(format!( + "failed to write plugin provenance temp file '{}': {error}", + temp_path.display() + )) + })?; + file.sync_all().map_err(|error| { + DomainError::PluginError(format!( + "failed to sync plugin provenance temp file '{}': {error}", + temp_path.display() + )) + })?; + } + std::fs::rename(temp_path, &self.path).map_err(|error| { DomainError::PluginError(format!( - "failed to write plugin provenance '{}': {error}", + "failed to replace plugin provenance '{}': {error}", self.path.display() )) })?; #[cfg(unix)] - std::fs::set_permissions(&self.path, std::fs::Permissions::from_mode(0o600)).map_err( - |error| { + std::fs::File::open(parent) + .and_then(|directory| directory.sync_all()) + .map_err(|error| { DomainError::PluginError(format!( - "failed to secure plugin provenance '{}': {error}", - self.path.display() + "failed to sync plugin provenance directory '{}': {error}", + parent.display() )) - }, - )?; + })?; Ok(()) } } -#[cfg(unix)] -use std::os::unix::fs::PermissionsExt; - fn digest(bytes: &[u8]) -> String { hex::encode(Sha256::digest(bytes)) } @@ -174,6 +216,8 @@ fn validate_digest(value: &str) -> Result<(), DomainError> { #[cfg(test)] mod tests { use super::*; + #[cfg(unix)] + use std::io::Read; use tempfile::TempDir; #[test] @@ -269,4 +313,81 @@ mod tests { .ytdlp ); } + + #[test] + fn test_failed_revoke_keeps_in_memory_and_persisted_grant_consistent() { + let temp = TempDir::new().unwrap(); + let state_dir = temp.path().join("host-state"); + std::fs::create_dir(&state_dir).unwrap(); + let state_path = state_dir.join("plugin-provenance.json"); + let wasm = b"official wasm"; + let manifest = b"official manifest"; + let provenance = OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: digest(wasm), + manifest_sha256: digest(manifest), + }; + let store = OfficialProvenanceStore::new(state_path).unwrap(); + store.record(&provenance).unwrap(); + let moved_state_dir = temp.path().join("moved-host-state"); + std::fs::rename(&state_dir, &moved_state_dir).unwrap(); + std::fs::write(&state_dir, b"file").unwrap(); + + let result = store.revoke("vortex-mod-youtube"); + + assert!(result.is_err()); + assert!( + store + .grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest) + .ytdlp + ); + let reloaded = + OfficialProvenanceStore::new(moved_state_dir.join("plugin-provenance.json")).unwrap(); + assert!( + reloaded + .grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest) + .ytdlp + ); + } + + #[cfg(unix)] + #[test] + fn test_persist_replaces_live_state_atomically() { + let temp = TempDir::new().unwrap(); + let state_path = temp.path().join("plugin-provenance.json"); + let store = OfficialProvenanceStore::new(state_path.clone()).unwrap(); + let first = OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: digest(b"first wasm"), + manifest_sha256: digest(b"first manifest"), + }; + store.record(&first).unwrap(); + let mut open_snapshot = std::fs::File::open(&state_path).unwrap(); + let second = OfficialPluginProvenance { + name: first.name.clone(), + version: "2.0.0".into(), + wasm_sha256: digest(b"second wasm"), + manifest_sha256: digest(b"second manifest"), + }; + + store.record(&second).unwrap(); + + let mut snapshot_json = String::new(); + open_snapshot.read_to_string(&mut snapshot_json).unwrap(); + let snapshot: ProvenanceFile = serde_json::from_str(&snapshot_json).unwrap(); + assert_eq!(snapshot.plugins["vortex-mod-youtube"].version, "1.0.0"); + let current = OfficialProvenanceStore::new(state_path).unwrap(); + assert!( + current + .grants_for( + "vortex-mod-youtube", + "2.0.0", + b"second wasm", + b"second manifest" + ) + .ytdlp + ); + } } From 9f1af9a4723300248c334273c0de7c5cfee918a1 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 18:58:47 +0200 Subject: [PATCH 11/16] fix(plugins): reconcile committed provenance state --- CHANGELOG.md | 3 +- .../src/adapters/driven/plugin/provenance.rs | 112 ++++++++++++++++-- 2 files changed, 101 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64af16be..d35a73eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 hot-reloaded modifications, unsafe executable paths, oversized process output, and failed partial downloads all fail closed. Provenance paths are resolved before containment checks, state updates use an atomic synced - replacement, and failed revocations leave memory and disk consistent. + replacement, and failed revocations or post-rename sync errors leave memory + and disk consistent. ### Fixed diff --git a/src-tauri/src/adapters/driven/plugin/provenance.rs b/src-tauri/src/adapters/driven/plugin/provenance.rs index 76c4f4ca..dfde3617 100644 --- a/src-tauri/src/adapters/driven/plugin/provenance.rs +++ b/src-tauri/src/adapters/driven/plugin/provenance.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; use std::io::Write; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::Mutex; use serde::{Deserialize, Serialize}; @@ -12,6 +12,22 @@ use crate::domain::ports::driven::plugin_store_client::OfficialPluginProvenance; use super::capabilities::HostFunctionGrants; +type ParentSync = fn(&Path) -> std::io::Result<()>; + +enum PersistResult { + Committed, + CommittedWithDurabilityError(DomainError), +} + +impl PersistResult { + fn into_result(self) -> Result<(), DomainError> { + match self { + Self::Committed => Ok(()), + Self::CommittedWithDurabilityError(error) => Err(error), + } + } +} + #[derive(Clone, Debug, Deserialize, Serialize)] struct PersistedProvenance { version: String, @@ -29,10 +45,15 @@ struct ProvenanceFile { pub(super) struct OfficialProvenanceStore { path: PathBuf, entries: Mutex>, + sync_parent: ParentSync, } impl OfficialProvenanceStore { pub(super) fn new(path: PathBuf) -> Result { + Self::new_with_parent_sync(path, sync_parent_directory) + } + + fn new_with_parent_sync(path: PathBuf, sync_parent: ParentSync) -> Result { let entries = match std::fs::read(&path) { Ok(bytes) => match serde_json::from_slice::(&bytes) { Ok(file) => file.plugins, @@ -56,6 +77,7 @@ impl OfficialProvenanceStore { Ok(Self { path, entries: Mutex::new(entries), + sync_parent, }) } @@ -75,9 +97,9 @@ impl OfficialProvenanceStore { manifest_sha256: provenance.manifest_sha256.to_ascii_lowercase(), }, ); - self.persist(&updated_entries)?; + let persist_result = self.persist(&updated_entries)?; *entries = updated_entries; - Ok(()) + persist_result.into_result() } pub(super) fn grants_for( @@ -106,13 +128,17 @@ impl OfficialProvenanceStore { .unwrap_or_else(|poisoned| poisoned.into_inner()); let mut updated_entries = entries.clone(); if updated_entries.remove(name).is_some() { - self.persist(&updated_entries)?; + let persist_result = self.persist(&updated_entries)?; *entries = updated_entries; + return persist_result.into_result(); } Ok(()) } - fn persist(&self, entries: &HashMap) -> Result<(), DomainError> { + fn persist( + &self, + entries: &HashMap, + ) -> Result { let parent = self.path.parent().ok_or_else(|| { DomainError::PluginError(format!( "plugin provenance path '{}' has no parent", @@ -152,7 +178,7 @@ impl OfficialProvenanceStore { temp_path: &std::path::Path, parent: &std::path::Path, payload: &[u8], - ) -> Result<(), DomainError> { + ) -> Result { let mut options = std::fs::OpenOptions::new(); options.create_new(true).write(true); #[cfg(unix)] @@ -186,19 +212,28 @@ impl OfficialProvenanceStore { self.path.display() )) })?; - #[cfg(unix)] - std::fs::File::open(parent) - .and_then(|directory| directory.sync_all()) - .map_err(|error| { + match (self.sync_parent)(parent) { + Ok(()) => Ok(PersistResult::Committed), + Err(error) => Ok(PersistResult::CommittedWithDurabilityError( DomainError::PluginError(format!( "failed to sync plugin provenance directory '{}': {error}", parent.display() - )) - })?; - Ok(()) + )), + )), + } } } +#[cfg(unix)] +fn sync_parent_directory(parent: &Path) -> std::io::Result<()> { + std::fs::File::open(parent).and_then(|directory| directory.sync_all()) +} + +#[cfg(not(unix))] +fn sync_parent_directory(_: &Path) -> std::io::Result<()> { + Ok(()) +} + fn digest(bytes: &[u8]) -> String { hex::encode(Sha256::digest(bytes)) } @@ -220,6 +255,10 @@ mod tests { use std::io::Read; use tempfile::TempDir; + fn fail_parent_sync(_: &Path) -> std::io::Result<()> { + Err(std::io::Error::other("injected parent sync failure")) + } + #[test] fn test_verified_provenance_persists_and_grants_unchanged_plugin() { let temp = TempDir::new().unwrap(); @@ -351,6 +390,53 @@ mod tests { ); } + #[test] + fn test_committed_sync_errors_keep_record_and_revoke_consistent_with_disk() { + let temp = TempDir::new().unwrap(); + let state_path = temp.path().join("plugin-provenance.json"); + let wasm = b"official wasm"; + let manifest = b"official manifest"; + let provenance = OfficialPluginProvenance { + name: "vortex-mod-youtube".into(), + version: "1.0.0".into(), + wasm_sha256: digest(wasm), + manifest_sha256: digest(manifest), + }; + let store = + OfficialProvenanceStore::new_with_parent_sync(state_path.clone(), fail_parent_sync) + .unwrap(); + + let record_result = store.record(&provenance); + + assert!(record_result.is_err()); + assert!( + store + .grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest) + .ytdlp + ); + let reloaded = OfficialProvenanceStore::new(state_path.clone()).unwrap(); + assert!( + reloaded + .grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest) + .ytdlp + ); + + let revoke_result = store.revoke("vortex-mod-youtube"); + + assert!(revoke_result.is_err()); + assert!( + !store + .grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest) + .ytdlp + ); + let reloaded = OfficialProvenanceStore::new(state_path).unwrap(); + assert!( + !reloaded + .grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest) + .ytdlp + ); + } + #[cfg(unix)] #[test] fn test_persist_replaces_live_state_atomically() { From 8a8baa18169e1dc81c797d755364f088c06b24c6 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 19:13:29 +0200 Subject: [PATCH 12/16] fix(plugins): complete committed provenance records --- CHANGELOG.md | 4 ++-- .../src/adapters/driven/plugin/provenance.rs | 20 ++++++++++++++++--- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d35a73eb..7b24ae7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,8 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 hot-reloaded modifications, unsafe executable paths, oversized process output, and failed partial downloads all fail closed. Provenance paths are resolved before containment checks, state updates use an atomic synced - replacement, and failed revocations or post-rename sync errors leave memory - and disk consistent. + replacement, committed record sync errors no longer abort verified installs, + and revocation sync errors remain blocking before local replacement. ### Fixed diff --git a/src-tauri/src/adapters/driven/plugin/provenance.rs b/src-tauri/src/adapters/driven/plugin/provenance.rs index dfde3617..e380dbf7 100644 --- a/src-tauri/src/adapters/driven/plugin/provenance.rs +++ b/src-tauri/src/adapters/driven/plugin/provenance.rs @@ -20,6 +20,15 @@ enum PersistResult { } impl PersistResult { + fn warn_if_not_durable(self) { + if let Self::CommittedWithDurabilityError(error) = self { + tracing::warn!( + %error, + "plugin provenance record committed without durable directory sync" + ); + } + } + fn into_result(self) -> Result<(), DomainError> { match self { Self::Committed => Ok(()), @@ -99,7 +108,12 @@ impl OfficialProvenanceStore { ); let persist_result = self.persist(&updated_entries)?; *entries = updated_entries; - persist_result.into_result() + // If directory sync fails after rename, startup checksum revalidation + // still makes a lost record fail closed. Do not abort an install whose + // files and trust record are already committed. Revocation remains + // stricter because it runs before an untrusted local replacement. + persist_result.warn_if_not_durable(); + Ok(()) } pub(super) fn grants_for( @@ -391,7 +405,7 @@ mod tests { } #[test] - fn test_committed_sync_errors_keep_record_and_revoke_consistent_with_disk() { + fn test_committed_sync_errors_allow_record_but_fail_revoke_without_divergence() { let temp = TempDir::new().unwrap(); let state_path = temp.path().join("plugin-provenance.json"); let wasm = b"official wasm"; @@ -408,7 +422,7 @@ mod tests { let record_result = store.record(&provenance); - assert!(record_result.is_err()); + assert!(record_result.is_ok()); assert!( store .grants_for("vortex-mod-youtube", "1.0.0", wasm, manifest) From f63e53f866bbdf5e5e3e8f62a37f78e13cec5298 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 19:34:30 +0200 Subject: [PATCH 13/16] fix(ci): harden plugin artifact verification --- .github/workflows/plugin-ci.yml | 58 +++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/.github/workflows/plugin-ci.yml b/.github/workflows/plugin-ci.yml index d9a3f243..2863899c 100644 --- a/.github/workflows/plugin-ci.yml +++ b/.github/workflows/plugin-ci.yml @@ -24,6 +24,8 @@ jobs: steps: - name: Check out plugin repository uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false - name: Activate repository Rust toolchain shell: bash @@ -33,7 +35,6 @@ jobs: cargo --version rustup component list --installed | grep -q '^clippy-' rustup component list --installed | grep -q '^rustfmt-' - rustup component list --installed | grep -q '^llvm-tools-' rustup target list --installed | grep -qx 'wasm32-wasip1' - name: Cache Cargo build data @@ -108,10 +109,6 @@ jobs: run: | test -s "${WASM_PATH}" - llvm_nm="$(find "$(rustc --print sysroot)" -type f -name llvm-nm -print -quit)" - test -x "${llvm_nm}" - symbols="$("${llvm_nm}" --defined-only "${WASM_PATH}" | awk '{print $3}')" - case "${PACKAGE_NAME}" in vortex-mod-containers) required="can_decrypt detect decrypt" @@ -133,37 +130,58 @@ jobs: ;; esac - for export in ${required}; do - grep -qx "${export}" <<<"${symbols}" || { - echo "missing required WASM export: ${export}" >&2 - exit 1 + REQUIRED_EXPORTS="${required}" node <<'JS' + const fs = require("node:fs"); + + let compiled; + try { + compiled = new WebAssembly.Module(fs.readFileSync(process.env.WASM_PATH)); + } catch (error) { + console.error(`invalid WASM artifact: ${error.message}`); + process.exit(1); + } + const functionExports = new Set( + WebAssembly.Module.exports(compiled) + .filter(({ kind }) => kind === "function") + .map(({ name }) => name), + ); + for (const required of process.env.REQUIRED_EXPORTS.split(/\s+/)) { + if (!functionExports.has(required)) { + console.error(`missing required WASM export: ${required}`); + process.exit(1); } - done + } + JS - name: Run native tests including ABI smoke run: cargo test --locked --all-targets --target x86_64-unknown-linux-gnu - name: Build verified release bundle + id: bundle shell: bash run: | - mkdir -p dist - cp "${WASM_PATH}" dist/plugin.wasm + bundle_dir="$(mktemp -d "${RUNNER_TEMP}/plugin-release.XXXXXX")" + printf 'bundle_dir=%s\n' "${bundle_dir}" >> "${GITHUB_OUTPUT}" + + cp "${WASM_PATH}" "${bundle_dir}/plugin.wasm" if [[ "${PLUGIN_WASM_ASSET}" != "plugin.wasm" ]]; then - cp "${WASM_PATH}" "dist/${PLUGIN_WASM_ASSET}" + cp "${WASM_PATH}" "${bundle_dir}/${PLUGIN_WASM_ASSET}" fi - cp plugin.toml dist/plugin.toml + cp plugin.toml "${bundle_dir}/plugin.toml" ( - cd dist + cd "${bundle_dir}" sha256sum plugin.wasm plugin.toml > SHA256SUMS sha256sum --check SHA256SUMS ) - expected_assets=(plugin.wasm plugin.toml SHA256SUMS) + expected_asset_names=(plugin.wasm plugin.toml SHA256SUMS) if [[ "${PLUGIN_WASM_ASSET}" != "plugin.wasm" ]]; then - expected_assets+=("${PLUGIN_WASM_ASSET}") + expected_asset_names+=("${PLUGIN_WASM_ASSET}") fi - expected_assets="$(printf '%s\n' "${expected_assets[@]}" | LC_ALL=C sort)" - actual_assets="$(find dist -maxdepth 1 -type f -printf '%f\n' | LC_ALL=C sort)" + expected_assets="$(printf '%s\n' "${expected_asset_names[@]}" | LC_ALL=C sort)" + actual_assets="$( + find "${bundle_dir}" -mindepth 1 -printf '%P\n' | LC_ALL=C sort + )" if [[ "${actual_assets}" != "${expected_assets}" ]]; then echo "verified bundle asset set is not canonical" >&2 diff -u <(printf '%s\n' "${expected_assets}") \ @@ -175,6 +193,6 @@ jobs: uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: plugin-release-${{ github.sha }} - path: dist/ + path: ${{ steps.bundle.outputs.bundle_dir }}/ if-no-files-found: error retention-days: 14 From d85baa93ac7905c5813095b2406f2a3642458356 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 19:35:11 +0200 Subject: [PATCH 14/16] fix(ci): reject ambiguous registry releases --- .github/workflows/plugin-release.yml | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/workflows/plugin-release.yml b/.github/workflows/plugin-release.yml index 037e1cbe..5e2c4db7 100644 --- a/.github/workflows/plugin-release.yml +++ b/.github/workflows/plugin-release.yml @@ -14,7 +14,7 @@ jobs: name: Verify release bundle permissions: contents: read - uses: mpiton/vortex/.github/workflows/plugin-ci.yml@9b3b7b3b7956150f7475ab409bb0eed424573bae + uses: mpiton/vortex/.github/workflows/plugin-ci.yml@f63e53f866bbdf5e5e3e8f62a37f78e13cec5298 release: name: Publish immutable GitHub release @@ -159,12 +159,14 @@ jobs: with open(os.environ["REGISTRY_PATH"], "rb") as file: entries = tomllib.load(file)["plugin"] - entry = next( - (item for item in entries if item["name"] == manifest["name"]), - None, - ) - if entry is None: + matches = [item for item in entries if item["name"] == manifest["name"]] + if not matches: raise SystemExit("plugin is missing from the central registry") + if len(matches) != 1: + raise SystemExit( + f"expected exactly one registry entry, found {len(matches)}" + ) + entry = matches[0] for key in ("version", "min_vortex_version"): if entry[key] != manifest[key]: raise SystemExit(f"registry {key} differs from plugin.toml") From 333a2c996bd217bf55b6f65726688abd2378e5ff Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 19:36:27 +0200 Subject: [PATCH 15/16] fix(plugins): close broker validation races --- CHANGELOG.md | 7 ++ .../adapters/driven/plugin/extism_loader.rs | 39 ++++++++++ .../plugin/ytdlp_broker/tests/platform.rs | 5 +- .../plugin/ytdlp_broker/tests/request.rs | 19 +++++ .../driven/plugin/ytdlp_broker/validation.rs | 78 ++++++++++++------- 5 files changed, 120 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b24ae7f..92d31a6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 resolved before containment checks, state updates use an atomic synced replacement, committed record sync errors no longer abort verified installs, and revocation sync errors remain blocking before local replacement. +- **Lot 1 review hardening (MAT-129, MAT-131)**: reusable plugin CI now drops + checkout credentials, verifies the real WASM export table, builds release + assets in a clean directory, and rejects duplicate registry entries. Plugin + trust paths are canonicalized against the real filesystem, yt-dlp accepts + HTTPS only and tolerates concurrent private-directory creation, adaptive + downloads reserve destinations without overwrite races, and failed Store + installs clean their staging directory before returning an error. ### Fixed diff --git a/src-tauri/src/adapters/driven/plugin/extism_loader.rs b/src-tauri/src/adapters/driven/plugin/extism_loader.rs index 11237a41..a59b1850 100644 --- a/src-tauri/src/adapters/driven/plugin/extism_loader.rs +++ b/src-tauri/src/adapters/driven/plugin/extism_loader.rs @@ -83,6 +83,18 @@ impl ExtismPluginLoader { provenance_path: PathBuf, ) -> Result { let plugins_dir = resolve_path(&plugins_dir)?; + std::fs::create_dir_all(&plugins_dir).map_err(|error| { + DomainError::PluginError(format!( + "failed to create plugin directory '{}': {error}", + plugins_dir.display() + )) + })?; + let plugins_dir = std::fs::canonicalize(&plugins_dir).map_err(|error| { + DomainError::PluginError(format!( + "failed to resolve plugin directory '{}': {error}", + plugins_dir.display() + )) + })?; let provenance_path = resolve_path(&provenance_path)?; if provenance_path.starts_with(&plugins_dir) { return Err(DomainError::ValidationError( @@ -779,6 +791,33 @@ subprocess = ["yt-dlp"] assert!(matches!(result, Err(DomainError::ValidationError(_)))); } + #[test] + fn test_new_rejects_case_only_provenance_alias_inside_plugins_dir() { + let temp = TempDir::new().unwrap(); + let plugins_dir = temp.path().join("Plugins"); + std::fs::create_dir(&plugins_dir).unwrap(); + let canonical_plugins = std::fs::canonicalize(&plugins_dir).unwrap(); + let case_only_parent = temp.path().join("plugins"); + let case_only_parent_is_alias = + std::fs::canonicalize(&case_only_parent).is_ok_and(|path| path == canonical_plugins); + let provenance_path = case_only_parent.join("provenance.json"); + + let result = ExtismPluginLoader::new_with_provenance_path( + plugins_dir, + Arc::new(SharedHostResources::new()), + provenance_path, + ); + + if case_only_parent_is_alias { + assert!(matches!(result, Err(DomainError::ValidationError(_)))); + } else { + assert!( + result.is_ok(), + "distinct case-sensitive path must remain valid" + ); + } + } + #[cfg(unix)] #[test] fn test_new_rejects_provenance_symlink_resolving_inside_plugins_dir() { diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs index 4257d2a1..67200fce 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/platform.rs @@ -11,7 +11,10 @@ mod unix { let outside = tempfile::tempdir().unwrap(); let valid = executable(approved.path(), "yt-dlp", 0o700); let unsafe_binary = executable(outside.path(), "yt-dlp", 0o700); - let link = approved.path().join("linked-yt-dlp"); + let link_dir = approved.path().join("linked"); + std::fs::create_dir(&link_dir).unwrap(); + std::fs::set_permissions(&link_dir, std::fs::Permissions::from_mode(0o700)).unwrap(); + let link = link_dir.join("yt-dlp"); symlink(&unsafe_binary, &link).unwrap(); let found = diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs index 41c86284..ae3af258 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/tests/request.rs @@ -65,6 +65,25 @@ fn provider_host_mismatch_is_rejected() { assert!(error.to_string().contains("YouTube URL")); } +#[test] +fn provider_scoped_urls_require_https() { + for (provider, url) in [ + ( + YtDlpProvider::Youtube, + "http://www.youtube.com/watch?v=abcdefghijk", + ), + (YtDlpProvider::Vimeo, "http://vimeo.com/123456"), + ( + YtDlpProvider::Soundcloud, + "http://soundcloud.com/artist/track", + ), + ] { + let error = validation::validate_url(provider, url) + .expect_err("provider-scoped URLs must use HTTPS"); + assert!(error.to_string().contains("must use HTTPS")); + } +} + #[test] fn generic_metadata_rejects_loopback_and_untrusted_hosts() { for url in [ diff --git a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs index fa4eafe0..5f6ccc78 100644 --- a/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs +++ b/src-tauri/src/adapters/driven/plugin/ytdlp_broker/validation.rs @@ -12,12 +12,11 @@ pub(super) fn validate_url(provider: YtDlpProvider, url: &str) -> anyhow::Result bail!("run_ytdlp: URL length is invalid"); } let parsed = reqwest::Url::parse(url).context("run_ytdlp: invalid URL")?; - if !matches!(parsed.scheme(), "http" | "https") { - bail!("run_ytdlp: URL must use HTTP or HTTPS"); + if parsed.scheme() != "https" { + bail!("run_ytdlp: URL must use HTTPS"); } if provider == YtDlpProvider::Generic - && (parsed.scheme() != "https" - || !parsed.username().is_empty() + && (!parsed.username().is_empty() || parsed.password().is_some() || parsed.port().is_some_and(|port| port != 443)) { @@ -137,25 +136,35 @@ pub(super) fn ensure_private_root(root: &Path) -> anyhow::Result<()> { match std::fs::symlink_metadata(root) { Ok(_) => validate_private_root(root), Err(error) if error.kind() == std::io::ErrorKind::NotFound => { - let mut builder = std::fs::DirBuilder::new(); - #[cfg(unix)] - { - use std::os::unix::fs::DirBuilderExt; - builder.mode(0o700); - } - builder.create(root).with_context(|| { - format!( - "run_ytdlp: failed to create private root '{}'", - root.display() - ) - })?; - validate_private_root(root) + create_private_directory(root, "private root") } Err(error) => Err(error) .with_context(|| format!("run_ytdlp: failed to inspect root '{}'", root.display())), } } +fn create_private_directory(path: &Path, description: &str) -> anyhow::Result<()> { + let mut builder = std::fs::DirBuilder::new(); + #[cfg(unix)] + { + use std::os::unix::fs::DirBuilderExt; + builder.mode(0o700); + } + match builder.create(path) { + Ok(()) => {} + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(error) => { + return Err(error).with_context(|| { + format!( + "run_ytdlp: failed to create {description} '{}'", + path.display() + ) + }); + } + } + validate_private_root(path) +} + #[cfg(unix)] pub(super) fn ensure_owned_cache_directory(path: &Path) -> anyhow::Result { ensure_owned_directory_until(path, Path::new("/")) @@ -219,16 +228,7 @@ pub(super) fn ensure_owned_directory_until(path: &Path, anchor: &Path) -> anyhow for component in missing.into_iter().rev() { current.push(component); - let mut builder = std::fs::DirBuilder::new(); - use std::os::unix::fs::DirBuilderExt; - builder.mode(0o700); - builder.create(¤t).with_context(|| { - format!( - "run_ytdlp: failed to create user cache directory '{}'", - current.display() - ) - })?; - validate_private_root(¤t)?; + create_private_directory(¤t, "user cache directory")?; } Ok(current) } @@ -354,3 +354,27 @@ fn has_prefix(path: &Path, prefix: &str) -> bool { .and_then(|name| name.to_str()) .is_some_and(|name| name.starts_with(prefix)) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn already_created_private_directory_is_revalidated() { + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("race-winner"); + std::fs::create_dir(&path).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o700)).unwrap(); + } + + let result = create_private_directory(&path, "private root"); + + assert!( + result.is_ok(), + "race winner must be revalidated: {result:?}" + ); + } +} From c2d55fb21601f0029b7a7f43b0cb19d04d07e0c2 Mon Sep 17 00:00:00 2001 From: Mathieu Piton <27002047+mpiton@users.noreply.github.com> Date: Wed, 15 Jul 2026 19:37:05 +0200 Subject: [PATCH 16/16] fix(downloads): reserve adaptive outputs safely --- CHANGELOG.md | 7 +- src-tauri/src/adapters/driving/tauri_ipc.rs | 181 +++++++++++++++--- .../src/application/commands/store_install.rs | 126 +++++++++++- src-tauri/src/application/test_support.rs | 23 ++- 4 files changed, 304 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92d31a6b..910ee338 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,12 +31,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 checkout credentials, verifies the real WASM export table, builds release assets in a clean directory, and rejects duplicate registry entries. Plugin trust paths are canonicalized against the real filesystem, yt-dlp accepts - HTTPS only and tolerates concurrent private-directory creation, adaptive - downloads reserve destinations without overwrite races, and failed Store - installs clean their staging directory before returning an error. + HTTPS only and tolerates concurrent private-directory creation. ### Fixed +- **Lot 1 install race fixes (MAT-131)**: adaptive downloads atomically reserve + unique destination files before copying, and failed Store installs clean + their staging directory before propagating loader or task errors. - **Lot 0 maintenance baseline (MAT-126–MAT-128)**: restored CI by allowing non-secret `.npmrc` configuration through the shared redacted scanner, updated compatible npm and Rust dependencies to clear security audits, pinned CI actions on their Node 24 releases, kept the codebase clean under the current Rust stable Clippy, and added multi-OS Tauri build diagnostics. - **Task 40 follow-up — PR #153 review fixes round 6** (scope `download`, sprint task 40, PR #153 review): `chatgpt-codex-connector[bot]` flagged a cancellation race in the failover loop. After `AttemptOutcome::Failed`, the loop bumped `mirror_idx`, ran the file/meta cleanup, and unconditionally published `MirrorSwitched` (which `progress_bridge` persists to `current_mirror_index`). A user-cancel that landed in the window between the failed attempt returning and the next attempt starting — including while `tokio::task::spawn_blocking` was running the cleanup — would still bump the persisted cursor through the bridge before the next iteration's `run_mirror_attempt` observed `cancel_token.is_cancelled()` and reported `AttemptOutcome::Cancelled`. The retry would then resume from a slot the user never asked for. Two `cancel_token.is_cancelled()` guards added inside the failover branch: one before bumping `mirror_idx` (catches a cancel that arrived between the attempt finishing and the loop re-entering), one after the cleanup completes (catches a cancel that arrived during the spawn_blocking). Both paths emit `DomainEvent::DownloadCancelled` and break out, so no `MirrorSwitched` ever fires for an aborted switch. - **Task 40 follow-up — PR #153 review fixes round 5** (scope `download`, sprint task 40, PR #153 review): `coderabbitai[bot]` flagged that the round-2 cursor-reset path mapped every `DomainEvent::DownloadFailed` to `MirrorCursorReset`, but `extract_archive::extract_archive_handler` (and the domain `Download::fail()` max-retries path) also publish `DownloadFailed` for post-download errors. A successful download from mirror N followed by an extract / verify failure would zero the cursor and lose the last-known-good slot, so a manual re-download would walk the full mirror list from the top instead of going straight back to the mirror that already produced the bytes. New domain event `DomainEvent::AllMirrorsExhausted { id }` published by the engine's failover loop right before the existing `DownloadFailed`; `progress_bridge` now keys the cursor reset on `AllMirrorsExhausted` and ignores generic `DownloadFailed`. Wired through `tauri_bridge` (`mirrors-exhausted` event name, `{ "id": }` payload) and the download log bridge (silent — no log line). Three tests: `test_event_to_message_maps_all_mirrors_exhausted_to_cursor_reset`, `test_event_to_message_does_not_reset_cursor_on_generic_download_failed` (asserts `event_to_message(&DownloadFailed)` returns `None`), `test_all_mirrors_exhausted_resets_persisted_mirror_cursor_to_zero` (write-through). The previous round-2 tests were renamed in place. diff --git a/src-tauri/src/adapters/driving/tauri_ipc.rs b/src-tauri/src/adapters/driving/tauri_ipc.rs index 98cd359c..e48d38b0 100644 --- a/src-tauri/src/adapters/driving/tauri_ipc.rs +++ b/src-tauri/src/adapters/driving/tauri_ipc.rs @@ -1588,11 +1588,9 @@ fn sanitize_extension(ext: &str) -> Result { /// Errors out after 9999 collisions rather than silently overwriting — that /// branch is meant to *prevent* overwrites, not fall back to them. /// -/// Race condition note: TOCTOU-safe this is not — another process could create -/// the same path between the `exists()` check and the subsequent -/// `rename`/`copy`. That would result in an overwrite. For downloads, the -/// window is small and the alternative (`O_EXCL`-style create + rename) is not -/// available on `std::fs::rename`. Accepted as a practical compromise. +/// This helper only suggests a free path and is not TOCTOU-safe. Callers that +/// write immediately and must prevent overwrites should use +/// [`reserve_unique_destination`] instead. fn unique_destination( dir: &std::path::Path, filename: &str, @@ -1626,6 +1624,54 @@ fn unique_destination( )) } +/// Atomically reserve a unique destination for an adaptive download. +/// +/// Unlike [`unique_destination`], this creates the selected file with +/// `create_new(true)`. A concurrent reservation therefore fails with +/// `AlreadyExists` and retries the next suffix instead of sharing a path. +fn reserve_unique_destination( + dir: &std::path::Path, + filename: &str, +) -> Result<(std::fs::File, std::path::PathBuf, String), String> { + let path = std::path::Path::new(filename); + let stem = path + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or(filename); + let ext = path.extension().and_then(|s| s.to_str()); + + for n in 0..=9999 { + let candidate_name = if n == 0 { + filename.to_string() + } else { + match ext { + Some(e) => format!("{stem} ({n}).{e}"), + None => format!("{stem} ({n})"), + } + }; + let candidate = dir.join(&candidate_name); + match std::fs::OpenOptions::new() + .write(true) + .create_new(true) + .open(&candidate) + { + Ok(file) => return Ok((file, candidate, candidate_name)), + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(error) => { + return Err(format!( + "failed to reserve destination {}: {error}", + candidate.display() + )); + } + } + } + + Err(format!( + "too many existing files named like {filename:?} in {}", + dir.display() + )) +} + /// Extract the hostname from a URL string (e.g. "www.youtube.com" from /// "https://www.youtube.com/watch?v=..."). Returns `None` when the URL /// cannot be parsed. @@ -1802,19 +1848,36 @@ fn resolve_media_stream( dest_dir.display() ) })?; - let (dest_path, dest_filename) = unique_destination(&dest_dir, &filename) - .map_err(|e| format!("failed to select unique destination: {e}"))?; - - if std::fs::rename(&produced_canonical, &dest_path).is_err() { - std::fs::copy(&produced_canonical, &dest_path) + let (mut destination, dest_path, dest_filename) = + reserve_unique_destination(&dest_dir, &filename) + .map_err(|e| format!("failed to select unique destination: {e}"))?; + + let copy_result = (|| -> Result<(), String> { + let mut source = std::fs::File::open(&produced_canonical) + .map_err(|e| format!("failed to open merged file: {e}"))?; + std::io::copy(&mut source, &mut destination) .map_err(|e| format!("failed to copy merged file: {e}"))?; - if let Err(e) = std::fs::remove_file(&produced_canonical) { + Ok(()) + })(); + drop(destination); + + if let Err(error) = copy_result { + if let Err(cleanup_error) = std::fs::remove_file(&dest_path) { tracing::warn!( - path = %produced_canonical.display(), - error = %e, - "failed to remove temp file after copy" + path = %dest_path.display(), + error = %cleanup_error, + "failed to remove incomplete destination file" ); } + return Err(error); + } + + if let Err(e) = std::fs::remove_file(&produced_canonical) { + tracing::warn!( + path = %produced_canonical.display(), + error = %e, + "failed to remove temp file after copy" + ); } Ok(StreamResolution::LocalFile { @@ -3438,7 +3501,10 @@ mod tests { use crate::domain::model::views::StatsPeriod; use crate::domain::ports::driven::PluginLoader; use crate::domain::ports::driven::plugin_loader::DownloadedFileInfo; + use std::collections::HashSet; use std::path::PathBuf; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::{Arc, Barrier}; #[derive(Clone)] struct MetadataPluginLoader { @@ -3477,8 +3543,20 @@ mod tests { } } - #[derive(Clone)] - struct AdaptiveDownloadPluginLoader; + #[derive(Clone, Default)] + struct AdaptiveDownloadPluginLoader { + ready: Option>, + sequence: Arc, + } + + impl AdaptiveDownloadPluginLoader { + fn synchronized(downloads: usize) -> Self { + Self { + ready: Some(Arc::new(Barrier::new(downloads))), + sequence: Arc::new(AtomicUsize::new(0)), + } + } + } impl PluginLoader for AdaptiveDownloadPluginLoader { fn load(&self, _manifest: &PluginManifest) -> Result<(), DomainError> { @@ -3522,14 +3600,18 @@ mod tests { let output_dir = PathBuf::from(output_dir); std::fs::create_dir_all(&output_dir) .map_err(|e| DomainError::StorageError(e.to_string()))?; - let unique = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_nanos(); - let path = output_dir.join(format!("adaptive-{unique}.mp4")); - std::fs::write(&path, b"merged") + let sequence = self.sequence.fetch_add(1, Ordering::SeqCst); + let path = output_dir.join(format!("adaptive-{sequence}.mp4")); + let contents = format!("merged-{sequence}"); + std::fs::write(&path, contents.as_bytes()) .map_err(|e| DomainError::StorageError(e.to_string()))?; - Ok(DownloadedFileInfo { path, size: 6 }) + if let Some(ready) = &self.ready { + ready.wait(); + } + Ok(DownloadedFileInfo { + path, + size: contents.len() as u64, + }) } } @@ -3913,7 +3995,7 @@ mod tests { let configured_dir = configured_root.path().join("custom-downloads"); let resolution = resolve_media_stream( - &AdaptiveDownloadPluginLoader, + &AdaptiveDownloadPluginLoader::default(), "https://example.com/video", "720p", "mp4", @@ -3931,7 +4013,7 @@ mod tests { } => { assert!(path.starts_with(&configured_dir)); assert_eq!(filename, "Adaptive Title.mp4"); - assert_eq!(size, 6); + assert_eq!(size, 8); assert!(path.exists()); } StreamResolution::CdnUrl(url) => { @@ -3940,6 +4022,55 @@ mod tests { } } + #[test] + fn test_resolve_media_stream_reserves_distinct_destinations_concurrently() { + const DOWNLOADS: usize = 16; + + let configured_root = tempfile::tempdir().expect("temp dir"); + let configured_dir = configured_root.path().join("custom-downloads"); + let loader = AdaptiveDownloadPluginLoader::synchronized(DOWNLOADS); + + let workers = (0..DOWNLOADS) + .map(|_| { + let configured_dir = configured_dir.clone(); + let loader = loader.clone(); + std::thread::spawn(move || { + resolve_media_stream( + &loader, + "https://example.com/video", + "720p", + "mp4", + false, + Some("Concurrent Title".to_string()), + Some(configured_dir), + ) + .expect("adaptive stream should resolve to a local file") + }) + }) + .collect::>(); + + let mut paths = HashSet::new(); + let mut contents = HashSet::new(); + for worker in workers { + match worker.join().expect("download worker should not panic") { + StreamResolution::LocalFile { path, .. } => { + contents.insert(std::fs::read(&path).expect("destination should be readable")); + paths.insert(path); + } + StreamResolution::CdnUrl(url) => { + panic!("expected local file resolution, got CDN URL: {url}") + } + } + } + + assert_eq!(paths.len(), DOWNLOADS, "destinations must not collide"); + assert_eq!( + contents.len(), + DOWNLOADS, + "concurrent downloads must not overwrite each other's contents" + ); + } + #[test] fn test_load_plugin_media_metadata_dispatches_by_extract_links_kind() { let extract_links = serde_json::json!({ diff --git a/src-tauri/src/application/commands/store_install.rs b/src-tauri/src/application/commands/store_install.rs index c4f1ff00..a7b27d7b 100644 --- a/src-tauri/src/application/commands/store_install.rs +++ b/src-tauri/src/application/commands/store_install.rs @@ -84,13 +84,13 @@ impl CommandBus { // final `load()` to fail with `AlreadyExists`. let loader = self.plugin_loader_arc(); let staging_for_cleanup = plugin_dir.clone(); - tokio::task::spawn_blocking(move || match download.provenance { + let install_result = tokio::task::spawn_blocking(move || match download.provenance { Some(provenance) => loader.load_official_from_dir(&plugin_dir, &provenance), None => loader.load_from_dir(&plugin_dir), }) .await - .map_err(|e| AppError::Plugin(format!("plugin install task failed: {e}")))? - .map_err(AppError::from)?; + .map_err(|e| AppError::Plugin(format!("plugin install task failed: {e}"))) + .and_then(|result| result.map_err(AppError::from)); // Staging is only meaningful until `load_from_dir` has copied the // files to the permanent plugins directory. After that the staged @@ -107,6 +107,8 @@ impl CommandBus { ); } + install_result?; + tracing::info!(plugin = %cmd.name, "plugin installed from store"); Ok(()) } @@ -129,10 +131,75 @@ impl CommandBus { mod tests { use super::*; use crate::application::commands::store_refresh::write_cache; - use crate::domain::model::plugin::PluginCategory; + use crate::domain::error::DomainError; + use crate::domain::model::plugin::{PluginCategory, PluginInfo, PluginManifest}; use crate::domain::model::plugin_store::{PluginStoreEntry, PluginStoreStatus}; + use crate::domain::ports::driven::plugin_store_client::StorePluginDownload; + use crate::domain::ports::driven::{PluginLoader, PluginStoreClient}; + use std::path::{Path, PathBuf}; + use std::sync::Arc; use tempfile::TempDir; + struct StagingStoreClient { + directory: PathBuf, + } + + impl PluginStoreClient for StagingStoreClient { + fn fetch_registry(&self) -> Result, DomainError> { + Ok(Vec::new()) + } + + fn download_plugin(&self, _entry: &PluginStoreEntry) -> Result { + Ok(self.directory.clone()) + } + + fn download_store_plugin(&self, _name: &str) -> Result { + Ok(StorePluginDownload { + directory: self.directory.clone(), + provenance: None, + min_vortex_version: None, + }) + } + } + + enum LoaderFailure { + Error, + Panic, + } + + struct FailingDirectoryLoader { + failure: LoaderFailure, + } + + impl PluginLoader for FailingDirectoryLoader { + fn load(&self, _manifest: &PluginManifest) -> Result<(), DomainError> { + Ok(()) + } + + fn load_from_dir(&self, _dir: &Path) -> Result<(), DomainError> { + match self.failure { + LoaderFailure::Error => Err(DomainError::PluginError("load failed".into())), + LoaderFailure::Panic => panic!("loader panicked"), + } + } + + fn unload(&self, _name: &str) -> Result<(), DomainError> { + Ok(()) + } + + fn resolve_url(&self, _url: &str) -> Result, DomainError> { + Ok(None) + } + + fn list_loaded(&self) -> Result, DomainError> { + Ok(Vec::new()) + } + + fn set_enabled(&self, _name: &str, _enabled: bool) -> Result<(), DomainError> { + Ok(()) + } + } + fn make_entry(name: &str, version: &str) -> PluginStoreEntry { PluginStoreEntry { name: name.into(), @@ -182,6 +249,57 @@ mod tests { assert_eq!(found.unwrap().version, "1.0.0"); } + async fn install_with_failure(failure: LoaderFailure) -> AppError { + let tmp = TempDir::new().unwrap(); + let cache = tmp.path().join("cache.json"); + write_cache(&cache, &[make_entry("my-plugin", "1.0.0")]).unwrap(); + + let staging = tmp.path().join("staging"); + std::fs::create_dir_all(&staging).unwrap(); + std::fs::write(staging.join("plugin.wasm"), b"wasm").unwrap(); + + let bus = crate::application::test_support::make_store_command_bus( + Arc::new(FailingDirectoryLoader { failure }), + Arc::new(StagingStoreClient { + directory: staging.clone(), + }), + ); + + let result = bus + .handle_store_install( + StoreInstallCommand { + name: "my-plugin".into(), + }, + &cache, + ) + .await; + + let error = result.expect_err("installation must propagate the loader failure"); + assert!( + !staging.exists(), + "staging directory must be removed before the install error propagates" + ); + error + } + + #[tokio::test] + async fn test_store_install_cleans_staging_after_loader_error() { + let error = install_with_failure(LoaderFailure::Error).await; + assert!(matches!( + error, + AppError::Domain(DomainError::PluginError(message)) if message == "load failed" + )); + } + + #[tokio::test] + async fn test_store_install_cleans_staging_after_join_error() { + let error = install_with_failure(LoaderFailure::Panic).await; + assert!(matches!( + error, + AppError::Plugin(message) if message.starts_with("plugin install task failed:") + )); + } + #[test] fn test_is_version_compatible_running_equal() { assert!(is_version_compatible("1.0.0", "1.0.0")); diff --git a/src-tauri/src/application/test_support.rs b/src-tauri/src/application/test_support.rs index 0d5f3292..40e54fae 100644 --- a/src-tauri/src/application/test_support.rs +++ b/src-tauri/src/application/test_support.rs @@ -34,7 +34,7 @@ use crate::domain::ports::driven::{ AccountRepository, ArchiveExtractor, ClipboardObserver, ConfigStore, CredentialStore, DownloadEngine, DownloadReadRepository, DownloadRepository, EventBus, FileStorage, HistoryRepository, HttpClient, PackageReadRepository, PluginLoader, PluginReadRepository, - StatsRepository, + PluginStoreClient, StatsRepository, }; fn host_component(url: &str) -> Option<&str> { @@ -455,6 +455,27 @@ pub(crate) fn make_history_command_bus(history: Arc) -> C ) } +/// Build a [`CommandBus`] wired for Store installation handler tests. +pub(crate) fn make_store_command_bus( + plugin_loader: Arc, + plugin_store_client: Arc, +) -> CommandBus { + CommandBus::new( + Arc::new(StubDownloadRepo), + Arc::new(StubDownloadEngine), + Arc::new(StubEventBus), + Arc::new(StubFileStorage), + Arc::new(StubHttpClient), + plugin_loader, + Arc::new(StubConfigStore), + Arc::new(StubCredentialStore), + Arc::new(StubClipboardObserver), + Arc::new(StubArchiveExtractor), + Arc::new(NoopHistoryRepo), + Some(plugin_store_client), + ) +} + // ── Stub read-side adapters for `QueryBus` construction ────────────── struct StubDownloadReadRepo;