From 36f397e768e22daf1679e9653b8896a8c9c45a3d Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Mon, 1 Jun 2026 15:51:37 -0300 Subject: [PATCH] fix(api): block API keys on account-level routes API keys are namespace-scoped tokens and should not be usable on account-level routes. Four routes were missing the BlockAPIKey middleware, allowing API keys to reach handlers they should never reach. The most concrete gap was ListAPIKeys: an API key could enumerate all other API keys in the namespace. The remaining three routes (GetNamespaceList, CreateNamespace, LeaveNamespace) were safe by accident -- the service layer rejected them -- but returned confusing 404s instead of a clean 403. Added BlockAPIKey to all four routes, matching the pattern already used by CreateAPIKey, UpdateAPIKey and DeleteAPIKey. Removed the x-internal overrides and api-key security entries from the OpenAPI spec for the same operations, since the spec now accurately reflects what the code enforces. Removed TestListNamespacesAPIKeyScope, which tested the old scoped behavior that no longer applies. --- api/routes/api-key_test.go | 15 ++++++ api/routes/nsadm_test.go | 46 ++++++++++++------- api/routes/routes.go | 8 ++-- openapi/spec/paths/api@namespaces.yaml | 4 -- .../spec/paths/api@namespaces@api-key.yaml | 2 - .../api@namespaces@{tenant}@members.yaml | 2 - 6 files changed, 48 insertions(+), 29 deletions(-) diff --git a/api/routes/api-key_test.go b/api/routes/api-key_test.go index 435d718edce..752e2e74aac 100644 --- a/api/routes/api-key_test.go +++ b/api/routes/api-key_test.go @@ -288,6 +288,21 @@ func TestListAPIKey(t *testing.T) { requiredMocks func() expected Expected }{ + { + description: "fails with api key", + headers: map[string]string{ + "Content-Type": "application/json", + "X-API-KEY": "b2f7cc0e-d933-4aad-9ab2-b557f2f2554f", + "X-Tenant-ID": "00000000-0000-4000-0000-000000000000", + "X-Role": "owner", + }, + query: func() string { + return "" + }, + requiredMocks: func() { + }, + expected: Expected{body: nil, status: http.StatusForbidden}, + }, { description: "success", headers: map[string]string{ diff --git a/api/routes/nsadm_test.go b/api/routes/nsadm_test.go index 962ad361270..e361b94f371 100644 --- a/api/routes/nsadm_test.go +++ b/api/routes/nsadm_test.go @@ -513,6 +513,16 @@ func TestHandler_LeaveNamespace(t *testing.T) { requiredMocks func() expected int }{ + { + description: "fails with api key", + tenantID: "00000000-0000-4000-0000-000000000000", + headers: map[string]string{ + "X-API-KEY": "b2f7cc0e-d933-4aad-9ab2-b557f2f2554f", + "X-Tenant-ID": "00000000-0000-4000-0000-000000000000", + }, + requiredMocks: func() {}, + expected: http.StatusForbidden, + }, { description: "fails to leave the namespace", tenantID: "00000000-0000-4000-0000-000000000000", @@ -673,29 +683,31 @@ func TestNamespaceCrossTenantAccess(t *testing.T) { } } -// TestListNamespacesAPIKeyScope ensures a caller without a user ID (API key -// path) cannot enumerate namespaces across tenants: the list is scoped to the -// caller's own tenant. Covers the regression described in GHSA-vwx9-7qcf-gg7f. -func TestListNamespacesAPIKeyScope(t *testing.T) { - const callerTenant = "00000000-0000-4000-0000-000000000000" - +func TestGetNamespaceListBlocksAPIKey(t *testing.T) { mock := new(mocks.Service) - scopedNS := &models.Namespace{TenantID: callerTenant, Name: "dev"} - mock. - On("ListNamespaces", gomock.Anything, gomock.MatchedBy(func(req *requests.NamespaceList) bool { - return req.UserID == "" && req.TenantID == callerTenant && !req.IsAdmin - })). - Return([]models.Namespace{*scopedNS}, 1, nil). - Once() req := httptest.NewRequest(http.MethodGet, "/api/namespaces", nil) - req.Header.Set("X-Role", authorizer.RoleOwner.String()) - req.Header.Set("X-Tenant-ID", callerTenant) - // No X-ID set: this simulates an API key caller. + req.Header.Set("X-API-KEY", "b2f7cc0e-d933-4aad-9ab2-b557f2f2554f") + req.Header.Set("X-Tenant-ID", "00000000-0000-4000-0000-000000000000") + + rec := httptest.NewRecorder() + NewRouter(mock).ServeHTTP(rec, req) + + assert.Equal(t, http.StatusForbidden, rec.Result().StatusCode) + mock.AssertExpectations(t) +} + +func TestCreateNamespaceBlocksAPIKey(t *testing.T) { + mock := new(mocks.Service) + + req := httptest.NewRequest(http.MethodPost, "/api/namespaces", strings.NewReader(`{}`)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-API-KEY", "b2f7cc0e-d933-4aad-9ab2-b557f2f2554f") + req.Header.Set("X-Tenant-ID", "00000000-0000-4000-0000-000000000000") rec := httptest.NewRecorder() NewRouter(mock).ServeHTTP(rec, req) - assert.Equal(t, http.StatusOK, rec.Result().StatusCode) + assert.Equal(t, http.StatusForbidden, rec.Result().StatusCode) mock.AssertExpectations(t) } diff --git a/api/routes/routes.go b/api/routes/routes.go index c32fc734599..4c4cc374c13 100644 --- a/api/routes/routes.go +++ b/api/routes/routes.go @@ -133,7 +133,7 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { publicAPI.POST(AuthPublicKeyURL, gateway.Handler(handler.AuthPublicKey)) publicAPI.POST(CreateAPIKeyURL, gateway.Handler(handler.CreateAPIKey), routesmiddleware.BlockAPIKey, routesmiddleware.RequiresPermission(authorizer.APIKeyCreate)) - publicAPI.GET(ListAPIKeysURL, gateway.Handler(handler.ListAPIKeys)) + publicAPI.GET(ListAPIKeysURL, gateway.Handler(handler.ListAPIKeys), routesmiddleware.BlockAPIKey) publicAPI.PATCH(UpdateAPIKeyURL, gateway.Handler(handler.UpdateAPIKey), routesmiddleware.BlockAPIKey, routesmiddleware.RequiresPermission(authorizer.APIKeyUpdate)) publicAPI.DELETE(DeleteAPIKeyURL, gateway.Handler(handler.DeleteAPIKey), routesmiddleware.BlockAPIKey, routesmiddleware.RequiresPermission(authorizer.APIKeyDelete)) @@ -178,16 +178,16 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { publicAPI.PUT(UpdatePublicKeyURL, gateway.Handler(handler.UpdatePublicKey), routesmiddleware.DecodeParam(ParamPublicKeyFingerprint), routesmiddleware.RequiresPermission(authorizer.PublicKeyEdit)) publicAPI.DELETE(DeletePublicKeyURL, gateway.Handler(handler.DeletePublicKey), routesmiddleware.DecodeParam(ParamPublicKeyFingerprint), routesmiddleware.RequiresPermission(authorizer.PublicKeyRemove)) - publicAPI.POST(CreateNamespaceURL, gateway.Handler(handler.CreateNamespace)) + publicAPI.POST(CreateNamespaceURL, gateway.Handler(handler.CreateNamespace), routesmiddleware.BlockAPIKey) publicAPI.GET(GetNamespaceURL, gateway.Handler(handler.GetNamespace), routesmiddleware.RequiresTenant(ParamNamespaceTenant)) - publicAPI.GET(ListNamespaceURL, gateway.Handler(handler.GetNamespaceList)) + publicAPI.GET(ListNamespaceURL, gateway.Handler(handler.GetNamespaceList), routesmiddleware.BlockAPIKey) publicAPI.PUT(EditNamespaceURL, gateway.Handler(handler.EditNamespace), routesmiddleware.RequiresTenant(ParamNamespaceTenant), routesmiddleware.RequiresPermission(authorizer.NamespaceUpdate)) publicAPI.DELETE(DeleteNamespaceURL, gateway.Handler(handler.DeleteNamespace), routesmiddleware.RequiresTenant(ParamNamespaceTenant), routesmiddleware.RequiresPermission(authorizer.NamespaceDelete)) publicAPI.POST(AddNamespaceMemberURL, gateway.Handler(handler.AddNamespaceMember), routesmiddleware.RequiresPermission(authorizer.NamespaceAddMember)) publicAPI.PATCH(EditNamespaceMemberURL, gateway.Handler(handler.EditNamespaceMember), routesmiddleware.RequiresPermission(authorizer.NamespaceEditMember)) publicAPI.DELETE(RemoveNamespaceMemberURL, gateway.Handler(handler.RemoveNamespaceMember), routesmiddleware.RequiresPermission(authorizer.NamespaceRemoveMember)) - publicAPI.DELETE(LeaveNamespaceURL, gateway.Handler(handler.LeaveNamespace)) + publicAPI.DELETE(LeaveNamespaceURL, gateway.Handler(handler.LeaveNamespace), routesmiddleware.BlockAPIKey) publicAPI.GET(GetSessionRecordURL, gateway.Handler(handler.GetSessionRecord)) publicAPI.PUT(EditSessionRecordStatusURL, gateway.Handler(handler.EditSessionRecordStatus), routesmiddleware.RequiresTenant(ParamNamespaceTenant), routesmiddleware.RequiresPermission(authorizer.NamespaceEnableSessionRecord)) diff --git a/openapi/spec/paths/api@namespaces.yaml b/openapi/spec/paths/api@namespaces.yaml index 42718698fec..4a9e4bd09a6 100644 --- a/openapi/spec/paths/api@namespaces.yaml +++ b/openapi/spec/paths/api@namespaces.yaml @@ -1,6 +1,5 @@ get: operationId: getNamespaces - x-internal: true summary: Get namespaces list description: Returns a list of namespaces. tags: @@ -8,7 +7,6 @@ get: - namespaces security: - jwt: [] - - api-key: [] parameters: - name: filter description: | @@ -62,7 +60,6 @@ get: $ref: ../components/responses/500.yaml post: operationId: createNamespace - x-internal: true summary: Create namespace description: Create a namespace. tags: @@ -70,7 +67,6 @@ post: - namespaces security: - jwt: [] - - api-key: [] requestBody: content: application/json: diff --git a/openapi/spec/paths/api@namespaces@api-key.yaml b/openapi/spec/paths/api@namespaces@api-key.yaml index bb4ed5520ec..2a59ce1be26 100644 --- a/openapi/spec/paths/api@namespaces@api-key.yaml +++ b/openapi/spec/paths/api@namespaces@api-key.yaml @@ -34,7 +34,6 @@ post: $ref: ../components/responses/500.yaml get: operationId: apiKeyList - x-internal: true summary: List API Keys tags: - api-keys @@ -42,7 +41,6 @@ get: - namespaces security: - jwt: [] - - api-key: [] parameters: - $ref: ../components/parameters/query/pageQuery.yaml - $ref: ../components/parameters/query/perPageQuery.yaml diff --git a/openapi/spec/paths/api@namespaces@{tenant}@members.yaml b/openapi/spec/paths/api@namespaces@{tenant}@members.yaml index 2b4e8e88cce..845d2a62c24 100644 --- a/openapi/spec/paths/api@namespaces@{tenant}@members.yaml +++ b/openapi/spec/paths/api@namespaces@{tenant}@members.yaml @@ -1,6 +1,5 @@ delete: operationId: leaveNamespace - x-internal: true summary: Leave Namespace description: | Allows the authenticated user to leave the specified namespace. Owners @@ -13,7 +12,6 @@ delete: - namespaces security: - jwt: [] - - api-key: [] parameters: - $ref: ../components/parameters/path/namespaceTenantIDPath.yaml responses: