diff --git a/.planning/phases/18-desktop-client-fertigstellen/18-REVIEW-FIX.md b/.planning/phases/18-desktop-client-fertigstellen/18-REVIEW-FIX.md new file mode 100644 index 0000000..d95f670 --- /dev/null +++ b/.planning/phases/18-desktop-client-fertigstellen/18-REVIEW-FIX.md @@ -0,0 +1,85 @@ +--- +phase: 18-desktop-client-fertigstellen +fixed_at: 2026-09-16T17:40:00Z +review_path: .planning/phases/18-desktop-client-fertigstellen/18-REVIEW.md +iteration: 1 +findings_in_scope: 4 +fixed: 4 +skipped: 2 +status: all_fixed +verification_env: main checkout (workflow.use_worktrees=false, no isolated worktree used) +--- + +# Phase 18: Code Review Fix Report + +**Fixed at:** 2026-09-16T17:40:00Z +**Source review:** `.planning/phases/18-desktop-client-fertigstellen/18-REVIEW.md` +**Iteration:** 1 + +**Summary:** +- Findings in scope (Critical + Warning): 4 +- Fixed: 4 +- Skipped (Info, out of scope by instruction): 2 + +Verification ran directly in the main checkout at `/home/vicolab/projects/tessera-ctl` — `.planning/config.json` has `workflow.use_worktrees: false`, so no isolated git worktree was created for this run; per the fixer's setup rules this is the documented, safe opt-out path. + +## Fixed Issues + +### CR-01: Path-traversal defense-in-depth regex accepts dot-only filenames + +**Files modified:** `apps/api/src/desktop/desktop.service.ts`, `apps/api/src/desktop/desktop.service.spec.ts` +**Commit:** `0d5c80f` +**Applied fix:** In `getPackage()` step (4), `entry.name === '.'` and `entry.name === '..'` are now rejected explicitly (the character-class regex alone accepted them since `.` and `-` are both allowed characters). Additionally, the resolved absolute path is now checked to still start with the resolved `desktopDistDir` before any filesystem access, as a second, independent layer of defense against future variants of this pattern if the character whitelist is ever reused elsewhere. Added `desktop.service.spec.ts` Test 7a (`entry.name: '..'` → 404), Test 7b (`entry.name: '.'` → 404), and Test 7c (`entry.name: '../manifest.json'` → 404, matching the exact case named in the fix task). +**Verification:** Tier 1 (re-read, clean) + Tier 2 (`vitest run src/desktop`: 13/13 passing; `tsc --noEmit`: clean). + +### WR-01: Tauri CSP grants `'unsafe-eval'` and wildcard sources that are never needed + +**File modified:** `apps/desktop/src-tauri/tauri.conf.json` +**Commit:** `1b2f803` +**Applied fix:** Tightened `app.security.csp` from `default-src 'self' 'unsafe-inline' 'unsafe-eval'; connect-src *; img-src * data:; font-src * data:; style-src 'self' 'unsafe-inline' *; script-src 'self' 'unsafe-inline' 'unsafe-eval'` to `default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self'` — exactly the value REVIEW.md suggested. Confirmed by reading `setup.html`: it uses no `eval()`, no remote fonts/images/styles, and talks to the Rust side only via `window.__TAURI__.core.invoke` (IPC bridge, not `fetch()`). Confirmed via Tauri knowledge that `app.security.csp` is injected only into responses served by the app's own asset protocol (the bundled frontend, i.e. `setup.html`) — the `window.navigate()` call in `save_server_url()` that follows loads the user's configured server fresh, governed by that server's own response headers, not by this config, so tightening `connect-src` here does not affect the subsequently loaded remote page. +**Verification:** Tier 1 (re-read, clean) + Tier 2 (`node -e "JSON.parse(...)"`: valid JSON). + +### WR-02: Desktop update check ignores commit/channel — beta users between tags never see "update available" + +**Files modified:** `apps/desktop/src-tauri/build.rs`, `apps/desktop/src-tauri/src/lib.rs` +**Commit:** `579e24b` +**Applied fix:** `build.rs` now runs `git rev-parse --short=7 HEAD` at compile time and embeds the result as `APP_COMMIT` via `cargo:rustc-env` (falls back to an empty string if `git` is unavailable, e.g. a source tarball without `.git`). This mirrors exactly the format `desktop-collect.sh` already writes into `manifest.json`'s `commit` field. `lib.rs`'s `DesktopLatest` struct now also deserializes `channel` and `commit` (both already present in every `/desktop/latest` response per `DesktopLatestResponse`); the update-available check is now `info.version != app_version || (info.channel == "beta" && info.commit != app_commit)`, so beta clients see the notice for a newer commit on the same tag-derived version, while the live channel keeps the plain version comparison. D-07 (plain `X.Y.Z` in `tauri.conf.json`/`Cargo.toml`, unaffected by this change) is untouched — `desktop-version.sh` was not modified. +**Verification:** Tier 1 (re-read, clean) + Tier 2 (`cargo check`: clean; `cargo clippy -- -D warnings`: clean, forced re-run via `touch src/lib.rs`). +**Note:** This is a logic-level fix to an async comparison with no existing Rust unit tests in this crate to exercise it automatically (only `cargo check`/`clippy`, which verify syntax/lints, not runtime behavior). Per the fixer's verification policy for logic findings, **this one requires human/manual verification** before relying on it — e.g. building a beta package, bumping only the commit (not the tag-derived version), and confirming the tray notice now appears. Flagged in `18-REVIEW.md`. + +### WR-03: `getManifest()` shallow-validates `files`; malformed entries fall through to string-coerced lookups + +**Files modified:** `apps/api/src/desktop/desktop.service.ts`, `apps/api/src/desktop/desktop.service.spec.ts` +**Commit:** `a8964f1` +**Applied fix:** `getManifest()`'s top-level shape check now also rejects `files` being an array (`Array.isArray(parsed.files)`, since `typeof [] === 'object'` previously slipped through). A new `isValidManifestFileEntry()` helper validates each present platform entry has `name: string`, `size: number`, and `sha256: string` matching a 64-character hex pattern (`/^[a-f0-9]{64}$/i`) — stricter than REVIEW.md's minimum suggestion (which only asked for the three `typeof` checks), matching the fix-task's explicit scope instruction to also validate the sha256 hex format. A malformed entry makes the whole manifest treated as missing (`null` return, same 404 path, plus a `logger.warn`), matching the docstring's stated guarantee that a bad manifest shape means 404. Added Test 9 (manifest with `name` missing on the `linux` entry → `getLatest()` throws `NotFoundException` instead of proceeding to a stringified-`undefined` lookup) and Test 10 (`sha256: 'not-a-hash'` → download 404). +**Verification:** Tier 1 (re-read, clean) + Tier 2 (`vitest run src/desktop`: 13/13 passing; `tsc --noEmit`: clean). + +## Skipped Issues + +### IN-01: Redundant/duplicated version-format validation in `desktop-collect.sh` + +**File:** `.gitea/scripts/desktop-collect.sh:66-77` +**Reason:** Info-severity finding, explicitly out of scope for this fix run per the fix task's scope instruction ("Skip the two Info findings (document as skipped)"). No code change made. +**Original issue:** The `case` glob pattern is immediately followed by a strict `grep -qE` doing the actual validation; the glob branch adds no protection the grep doesn't already provide. + +### IN-02: `apps/desktop/src-tauri/capabilities/default.json` grants `opener:allow-open-url` for any http/https URL + +**File:** `apps/desktop/src-tauri/capabilities/default.json:17` +**Reason:** Info-severity finding, explicitly out of scope for this fix run. The finding itself also states "no action required unless the opener use expands to cover more than the update link" — not a code change candidate even under broader scope. +**Original issue:** Capability allows opening any http/https URL in the system browser; currently only used for the tray "Update herunterladen" link to the user-configured server, consistent with D-02 (no fixed built-in server) and not expressible more tightly in the static capability file. + +## Gate Results + +| Gate | Result | +|------|--------| +| `pnpm --filter @tessera/api exec vitest run src/desktop` | 13/13 passed | +| `pnpm --filter @tessera/api type-check` | clean (`tsc --noEmit`, no errors) | +| `cargo check` (`apps/desktop/src-tauri`) | clean | +| `cargo clippy -- -D warnings` (`apps/desktop/src-tauri`) | clean, no warnings | +| `pnpm --filter @tessera/web exec vitest run` | not run — no web files touched by any of the four fixes | + +--- + +_Fixed: 2026-09-16T17:40:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_