# Security Audit and Hardening

> **WARNING:** This document describes only the change applied in this PR.
> It may already be outdated once the next PR is merged.
> Historic PR-documentation is not maintained along with new PRs — treat it as a snapshot, not as current documentation.

## Related Links

- ADR 0004 — server-rendered UI, systemd behind the host's reverse proxy (the deployment posture this audit assumes).
- ADR 0005 — opt-in managed nginx/TLS container for hosts without a reverse proxy (e.g. Hostsharing, a multi-tenant host — relevant to the secret-file findings).

## The Audit Summary

A security audit of the Kotlin/Spring source was performed on 2026-07-08.
It found no critical injection or memory-safety class bug — the classic sinks are correctly closed (see [Verified Safe](#verified-safe-checked-not-vulnerable)).
The real weaknesses are deployment-posture and secret-handling issues.

Two threat actors frame the findings:

- Anyone who can push a branch or open a pull request — they influence branch names and the built commit.
- Anyone who can reach the HTTP port — the UI and API are unauthenticated by default and bind `0.0.0.0`.

The whole security model leans on an external reverse proxy providing access control.
Nothing in the app enforces that, and the one in-app control (the control token) is undermined by how it is distributed.
The highest-impact confidentiality issue is a world-readable Gitea-token file on exactly the multi-tenant host GitTally is meant to run on.

This PR tracks the remediation as a checklist.
It may be split into smaller PRs per severity; the filename and scenario IDs use the `PR#000` placeholder until a pull request is opened.

## Non-Goals

- The applicaiton has deliberately no authentication/authorization layer (no Spring Security, no user accounts) — the reverse proxy stays the primary access gate.
- No change to the "build config comes from the primary checkout, never the build worktree" trust model — this PR documents and pins it, it does not rework it.
- No re-audit of the confirmed-safe areas; they are recorded here so future changes do not silently regress them.

## The Identified Issues

The remediation, grouped by severity.
Each item is a TODO with the background that justifies it.

### High

#### TODO 1 — Restrict the Gitea-token config file to the owner at creation

- [ ] In [`InitCommand.kt:86-106`](../../src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt), create `.git/gittally/.gittally.yml` and its parent `.git/gittally/` with `0600`/`0700`, atomically at creation (as [`GitAskPass`](../../src/main/kotlin/de/hoennig/gittally/git/GitAskPass.kt) does), not via plain `writeText` at the umask default.

**Background.**
`init` creates the file it labels "secrets" — where the operator pastes the Gitea API token — with `writeText` and no permission restriction, so it inherits the umask (typically `0644`, world-readable).
The shell path (`tools/setup-gittally-instance:245-253`) already does `chmod 600` on its equivalent file, and `ControlTokenService`/`GitAskPass` both restrict theirs — the Kotlin `init` path is the lone exception.
On a shared host (Hostsharing is a multi-tenant deployment target, per ADR 0005) any local user can then read the token, which grants git push and commit-status access to the repo.

#### TODO 2 — Stop handing the control token to every unauthenticated reader

- [ ] Reconsider the token distribution in [`UiController.kt:186`](../../src/main/kotlin/de/hoennig/gittally/server/UiController.kt) and [`fragments.html:9`](../../src/main/resources/templates/fragments.html): do not embed the live token in public HTML, or gate the pages behind the same check as the mutations.
- [ ] At minimum, document loudly that under the current design any read access equals full write access.

**Background.**
Every server-rendered page embeds the live control token in `<meta name="gittally-control-token">` so [`gittally.js`](../../src/main/resources/static/gittally.js) can read it, but no GET is authenticated.
So `curl -s http://host:18080/ | grep gittally-control-token` yields the token, which unlocks restart/cancel/delete.
Read access therefore equals write access, and the token provides no real second trust tier.
Blast radius is limited to build-lifecycle operations (a DoS/integrity concern, not secret disclosure or RCE), which is why this is a design flaw rather than Critical — but the token gives a false sense of protection.

### Medium

#### TODO 3 — Accept the control token via header only

- [ ] Remove the `token` query-parameter variant from the three mutating endpoints in [`BuildsApiController.kt:90,115,129`](../../src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt); keep only the `X-GitTally-Token` header (which the bundled UI already uses).

**Background.**
Tokens in URLs are routinely written to access logs, reverse-proxy logs, browser history, and the `Referer` header on outbound navigation.
The token never expires (static file secret), so any historical log capture yields a still-valid credential.
The query-param path exists only for legacy convenience.

#### TODO 4 — Redact the Gitea token in `config:print`

- [ ] Mask `git.token` (and any future secret) by default in [`ConfigPrintCommand.kt:20-31`](../../src/main/kotlin/de/hoennig/gittally/commands/ConfigPrintCommand.kt); gate the plaintext value behind an explicit `--show-secrets` flag.
- [ ] Update `tools/setup-gittally-instance:272`, which currently steers the operator to run `config:print --full` to view the token.

**Background.**
Both the `--full` and default branches print the token verbatim to stdout, landing it in terminal scrollback, `script(1)` captures, screen-shares, or CI logs.
There is no redaction and no masked default.

#### TODO 5 — Reduce information disclosure on the unauthenticated read API

- [ ] Decide whether build-log streaming ([`BuildsApiController`](../../src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt) `/api/builds/current/{key}/log`), [`SystemApiController`](../../src/main/kotlin/de/hoennig/gittally/server/SystemApiController.kt), and [`WatcherApiController`](../../src/main/kotlin/de/hoennig/gittally/server/WatcherApiController.kt) should stay fully public, or be gated / scrubbed.

**Background.**
Every read endpoint is public.
Raw build output may contain secrets echoed by build scripts; `/api/system` exposes host metrics; `/api/watcher` exposes last fetch/poll error strings, which can leak git remote URLs or error detail.

### Low / defense-in-depth

#### TODO 6 — Default `bindAddress` to `127.0.0.1`

- [ ] Change the default in [`GitTallyConfig.kt:21`](../../src/main/kotlin/de/hoennig/gittally/config/GitTallyConfig.kt) and the `init` template ([`InitCommand.kt:126`](../../src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt)) from `0.0.0.0` to `127.0.0.1`; require operators to opt into all-interfaces.

**Background.**
The current default binds all interfaces, which — combined with the public read surface and token-in-HTML — exposes the whole UI and the control token to the network whenever the reverse proxy is forgotten.
The deployment doc already recommends `127.0.0.1`; the default and template should match it.

#### TODO 7 — Compare fixed-length hashes in the token check

- [ ] In [`ControlTokenService.kt:35-37`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt), compare `SHA-256(submitted)` against `SHA-256(secret)` with `MessageDigest.isEqual`, so the comparison is always over equal-length buffers.

**Background.**
`MessageDigest.isEqual` returns early on a length mismatch, leaking the token length via timing.
Largely theoretical given the 192-bit CSPRNG token, but a cheap deviation from constant-time best practice to close.

#### TODO 8 — Add `--` before positional refnames in git calls

- [ ] Insert `--` before the branch argument in `checkout`, `fetchBranch`, and `resetHardToOrigin` in [`GitService.kt:145,40,155`](../../src/main/kotlin/de/hoennig/gittally/git/GitService.kt) (e.g. `git switch -- <branch>`).

**Background.**
Git accepts refnames beginning with `-` (verified: `git check-ref-format 'refs/heads/-foo'` exits 0), so a pushed branch name could in principle be read as a git option.
These three methods currently have no production callers and the actively-used paths embed the branch in a `refs/...` prefix, so this is dormant — but the guard should be in place before any of them is wired to server-triggered input.

#### TODO 9 — Pin the "config from primary checkout" invariant

- [ ] Add a code comment / assertion at the config-read sites ([`BuildExecutor.kt:181`](../../src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt), [`DockerBuildRunner.kt`](../../src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt)) and in `AGENTS.md`, stating build config must be read from the primary checkout, never the build worktree.

**Background.**
`buildCommand` is executed via `bash -c "$3"` inside the build container ([`DockerBuildRunner.kt:285`](../../src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt)).
Today that is safe because config comes from `configLoader.load(workingDir)` (the operator's checkout), not the attacker's committed `.gittally.yml`.
The moment a future feature reads per-branch config from the branch itself, this becomes direct RCE — so the invariant should be explicit, not implicit.

#### TODO 10 — Create secret files restricted atomically

- [ ] Set the mode at creation for the control-token file ([`ControlTokenService.kt:29-30`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt)) and the setup-script YAML (`tools/setup-gittally-instance:253`), instead of `chmod 0600` after the write.

**Background.**
Both currently write the file at the umask default and tighten it afterward, leaving a brief window where the secret exists world-readable.
A small TOCTOU gap; `GitAskPass`'s atomic-at-creation approach is the pattern to copy.

## Open Questions

- TODO 2: full fix (gating the pages) versus documentation-only — the pages are the UI, so gating them needs a decision on how operators authenticate; the current implemented behavior is fully public.
- TODO 5: whether public read access is acceptable by design (it matches legacy) or should change; current behavior leaves all reads public.
- TODO 6: changing the default `bindAddress` is a behavior change for existing installs that rely on `0.0.0.0`; needs a migration note.

## Additional Changes

- Update `docs/deployment.md` to state explicitly that the reverse proxy is a hard requirement, not a recommendation, and that read access currently implies write access via the embedded token (until TODO 2 lands).

## Follow-up PRs

- If TODO 2 grows into real UI authentication, that is a separate PR with its own ADR (it revises the "no auth layer" stance of ADR 0004).

## Attachments

### Verified Safe (checked, not vulnerable)

Recorded so future changes do not silently regress these.

- **Path traversal in artifact serving** — [`ArtifactFileController`](../../src/main/kotlin/de/hoennig/gittally/server/ArtifactFileController.kt) normalizes then enforces `startsWith(artifactDir)` and `isRegularFile(..., NOFOLLOW_LINKS)` (rejects symlink escape); [`FileArtifactStore`](../../src/main/kotlin/de/hoennig/gittally/artifacts/FileArtifactStore.kt) whitelists the key to `[A-Za-z0-9._-]+` and requires `dir.parent == branchesDir`.
- **Command injection** — every git/docker/certbot/nginx call uses `ProcessBuilder(List<String>)` ([`GitCommandRunner.kt`](../../src/main/kotlin/de/hoennig/gittally/git/GitCommandRunner.kt)); no `Runtime.exec(String)`, no shell-string interpolation; the only `sh -c`/`bash -c` uses pass data as positional args.
- **Branch-name → filesystem** — always routed through [`ArtifactKeys.sanitize`](../../src/main/kotlin/de/hoennig/gittally/build/ArtifactKeys.kt) (`/` → `_`) plus a SHA suffix, so worktree/container/volume names cannot traverse.
- **XSS** — no `th:utext` in any template; `gittally.js` builds all DOM via `createElement` + `textContent`/`dataset`, no `innerHTML`.
- **SSRF** — the Gitea status proxy validates the commit against `[0-9a-fA-F]{7,40}` ([`StatusApiController.kt`](../../src/main/kotlin/de/hoennig/gittally/server/StatusApiController.kt)); the Gitea base URL/owner/repo/token come from config, not the request.
- **Deserialization** — Jackson JSON/YAML without polymorphic/default typing (no gadget-chain RCE); unreadable state files degrade to empty.
- **Credential transport** — Gitea token sent as an `Authorization` header, never in a URL ([`GiteaClient.kt`](../../src/main/kotlin/de/hoennig/gittally/gitea/GiteaClient.kt)); git auth via env-based `GIT_ASKPASS` with a `0700` script containing no secrets, deleted in `finally`; TLS verification never disabled.
- **nginx/certbot** — `serverName`/`upstreamHost` validated against `[A-Za-z0-9][A-Za-z0-9.-]*` before substitution ([`NginxProxyManager.kt`](../../src/main/kotlin/de/hoennig/gittally/server/NginxProxyManager.kt)); ports range-checked.