From 51529ea60998d6820f9784c6bfe90320160ec069 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Thu, 14 May 2026 03:32:14 +0000 Subject: [PATCH] =?UTF-8?q?fix(router):=20invert=20ETag=20wrap=20so=20rbac?= =?UTF-8?q?Gate=20stays=20outer=20=E2=80=94=20close=20CRIT-1=20ratchet?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI run on master@0ad881c2 failed TestRouterRBACGateCoverage on five routes: GET /api/v1/agents GET /api/v1/audit GET /api/v1/certificates GET /api/v1/discovered-certificates GET /api/v1/jobs These are the five top-5 read endpoints that Phase 6 SCALE-L2 (commit 8191b1ee) wrapped with the new etagged() helper. The existing rbacGate wrap was preserved INSIDE the etagged() call: r.Register("GET /api/v1/certificates", etagged(rbacGate(reg.Checker, "cert.read", reg.Certificates.ListCertificates))) Functionally this is safe (the rbacGate still runs at request time; the ETag middleware emits ETag only on 2xx, so 401s/403s never get cached), but it FAILS the AST-based RBAC coverage test introduced by the 2026-05-10 auth-bundle audit (CRIT-1). That test walks router.go's `r.Register(route, handler)` calls and asserts the second argument is either `rbacGate(...)` or `rbacGateScoped(...)` or that the route is in `authExemptRoutes` / matches a `protocolPrefixes` entry. With `etagged()` as the outer wrap, the test's AST inspection sees `etagged(...)` and counts the route as ungated. CRIT-1's standing rule (test header): "Removing an existing rbacGate wrap requires either (a) moving the route to authExemptRoutes here, or (b) demonstrating the new approach in the commit body." Phase 6 did neither — the rbacGate wrap was demoted from outer to inner without an authExemptRoutes entry and without the test being taught about the new shape. This is exactly the regression the CRIT-1 ratchet is designed to catch. Root cause: rbacGate's signature is func rbacGate(checker, perm string, h http.HandlerFunc) http.Handler and etagged's signature was func etagged(h http.Handler) http.Handler so etagged COULD wrap rbacGate but rbacGate could NOT wrap etagged (the third arg type didn't match). Phase 6 took the type-easy path; this hotfix takes the security-correct path. Fix ==== Rename `etagged()` → `etaggedFunc()` and change its signature to `http.HandlerFunc → http.HandlerFunc` so it can be used INSIDE the rbacGate call: r.Register("GET /api/v1/certificates", rbacGate(reg.Checker, "cert.read", etaggedFunc(reg.Certificates.ListCertificates))) New runtime order: request → rbacGate → etaggedFunc → handler Unauthenticated requests now bounce at HTTP 403 BEFORE the response-buffering ETag middleware ever runs. The SHA-256-over-body cost only applies to authenticated 2xx responses — also a small perf win on top of fixing the lint. The internal implementation reduces to: func etaggedFunc(h http.HandlerFunc) http.HandlerFunc { return middleware.ETag(h).ServeHTTP } middleware.ETag itself is unchanged. The five call sites swap wrap order; everything else stays identical. Pattern lesson ============== golangci-lint and staticcheck check different layers; the AST-based TestRouterRBACGateCoverage is ANOTHER layer (a Go test, not a linter) that the local `go test ./internal/api/router/...` step would have caught. Phase 6's pre-commit verification ran `go test ./internal/scheduler/ ./internal/api/middleware/` explicitly but missed `./internal/api/router/` — which is where this test lives. Future commits that touch router.go MUST run `go test ./internal/api/router/... -count=1` before push. Adding this to the standing pre-commit rule alongside the "`golangci-lint run` AND `staticcheck` BOTH must pass" rule from the previous hotfix. Verification: go build ./internal/api/router/... → ok go test ./internal/api/router/... -count=1 -short → ok (TestRouterRBACGateCoverage passes) go test ./internal/api/router/... \ ./internal/api/middleware/... -count=1 -short → ok (router + ETag tests both green) staticcheck ./internal/api/router/... \ ./internal/api/middleware/... → clean gofmt -l internal/api/router/router.go → clean Closes: CI failure run on master@0ad881c2 — TestRouterRBACGateCoverage --- internal/api/router/router.go | 48 +++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 14 deletions(-) diff --git a/internal/api/router/router.go b/internal/api/router/router.go index 5de3b35..464cd89 100644 --- a/internal/api/router/router.go +++ b/internal/api/router/router.go @@ -11,21 +11,41 @@ import ( "github.com/certctl-io/certctl/internal/auth" ) -// etagged wraps a list-endpoint handler with the SCALE-L2 ETag +// etaggedFunc wraps a list-endpoint handler with the SCALE-L2 ETag // middleware. Phase 6 SCALE-L2 closure (2026-05-14): the top-5 // read-heavy list endpoints (/certificates, /jobs, /agents, // /audit, /discovered-certificates) get ETag + If-None-Match // short-circuit to avoid re-running their SELECT COUNT(*) + -// row-marshaling pass on every dashboard poll. The helper chains -// ETag around an already-rbac-gated handler so order is: +// row-marshaling pass on every dashboard poll. // -// request → ETag → rbacGate → handler +// Call-site shape (rbacGate is OUTER, etaggedFunc is INNER): // -// (auth runs BEFORE the cache check; we never short-circuit a -// 304 to an unauthenticated request. The middleware emits ETag -// only on 2xx responses, so 401s/403s never get cached.) -func etagged(h http.Handler) http.Handler { - return middleware.ETag(h) +// r.Register(route, rbacGate(checker, "perm", etaggedFunc(handler))) +// +// Wrap order at request time: +// +// request → rbacGate → etaggedFunc → handler +// +// Auth runs FIRST. Unauthenticated requests bounce at HTTP 403 +// before the response-buffering ETag middleware ever runs, so the +// SHA-256-over-body cost only applies to authenticated 2xx +// responses. This shape is also what TestRouterRBACGateCoverage +// asserts (the AST CI guard introduced for 2026-05-10 audit CRIT-1 +// requires rbacGate / rbacGateScoped to be the OUTER wrap on every +// state-changing or read endpoint). +// +// Phase 6's initial commit shipped the OPPOSITE order +// (etagged(rbacGate(handler))) — functionally safe because the ETag +// middleware emits ETag only on 2xx responses, but it failed the +// AST coverage test. Phase 8 hotfix (commit see git log --grep=U1000 +// follow-on) inverted the wrap so rbacGate is the outer call. +// +// The signature is http.HandlerFunc → http.HandlerFunc (not the +// http.Handler form) because rbacGate expects http.HandlerFunc as +// its third arg; nesting an http.Handler-returning helper inside it +// would type-fail. +func etaggedFunc(h http.HandlerFunc) http.HandlerFunc { + return middleware.ETag(h).ServeHTTP } // rbacGate wraps a handler with auth.RequirePermission(checker, perm, @@ -584,7 +604,7 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) { r.Register("POST /api/v1/est/certificates/bulk-revoke", rbacGate(reg.Checker, "cert.bulk_revoke", reg.BulkRevocation.BulkRevokeEST)) r.Register("POST /api/v1/certificates/bulk-renew", rbacGate(reg.Checker, "cert.issue", reg.BulkRenewal.BulkRenew)) r.Register("POST /api/v1/certificates/bulk-reassign", rbacGate(reg.Checker, "cert.edit", reg.BulkReassignment.BulkReassign)) - r.Register("GET /api/v1/certificates", etagged(rbacGate(reg.Checker, "cert.read", reg.Certificates.ListCertificates))) + r.Register("GET /api/v1/certificates", rbacGate(reg.Checker, "cert.read", etaggedFunc(reg.Certificates.ListCertificates))) r.Register("POST /api/v1/certificates", rbacGate(reg.Checker, "cert.issue", reg.Certificates.CreateCertificate)) r.Register("GET /api/v1/certificates/{id}", rbacGate(reg.Checker, "cert.read", reg.Certificates.GetCertificate)) r.Register("PUT /api/v1/certificates/{id}", rbacGate(reg.Checker, "cert.edit", reg.Certificates.UpdateCertificate)) @@ -636,7 +656,7 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) { // * DELETE /api/v1/agents/{id} — RetireAgent. Replaces the pre-I-004 // hard-delete; the underlying repo does a soft-retire with // optional cascade. - r.Register("GET /api/v1/agents", etagged(rbacGate(reg.Checker, "agent.read", reg.Agents.ListAgents))) + r.Register("GET /api/v1/agents", rbacGate(reg.Checker, "agent.read", etaggedFunc(reg.Agents.ListAgents))) r.Register("POST /api/v1/agents", rbacGate(reg.Checker, "agent.edit", reg.Agents.RegisterAgent)) r.Register("GET /api/v1/agents/retired", rbacGate(reg.Checker, "agent.read", reg.Agents.ListRetiredAgents)) r.Register("GET /api/v1/agents/{id}", rbacGate(reg.Checker, "agent.read", reg.Agents.GetAgent)) @@ -648,7 +668,7 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) { r.Register("POST /api/v1/agents/{id}/jobs/{job_id}/status", rbacGate(reg.Checker, "agent.job.complete", reg.Agents.AgentReportJobStatus)) // Jobs routes: /api/v1/jobs - r.Register("GET /api/v1/jobs", etagged(rbacGate(reg.Checker, "job.read", reg.Jobs.ListJobs))) + r.Register("GET /api/v1/jobs", rbacGate(reg.Checker, "job.read", etaggedFunc(reg.Jobs.ListJobs))) r.Register("GET /api/v1/jobs/{id}", rbacGate(reg.Checker, "job.read", reg.Jobs.GetJob)) r.Register("POST /api/v1/jobs/{id}/cancel", rbacGate(reg.Checker, "job.cancel", reg.Jobs.CancelJob)) r.Register("POST /api/v1/jobs/{id}/approve", rbacGate(reg.Checker, "approval.approve", reg.Jobs.ApproveJob)) @@ -712,7 +732,7 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) { r.Register("GET /api/v1/agent-groups/{id}/members", rbacGate(reg.Checker, "agent.read", reg.AgentGroups.ListAgentGroupMembers)) // Audit routes: /api/v1/audit - r.Register("GET /api/v1/audit", etagged(rbacGate(reg.Checker, "audit.read", reg.Audit.ListAuditEvents))) + r.Register("GET /api/v1/audit", rbacGate(reg.Checker, "audit.read", etaggedFunc(reg.Audit.ListAuditEvents))) // Audit 2026-05-10 HIGH-11 closure — `audit.export` permission was // already seeded into r-admin + r-auditor (migration 000031), but // no endpoint enforced it pre-fix; r-auditor's claim was misleading @@ -782,7 +802,7 @@ func (r *Router) RegisterHandlers(reg HandlerRegistry) { // Discovery routes: /api/v1/discovered-certificates, /api/v1/discovery-scans r.Register("POST /api/v1/agents/{id}/discoveries", rbacGate(reg.Checker, "discovery.run", reg.Discovery.SubmitDiscoveryReport)) - r.Register("GET /api/v1/discovered-certificates", etagged(rbacGate(reg.Checker, "discovery.read", reg.Discovery.ListDiscovered))) + r.Register("GET /api/v1/discovered-certificates", rbacGate(reg.Checker, "discovery.read", etaggedFunc(reg.Discovery.ListDiscovered))) r.Register("GET /api/v1/discovered-certificates/{id}", rbacGate(reg.Checker, "discovery.read", reg.Discovery.GetDiscovered)) r.Register("POST /api/v1/discovered-certificates/{id}/claim", rbacGate(reg.Checker, "discovery.claim", reg.Discovery.ClaimDiscovered)) r.Register("POST /api/v1/discovered-certificates/{id}/dismiss", rbacGate(reg.Checker, "discovery.claim", reg.Discovery.DismissDiscovered))