From 88f7d0a5fac51050af5b402536d35bb4a61d4b0f Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 11:22:53 -0400 Subject: [PATCH] Feature 03 integration fix: sanitize discovery errors and narrow CSRF exemption (#25) Co-authored-by: dev_agent Co-committed-by: dev_agent --- internal/auth/auth.go | 34 ++++++++++++++++++++++++++++++++-- internal/auth/auth_test.go | 36 ++++++++++++++++++++++++++++++++++++ internal/server/csrf.go | 4 ++-- internal/server/csrf_test.go | 9 +++++---- internal/server/server.go | 3 +-- 5 files changed, 76 insertions(+), 10 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 4d3c1fb..aa17cdd 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -13,6 +13,7 @@ import ( "net/http" "net/url" "slices" + "sync/atomic" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -107,9 +108,12 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger } client := &http.Client{Timeout: providerTimeout, CheckRedirect: checkRedirect} - provider, err := oidc.NewProvider(oidc.ClientContext(ctx, client), cfg.Issuer) + discovery := *client + recorder := &statusRecorder{next: http.DefaultTransport} + discovery.Transport = recorder + provider, err := oidc.NewProvider(oidc.ClientContext(ctx, &discovery), cfg.Issuer) if err != nil { - return nil, fmt.Errorf("auth: OIDC discovery: %w", err) + return nil, discoveryError(err, int(recorder.status.Load())) } endpoint := provider.Endpoint() var discovered struct { @@ -159,6 +163,32 @@ func New(ctx context.Context, cfg Config, sessions Sessions, logger *slog.Logger }, nil } +// statusRecorder records the status of the last response it carries. +type statusRecorder struct { + next http.RoundTripper + status atomic.Int32 +} + +func (s *statusRecorder) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := s.next.RoundTrip(req) + if err == nil { + s.status.Store(int32(resp.StatusCode)) + } + return resp, err +} + +// discoveryError reports a failed discovery. go-oidc puts the provider's +// response body in its error for a non-200 discovery response, so that +// error is replaced by the status alone. Transport errors carry no response +// content and keep their cause. +func discoveryError(err error, status int) error { + var urlErr *url.Error + if !errors.As(err, &urlErr) && status != 0 && status != http.StatusOK { + return fmt.Errorf("auth: OIDC discovery: provider answered HTTP %d", status) + } + return fmt.Errorf("auth: OIDC discovery: %w", err) +} + // checkEndpoint applies secureurl.Check to the provider URL raw, naming the // endpoint in the error. func checkEndpoint(name, raw string) error { diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 4ecf409..8af6771 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -11,6 +11,7 @@ import ( "encoding/base64" "encoding/json" "errors" + "fmt" "io" "log/slog" "maps" @@ -789,6 +790,41 @@ func TestNewFailsWhenDiscoveryFails(t *testing.T) { } } +func TestNewDiscoveryErrorOmitsProviderBody(t *testing.T) { + t.Parallel() + const sentinel = "sentinel-discovery-body-7f3a" + for _, status := range []int{http.StatusNotFound, http.StatusInternalServerError} { + t.Run(http.StatusText(status), func(t *testing.T) { + t.Parallel() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(status) + io.WriteString(w, sentinel) + })) + t.Cleanup(srv.Close) + logs := &syncBuffer{} + _, err := New(context.Background(), Config{ + Issuer: srv.URL, + ClientID: testClientID, + ClientSecret: testClientSecret, + PublicURL: testPublicURL, + SecretKey: []byte("k"), + AllowedGroups: testAllowedGroups, + }, &memorySessions{}, slog.New(slog.NewTextHandler(logs, nil))) + if err == nil { + t.Fatal("New succeeded without a discovery document") + } + // cmd/server logs the returned error at startup. + slog.New(slog.NewTextHandler(logs, nil)).Error("startup failed", "err", err) + if strings.Contains(logs.String(), sentinel) { + t.Errorf("logs contain the provider response body: %s", logs) + } + if want := fmt.Sprintf("auth: OIDC discovery: provider answered HTTP %d", status); err.Error() != want { + t.Errorf("error = %q, want %q", err, want) + } + }) + } +} + func TestNewFailsOnIssuerMismatch(t *testing.T) { t.Parallel() p := newFakeProvider(t) diff --git a/internal/server/csrf.go b/internal/server/csrf.go index 3f58ff2..65c21a3 100644 --- a/internal/server/csrf.go +++ b/internal/server/csrf.go @@ -12,7 +12,7 @@ import ( // GET /api/v1/me. const CSRFHeader = "X-MealQuest-CSRF" -// requireCSRF passes GET, HEAD and OPTIONS requests to next; every other +// requireCSRF passes GET requests to next; every other // request must carry CSRFHeader equal to its session's CSRF token or get 403 // csrf. It runs behind requireSession, which puts the session in the context. func requireCSRF(logger *slog.Logger) func(http.Handler) http.Handler { @@ -21,7 +21,7 @@ func requireCSRF(logger *slog.Logger) func(http.Handler) http.Handler { return func(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.Method { - case http.MethodGet, http.MethodHead, http.MethodOptions: + case http.MethodGet: next.ServeHTTP(w, r) return } diff --git a/internal/server/csrf_test.go b/internal/server/csrf_test.go index e509c8e..7e1dd54 100644 --- a/internal/server/csrf_test.go +++ b/internal/server/csrf_test.go @@ -38,10 +38,9 @@ func TestCSRFThroughRouter(t *testing.T) { {"POST with right header", http.MethodPost, "/api/v1/me", []string{testSession.CSRFToken}, http.StatusMethodNotAllowed, methodNotAllowedJSON}, {"PATCH with right header", http.MethodPatch, "/api/v1/settings/household", []string{testSession.CSRFToken}, http.StatusNotFound, notFoundJSON}, {"GET without header", http.MethodGet, "/api/v1/me", nil, http.StatusOK, ""}, - // HEAD and OPTIONS pass the check and reach the router, which - // answers neither for /api/v1/me. - {"HEAD without header", http.MethodHead, "/api/v1/me", nil, http.StatusMethodNotAllowed, ""}, - {"OPTIONS without header", http.MethodOptions, "/api/v1/me", nil, http.StatusMethodNotAllowed, methodNotAllowedJSON}, + // Only GET is exempt; HEAD and OPTIONS need the header too. + {"HEAD without header", http.MethodHead, "/api/v1/me", nil, http.StatusForbidden, csrfJSON}, + {"OPTIONS without header", http.MethodOptions, "/api/v1/me", nil, http.StatusForbidden, csrfJSON}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() @@ -89,6 +88,8 @@ func TestRequireCSRF(t *testing.T) { {"right header reaches handler", http.MethodPost, &testSession, testSession.CSRFToken, http.StatusNoContent}, {"wrong header", http.MethodPut, &testSession, "nope", http.StatusForbidden}, {"GET needs no header", http.MethodGet, &testSession, "", http.StatusNoContent}, + {"HEAD needs the header", http.MethodHead, &testSession, "", http.StatusForbidden}, + {"OPTIONS needs the header", http.MethodOptions, &testSession, "", http.StatusForbidden}, {"no session in context", http.MethodPost, nil, "", http.StatusInternalServerError}, {"session without token", http.MethodPost, &auth.Session{Subject: "user-123"}, "", http.StatusInternalServerError}, } { diff --git a/internal/server/server.go b/internal/server/server.go index 82a1e8c..46520c2 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -27,8 +27,7 @@ type RouteMounter interface { // only from a peer in trusted (see resolveOrigin). version is reported by // GET /health; auth mounts the /auth routes; sessions guards /api/v1 and // /media; assets is the web build served for every other GET. /health, /auth -// and the web build are public. Requests to /api/v1 other than GET, HEAD and -// OPTIONS also need the session's CSRF token in CSRFHeader. +// and the web build are public. Requests to /api/v1 other than GET also need the session's CSRF token in CSRFHeader. func NewRouter(logger *slog.Logger, version string, trusted forwarded.Trusted, auth RouteMounter, sessions SessionLookup, assets fs.FS) (http.Handler, error) { notFound := apiErrorHandler(logger, http.StatusNotFound, "not_found", "not found") methodNotAllowed := apiErrorHandler(logger, http.StatusMethodNotAllowed, "method_not_allowed", "method not allowed")