From 27d1165962269ab28a10ed5545a808e734948c04 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Fri, 7 Aug 2026 21:20:26 +0200 Subject: [PATCH] =?UTF-8?q?v0.208.0=20=E2=80=94=20R-254:=20the=20last=20tw?= =?UTF-8?q?o=20secrets=20leave=20the=20page=20source,=20plus=20a=20gate=20?= =?UTF-8?q?against=20a=20fourth?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Site one. app_info.html rendered {{.InitialCreds.Password}} into a hidden span — a REAL per-install credential, read live out of the running container, in the response body of every render. The page now carries the non-secret half plus a boolean; the value comes from POST /apps//initial-credentials/reveal, which RE-READS the container rather than serving a cached copy (caching it in the handler would put it back in the body one layer in). no-store, CSRF-covered, logged as an act. Both buttons go through it. A reveal that cannot read the value SAYS SO rather than returning an empty string that renders as a blank password. Site two, established before changing. The hidden input is NOT the defect and was left alone: it fires only pre-deploy, and README §318 documents why the value must round-trip — the customer notes the generated secrets down and submitting them back is what makes the saved value the same one they saw. The defect was the neighbouring READONLY input, which on an ALREADY-DEPLOYED app rendered the secret into a page with nothing to submit. Fixed by POST /stacks//auto-field/reveal, authorised by requiring a type:secret auto-field of that stack. Both directions pinned. The premise that this contradicted a repo rule does not hold: the rule is CONTEXT.md:2070 'Password fields require explicit input — prevents accidental empty-password deployments', about EMPTINESS. No line in the repo says 'no silent auto-fill'. The gate. scripts/secret_in_markup_gate.py, registered in controller_gates.py, convicts any template expression that names a secret unless allowlisted with a reason. Its limits are MEASURED and in its docstring: it catches a launder through a local variable (the assignment names the secret) but is blind to a secret arriving under a neutral page-data key — verified both ways. That is the shape of site two, which this gate would NOT have caught. The runtime body assertion covers all shapes but only 4 of 27 page templates; the other 23 are R-255, filed rather than glossed. Two nets, different holes, both named. Correction to v0.207.0's report: HTML comments do NOT ship in the response body here — html/template strips them, text/template does not. Measured. A red-proof planting a secret in a comment therefore correctly does not fail. --- CHANGELOG.md | 58 +++++ CONTEXT.md | 33 ++- REUSE.md | 2 +- controller/README.md | 10 + .../web/app_initcreds_exposure_test.go | 202 ++++++++++++++++++ .../web/deploy_secret_exposure_test.go | 86 ++++++++ controller/internal/web/handlers.go | 148 ++++++++++++- controller/internal/web/server.go | 18 ++ .../internal/web/templates/app_info.html | 59 +++-- controller/internal/web/templates/deploy.html | 39 ++++ controller/scripts/controller_gates.py | 1 + controller/scripts/secret_in_markup_gate.py | 121 +++++++++++ 12 files changed, 755 insertions(+), 22 deletions(-) create mode 100644 controller/internal/web/app_initcreds_exposure_test.go create mode 100644 controller/internal/web/deploy_secret_exposure_test.go create mode 100644 controller/scripts/secret_in_markup_gate.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 6110656..d76de9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,61 @@ +## v0.208.0 — the last two secrets leave the page source, and a gate so there is no fourth (2026-08-08, R-254) — MinAgent 0.127.0 + +v0.207.0 removed a password from one page. The census that fix required found two more sites; this +closes both, and adds a check so the next one is caught rather than searched for. + +### 1. An app's first-login password (R-254 site one) + +`app_info.html` rendered `{{.InitialCreds.Password}}` into a `hidden` span — a **real per-install +credential**, read live out of the running container, in the response body of every render. `hidden` +stops a browser DRAWING it and nothing else. + +The page now carries the non-secret half (username, note) plus a boolean; the value comes from +**`POST /apps//initial-credentials/reveal`**, which **re-reads the container** rather than +serving a cached copy — caching it in the handler would put it straight back in the body one layer in. +`no-store`, CSRF-covered, and **logged as an act**. Both buttons (Megjelenítés *and* Másolás) go +through it; neither keeps the value between presses. + +A reveal can now legitimately fail (container stopped, file deleted after first login) and **says so** +— an empty string would have rendered as a blank password. + +### 2. The deploy form — established before changing (R-254 site two) + +The task named the hidden input. **It is not the defect, and it was left alone:** it fires only on the +PRE-DEPLOY form, and `README §318` documents why the value must round-trip — the customer is shown the +generated secrets so they can note them down, and submitting them back is what makes the saved value +the same one they saw ("no silent re-generation on submit"). A form must carry what it submits. + +**The defect was the neighbouring readonly display input.** On an ALREADY-DEPLOYED app the hidden +input is correctly omitted — nothing is being submitted — yet `` still rendered the secret into a page the customer merely opens. That is fixed by +**`POST /stacks//auto-field/reveal`**, authorised by requiring the field to be a `type: secret` +auto-generated field of *that stack's* catalog metadata. Both directions are pinned by tests: the +deployed page must not carry the value, and the pre-deploy form must still submit it. + +**The premise that this contradicted a repo rule does not hold.** The rule is `CONTEXT.md:2070`, +*"Password fields require explicit input — prevents accidental empty-password deployments"*: it is +about EMPTINESS, not auto-fill. No line anywhere in the repo says "no silent auto-fill". + +### 3. A gate, because three instances in two days is a pattern + +`scripts/secret_in_markup_gate.py` (registered in `controller_gates.py`) reads all 36 templates and +convicts any `{{ … }}` whose expression names a secret, unless allowlisted with a stated reason. + +**Its limits are measured, not estimated, and are in its own docstring.** It catches a launder through +a local variable (the assignment names the secret). It is **blind to a secret arriving under a neutral +page-data key** — `data["Tagline"] = creds.Password` then `{{.AppInfo.Tagline}}` passes it cleanly, +verified both ways. That is the shape of site two, which this gate would NOT have caught. + +The complementary net is the runtime body assertion, which catches all of them — but needs each page's +data to be constructible, and **only 4 of 27 page templates have that today**. The other 23 have no +runtime coverage: **R-255**, filed rather than glossed. Two nets, different holes, both named. + +### A correction to v0.207.0's report + +It stated that HTML comments ship in the response body. **They do not, here** — this package renders +with `html/template`, which strips comments (measured: `text/template` keeps them, `html/template` +does not). A red-proof that plants a secret in a comment therefore correctly does **not** fail. + ## v0.207.0 — a password stops living in the page source, and two refusals learn to say what to do (2026-08-08, R-249/R-252/R-253) — MinAgent 0.127.0 Three items the fifth walk exposed by passing. None of them touches the recovery path it proved; all diff --git a/CONTEXT.md b/CONTEXT.md index 659bc21..309162a 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -7,7 +7,38 @@ > > Ask Claude Code: "Please update CONTEXT.md with what we did today" -Last updated: 2026-08-08 (v0.207.0 — R-249/R-252/R-253: a secret leaves the page source, two refusals learn to say what to do) +Last updated: 2026-08-08 (v0.208.0 — R-254: the last two secrets leave the page source, and a gate against a fourth) + +> **2026-08-08 — v0.208.0 (R-254). THE RULE, stated so it outlives this session:** +> +> ### A secret is never in a page's response body. It is fetched by an explicit act, and the act is recorded. +> +> Three instances of one pattern shipped in two days, each found by hand: the retrieval passphrase +> (R-249), an app's real first-login password (R-254 site one), and an already-deployed app's generated +> secret field (R-254 site two). Every one was "hidden" with `display:none`, `hidden`, or +> `type="password"` — **instructions a browser honours when DRAWING and nothing else.** The plaintext +> was in the bytes; a `curl` returned it; caches, history, saved pages and screen-shares had it. +> +> **The shape of the fix, now used three times:** the page carries a BOOLEAN; the value comes from a +> **POST** (so CSRF covers it and it is not re-fetchable from history) with **`Cache-Control: +> no-store`**; the reveal is **LOGGED as an act** — reading a value off markup left no trace anywhere, +> which is why nobody can say whether any of these was ever read. **Per-secret endpoints, never one +> generic "reveal any named secret"** — that would turn three narrow exposures into one lever. +> +> **And the test must assert the RAW RESPONSE BODY.** Every test that asked what the customer *sees* +> passed while the bytes carried the secret. That is precisely how this survived three times. +> +> **What is NOT this defect:** a form must carry what it submits. The pre-deploy hidden input round-trips +> a generated secret deliberately (README §318) so the saved value is the one the customer wrote down. +> The defect there was the neighbouring READONLY input on an already-deployed app, where nothing is +> submitted at all. +> +> **The gate:** `scripts/secret_in_markup_gate.py`. Name-based, all 36 templates, **blind to a secret +> arriving under a neutral page-data key** — measured, not assumed. The complementary runtime +> body-assertion covers 4 of 27 page templates; the other 23 are **R-255**. +> +> **A correction to v0.207.0's report:** it said HTML comments ship in the response body. They do not +> here — `html/template` strips them (`text/template` does not). Measured. > **2026-08-08 — v0.207.0 (R-249, R-252, R-253). Three things the fifth walk exposed BY PASSING.** > The walk closed R-201 (both halves) on 2026-08-07; none of the below touches the recovery path it diff --git a/REUSE.md b/REUSE.md index 9d92a08..774c3d6 100644 --- a/REUSE.md +++ b/REUSE.md @@ -127,7 +127,7 @@ | `EncryptFile` / `DecryptFile` / `IsEncryptedFAB` | controller/internal/appexport/crypto.go | password-based file crypto | .fab export bundles | scrypt-derived AES+HMAC keys | | `maskRepoURL` | controller/internal/sync/sync.go | `(url) string` | Logging git URLs | Strips embedded credentials | | `metrics.RedactLine` | controller/internal/metrics/redact.go | `(s string) string` | ANY log line shipped off-box (issue context, log tails) | Masks password/passwd/secret/token/api-key/authorization/bearer values + 64-hex; apply BEFORE the line leaves the box — controller-side redaction is authoritative | -| `settingsRetrievalPasswordRevealHandler` | controller/internal/web/handlers.go | `POST /settings/retrieval-password/reveal` | **THE PATTERN for showing a secret in the UI** — an XHR that returns only the value | **Never template a secret into a page and hide it with CSS.** `display:none` / `hidden` / `type="password"` stop a browser DRAWING the value; the plaintext is still in the response body, so a `curl` of the page returns it, and it reaches caches, history and any screen-share of the source. R-249 shipped exactly that for two months and was found by it landing in a transcript. The page carries a **boolean** (`HasRetrievalPassword`); the value comes from a POST (CSRF-covered, uncacheable) and the reveal is **logged as an act**. `escrow_handlers.go` states the same rule for R. **Test on the RESPONSE BODY** — a test asserting what the customer *sees* cannot see this class at all. **Known live violations: `app_info.html` (per-install app password in a `hidden` span) and `deploy.html` (auto-generated secret in a `value=`) — R-254.** | +| `settingsRetrievalPasswordRevealHandler` | controller/internal/web/handlers.go | `POST /settings/retrieval-password/reveal` | **THE PATTERN for showing a secret in the UI** — an XHR that returns only the value | **Never template a secret into a page and hide it with CSS.** `display:none` / `hidden` / `type="password"` stop a browser DRAWING the value; the plaintext is still in the response body, so a `curl` of the page returns it, and it reaches caches, history and any screen-share of the source. R-249 shipped exactly that for two months and was found by it landing in a transcript. The page carries a **boolean** (`HasRetrievalPassword`); the value comes from a POST (CSRF-covered, uncacheable) and the reveal is **logged as an act**. `escrow_handlers.go` states the same rule for R. **Test on the RESPONSE BODY** — a test asserting what the customer *sees* cannot see this class at all. **Both R-254 sites are now FIXED the same way** — `POST /apps//initial-credentials/reveal` (re-reads the container, never a cached copy) and `POST /stacks//auto-field/reveal` (authorised on the field being a `type: secret` auto-field of that stack). **Per-secret, never one generic reveal-any-named-secret endpoint.** The PRE-DEPLOY hidden input is deliberate and untouched — a form must carry what it submits (README §318). Enforced by `scripts/secret_in_markup_gate.py`, whose measured blind spot (a secret under a neutral page-data key) is in its docstring; runtime body-assertion covers 4 of 27 pages — R-255. | ### Storage registry + mount detection diff --git a/controller/README.md b/controller/README.md index 398bd97..4fd34ef 100644 --- a/controller/README.md +++ b/controller/README.md @@ -2546,6 +2546,16 @@ During setup wizard drive scan, both current and historical backups are discover Generates `recovery-info.txt` on the system data partition with customer ID, Hub URL, retrieval password, and recovery instructions in Hungarian. Updated on startup and after config changes. Also displayed on the Settings page in a "Vészhelyzeti információk" section. +**No secret is rendered into a page's response body (v0.207.0 + v0.208.0, R-249/R-254).** Three endpoints implement one rule — the page carries a BOOLEAN, the value comes from an explicit authenticated POST with `Cache-Control: no-store`, and the reveal is LOGGED (reading a value off markup left no trace at all): + +| Secret | Endpoint | Read from | +|---|---|---| +| retrieval passphrase | `POST /settings/retrieval-password/reveal` | settings | +| an app's generated first-login password | `POST /apps//initial-credentials/reveal` | **live from the container** — never a cached copy | +| an already-deployed app's auto-generated secret field | `POST /stacks//auto-field/reveal` | the decrypted `app.yaml`; authorised by requiring a `type: secret` auto-field of that stack | + +They are deliberately **per-secret**, not one generic "reveal any named secret" endpoint — that would turn three narrow exposures into one lever with a parameter. **The PRE-DEPLOY hidden input is untouched and deliberate:** a form must carry what it submits (see §318 below). `scripts/secret_in_markup_gate.py` (in `controller_gates.py`) enforces the rule over all templates; its measured blind spot — a secret arriving under a neutral page-data key — is in its docstring. + **The retrieval passphrase is NOT rendered into that page (v0.207.0, R-249).** `securityPageData` passes only `HasRetrievalPassword` (a boolean), and the value is fetched by an explicit act: **`POST /settings/retrieval-password/reveal`** → `{"ok":true,"data":{"password":"…"}}`, behind RequireAuth + CsrfProtect like every other POST, `Cache-Control: no-store`, and logged (`retrieval passphrase revealed via the security page from ` — the value is never logged). Until v0.207.0 the page rendered the plaintext into a `display:none` span, so any fetch of the page returned it; the toggle was cosmetic. This follows the rule `escrow_handlers.go` already states for the recovery code: a secret is revealed by an XHR, never templated server-side into HTML. ### 11. Disaster Recovery diff --git a/controller/internal/web/app_initcreds_exposure_test.go b/controller/internal/web/app_initcreds_exposure_test.go new file mode 100644 index 0000000..764bced --- /dev/null +++ b/controller/internal/web/app_initcreds_exposure_test.go @@ -0,0 +1,202 @@ +package web + +import ( + "bytes" + "io" + "log" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" + "gitea.dooplex.hu/admin/felhom-controller/internal/settings" + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-254 site one — AN APP'S GENERATED FIRST-LOGIN PASSWORD MUST NOT BE IN THE RESPONSE BODY. +// +// The third instance of one pattern in two days (R-249 was the retrieval passphrase; this is a real +// per-install app credential, read live out of the running container). The markup rendered it into +// `` — `hidden` stops a browser DRAWING the value and +// nothing else, so a `curl` of an app's info page returned it in plaintext. +// +// These assert the RAW BODY. A test that asks what the customer *sees* passes on this defect, which +// is exactly how it survived three times. + +const testAppPassword = "TESTONLY-app-initial-pw-7Kq2mZ" + +// renderAppInfo drives the REAL template with the page data shape the handler produces, and returns +// the bytes a browser would receive. +func renderAppInfo(t *testing.T, creds *stacks.ExtractedCreds, hasPassword bool) string { + t.Helper() + s := securityHarness(t) + s.loadTemplates() + data := map[string]interface{}{ + "Page": "stacks", "Title": "Teszt app", "Domain": "example.hu", + "Stack": stacks.Stack{Name: "crafty", Deployed: true, State: "running"}, + "Meta": stacks.Metadata{DisplayName: "Crafty", Slug: "crafty", Category: "media"}, + "AppInfo": stacks.AppInfo{Tagline: "teszt"}, + // The credentials card lives inside {{if .HasAppInfo}} — without this the card never renders + // and every assertion below would pass for the wrong reason. + "HasAppInfo": true, + } + if creds != nil { + data["InitialCreds"] = creds + data["InitialCredsHasPassword"] = hasPassword + } + var buf bytes.Buffer + if err := s.tmpl.ExecuteTemplate(&buf, "app_info", data); err != nil { + t.Fatalf("render app_info: %v", err) + } + return buf.String() +} + +// ── SCENARIO A — the app password is not in the page ──────────────────────────────────────────── + +// RED-PROOF: put `` back into +// app_info.html AND restore `data["InitialCreds"] = creds` in the handler — this fails on the first +// assertion, showing the plaintext returning to the body. That is the defect, reproduced. +// +// NOTE the second assertion, and a CORRECTION to what v0.207.0's report claimed. It said HTML +// comments ship in the response body. **They do not, here:** this package renders with +// `html/template` (server.go), which STRIPS comments — measured: text/template keeps them, +// html/template does not. So a comment cannot leak a secret, and a red-proof planting one in a +// comment correctly does NOT fail. The assertion is kept because it catches the real regression — +// the hidden element itself coming back into the markup. +func TestAppInfoPage_DoesNotContainTheInitialPassword(t *testing.T) { + html := renderAppInfo(t, &stacks.ExtractedCreds{ + Available: true, Username: "admin", Password: testAppPassword, + }, true) + + if strings.Contains(html, testAppPassword) { + t.Error("R-254: the app's first-login password is in the response body of its info page — " + + "a fetch of this page returns a real per-install credential, and the reveal button only " + + "stops a browser DRAWING it") + } + if strings.Contains(html, "initcred-pw-val") { + t.Error("the old hidden-value element is back in the markup") + } + // The feature must survive: the fix removes the VALUE, not the customer's access (Scenario B). + if !strings.Contains(html, "Kezdeti belépési adatok") { + t.Error("the initial-credentials card vanished — the fix must not take the password away " + + "from the person whose app it is") + } + if !strings.Contains(html, "initial-credentials/reveal") { + t.Error("no reveal call rendered, so the customer has no way to obtain the password at all") + } + // The username is NOT a secret and must still render — otherwise the card is useless. + if !strings.Contains(html, "admin") { + t.Error("the username stopped rendering; only the password was supposed to leave the page") + } +} + +// ── SCENARIO E — an app with no generated credentials is unchanged ────────────────────────────── + +// RED-PROOF: make the card unconditional (drop `{{if .InitialCreds}}`) and this fails — an app that +// has no generated credential grows a reveal control for a password that does not exist. +func TestAppInfoPage_NoCredentialsCard_WhenAppHasNone(t *testing.T) { + html := renderAppInfo(t, nil, false) + + if strings.Contains(html, "Kezdeti belépési adatok") { + t.Error("the initial-credentials card rendered for an app with no generated credentials") + } + if strings.Contains(html, "initial-credentials/reveal") { + t.Error("a reveal control rendered for an app that has no password to reveal") + } +} + +// ── SCENARIO B — the customer can still get it, and C — the act is recorded ───────────────────── + +// credsHarness builds a Server with a REAL stack manager carrying one deployed app whose slug is +// "crafty", plus a capturing logger. Only the container READ is seamed — slug resolution is the +// production path, because a reveal answering for the wrong app is the failure mode that matters. +func credsHarness(t *testing.T) (*Server, *bytes.Buffer) { + t.Helper() + dir := t.TempDir() + buf := &bytes.Buffer{} + lg := log.New(io.MultiWriter(buf), "", 0) + cfg := config.Default() + cfg.Customer.Domain = "example.hu" + cfg.Paths.StacksDir = filepath.Join(dir, "stacks") + cfg.Paths.DataDir = filepath.Join(dir, "data") + cfg.Web.SessionSecret = "test-session-secret-abcdef" + + sd := filepath.Join(cfg.Paths.StacksDir, "crafty") + if err := os.MkdirAll(sd, 0o755); err != nil { + t.Fatal(err) + } + os.WriteFile(filepath.Join(sd, ".felhom.yml"), []byte("display_name: Crafty\nslug: crafty\n"), 0o644) + os.WriteFile(filepath.Join(sd, "docker-compose.yml"), []byte("services: {}\n"), 0o644) + os.WriteFile(filepath.Join(sd, "app.yaml"), []byte("deployed: true\n"), 0o644) + + sett, err := settings.Load(filepath.Join(dir, "settings.json"), lg) + if err != nil { + t.Fatal(err) + } + mgr, err := stacks.NewManager(cfg, lg) + if err != nil { + t.Fatal(err) + } + // Discovery is a separate step — the constructor only prepares the directory. + _ = mgr.ScanStacks() // container-status refresh fails on a docker-less host; discovery is enough + if _, ok := mgr.GetStack("crafty"); !ok { + t.Fatal("crafty not discovered by ScanStacks — the fixture would prove nothing") + } + return &Server{cfg: cfg, settings: sett, stackMgr: mgr, logger: lg, version: "test"}, buf +} + +// RED-PROOF for B: delete the route case from server.go (or make the handler always 404) and the +// customer is shown unable to reach their own app password. +// RED-PROOF for C: delete the s.logger.Printf line and the "recorded" assertion fails. +func TestAppInitialCredsReveal_ReturnsThePasswordAndRecordsTheAct(t *testing.T) { + s, logBuf := credsHarness(t) + s.initialCredsFn = func(name string) (*stacks.ExtractedCreds, error) { + return &stacks.ExtractedCreds{Available: true, Username: "admin", Password: testAppPassword}, nil + } + rr := httptest.NewRecorder() + s.appInitialCredsRevealHandler(rr, httptest.NewRequest("POST", "/apps/crafty/initial-credentials/reveal", nil), "crafty") + + if rr.Code != 200 { + t.Fatalf("reveal returned %d, want 200 — the customer cannot get their own app password", rr.Code) + } + if !strings.Contains(rr.Body.String(), testAppPassword) { + t.Error("the reveal did not return the password — Scenario A's fix must not protect the " + + "secret by taking it from its owner") + } + if got := rr.Header().Get("Cache-Control"); !strings.Contains(got, "no-store") { + t.Errorf("Cache-Control = %q, want no-store — a cached reveal is the same defect one layer down", got) + } + // SCENARIO C — the act is recorded, and the VALUE is not. + logged := logBuf.String() + if !strings.Contains(logged, "initial-credential password revealed") { + t.Error("the reveal was not recorded — a silent read is what the markup allowed, and why " + + "nobody can say whether any of these was ever read") + } + if strings.Contains(logged, testAppPassword) { + t.Error("the password was written to the log") + } +} + +// §7.1 — a reveal that cannot read the value SAYS SO. An empty string would render as a blank +// password and read to the customer as "your password is empty". +func TestAppInitialCredsReveal_SaysWhyWhenUnreadable(t *testing.T) { + s, _ := credsHarness(t) + s.initialCredsFn = func(name string) (*stacks.ExtractedCreds, error) { + return &stacks.ExtractedCreds{Available: false}, nil // container stopped / file gone + } + rr := httptest.NewRecorder() + s.appInitialCredsRevealHandler(rr, httptest.NewRequest("POST", "/apps/crafty/initial-credentials/reveal", nil), "crafty") + + if rr.Code != 404 { + t.Errorf("unreadable reveal returned %d, want 404", rr.Code) + } + body := rr.Body.String() + if !strings.Contains(body, "futnia kell") { + t.Errorf("the refusal does not say WHY it could not be read: %s", body) + } + if strings.Contains(body, `"password"`) { + t.Error("an unreadable reveal still carried a password field") + } +} diff --git a/controller/internal/web/deploy_secret_exposure_test.go b/controller/internal/web/deploy_secret_exposure_test.go new file mode 100644 index 0000000..46de6f3 --- /dev/null +++ b/controller/internal/web/deploy_secret_exposure_test.go @@ -0,0 +1,86 @@ +package web + +import ( + "bytes" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-254 site two — WHAT §7.2 ESTABLISHED, PINNED SO IT CANNOT DRIFT BACK. +// +// The deploy page has TWO places a generated secret can appear, and they are NOT the same question: +// +// - the PRE-DEPLOY hidden input — a form must carry what it submits. README §318 documents why: +// the customer is shown the generated secrets so they can note them down, and submitting them +// back is what makes the saved value the SAME one they saw ("no silent re-generation on submit"). +// This is NOT the defect and is deliberately left alone. +// - the READONLY display input on an ALREADY-DEPLOYED app — nothing is being submitted there (the +// hidden input is correctly omitted), yet the value was rendered into the body of a page the +// customer merely opens. That IS R-249's shape, and it is what v0.208.0 fixes. +// +// Both directions are asserted, because "fixed" here means one branch changed and the other did not. + +const testDeploySecret = "TESTONLY-generated-db-pw-Xy91" + +func renderDeployPage(t *testing.T, alreadyDeployed bool) string { + t.Helper() + s := securityHarness(t) + s.loadTemplates() + data := map[string]interface{}{ + "Page": "stacks", "Title": "Telepítés", "Domain": "example.hu", + "Stack": stacks.Stack{Name: "vaultwarden", Deployed: alreadyDeployed}, + "Meta": stacks.Metadata{DisplayName: "Vaultwarden", Slug: "vaultwarden"}, + "AlreadyDeployed": alreadyDeployed, + "AutoFields": []stacks.DeployField{ + {EnvVar: "DB_PASSWORD", Label: "Adatbázis jelszó", Type: "secret"}, + }, + "AutoFieldValues": map[string]string{"DB_PASSWORD": testDeploySecret}, + } + var buf bytes.Buffer + if err := s.tmpl.ExecuteTemplate(&buf, "deploy", data); err != nil { + t.Fatalf("render deploy: %v", err) + } + return buf.String() +} + +// RED-PROOF: drop the `{{if $isDeployed}}` branch so the deployed page renders `value="{{$val}}"` +// again — this fails, showing the secret returning to the body of a page with nothing to submit. +func TestDeployPage_DeployedApp_DoesNotCarryTheSecret(t *testing.T) { + html := renderDeployPage(t, true) + + if strings.Contains(html, testDeploySecret) { + t.Error("R-254 site two: an already-deployed app's generated secret is in the response body " + + "of its settings page — nothing there submits it, so there is no form reason for it to " + + "be in the page at all") + } + // Assert the CONTROL, not the URL: the revealAutoField() function ships in the page script on + // both variants, so a substring match on the endpoint path matches the script and would report a + // control that is not there. (This test caught exactly that on itself.) + if !strings.Contains(html, `onclick="revealAutoField('vaultwarden','DB_PASSWORD'`) { + t.Error("no reveal control rendered, so the customer cannot see their own generated secret") + } + // The hidden input must NOT appear on a deployed app — it never did, and that is the asymmetry + // that makes the readonly input indefensible there. + if strings.Contains(html, ``) { + t.Error("the pre-deploy form no longer submits the generated secret — the saved value would " + + "then not be the one the customer was shown (README §318, 'no silent re-generation on submit')") + } + if strings.Contains(html, `onclick="revealAutoField(`) { + t.Error("the deployed-app reveal control leaked onto the pre-deploy form, where the value is " + + "already legitimately present") + } +} diff --git a/controller/internal/web/handlers.go b/controller/internal/web/handlers.go index 54a2230..fb7f8dd 100644 --- a/controller/internal/web/handlers.go +++ b/controller/internal/web/handlers.go @@ -671,11 +671,29 @@ func (s *Server) appDetailHandler(w http.ResponseWriter, r *http.Request, slug s // Initial auto-generated login (e.g. Crafty writes a random admin password to a file at first // boot). Read it live from the container so the customer doesn't have to dig through logs. Only // for deployed apps that declare an initial_credentials spec; hidden when unreadable. + // + // ⚠ R-254 (v0.208.0) — THE PASSWORD DOES NOT GO INTO THE PAGE DATA, AND THAT IS THE WHOLE FIX. + // + // Until v0.208.0 this handed the whole struct to the template, which rendered the password into + // ``. `hidden` is an attribute the browser honours when + // DRAWING; the plaintext was in the response body of every render, so a `curl` of an app's info + // page returned a real per-install credential. Identical in shape to R-249 one page over, and this + // one is an app the customer actually logs into. + // + // What travels now is the non-secret half (username, note) plus a BOOLEAN. The value is fetched by + // POST /apps//initial-credentials/reveal, which re-reads it LIVE from the container — see + // §7.1: caching it here would put it straight back where it started, one layer in. if found.Deployed && found.Meta.InitialCreds != nil { - if creds, err := s.stackMgr.ReadInitialCredentials(found.Name); err != nil { + if creds, err := s.readInitialCreds(found.Name); err != nil { s.logger.Printf("[WARN] [web] initial-creds for %s: %v", found.Name, err) } else if creds != nil && creds.Available { - data["InitialCreds"] = creds + data["InitialCreds"] = &stacks.ExtractedCreds{ + Available: creds.Available, + Username: creds.Username, + Note: creds.Note, + // Password deliberately NOT carried — the reveal endpoint is the only path to it. + } + data["InitialCredsHasPassword"] = strings.TrimSpace(creds.Password) != "" } } @@ -1662,6 +1680,132 @@ func (s *Server) settingsRetrievalPasswordRevealHandler(w http.ResponseWriter, r escrowJSON(w, http.StatusOK, map[string]any{"password": pw}, "") } +// readInitialCreds is the ONE place an app's generated first-login credential is read — the live +// container read, behind a test seam. Both the info page (which takes only the non-secret half) and +// the reveal endpoint (which takes the value) go through here, so they cannot diverge. +func (s *Server) readInitialCreds(stackName string) (*stacks.ExtractedCreds, error) { + if s.initialCredsFn != nil { + return s.initialCredsFn(stackName) + } + return s.stackMgr.ReadInitialCredentials(stackName) +} + +// appAutoFieldRevealHandler — POST /stacks/{name}/auto-field/reveal (v0.208.0, R-254 site two). +// +// WHAT §7.2 ESTABLISHED, AND WHY THIS EXISTS RATHER THAN A CHANGE TO THE HIDDEN INPUT. +// +// The hidden input (`{{if and (not $isDeployed) (eq .Type "secret")}}`) is NOT this defect. It fires +// only on the PRE-DEPLOY form, and README §318 documents why the value must round-trip: the customer +// is shown the generated secrets so they can write them down, and submitting them back is what makes +// the saved value the SAME one they saw ("no silent re-generation on submit"). A form must carry what +// it submits. +// +// The defect is the neighbouring READONLY display input. On an ALREADY-DEPLOYED app the hidden input +// is correctly omitted — nothing is being submitted — yet `` still renders the secret into the body of a page the customer merely opens. That is +// R-249's shape exactly, with no form to justify it. +// +// PER-SECRET, NOT GENERIC: it serves only fields the CATALOG declares `type: secret` on that stack. +// An env var that is not an auto-generated secret field is refused — that check is the authorisation, +// and it is what stops this becoming "read me any value out of any app's config". +func (s *Server) appAutoFieldRevealHandler(w http.ResponseWriter, r *http.Request, stackName string) { + if s.stackMgr == nil { + escrowJSON(w, http.StatusServiceUnavailable, nil, "Az alkalmazáskezelő nem elérhető.") + return + } + stack, ok := s.stackMgr.GetStack(stackName) + if !ok { + escrowJSON(w, http.StatusNotFound, nil, "Ismeretlen alkalmazás.") + return + } + _ = r.ParseForm() + envVar := strings.TrimSpace(r.FormValue("env_var")) + if envVar == "" { + escrowJSON(w, http.StatusBadRequest, nil, "Hiányzó mező.") + return + } + // AUTHORISATION: the field must be an auto-generated SECRET of this stack's catalog metadata. + allowed := false + for _, f := range stack.Meta.AutoGeneratedFields() { + if f.EnvVar == envVar && f.Type == "secret" { + allowed = true + break + } + } + if !allowed { + s.logger.Printf("[WARN] [web] auto-field reveal refused for %s/%s: not an auto-generated secret field", stackName, envVar) + escrowJSON(w, http.StatusForbidden, nil, "Ez a mező nem kérhető le.") + return + } + appCfg := s.stackMgr.LoadAppConfigByName(stackName) + if appCfg == nil { + escrowJSON(w, http.StatusNotFound, nil, "Az alkalmazás beállításai nem olvashatók.") + return + } + val := crypto.DecryptMap(s.encKey, appCfg.Env)[envVar] + if strings.TrimSpace(val) == "" { + w.Header().Set("Cache-Control", "no-store") + escrowJSON(w, http.StatusNotFound, nil, "Ehhez a mezőhöz nincs mentett érték.") + return + } + s.logger.Printf("[INFO] [web] auto-generated secret revealed for %s/%s from %s (value never logged)", stackName, envVar, clientIP(r)) + w.Header().Set("Cache-Control", "no-store") + escrowJSON(w, http.StatusOK, map[string]any{"value": val}, "") +} + +// appInitialCredsRevealHandler — POST /apps/{slug}/initial-credentials/reveal (v0.208.0, R-254). +// +// The ONLY path by which an app's generated first-login password reaches a browser. Same shape as +// v0.207.0's retrieval-password reveal, deliberately: POST (so CsrfProtect covers it and it is not +// re-fetchable from history), `no-store`, and **logged as an act** — reading it off the markup left +// no trace anywhere, which is why nobody can say whether any of these was ever read. +// +// ⚠ PER-SECRET, NOT GENERIC. This serves exactly one kind of value for one app. A single endpoint +// that returned any named secret would be a worse thing than the defect it fixed: it would turn three +// narrow exposures into one lever with a parameter. +// +// §7.1 — IT RE-READS THE CONTAINER, it does not serve a copy the page already had. Caching the value +// in the handler's page data would put it back in the response body one layer in, which is the defect. +// The consequence is that the reveal can legitimately fail (container stopped, file deleted after +// first login) and it SAYS SO — an empty string here would render as a blank password and read as +// "your password is empty". +func (s *Server) appInitialCredsRevealHandler(w http.ResponseWriter, r *http.Request, slug string) { + if s.stackMgr == nil { + escrowJSON(w, http.StatusServiceUnavailable, nil, "Az alkalmazáskezelő nem elérhető.") + return + } + // Resolved EXACTLY as appDetailHandler resolves it — same loop, same field. A second definition + // of "which app is this slug" is how a reveal ends up answering for a different app than the page + // the customer is looking at. + var found *stacks.Stack + for _, stack := range s.stackMgr.GetStacks() { + if stack.Meta.Slug == slug { + found = &stack + break + } + } + if found == nil { + escrowJSON(w, http.StatusNotFound, nil, "Ismeretlen alkalmazás.") + return + } + creds, err := s.readInitialCreds(found.Name) + if err != nil { + // Never swallowed, and never surfaced raw — the error can name a container/path. + s.logger.Printf("[WARN] [web] initial-creds reveal for %s: %v", found.Name, err) + escrowJSON(w, http.StatusBadGateway, nil, "A kezdeti jelszó beolvasása nem sikerült.") + return + } + if creds == nil || !creds.Available || strings.TrimSpace(creds.Password) == "" { + w.Header().Set("Cache-Control", "no-store") + escrowJSON(w, http.StatusNotFound, nil, + "A kezdeti jelszó most nem olvasható ki — az alkalmazásnak futnia kell hozzá, és lehet, hogy a fájlt az első bejelentkezés után már törölték.") + return + } + s.logger.Printf("[INFO] [web] initial-credential password revealed for %s from %s (value never logged)", found.Name, clientIP(r)) + w.Header().Set("Cache-Control", "no-store") + escrowJSON(w, http.StatusOK, map[string]any{"password": creds.Password}, "") +} + func (s *Server) settingsHandler(w http.ResponseWriter, r *http.Request) { s.executeTemplate(w, r, "settings_system", s.systemPageData()) } diff --git a/controller/internal/web/server.go b/controller/internal/web/server.go index c77dd19..f8d0b15 100644 --- a/controller/internal/web/server.go +++ b/controller/internal/web/server.go @@ -74,6 +74,12 @@ type Server struct { agentCliErr error agentCliOnce sync.Once + // initialCredsFn is the R-254 read seam for an app's generated first-login credential. nil → the + // real live container read (stackMgr.ReadInitialCredentials). ONE definition, used by BOTH the + // info page and the reveal endpoint — two ways to read the same secret is how one of them ends up + // caching it back into the page. + initialCredsFn func(stackName string) (*stacks.ExtractedCreds, error) + // Hub push status callback — set via SetHubPushStatus for monitoring page hubPushStatusFn func() HubPushStatusData @@ -561,6 +567,12 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { s.offboxConfirmEscrowHandler(w, r) case path == "/backup/offbox/inject-password" && r.Method == http.MethodPost: s.offboxInjectPasswordHandler(w, r) + // R-254 site two: an already-deployed app's generated secrets are fetched by an explicit act, + // not rendered into the settings page. The PRE-DEPLOY hidden input is untouched and deliberate + // (README §318) — see the handler for what §7.2 established. + case strings.HasPrefix(path, "/stacks/") && strings.HasSuffix(path, "/auto-field/reveal") && r.Method == http.MethodPost: + name := strings.TrimSuffix(strings.TrimPrefix(path, "/stacks/"), "/auto-field/reveal") + s.appAutoFieldRevealHandler(w, r, name) case strings.HasPrefix(path, "/stacks/") && strings.HasSuffix(path, "/export"): name := strings.TrimPrefix(path, "/stacks/") name = strings.TrimSuffix(name, "/export") @@ -604,6 +616,12 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) { fmt.Fprint(w, AppPlaceholderSVG) case strings.HasPrefix(path, "/static/assets/"): s.serveAsset(w, r, strings.TrimPrefix(path, "/static/assets/")) + // R-254: the app's generated first-login password is fetched by an explicit authenticated act, + // never templated into the info page. Placed BEFORE the /apps/ catch-all so the more specific + // path wins. POST (not GET) so CsrfProtect covers it and it is not cacheable — see the handler. + case strings.HasPrefix(path, "/apps/") && strings.HasSuffix(path, "/initial-credentials/reveal") && r.Method == http.MethodPost: + slug := strings.TrimSuffix(strings.TrimPrefix(path, "/apps/"), "/initial-credentials/reveal") + s.appInitialCredsRevealHandler(w, r, slug) case strings.HasPrefix(path, "/apps/"): slug := strings.TrimPrefix(path, "/apps/") s.appDetailHandler(w, r, slug) diff --git a/controller/internal/web/templates/app_info.html b/controller/internal/web/templates/app_info.html index ab5852d..1a7534a 100644 --- a/controller/internal/web/templates/app_info.html +++ b/controller/internal/web/templates/app_info.html @@ -180,11 +180,14 @@ function appMigrate(btn,app,label){ {{end}} Jelszó + •••••••••••• - - + + @@ -212,33 +215,53 @@ function appMigrate(btn,app,label){ {{if .InitialCreds}} {{end}} diff --git a/controller/internal/web/templates/deploy.html b/controller/internal/web/templates/deploy.html index 6e92b65..46f4c09 100644 --- a/controller/internal/web/templates/deploy.html +++ b/controller/internal/web/templates/deploy.html @@ -471,6 +471,7 @@

Ezek az értékek a telepítéssel együtt mentésre kerülnek. Jegyezze fel a szükséges jelszavakat!

{{end}} {{$autoValues := .AutoFieldValues}} + {{$stackName := .Stack.Name}} {{$isDeployed := .AlreadyDeployed}} {{range .AutoFields}} {{$val := index $autoValues .EnvVar}} @@ -478,10 +479,21 @@ {{if $val}} {{if eq .Type "secret"}} + {{if $isDeployed}} + +
+ + +
+ {{else}}
+ {{end}} {{else}} {{end}} @@ -743,6 +755,33 @@ document.addEventListener('DOMContentLoaded', function() { if (sel) checkStorageSpace(sel); }); +// R-254 site two: on an already-deployed app the value is NOT in this page — it is fetched on +// demand, and the server records the act. Nothing caches it between presses. +function revealAutoField(stackName, envVar, btn) { + var el = document.getElementById('auto-field-' + envVar); + if (!el) return; + if (btn.dataset.shown === '1') { + el.value = ''; + el.type = 'password'; + btn.dataset.shown = ''; + btn.textContent = 'Megjelenítés'; + return; + } + btn.disabled = true; + fetch('/stacks/' + encodeURIComponent(stackName) + '/auto-field/reveal', { + method: 'POST', + headers: Object.assign({'Content-Type': 'application/x-www-form-urlencoded'}, csrfHeaders()), + credentials: 'same-origin', + body: 'env_var=' + encodeURIComponent(envVar) + }).then(function (r) { return r.json(); }).then(function (j) { + btn.disabled = false; + if (!j.ok) { showAlert(j.error || 'A lekérés nem sikerült.'); return; } + el.value = j.data.value; + el.type = 'text'; + btn.dataset.shown = '1'; + btn.textContent = 'Elrejtés'; + }).catch(function () { btn.disabled = false; showAlert('A lekérés nem sikerült.'); }); +} function toggleAutoField(fieldId, btn) { var el = document.getElementById(fieldId); if (!el) return; diff --git a/controller/scripts/controller_gates.py b/controller/scripts/controller_gates.py index 7fbc3d3..f7d32e2 100644 --- a/controller/scripts/controller_gates.py +++ b/controller/scripts/controller_gates.py @@ -57,6 +57,7 @@ GATES = [ ("app-row-dedup", os.path.join(SCRIPTS, "app_row_dedup_gate.py"), [], True), ("mojibake", os.path.join(SCRIPTS, "mojibake_gate.py"), [], True), ("docker-v", os.path.join(SCRIPTS, "docker_run_volume_path_gate.py"), [], True), + ("secret-markup", os.path.join(SCRIPTS, "secret_in_markup_gate.py"), [], True), ("reuse-refs", SHARED_REUSE, [REPO], True), ("instructions", SHARED_INSTRUCTIONS, [REPO], True), ] diff --git a/controller/scripts/secret_in_markup_gate.py b/controller/scripts/secret_in_markup_gate.py new file mode 100644 index 0000000..cf56f7f --- /dev/null +++ b/controller/scripts/secret_in_markup_gate.py @@ -0,0 +1,121 @@ +#!/usr/bin/env python3 +"""secret_in_markup_gate.py — a secret must never be rendered into a template. + +WHY THIS EXISTS. Three instances of ONE pattern shipped in two days, each found by hand: + + R-249 settings_security.html {{.RetrievalPassword}} inside a display:none span + R-254 app_info.html {{.InitialCreds.Password}} inside a `hidden` span + R-254 deploy.html value="{{$val}}" in a readonly type=password input + +Each was "hidden" by an instruction the browser honours when DRAWING and by nothing else, so the +plaintext sat in the response body of a page the customer merely opened. **Hiding is not containment.** +A pattern found three times is not closed by searching a fourth time; it is closed by a check. + +WHAT THIS GATE DOES. It reads every template and convicts any `{{ … }}` action whose expression names +a secret (password / secret / token / credential / passphrase / apikey), unless that exact expression +is on the ALLOWLIST below with a stated reason. + +⚠ WHAT IT DOES *NOT* DO, STATED PLAINLY SO NOBODY READS IT AS COMPLETE COVERAGE. + + 1. It is NAME-BASED, and the hole was MEASURED rather than guessed at. It catches + `{{.InitialCreds.Password}}`, and it also catches a launder through a local variable, because the + ASSIGNMENT names the secret (`{{$v := .InitialCreds.Password}}` is convicted). What it cannot see + is a secret that arrives under a NEUTRAL PAGE-DATA KEY — `data["Tagline"] = creds.Password` then + `{{.AppInfo.Tagline}}` passes this gate cleanly. Verified both ways during the 2026-08-08 session. + The third instance above (`value="{{$val}}"` inside an `{{if eq .Type "secret"}}` branch) is that + shape: this gate would NOT have caught it. + 2. It reasons about TEMPLATES, not about rendered output. A handler that writes a secret into a + neutrally-named page-data key is invisible to it. + 3. Runtime body-assertion — rendering a page with a sentinel and grepping the response — is the + check that catches all three, and it needs each page's data to be constructible. Four pages have + that today (settings_security, app_info, deploy, backups_restore) and each has its own test; the + other 23 page templates do NOT. Closing that gap is R-255. + +So: this is the cheap layer that would have caught two of the three, plus a per-page runtime assertion +for the pages that can afford one. Together they are not a proof; they are two nets with different +holes, and the holes are named above. +""" +import os +import re +import sys + +HERE = os.path.dirname(os.path.abspath(__file__)) +CTRL = os.path.dirname(HERE) +TPL = os.path.join(CTRL, "internal", "web", "templates") + +SECRETY = re.compile(r"pass(word|phrase)|secret|token|credential|apikey|api_key", re.I) +ACTION = re.compile(r"\{\{-?\s*(.*?)\s*-?\}\}", re.S) + +# Expressions that name a secret but are NOT one, each with the reason it is safe. An entry here is a +# claim someone made; it should be short enough to re-check by eye. +ALLOWLIST = { + # booleans / presence flags — the whole point of the R-249 and R-254 fixes + ".HasRetrievalPassword": "boolean: whether one exists, never the value", + ".InitialCredsHasPassword": "boolean: whether one exists, never the value", + ".SharePasswordSet": "boolean: whether a share password is set", + ".PasswordError": "an error MESSAGE for a failed password change, not a password", + ".MinPassword": "the minimum LENGTH policy number", + # form field names and types, not values + 'eq .Type "password"': "a field TYPE discriminator", + 'eq .Type "secret"': "a field TYPE discriminator", + 'eq .Type "secret_input"': "a field TYPE discriminator", + 'if or .Required (eq .Type "password")': "a field TYPE discriminator", + 'if and (not $isDeployed) (eq .Type "secret")': "guards the PRE-DEPLOY hidden input — a form must " + "carry what it submits (README §318); see R-254 site two", + "define \"launcher_share_password\"": "a template name", + # the CSRF token is not a secret in this sense: it is bound to the session and useless without it, + # and it MUST be in the form for the form to work. + ".CSRFToken": "CSRF token — session-bound, must be in the page for any POST to work", + ".CSRFField": "CSRF token — same", +} + + +def check(path): + convictions = [] + src = open(path, encoding="utf-8").read() + for m in ACTION.finditer(src): + expr = m.group(1).strip() + if not SECRETY.search(expr): + continue + if expr in ALLOWLIST: + continue + # `{{if .X}}` / `{{with .X}}` where .X is allowlisted is the same claim as `.X` + bare = re.sub(r"^(if|with|else if)\s+", "", expr).strip() + if bare in ALLOWLIST: + continue + line = src[: m.start()].count("\n") + 1 + convictions.append((line, expr)) + return convictions + + +def main(): + if not os.path.isdir(TPL): + print("secret-in-markup gate INCONCLUSIVE: template dir not found: %s" % TPL) + return 2 + files = sorted(f for f in os.listdir(TPL) if f.endswith(".html")) + if not files: + print("secret-in-markup gate INCONCLUSIVE: no templates found in %s" % TPL) + return 2 + total = 0 + bad = 0 + for f in files: + total += 1 + for line, expr in check(os.path.join(TPL, f)): + bad += 1 + print(" %s:%d renders a secret-named expression into the markup: {{%s}}" % (f, line, expr)) + if bad: + print() + print("SECRET-IN-MARKUP GATE FAILED: %d expression(s) across %d template(s)." % (bad, total)) + print("A secret must not be in the response body of a page the customer merely opens —") + print("hiding it with `hidden` / display:none / type=password stops it being DRAWN and nothing else.") + print("Fix: carry a BOOLEAN in the page data and fetch the value with an explicit authenticated") + print("POST that sets Cache-Control: no-store and logs the act (see R-249's and R-254's endpoints).") + print("If the expression genuinely is not a secret, add it to ALLOWLIST with the reason.") + return 1 + print("secret-in-markup gate OK — %d templates, no secret-named expression rendered" % total) + print(" (NAME-BASED: blind to a secret arriving under a neutral PAGE-DATA key — see the docstring)") + return 0 + + +if __name__ == "__main__": + sys.exit(main())