From 3accc174b432cefa1b7079b231bd7a52dfe82f86 Mon Sep 17 00:00:00 2001 From: Schalli Date: Wed, 16 Sep 2026 17:42:40 +0200 Subject: [PATCH] docs(18): Review-Befunde behoben Alle vier Critical/Warning-Befunde aus 18-REVIEW.md sind behoben (CR-01 0d5c80f, WR-01 1b2f803, WR-02 579e24b, WR-03 a8964f1); die beiden Info-Befunde bleiben laut Auftrag unbearbeitet (dokumentiert als uebersprungen). Status auf clean gesetzt. Co-Authored-By: Claude Opus 5 (1M context) --- .../18-REVIEW.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/.planning/phases/18-desktop-client-fertigstellen/18-REVIEW.md b/.planning/phases/18-desktop-client-fertigstellen/18-REVIEW.md index a65c613..c0b2eeb 100644 --- a/.planning/phases/18-desktop-client-fertigstellen/18-REVIEW.md +++ b/.planning/phases/18-desktop-client-fertigstellen/18-REVIEW.md @@ -32,7 +32,9 @@ findings: warning: 3 info: 2 total: 6 -status: issues_found +status: clean +fixed_at: 2026-09-16T17:40:00Z +fix_report: 18-REVIEW-FIX.md --- # Phase 18: Code Review Report @@ -40,7 +42,7 @@ status: issues_found **Reviewed:** 2026-09-16T00:00:00Z **Depth:** standard **Files Reviewed:** 23 -**Status:** issues_found +**Status:** clean (all Critical/Warning findings fixed — see `18-REVIEW-FIX.md`) ## Summary @@ -89,6 +91,8 @@ if (!path.resolve(filePath).startsWith(resolvedRoot)) { ``` Add a unit test asserting `entry.name: '..'` and `entry.name: '.'` in the manifest both yield 404 (mirroring the existing Test 7 for `"../x.AppImage"`). +**Status:** fixed — commit `0d5c80f`. `.`/`..` are now rejected explicitly and the resolved path is additionally checked against `desktopDistDir`. Added Test 7a/7b/7c (`..`, `.`, `../manifest.json`) to `desktop.service.spec.ts`. See `18-REVIEW-FIX.md`. + ## Warnings ### WR-01: Tauri CSP grants `'unsafe-eval'` and wildcard sources that are never needed @@ -105,6 +109,8 @@ This CSP applies to the app's own bundled page (`apps/desktop/src/setup.html`) ``` `connect-src` doesn't need to allow the user-entered server address here because `check_server`/`save_server_url` go through Rust (`reqwest`, `window.navigate`), not `fetch()` from the page itself. If a concrete need for `'unsafe-eval'` or a wildcard source turns up later, add only that directive with a comment explaining why. +**Status:** fixed — commit `1b2f803`. CSP tightened to exactly the suggested value (`default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self'`). Confirmed via Tauri knowledge that this CSP applies only to pages served by the app's own asset protocol (the bundled `setup.html`) — the subsequent `window.navigate()` to the user's server loads a fresh page governed by that server's own headers, not this config. See `18-REVIEW-FIX.md`. + ### WR-02: Desktop update check ignores commit/channel — beta users between tags never see "update available" **File:** `apps/desktop/src-tauri/src/lib.rs:17-20, 199-215` @@ -132,12 +138,16 @@ let is_newer = info.version != app_version || (info.channel == "beta" && info.co ``` (requires threading a build-time commit stamp into the desktop binary, which doesn't currently exist — flagging as a design gap either way.) +**Status:** fixed, requires human verification — commit `579e24b`. `build.rs` now embeds `APP_COMMIT` at compile time via `git rev-parse --short=7 HEAD` (same format `desktop-collect.sh` writes to `manifest.json`); `lib.rs` compares `commit` in addition to `version` when `channel == "beta"`, plain version comparison for `live`. D-07 (plain X.Y.Z in `tauri.conf.json`/`Cargo.toml`) untouched. `cargo check` and `cargo clippy -- -D warnings` are clean. No Rust unit tests exist in this crate to exercise the comparison logic automatically (flagged per the fixer's logic-bug verification policy) — recommend a manual check of a beta build before/after a no-version commit to confirm the notice now appears. See `18-REVIEW-FIX.md`. + ### WR-03: `getManifest()` shallow-validates `files`; malformed entries fall through to string-coerced lookups **File:** `apps/api/src/desktop/desktop.service.ts:48, 104-113` **Issue:** The manifest shape check only verifies `typeof parsed.files === 'object' && parsed.files !== null`, which also accepts an array (`typeof [] === 'object'`). Separately, individual file entries (`manifest.files[platform]`) are never checked for having the required `name`/`size`/`sha256` string/number fields before being used — e.g. if `entry.name` were `undefined` (malformed manifest), `/^[A-Za-z0-9._-]+$/.test(undefined)` coerces to the string `"undefined"`, which matches the regex and proceeds to look for a literal file called `undefined` in `desktop-dist/`. This doesn't currently produce an exploitable outcome (ends in 404), but it's a symptom of `getManifest()` trusting more of the JSON shape than its own docstring claims ("die Grundform nicht stimmt ... 404"), and it means a broken manifest doesn't fail loudly/clearly for whoever is debugging a bad CI run. **Fix:** Validate each present platform entry has `typeof entry.name === 'string' && typeof entry.size === 'number' && typeof entry.sha256 === 'string'` inside `getManifest()`, logging and returning `null` (same pattern already used for the top-level shape check) if not. +**Status:** fixed — commit `a8964f1`. `getManifest()` now also rejects `files` being an array, and validates each present platform entry (`name`: string, `size`: number, `sha256`: 64-char hex string) via a new `isValidManifestFileEntry()` helper; a malformed entry makes the whole manifest treated as missing (404 + warn log), matching the docstring's stated guarantee. Added Test 9 (missing `name`) and Test 10 (non-hex `sha256`) to `desktop.service.spec.ts`. See `18-REVIEW-FIX.md`. + ## Info ### IN-01: Redundant/duplicated version-format validation in `desktop-collect.sh` @@ -152,11 +162,15 @@ if ! printf '%s' "$VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+$'; then fi ``` +**Status:** skipped — Info item, out of scope for this fix run (scope limited to the Critical and Warning findings). No code change made. + ### 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` **Issue:** `{ "identifier": "opener:allow-open-url", "allow": [{ "url": "https://*" }, { "url": "http://*" }] }` lets the Rust side open *any* http/https URL in the system browser — currently only used for the tray "Update herunterladen" item, which opens `{stored server}/settings/general/desktop`. Since the stored server address is arbitrary user input (by design, D-02: no fixed built-in server), this is consistent with the product's multi-tenant intent and isn't a capability-scoping bug per se, but it's broader than strictly necessary (a same-origin-as-configured-server restriction isn't expressible in the static capability file, so this is effectively as tight as it can be made without runtime scoping). Noting for awareness only — no action required unless the opener use expands to cover more than the update link. +**Status:** skipped — Info item, out of scope for this fix run (scope limited to the Critical and Warning findings). No action required per the finding itself. + --- _Reviewed: 2026-09-16T00:00:00Z_