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

phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status, fixed_at, fix_report

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status	fixed_at	fix_report
18-desktop-client-fertigstellen	2026-09-16T00:00:00Z	standard	23	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	critical warning info total 1 3 2 6	clean	2026-09-16T17:40:00Z	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:

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:

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:

"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.:

"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:

#[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:

#[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:

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