From 36f397e768e22daf1679e9653b8896a8c9c45a3d Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Mon, 1 Jun 2026 15:51:37 -0300 Subject: [PATCH 1/2] 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: From b55826864344701f0cd1cc277ada533de3214140 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Mon, 1 Jun 2026 17:11:39 -0300 Subject: [PATCH 2/2] refactor(api): remove redundant GET /api/users/security route The GET /api/users/security route returned a single boolean (namespace session record status) that is already available from GET /api/namespaces/{tenant} via settings.session_record. The active frontend (ui-react) does not call it. The only reference was in the dead Vue UI under /v1/. The path was also misleading: /users/security for a namespace-level setting, with no tenant guard unlike its sibling PUT route. Removed the route registration, URL constant, handler, service method, mock, and all associated tests. --- api/routes/nsadm.go | 15 -------- api/routes/nsadm_test.go | 47 ----------------------- api/routes/routes.go | 1 - api/services/mocks/services.go | 28 -------------- api/services/namespace.go | 16 -------- api/services/namespace_test.go | 68 ---------------------------------- 6 files changed, 175 deletions(-) diff --git a/api/routes/nsadm.go b/api/routes/nsadm.go index 4b0c0945bd9..1c302177bc2 100644 --- a/api/routes/nsadm.go +++ b/api/routes/nsadm.go @@ -18,7 +18,6 @@ const ( AddNamespaceMemberURL = "/namespaces/:tenant/members" RemoveNamespaceMemberURL = "/namespaces/:tenant/members/:uid" EditNamespaceMemberURL = "/namespaces/:tenant/members/:uid" - GetSessionRecordURL = "/users/security" EditSessionRecordStatusURL = "/users/security/:tenant" EditDeviceAutoAcceptURL = "/namespaces/device-auto-accept/:tenant" ) @@ -249,17 +248,3 @@ func (h *Handler) EditSessionRecordStatus(c gateway.Context) error { return c.NoContent(http.StatusOK) } - -func (h *Handler) GetSessionRecord(c gateway.Context) error { - var tenant string - if v := c.Tenant(); v != nil { - tenant = v.ID - } - - status, err := h.service.GetSessionRecord(c.Ctx(), tenant) - if err != nil { - return err - } - - return c.JSON(http.StatusOK, status) -} diff --git a/api/routes/nsadm_test.go b/api/routes/nsadm_test.go index e361b94f371..1736bb6e0d0 100644 --- a/api/routes/nsadm_test.go +++ b/api/routes/nsadm_test.go @@ -244,53 +244,6 @@ func TestDeleteNamespace(t *testing.T) { mock.AssertExpectations(t) } -func TestGetSessionRecord(t *testing.T) { - mock := new(mocks.Service) - - cases := []struct { - name string - tenant string - requiredMocks func() - expectedStatus int - }{ - { - name: "fails when try to get a session record of a non-existing session", - tenant: "tenant", - requiredMocks: func() { - mock.On("GetSessionRecord", gomock.Anything, "tenant").Return(false, svc.ErrNotFound).Once() - }, - expectedStatus: http.StatusNotFound, - }, - { - name: "success when try to get a session record of a existing session", - tenant: "tenant", - requiredMocks: func() { - mock.On("GetSessionRecord", gomock.Anything, "tenant").Return(true, nil).Once() - }, - expectedStatus: http.StatusOK, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - tc.requiredMocks() - - req := httptest.NewRequest(http.MethodGet, "/api/users/security", nil) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("X-Role", authorizer.RoleOwner.String()) - req.Header.Set("X-Tenant-ID", tc.tenant) - rec := httptest.NewRecorder() - - e := NewRouter(mock) - e.ServeHTTP(rec, req) - - assert.Equal(t, tc.expectedStatus, rec.Result().StatusCode) - }) - } - - mock.AssertExpectations(t) -} - func TestEditNamespace(t *testing.T) { svcMock := new(mocks.Service) diff --git a/api/routes/routes.go b/api/routes/routes.go index 4c4cc374c13..85155d6b279 100644 --- a/api/routes/routes.go +++ b/api/routes/routes.go @@ -189,7 +189,6 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { publicAPI.DELETE(RemoveNamespaceMemberURL, gateway.Handler(handler.RemoveNamespaceMember), routesmiddleware.RequiresPermission(authorizer.NamespaceRemoveMember)) 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)) publicAPI.PUT(EditDeviceAutoAcceptURL, gateway.Handler(handler.EditDeviceAutoAccept), routesmiddleware.RequiresTenant(ParamNamespaceTenant), routesmiddleware.RequiresPermission(authorizer.NamespaceDeviceAutoAccept)) diff --git a/api/services/mocks/services.go b/api/services/mocks/services.go index 33d33847011..0aa288167f9 100644 --- a/api/services/mocks/services.go +++ b/api/services/mocks/services.go @@ -855,34 +855,6 @@ func (_m *Service) GetSession(ctx context.Context, uid models.UID) (*models.Sess return r0, r1 } -// GetSessionRecord provides a mock function with given fields: ctx, tenantID -func (_m *Service) GetSessionRecord(ctx context.Context, tenantID string) (bool, error) { - ret := _m.Called(ctx, tenantID) - - if len(ret) == 0 { - panic("no return value specified for GetSessionRecord") - } - - var r0 bool - var r1 error - if rf, ok := ret.Get(0).(func(context.Context, string) (bool, error)); ok { - return rf(ctx, tenantID) - } - if rf, ok := ret.Get(0).(func(context.Context, string) bool); ok { - r0 = rf(ctx, tenantID) - } else { - r0 = ret.Get(0).(bool) - } - - if rf, ok := ret.Get(1).(func(context.Context, string) error); ok { - r1 = rf(ctx, tenantID) - } else { - r1 = ret.Error(1) - } - - return r0, r1 -} - // GetStats provides a mock function with given fields: ctx, req func (_m *Service) GetStats(ctx context.Context, req *requests.GetStats) (*models.Stats, error) { ret := _m.Called(ctx, req) diff --git a/api/services/namespace.go b/api/services/namespace.go index ba52b9508fc..77ce8dfbdbb 100644 --- a/api/services/namespace.go +++ b/api/services/namespace.go @@ -20,7 +20,6 @@ type NamespaceService interface { GetNamespace(ctx context.Context, tenantID string) (*models.Namespace, error) DeleteNamespace(ctx context.Context, tenantID string) error EditSessionRecordStatus(ctx context.Context, sessionRecord bool, tenantID string) error - GetSessionRecord(ctx context.Context, tenantID string) (bool, error) EditDeviceAutoAccept(ctx context.Context, deviceAutoAccept bool, tenantID string) error } @@ -248,18 +247,3 @@ func (s *service) EditDeviceAutoAccept(ctx context.Context, deviceAutoAccept boo return nil } - -// GetSessionRecord gets the session record data. -// -// It receives a context, used to "control" the request flow, the tenant ID from models.Namespace. -// -// GetSessionRecord returns a boolean indicating the session record status and an error. When error is not nil, -// the boolean is false. -func (s *service) GetSessionRecord(ctx context.Context, tenantID string) (bool, error) { - n, err := s.store.NamespaceResolve(ctx, store.NamespaceTenantIDResolver, tenantID) - if err != nil { - return false, NewErrNamespaceNotFound(tenantID, err) - } - - return n.Settings.SessionRecord, nil -} diff --git a/api/services/namespace_test.go b/api/services/namespace_test.go index 12173bba867..6a13361ee02 100644 --- a/api/services/namespace_test.go +++ b/api/services/namespace_test.go @@ -1499,71 +1499,3 @@ func TestDeleteNamespace(t *testing.T) { storeMock.AssertExpectations(t) } - -func TestGetSessionRecord(t *testing.T) { - storeMock := new(storemock.Store) - - type Expected struct { - status bool - err error - } - - cases := []struct { - name string - tenantID string - mocks func(context.Context) - expected Expected - }{ - { - name: "fails when namespace not found", - tenantID: "a736a52b-5777-4f92-b0b8-e359bf484713", - mocks: func(ctx context.Context) { - storeMock.On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "a736a52b-5777-4f92-b0b8-e359bf484713").Return(nil, store.ErrNoDocuments).Once() - }, - expected: Expected{false, NewErrNamespaceNotFound("a736a52b-5777-4f92-b0b8-e359bf484713", store.ErrNoDocuments)}, - }, - { - name: "fails when store namespace resolve fails", - tenantID: "a736a52b-5777-4f92-b0b8-e359bf484713", - mocks: func(ctx context.Context) { - storeMock. - On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "a736a52b-5777-4f92-b0b8-e359bf484713"). - Return(nil, errors.New("error")). - Once() - }, - expected: Expected{false, NewErrNamespaceNotFound("a736a52b-5777-4f92-b0b8-e359bf484713", errors.New("error"))}, - }, - { - name: "succeeds", - tenantID: "a736a52b-5777-4f92-b0b8-e359bf484713", - mocks: func(ctx context.Context) { - storeMock. - On("NamespaceResolve", ctx, store.NamespaceTenantIDResolver, "a736a52b-5777-4f92-b0b8-e359bf484713"). - Return( - &models.Namespace{ - Name: "group1", - Owner: "hash1", - TenantID: "a736a52b-5777-4f92-b0b8-e359bf484713", - Settings: &models.NamespaceSettings{SessionRecord: false}, - }, - nil, - ). - Once() - }, - expected: Expected{false, nil}, - }, - } - - s := NewService(store.Store(storeMock), privateKey, publicKey, storecache.NewNullCache(), clientMock) - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - ctx := context.Background() - tc.mocks(ctx) - status, err := s.GetSessionRecord(ctx, tc.tenantID) - assert.Equal(t, tc.expected, Expected{status, err}) - }) - } - - storeMock.AssertExpectations(t) -}