From d62874d0eadc1a766212685b9f6bc3e54655569e Mon Sep 17 00:00:00 2001 From: Nguyen Van Long Date: Tue, 6 Oct 2026 12:20:55 +0700 Subject: [PATCH] fix(api): drop dead auth knobs, require https sign-out URLs, generic decode errors (FIX-AUTH-P3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AuthConfig.IdleTimeout and AuthConfig.AllowedTenants were dead configuration surface: IdleTimeout was defaulted but never consumed (the session idle window belongs to the session store, configured by -session-idle/TCDI_SESSION_IDLE) and AllowedTenants was enforced by tenantAllowed but had no flag/env/chart path able to populate it. Both are deleted rather than wired — wiring IdleTimeout would have created a second source of truth for one window, and the supported login gate remains -required-groups / oidc.requiredGroups. - PostLogoutRedirect and the discovered end_session_endpoint now require an absolute https URL; http is accepted only for loopback hosts (localhost, 127.0.0.0/8, ::1 — strict netip parse), matching the codebase's plain-http-on-loopback dev/test IdP convention. - POST /v1/workspaces/{id}/connections no longer echoes the JSON decoder error to the client: it decodes via the shared decodeJSON helper (now returning the error for server-side logging, and rejecting trailing data the inline decode tolerated), answers the generic "invalid request body", and logs the detail with the request id. Generated with [Devin](https://devin.ai) Co-Authored-By: Devin <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- docs/runbooks/install.md | 3 +- docs/security/threat-model.md | 13 +++++-- internal/api/adminquota.go | 2 +- internal/api/adminuserlimit.go | 2 +- internal/api/auth.go | 66 ++++++++++++++++---------------- internal/api/auth_test.go | 17 +++++--- internal/api/connections.go | 22 +++++++++-- internal/api/connections_test.go | 57 ++++++++++++++++++++++++++- internal/api/data.go | 4 +- internal/api/logout_test.go | 66 +++++++++++++++++++++++++++++--- internal/api/workspaces.go | 21 ++++++---- internal/backend/wire.go | 3 +- 12 files changed, 212 insertions(+), 64 deletions(-) diff --git a/docs/runbooks/install.md b/docs/runbooks/install.md index 499dcd9d..9c165030 100644 --- a/docs/runbooks/install.md +++ b/docs/runbooks/install.md @@ -390,7 +390,8 @@ back in. So, by default, sign-out continues at the provider - The target comes only from the discovery document and the chart values. Nothing in the sign-out request (query, body, headers, `Host`) can change it, so it is not an open redirect. A discovered endpoint that is not an - absolute `http(s)` URL is ignored and sign-out stays local. + absolute `https` URL — `http` is accepted only for loopback dev hosts — + is ignored and sign-out stays local. ```yaml oidc: diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 0763c69b..b7fdcf8f 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -146,8 +146,9 @@ Controls: verifier are bound to the initiating browser (SEC-03). - **Portal session** — server-side, opaque cookie value; rows keyed by SHA-256 digest of the session ID so a DB/backup read never yields a usable - cookie (`internal/store/sessions.go:23,63,102`). Sliding 30 min idle, - 12 h absolute cap (`auth.go:133-137` config defaults); the backend extends + cookie (`internal/store/sessions.go:23,63,102`). Sliding 30 min idle + (`-session-idle`/`TCDI_SESSION_IDLE` → `store.NewSessionStore`), 12 h + absolute cap (`AuthConfig.AbsoluteTimeout`); the backend extends idle on server-measured input activity. The retained `id_token` is sealed at rest with the same keys under a purpose-bound AAD (`internal/api/idtoken.go`); on read failure logout degrades to a @@ -233,8 +234,9 @@ PKCE means a secretless public client also works), RP-initiated logout. Controls: exact issuer/audience match via go-oidc; `endSessionURL` is built only from discovery + static configuration so request input cannot produce an open redirect (`internal/api/auth.go:770-787`); -`PostLogoutRedirect` is validated as an absolute http(s) URL at startup -(auth.go:359). Client secret comes from `oidc.existingSecret` (chart). +`PostLogoutRedirect` is validated as an absolute https URL at startup +(auth.go) — http is accepted only for loopback dev hosts. Client secret +comes from `oidc.existingSecret` (chart). Tests: `auth_test.go`, `logout_test.go`, `oidctest/` in-memory IdP. Open question: A6-S2 — replay of the sealed login cookie within its TTL @@ -523,6 +525,9 @@ items that landed since. | S22 | Deploy-time guards for hazardous value combinations | Implemented (#119): `tinycdi.validate` now fails the render on `operator.leaderElect=false` with `operator.replicas>1` — unelected replicas would double-drive reconciliation and the binary cannot observe the Deployment's replica count (`templates/_helpers.tpl`, `cmd/operator` note; `TestOperatorLeaderElectionGuard`). Metrics-port isolation is pinned for every `edgeIngress` mode: only `allow-metrics-scrape` opens :9090, to exactly `networkPolicy.prometheusPeers` (`TestMetricsListenerIsolation`, `TestEdgePolicyNeverOpensInternalPort`). Residual: single-replica installs (`replicas: 1`) may still run unelected, and guards only constrain chart-rendered manifests — hand-rolled manifests bypass them | | S23 | Postgres-only restore onto a live cluster (DR) | Implemented (#122 runbook + #124 tool): `docs/runbooks/disaster-recovery.md` §"Postgres-only restore onto a live cluster" enumerates what an older dump resurrects — `sessions` rows including post-dump sign-outs (killed by `platform_meta.session_epoch` rotation — `SessionStore.Get` compares epoch in the same read), `'active'` `connection_lease` rows including post-dump revokes (a non-NULL `portal_session_digest` dies at next renew/attach via the S17 check, but the tool revokes every restored lease unconditionally anyway — NULL-digest rows have no barrier), unconsumed `launch_ticket` rows re-arming (denied; redeem also re-checks the session), and `workspaces` rows trailing live CRs (aligned to `spec.desiredState`/`runtimeGeneration`/`intentRevision` so the applied-intent fence is not bypassed). `backend post-restore` is the one-shot implementation: dry-run by default; `-apply` requires `-i-have-scaled-down`, takes the leader advisory lock on a dedicated connection and holds it for the whole run (a serving replica or concurrent run means refusal with zero writes), refuses while backend connections remain in `pg_stat_activity` unless `-i-know-backends-are-running`, and exits non-zero printing per-workspace SQL when Kubernetes read access for CR alignment is absent (`internal/backend/postrestore.go`; tests `postrestore_test.go`, `TestSessionEpochRotation`). The `hack/quickstart/restore-drill.sh` kind drill is manual, not a per-PR gate | | S24 | Intent-stream drift vs the applied-intent fence | Implemented (#125): after a DB restore the platform's `workspaces.intent_revision` can trail the fence the CR already recorded, and every new intent would be dropped as stale — silently. The applier now detects the two unreachable-by-replay shapes (strictly-behind revision; equal revision with diverged `desiredState`/`runtimeGeneration`) and stamps `workspaces.cdi.tinyorbit.vn/intent-behind` `{rowRevision, crRevision}` (`intentFenceDrifted`, `internal/provisioning/k8sapplier.go`); the operator raises `ConditionIntentBehind` with the revision params plus one edge-triggered Warning event, and clears it once the stream applies forward again (`internal/operator/workspace_controller.go`, `api/v1alpha1/validation.go`; events `create`/`patch` RBAC added). The fence itself is unchanged — a stale intent is still never applied; drift is only surfaced. Surfaced to users via the conditions/events projection (`statusview.go`, `events.go`; `TestIntentBehindDrift`, `TestK8sApplierIntentDrift`, `TestProjectConditions_IntentBehind`) | +| S25 | Dead/misleading auth config knobs | Fixed: `AuthConfig` carried `IdleTimeout` — defaulted at startup but never consumed (the session idle window is owned by the session store: `-session-idle`/`TCDI_SESSION_IDLE` → `store.NewSessionStore`) — and `AllowedTenants` — enforced in `tenantAllowed` but with no flag/env/chart path able to populate it, so the gate could never engage; `-required-groups` (`oidc.requiredGroups`) remains the supported login gate. Both knobs were deleted rather than wired: wiring `IdleTimeout` would have created a second source of truth for the store's window, and `AllowedTenants` had no reachable configuration. Guard: `TestAuthConfig_NoDeadSessionKnobs` fails if either field returns | +| S26 | Post-logout redirect accepted plaintext `http` | Fixed: `AuthConfig.PostLogoutRedirect` and the discovered `end_session_endpoint` now require an absolute https URL — `http` is accepted only for loopback hosts (`localhost`, 127.0.0.0/8, `::1`; strict `netip` parse — mapped/octal spellings do not count), matching the dev plain-http-on-loopback IdP convention (`oidctest`); the chart schema already required `^https://` (`values.schema.json`). Tests: `TestNewAuthenticator_RejectsBadPostLogoutRedirect`, `TestNewAuthenticator_AllowsLoopbackPostLogoutRedirect`, `TestLogout_UntrustedDiscoveredEndpointIs204` | +| S27 | create-connection echoed JSON decoder detail | Fixed: `ConnectionHandler.Create` decodes via the shared `decodeJSON` helper — which also rejects trailing data after the first document, a check the previous inline decode lacked — answers 400 `INVALID_REQUEST` with the generic "invalid request body", and logs the decode detail server-side with the request id (SEC-I7). `decodeJSON` now returns the error so callers can log it without echoing it. Tests: `TestCreateConnection_InvalidBodyGenericMessage`, `TestCreateConnection_TrailingJSONRejected` | *Review note — S17:* the ticket-lock serialization claim (a redeem's ticket-row `FOR UPDATE` vs the revoke's `UPDATE` under READ COMMITTED) is diff --git a/internal/api/adminquota.go b/internal/api/adminquota.go index b963c884..13fad2c3 100644 --- a/internal/api/adminquota.go +++ b/internal/api/adminquota.go @@ -345,7 +345,7 @@ func (h *AdminQuotaHandler) Put(w http.ResponseWriter, r *http.Request) { return } var req adminQuotaLimits - if !decodeJSON(body, &req) { + if decodeJSON(body, &req) != nil { writeError(w, r, CodeInvalidRequest, "invalid request body") return } diff --git a/internal/api/adminuserlimit.go b/internal/api/adminuserlimit.go index e9447fb5..325f7609 100644 --- a/internal/api/adminuserlimit.go +++ b/internal/api/adminuserlimit.go @@ -227,7 +227,7 @@ func (h *AdminUserLimitsHandler) decodeBody(w http.ResponseWriter, r *http.Reque writeError(w, r, CodeInvalidRequest, "unreadable or oversized body") return false } - if !decodeJSON(body, v) { + if decodeJSON(body, v) != nil { writeError(w, r, CodeInvalidRequest, "invalid request body") return false } diff --git a/internal/api/auth.go b/internal/api/auth.go index 1d947208..df8488f7 100644 --- a/internal/api/auth.go +++ b/internal/api/auth.go @@ -14,6 +14,7 @@ import ( "fmt" "log/slog" "net/http" + "net/netip" "net/url" "strconv" "strings" @@ -67,10 +68,11 @@ type AuthConfig struct { // set it false for httptest servers, release builds must not. SecureCookies *bool - // IdleTimeout is the sliding inactivity lifetime of a session - // (default 30m). AbsoluteTimeout is the hard cap from creation - // (default 12h). - IdleTimeout time.Duration + // AbsoluteTimeout is the hard cap on a session's lifetime from + // creation (default 12h). The sliding idle timeout is not configured + // here: it is owned by the session store (-session-idle / + // TCDI_SESSION_IDLE -> store.NewSessionStore), which enforces it on + // every read. AbsoluteTimeout time.Duration // PendingTTL bounds how long a login attempt (state/nonce/PKCE // verifier) stays valid (default 10m). It is sealed into the login @@ -86,9 +88,6 @@ type AuthConfig struct { // membership and group membership. Defaults: "tenant_id", "groups". TenantClaim string GroupsClaim string - // AllowedTenants, when non-empty, restricts login to principals whose - // TenantID is listed. TenantID empty is always rejected. - AllowedTenants []string // RequiredGroups, when non-empty, additionally restricts login to // principals whose Groups claim contains at least one listed group. // The match is exact — the Keycloak full-path form "/platform-admins" @@ -108,7 +107,7 @@ type AuthConfig struct { // end-session URL (chart oidc.postLogoutRedirect). Empty omits it: the // URI must be registered at the provider, so it is opt-in and the // provider then shows its own logged-out page. Must be an absolute - // http(s) URL. + // https URL (http only for loopback dev hosts). PostLogoutRedirect string } @@ -129,9 +128,6 @@ func (c *AuthConfig) withDefaults() { t := true c.SecureCookies = &t } - if c.IdleTimeout == 0 { - c.IdleTimeout = 30 * time.Minute - } if c.AbsoluteTimeout == 0 { c.AbsoluteTimeout = 12 * time.Hour } @@ -359,8 +355,8 @@ func NewAuthenticator(ctx context.Context, cfg AuthConfig, sessions SessionStore if log == nil { log = slog.Default() } - if cfg.PostLogoutRedirect != "" && !isAbsoluteHTTPURL(cfg.PostLogoutRedirect) { - return nil, errors.New("api: AuthConfig PostLogoutRedirect must be an absolute http(s) URL") + if cfg.PostLogoutRedirect != "" && !isHTTPSOrLoopbackURL(cfg.PostLogoutRedirect) { + return nil, errors.New("api: AuthConfig PostLogoutRedirect must be an absolute https URL (http only for loopback hosts)") } provider, err := oidc.NewProvider(ctx, cfg.Issuer) if err != nil { @@ -374,10 +370,10 @@ func NewAuthenticator(ctx context.Context, cfg AuthConfig, sessions SessionStore if err := provider.Claims(&disc); err != nil { return nil, fmt.Errorf("api: OIDC discovery claims: %w", err) } - if isAbsoluteHTTPURL(disc.EndSessionEndpoint) { + if isHTTPSOrLoopbackURL(disc.EndSessionEndpoint) { endSession = disc.EndSessionEndpoint } else if disc.EndSessionEndpoint != "" { - log.Warn("oidc end_session_endpoint ignored: not an absolute http(s) URL without credentials") + log.Warn("oidc end_session_endpoint ignored: not an absolute https URL (http only for loopback hosts) without credentials") } } a := &Authenticator{ @@ -576,7 +572,7 @@ func (a *Authenticator) CallbackHandler(w http.ResponseWriter, r *http.Request) DisplayName: displayNameClaim(claims), Email: displayClaim(stringClaim(claims, "email"), maxEmailLen), } - if principal.TenantID == "" || !a.tenantAllowed(principal.TenantID) { + if principal.TenantID == "" { writeError(w, r, CodeForbidden, "no tenant membership for this account") return } @@ -793,14 +789,32 @@ func (a *Authenticator) endSessionURL(idToken string) string { return u.String() } -// isAbsoluteHTTPURL reports whether s is an absolute http(s) URL with a host -// and no embedded credentials. -func isAbsoluteHTTPURL(s string) bool { +// isHTTPSOrLoopbackURL reports whether s is an absolute URL the sign-out +// flow may hand to the browser: https always; http only when the host is +// loopback (localhost / 127.0.0.0/8 / ::1) — the codebase's dev convention +// is a plain-http IdP on loopback (oidctest, a dev Keycloak), and plaintext +// to anywhere else is never a legitimate navigation target. Embedded +// credentials are rejected. +func isHTTPSOrLoopbackURL(s string) bool { u, err := url.Parse(s) if err != nil || u.Host == "" || u.User != nil { return false } - return u.Scheme == "https" || u.Scheme == "http" + if u.Scheme == "https" { + return true + } + return u.Scheme == "http" && isLoopbackHost(u.Hostname()) +} + +// isLoopbackHost reports whether host names a loopback address: the literal +// "localhost" or an IP in the loopback range. Strict parsing — non-canonical +// IP spellings (octal, hex, v4-in-v6 mapped) never count as loopback. +func isLoopbackHost(host string) bool { + if strings.EqualFold(host, "localhost") { + return true + } + ip, err := netip.ParseAddr(host) + return err == nil && ip.Unmap().IsLoopback() } // sessionCookie builds the host-only session cookie: no Domain attribute @@ -902,18 +916,6 @@ type throttleEntry struct { at time.Time } -func (a *Authenticator) tenantAllowed(tenant string) bool { - if len(a.cfg.AllowedTenants) == 0 { - return true - } - for _, t := range a.cfg.AllowedTenants { - if t == tenant { - return true - } - } - return false -} - // groupsAllowed enforces RequiredGroups: with none configured the gate is // off; otherwise at least one required group must appear verbatim in the // verified claim. A missing or malformed claim parses to no groups and is diff --git a/internal/api/auth_test.go b/internal/api/auth_test.go index 6aa87339..0c23233a 100644 --- a/internal/api/auth_test.go +++ b/internal/api/auth_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "reflect" "strings" "sync" "testing" @@ -426,7 +427,6 @@ func TestSessionFixationPrevented(t *testing.T) { func TestSessionIdleAndAbsoluteExpiry(t *testing.T) { env := newTestEnv(t, func(c *AuthConfig) { - c.IdleTimeout = time.Minute c.AbsoluteTimeout = 10 * time.Minute }) fc := &fakeClock{now: time.Now()} @@ -477,10 +477,17 @@ func TestTenantMembershipRequired(t *testing.T) { } } -func TestTenantNotInAllowlistRejected(t *testing.T) { - env := newTestEnv(t, func(c *AuthConfig) { c.AllowedTenants = []string{"tenant-b"} }) - if status, code := callbackStatus(t, env); status != http.StatusForbidden || code != string(CodeForbidden) { - t.Fatalf("status=%d code=%q", status, code) +// AuthConfig must not grow knobs that cannot be wired to a flag: an +// operator-facing field that is parsed but never populated or consumed is a +// misleading security surface. The session idle window belongs to the +// session store (-session-idle / TCDI_SESSION_IDLE); the login gate is +// -required-groups. Keep the struct free of both dead fields. +func TestAuthConfig_NoDeadSessionKnobs(t *testing.T) { + typ := reflect.TypeOf(AuthConfig{}) + for _, field := range []string{"IdleTimeout", "AllowedTenants"} { + if _, ok := typ.FieldByName(field); ok { + t.Fatalf("AuthConfig.%s is a dead knob — wire it to a flag or remove it", field) + } } } diff --git a/internal/api/connections.go b/internal/api/connections.go index d0f47e2d..ba2c0ca3 100644 --- a/internal/api/connections.go +++ b/internal/api/connections.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "io" + "log/slog" "net/http" "time" @@ -55,6 +56,7 @@ type ConnectionHandler struct { // audit is the dedicated audit-event sink the route emits through // (nil = no domain audit events). audit observability.AuditSink + log *slog.Logger } // NewConnectionHandler wires the handler. domain is the session domain the @@ -66,9 +68,19 @@ func NewConnectionHandler(issuer ConnectionIssuer, tenants TenantResolver, domai tenants: tenants, domain: domain, maxBody: 16 << 10, + log: slog.Default(), } } +// WithLogger attaches the logger the handler writes server-side detail to +// (e.g. the request-body decode detail that never reaches the client). +func (h *ConnectionHandler) WithLogger(l *slog.Logger) *ConnectionHandler { + if l != nil { + h.log = l + } + return h +} + // WithClipboardSource wires the resolver that supplies the workspace's // template clipboard policy for ticket recording (V3.24: the gateway // redirect re-asserts the client flags from what the ticket recorded). @@ -133,10 +145,12 @@ func (h *ConnectionHandler) Create(w http.ResponseWriter, r *http.Request) { return } if len(bytes.TrimSpace(body)) > 0 { - dec := json.NewDecoder(bytes.NewReader(body)) - dec.DisallowUnknownFields() - if err := dec.Decode(&req); err != nil { - writeError(w, r, CodeInvalidRequest, "invalid request body: "+err.Error()) + // SEC-I7: decode detail is logged server-side and never echoed — + // the client gets the generic message every other handler uses. + if err := decodeJSON(body, &req); err != nil { + h.log.Warn("connection create: invalid request body", + "request_id", RequestIDFromContext(r.Context()), "err", err) + writeError(w, r, CodeInvalidRequest, "invalid request body") return } } diff --git a/internal/api/connections_test.go b/internal/api/connections_test.go index 9e8454d9..47cf3e8a 100644 --- a/internal/api/connections_test.go +++ b/internal/api/connections_test.go @@ -65,7 +65,7 @@ func newConnectionEnv(t *testing.T, issuer ConnectionIssuer, opts ...func(*Conne if err != nil { t.Fatalf("sessionhost.ParseDomain: %v", err) } - h := NewConnectionHandler(issuer, defaultTenants(), d) + h := NewConnectionHandler(issuer, defaultTenants(), d).WithLogger(logger) for _, o := range opts { o(h) } @@ -164,6 +164,61 @@ func TestCreateConnection_RequiresAuthAndCSRF(t *testing.T) { } } +// TestCreateConnection_InvalidBodyGenericMessage: a malformed body answers +// 400 INVALID_REQUEST with the generic message every other handler uses — +// the decoder detail is logged server-side, never echoed (SEC-I7). +func TestCreateConnection_InvalidBodyGenericMessage(t *testing.T) { + fi := &fakeIssuer{} + env := newConnectionEnv(t, fi) + sess, csrf := login(t, env, "alice") + + r := doReq(t, env, sess, csrf, http.MethodPost, + "/v1/workspaces/ws_00000000000000000000000001/connections", + `{"takeover":true,"attacker_field":1}`, nil) + defer r.Body.Close() + if r.StatusCode != http.StatusBadRequest { + b, _ := io.ReadAll(r.Body) + t.Fatalf("status=%d body=%s, want 400", r.StatusCode, b) + } + b, _ := io.ReadAll(r.Body) + var e Error + if err := json.Unmarshal(b, &e); err != nil { + t.Fatalf("error body not JSON: %q", b) + } + if e.Code != CodeInvalidRequest || e.Message != "invalid request body" { + t.Fatalf("code=%q message=%q, want INVALID_REQUEST + generic message", e.Code, e.Message) + } + if bytes.Contains(b, []byte("attacker_field")) { + t.Fatalf("decoder detail echoed to client: %s", b) + } + if !strings.Contains(env.logs.String(), "attacker_field") { + t.Fatalf("decoder detail missing from server log: %s", env.logs.String()) + } + if fi.calls != 0 { + t.Fatal("issuer called on an invalid body") + } +} + +// TestCreateConnection_TrailingJSONRejected: the shared decoder accepts +// exactly one document — a second JSON value after the request object is a +// 400 like everywhere else. +func TestCreateConnection_TrailingJSONRejected(t *testing.T) { + fi := &fakeIssuer{} + env := newConnectionEnv(t, fi) + sess, csrf := login(t, env, "alice") + + r := doReq(t, env, sess, csrf, http.MethodPost, + "/v1/workspaces/ws_00000000000000000000000001/connections", + `{"takeover":true} {"extra":true}`, nil) + defer r.Body.Close() + if r.StatusCode != http.StatusBadRequest { + t.Fatalf("status=%d, want 400", r.StatusCode) + } + if fi.calls != 0 { + t.Fatal("issuer called on trailing data") + } +} + // TestCreateConnection_ConnectionInUse: the broker's ErrConnectionInUse maps // to 409 CONNECTION_IN_USE per openapi.yaml. func TestCreateConnection_ConnectionInUse(t *testing.T) { diff --git a/internal/api/data.go b/internal/api/data.go index 51ba5a9e..dade268c 100644 --- a/internal/api/data.go +++ b/internal/api/data.go @@ -368,7 +368,7 @@ func (h *DataHandler) Attach(w http.ResponseWriter, r *http.Request) { return } var req attachDataRequest - if !decodeJSON(body, &req) { + if decodeJSON(body, &req) != nil { writeError(w, r, CodeInvalidRequest, "invalid request body") return } @@ -444,7 +444,7 @@ func (h *DataHandler) Purge(w http.ResponseWriter, r *http.Request) { return } var req purgeDataRequest - if !decodeJSON(body, &req) { + if decodeJSON(body, &req) != nil { writeError(w, r, CodeInvalidRequest, "invalid request body") return } diff --git a/internal/api/logout_test.go b/internal/api/logout_test.go index 59fe51b4..10a209a0 100644 --- a/internal/api/logout_test.go +++ b/internal/api/logout_test.go @@ -310,7 +310,23 @@ func TestLogout_RequiresCSRFAndSession(t *testing.T) { } func TestNewAuthenticator_RejectsBadPostLogoutRedirect(t *testing.T) { - for _, redirect := range []string{"/signed-out", "javascript:alert(1)", "portal.test/signed-out", "ftp://portal.test/x"} { + for _, redirect := range []string{ + "/signed-out", + "javascript:alert(1)", + "portal.test/signed-out", + "ftp://portal.test/x", + // Plain http is never a legitimate post-logout hop off-loopback — + // the browser would follow it over cleartext. Rejected outright, + // not downgraded. + "http://portal.test/signed-out", + "http://192.168.1.10/signed-out", + // Hostnames that only look loopback-adjacent are not loopback. + "http://localhost.evil.test/x", + "http://127.0.0.1.evil.test/x", + "http://2130706433/x", // dotted-quad decimal for 127.0.0.1 — strict parser rejects it + // Credentials in the URL are rejected on any scheme. + "https://user:pw@portal.test/x", + } { iss, err := oidctest.NewIssuer() if err != nil { t.Fatal(err) @@ -329,11 +345,51 @@ func TestNewAuthenticator_RejectsBadPostLogoutRedirect(t *testing.T) { } } -// A discovered endpoint that is not an absolute http(s) URL is not trusted: -// sign-out degrades to the plain 204 instead of handing the browser an -// attacker-shaped navigation target. +// http stays acceptable on loopback only: the dev/test IdP convention is a +// plain-http issuer on loopback (oidctest, a dev Keycloak), so +// "http://localhost…" and "http://127.0.0.1…" redirect targets are dev +// reality — anything off-loopback must be https. +func TestNewAuthenticator_AllowsLoopbackPostLogoutRedirect(t *testing.T) { + for _, redirect := range []string{ + "https://portal.test/signed-out", + "http://localhost/signed-out", + "http://localhost:8080/signed-out", + "http://127.0.0.1:8080/signed-out", + "http://[::1]:8080/signed-out", + } { + iss, err := oidctest.NewIssuer() + if err != nil { + t.Fatal(err) + } + _, err = NewAuthenticator(context.Background(), AuthConfig{ + Issuer: iss.URL(), ClientID: iss.ClientID, + RedirectURL: "https://portal.test/auth/callback", + LoginSealer: testLoginSealer(t), + EndSession: true, + PostLogoutRedirect: redirect, + }, NewInMemorySessionStore(time.Minute), slog.Default()) + iss.Close() + if err != nil { + t.Fatalf("NewAuthenticator rejected PostLogoutRedirect %q: %v", redirect, err) + } + } +} + +// A discovered endpoint that is not an absolute https URL (or http on a +// loopback host) is not trusted: sign-out degrades to the plain 204 instead +// of handing the browser an attacker-shaped navigation target. func TestLogout_UntrustedDiscoveredEndpointIs204(t *testing.T) { - for _, endpoint := range []string{"javascript:alert(1)", "/logout", "//evil.example/logout", "https://user:pw@idp.example/logout"} { + for _, endpoint := range []string{ + "javascript:alert(1)", + "/logout", + "//evil.example/logout", + "https://user:pw@idp.example/logout", + // Plain http off-loopback is not a sign-out target either — a + // compromised or misconfigured discovery document cannot downgrade + // the browser to cleartext. + "http://idp.example/logout", + "http://localhost.evil.example/logout", + } { env := newTestEnvIssuer(t, func(i *oidctest.Issuer) { i.EndSessionEndpoint = endpoint }, func(c *AuthConfig) { c.EndSession = true }) sess := env.loginSession(t) diff --git a/internal/api/workspaces.go b/internal/api/workspaces.go index fe0844be..7d792b48 100644 --- a/internal/api/workspaces.go +++ b/internal/api/workspaces.go @@ -26,16 +26,23 @@ func respondJSON(w http.ResponseWriter, v any) { // decodeJSON decodes exactly one JSON document from body into v: unknown // fields are rejected and trailing data after the first value is an error. -// The error detail is never echoed to the client (SEC-I7); callers respond -// with a generic "invalid request body". -func decodeJSON(body []byte, v any) bool { +// The returned detail is for server-side logs only — it is never echoed to +// the client (SEC-I7); callers respond with a generic "invalid request +// body". +func decodeJSON(body []byte, v any) error { dec := json.NewDecoder(bytes.NewReader(body)) dec.DisallowUnknownFields() if err := dec.Decode(v); err != nil { - return false + return err } var extra any - return dec.Decode(&extra) == io.EOF + if err := dec.Decode(&extra); err != io.EOF { + if err == nil { + return errors.New("json: unexpected trailing data") + } + return err + } + return nil } // TenantResolver maps a verified tenant ID to the Kubernetes namespace @@ -454,7 +461,7 @@ func (h *WorkspaceHandler) Create(w http.ResponseWriter, r *http.Request) { return } var req createWorkspaceRequest - if !decodeJSON(body, &req) { + if decodeJSON(body, &req) != nil { writeError(w, r, CodeInvalidRequest, "invalid request body") return } @@ -606,7 +613,7 @@ func (h *WorkspaceHandler) signal(w http.ResponseWriter, r *http.Request, kind p // be a single well-formed JSON document (SEC-I7). if len(bytes.TrimSpace(body)) > 0 { var v json.RawMessage - if !decodeJSON(body, &v) { + if decodeJSON(body, &v) != nil { writeError(w, r, CodeInvalidRequest, "invalid request body") return } diff --git a/internal/backend/wire.go b/internal/backend/wire.go index d659c3fa..5eae4b80 100644 --- a/internal/backend/wire.go +++ b/internal/backend/wire.go @@ -645,7 +645,8 @@ func (b *Backend) newAppHandler(ctx context.Context, cfg Config, db *store.DB, } return e.ClipboardPolicy, nil }). - WithAuditSink(appAudit) + WithAuditSink(appAudit). + WithLogger(b.log) meHandler := api.NewMeHandler(sessionDomain.String()) connStatusHandler := api.NewConnectionStatusHandler(broker.PublicStater{B: brk}, svc, tenants) dataHandler := api.NewDataHandler(retained, catalog, tenants).