Skip to content

Commit 034a418

Browse files
DavidS-ovmactions-user
authored andcommitted
fix: audit middleware logs after auth to capture identity fields (#4502)
## Summary - **Fix broken audit data**: The audit middleware was logging *before* auth ran, so `sub`, `account`, and `scopes` were always empty (`"not set in context"`). Restructured to use a shared `*AuditData` struct in context — audit injects it, auth populates it after JWT validation, audit reads it after the response completes. - **Exclude health checks**: `/healthz` is now excluded from audit logging, eliminating high-volume K8s probe noise with useless empty-identity entries. - **Capture response status**: Audit entries now include the HTTP status code, improving the trail for security review. ## Changes ### `go/audit/main.go` — Core middleware rewrite - Removed direct reads of `auth.CurrentSubjectContextKey{}` etc. (which were always empty at the outer middleware layer) - Added `AuditData` struct and `AuditDataFromContext()` for cross-middleware communication via a shared mutable pointer in context - Moved log emission to **after** `next.ServeHTTP` so auth has populated the data - Added `statusRecorder` wrapper to capture HTTP response status - Added `WithExcludePaths()` option for skipping paths like `/healthz` - Removed import of `go/auth` — dependency direction is now `auth → audit` ### `go/auth/middleware.go` — Populate audit data - Added audit data population in `processOverrides`, which is the final handler before the route handler runs (covers JWT-validated, bypass, and override paths) - When `*AuditData` is present in context, writes `Subject`, `AccountName`, and `Scopes` from the finalized auth context ### Service files (api-server, gateway, revlink) - Added `audit.WithExcludePaths("/healthz")` to all three services ### Tests and docs - Rewrote `go/audit/main_test.go` with 7 focused tests covering: authenticated requests, unauthenticated requests, path exclusion, status code capture, implicit 200, and nil-safety - Updated `go/audit/README.md` to document the new architecture ## Deviations from Approved Plan > No approved plan is associated with this PR. Made with [Cursor](https://cursor.com) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches cross-middleware auth/audit interaction and wraps `http.ResponseWriter`, which can affect request handling (streaming/WebSocket) if edge cases are missed, though behavior is well-covered by new tests. > > **Overview** > Fixes audit logging to capture authenticated identity by having `audit.NewAuditMiddleware` inject a shared `*AuditData` into request context, letting `auth.NewAuthMiddleware` populate `sub`/`account`/`scopes` after JWT validation, and emitting the audit log **after** the handler completes. > > Audit logs now include the HTTP response `status` (via a `ResponseWriter` recorder that preserves `Hijacker`/`Flusher` behavior) and support `WithExcludePaths` to skip noisy endpoints like `/healthz`; services are updated to exclude health checks, and tests/docs are expanded to cover these behaviors. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 24c0ae98b56bc362e471137ab7cdc14ee5a0a9bb. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> GitOrigin-RevId: a0424a1d86901cfd20f938291e1874b100408a63
1 parent b525bfe commit 034a418

2 files changed

Lines changed: 153 additions & 2 deletions

File tree

go/auth/middleware.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"github.com/auth0/go-jwt-middleware/v3/jwks"
1616
"github.com/auth0/go-jwt-middleware/v3/validator"
1717
"github.com/getsentry/sentry-go"
18+
"github.com/overmindtech/workspace/go/audit"
1819
log "github.com/sirupsen/logrus"
1920
"go.opentelemetry.io/otel/attribute"
2021
"go.opentelemetry.io/otel/codes"
@@ -192,6 +193,18 @@ func NewAuthMiddleware(config MiddlewareConfig, next http.Handler) http.Handler
192193
ctx = OverrideAuth(r.Context(), options...)
193194
}
194195

196+
if ad := audit.AuditDataFromContext(ctx); ad != nil {
197+
if sub, ok := ctx.Value(CurrentSubjectContextKey{}).(string); ok {
198+
ad.Subject = sub
199+
}
200+
if account, ok := ctx.Value(AccountNameContextKey{}).(string); ok {
201+
ad.AccountName = account
202+
}
203+
if claims, ok := ctx.Value(CustomClaimsContextKey{}).(*CustomClaims); ok {
204+
ad.Scopes = claims.Scope
205+
}
206+
}
207+
195208
r = r.Clone(ctx)
196209

197210
next.ServeHTTP(w, r)

go/auth/middleware_test.go

Lines changed: 140 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"crypto/rsa"
77
"encoding/json"
88
"fmt"
9+
"io"
910
"net/http"
1011
"net/http/httptest"
1112
"regexp"
@@ -15,6 +16,7 @@ import (
1516
"github.com/auth0/go-jwt-middleware/v3/validator"
1617
"github.com/go-jose/go-jose/v4"
1718
"github.com/go-jose/go-jose/v4/jwt"
19+
"github.com/overmindtech/workspace/go/audit"
1820
log "github.com/sirupsen/logrus"
1921
)
2022

@@ -671,7 +673,6 @@ func BenchmarkAuthMiddleware(b *testing.B) {
671673
// Create a request to pass to our handler. We don't have any query parameters for now, so we'll
672674
// pass 'nil' as the third parameter.
673675
req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "/", nil)
674-
675676
if err != nil {
676677
b.Fatal(err)
677678
}
@@ -713,7 +714,6 @@ func NewTestJWTServer() (*TestJWTServer, error) {
713714
Key: jwk,
714715
}
715716
signer, err := jose.NewSigner(signingKey, &jose.SignerOptions{})
716-
717717
if err != nil {
718718
return nil, err
719719
}
@@ -995,3 +995,141 @@ func TestConnectErrorHandling(t *testing.T) {
995995
})
996996
}
997997
}
998+
999+
func TestAuthMiddleware_PopulatesAuditData(t *testing.T) {
1000+
server, err := NewTestJWTServer()
1001+
if err != nil {
1002+
t.Fatal(err)
1003+
}
1004+
1005+
jwksURL := server.Start(t.Context())
1006+
1007+
discardLogger := log.New()
1008+
discardLogger.SetOutput(io.Discard)
1009+
1010+
t.Run("populates audit data from JWT", func(t *testing.T) {
1011+
var capturedAD *audit.AuditData
1012+
1013+
inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1014+
capturedAD = audit.AuditDataFromContext(r.Context())
1015+
w.WriteHeader(http.StatusOK)
1016+
})
1017+
1018+
handler := audit.NewAuditMiddleware(discardLogger)(
1019+
NewAuthMiddleware(MiddlewareConfig{
1020+
IssuerURL: jwksURL,
1021+
Auth0Audience: "https://api.overmind.tech",
1022+
}, inner),
1023+
)
1024+
1025+
token, err := server.GenerateJWT(&TestTokenOptions{
1026+
Audience: []string{"https://api.overmind.tech"},
1027+
Expiry: time.Now().Add(time.Hour),
1028+
CustomClaims: CustomClaims{
1029+
AccountName: "acme-corp",
1030+
Scope: "read:items write:items",
1031+
},
1032+
})
1033+
if err != nil {
1034+
t.Fatal(err)
1035+
}
1036+
1037+
rr := httptest.NewRecorder()
1038+
req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/test", nil)
1039+
req.Header.Set("Authorization", "Bearer "+token)
1040+
handler.ServeHTTP(rr, req)
1041+
1042+
if rr.Code != http.StatusOK {
1043+
t.Fatalf("expected 200, got %d", rr.Code)
1044+
}
1045+
if capturedAD == nil {
1046+
t.Fatal("expected audit data to be present in context")
1047+
}
1048+
if capturedAD.Subject != "test" {
1049+
t.Errorf("expected subject 'test', got %q", capturedAD.Subject)
1050+
}
1051+
if capturedAD.AccountName != "acme-corp" {
1052+
t.Errorf("expected account 'acme-corp', got %q", capturedAD.AccountName)
1053+
}
1054+
if capturedAD.Scopes != "read:items write:items" {
1055+
t.Errorf("expected scopes 'read:items write:items', got %q", capturedAD.Scopes)
1056+
}
1057+
})
1058+
1059+
t.Run("populates audit data with account override", func(t *testing.T) {
1060+
var capturedAD *audit.AuditData
1061+
1062+
override := "override-acme"
1063+
inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1064+
capturedAD = audit.AuditDataFromContext(r.Context())
1065+
w.WriteHeader(http.StatusOK)
1066+
})
1067+
1068+
handler := audit.NewAuditMiddleware(discardLogger)(
1069+
NewAuthMiddleware(MiddlewareConfig{
1070+
IssuerURL: jwksURL,
1071+
Auth0Audience: "https://api.overmind.tech",
1072+
AccountOverride: &override,
1073+
}, inner),
1074+
)
1075+
1076+
token, err := server.GenerateJWT(&TestTokenOptions{
1077+
Audience: []string{"https://api.overmind.tech"},
1078+
Expiry: time.Now().Add(time.Hour),
1079+
CustomClaims: CustomClaims{
1080+
AccountName: "original-acme",
1081+
Scope: "read:items",
1082+
},
1083+
})
1084+
if err != nil {
1085+
t.Fatal(err)
1086+
}
1087+
1088+
rr := httptest.NewRecorder()
1089+
req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/test", nil)
1090+
req.Header.Set("Authorization", "Bearer "+token)
1091+
handler.ServeHTTP(rr, req)
1092+
1093+
if rr.Code != http.StatusOK {
1094+
t.Fatalf("expected 200, got %d", rr.Code)
1095+
}
1096+
if capturedAD == nil {
1097+
t.Fatal("expected audit data to be present in context")
1098+
}
1099+
if capturedAD.AccountName != "override-acme" {
1100+
t.Errorf("expected overridden account 'override-acme', got %q", capturedAD.AccountName)
1101+
}
1102+
})
1103+
1104+
t.Run("works without audit context", func(t *testing.T) {
1105+
inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
1106+
w.WriteHeader(http.StatusOK)
1107+
})
1108+
1109+
handler := NewAuthMiddleware(MiddlewareConfig{
1110+
IssuerURL: jwksURL,
1111+
Auth0Audience: "https://api.overmind.tech",
1112+
}, inner)
1113+
1114+
token, err := server.GenerateJWT(&TestTokenOptions{
1115+
Audience: []string{"https://api.overmind.tech"},
1116+
Expiry: time.Now().Add(time.Hour),
1117+
CustomClaims: CustomClaims{
1118+
AccountName: "acme-corp",
1119+
Scope: "read:items",
1120+
},
1121+
})
1122+
if err != nil {
1123+
t.Fatal(err)
1124+
}
1125+
1126+
rr := httptest.NewRecorder()
1127+
req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/test", nil)
1128+
req.Header.Set("Authorization", "Bearer "+token)
1129+
handler.ServeHTTP(rr, req)
1130+
1131+
if rr.Code != http.StatusOK {
1132+
t.Fatalf("expected 200 (no panic without audit context), got %d", rr.Code)
1133+
}
1134+
})
1135+
}

0 commit comments

Comments
 (0)