docs(18): Review-Befunde behoben
Alle vier Critical/Warning-Befunde aus 18-REVIEW.md sind behoben (CR-010d5c80f, WR-011b2f803, WR-02579e24b, WR-03a8964f1); die beiden Info-Befunde bleiben laut Auftrag unbearbeitet (dokumentiert als uebersprungen). Status auf clean gesetzt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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_
|
||||
|
||||
Reference in New Issue
Block a user