diff --git a/CHANGELOG.md b/CHANGELOG.md index 16ce4d0..b9ca6f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,41 @@ ## Changelog +### v0.129.0 — CAMPAIGN-4 fixes: rate-limiter key (F-B) + volume-blind estimate (F-A) + no-op claim status (F-C) (2026-07-14) + +Three controller-side fixes from CAMPAIGN-4 (2026-07-13). Controller-only. + +- **F-B (MED, security):** the login/escrow-reauth rate-limiter keyed on `r.RemoteAddr` (which is + `IP:PORT`) whenever `X-Forwarded-For` was absent, so every fresh direct connection from one host + got a distinct ephemeral port → a distinct key → the failed-attempt counter never accrued. A + direct-to-controller path (LAN/guest, bypassing the traefik/CF proxy) therefore had **no + brute-force protection**. Fix: a single shared `clientIP(r)` helper (XFF first-hop, else + `net.SplitHostPort(RemoteAddr)` host, else raw) — replaces the former `requestIP` and the + duplicated inline derivation in `handleLogin`, so the escrow re-auth limiter shares the exact same + fixed key. Accepted limitation (out of scope, commented): XFF is attacker-controlled on a direct + path — the fix closes the port-in-key bug, not XFF trust. Red-proof: revert `clientIP` to raw + `RemoteAddr` → the distinct-ports scenario stops limiting and escrow re-auth stays 401 not 429. +- **F-A (MED, honesty):** the export size-estimate's volume branch `du`'d the raw host mountpoint + from `docker volume inspect`, which is not mounted inside the containerized controller → returned + 0, so a >1 GB volume-only app reported `data_size_bytes:0` / "3.6 KB" / `fits_on_dest:true`. Fix: + a `volumeSizer` seam whose real impl reads the size from a **container view** (`docker run --rm -v + :/vol:ro alpine du -sb /vol` — the same named-volume pattern the export path uses; never a + controller-host path, the v0.125.0 strand class). A failed read now sets `size_unknown` and + **forces `fits_on_dest:false`** (never renders as "fits") with the human string "ismeretlen méret". + The export pre-flight hard-aborts only on a KNOWN doesn't-fit (an unmeasured size no longer blocks + the export — the tar stream + destination FS surface a real ENOSPC). HDD-path branch unchanged. + Red-proof: revert the estimate to the host-path read → the >1 GiB scenario reads 0. +- **F-C (LOW-MED, correctness):** a no-op escrow claim (agent `phase:none` → HTTP 404) fell through + `escrowClaimAPIHandler` to a generic **502**. Fix: relay the agent's 404 as a clean 404 ("Nincs + aktív helyreállítási folyamat…") and 409 as 409; 410 (void) and a genuinely-unreachable agent + (status 0 → real bad gateway) are unchanged. Red-proof: remove the 404 mapping → the no-ceremony + claim returns 502. + +Tests (non-hollow, all red-proofed): `ratelimit_ip_test.go` (F-B: 6 — direct-distinct-ports, +stable-XFF, rotating-XFF, escrow-reauth-shared-key, success-clears, clientIP unit), +`estimate_volsize_test.go` (F-A: 3 — real-not-zero, failure-never-fits, HDD-unchanged), +`TestEscrowClaim_ProxySemantics` +3 subtests (F-C: 404-not-502, 409, unreachable-stays-502). Live: +Alpine busybox `du -sb` verified supported (prod-valid). + ### v0.128.1 — USB drives never show the rotational class hint (2026-07-13, ruling F5) `storage.html` `classTag(d)`: `if(d.type==='usb') return '';` ahead of the class branches — diff --git a/controller/README.md b/controller/README.md index c79b49f..d7ce04a 100644 --- a/controller/README.md +++ b/controller/README.md @@ -1623,7 +1623,10 @@ race where a new `felhom.` cert appears in CT logs minutes before any pa registered address only. Reset rides the same page (a claimed box reaches `/claim` pre-auth). - **Anti-brute-force**: per-source + global counter, 5 failures → 15-minute lockout (both scopes), raising the allowlisted `claim_lockout` event. Pre-auth CSRF is an HMAC over `web.session_secret` - (fixes the CTRL-007 bare-double-submit weakness). + (fixes the CTRL-007 bare-double-submit weakness). The per-source key is the client IP resolved by + the shared `clientIP(r)` helper — XFF first-hop, else `RemoteAddr` with the ephemeral **port + stripped** (v0.129.0 F-B; keying on the raw `RemoteAddr` let distinct direct connections evade the + counter). The login form and the escrow wizard re-auth share the same helper/key. - **Delivery**: the hub bakes `web.claim_code_{hash,generation,issued_at}` into the Day-0 controller.yaml (gate-from-first-boot) and serves the freshest state in the report ACK (`report/claim_sync.go` caches it idempotently by generation — newer advances, same/older/nil diff --git a/controller/internal/appexport/estimate.go b/controller/internal/appexport/estimate.go index 70eea61..ba94008 100644 --- a/controller/internal/appexport/estimate.go +++ b/controller/internal/appexport/estimate.go @@ -1,6 +1,7 @@ package appexport import ( + "bytes" "context" "fmt" "os" @@ -22,6 +23,10 @@ type ExportEstimate struct { DestFreeBytes int64 `json:"dest_free_bytes"` DestFreeHuman string `json:"dest_free_human"` FitsOnDest bool `json:"fits_on_dest"` + // SizeUnknown is set (v0.129.0 F-A) when a volume's size could not be read (docker helper + // failed). When true, DataSizeBytes is a partial/understated sum and FitsOnDest is FORCED false + // — a failed read must NEVER render as "fits". The UI shows "ismeretlen méret". + SizeUnknown bool `json:"size_unknown"` } // EstimateExport calculates size estimates for an app export. @@ -52,12 +57,23 @@ func (e *Exporter) EstimateExport(stackName, destDrive string) (*ExportEstimate, volumes := e.provider.GetDockerVolumes(stackName) e.debugf("EstimateExport: Docker volumes: %v", volumes) for _, vol := range volumes { - volSize := dockerVolumeSize(vol) + volSize, err := volumeSizer(vol) + if err != nil { + // F-A: the controller runs containerized, so a failed helper read must not + // silently become 0-that-reads-as-fits. Mark unknown and keep going. + e.logger.Printf("[WARN] appexport: volume size unknown for %s: %v", vol, err) + est.SizeUnknown = true + continue + } e.debugf("EstimateExport: volume %s = %s", vol, humanizeBytes(volSize)) est.DataSizeBytes += volSize } } - est.DataSizeHuman = humanizeBytes(est.DataSizeBytes) + if est.SizeUnknown { + est.DataSizeHuman = "ismeretlen méret" + } else { + est.DataSizeHuman = humanizeBytes(est.DataSizeBytes) + } est.TotalSizeBytes = est.ConfigSizeBytes + est.DataSizeBytes est.TotalSizeHuman = humanizeBytes(est.TotalSizeBytes) @@ -75,9 +91,10 @@ func (e *Exporter) EstimateExport(stackName, destDrive string) (*ExportEstimate, est.DestFreeBytes = DiskFree(exportDir) est.DestFreeHuman = humanizeBytes(est.DestFreeBytes) - // Need ~10% overhead for tar.gz metadata + compression margin + // Need ~10% overhead for tar.gz metadata + compression margin. F-A: a size we could not read + // must never render as "fits" — an unknown-size estimate is conservatively not-fits. needed := est.TotalSizeBytes + est.TotalSizeBytes/10 - est.FitsOnDest = est.DestFreeBytes >= needed + est.FitsOnDest = !est.SizeUnknown && est.DestFreeBytes >= needed e.debugf("EstimateExport: total=%s free=%s fits=%v needed=%s minutes=%d", est.TotalSizeHuman, est.DestFreeHuman, est.FitsOnDest, humanizeBytes(needed), est.EstimatedMinutes) @@ -118,21 +135,33 @@ func duBytes(path string) int64 { return size } -// dockerVolumeSize estimates the size of a Docker named volume. -func dockerVolumeSize(volumeName string) int64 { - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) +// volumeSizer returns the byte size of a named Docker volume as seen from a CONTAINER view. +// Package var so unit tests inject a fake (returning a known size or an error) without shelling out +// to real docker. F-A (v0.129.0): the old dockerVolumeSize `du`d the host mountpoint from +// `docker volume inspect`, which is NOT visible inside the containerized controller → always 0. +var volumeSizer = realVolumeSize + +// realVolumeSize `du -sb`s the volume mounted read-only into a throwaway helper container — the same +// container-view pattern the export path uses (appexport/export.go withVolumeHelper). It mounts the +// NAMED VOLUME by name (never a controller-host path — the v0.125.0 strand class). Returns an error +// on any failure; callers treat that as "unknown size", never as 0. +func realVolumeSize(volumeName string) (int64, error) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() - // Use docker system df -v and parse, or inspect the volume mount path - out, err := exec.CommandContext(ctx, "docker", "volume", "inspect", - "--format", "{{.Mountpoint}}", volumeName).Output() + var out bytes.Buffer + stderr, err := dockerExec(ctx, nil, &out, "run", "--rm", "-v", volumeName+":/vol:ro", "alpine", "du", "-sb", "/vol") if err != nil { - return 0 + return 0, fmt.Errorf("sizing volume %s: %s: %w", volumeName, stderr, err) } - mountpoint := strings.TrimSpace(string(out)) - if mountpoint == "" { - return 0 + fields := strings.Fields(out.String()) + if len(fields) == 0 { + return 0, fmt.Errorf("sizing volume %s: empty du output", volumeName) } - return duBytes(mountpoint) + var size int64 + if _, err := fmt.Sscanf(fields[0], "%d", &size); err != nil { + return 0, fmt.Errorf("sizing volume %s: parse %q: %w", volumeName, fields[0], err) + } + return size, nil } // DiskFree returns available bytes on the filesystem containing path (0 on any error). diff --git a/controller/internal/appexport/estimate_volsize_test.go b/controller/internal/appexport/estimate_volsize_test.go new file mode 100644 index 0000000..091f9a2 --- /dev/null +++ b/controller/internal/appexport/estimate_volsize_test.go @@ -0,0 +1,97 @@ +package appexport + +import ( + "errors" + "io" + "log" + "strings" + "testing" +) + +// hddProvider is an rtProvider that reports an HDD-backed stack (for the regression scenario H). +type hddProvider struct { + *rtProvider + mounts []string +} + +func (p *hddProvider) GetStackNeedsHDD(string) bool { return true } +func (p *hddProvider) GetStackHDDMounts(string) []string { return p.mounts } + +func newEstimator(t *testing.T, provider ExportStackProvider) *Exporter { + t.Helper() + return NewExporter(provider, log.New(io.Discard, "", 0), "test") +} + +// Scenario F (the F-A fix): a volume-only app with a >1 GiB volume reports the REAL size via the +// container-view sizer — not 0/"3.6 KB". This is the F-A red-proof anchor (revert EstimateExport to +// dockerVolumeSize → reads 0). +func TestEstimate_VolumeSize_RealNotZero(t *testing.T) { + const twoGiB = int64(2) << 30 + orig := volumeSizer + volumeSizer = func(vol string) (int64, error) { return twoGiB, nil } + defer func() { volumeSizer = orig }() + + e := newEstimator(t, &rtProvider{stackDir: t.TempDir(), volumes: []string{"app_data"}}) + est, err := e.EstimateExport("app", t.TempDir()) + if err != nil { + t.Fatal(err) + } + if est.SizeUnknown { + t.Fatalf("size must be known when the sizer succeeds") + } + if est.DataSizeBytes != twoGiB { + t.Fatalf("DataSizeBytes = %d, want %d (WRONG would be 0 — the F-A bug)", est.DataSizeBytes, twoGiB) + } + if !strings.Contains(est.DataSizeHuman, "GB") { + t.Fatalf("DataSizeHuman = %q, want GB-scale (WRONG would be \"3.6 KB\")", est.DataSizeHuman) + } +} + +// Scenario G: a failed volume read must never render as "fits". Size is marked unknown, the human +// string says so, and FitsOnDest is forced false. +func TestEstimate_VolumeSize_FailureNeverFits(t *testing.T) { + orig := volumeSizer + volumeSizer = func(vol string) (int64, error) { return 0, errors.New("docker: no such image") } + defer func() { volumeSizer = orig }() + + e := newEstimator(t, &rtProvider{stackDir: t.TempDir(), volumes: []string{"app_data"}}) + est, err := e.EstimateExport("app", t.TempDir()) + if err != nil { + t.Fatal(err) + } + if !est.SizeUnknown { + t.Fatalf("a failed volume read must set SizeUnknown") + } + if est.FitsOnDest { + t.Fatalf("an unknown size must NEVER render as fits_on_dest:true") + } + if est.DataSizeHuman != "ismeretlen méret" { + t.Fatalf("DataSizeHuman = %q, want \"ismeretlen méret\"", est.DataSizeHuman) + } + if est.DataSizeBytes != 0 { + t.Fatalf("no successful read → DataSizeBytes should be 0, got %d", est.DataSizeBytes) + } +} + +// Scenario H (regression): an HDD-backed stack must NOT touch the new volume sizer — the HDD branch +// (duBytes on the mounted /mnt path) is unchanged. Platform-independent: assert the seam is not +// invoked and SizeUnknown stays false. +func TestEstimate_HDDPath_DoesNotUseVolumeSizer(t *testing.T) { + called := false + orig := volumeSizer + volumeSizer = func(vol string) (int64, error) { called = true; return 0, nil } + defer func() { volumeSizer = orig }() + + p := &hddProvider{rtProvider: &rtProvider{stackDir: t.TempDir()}, mounts: []string{t.TempDir()}} + e := newEstimator(t, p) + est, err := e.EstimateExport("app", t.TempDir()) + if err != nil { + t.Fatal(err) + } + if called { + t.Fatalf("HDD-backed stack must not call the docker volume sizer") + } + if est.SizeUnknown { + t.Fatalf("HDD branch must not set SizeUnknown") + } +} diff --git a/controller/internal/appexport/export.go b/controller/internal/appexport/export.go index a378069..0484c88 100644 --- a/controller/internal/appexport/export.go +++ b/controller/internal/appexport/export.go @@ -197,9 +197,14 @@ func (e *Exporter) executeExport(req ExportRequest, job *Job) { if err != nil { e.debugf("estimate error (non-fatal): %v", err) } else { - e.debugf("estimate: config=%s data=%s total=%s destFree=%s fits=%v", - est.ConfigSizeHuman, est.DataSizeHuman, est.TotalSizeHuman, est.DestFreeHuman, est.FitsOnDest) - if !est.FitsOnDest { + e.debugf("estimate: config=%s data=%s total=%s destFree=%s fits=%v unknown=%v", + est.ConfigSizeHuman, est.DataSizeHuman, est.TotalSizeHuman, est.DestFreeHuman, est.FitsOnDest, est.SizeUnknown) + // Hard-abort only on a KNOWN doesn't-fit. F-A: est.SizeUnknown forces FitsOnDest=false for + // the UI honesty signal, but an unmeasured size must NOT block the export here — the tar + // streaming and the destination filesystem surface a real ENOSPC if it genuinely won't fit. + if est.SizeUnknown { + e.logger.Printf("[WARN] appexport: export space pre-check skipped for %s — volume size unknown", req.StackName) + } else if !est.FitsOnDest { e.failJob(job, step, fmt.Sprintf("Nincs elég hely: szükséges ~%s, szabad %s", est.TotalSizeHuman, est.DestFreeHuman)) return diff --git a/controller/internal/web/auth.go b/controller/internal/web/auth.go index 228dc83..e046ae1 100644 --- a/controller/internal/web/auth.go +++ b/controller/internal/web/auth.go @@ -148,12 +148,10 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) { return } - // Rate limit: check failed attempts from this IP - ip := r.RemoteAddr - if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" { - ip = strings.Split(fwd, ",")[0] - } - ip = strings.TrimSpace(ip) + // Rate limit: check failed attempts from this host. clientIP strips the ephemeral port + // (CAMPAIGN-4 F-B) so distinct direct connections from one host share a key and the counter + // actually accrues; XFF first-hop still wins for proxied clients. + ip := clientIP(r) s.loginAttemptMu.Lock() attempt := s.loginAttempts[ip] diff --git a/controller/internal/web/claim.go b/controller/internal/web/claim.go index 9512d33..032fe08 100644 --- a/controller/internal/web/claim.go +++ b/controller/internal/web/claim.go @@ -9,6 +9,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "os" "strings" @@ -193,12 +194,26 @@ func (s *Server) claimNow() time.Time { return time.Now() } -func requestIP(r *http.Request) string { - ip := r.RemoteAddr +// clientIP returns the client IP used as the rate-limiter key. Order: the X-Forwarded-For first +// hop (set by the traefik/Cloudflare proxy) wins; otherwise the HOST portion of RemoteAddr with the +// ephemeral PORT stripped (net.SplitHostPort). This is the CAMPAIGN-4 F-B fix: keying on the raw +// RemoteAddr (IP:PORT) meant every fresh direct connection from one host got a distinct ephemeral +// port → a distinct key → the failed-attempt counter never accrued, so a direct-to-controller +// (LAN/guest, non-proxied) path had NO brute-force protection. A RemoteAddr with no port +// (tests/edge) or an IPv6 form is handled by SplitHostPort, falling back to the raw value. +// +// Accepted limitation (out of scope here): X-Forwarded-For is attacker-controlled on a direct path, +// so a client rotating the first hop still evades the per-IP counter. This fix only closes the +// port-in-key bug so the proxied / stable-source-IP case — the real deployment — works; it does NOT +// attempt to establish XFF trust. +func clientIP(r *http.Request) string { if fwd := r.Header.Get("X-Forwarded-For"); fwd != "" { - ip = strings.Split(fwd, ",")[0] + return strings.TrimSpace(strings.Split(fwd, ",")[0]) } - return strings.TrimSpace(ip) + if host, _, err := net.SplitHostPort(r.RemoteAddr); err == nil { + return host + } + return strings.TrimSpace(r.RemoteAddr) } // ── the pages ──────────────────────────────────────────────────────────────────────────────── @@ -260,7 +275,7 @@ func (s *Server) handleClaimSubmit(w http.ResponseWriter, r *http.Request) { return } wasReset := s.authEnabled() // a password already set → this is a reset, not a first-claim - ip := requestIP(r) + ip := clientIP(r) if locked, _ := s.claimRateLocked(); locked { s.handleClaimPage(w, r, "Túl sok próbálkozás — próbáld újra 15 perc múlva.", "") diff --git a/controller/internal/web/escrow_handlers.go b/controller/internal/web/escrow_handlers.go index e6a0bc4..6fe4106 100644 --- a/controller/internal/web/escrow_handlers.go +++ b/controller/internal/web/escrow_handlers.go @@ -173,7 +173,7 @@ func (s *Server) escrowStartAPIHandler(w http.ResponseWriter, r *http.Request) { escrowJSON(w, http.StatusForbidden, nil, "A vezérlőpult jelszava nincs beállítva — előbb állítson be jelszót.") return } - ip := requestIP(r) + ip := clientIP(r) if s.escrowRateLimited(ip) { s.logger.Printf("[WARN] [web] escrow start rate limited for %s", ip) escrowJSON(w, http.StatusTooManyRequests, nil, "Túl sok sikertelen próbálkozás, próbálja újra 1 perc múlva.") @@ -254,12 +254,20 @@ func (s *Server) escrowClaimAPIHandler(w http.ResponseWriter, r *http.Request) { } code, status, err := agent.EscrowCeremonyClaim(r.Context()) if err != nil { - if status == http.StatusGone { + switch status { + case http.StatusGone: // 410 — the code was minted but never shown; permanently void. escrowJSON(w, http.StatusGone, nil, "A kód létrejött, de nem lett megjelenítve — biztonsági okból újra nem kérhető le. Indítsa újra a folyamatot: az új kód a régit érvényteleníti.") - return + case http.StatusNotFound: // 404 — no ceremony has run (e.g. phase:none post-reboot). F-C: + // this used to fall through to a 502; a bad-gateway class code for "nothing to claim" + // is wrong. Relay a clean, honest 4xx. + escrowJSON(w, http.StatusNotFound, nil, "Nincs aktív helyreállítási folyamat — előbb indítsa el a kódkészítést.") + case http.StatusConflict: // 409 — the ceremony state doesn't allow a claim right now. F-C. + escrowJSON(w, http.StatusConflict, nil, "A folyamat jelenlegi állapotában a kód nem kérhető le.") + default: // status 0 (agent unreachable / transport error) or a genuine agent 5xx — a real + // bad gateway; keep 502. + s.logger.Printf("[WARN] [web] escrow claim failed (status %d)", status) // reason text may echo agent detail; the code itself is never in errors + escrowJSON(w, http.StatusBadGateway, nil, "A kód lekérése nem sikerült.") } - s.logger.Printf("[WARN] [web] escrow claim failed (status %d)", status) // reason text may echo agent detail; the code itself is never in errors - escrowJSON(w, http.StatusBadGateway, nil, "A kód lekérése nem sikerült.") return } s.logger.Printf("[INFO] [web] escrow recovery code claimed (one-shot; not logged)") diff --git a/controller/internal/web/escrow_wizard_test.go b/controller/internal/web/escrow_wizard_test.go index 42facd2..2e755d8 100644 --- a/controller/internal/web/escrow_wizard_test.go +++ b/controller/internal/web/escrow_wizard_test.go @@ -286,6 +286,43 @@ func TestEscrowClaim_ProxySemantics(t *testing.T) { t.Fatalf("gone: got %d %s", w.Code, w.Body.String()) } }) + // Scenario I (F-C fix): agent 404 "no ceremony has run" → a clean 404, NOT 502. + t.Run("no_active_ceremony_404_not_502", func(t *testing.T) { + h := newEscrowWizardHarness(t) + h.agent.claimStatus = http.StatusNotFound + h.agent.claimErr = fmt.Errorf("no ceremony has run") + w := httptest.NewRecorder() + h.s.escrowClaimAPIHandler(w, httptest.NewRequest("POST", "/api/escrow/claim", nil)) + if w.Code != http.StatusNotFound { + t.Fatalf("no-ceremony claim must be 404, got %d %s", w.Code, w.Body.String()) + } + if !strings.Contains(w.Body.String(), "Nincs aktív helyreállítási folyamat") { + t.Fatalf("expected honest Hungarian no-ceremony message, got %s", w.Body.String()) + } + }) + // Scenario J (regression): agent 409 → passed through as 409, not 502. + t.Run("conflict_409", func(t *testing.T) { + h := newEscrowWizardHarness(t) + h.agent.claimStatus = http.StatusConflict + h.agent.claimErr = fmt.Errorf("conflict") + w := httptest.NewRecorder() + h.s.escrowClaimAPIHandler(w, httptest.NewRequest("POST", "/api/escrow/claim", nil)) + if w.Code != http.StatusConflict { + t.Fatalf("409 must pass through, got %d", w.Code) + } + }) + // Scenario K (regression): a genuinely-unreachable agent (status 0) stays 502 — that IS a real + // bad gateway; the fix must NOT over-correct it to a 4xx. + t.Run("unreachable_stays_502", func(t *testing.T) { + h := newEscrowWizardHarness(t) + h.agent.claimStatus = 0 + h.agent.claimErr = fmt.Errorf("dial tcp: connection refused") + w := httptest.NewRecorder() + h.s.escrowClaimAPIHandler(w, httptest.NewRequest("POST", "/api/escrow/claim", nil)) + if w.Code != http.StatusBadGateway { + t.Fatalf("unreachable agent must stay 502, got %d", w.Code) + } + }) } // The status proxy relays the agent's non-secret job view verbatim. diff --git a/controller/internal/web/ratelimit_ip_test.go b/controller/internal/web/ratelimit_ip_test.go new file mode 100644 index 0000000..8b6c1f6 --- /dev/null +++ b/controller/internal/web/ratelimit_ip_test.go @@ -0,0 +1,151 @@ +package web + +import ( + "fmt" + "io" + "log" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-controller/internal/config" + "golang.org/x/crypto/bcrypt" +) + +// rateLimitTestServer builds a minimal Server with a real password hash and an initialized +// loginAttempts map, templates loaded (renderLogin needs s.tmpl). No agent/backup/settings — the +// re-auth rate limiter fires before any of those. +func rateLimitTestServer(t *testing.T) *Server { + t.Helper() + cfg := &config.Config{} + hash, _ := bcrypt.GenerateFromPassword([]byte("correct-pass"), bcrypt.MinCost) + cfg.Web.PasswordHash = string(hash) + s := &Server{cfg: cfg, logger: log.New(io.Discard, "", 0), version: "test", + loginAttempts: map[string]*loginAttempt{}, sessions: map[string]*session{}} + s.loadTemplates() + return s +} + +func doLogin(s *Server, remoteAddr, xff, password string) *httptest.ResponseRecorder { + form := url.Values{"password": {password}} + r := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode())) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + r.RemoteAddr = remoteAddr + if xff != "" { + r.Header.Set("X-Forwarded-For", xff) + } + w := httptest.NewRecorder() + s.handleLogin(w, r) + return w +} + +func ex(s string) string { + if len(s) > 200 { + return s[:200] + } + return s +} + +// Scenario A (the F-B fix): 6 failed logins from ONE host over DISTINCT ephemeral ports and NO +// X-Forwarded-For must share a rate-limit key and trip the limiter on attempt 6. Pre-fix (raw +// RemoteAddr key) each port is its own key → never limited. This is the F-B red-proof anchor. +func TestLoginRateLimit_DirectDistinctPorts_Limited(t *testing.T) { + s := rateLimitTestServer(t) + var last string + for i := 1; i <= 6; i++ { + last = doLogin(s, fmt.Sprintf("127.0.0.1:%d", 5000+i), "", "wrong").Body.String() + if i < 6 && !strings.Contains(last, "Hibás jelszó") { + t.Fatalf("attempt %d expected Hibás jelszó, got: %s", i, ex(last)) + } + } + if !strings.Contains(last, "Túl sok sikertelen") { + t.Fatalf("attempt 6 (distinct ports, no XFF) MUST be rate-limited; got: %s", ex(last)) + } +} + +// Scenario B (regression): stable X-Forwarded-For across 6 attempts still limits on 6. +func TestLoginRateLimit_StableXFF_Limited(t *testing.T) { + s := rateLimitTestServer(t) + var last string + for i := 1; i <= 6; i++ { + last = doLogin(s, fmt.Sprintf("10.9.9.9:%d", 5000+i), "203.0.113.9", "wrong").Body.String() + } + if !strings.Contains(last, "Túl sok sikertelen") { + t.Fatalf("attempt 6 with a stable XFF MUST be rate-limited; got: %s", ex(last)) + } +} + +// Scenario C (documented accepted limitation): rotating the X-Forwarded-For first hop evades the +// per-IP counter. This is NOT what the fix targets (XFF is attacker-controlled on a direct path); +// the test pins the known behavior so a future XFF-trust change is a conscious decision. +func TestLoginRateLimit_RotatingXFF_NotLimited(t *testing.T) { + s := rateLimitTestServer(t) + var last string + for i := 1; i <= 6; i++ { + last = doLogin(s, "10.9.9.9:5000", fmt.Sprintf("203.0.113.%d", i), "wrong").Body.String() + } + if strings.Contains(last, "Túl sok sikertelen") { + t.Fatalf("rotating XFF is a known evasion (out of scope) — expected NOT limited") + } + if !strings.Contains(last, "Hibás jelszó") { + t.Fatalf("expected Hibás jelszó on rotating XFF; got: %s", ex(last)) + } +} + +// Scenario D: the escrow wizard re-auth path shares the SAME fixed key (proves the shared clientIP +// helper reached escrow_handlers, not just handleLogin). 6 wrong wizard passwords over distinct +// ports → 429 on attempt 6. +func TestEscrowReauthRateLimit_SharesFixedKey(t *testing.T) { + s := rateLimitTestServer(t) + var code int + for i := 1; i <= 6; i++ { + form := url.Values{"password": {"wrong"}} + r := httptest.NewRequest(http.MethodPost, "/api/escrow/start", strings.NewReader(form.Encode())) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + r.RemoteAddr = fmt.Sprintf("127.0.0.1:%d", 6000+i) + w := httptest.NewRecorder() + s.escrowStartAPIHandler(w, r) + code = w.Code + } + if code != http.StatusTooManyRequests { + t.Fatalf("escrow re-auth attempt 6 (distinct ports, no XFF) MUST be 429; got %d", code) + } +} + +// Scenario E: a successful login clears the counter for that host key. +func TestLoginRateLimit_SuccessClearsCounter(t *testing.T) { + s := rateLimitTestServer(t) + for i := 1; i <= 4; i++ { + doLogin(s, fmt.Sprintf("127.0.0.1:%d", 7000+i), "", "wrong") + } + if s.loginAttempts["127.0.0.1"] == nil || s.loginAttempts["127.0.0.1"].count != 4 { + t.Fatalf("expected 4 accrued failures on the shared key before success") + } + doLogin(s, "127.0.0.1:7099", "", "correct-pass") // success + if s.loginAttempts["127.0.0.1"] != nil { + t.Fatalf("a successful login must clear the host's failure counter") + } +} + +// clientIP unit: port stripped; XFF first-hop wins; no-port and IPv6 handled. +func TestClientIP_StripsPort(t *testing.T) { + cases := []struct{ remote, xff, want string }{ + {"127.0.0.1:5001", "", "127.0.0.1"}, + {"127.0.0.1:5002", "203.0.113.9", "203.0.113.9"}, + {"[::1]:443", "", "::1"}, + {"192.168.0.5", "", "192.168.0.5"}, // no port → raw + {"10.0.0.1:80", "198.51.100.7, 203.0.113.9", "198.51.100.7"}, + } + for _, c := range cases { + r := httptest.NewRequest(http.MethodGet, "/", nil) + r.RemoteAddr = c.remote + if c.xff != "" { + r.Header.Set("X-Forwarded-For", c.xff) + } + if got := clientIP(r); got != c.want { + t.Errorf("clientIP(remote=%q xff=%q) = %q, want %q", c.remote, c.xff, got, c.want) + } + } +}