From a734d9191818a23ae31517b91d6ffc58deebe706 Mon Sep 17 00:00:00 2001 From: mhoennig Date: Tue, 11 Aug 2026 08:00:09 +0200 Subject: [PATCH] Stop embedding the control token in every page (v0.9.10) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes TODO 2 of the security audit in docs/prs/2026-07-08-PR#000. Every rendered page carried the live control token in a meta tag so that gittally.js could send it, but no GET is authenticated — so `curl … | grep gittally-control-token` handed the token to anyone, and read access was effectively write access. Reading stays fully public, which is a requirement rather than an oversight: build states, logs and artifacts must be linkable from Gitea, chats or tickets without a login. Only the distribution of the token changed. The meta tag is gone; gittally.js keeps the token in localStorage and asks for it once per browser, so knowing it requires shell access to `.git/gittally/control-token` on the host. A token the server rejects is dropped and asked for once more, so a rotated secret is not a dead end. As a request header it stays inherently CSRF-safe. The five branches of that flow (first use, reuse, stale token, cancelled prompt, wrong token twice) were exercised against the real source with a throwaway node harness; the UI test now asserts the token does not appear in the rendered page. `docs/deployment.md` gained a "Control Token" section on the public-read/token-protected-write split. Co-Authored-By: Claude --- build.gradle.kts | 2 +- docs/configuration.md | 2 +- docs/deployment.md | 20 ++++++ ...6-07-08-PR#000-security-audit-hardening.md | 13 ++-- .../hoennig/gittally/server/UiController.kt | 2 - src/main/resources/static/gittally.js | 67 +++++++++++++++++-- src/main/resources/templates/fragments.html | 1 - src/main/resources/templates/releases.html | 8 +++ .../gittally/server/UiControllerTest.kt | 4 +- 9 files changed, 103 insertions(+), 16 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index 7e9f089..7ab0dc6 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -11,7 +11,7 @@ plugins { group = "de.hoennig" // bump at least the patch version for every deployment, so the UI footer // (BuildProperties) and --version identify what is actually running -version = "0.9.9" +version = "0.9.10" java { toolchain { diff --git a/docs/configuration.md b/docs/configuration.md index 13fb289..bc118fd 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -179,7 +179,7 @@ branches: ### Notes on `server.bindAddress` The default is `127.0.0.1`. -Neither the web UI nor the JSON API authenticates read access, and every page carries the control token that unlocks the build controls, so GitTally is meant to sit behind the host's reverse proxy rather than on a public interface. +Neither the web UI nor the JSON API authenticates read access — which is intended, so build states and artifacts can be linked from anywhere — so GitTally is meant to sit behind the host's reverse proxy rather than on a public interface. Set `0.0.0.0` only deliberately — for the managed nginx container (which reaches GitTally over the Docker bridge, not over loopback), or when the proxy runs on another host. Installations created before v0.9.9 have `bindAddress: 0.0.0.0` written into their `.gittally.yml` and keep it; the new default only applies where the key is absent or `init` writes a fresh file. diff --git a/docs/deployment.md b/docs/deployment.md index e9bb411..bba8336 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -92,6 +92,26 @@ systemctl --user stop gittally-.service # stop To update GitTally, replace `~/bin/gittally.jar` and restart the service. +## Control Token + +Viewing is public by design: build states, logs and artifacts are readable without any login, so they can be linked from Gitea, chats or tickets. +Only the three mutating actions — restart, cancel, delete — require the control token from `.git/gittally/control-token`, a random secret the server generates on first start (mode `0600`; delete the file to rotate it). + +The token is never embedded in a page. +The first time you press one of the control buttons, the browser asks for it once and keeps it in `localStorage` for that browser; a rejected token is dropped and asked for again. +Read it on the host: + +```bash +cat ~//.git/gittally/control-token +``` + +For scripts, pass it as a header — it is not accepted as a query parameter, because URLs end up in access logs and browser history: + +```bash +curl -X POST -H "X-GitTally-Token: $(cat .git/gittally/control-token)" \ + "https://ci.example.org/api/builds/restart?branch=main" +``` + ## Environment File `.git/gittally/gittally.env` is loaded by the unit as `EnvironmentFile`. diff --git a/docs/prs/2026-07-08-PR#000-security-audit-hardening.md b/docs/prs/2026-07-08-PR#000-security-audit-hardening.md index 2b7a9fb..7535402 100644 --- a/docs/prs/2026-07-08-PR#000-security-audit-hardening.md +++ b/docs/prs/2026-07-08-PR#000-security-audit-hardening.md @@ -52,8 +52,13 @@ On a shared host (Hostsharing is a multi-tenant deployment target, per ADR 0005) #### 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. +- [x] 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. + **Done in v0.9.10, without gating the pages.** Public read access is a requirement, not an oversight: build states, logs and artifacts must stay linkable without a login. + So the `` tag is gone and `gittally.js` keeps the token in `localStorage`, asking for it once per browser (the operator reads it from `.git/gittally/control-token`, which needs shell access to the host). + The token is a real secret again, and as a request header it stays inherently CSRF-safe — a foreign origin cannot set it without a CORS grant. + A login with a session cookie (the alternative that would also hide the buttons from visitors) stays possible later; it would revise ADR 0004 and needs its own PR. +- [x] At minimum, document loudly that under the current design any read access equals full write access. + Superseded: read access no longer implies write access. `docs/deployment.md` gained a "Control Token" section describing the split. **Background.** Every server-rendered page embeds the live control token in `` so [`gittally.js`](../../src/main/resources/static/gittally.js) can read it, but no GET is authenticated. @@ -148,8 +153,8 @@ A small TOCTOU gap; `GitAskPass`'s atomic-at-creation approach is the pattern to ## 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 2: full fix (gating the pages) versus documentation-only.~~ Settled by the operator: unauthenticated **read** access is a requirement — build states and artifacts must stay linkable without a login. So the pages are not gated; only the token distribution changed (v0.9.10). +- TODO 5: settled for the endpoints as such — public reads are by design, as above and as in legacy. What remains open is build-log content: a build script echoing a secret publishes it. Scrubbing or gating just the log endpoint is the only part still worth deciding. - TODO 6: the pinned set is settled (secrets + Gitea/server + `docker.enabled`/`docker.network`); the open point is whether `docker.env` should also be pinned, since a branch overriding it controls its own container's environment (currently proposed as worktree-overridable). - ~~TODO 7: changing the default `bindAddress` is a behavior change for existing installs that rely on `0.0.0.0`; needs a migration note.~~ Settled: the default changed in v0.9.9 and the migration note is in the release notes and both deployment docs. diff --git a/src/main/kotlin/de/hoennig/gittally/server/UiController.kt b/src/main/kotlin/de/hoennig/gittally/server/UiController.kt index 7536304..1406d47 100644 --- a/src/main/kotlin/de/hoennig/gittally/server/UiController.kt +++ b/src/main/kotlin/de/hoennig/gittally/server/UiController.kt @@ -34,7 +34,6 @@ class UiController( private val repository: BuildResultRepository, private val buildExecutor: BuildExecutor, private val artifactStore: ArtifactStore, - private val controlTokens: ControlTokenService, private val configLoader: ConfigLoader, private val metricsCollector: SystemMetricsCollector, private val branchListing: BranchListing, @@ -211,7 +210,6 @@ class UiController( model.addAttribute("repoName", repoName) model.addAttribute("version", buildProperties.getIfAvailable()?.version ?: "dev") model.addAttribute("impressumUrl", config.server.impressumUrl.trim()) - model.addAttribute("controlToken", controlTokens.token()) model.addAttribute("giteaRepoUrl", links.repoUrl ?: "") return links } diff --git a/src/main/resources/static/gittally.js b/src/main/resources/static/gittally.js index 502c2d9..544fd14 100644 --- a/src/main/resources/static/gittally.js +++ b/src/main/resources/static/gittally.js @@ -95,9 +95,46 @@ function metaContent(name) { return element ? element.content : ""; } -const controlToken = metaContent("gittally-control-token"); const giteaRepoUrl = metaContent("gittally-gitea-repo-url"); +// The control token is deliberately NOT embedded in the pages: reading them is +// unauthenticated, so anyone could have read it out of the HTML. The operator +// pastes it once per browser from `.git/gittally/control-token` on the server; +// it is kept in localStorage and only ever sent as a request header. +const CONTROL_TOKEN_KEY = "gittally.controlToken"; + +function storedControlToken() { + try { + return window.localStorage.getItem(CONTROL_TOKEN_KEY) || ""; + } catch (error) { + return ""; // localStorage unavailable (private mode, blocked cookies) + } +} + +function rememberControlToken(token) { + try { + window.localStorage.setItem(CONTROL_TOKEN_KEY, token); + } catch (error) { + // not persistable — the token is asked for again on the next action + } +} + +function forgetControlToken() { + try { + window.localStorage.removeItem(CONTROL_TOKEN_KEY); + } catch (error) { + // nothing to clean up when localStorage is unavailable + } +} + +function askForControlToken() { + const answer = window.prompt( + "Control token — the content of .git/gittally/control-token on the GitTally host:", + "", + ); + return answer ? answer.trim() : ""; +} + async function fetchJson(url) { const response = await fetch(url, { signal: AbortSignal.timeout(FETCH_TIMEOUT_MS) }); if (!response.ok) { @@ -106,15 +143,33 @@ async function fetchJson(url) { return response.json(); } +/** A rejected token is dropped and asked for once more, so a stale one is not a dead end. */ async function sendAction(url, method) { - const response = await fetch(url, { - method, - headers: { "X-GitTally-Token": controlToken }, - signal: AbortSignal.timeout(FETCH_TIMEOUT_MS), - }); + let token = storedControlToken() || askForControlToken(); + if (!token) { + throw new Error("no control token"); + } + let response = await sendWithToken(url, method, token); + if (response.status === 403) { + forgetControlToken(); + token = askForControlToken(); + if (!token) { + throw new Error("wrong control token"); + } + response = await sendWithToken(url, method, token); + } if (!response.ok) { throw new Error("HTTP " + response.status); } + rememberControlToken(token); +} + +function sendWithToken(url, method, token) { + return fetch(url, { + method, + headers: { "X-GitTally-Token": token }, + signal: AbortSignal.timeout(FETCH_TIMEOUT_MS), + }); } function setLiveIndicator(ok, detail) { diff --git a/src/main/resources/templates/fragments.html b/src/main/resources/templates/fragments.html index ab8407c..0a8e195 100644 --- a/src/main/resources/templates/fragments.html +++ b/src/main/resources/templates/fragments.html @@ -6,7 +6,6 @@ GitTally - diff --git a/src/main/resources/templates/releases.html b/src/main/resources/templates/releases.html index 2276c5b..321f02e 100644 --- a/src/main/resources/templates/releases.html +++ b/src/main/resources/templates/releases.html @@ -7,6 +7,14 @@
+

v0.9.10 — 2026-08-11

+
    +
  • The control token is no longer embedded in the pages — reading them is unauthenticated, + so anyone could have picked it out of the HTML. Viewing stays public; the first click on + a restart/cancel/delete button asks for the token once (it is in + .git/gittally/control-token on the host) and keeps it in the browser.
    • +
+

v0.9.9 — 2026-08-11

  • Changed default: server.bindAddress is now 127.0.0.1 diff --git a/src/test/kotlin/de/hoennig/gittally/server/UiControllerTest.kt b/src/test/kotlin/de/hoennig/gittally/server/UiControllerTest.kt index d46e9c1..f3047bc 100644 --- a/src/test/kotlin/de/hoennig/gittally/server/UiControllerTest.kt +++ b/src/test/kotlin/de/hoennig/gittally/server/UiControllerTest.kt @@ -141,7 +141,9 @@ class UiControllerTest : FunSpec() { ).andExpect(content().string(containsString("/builds/main-abc123-key"))) .andExpect(content().string(containsString("""data-action="restart""""))) .andExpect(content().string(containsString("""data-action="delete""""))) - .andExpect(content().string(containsString("""name="gittally-control-token" content="test-token""""))) + // the control token must never reach the browser: reading a page is unauthenticated + .andExpect(content().string(not(containsString("gittally-control-token")))) + .andExpect(content().string(not(containsString("test-token")))) .andExpect(content().string(containsString("https://example.org/imprint"))) .andExpect(content().string(containsString("1:23"))) }