From 9f436c8a3beb05461d54edb0d56345ed9e8ce9de Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 20 Jul 2026 09:42:24 +0200 Subject: [PATCH] =?UTF-8?q?v0.150.0=20=E2=80=94=20green=20gate=20restored?= =?UTF-8?q?=20+=20the=20export=20link=20stops=20leaking=20the=20CSRF=20tok?= =?UTF-8?q?en?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F7/R-53: app_export.html built the app's public URL as '.{{$.CSRFToken}}', so the "Megnyitás" link was wrong for every app with a subdomain and a session CSRF token was written into a URL. Template now uses {{$.Domain}}, and exportPageHandler supplies the key — it builds its own data map instead of going through baseData, which is where every other page gets it. The page's real CSRF path (csrfH() reading the meta tag) is correct and untouched. The 7 red internal/backup tests are green again, with no behaviour change. TestTier2V2_* / TestSharesTier2* all failed for one environmental reason: Tier-2's off-drive guard asks system.SamePhysicalDevice (st_dev equality) whether a target is really a second disk, and every t.TempDir() here shares one filesystem — so the guard correctly refused the fixture's "two drives" and the tests never reached their subject ("nincs másik fizikai meghajtó"). Seam in the package's existing style: a nil-defaulted Manager.samePhysicalDevice field + sameDevice wrapper, seven call sites routed through it. Nil resolves to system.SamePhysicalDevice, so production is byte-for-byte unchanged; only the two fixtures inject a fake modelling one drive per directory subtree. No assertion weakened, nothing skipped/renamed/deleted; all 7 mutation-proved. Also: the ssh->pct-exec ASCII-grep and heredoc-credential traps are now in CLAUDE.md's live-validation section. --- CHANGELOG.md | 27 +++++++ CLAUDE.md | 9 +++ CONTEXT.md | 16 +++- controller/internal/backup/backup.go | 18 ++++- .../internal/backup/device_seam_test.go | 28 +++++++ controller/internal/backup/shares_test.go | 3 + controller/internal/backup/tier2.go | 10 +-- controller/internal/backup/tier2_shares.go | 3 +- controller/internal/backup/tier2_v2_test.go | 1 + .../internal/web/app_export_domain_test.go | 76 +++++++++++++++++++ controller/internal/web/handler_export.go | 4 + .../internal/web/templates/app_export.html | 2 +- 12 files changed, 187 insertions(+), 10 deletions(-) create mode 100644 controller/internal/backup/device_seam_test.go create mode 100644 controller/internal/web/app_export_domain_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 68fc234..cd06d73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,32 @@ ## Changelog +### v0.150.0 — green gate restored + the export link stops leaking the CSRF token (2026-07-20) + +**F7 / R-53 — `app_export.html` built the app's public URL from the CSRF token.** The line read +`var domain = '.{{$.CSRFToken}}'`, so the „Megnyitás" link was wrong for every app with a +subdomain and a session CSRF token was written into a URL (history, referrers, logs). Two-part fix: +the template token becomes `{{$.Domain}}`, and `exportPageHandler` supplies `Domain` — that handler +builds its own data map instead of going through `baseData`, which is where every other page gets +the key, so the template had nothing to read. The page's real CSRF path (the `csrfH()` helper +reading the meta tag) is correct and untouched. Render tests assert the joined `.` and +that the token appears nowhere on that line; red-proofed against the pre-fix template. + +**The 7 red `internal/backup` tests are green again — no behaviour change.** `TestTier2V2_*` and +`TestSharesTier2*` had been failing on DooPlex since before v0.149.0. Root cause is environmental, +one class for all seven: Tier-2's off-drive guard asks `system.SamePhysicalDevice` (st_dev equality) +whether a candidate target is really a *second* disk, and on a host where every `t.TempDir()` lands +on one filesystem the fixture's "two drives" are indistinguishable — so the guard correctly refused +the target and the tests could never reach their subject. The failure message said so outright: +`nincs másik fizikai meghajtó`. + +Fixed with one behaviour-preserving seam in the package's existing style: a nil-defaulted +`Manager.samePhysicalDevice` field plus a `sameDevice` wrapper, with the seven call sites routed +through it. **Nil → `system.SamePhysicalDevice`, so production is byte-for-byte unchanged**; only +the two test fixtures install a fake that models one drive per directory subtree. No assertion was +weakened, no test skipped, renamed or deleted, and every one of the seven was mutation-proved: the +defect each guards was re-introduced one at a time and each test failed, including the notifier +test's own documented red-proof (`_shares` reaching Hungarian copy). + ### v0.149.0 — the dashboard tells the truth about the last backup (2026-07-20) Closes **F3** from `felhom.eu/documentation/audits/AUDIT-vacation-remote-ops-2026-07-20.md`. diff --git a/CLAUDE.md b/CLAUDE.md index 229f956..fce091d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -110,6 +110,15 @@ state). **`claude-in-chrome` is NOT available in the DooPlex environment** — t endpoint-level: invoke the exact endpoint the UI invokes (no server logic is skipped, only rendering) and say which method was used. Strict end-to-end UI coverage is a manual click-through. +Two traps in that method, both from the 2026-07-20 remediation: +- **Grep the fetched page with ASCII-only substrings.** Accented Hungarian patterns get mangled + through the `ssh → pct exec → bash -c` chain and return a false `0` — which reads exactly like the + banner/string being gone. Use `kezel`, `Utols`, `Biztons`; never let an accented pattern gate a + conclusion (it nearly produced a wrong "banner cleared" claim). +- **Credentials with `!` or `'` break in heredoc-built helper scripts** (history expansion eats + `!!`). Use the proven inline `-d "password=$PW"` form for authed curl, and delete any + credential-bearing helper from `/tmp` (host AND guest) when done. + ## Environment & access Claude Code runs **on DooPlex (192.168.0.180, Debian 13, user `kisfenyo`)**; repos in diff --git a/CONTEXT.md b/CONTEXT.md index 41a9ad9..9aecf71 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -7,7 +7,21 @@ > > Ask Claude Code: "Please update CONTEXT.md with what we did today" -Last updated: 2026-07-20 (v0.149.0 — F3 backup-card fix + remote-site remediation) +Last updated: 2026-07-20 (v0.150.0 — green gate restored + F7 export-link fix) + +> **2026-07-20 — v0.150.0.** **`go test ./...` on DooPlex is fully green again (23/23 packages, run +> twice) — no surviving `t.Skip`s, no weakened assertions, no deleted tests.** The 7 red +> `TestTier2V2_*` / `TestSharesTier2*` were all one environmental class: Tier-2's off-drive guard +> uses `system.SamePhysicalDevice` (st_dev equality), and every `t.TempDir()` here shares one +> filesystem, so the fixture's "two drives" looked identical and the guard correctly refused the +> target (`nincs másik fizikai meghajtó`). Fixed with a nil-defaulted `Manager.samePhysicalDevice` +> seam + `sameDevice` wrapper — **production behaviour is byte-for-byte unchanged** (nil → the real +> check); only the two fixtures inject a fake. All 7 mutation-proved. **F7/R-53 shipped:** +> `app_export.html` built the app URL from `{{$.CSRFToken}}`; now `{{$.Domain}}`, with +> `exportPageHandler` supplying the key (it bypasses `baseData`). Host: the orphaned `dhclient` on +> the non-existent `eth0` was killed and did not respawn. Still open from the arc: R-50 (durable F1, +> spike-first — its ROADMAP note about needing a new cert SAN was **corrected**: the pin is a raw +> leaf-DER SHA-256 with `InsecureSkipVerify`, so SAN never enters it), R-51, R-52, R-39(b)/F6. > **2026-07-20 — remote-site remediation + v0.149.0.** **F1 is MITIGATED FOR THE WINDOW, not durably > fixed:** `vmbr0` on the demo host is now **static `192.168.0.162/24`** (was DHCP; the remote router diff --git a/controller/internal/backup/backup.go b/controller/internal/backup/backup.go index 6e1b611..adb9e59 100644 --- a/controller/internal/backup/backup.go +++ b/controller/internal/backup/backup.go @@ -127,6 +127,13 @@ type Manager struct { // real tier2FitsSystemDrive. tier2SSDFits func(sys string, sizeBytes int64) bool + // samePhysicalDevice — the off-drive identity predicate behind every Tier-2 "is this really a + // SECOND disk?" guard, overridable in tests. The real check is `st_dev` equality, so on a host + // where every `t.TempDir()` lands on one filesystem the fixture's "two drives" are indistinguishable + // and Tier-2 correctly refuses them — which makes the off-drive tests unrunnable rather than wrong. + // Nil → the real system.SamePhysicalDevice (production always takes this path). + samePhysicalDevice func(a, b string) bool + // migrationRunning, if set, reports whether a data migration is in progress. The scheduled // backup paths skip when it returns true (Change 3 — backup ↔ migration mutual exclusion), so a // nightly dump/Tier-2 can't race a migration copy/cleanup on the same drive. @@ -991,6 +998,15 @@ func (m *Manager) GetFullStatus(nextDBDump time.Time) *FullBackupStatus { return status } +// sameDevice reports whether two paths sit on the same physical device, through the test seam when +// one is installed. Nil seam → system.SamePhysicalDevice, i.e. byte-for-byte the previous behaviour. +func (m *Manager) sameDevice(a, b string) bool { + if m.samePhysicalDevice != nil { + return m.samePhysicalDevice(a, b) + } + return system.SamePhysicalDevice(a, b) +} + // hasOffDriveTarget reports whether any registered, schedulable storage path lives on a physical disk // OTHER than the system drive — i.e. whether a genuine off-drive (tier-2) copy is possible at all. // When false the box is single-drive: tier-1 is the ONLY local copy and 3-2-1 needs a 2nd drive or @@ -1000,7 +1016,7 @@ func (m *Manager) hasOffDriveTarget() bool { return false } for _, sp := range m.settings.GetSchedulableStoragePaths() { - if sp.Path == m.systemDataPath || system.SamePhysicalDevice(m.systemDataPath, sp.Path) { + if sp.Path == m.systemDataPath || m.sameDevice(m.systemDataPath, sp.Path) { continue } return true diff --git a/controller/internal/backup/device_seam_test.go b/controller/internal/backup/device_seam_test.go new file mode 100644 index 0000000..d0bc716 --- /dev/null +++ b/controller/internal/backup/device_seam_test.go @@ -0,0 +1,28 @@ +package backup + +import ( + "path/filepath" + "strings" +) + +// oneDrivePerSubtree is the test stand-in for system.SamePhysicalDevice (st_dev equality). +// +// Why it exists: the real predicate asks "are these two paths on the same physical disk?", and +// Tier-2's whole purpose is to refuse a target that is. On a host where every t.TempDir() lands on +// one filesystem — DooPlex, and any CI box with a single volume — a fixture's "usb" and "flash" +// dirs share one st_dev, so the guard correctly refuses them and the off-drive tests can never +// exercise their subject. This models what the fixture is actually depicting: one drive per +// directory subtree, so two paths share a device only when one contains the other (a path inside a +// drive IS on that drive). Unrelated subtrees are distinct devices, exactly as real mountpoints are. +// +// It does NOT relax any assertion — the guard still runs, still refuses same-device targets (see +// TestSharesTier2NeverTargetsItsOwnSourceDrive, which passes under this seam), and production keeps +// using the real st_dev check because the seam is nil there. +func oneDrivePerSubtree(a, b string) bool { + a, b = filepath.Clean(a), filepath.Clean(b) + if a == b { + return true + } + sep := string(filepath.Separator) + return strings.HasPrefix(a, b+sep) || strings.HasPrefix(b, a+sep) +} diff --git a/controller/internal/backup/shares_test.go b/controller/internal/backup/shares_test.go index 85d91c8..01c36e7 100644 --- a/controller/internal/backup/shares_test.go +++ b/controller/internal/backup/shares_test.go @@ -68,6 +68,9 @@ func newSharesEnv(t *testing.T, driveNames ...string) *sharesEnv { return copyTreeForTest(src, dst) }, sharesPassdbCapture: func() ([]byte, error) { return []byte("FAKE-PASSDB-TAR"), nil }, + // Each drive dir is its own "device" — otherwise hdd_1/hdd_2 share one st_dev here and the + // off-drive guard refuses the target, so the mirror under test never runs. + samePhysicalDevice: oneDrivePerSubtree, } return env } diff --git a/controller/internal/backup/tier2.go b/controller/internal/backup/tier2.go index 2646dd3..05d3f3e 100644 --- a/controller/internal/backup/tier2.go +++ b/controller/internal/backup/tier2.go @@ -115,7 +115,7 @@ func (m *Manager) selectTier2TargetFrom(stackName, sourceDrive string, fullSize, sawNetworkCandidate = true break } - if sp.Path == sourceDrive || system.SamePhysicalDevice(sourceDrive, sp.Path) { + if sp.Path == sourceDrive || m.sameDevice(sourceDrive, sp.Path) { break // pinned target is on the same physical disk — not off-drive; fall through } label := sp.Label @@ -134,7 +134,7 @@ func (m *Manager) selectTier2TargetFrom(stackName, sourceDrive string, fullSize, // 1. Prefer another registered user-data drive on a DIFFERENT physical disk, NON-network. if m.settings != nil { for _, sp := range m.settings.GetSchedulableStoragePaths() { - if sp.Path == sourceDrive || system.SamePhysicalDevice(sourceDrive, sp.Path) { + if sp.Path == sourceDrive || m.sameDevice(sourceDrive, sp.Path) { continue } if sp.IsNetwork() { @@ -155,7 +155,7 @@ func (m *Manager) selectTier2TargetFrom(stackName, sourceDrive string, fullSize, // 2. Fall back to the internal SSD (system data path) — STATE-ONLY set only. sys := m.systemDataPath - if sys == "" || system.SamePhysicalDevice(sourceDrive, sys) { + if sys == "" || m.sameDevice(sourceDrive, sys) { if sawNetworkCandidate { return nil, errTier2NetworkOnly // the only off-disk candidate was a NAS } @@ -324,7 +324,7 @@ func (m *Manager) RunTier2(stackName string) error { return nil } // Defense-in-depth off-drive guard (selection already enforced it). - if system.SamePhysicalDevice(sourceDrive, target.NamespaceRoot) { + if m.sameDevice(sourceDrive, target.NamespaceRoot) { m.recordTier2NoTarget(stackName, "a kiválasztott cél ugyanazon a fizikai lemezen van") return nil } @@ -490,7 +490,7 @@ func (m *Manager) Tier2Info(stackName string) Tier2Info { } // Eligible alternative drives: registered, schedulable, on a DIFFERENT physical disk. for _, sp := range m.settings.GetSchedulableStoragePaths() { - if sp.Path == source || system.SamePhysicalDevice(source, sp.Path) { + if sp.Path == source || m.sameDevice(source, sp.Path) { continue } label := sp.Label diff --git a/controller/internal/backup/tier2_shares.go b/controller/internal/backup/tier2_shares.go index c342fef..b84a420 100644 --- a/controller/internal/backup/tier2_shares.go +++ b/controller/internal/backup/tier2_shares.go @@ -9,7 +9,6 @@ import ( "time" "gitea.dooplex.hu/admin/felhom-controller/internal/settings" - "gitea.dooplex.hu/admin/felhom-controller/internal/system" ) // Tier-2 shares job — R-7b Part 2, the local cross-drive leg of Model B′. @@ -178,7 +177,7 @@ func (m *Manager) RunSharesTier2() error { } // Defense-in-depth off-drive guard (selection already enforced it): a leg's target may never // be that leg's own source drive — that would be a same-disk "copy" pretending to be tier 2. - if system.SamePhysicalDevice(g.sourceDrive, target.NamespaceRoot) { + if m.sameDevice(g.sourceDrive, target.NamespaceRoot) { noTargetWhy = append(noTargetWhy, fmt.Sprintf("%s: a kiválasztott cél ugyanazon a fizikai lemezen van", g.sourceDrive)) continue } diff --git a/controller/internal/backup/tier2_v2_test.go b/controller/internal/backup/tier2_v2_test.go index 7520b04..56531fb 100644 --- a/controller/internal/backup/tier2_v2_test.go +++ b/controller/internal/backup/tier2_v2_test.go @@ -61,6 +61,7 @@ func newTier2V2(t *testing.T, stack string) (m *Manager, src, target string, pro m.systemDataPath = sys m.tier2Mirror = copyTree m.tier2SSDFits = func(string, int64) bool { return true } + m.samePhysicalDevice = oneDrivePerSubtree // src/target/sys are separate "drives" on one filesystem return m, src, target, prov } diff --git a/controller/internal/web/app_export_domain_test.go b/controller/internal/web/app_export_domain_test.go new file mode 100644 index 0000000..1c9a524 --- /dev/null +++ b/controller/internal/web/app_export_domain_test.go @@ -0,0 +1,76 @@ +package web + +import ( + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/stacks" +) + +// R-53 / F7: app_export.html built the app's public URL as `.{{$.CSRFToken}}` — the +// session CSRF token substituted where the customer domain belongs. That is two defects in one +// token: the „open in browser" link is wrong for every app that has a subdomain, and a CSRF token +// lands in a URL (history, referrers, logs). The correct CSRF usage on this page is the csrfH() +// helper reading the meta tag, which is untouched. + +const exportTestToken = "deadbeefcafebabedeadbeefcafebabedeadbeefcafebabedeadbeefcafebabe" + +// exportScriptLine returns the `var domain = …` line, so an assertion cannot accidentally match the +// token where it legitimately appears (the meta tag) elsewhere on the page. +func exportScriptLine(t *testing.T, html string) string { + t.Helper() + for _, ln := range strings.Split(html, "\n") { + if strings.Contains(ln, "var domain =") { + return ln + } + } + t.Fatalf("no `var domain =` line in the rendered export page") + return "" +} + +func renderExport(t *testing.T, subdomain string) string { + t.Helper() + return renderBackupPage(t, "app_export", map[string]interface{}{ + "Stack": stacks.Stack{ + Name: "immich", Deployed: true, + Meta: stacks.Metadata{Slug: "immich", DisplayName: "Immich", Subdomain: subdomain}, + }, + "Drives": nil, + "Domain": "demo-felhom.eu", + "CSRFToken": exportTestToken, + "CSRFField": "", + "Page": "apps", + "Title": "Export", + "ExportPage": true, + }) +} + +// Scenario C — the domain is joined from the CUSTOMER DOMAIN, and the CSRF token appears nowhere in +// that line. COMPANION red-proof: restore `{{$.CSRFToken}}` in app_export.html's `var domain` line +// → both assertions FAIL. Run → fail → revert (recorded in REPORT). +func TestAppExportDomainUsesCustomerDomainNotCSRFToken(t *testing.T) { + line := exportScriptLine(t, renderExport(t, "photos")) + + if !strings.Contains(line, "'photos.demo-felhom.eu'") { + t.Errorf("export link must be built from the customer domain, got: %s", line) + } + if strings.Contains(line, exportTestToken) { + t.Errorf("the CSRF token must NEVER appear in the export URL, got: %s", line) + } +} + +// The empty-subdomain branch still yields '' — an app without a subdomain must not get a link to +// a bare domain (the truthiness guard is what produces that, and the fix must not disturb it). +func TestAppExportDomainEmptyWithoutSubdomain(t *testing.T) { + line := exportScriptLine(t, renderExport(t, "")) + + if !strings.Contains(line, "''") { + t.Errorf("no subdomain must yield an empty domain, got: %s", line) + } + if strings.Contains(line, "demo-felhom.eu'") && !strings.Contains(line, "'' ? ") { + t.Errorf("no subdomain must not produce a bare-domain link, got: %s", line) + } + if strings.Contains(line, exportTestToken) { + t.Errorf("the CSRF token must NEVER appear in the export URL, got: %s", line) + } +} diff --git a/controller/internal/web/handler_export.go b/controller/internal/web/handler_export.go index 16b1bb2..2204cb8 100644 --- a/controller/internal/web/handler_export.go +++ b/controller/internal/web/handler_export.go @@ -103,6 +103,10 @@ func (s *Server) exportPageHandler(w http.ResponseWriter, r *http.Request, name data := map[string]interface{}{ "Stack": stack, "Drives": drives, + // R-53: the page builds the app's public URL from the customer domain. This handler does not + // go through baseData (which is where every other page gets "Domain"), so it must supply the + // key itself — the template used to substitute the CSRF token here instead. + "Domain": s.cfg.Customer.Domain, } s.executeTemplate(w, r, "app_export", data) } diff --git a/controller/internal/web/templates/app_export.html b/controller/internal/web/templates/app_export.html index c9100af..c3a1d8d 100644 --- a/controller/internal/web/templates/app_export.html +++ b/controller/internal/web/templates/app_export.html @@ -90,7 +90,7 @@