From 8de58ebc9eecbb1251618ac89b25c315dcc9f4a8 Mon Sep 17 00:00:00 2001 From: SyniRon <66834451+SyniRon@users.noreply.github.com> Date: Sat, 30 May 2026 23:09:36 -0400 Subject: [PATCH] fix(auth): two-tier 401 distinguishing bad Bearer scheme from key failure HTTP authMiddleware and gRPC unary interceptor collapsed both the bad/missing Bearer scheme case and the key-validation failure case into a bare Unauthorized / codes.Unauthenticated. Callers pasting a raw cav7_ key without the "Bearer " prefix got a generic 401 with no hint. Split into two tiers on both surfaces: - Bearer-scheme problem (ParseBearerToken returns empty): name the expected format. HTTP body "Unauthorized: expected 'Authorization: Bearer ' header"; gRPC codes.Unauthenticated with mirrored message. gRPC unifies the missing-metadata / missing-header / empty-token branches here. - Key-validation failure (token present, ValidateApiKey nil/err): keep the generic message, leaking nothing about key existence/expiry/scopes. ParseBearerToken unchanged (empty-return contract intact). Adds gateway auth_test.go (no prior coverage) and grpc interceptor cases exercising both 401 branches on both surfaces. Closes #99 Co-Authored-By: Claude Opus 4.8 (1M context) --- servers/gateway/auth_test.go | 111 +++++++++++++++++++++++++++++++++ servers/gateway/gateway.go | 12 +++- servers/grpc/auth_test.go | 77 +++++++++++++++++++++++ servers/grpc/authentication.go | 15 +++-- 4 files changed, 209 insertions(+), 6 deletions(-) create mode 100644 servers/gateway/auth_test.go diff --git a/servers/gateway/auth_test.go b/servers/gateway/auth_test.go new file mode 100644 index 0000000..ddefc2b --- /dev/null +++ b/servers/gateway/auth_test.go @@ -0,0 +1,111 @@ +package gateway + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/7cav/api/datastores" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeAuthDatastore embeds the Datastore interface so it satisfies the type +// without implementing every method; only ValidateApiKey is exercised by +// authMiddleware. Any other call panics (nil method) — a loud failure if a +// test accidentally reaches further into the datastore. +type fakeAuthDatastore struct { + datastores.Datastore + validateApiKey func(string) (*datastores.ApiKeyResult, error) +} + +func (f *fakeAuthDatastore) ValidateApiKey(rawKey string) (*datastores.ApiKeyResult, error) { + return f.validateApiKey(rawKey) +} + +// callMiddleware runs authMiddleware in front of a handler that records whether +// it was reached, and returns the recorded response plus the next-called flag. +func callMiddleware(t *testing.T, ds datastores.Datastore, authHeader string) (*httptest.ResponseRecorder, bool) { + t.Helper() + nextCalled := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + nextCalled = true + w.WriteHeader(http.StatusOK) + }) + h := authMiddleware(ds, next) + + req := httptest.NewRequest(http.MethodGet, "/api/v1/whatever", nil) + if authHeader != "" { + req.Header.Set("Authorization", authHeader) + } + rr := httptest.NewRecorder() + h.ServeHTTP(rr, req) + return rr, nextCalled +} + +func TestAuthMiddleware_NoAuthHeader_NamesBearerScheme(t *testing.T) { + ds := &fakeAuthDatastore{validateApiKey: func(string) (*datastores.ApiKeyResult, error) { + t.Fatal("ValidateApiKey must not be called when no bearer token is present") + return nil, nil + }} + rr, nextCalled := callMiddleware(t, ds, "") + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.False(t, nextCalled) + body := rr.Body.String() + assert.Contains(t, body, "Bearer") + assert.Contains(t, body, "Authorization") +} + +func TestAuthMiddleware_RawKeyNoBearerPrefix_NamesBearerScheme(t *testing.T) { + ds := &fakeAuthDatastore{validateApiKey: func(string) (*datastores.ApiKeyResult, error) { + t.Fatal("ValidateApiKey must not be called when the Bearer scheme is absent") + return nil, nil + }} + rr, nextCalled := callMiddleware(t, ds, "cav7_rawkeywithoutprefix") + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.False(t, nextCalled) + body := rr.Body.String() + assert.Contains(t, body, "Bearer") + assert.Contains(t, body, "Authorization") +} + +func TestAuthMiddleware_BadKey_GenericUnauthorizedNoLeak(t *testing.T) { + ds := &fakeAuthDatastore{validateApiKey: func(token string) (*datastores.ApiKeyResult, error) { + assert.Equal(t, "cav7_badkey", token) + return nil, nil // zero rows → nil result, no error + }} + rr, nextCalled := callMiddleware(t, ds, "Bearer cav7_badkey") + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.False(t, nextCalled) + body := strings.TrimSpace(rr.Body.String()) + // Generic — must NOT name the Bearer scheme (that branch is for scheme errors) + // and must not leak anything about the key. + assert.Equal(t, "Unauthorized", body) + assert.NotContains(t, body, "Bearer") +} + +func TestAuthMiddleware_ValidKey_CallsNext(t *testing.T) { + ds := &fakeAuthDatastore{validateApiKey: func(string) (*datastores.ApiKeyResult, error) { + return &datastores.ApiKeyResult{KeyId: 1, UserId: 2}, nil + }} + rr, nextCalled := callMiddleware(t, ds, "Bearer cav7_goodkey") + + require.True(t, nextCalled) + assert.Equal(t, http.StatusOK, rr.Code) +} + +func TestAuthMiddleware_ValidateError_GenericUnauthorized(t *testing.T) { + ds := &fakeAuthDatastore{validateApiKey: func(string) (*datastores.ApiKeyResult, error) { + return nil, io.ErrUnexpectedEOF + }} + rr, nextCalled := callMiddleware(t, ds, "Bearer cav7_anykey") + + assert.Equal(t, http.StatusUnauthorized, rr.Code) + assert.False(t, nextCalled) + assert.Equal(t, "Unauthorized", strings.TrimSpace(rr.Body.String())) +} diff --git a/servers/gateway/gateway.go b/servers/gateway/gateway.go index 857c86c..f3f74e9 100644 --- a/servers/gateway/gateway.go +++ b/servers/gateway/gateway.go @@ -66,12 +66,20 @@ func getOpenAPIHandler() http.Handler { // cav7_ prefix (5) + 64 hex chars = 69; 128 gives generous headroom. const maxTokenLen = 128 +// errBearerScheme is the 401 body returned when the Authorization header is +// missing or doesn't carry a usable Bearer token (no/empty/oversized token). +// It names the expected format so callers who paste a raw key without the +// "Bearer " prefix get a self-explanatory error. The key-validation-failure +// branch stays the generic "Unauthorized" so it leaks nothing about whether a +// key exists, is expired, or lacks scopes. +const errBearerScheme = "Unauthorized: expected 'Authorization: Bearer ' header" + func authMiddleware(ds datastores.Datastore, next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { token := datastores.ParseBearerToken(r.Header.Get("Authorization"), maxTokenLen) if token == "" { - Warn.Printf("Unauthorized HTTP access attempt from %s", r.RemoteAddr) - http.Error(w, "Unauthorized", http.StatusUnauthorized) + Warn.Printf("Unauthorized HTTP access attempt (bad bearer scheme) from %s", r.RemoteAddr) + http.Error(w, errBearerScheme, http.StatusUnauthorized) return } diff --git a/servers/grpc/auth_test.go b/servers/grpc/auth_test.go index 84d2e7f..c9164ba 100644 --- a/servers/grpc/auth_test.go +++ b/servers/grpc/auth_test.go @@ -108,6 +108,83 @@ func TestAuthInterceptor_LogsRequestOnSuccess(t *testing.T) { assert.Contains(t, logged, "key_id=17") } +// rawAuthCtx sets the authorization metadata verbatim (no implicit "Bearer " +// prefix), so scheme-problem cases can be exercised. +func rawAuthCtx(authHeader, peerIP string) context.Context { + var ctx context.Context + if authHeader == "" { + ctx = context.Background() + } else { + ctx = metadata.NewIncomingContext( + context.Background(), + metadata.Pairs("authorization", authHeader), + ) + } + return peer.NewContext(ctx, &peer.Peer{ + Addr: &net.TCPAddr{IP: net.ParseIP(peerIP), Port: 4242}, + }) +} + +func runInterceptor(t *testing.T, ds *fakeDatastore, ctx context.Context) (any, error, bool) { + t.Helper() + interceptor := NewAuthInterceptor(ds) + info := &grpc.UnaryServerInfo{FullMethod: "/proto.MilpacService/GetProfile"} + handlerCalled := false + resp, err := interceptor(ctx, "req", info, func(ctx context.Context, req any) (any, error) { + handlerCalled = true + return "ok", nil + }) + return resp, err, handlerCalled +} + +func TestAuthInterceptor_NoAuthHeader_NamesBearerScheme(t *testing.T) { + ds := &fakeDatastore{validateApiKey: func(string) (*datastores.ApiKeyResult, error) { + t.Fatal("ValidateApiKey must not be called when no authorization metadata is present") + return nil, nil + }} + _, err, called := runInterceptor(t, ds, rawAuthCtx("", "10.0.0.5")) + + require.Error(t, err) + assert.False(t, called) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.Unauthenticated, st.Code()) + assert.Contains(t, st.Message(), "Bearer") + assert.Contains(t, st.Message(), "Authorization") +} + +func TestAuthInterceptor_RawKeyNoBearerPrefix_NamesBearerScheme(t *testing.T) { + ds := &fakeDatastore{validateApiKey: func(string) (*datastores.ApiKeyResult, error) { + t.Fatal("ValidateApiKey must not be called when the Bearer scheme is absent") + return nil, nil + }} + _, err, called := runInterceptor(t, ds, rawAuthCtx("cav7_rawkeynoprefix", "10.0.0.5")) + + require.Error(t, err) + assert.False(t, called) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.Unauthenticated, st.Code()) + assert.Contains(t, st.Message(), "Bearer") + assert.Contains(t, st.Message(), "Authorization") +} + +func TestAuthInterceptor_BadKey_GenericNoLeak(t *testing.T) { + ds := &fakeDatastore{validateApiKey: func(token string) (*datastores.ApiKeyResult, error) { + assert.Equal(t, "cav7_badkey", token) + return nil, nil + }} + _, err, called := runInterceptor(t, ds, buildAuthCtx("cav7_badkey", "10.0.0.5")) + + require.Error(t, err) + assert.False(t, called) + st, ok := status.FromError(err) + require.True(t, ok) + assert.Equal(t, codes.Unauthenticated, st.Code()) + // Generic — must not name the Bearer scheme (that's reserved for scheme errors). + assert.NotContains(t, st.Message(), "Bearer") +} + func TestAuthInterceptor_LogsRequestOnAuthFailure(t *testing.T) { ds := &fakeDatastore{ validateApiKey: func(token string) (*datastores.ApiKeyResult, error) { diff --git a/servers/grpc/authentication.go b/servers/grpc/authentication.go index fd32cd0..69b6284 100644 --- a/servers/grpc/authentication.go +++ b/servers/grpc/authentication.go @@ -32,6 +32,13 @@ import ( const maxTokenLen = 128 +// errBearerScheme mirrors the HTTP gateway's scheme-problem message: it names +// the expected Authorization format so callers missing the "Bearer " prefix +// get a self-explanatory error. Returned for any Bearer-scheme problem (no +// metadata, no authorization header, or an empty/oversized token). The +// key-validation-failure branch stays generic to leak nothing about the key. +const errBearerScheme = "Unauthenticated: expected 'Authorization: Bearer ' metadata" + func NewAuthInterceptor(ds datastores.Datastore) grpc.UnaryServerInterceptor { return func(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (any, error) { peerAddr := "unknown" @@ -45,22 +52,22 @@ func NewAuthInterceptor(ds datastores.Datastore) grpc.UnaryServerInterceptor { md, ok := metadata.FromIncomingContext(ctx) if !ok { - return nil, status.Errorf(codes.Unauthenticated, "missing metadata") + return nil, status.Error(codes.Unauthenticated, errBearerScheme) } authHeaders := md.Get("authorization") if len(authHeaders) < 1 { - return nil, status.Errorf(codes.Unauthenticated, "missing authorization token") + return nil, status.Error(codes.Unauthenticated, errBearerScheme) } token := datastores.ParseBearerToken(authHeaders[0], maxTokenLen) if token == "" { - return nil, status.Errorf(codes.Unauthenticated, "missing authorization token") + return nil, status.Error(codes.Unauthenticated, errBearerScheme) } key, err := ds.ValidateApiKey(token) if err != nil || key == nil { - return nil, status.Errorf(codes.Unauthenticated, "invalid api key") + return nil, status.Error(codes.Unauthenticated, "invalid api key") } keyID = strconv.FormatUint(uint64(key.KeyId), 10)