From b9e6d74cc652961be596896f54d5ec8441cde8fc Mon Sep 17 00:00:00 2001 From: Vinayak Mishra Date: Fri, 7 Aug 2026 17:15:16 +0545 Subject: [PATCH] fix(middleware): admin JSON endpoints no longer serve unauthenticated callers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Soft-fail keyed on Accept containing application/json, so `*/*` or an absent header matched neither branch and reached the handler — plain curl against /admin/metrics returned 200 with runtime and article statistics. Soft-fail now requires explicit text/html, and the JSON-only endpoints take hard SessionAuth. --- CHANGELOG.md | 11 ++++++ internal/commands/serve/command.go | 15 ++++++-- internal/middleware/errors.go | 11 ++++++ internal/middleware/session.go | 14 ++++++-- internal/middleware/session_test.go | 56 ++++++++++++++++++++++++++++- 5 files changed, 102 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 73700f0..abb8d64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Security +- **Unauthenticated read of `/admin/stats` and `/admin/metrics` is fixed.** `SoftSessionAuth` + decided between the login overlay and a hard 401 by testing the request for + `Accept: application/json`. A caller sending anything non-committal — `*/*`, or no + Accept header at all, which is curl's default — matched neither branch and fell + through to the handler, which returned its payload in full: Go version, goroutine + count, heap and GC statistics, uptime, and article/draft/tag counts. Present since + the endpoints moved under `/admin`; affects any deployment with `ADMIN_USERNAME` and + `ADMIN_PASSWORD` set. No article content, credentials, or session data was reachable, + and write paths were never exposed. Fixed twice over: soft-fail now requires an + explicit `text/html` Accept, and the two JSON-only endpoints moved to hard + `SessionAuth`, which has no fall-through path at all. Browser behaviour is unchanged. - Go toolchain 1.26.4 → 1.26.5 for GO-2026-5856. - `golang.org/x/crypto` 0.48.0 → 0.52.0, `golang.org/x/net` 0.51.0 → 0.55.0, `golang.org/x/sys` 0.41.0 → 0.45.0, `golang.org/x/text` 0.38.0 → 0.40.0. diff --git a/internal/commands/serve/command.go b/internal/commands/serve/command.go index 42fe124..21de7e7 100644 --- a/internal/commands/serve/command.go +++ b/internal/commands/serve/command.go @@ -594,8 +594,19 @@ func setupRoutes(router *gin.Engine, h *handlers.Router, sessionStore *middlewar registerGET(adminGroup, "/writing", h.Admin.Writing) registerGET(adminGroup, "/drafts", h.Admin.Drafts) adminGroup.POST("/cache/clear", h.ClearCache) - registerGET(adminGroup, "/stats", h.Admin.Stats) - registerGET(adminGroup, "/metrics", h.Admin.Metrics) + // JSON-only endpoints take hard auth, not soft. Soft auth exists so an HTML + // page can render the login overlay in place; a handler that always answers + // JSON has no overlay to render, so soft-failing it can only ever mean + // returning the data. Belt and braces with the Accept check in + // SoftSessionAuth — neither should be the only thing standing here. + adminJSONGroup := router.Group("/admin") + adminJSONGroup.Use( + middleware.RecoveryWithErrorHandler(logger), + middleware.SessionAuth(sessionStore), + middleware.NoCache(), + ) + registerGET(adminJSONGroup, "/stats", h.Admin.Stats) + registerGET(adminJSONGroup, "/metrics", h.Admin.Metrics) adminGroup.POST("/articles/reload", h.Admin.ReloadArticles) // AMA moderation routes diff --git a/internal/middleware/errors.go b/internal/middleware/errors.go index 891935b..77cc4f5 100644 --- a/internal/middleware/errors.go +++ b/internal/middleware/errors.go @@ -26,6 +26,17 @@ func wantsJSON(c *gin.Context) bool { return strings.Contains(accept, "application/json") } +// acceptsHTML reports whether the caller explicitly asked for HTML. +// +// Deliberately not the negation of wantsJSON. Use this to decide whether a caller +// can be shown an HTML affordance such as the login overlay: the answer must be no +// for `Accept: */*`, an absent Accept header, or anything else non-committal, so +// that a caller which cannot render the affordance is refused rather than handed +// whatever the handler would have produced. +func acceptsHTML(c *gin.Context) bool { + return strings.Contains(c.GetHeader("Accept"), "text/html") +} + // errorHTML returns a minimal self-contained HTML error page. // Does not use the template engine — middleware must be dependency-free. func errorHTML(status int, message string) string { diff --git a/internal/middleware/session.go b/internal/middleware/session.go index 987433c..0f001d7 100644 --- a/internal/middleware/session.go +++ b/internal/middleware/session.go @@ -197,8 +197,18 @@ func SoftSessionAuth(store *SessionStore, secureCookie bool) gin.HandlerFunc { // fall-through would leak handler data (see GHSA / #42). // Do NOT collapse this asymmetry without revisiting the bypass class. if c.Request.Method == http.MethodGet || c.Request.Method == http.MethodHead { - if wantsJSON(c) { - slog.Warn("Unauthenticated JSON request", "method", c.Request.Method, "path", c.Request.URL.Path) + // Fail closed. Soft-fail hands the request to the handler so an HTML page + // can render the login overlay in place, so it is only safe for a caller + // that both prefers HTML and can render it. Testing wantsJSON alone left + // `Accept: */*` — curl's default, and most scripts — falling through to + // the handler, which for the JSON-only admin endpoints meant returning + // their data in full to an unauthenticated caller. + // + // Order matters: a client that lists both still gets 401 when it prefers + // JSON, so this tightens the non-committal case without loosening any + // case that already refused. + if wantsJSON(c) || !acceptsHTML(c) { + slog.Warn("Unauthenticated non-HTML request", "method", c.Request.Method, "path", c.Request.URL.Path) abortWithError(c, http.StatusUnauthorized, "Authentication required") return } diff --git a/internal/middleware/session_test.go b/internal/middleware/session_test.go index 5a4d0c6..cc71f2c 100644 --- a/internal/middleware/session_test.go +++ b/internal/middleware/session_test.go @@ -280,6 +280,9 @@ func TestSoftSessionAuth_NoSession_GET(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest(http.MethodGet, "/compose", http.NoBody) + // A browser is the only caller that can render the overlay, and a browser always + // sends this. Without it the request is non-committal and must be refused. + c.Request.Header.Set("Accept", "text/html,application/xhtml+xml") handler := SoftSessionAuth(store, false) handler(c) @@ -324,6 +327,7 @@ func TestSoftSessionAuth_InvalidToken_GET(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest(http.MethodGet, "/admin", http.NoBody) + c.Request.Header.Set("Accept", "text/html,application/xhtml+xml") c.Request.AddCookie(&http.Cookie{Name: "_session", Value: "bogus"}) handler := SoftSessionAuth(store, false) @@ -345,6 +349,7 @@ func TestSoftSessionAuth_HEAD_Request(t *testing.T) { w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) c.Request = httptest.NewRequest(http.MethodHead, "/compose", http.NoBody) + c.Request.Header.Set("Accept", "text/html,application/xhtml+xml") handler := SoftSessionAuth(store, false) handler(c) @@ -467,7 +472,12 @@ func TestSoftSessionAuth_NoSession_JSON_AcceptVariants(t *testing.T) { {"with charset", "application/json; charset=utf-8", true, http.StatusUnauthorized}, {"q-weighted multi", "text/html;q=0.9, application/json;q=1.0", true, http.StatusUnauthorized}, {"html only", "text/html", false, 0}, // falls through to soft-fail - {"empty", "", false, 0}, // falls through to soft-fail + // A caller that does not commit to HTML cannot render the overlay, so it is + // refused rather than handed whatever the handler produces. This case used to + // fall through, which is how an unauthenticated `curl /admin/metrics` — no + // Accept header at all — got a 200 with full runtime metrics. + {"empty", "", true, http.StatusUnauthorized}, + {"wildcard only", "*/*", true, http.StatusUnauthorized}, } for _, tc := range cases { @@ -565,3 +575,47 @@ func TestShutdownRateLimiters_StopsAllCleanupGoroutines(t *testing.T) { // Idempotency: second call should be a no-op. assert.NotPanics(t, ShutdownRateLimiters) } + +// The bypass this pins is the one an attacker actually runs: plain `curl` against a +// JSON admin endpoint. curl sends no Accept header, which was neither "wants JSON" +// nor anything the old check refused, so the request fell through to the handler and +// came back 200 with the payload. Exercised through a real router with a real +// handler, because the leak was never in the middleware's own response — it was in +// what ran after the middleware declined to stop it. +func TestSoftSessionAuth_NonCommittalAccept_DoesNotReachHandler(t *testing.T) { + gin.SetMode(gin.TestMode) + + for _, tc := range []struct { + name string + accept string + }{ + {"no accept header at all (curl default)", ""}, + {"wildcard", "*/*"}, + {"unrelated type", "application/xml"}, + } { + t.Run(tc.name, func(t *testing.T) { + store := NewSessionStore() + router := gin.New() + handlerRan := false + router.Use(SoftSessionAuth(store, false)) + router.GET("/admin/metrics", func(c *gin.Context) { + handlerRan = true + c.JSON(http.StatusOK, gin.H{"goroutines": 25, "memory": gin.H{"heap_alloc": 51403288}}) + }) + + req := httptest.NewRequest(http.MethodGet, "/admin/metrics", http.NoBody) + if tc.accept != "" { + req.Header.Set("Accept", tc.accept) + } + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + assert.False(t, handlerRan, "handler must not run for an unauthenticated non-HTML caller") + assert.Equal(t, http.StatusUnauthorized, w.Code) + for _, leaked := range []string{"goroutines", "heap_alloc", "memory"} { + assert.NotContains(t, w.Body.String(), leaked, + "response must not carry admin payload key %q", leaked) + } + }) + } +}