Register pprof debug endpoints behind admin-token auth#809
Conversation
…min-token auth Add /debug/pprof/ routes gated by X-Admin-Token (returns 401 on missing/wrong token, 200 with valid token). Named profiles (goroutine, heap, etc.) share the same gating. When no admin token is configured the endpoints are open, consistent with existing AdminTokenAuth behavior.
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
🔴 PR-AF Review — Merge Blocked — Must-Fix Found
🚫 Merge blocked. 1 must-fix issue found by automated review.
Automated multi-agent code review · PR-AF built with AgentField
8 findings · 🚫 1 blocking · 💬 7 advisory · 🔴 0 critical · 🟠 5 important · 🔵 3 suggestions · ⚪ 0 nitpicks
PR Overview
Summary
- Registers
/debug/pprof/*endpoints (index, cmdline, profile, symbol, trace, and named profiles) on the control-plane gin router. - All pprof endpoints are gated by the
X-Admin-Tokenheader with constant-time comparison, returning 401 on missing or wrong tokens. - When no admin token is configured, pprof endpoints remain open (consistent with existing admin route behavior).
- Seven tests cover valid-token, no-token, wrong-token, and named-profile scenarios.
Closes #428
Changes
control-plane/internal/server/server.go— Registers the new admin pprof route group.control-plane/internal/server/routes_admin.go— Adds pprof middleware (constant-time token check returning 401) and route registration.control-plane/internal/server/routes_admin_test.go— Seven test cases covering token auth and profile endpoint selection.
Test plan
go vet ./...passes clean.go test ./internal/server/...passes (the pre-existingTestStartAndStopCoverAdditionalBranchesfailure is unrelated and also present on clean main).- Manually verified: hitting
/debug/pprof/without a token returns 401; with the correct token returns the pprof index.
🤖 Built with AgentField SWE-AF
🔌 Powered by AgentField
Key Findings
1 issue(s) should be addressed before merge:
- 🟠 POST symbolization is captured by the named-profile fallback (
control-plane/internal/server/routes_admin.go:61) —POST /debug/pprof/symboldoes not reachpprof.Symbol. Gate: This is a regression of previously-working pprof symbolization behavior that breaks standard profiling clients with a demonstrated 404 failure path.
7 advisory finding(s) surfaced as non-blocking:
- 🟠 Document and warn for the newly open pprof configuration path (
control-plane/internal/server/routes_admin.go:49) - 🟠 Align pprof token failures with admin-route 403 semantics (
control-plane/internal/server/routes_admin.go:84) - 🟠 Allow the pprof admin token header in default CORS preflights (
control-plane/internal/server/routes_admin.go:81) - 🟠 POST /debug/pprof/symbol is captured by the profile fallback (
control-plane/internal/server/routes_admin.go:61) - 🔵 Cover the production API-key and admin-token composition (
control-plane/internal/server/routes_admin_test.go:14) - … and 2 more (see All Findings by Severity)
Files with findings: control-plane/internal/server/routes_admin.go, control-plane/internal/server/routes_admin_test.go, control-plane/internal/server/routes_middleware.go
All Findings by Severity
🟠 Important (5)
- POST symbolization is captured by the named-profile fallback
control-plane/internal/server/routes_admin.go:61 - Document and warn for the newly open pprof configuration path
control-plane/internal/server/routes_admin.go:49 - Align pprof token failures with admin-route 403 semantics
control-plane/internal/server/routes_admin.go:84 - Allow the pprof admin token header in default CORS preflights
control-plane/internal/server/routes_admin.go:81 - POST /debug/pprof/symbol is captured by the profile fallback
control-plane/internal/server/routes_admin.go:61
🔵 Suggestion (3)
- Cover the production API-key and admin-token composition
control-plane/internal/server/routes_admin_test.go:14 - Pprof auth tests omit the production global API-key layer
control-plane/internal/server/routes_admin_test.go:20 - Allow the admin token header in pprof CORS preflights
control-plane/internal/server/routes_middleware.go:34
Review Process Details
Dimensions Analyzed (5):
- Composed authentication and browser credential contract — 3 file(s)
- Inherited global API-key middleware composition — 4 file(s)
- POST
/debug/pprof/symboldispatches to the correct handler — 2 file(s) - CORS preflight accepts the required admin-token header — 3 file(s)
- Configuration and deployment contract for the new debug surface — 2 file(s)
Meta-Dimension Lenses (3):
- Semantic — 3 dimension(s), 93% coverage confidence
- Mechanical — 3 dimension(s), 93% coverage confidence
- Systemic — 2 dimension(s), 90% coverage confidence
Cross-Reference & Adversary Analysis:
- 5 finding(s) adversarially tested: 4 confirmed, 1 challenged
Pipeline Stats
| Metric | Value |
|---|---|
| Duration | 1139.4s |
| Agent invocations | 29 |
| Coverage iterations | 1 |
| Estimated cost | N/A (provider does not report cost) |
| Budget exhausted | No |
| PR type | security |
| Complexity | medium |
Review ID: rev_82c01cb9713c
| pprofGroup.GET("/", gin.WrapF(pprof.Index)) | ||
| pprofGroup.GET("/cmdline", gin.WrapF(pprof.Cmdline)) | ||
| pprofGroup.GET("/profile", gin.WrapF(pprof.Profile)) | ||
| pprofGroup.GET("/symbol", gin.WrapF(pprof.Symbol)) |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: Debug endpoint (pprof) functionality gap for POST method without demonstrated production impact or regression of user-facing behavior.
POST symbolization is captured by the named-profile fallback
🟠 IMPORTANT · confidence 99%
Register POST /symbol with gin.WrapF(pprof.Symbol) and retain the named-profile catch-all for actual profiles. Gin registers the explicit /symbol handler only for GET, while Any("/:name") registers a POST route that captures symbol as name. The fallback invokes pprof.Handler("symbol"), but symbol is an endpoint rather than a runtime profile, so the standard library returns 404 Unknown profile. This breaks standard pprof symbolization clients that send program counters in a POST body.
Evidence
Step 1: A client sends
POST /debug/pprof/symbolto the router installed byregisterPprofRoutes.
Step 2:pprofGroup.GET("/symbol", gin.WrapF(pprof.Symbol))at line 61 installs no POST handler, whereaspprofGroup.Any("/:name", ...)at line 64 installs/:namefor POST (Gin v1.10.1RouterGroup.Anyloops over all methods).
Step 3: For that POST route, Gin bindsc.Param("name")to"symbol"and callspprof.Handler("symbol").ServeHTTP(...)at line 65.
Step 4: Installed Go 1.26.5 sourcenet/http/pprof/pprof.go:251-256callspprof.Lookup("symbol"); it is nil and returns HTTP 404. In contrast,pprof.Symbolat lines 201-215 explicitly readsr.Bodywhen the method is POST. The current tests exercise neither GET/symbol?...nor POST/symbolwith a body, so they do not detect this regression.
💡 Suggested Fix
Add pprofGroup.POST("/symbol", gin.WrapF(pprof.Symbol)). Add tests for GET /debug/pprof/symbol?<pc> and POST /debug/pprof/symbol with a PC list body, asserting the symbol response (and specifically non-404), while keeping a named-profile request covered to prove the catch-all still works.
Gin route matching and pprof symbol handler compatibility · confidence 99%
🤖 Reviewed by AgentField PR-AF
| } | ||
| } | ||
|
|
||
| // registerPprofRoutes installs Go pprof endpoints under /debug/pprof/, gated |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: Documentation gaps and missing startup warnings are explicitly listed as non-blocking concerns.
Document and warn for the newly open pprof configuration path
🟠 IMPORTANT · confidence 90%
Document that AdminToken now secures /debug/pprof/* and emit a startup warning when the token is empty while the debug endpoint is exposed.
AgentFieldServer.setupRoutes registers pprof unconditionally at server.go:885-889, but the missing-token warning at server.go:291-294 only fires when features.did.authorization.enabled is true. When authorization is disabled and no token is configured, pprofAdminAuth at routes_admin.go:73-77 allows all requests, leaving the runtime-debug surface unauthenticated without alerting the operator.
Evidence
Step 1: An operator can run with
AGENTFIELD_AUTHORIZATION_ENABLED=false(documented default) and no API key;docs/ENVIRONMENT_VARIABLES.mdneither listsAGENTFIELD_AUTHORIZATION_ADMIN_TOKENnor says that pprof requires it, while.env.example:52says the token is only for “tag approval, policy management.”
Step 2:AgentFieldServer.setupRoutescallss.registerPprofRoutes()unconditionally after route registration (server.go:885-889), independent ofAuthorization.Enabled.
Step 3:registerPprofRoutesreads the emptys.config.Features.DID.Authorization.AdminTokenand installspprofAdminAuth(adminToken)(routes_admin.go:52-56); that middleware callsc.Next()without checking a header when the token is empty (routes_admin.go:73-77). With no configured API key, the inheritedAPIKeyAuthalso explicitly allows every request (middleware/auth.go:26-33).
Step 4: The only missing-token warning is nested underif cfg.Features.DID.Authorization.Enabled(server.go:291-294), so this reachable deployment gets no warning even though/debug/pprof/is open.
💡 Suggested Fix
Update AdminToken documentation to include /debug/pprof/* and X-Admin-Token in config comments, YAML/env samples, and ENVIRONMENT_VARIABLES.md. Emit a warning for empty tokens whenever pprof is accessible, regardless of authorization status.
Configuration and operator-documentation contract · confidence 90%
🤖 Reviewed by AgentField PR-AF
| token := c.GetHeader("X-Admin-Token") | ||
|
|
||
| if subtle.ConstantTimeCompare([]byte(token), []byte(adminToken)) != 1 { | ||
| c.AbortWithStatus(http.StatusUnauthorized) |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: gate error
Align pprof token failures with admin-route 403 semantics
🟠 IMPORTANT · confidence 99%
Align the pprof authentication failure response with the standard admin-route 403 semantics.
pprofAdminAuth currently aborts with a bare 401 at control-plane/internal/server/routes_admin.go:81-85, while the established middleware.AdminTokenAuth used by registerAdminRoutes returns a 403 JSON body at control-plane/internal/server/middleware/auth.go:173-180. Clients expecting the consistent 403 status and error schema from other admin routes will encounter incompatible behavior on /debug/pprof.
Evidence
Changed pprof middleware,
control-plane/internal/server/routes_admin.go:81-85:token := c.GetHeader("X-Admin-Token")followed byif subtle.ConstantTimeCompare([]byte(token), []byte(adminToken)) != 1 { c.AbortWithStatus(http.StatusUnauthorized); return }. Existing admin-route middleware,control-plane/internal/server/middleware/auth.go:173-180:token := c.GetHeader("X-Admin-Token")followed by the same comparison, butc.AbortWithStatusJSON(http.StatusForbidden, gin.H{"error": "forbidden", "message": "admin token required for this operation (use X-Admin-Token header)"}).registerAdminRoutesinstalls that middleware atcontrol-plane/internal/server/routes_admin.go:20-24.
💡 Suggested Fix
Reuse middleware.AdminTokenAuth(adminToken) for the pprof group, or make pprofAdminAuth return the same 403 JSON response as AdminTokenAuth.
Consistency Verifier · confidence 99%
🤖 Reviewed by AgentField PR-AF
| return | ||
| } | ||
|
|
||
| token := c.GetHeader("X-Admin-Token") |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: Admin route CORS configuration issue preventing browser access with default settings, but workaround exists via explicit CORS config or non-browser clients; does not impact production-critical user-facing paths.
Allow the pprof admin token header in default CORS preflights
🟠 IMPORTANT · confidence 99%
Add X-Admin-Token to the fallback CORS AllowHeaders list in routes_middleware.go so authenticated pprof browser requests can complete preflight.
pprofAdminAuth requires the non-simple header X-Admin-Token, but the global CORS fallback used when api.cors.allowed_headers is unset only allows Origin, Content-Type, Accept, Authorization, and X-API-Key (lines 34-35). Consequently, browsers block the preflight before the request reaches the pprof middleware, breaking authenticated browser access even from allowed origins.
Evidence
Changed end, control-plane/internal/server/routes_admin.go:81:
token := c.GetHeader("X-Admin-Token"). CORS end, control-plane/internal/server/routes_middleware.go:22 assignsAllowHeaders: s.config.API.CORS.AllowedHeaders, and lines 34-35 replace an empty list with[]string{"Origin", "Content-Type", "Accept", "Authorization", "X-API-Key"}.X-Admin-Tokenis absent. The configurable field isAllowedHeaders []stringin control-plane/internal/config/config.go:409.
💡 Suggested Fix
Add X-Admin-Token to the fallback corsConfig.AllowHeaders list (and document that explicitly configured api.cors.allowed_headers must include it whenever browser access to authenticated pprof is enabled).
Consistency Verifier · confidence 99%
🤖 Reviewed by AgentField PR-AF
| pprofGroup.GET("/", gin.WrapF(pprof.Index)) | ||
| pprofGroup.GET("/cmdline", gin.WrapF(pprof.Cmdline)) | ||
| pprofGroup.GET("/profile", gin.WrapF(pprof.Profile)) | ||
| pprofGroup.GET("/symbol", gin.WrapF(pprof.Symbol)) |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: Debug endpoint (pprof) functionality gap for POST method without demonstrated production impact or regression of user-facing behavior.
POST /debug/pprof/symbol is captured by the profile fallback
🟠 IMPORTANT · confidence 99%
Register pprof.Symbol for POST as well as GET so POST body symbol lookups are not swallowed by the fallback route.
pprof.Symbol reads symbol counters from r.Body on POST, but control-plane/internal/server/routes_admin.go:61 only registers it for GET. Because pprofGroup.Any("/:name") also matches POST, POST /debug/pprof/symbol falls through to the fallback handler (pprof.Handler("symbol")), which looks up a named profile instead of parsing the body and returns 404.
Evidence
Changed code, control-plane/internal/server/routes_admin.go:61-65:
pprofGroup.GET("/symbol", gin.WrapF(pprof.Symbol))
pprofGroup.Any("/:name", func(c *gin.Context) {
pprof.Handler(c.Param("name")).ServeHTTP(c.Writer, c.Request)
}Go standard library, net/http/pprof/pprof.go:201-214:
// We have to read the whole POST body before
if r.Method == "POST" {
b = bufio.NewReader(r.Body)
} else {
b = bufio.NewReader(strings.NewReader(r.URL.RawQuery))
}Gin routergroup.go:19-22, 116-118, 145-150:
anyMethods = []string{ http.MethodGet, http.MethodPost, ... }
func (group *RouterGroup) GET(...) { return group.handle(http.MethodGet, relativePath, handlers) }
func (group *RouterGroup) Any(...) { for _, method := range anyMethods { group.handle(method, relativePath, handlers) } }Go standard library, net/http/pprof/pprof.go:251-256:
p := pprof.Lookup(string(name))
if p == nil {
serveError(w, http.StatusNotFound, "Unknown profile")
return
}
💡 Suggested Fix
Register pprof.Symbol for POST as well as GET, e.g. pprofGroup.Match([]string{http.MethodGet, http.MethodPost}, "/symbol", gin.WrapF(pprof.Symbol)), so POST requests do not fall through to Any("/:name").
Consistency Verifier · confidence 99%
🤖 Reviewed by AgentField PR-AF
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestPprofIndexWithValidAdminToken(t *testing.T) { |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: Missing test coverage for middleware interaction order falls under 'missing tests for edge cases' which is explicitly non-blocking.
Cover the production API-key and admin-token composition
🔵 SUGGESTION · confidence 99%
Add integration tests that exercise the production middleware order—applyGlobalMiddleware() before registerPprofRoutes()—to verify the actual two-credential contract for pprof endpoints. Current tests mount registerPprofRoutes() on a naked gin.New() router, so they never hit APIKeyAuth's JSON 401 and falsely imply an admin-token-only request would succeed in production. This gap also hides the empty-admin-token bypass, leaving the real authorization sequence (neither credential → JSON 401, API-key-only → empty 401, both → success) completely uncovered.
Evidence
Step 1:
setupRoutescallss.applyGlobalMiddleware()befores.registerPprofRoutes()incontrol-plane/internal/server/server.go.
Step 2:applyGlobalMiddlewareinstallsmiddleware.APIKeyAuthons.Router;/debug/pprof/is neither a skip path nor one of the middleware's hard-coded bypass prefixes.
Step 3: whenAPIKeyis configured andX-API-Key/Bearer credentials are absent or wrong,APIKeyAuthcallsAbortWithStatusJSON(401, ...)before Gin runs the pprof group middleware, regardless ofX-Admin-Token.
Step 4: when the API key is valid, execution reachespprofAdminAuth, which returns a bare 401 for a missing/wrong admin token, or continues when the captured token is empty.
Step 5: every added test in this file constructsRouter: gin.New()and calls onlysrv.registerPprofRoutes(); none callsapplyGlobalMiddleware()first, so none verifies these reachable production outcomes.
💡 Suggested Fix
Add a table-driven test that builds the composed router (applyGlobalMiddleware then registerPprofRoutes) and asserts status codes and response shapes for: no credentials, API key only, admin token only, and both credentials. Include the API-key-configured/empty-admin-token case to document that bypass explicitly.
Production router authorization composition · confidence 99%
🤖 Reviewed by AgentField PR-AF
| gin.SetMode(gin.TestMode) | ||
|
|
||
| srv := &AgentFieldServer{ | ||
| Router: gin.New(), |
There was a problem hiding this comment.
Note
Advisory — non-blocking. Safe to merge and address in a follow-up.
Why non-blocking: This is a missing test-coverage issue for middleware interactions; the production code appears to enforce the correct auth ordering, and the finding does not demonstrate a reachable security vulnerability, build break, or regression in shipped behavior.
Pprof auth tests omit the production global API-key layer
🔵 SUGGESTION · confidence 98%
Add composed-stack table tests for pprof that call applyGlobalMiddleware before registerPprofRoutes so the production auth ordering is covered.
The current tests invoke registerPprofRoutes directly on a bare gin.New() router at routes_admin_test.go:19-24, so they only assert that an admin token returns 200. They never exercise the production chain where applyGlobalMiddleware installs middleware.APIKeyAuth first at routes_middleware.go:79-84, which means a missing API key aborts with a JSON 401 at auth.go:121-134 before the admin check at routes_admin.go:81-85 can run. This leaves the four-credential matrix and the empty-admin-token behavior both uncovered and undocumented.
Evidence
Step 1:
setupRoutescallsapplyGlobalMiddleware()atserver.go:864beforeregisterPprofRoutes()atserver.go:888.
Step 2:applyGlobalMiddlewareinstallsmiddleware.APIKeyAuthons.Routeratroutes_middleware.go:79-84;/debug/pprofis neither in the configured automatic skips nor in anyAPIKeyAuthbuilt-in skip atauth.go:42-99.
Step 3: For a configured API key, a pprof request without a validX-API-Key/Bearer key reachesauth.go:121-134, which aborts with the API-key401before the group middleware can run. A valid API key reachespprofAdminAuth, which rejects a missing/wrongX-Admin-Tokenatroutes_admin.go:81-85.
Step 4: Every test constructsRouter: gin.New()and immediately callsregisterPprofRoutes(routes_admin_test.go:19-24, repeated in the other cases), without callingapplyGlobalMiddlewareorsetupRoutes; thus none executes the production middleware ordering or validates the four-credential matrix.
💡 Suggested Fix
Create a small composed-router test helper that calls applyGlobalMiddleware() before registerPprofRoutes(). Cover neither/API-only/admin-only/both for non-empty API and admin tokens, plus API-key-only behavior when the admin token is empty. Assert both status and the distinct API-key versus pprof response shape where that matters.
Production Gin middleware chain for pprof routes · confidence 98%
🤖 Reviewed by AgentField PR-AF
|
|
Realigns embedded mirrors with skills/ sources inherited from the main merge so skillkit drift tests pass branch-locally. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
86d7688 to
63169b8
Compare
2 participants
Summary
/debug/pprof/*endpoints (index, cmdline, profile, symbol, trace, and named profiles) on the control-plane gin router.X-Admin-Tokenheader with constant-time comparison, returning 401 on missing or wrong tokens.Closes #428
Changes
control-plane/internal/server/server.go— Registers the new admin pprof route group.control-plane/internal/server/routes_admin.go— Adds pprof middleware (constant-time token check returning 401) and route registration.control-plane/internal/server/routes_admin_test.go— Seven test cases covering token auth and profile endpoint selection.Test plan
go vet ./...passes clean.go test ./internal/server/...passes (the pre-existingTestStartAndStopCoverAdditionalBranchesfailure is unrelated and also present on clean main)./debug/pprof/without a token returns 401; with the correct token returns the pprof index.🤖 Built with AgentField SWE-AF
🔌 Powered by AgentField