mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 18:11:32 +00:00
shankar0123
51529ea609
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