tessera-ctl/.planning/phases/18-desktop-client-fertigstellen/18-REVIEW.md

---
phase: 18-desktop-client-fertigstellen
reviewed: 2026-09-16T00:00:00Z
depth: standard
files_reviewed: 23
files_reviewed_list:
  - apps/api/src/desktop/desktop.controller.ts
  - apps/api/src/desktop/desktop.service.ts
  - apps/api/src/desktop/desktop.module.ts
  - apps/api/src/desktop/desktop.service.spec.ts
  - apps/api/src/app.module.ts
  - apps/api/Dockerfile
  - apps/desktop/src-tauri/src/lib.rs
  - apps/desktop/src/setup.html
  - apps/desktop/src-tauri/capabilities/default.json
  - apps/desktop/src-tauri/tauri.conf.json
  - apps/desktop/src-tauri/Cargo.toml
  - apps/web/src/lib/desktop.ts
  - apps/web/src/lib/desktop.test.ts
  - apps/web/src/components/desktop/desktop-download-links.tsx
  - apps/web/src/components/settings/desktop-app-settings.tsx
  - apps/web/src/components/settings/settings-sidebar.tsx
  - apps/web/src/app/(auth)/login/page.tsx
  - apps/web/src/app/(portal)/settings/general/desktop/page.tsx
  - .gitea/workflows/ci.yml
  - .gitea/scripts/desktop-collect.sh
  - .gitea/scripts/desktop-version.sh
  - .gitea/scripts/publish-images.sh
  - .gitea/scripts/publish-release.sh
findings:
  critical: 1
  warning: 3
  info: 2
  total: 6
status: clean
fixed_at: 2026-09-16T17:40:00Z
fix_report: 18-REVIEW-FIX.md
---

# Phase 18: Code Review Report

**Reviewed:** 2026-09-16T00:00:00Z
**Depth:** standard
**Files Reviewed:** 23
**Status:** clean (all Critical/Warning findings fixed — see `18-REVIEW-FIX.md`)

## Summary

Reviewed the desktop-client-fertigstellen phase: the new `apps/api/src/desktop/` module (public `latest`/`download` routes), the Tauri client's server-address setup flow (`check_server`/`save_server_url`, tray/update UI in `lib.rs`), the web download surfaces (login page link, Settings → Allgemein → Desktop-App), and the CI/CD pipeline that builds, collects, and publishes the desktop packages (`ci.yml`, `desktop-collect.sh`, `desktop-version.sh`, `publish-images.sh`, `publish-release.sh`).

Overall the phase is careful about the things it calls out as security-sensitive: the CI scripts build all JSON with `jq -n`/`--arg` (no manual string concatenation), the Gitea release token is only ever passed to curl via a header file (never on the command line or in a URL), temp files holding the token are created under a `umask 077` directory, and the HTTP-facing platform parameter on `GET /desktop/download/:platform` is whitelisted before any filesystem access (verified against real path-traversal-style HTTP requests in `desktop.service.spec.ts` Test 4). The Tauri capability/CSP surface and version-check flow largely match the locked decisions in `18-CONTEXT.md` (D-08 no network dependency, D-09 no signing).

One genuine gap was found in the second-layer defense against a tampered `manifest.json` (`desktop.service.ts`, D-10/T-18-02): the "defense in depth" filename regex does not reject filenames composed only of dots, so an entry name of `".."` passes the check and `path.join()`s outside `desktop-dist/`. This is not reachable from the public HTTP request today (the manifest is CI-written, not request-controlled), but it is precisely the case the code's own comment says this check exists to block, and it should be fixed to actually do so, especially since the same guard pattern may get reused elsewhere. Three warnings and two info items round out the rest of the findings — none of them break the stated D-10/D-08/D-09 decisions on their own, but they're worth cleaning up.

## Critical Issues

### CR-01: Path-traversal defense-in-depth regex accepts dot-only filenames

**File:** `apps/api/src/desktop/desktop.service.ts:111`
**Issue:** Step (4) is documented as "Verteidigung in der Tiefe (T-18-02): auch ein manipuliertes Manifest darf nicht aus dem Ordner hinausfuehren" — the whole point is that even if `manifest.json`'s `files[platform].name` were corrupted/attacker-influenced, the regex should stop it from resolving outside `desktopDistDir`. The regex used is:

```ts
if (!/^[A-Za-z0-9._-]+$/.test(entry.name)) {
  throw new NotFoundException(`No package for platform: ${knownPlatform}`);
}
```

`.`  and `-` are both allowed characters, so a name of exactly `".."` (or `"."`, `"..."`, etc.) passes this test — it contains only characters from the allowed class. Verified directly:
```
node -e "console.log(/^[A-Za-z0-9._-]+$/.test('..'))"   // true
node -e "console.log(require('path').join('/app/desktop-dist','..'))"  // '/app'
```
With `entry.name === '..'`, `path.join(this.desktopDistDir, entry.name)` resolves to the *parent* of `desktop-dist/` (e.g. `/app` in the container image). `fs.existsSync('/app')` is `true` (it's a directory), so the code proceeds to `fs.createReadStream('/app')`, which will emit an `EISDIR` stream error rather than serving a file — not a full data-exfiltration primitive by itself, but it is a real escape of the intended containment boundary, defeats the explicitly-documented guarantee, and produces an unhandled stream-error path (headers already sent) instead of the intended 404. The unit test suite (`desktop.service.spec.ts` Test 7) only exercises `"../x.AppImage"` (rejected because of the `/`), not a bare `".."`/`"."`, so this gap has no test coverage either.

Current exploitability requires `manifest.json` itself to be corrupted or attacker-controlled (today it is written exclusively by `desktop-collect.sh` in CI), so the live attack surface is currently narrow — but the code and the phase's own decision record (D-10, T-18-02) both frame this exact line as the safety net for that scenario, and it doesn't hold.

**Fix:** Don't rely on a character whitelist alone; verify the resolved path is still inside `desktopDistDir`, and/or explicitly reject `.`/`..` segments:
```ts
if (
  !/^[A-Za-z0-9._-]+$/.test(entry.name) ||
  entry.name === '.' ||
  entry.name === '..'
) {
  throw new NotFoundException(`No package for platform: ${knownPlatform}`);
}
const filePath = path.join(this.desktopDistDir, entry.name);
const resolvedRoot = path.resolve(this.desktopDistDir) + path.sep;
if (!path.resolve(filePath).startsWith(resolvedRoot)) {
  throw new NotFoundException(`No package for platform: ${knownPlatform}`);
}
```
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

**File:** `apps/desktop/src-tauri/tauri.conf.json:24`
**Issue:** `app.security.csp` is:
```json
"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'"
```
This CSP applies to the app's own bundled page (`apps/desktop/src/setup.html`) — the only local page the app serves. That page is a static, inline `<style>`/`<script type="module">` document that calls `eval()` nowhere, loads no remote fonts/images/styles, and only talks to the Tauri IPC bridge (`window.__TAURI__.core.invoke`). Granting `'unsafe-eval'` and wildcard `connect-src`/`img-src`/`font-src`/`style-src` removes CSP's protection against script injection (e.g. via a future dependency compromise or a bug that echoes untrusted content into the DOM) for no functional benefit — none of the permissive directives are exercised by the current page.
**Fix:** Tighten to what `setup.html` actually needs, e.g.:
```json
"csp": "default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data:; connect-src 'self'"
```
`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`
**Issue:** The update check compares only the numeric semantic version:
```rust
#[derive(serde::Deserialize)]
struct DesktopLatest {
    version: String,
}
...
if info.version != app_version {
```
`desktop-version.sh` (D-07) sets both `tauri.conf.json`'s `version` and `Cargo.toml`'s `version` from the *last reachable release tag*, not a fresh per-build number — so on `main` (beta channel), every commit between two tags produces a new build/beta package (`Tessera-X.Y.Z-beta.<sha>.AppImage`) whose `env!("CARGO_PKG_VERSION")` and whose freshly-published `manifest.json.version` are numerically identical (both `X.Y.Z` from the same last tag). A user running an older beta build from three commits ago will never be notified that a newer beta package exists, because the only field compared (`version`) hasn't changed — even though `commit`/`channel` in the JSON response did. This directly undermines D-13's stated purpose ("bei abweichender Version Benachrichtigung 'Neue Version X.Y.Z verfuegbar'") for anyone tracking the beta channel between tags.
**Fix:** Either compare `commit` as well when `channel == "beta"`, or accept this as an intentional scope limit (only tagged releases trigger the notice) and document it explicitly in `docs/anleitung-anwender.md`/`anleitung-betrieb.md` so it isn't mistaken for a bug later. If fixed in code:
```rust
#[derive(serde::Deserialize)]
struct DesktopLatest {
    version: String,
    channel: String,
    commit: String,
}
...
let app_commit = option_env!("APP_COMMIT").unwrap_or("");
let is_newer = info.version != app_version || (info.channel == "beta" && info.commit != app_commit);
```
(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`

**File:** `.gitea/scripts/desktop-collect.sh:66-77`
**Issue:** The `case` pattern (`[0-9]*.[0-9]*.[0-9]*`) is a loose glob check that's immediately followed by a strict `grep -qE '^[0-9]+\.[0-9]+\.[0-9]+$'` doing the actual validation — the outer `case` only decides whether to run the `grep`, but the `*)` fallback arm duplicates the exact same error message and exit. The glob branch adds no protection the grep doesn't already provide on its own.
**Fix:** Collapse to a single check:
```sh
if ! printf '%s' "$VERSION" | grep -qE '^[0-9]+\.[0-9]+\.[0-9]+$'; then
  echo "Version '$VERSION' aus $TAURI_DIR/tauri.conf.json ist nicht rein numerisch (X.Y.Z)." >&2
  exit 1
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_
_Reviewer: Claude (gsd-code-reviewer)_
_Depth: standard_