fix(router): invert ETag wrap so rbacGate stays outer — close CRIT-1 ratchet

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
This commit is contained in:
shankar0123
2026-05-14 03:32:14 +00:00
parent 1279172e9b
commit 51529ea609
+34 -14
View File
@@ -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))