13 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 18-desktop-client-fertigstellen | 2026-09-16T00:00:00Z | standard | 23 |
|
|
issues_found |
Phase 18: Code Review Report
Reviewed: 2026-09-16T00:00:00Z Depth: standard Files Reviewed: 23 Status: issues_found
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").
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.
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.)
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.
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
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.
Reviewed: 2026-09-16T00:00:00Z Reviewer: Claude (gsd-code-reviewer) Depth: standard