fix(api): drop dead auth knobs, require https sign-out URLs, generic decode errors (FIX-AUTH-P3) - #134
Merged
Conversation
…decode errors (FIX-AUTH-P3)
- 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Auth-hardening leftovers: dead
AuthConfigknobs, a plaintext-scheme gap on the sign-out redirect, and a decoder-error echo inconnections.Create.AuthConfig.IdleTimeoutwas defaulted inwithDefaultsbut never read: the sliding idle window is owned by the session store (-session-idle/TCDI_SESSION_IDLE→store.NewSessionStore,internal/backend/config.go,wire.go). Wiring it would have created a second source of truth for one window — the misleading pattern itself.AuthConfig.AllowedTenantswas enforced intenantAllowedbut no flag/env/chart path could ever populate it, so the gate could never engage;-required-groups(oidc.requiredGroups) remains the supported login gate and-tenant-namespacesbounds what a tenant can do. Deleting both is the smallest correct change: no reachable behavior changes (an empty allowlist already meant allow-all), and the misleading surface is gone rather than grown.TestTenantNotInAllowlistRejectedis removed with the feature it tested;TestSessionIdleAndAbsoluteExpiryno longer sets the dead field (it always drove the store'sidledirectly).PostLogoutRedirectand the discoveredend_session_endpointnow require an absolutehttpsURL;httpis accepted only for loopback hosts (localhost, 127.0.0.0/8,::1) via a strictnetipparse (mapped/octal spellings don't count). The loopback carve-out matches the codebase's existing dev convention: the in-process test IdP (oidctest/httptest) and dev Keycloak setups serve plain http on loopback, and the whole logout suite exercises it. The Helm schema already required^https://, so the Go side now matches it.connections.Createno longer echoes decoder errors. It decodes through the shareddecodeJSONhelper — which now returns the error so handlers can log it, and also rejects trailing data after the first document (the inline decode lacked that check). The 400 carries the generic"invalid request body"; the detail is logged server-side with the request id (WithLogger, wired tob.log). It was the sole handler deviating from the convention.Threat-model status lines added as S25–S27; Boundary-3 and portal-session claims updated; install runbook wording aligned.
Regression evidence (fails on v0.5.0, passes here)
Run against a
v0.5.0checkout with the new test files applied:v0.5.0 result:
TestAuthConfig_NoDeadSessionKnobsFAIL (AuthConfig.IdleTimeout is a dead knob),TestNewAuthenticator_RejectsBadPostLogoutRedirectFAIL (accepted"http://portal.test/signed-out"),TestLogout_UntrustedDiscoveredEndpointIs204FAIL ("http://idp.example/logout"→ 200),TestCreateConnection_InvalidBodyGenericMessageFAIL (body echoedinvalid request body: json: unknown field "attacker_field"),TestCreateConnection_TrailingJSONRejectedFAIL (201 on{"takeover":true} {"extra":true}). All pass on this branch.Verification
go build ./...,go vet ./...clean;gofmtclean.go test ./internal/api/ ./internal/backend/— ok;go test -race ./internal/api/ ./internal/backend/— ok.Test plan
TestAuthConfig_NoDeadSessionKnobsTestNewAuthenticator_RejectsBadPostLogoutRedirect(+ non-loopback http, look-alike loopback names, credential URLs)TestNewAuthenticator_AllowsLoopbackPostLogoutRedirectTestLogout_UntrustedDiscoveredEndpointIs204(+http://idp.example/logout,http://localhost.evil.example/logout)TestCreateConnection_InvalidBodyGenericMessage; trailing data:TestCreateConnection_TrailingJSONRejectedDiff stat (
git diff --stat origin/main...HEAD)v1.0 fix task, requested by the project orchestrator; the advisor reviews and merges.
Generated with Devin