From c58918313c319141f6e0a0a4d3220b1d2163956e Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Mon, 24 Aug 2026 13:33:03 -0300 Subject: [PATCH 1/7] feat(server): let a caller reach a request's embedded paginator and sorter A request type that embeds both query.Paginator and query.Sorter promotes two Normalize methods at the same depth, and they cancel each other out: neither is reachable through the outer type, and no interface over that name can be satisfied. Every list handler works around this by naming the embedded fields, which only works when the handler knows the concrete request type. GetPaginator and GetSorter are reachable because their names are unique, so code holding any request can normalize the page and the sort order without knowing what it is holding. --- pkg/api/query/normalize_test.go | 45 +++++++++++++++++++++++++++++++++ pkg/api/query/paginator.go | 15 +++++++++++ pkg/api/query/sorter.go | 11 ++++++++ 3 files changed, 71 insertions(+) create mode 100644 pkg/api/query/normalize_test.go diff --git a/pkg/api/query/normalize_test.go b/pkg/api/query/normalize_test.go new file mode 100644 index 00000000000..3b6d19fdde0 --- /dev/null +++ b/pkg/api/query/normalize_test.go @@ -0,0 +1,45 @@ +package query_test + +import ( + "testing" + + "github.com/shellhub-io/shellhub/pkg/api/query" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// listRequest is shaped like the request types the API binds: it embeds both a paginator and a +// sorter, which is exactly the case that makes a promoted Normalize method ambiguous. +type listRequest struct { + query.Paginator + query.Sorter +} + +func TestEmbeddedAccessorsReachBothValues(t *testing.T) { + req := &listRequest{} + + paginated, ok := any(req).(query.Paginated) + require.True(t, ok, "a request embedding query.Paginator must satisfy query.Paginated") + + sorted, ok := any(req).(query.Sorted) + require.True(t, ok, "a request embedding query.Sorter must satisfy query.Sorted") + + paginated.GetPaginator().Normalize() + sorted.GetSorter().Normalize() + + assert.Equal(t, query.MinPage, req.Paginator.Page) + assert.Equal(t, query.DefaultPerPage, req.Paginator.PerPage) + assert.Equal(t, query.OrderDesc, req.Sorter.Order) +} + +// TestAccessorsAreAbsentWhenNotEmbedded pins the other half: the wrapper skips normalization for a +// request that carries neither, instead of normalizing something it invented. +func TestAccessorsAreAbsentWhenNotEmbedded(t *testing.T) { + req := &struct{ UID string }{} + + _, paginated := any(req).(query.Paginated) + _, sorted := any(req).(query.Sorted) + + assert.False(t, paginated) + assert.False(t, sorted) +} diff --git a/pkg/api/query/paginator.go b/pkg/api/query/paginator.go index 8fdc242b2d7..851663c1a41 100644 --- a/pkg/api/query/paginator.go +++ b/pkg/api/query/paginator.go @@ -41,3 +41,18 @@ func (p *Paginator) Normalize() { p.PerPage = int(math.Max(math.Min(float64(p.PerPage), float64(MaxPerPage)), float64(MinPerPage))) } } + +// Paginated is a request that carries a [Paginator]. Every request type embedding one satisfies it, +// so a caller holding only the request can normalize the page without knowing its concrete type. +// +// The accessor exists because a request embedding both a [Paginator] and a [Sorter] promotes two +// Normalize methods at the same depth, which cancel each other out: neither is reachable through +// the outer type, and no interface over that name can be satisfied. +type Paginated interface { + GetPaginator() *Paginator +} + +// GetPaginator returns the paginator itself, satisfying [Paginated] for every type that embeds it. +func (p *Paginator) GetPaginator() *Paginator { + return p +} diff --git a/pkg/api/query/sorter.go b/pkg/api/query/sorter.go index 6e3688d4b00..cb9755d6d44 100644 --- a/pkg/api/query/sorter.go +++ b/pkg/api/query/sorter.go @@ -29,3 +29,14 @@ func (s *Sorter) Normalize() { s.Order = OrderDesc } } + +// Sorted is a request that carries a [Sorter]. It is the sorting half of [Paginated], and exists +// for the same reason: the promoted Normalize methods cancel out when both are embedded. +type Sorted interface { + GetSorter() *Sorter +} + +// GetSorter returns the sorter itself, satisfying [Sorted] for every type that embeds it. +func (s *Sorter) GetSorter() *Sorter { + return s +} From 122a2a065a397c6ec468c771a4f86fb48e46a16e Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Mon, 24 Aug 2026 13:33:43 -0300 Subject: [PATCH 2/7] refactor(server): let the service decide what a connector device filter is The device list handler read a raw query parameter and appended two filter entries of its own. It did so after validation, so the pair escaped the filter-count limit and a caller could buy two extra entries by asking for containers. It also branched on the parameter being present rather than on its value, which made connector=false mean the opposite of what it says. The request now carries the caller's intent and the service builds the filter, following the precedent the sorter's tiebreak field already set. The appended pair counts against the limit like any other, and the intent reads the value. Counting it has a consequence worth stating: the device list's effective cap on caller-supplied filters drops from eight to six, because the pair is appended before the count is checked. A caller sending seven or eight filters to /api/devices is now refused where it was served. That is the limit meaning what it says, but it is a wire change and not only a refactor. Only the count is re-checked at that point. The handler already validated what the caller sent, and the pair appended here is a known-good platform comparison against a field the device list knows, so the count is the one thing appending can break. The marker itself is what the connector agent writes to a device's platform field. A connector device is an ordinary device row; that marker is the only thing that sets it apart. The result is a new filter set rather than an edit of the request, because a request is an input and reusing one must not compound the filter. The alias states its intent ahead of the caller's query string. The binder reads the first value of a repeated parameter, so /api/containers?connector=false would otherwise have returned exactly the devices the container endpoint exists to exclude. The route test that covered this asserted only that an operator preceded a property, never which devices the comparison kept. It is replaced by one that asserts the intent reaching the service, and by service tests that assert the filter in both directions. --- pkg/api/requests/device.go | 5 + server/api/routes/device.go | 40 ------ server/api/routes/device_test.go | 137 ++++++++----------- server/api/routes/routes.go | 2 +- server/api/services/device.go | 39 +++++- server/api/services/device_connector_test.go | 130 ++++++++++++++++++ server/api/services/device_test.go | 22 +-- server/api/services/errors.go | 7 + 8 files changed, 252 insertions(+), 130 deletions(-) create mode 100644 server/api/services/device_connector_test.go diff --git a/pkg/api/requests/device.go b/pkg/api/requests/device.go index 382859c559d..7d46cb30b0b 100644 --- a/pkg/api/requests/device.go +++ b/pkg/api/requests/device.go @@ -10,6 +10,11 @@ import ( type DeviceList struct { TenantID string `header:"X-Tenant-ID"` DeviceStatus models.DeviceStatus `query:"status"` // TODO: validate + + // Connector asks for connector devices only. It is the caller's intent, not a filter: the + // service decides how to express it, and the default excludes them. + Connector bool `query:"connector"` + query.Paginator query.Sorter query.Filters diff --git a/server/api/routes/device.go b/server/api/routes/device.go index 684c4f56057..d57fda09d7a 100644 --- a/server/api/routes/device.go +++ b/server/api/routes/device.go @@ -57,46 +57,6 @@ func (h *Handler) GetDeviceList(c *gateway.Context) error { return c.NoContent(http.StatusBadRequest) } - if c.QueryParam("connector") != "" { - filter := []query.Filter{ - { - Type: query.FilterTypeOperator, - Params: &query.FilterOperator{ - Name: "and", - }, - }, - { - Type: query.FilterTypeProperty, - Params: &query.FilterProperty{ - Name: "platform", - Operator: "eq", - Value: "connector", - }, - }, - } - - req.Filters.Data = append(req.Filters.Data, filter...) - } else { - filter := []query.Filter{ - { - Type: query.FilterTypeOperator, - Params: &query.FilterOperator{ - Name: "and", - }, - }, - { - Type: query.FilterTypeProperty, - Params: &query.FilterProperty{ - Name: "platform", - Operator: "ne", - Value: "connector", - }, - }, - } - - req.Filters.Data = append(req.Filters.Data, filter...) - } - if err := c.Validate(req); err != nil { return err } diff --git a/server/api/routes/device_test.go b/server/api/routes/device_test.go index b0c6da0a2d1..404ca55be1b 100644 --- a/server/api/routes/device_test.go +++ b/server/api/routes/device_test.go @@ -497,88 +497,71 @@ func TestGetDeviceListBadFilter(t *testing.T) { } } -func TestGetDeviceListConnectorFilterOrder(t *testing.T) { - cases := []struct { - description string - connector string - userFilter []query.Filter - }{ - { - description: "connector filter has AND before property when user filter is present", - connector: "", - userFilter: []query.Filter{ - { - Type: query.FilterTypeProperty, - Params: &query.FilterProperty{Name: "name", Operator: "contains", Value: "foo"}, - }, - }, - }, - { - description: "connector=true filter has AND before property when user filter is present", - connector: "true", - userFilter: []query.Filter{ - { - Type: query.FilterTypeProperty, - Params: &query.FilterProperty{Name: "name", Operator: "contains", Value: "foo"}, - }, - }, - }, +// TestContainerAliasCarriesTheConnectorIntent pins the two behaviours the /api/containers rewrite +// depends on: the container list is the device list carrying the connector intent, and a single +// container is the plain device route carrying none. +// +// Which comparison that intent becomes is the service's decision, and is asserted there. +func TestContainerAliasCarriesTheConnectorIntent(t *testing.T) { + const tenantID = "00000000-0000-4000-0000-000000000000" + + get := func(t *testing.T, mock *mocks.MockService, target string) int { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, target, nil) + req.Header.Set("X-Role", authorizer.RoleOwner.String()) + req.Header.Set("X-ID", "000000000000000000000000") + req.Header.Set("X-Tenant-ID", tenantID) + + rec := httptest.NewRecorder() + NewRouter(mock).ServeHTTP(rec, req) + + return rec.Result().StatusCode } - for _, tc := range cases { - t.Run(tc.description, func(t *testing.T) { - mock := mocks.NewMockService(t) - - var captured *requests.DeviceList - mock. - On("ListDevices", gomock.Anything, gomock.Anything, gomock.AnythingOfType("*requests.DeviceList")). - Run(func(args gomock.Arguments) { - list, ok := args.Get(2).(*requests.DeviceList) - require.True(t, ok) - captured = list - }). - Return([]models.Device{}, 0, nil). - Once() - - filterJSON, err := json.Marshal(tc.userFilter) - require.NoError(t, err) - - filterB64 := base64.StdEncoding.EncodeToString(filterJSON) - - urlVal := &url.Values{} - urlVal.Set("page", "1") - urlVal.Set("per_page", "10") - urlVal.Set("sort_by", "name") - urlVal.Set("order_by", "asc") - urlVal.Set("status", "accepted") - urlVal.Set("filter", filterB64) - if tc.connector != "" { - urlVal.Set("connector", tc.connector) - } - - req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/devices?"+urlVal.Encode(), nil) - req.Header.Set("X-Role", authorizer.RoleOwner.String()) - req.Header.Set("X-Tenant-ID", "00000000-0000-4000-0000-000000000000") + listExpecting := func(t *testing.T, connector bool) *mocks.MockService { + t.Helper() - rec := httptest.NewRecorder() - e := NewRouter(mock) - e.ServeHTTP(rec, req) - - require.Equal(t, http.StatusOK, rec.Result().StatusCode) - require.NotNil(t, captured) + mock := mocks.NewMockService(t) + mock. + On("ListDevices", gomock.Anything, scope.MustBounded(tenantID), gomock.MatchedBy(func(req *requests.DeviceList) bool { + return req.Connector == connector + })). + Return([]models.Device{}, 0, nil). + Once() - data := captured.Data - require.GreaterOrEqual(t, len(data), 3) - - lastTwo := data[len(data)-2:] - require.Equal(t, query.FilterTypeOperator, lastTwo[0].Type, "AND operator must precede the platform property filter") - require.Equal(t, query.FilterTypeProperty, lastTwo[1].Type, "platform property filter must follow the AND operator") - - op, ok := lastTwo[0].Params.(*query.FilterOperator) - require.True(t, ok) - require.Equal(t, "and", op.Name) - }) + return mock } + + t.Run("the container list asks for connector devices", func(t *testing.T) { + mock := listExpecting(t, true) + require.Equal(t, http.StatusOK, get(t, mock, "/api/containers")) + }) + + t.Run("the container list keeps the intent alongside a query string", func(t *testing.T) { + mock := listExpecting(t, true) + require.Equal(t, http.StatusOK, get(t, mock, "/api/containers?status=accepted")) + }) + + t.Run("the container list keeps its intent against a connector the caller sent", func(t *testing.T) { + mock := listExpecting(t, true) + require.Equal(t, http.StatusOK, get(t, mock, "/api/containers?connector=false")) + }) + + t.Run("the device list asks for none", func(t *testing.T) { + mock := listExpecting(t, false) + require.Equal(t, http.StatusOK, get(t, mock, "/api/devices")) + }) + + t.Run("a single container resolves to the plain device route", func(t *testing.T) { + mock := mocks.NewMockService(t) + mock. + On("GetDevice", gomock.Anything, scope.MustBounded(tenantID), models.UID("uid1")). + Return(&models.Device{UID: "uid1"}, nil). + Once() + + require.Equal(t, http.StatusOK, get(t, mock, "/api/containers/uid1")) + }) } func TestUpdateDevice(t *testing.T) { diff --git a/server/api/routes/routes.go b/server/api/routes/routes.go index 9a45aeae724..7f4f14c55b4 100644 --- a/server/api/routes/routes.go +++ b/server/api/routes/routes.go @@ -253,7 +253,7 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { router.Pre(echoMiddleware.Rewrite(map[string]string{ "/api/containers": "/api/devices?connector=true", - "/api/containers?*": "/api/devices?$1&connector=true", + "/api/containers?*": "/api/devices?connector=true&$1", "/api/containers/*": "/api/devices/$1", })) diff --git a/server/api/services/device.go b/server/api/services/device.go index 0a16b13d8ba..7a7e2bd1396 100644 --- a/server/api/services/device.go +++ b/server/api/services/device.go @@ -111,7 +111,44 @@ func (s *service) deviceLimit(ctx context.Context, tenantID string) (models.Name return s.store.NamespaceGetDeviceLimit(ctx, tenantID) } +const connectorPlatform = "connector" + +func connectorFilters(filters query.Filters, connector bool) (query.Filters, error) { + operator := "ne" + if connector { + operator = "eq" + } + + narrowed := query.Filters{ + Raw: filters.Raw, + Data: make([]query.Filter, 0, len(filters.Data)+2), + } + + narrowed.Data = append(narrowed.Data, filters.Data...) + narrowed.Data = append(narrowed.Data, + query.Filter{ + Type: query.FilterTypeOperator, + Params: &query.FilterOperator{Name: "and"}, + }, + query.Filter{ + Type: query.FilterTypeProperty, + Params: &query.FilterProperty{Name: "platform", Operator: operator, Value: connectorPlatform}, + }, + ) + + if len(narrowed.Data) > query.MaxFilterItems { + return query.Filters{}, NewErrDeviceFilterInvalid(query.ErrFilterPropertyInvalid) + } + + return narrowed, nil +} + func (s *service) ListDevices(ctx context.Context, sc scope.Scope, req *requests.DeviceList) ([]models.Device, int, error) { + filters, err := connectorFilters(req.Filters, req.Connector) + if err != nil { + return nil, 0, err + } + opts := []store.QueryOption{} if req.DeviceStatus != "" { @@ -124,7 +161,7 @@ func (s *service) ListDevices(ctx context.Context, sc scope.Scope, req *requests req.Sorter.Tiebreak = "id" - opts = append(opts, s.store.Options().Match(&req.Filters), s.store.Options().Sort(&req.Sorter), s.store.Options().Paginate(&req.Paginator)) + opts = append(opts, s.store.Options().Match(&filters), s.store.Options().Sort(&req.Sorter), s.store.Options().Paginate(&req.Paginator)) if req.DeviceStatus == models.DeviceStatusRemoved { return s.store.DeviceList(ctx, sc, store.DeviceAcceptableFromRemoved, opts...) diff --git a/server/api/services/device_connector_test.go b/server/api/services/device_connector_test.go new file mode 100644 index 00000000000..a6fbc64052a --- /dev/null +++ b/server/api/services/device_connector_test.go @@ -0,0 +1,130 @@ +package services + +import ( + "testing" + + "github.com/shellhub-io/shellhub/pkg/api/query" + "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + storecache "github.com/shellhub-io/shellhub/pkg/cache" + "github.com/shellhub-io/shellhub/pkg/models" + "github.com/shellhub-io/shellhub/server/api/store" + storemock "github.com/shellhub-io/shellhub/server/api/store/mocks" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +func connectorFilter(operator string) query.Filter { + return query.Filter{ + Type: query.FilterTypeProperty, + Params: &query.FilterProperty{Name: "platform", Operator: operator, Value: "connector"}, + } +} + +func andFilter() query.Filter { + return query.Filter{ + Type: query.FilterTypeOperator, + Params: &query.FilterOperator{Name: "and"}, + } +} + +func withoutConnectors(filters ...query.Filter) *query.Filters { + return &query.Filters{Data: append(filters, andFilter(), connectorFilter("ne"))} +} + +// TestListDevicesConnectorIntent asserts what the caller's intent means, not how it is spelled: the +// route test it replaces only checked that an operator preceded a property, and never which of +// them included or excluded connector devices. +func TestListDevicesConnectorIntent(t *testing.T) { + const tenantID = "00000000-0000-4000-0000-000000000000" + + userFilter := query.Filter{ + Type: query.FilterTypeProperty, + Params: &query.FilterProperty{Name: "name", Operator: "contains", Value: "foo"}, + } + + cases := []struct { + description string + connector bool + filters []query.Filter + expectedFilters []query.Filter + expectedErr error + }{ + { + description: "excludes connector devices when the caller asked for none", + connector: false, + filters: nil, + expectedFilters: []query.Filter{andFilter(), connectorFilter("ne")}, + }, + { + description: "narrows to connector devices when the caller asked for them", + connector: true, + filters: nil, + expectedFilters: []query.Filter{andFilter(), connectorFilter("eq")}, + }, + { + description: "applies the intent after the caller's own filters", + connector: true, + filters: []query.Filter{userFilter}, + expectedFilters: []query.Filter{userFilter, andFilter(), connectorFilter("eq")}, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(tt *testing.T) { + storeMock := storemock.NewMockStore(tt) + queryOptionsMock := storemock.NewMockQueryOptions(tt) + storeMock.On("Options").Return(queryOptionsMock).Maybe() + + queryOptionsMock.On("Match", &query.Filters{Data: tc.expectedFilters}).Return(nil).Once() + queryOptionsMock.On("Sort", mock.Anything).Return(nil).Once() + queryOptionsMock.On("Paginate", mock.Anything).Return(nil).Once() + storeMock.On("NamespaceGetDeviceLimit", mock.Anything, tenantID).Return(models.NamespaceDeviceLimit{}, nil).Once() + storeMock. + On("DeviceList", mock.Anything, scope.MustBounded(tenantID), store.DeviceAcceptableIfNotAccepted, mock.Anything). + Return([]models.Device{}, 0, nil). + Once() + + service := NewService(storeMock, privateKey, publicKey, storecache.NewNullCache()) + + req := &requests.DeviceList{ + TenantID: tenantID, + Connector: tc.connector, + Paginator: query.Paginator{Page: 1, PerPage: 10}, + Sorter: query.Sorter{By: "created_at", Order: query.OrderAsc}, + Filters: query.Filters{Data: tc.filters}, + } + + _, _, err := service.ListDevices(t.Context(), scope.MustBounded(tenantID), req) + require.NoError(tt, err) + }) + } +} + +// TestListDevicesConnectorIntentRespectsTheFilterLimit pins that the pair the service appends is +// counted like any other filter. Appending it around the limit would let a caller buy two extra +// filter entries by asking for containers. +func TestListDevicesConnectorIntentRespectsTheFilterLimit(t *testing.T) { + const tenantID = "00000000-0000-4000-0000-000000000000" + + filters := make([]query.Filter, 0, query.MaxFilterItems) + for range query.MaxFilterItems { + filters = append(filters, query.Filter{ + Type: query.FilterTypeProperty, + Params: &query.FilterProperty{Name: "name", Operator: "contains", Value: "foo"}, + }) + } + + storeMock := storemock.NewMockStore(t) + service := NewService(storeMock, privateKey, publicKey, storecache.NewNullCache()) + + req := &requests.DeviceList{ + TenantID: tenantID, + Paginator: query.Paginator{Page: 1, PerPage: 10}, + Sorter: query.Sorter{By: "created_at", Order: query.OrderAsc}, + Filters: query.Filters{Data: filters}, + } + + _, _, err := service.ListDevices(t.Context(), scope.MustBounded(tenantID), req) + require.ErrorIs(t, err, ErrDeviceFilterInvalid) +} diff --git a/server/api/services/device_test.go b/server/api/services/device_test.go index 47af1041cb1..15d58ad72c3 100644 --- a/server/api/services/device_test.go +++ b/server/api/services/device_test.go @@ -57,7 +57,7 @@ func TestListDevices(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -99,7 +99,7 @@ func TestListDevices(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -140,7 +140,7 @@ func TestListDevices(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -194,7 +194,7 @@ func TestListDevices_namespaceFromRequestContext(t *testing.T) { expectQueryOptions := func(queryOptionsMock *storemock.MockQueryOptions) { queryOptionsMock.On("WithDeviceStatus", models.DeviceStatusAccepted).Return(nil).Once() - queryOptionsMock.On("Match", &query.Filters{}).Return(nil).Once() + queryOptionsMock.On("Match", withoutConnectors()).Return(nil).Once() queryOptionsMock.On("Sort", &query.Sorter{By: "created_at", Order: query.OrderAsc, Tiebreak: "id"}).Return(nil).Once() queryOptionsMock.On("Paginate", &query.Paginator{Page: 1, PerPage: 10}).Return(nil).Once() } @@ -276,7 +276,7 @@ func TestListDevices_status_removed(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -314,7 +314,7 @@ func TestListDevices_status_removed(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -392,7 +392,7 @@ func TestListDevices_tenant_not_empty(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -431,7 +431,7 @@ func TestListDevices_tenant_not_empty(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -474,7 +474,7 @@ func TestListDevices_tenant_not_empty(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -517,7 +517,7 @@ func TestListDevices_tenant_not_empty(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. @@ -560,7 +560,7 @@ func TestListDevices_tenant_not_empty(t *testing.T) { Return(nil). Once() queryOptionsMock. - On("Match", &query.Filters{}). + On("Match", withoutConnectors()). Return(nil). Once() queryOptionsMock. diff --git a/server/api/services/errors.go b/server/api/services/errors.go index b9271673b22..d3ceb721162 100644 --- a/server/api/services/errors.go +++ b/server/api/services/errors.go @@ -109,6 +109,7 @@ var ( ErrNoTags = errors.New("no tags has found", ErrLayer, ErrCodeNotFound) ErrConflictName = errors.New("name duplicated", ErrLayer, ErrCodeDuplicated) ErrInvalidFormat = errors.New("invalid format", ErrLayer, ErrCodeInvalid) + ErrDeviceFilterInvalid = errors.New("device filter invalid", ErrLayer, ErrCodeInvalid) ErrDeviceNotFound = errors.New("device not found", ErrLayer, ErrCodeNotFound) ErrDeviceLoginCodeNotFound = errors.New("device login code not found", ErrLayer, ErrCodeNotFound) ErrDevicePairingCodeNotFound = errors.New("device pairing code not found", ErrLayer, ErrCodeNotFound) @@ -433,6 +434,12 @@ func NewErrPublicKeyFilter(next error) error { return NewErrInvalid(ErrPublicKeyFilter, nil, next) } +// NewErrDeviceFilterInvalid returns an error when the device list filter cannot be honoured, such +// as when it exceeds the filter limits. +func NewErrDeviceFilterInvalid(next error) error { + return NewErrInvalid(ErrDeviceFilterInvalid, nil, next) +} + // NewErrDeviceNotFound returns an error when the device is not found. func NewErrDeviceNotFound(id models.UID, next error) error { return NewErrNotFound(ErrDeviceNotFound, string(id), next) From 2dbf0248d0e85a2726ca8103e99a58c76b49687d Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Mon, 24 Aug 2026 13:33:58 -0300 Subject: [PATCH 3/7] feat(server): name the actor a request carries, and read it back in one place The authenticator stamps seven headers through Identity.WriteTo, and everything downstream took them apart again one accessor at a time. Nothing held the two spellings of that header set together. IdentityFrom is the read side of that write, and a round-trip test is what keeps them naming the same headers: a field added to one and forgotten in the other passes review and the compiler, and shows up only as an identity that quietly loses part of itself on the way to a handler. IdentityHeaders names that set for a third caller: anything replaying a request internally has to carry the headers forward for the identity to survive the hop, and reading them from here is what stops a replayed subset drifting out of step with the write. An Actor is the identity narrowed to what a handler may see. Role and admin stay behind, because they decide what the caller may do and the middleware answers that first. An actor is not always a person: an API key and a device token name a namespace principal with no user behind them, which is why the type carries the credential rather than a user ID alone. --- server/api/pkg/gateway/actor.go | 29 +++++++++ server/api/pkg/gateway/identity.go | 31 +++++++++ server/api/pkg/gateway/identity_test.go | 84 +++++++++++++++++++++++++ 3 files changed, 144 insertions(+) create mode 100644 server/api/pkg/gateway/actor.go create mode 100644 server/api/pkg/gateway/identity_test.go diff --git a/server/api/pkg/gateway/actor.go b/server/api/pkg/gateway/actor.go new file mode 100644 index 00000000000..fdd663e7123 --- /dev/null +++ b/server/api/pkg/gateway/actor.go @@ -0,0 +1,29 @@ +package gateway + +// Actor is the authenticated identity a request carries: who is performing it, established before +// any namespace is considered. An actor is not yet a member of anything — resolving it within a +// namespace scope is what produces the acting member. +// +// Which fields are set follows the credential the request authenticated with. A user token names +// the acting person, filling ID and Username; an API key and a device token name a namespace +// principal with no person behind it, so both leave ID and Username empty. +type Actor struct { + // ID is the acting user's ID, empty when the credential names no person. + ID string + + // Username is the acting user's username. It is the only identifier an admin-console request + // carries, because that surface deliberately strips the user's ID. + Username string + + // APIKey is the key the request authenticated with, empty otherwise. + APIKey string + + // DeviceUID is the device the request authenticated as, empty otherwise. + DeviceUID string +} + +// IsZero reports whether the request carried no authenticated identity at all. A route that +// requires an actor refuses such a request; an anonymous route is the one place it is expected. +func (a Actor) IsZero() bool { + return a.ID == "" && a.Username == "" && a.APIKey == "" && a.DeviceUID == "" +} diff --git a/server/api/pkg/gateway/identity.go b/server/api/pkg/gateway/identity.go index b25372a4936..a0c69782c3b 100644 --- a/server/api/pkg/gateway/identity.go +++ b/server/api/pkg/gateway/identity.go @@ -2,6 +2,7 @@ package gateway import ( "net/http" + "slices" "github.com/shellhub-io/shellhub/pkg/api/authorizer" ) @@ -29,6 +30,13 @@ var identityHeaders = []string{ "X-Admin", } +// IdentityHeaders returns the headers [Identity.WriteTo] stamps. A request dispatched internally +// must carry them forward for the caller's identity to survive the hop, and reading them from here +// is what keeps that set from drifting out of step with the write. +func IdentityHeaders() []string { + return slices.Clone(identityHeaders) +} + // WriteTo stamps the identity onto header, clearing every identity header // first — including the ones this identity leaves empty. // @@ -63,6 +71,29 @@ func (i *Identity) WriteTo(header http.Header) { } } +// IdentityFrom reads back the identity [Identity.WriteTo] stamped onto header. +// +// It is the read side of that write, and the two must name the same headers. TestIdentityRoundTrip +// is what holds them together; a comment cannot. +func IdentityFrom(header http.Header) Identity { + return Identity{ + ID: header.Get("X-ID"), + Username: header.Get("X-Username"), + TenantID: header.Get("X-Tenant-ID"), + DeviceUID: header.Get("X-Device-UID"), + APIKey: header.Get("X-API-Key"), + Role: authorizer.RoleFromString(header.Get("X-Role")), + Admin: header.Get("X-Admin") == "true", + } +} + +// Actor returns the identity as the [Actor] a handler receives: who is performing the request, +// without the role and admin flag. Those decide what the caller may do, which the middleware +// answers before a handler runs. +func (i *Identity) Actor() Actor { + return Actor{ID: i.ID, Username: i.Username, APIKey: i.APIKey, DeviceUID: i.DeviceUID} +} + // WithoutUserScope returns the identity stripped of the acting user's ID and // namespace scope, keeping the admin flag. // diff --git a/server/api/pkg/gateway/identity_test.go b/server/api/pkg/gateway/identity_test.go new file mode 100644 index 00000000000..6bfeff84327 --- /dev/null +++ b/server/api/pkg/gateway/identity_test.go @@ -0,0 +1,84 @@ +package gateway + +import ( + "net/http" + "testing" + + "github.com/shellhub-io/shellhub/pkg/api/authorizer" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestIdentityRoundTrip is what keeps [Identity.WriteTo] and [IdentityFrom] naming the same headers. +// A field added to one and forgotten in the other survives review and the compiler, and shows up +// only as an identity that silently loses part of itself between the authenticator and the handler. +func TestIdentityRoundTrip(t *testing.T) { + cases := []struct { + description string + identity Identity + }{ + { + description: "a user token", + identity: Identity{ + ID: "user-id", + Username: "username", + TenantID: "00000000-0000-4000-0000-000000000000", + Role: authorizer.RoleOwner, + }, + }, + { + description: "an api key", + identity: Identity{ + TenantID: "00000000-0000-4000-0000-000000000000", + APIKey: "key", + Role: authorizer.RoleObserver, + }, + }, + { + description: "a device token", + identity: Identity{ + DeviceUID: "device-uid", + TenantID: "00000000-0000-4000-0000-000000000000", + }, + }, + { + description: "an admin browsing the admin console", + identity: Identity{ + Username: "username", + Role: authorizer.RoleOwner, + Admin: true, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(tt *testing.T) { + header := http.Header{} + tc.identity.WriteTo(header) + + require.Equal(tt, tc.identity, IdentityFrom(header)) + }) + } +} + +// TestIdentityActorDropsAuthorization pins what an actor deliberately is not. Role and admin decide +// what the caller may do, which the middleware owns; handing them to a handler invites it to make +// that decision a second time. +func TestIdentityActorDropsAuthorization(t *testing.T) { + identity := Identity{ + ID: "user-id", + Username: "username", + TenantID: "00000000-0000-4000-0000-000000000000", + DeviceUID: "device-uid", + APIKey: "key", + Role: authorizer.RoleOwner, + Admin: true, + } + + assert.Equal(t, Actor{ + ID: "user-id", + Username: "username", + APIKey: "key", + DeviceUID: "device-uid", + }, identity.Actor()) +} From 131720ea7615b7c701cb3682c6433857775ce541 Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Mon, 24 Aug 2026 13:34:18 -0300 Subject: [PATCH 4/7] feat(server): give routes three shapes that own the HTTP ceremony Eighty-five handlers each rewrote the same five blocks, and the copies had already drifted apart. X-Total-Count is written before the error check in two places and after it in ten, and two sites report the length of the page rather than the count the service returned. Three idioms answered "which namespace is this bounded to?", so a cross-tenant read could be introduced in any handler and reviewing for it meant reading all of them. Nothing enforced one answer, because no module held it. One module holds it now. A route registers under One, List or None, and the wrapper binds the request, normalizes the paginator and sorter, validates, resolves the namespace scope and the actor, calls the handler and encodes the result. Each shape produces an echo.HandlerFunc, so per-route middleware composes exactly as it does today. Every route is bounded and needs an actor unless its registration says why not, and both reasons are required arguments. The two claims are independent: a device authenticating with its own token is bounded to a namespace and still carries no actor. Each registration records what it claimed, which is what lets a test refuse a reason left empty -- Echo does not expose a route's handler, so there is nothing else to enumerate. The inventory of claims is a process-wide value keyed by the claim itself, so building the same route table twice records it once. It exists to be read by a test and nothing in production calls it: a required argument makes a reason impossible to omit, but only an inventory makes an empty one impossible to merge. The authenticator's anonymous-route accessor is the same shape for the same reason. Scope and actor are resolved inside that per-request preparation rather than when the gateway context is built, and the order is not incidental. The identity accessors read their values back off the request headers lazily, and the authenticator writes those headers later than the context is constructed. Resolving either one early reads headers that are not there yet, and yields an empty scope and a zero actor rather than an error. Normalization runs before validation rather than after. Validating first turns an out-of-range page or an unknown sort order into a 400, where every list route today corrects it and carries on. The total count is written after the error check and from the count the handler returned: a failed request must not answer with a count, and a count read back off the returned page is the page size rather than the size of the collection. The wrapper refuses a request when the gateway context is not installed, so a wiring mistake fails closed rather than serving unscoped data. It keeps stashing that context in the request context, which is how the service layer's tenant, username and identity lookups still reach it; dropping that would break them silently, with no compile error. The ceremony is asserted once, here, rather than re-tested per entity. A request embedding neither a paginator nor a sorter is driven through the wrapper itself, which is where skipping normalization is a real decision rather than a fact about Go's type system. --- pkg/api/query/normalize_test.go | 14 - pkg/api/requests/empty.go | 5 + server/api/pkg/gateway/route.go | 253 ++++++++++++++++++ server/api/pkg/gateway/route_test.go | 374 +++++++++++++++++++++++++++ server/api/pkg/gateway/utils.go | 8 +- 5 files changed, 637 insertions(+), 17 deletions(-) create mode 100644 pkg/api/requests/empty.go create mode 100644 server/api/pkg/gateway/route.go create mode 100644 server/api/pkg/gateway/route_test.go diff --git a/pkg/api/query/normalize_test.go b/pkg/api/query/normalize_test.go index 3b6d19fdde0..e8386cee375 100644 --- a/pkg/api/query/normalize_test.go +++ b/pkg/api/query/normalize_test.go @@ -8,8 +8,6 @@ import ( "github.com/stretchr/testify/require" ) -// listRequest is shaped like the request types the API binds: it embeds both a paginator and a -// sorter, which is exactly the case that makes a promoted Normalize method ambiguous. type listRequest struct { query.Paginator query.Sorter @@ -31,15 +29,3 @@ func TestEmbeddedAccessorsReachBothValues(t *testing.T) { assert.Equal(t, query.DefaultPerPage, req.Paginator.PerPage) assert.Equal(t, query.OrderDesc, req.Sorter.Order) } - -// TestAccessorsAreAbsentWhenNotEmbedded pins the other half: the wrapper skips normalization for a -// request that carries neither, instead of normalizing something it invented. -func TestAccessorsAreAbsentWhenNotEmbedded(t *testing.T) { - req := &struct{ UID string }{} - - _, paginated := any(req).(query.Paginated) - _, sorted := any(req).(query.Sorted) - - assert.False(t, paginated) - assert.False(t, sorted) -} diff --git a/pkg/api/requests/empty.go b/pkg/api/requests/empty.go new file mode 100644 index 00000000000..d47c5d3c6e6 --- /dev/null +++ b/pkg/api/requests/empty.go @@ -0,0 +1,5 @@ +package requests + +// Empty is the request of a route that takes no input. A handler is a function of its request, so +// a route with nothing to read still names the shape of what it reads. +type Empty struct{} diff --git a/server/api/pkg/gateway/route.go b/server/api/pkg/gateway/route.go new file mode 100644 index 00000000000..af1296a44fd --- /dev/null +++ b/server/api/pkg/gateway/route.go @@ -0,0 +1,253 @@ +package gateway + +import ( + "context" + "net/http" + "reflect" + "runtime" + "sort" + "strconv" + "sync" + + "github.com/labstack/echo/v5" + "github.com/shellhub-io/shellhub/pkg/api/query" + "github.com/shellhub-io/shellhub/pkg/api/scope" + routes "github.com/shellhub-io/shellhub/server/api/routes/errors" +) + +const totalCountHeader = "X-Total-Count" + +// Shape names the response a wrapped handler produces. Every API resource operation answers with +// one of the three; a route that fits none of them is registered directly and named in the route +// table's exempt set. +type Shape string + +const ( + // ShapeOne answers with a JSON body. + ShapeOne Shape = "one" + // ShapeList answers with a JSON body and the total count of the collection. + ShapeList Shape = "list" + // ShapeNone answers with 200 and no body. + ShapeNone Shape = "none" +) + +// OneHandler answers with a single value. It is a function of its inputs: it does not know that +// HTTP exists, and cannot be called without the namespace it is bounded to and the actor +// performing it. +type OneHandler[T, R any] func(ctx context.Context, sc scope.Scope, actor Actor, req *T) (R, error) + +// ListHandler answers with a page of values and the size of the whole collection. The wrapper +// writes that count to the response, so no handler decides where the header goes. +type ListHandler[T, R any] func(ctx context.Context, sc scope.Scope, actor Actor, req *T) (R, int, error) + +// NoneHandler answers with success alone. +type NoneHandler[T any] func(ctx context.Context, sc scope.Scope, actor Actor, req *T) error + +// One registers handler as a route answering with a JSON body. +func One[T, R any](handler OneHandler[T, R], options ...RouteOption) echo.HandlerFunc { + declaration := declare(handler, ShapeOne, options) + + return func(c *echo.Context) error { + in, err := prepare[T](c, declaration) + if err != nil { + return err + } + + res, err := handler(in.ctx, in.scope, in.actor, in.req) + if err != nil { + return err + } + + return c.JSON(http.StatusOK, res) + } +} + +// List registers handler as a route answering with a JSON body and the total-count header. +func List[T, R any](handler ListHandler[T, R], options ...RouteOption) echo.HandlerFunc { + declaration := declare(handler, ShapeList, options) + + return func(c *echo.Context) error { + in, err := prepare[T](c, declaration) + if err != nil { + return err + } + + res, count, err := handler(in.ctx, in.scope, in.actor, in.req) + if err != nil { + return err + } + + c.Response().Header().Set(totalCountHeader, strconv.Itoa(count)) + + return c.JSON(http.StatusOK, res) + } +} + +// None registers handler as a route answering with 200 and no body. +func None[T any](handler NoneHandler[T], options ...RouteOption) echo.HandlerFunc { + declaration := declare(handler, ShapeNone, options) + + return func(c *echo.Context) error { + in, err := prepare[T](c, declaration) + if err != nil { + return err + } + + if err := handler(in.ctx, in.scope, in.actor, in.req); err != nil { + return err + } + + return c.NoContent(http.StatusOK) + } +} + +type inputs[T any] struct { + ctx context.Context + scope scope.Scope + actor Actor + req *T +} + +func prepare[T any](c *echo.Context, declaration Declaration) (inputs[T], error) { + gCtx, ok := From(c) + if !ok { + return inputs[T]{}, echo.ErrInternalServerError + } + + stash(c, gCtx) + + req := new(T) + if err := c.Bind(req); err != nil { + return inputs[T]{}, err + } + + if paginated, ok := any(req).(query.Paginated); ok { + paginated.GetPaginator().Normalize() + } + + if sorted, ok := any(req).(query.Sorted); ok { + sorted.GetSorter().Normalize() + } + + if err := c.Validate(req); err != nil { + return inputs[T]{}, err + } + + sc, err := declaration.resolveScope(gCtx) + if err != nil { + return inputs[T]{}, err + } + + actor, err := declaration.resolveActor(gCtx) + if err != nil { + return inputs[T]{}, err + } + + return inputs[T]{ctx: gCtx.Ctx(), scope: sc, actor: actor, req: req}, nil +} + +// RouteOption declares an exception to the two rules every route follows: it is bounded to a +// namespace, and it is performed by an actor. +type RouteOption func(*Declaration) + +// Unbounded declares that the route deliberately reads across namespaces, and records why that is +// safe. The reason is a required argument, so breadth cannot arrive by omission — only by someone +// typing why. +func Unbounded(reason string) RouteOption { + return func(d *Declaration) { + d.Unbounded, d.UnboundedReason = true, reason + } +} + +// Anonymous declares that the route deliberately carries no actor, and records why that is safe. +// It is independent of [Unbounded]: a device authenticating with its own token is bounded to a +// namespace and still carries no actor. +func Anonymous(reason string) RouteOption { + return func(d *Declaration) { + d.Anonymous, d.AnonymousReason = true, reason + } +} + +// Declaration is what one route registration claims about itself: the shape it answers with, and +// any exception it takes to the default rules. +type Declaration struct { + // Handler is the fully qualified name of the wrapped function, which is what ties a claim back + // to the code it is about. + Handler string + Shape Shape + + Unbounded bool + UnboundedReason string + + Anonymous bool + AnonymousReason string +} + +func (d Declaration) resolveScope(c *Context) (scope.Scope, error) { + if d.Unbounded { + return scope.NewUnbounded(d.UnboundedReason), nil + } + + return c.AdminOrScope() +} + +func (d Declaration) resolveActor(c *Context) (Actor, error) { + identity := IdentityFrom(c.Request().Header) + + actor := identity.Actor() + + if d.Anonymous || !actor.IsZero() { + return actor, nil + } + + return Actor{}, routes.NewErrUnauthorized(nil) +} + +var declarations = struct { + sync.Mutex + set map[Declaration]struct{} +}{set: make(map[Declaration]struct{})} + +func declare(handler any, shape Shape, options []RouteOption) Declaration { + d := Declaration{Handler: handlerName(handler), Shape: shape} + for _, option := range options { + option(&d) + } + + declarations.Lock() + defer declarations.Unlock() + + declarations.set[d] = struct{}{} + + return d +} + +// Declarations returns every claim the route tables built in this process have made, ordered by +// handler name. +func Declarations() []Declaration { + declarations.Lock() + defer declarations.Unlock() + + all := make([]Declaration, 0, len(declarations.set)) + for d := range declarations.set { + all = append(all, d) + } + + sort.Slice(all, func(i, j int) bool { return all[i].Handler < all[j].Handler }) + + return all +} + +func handlerName(handler any) string { + value := reflect.ValueOf(handler) + if value.Kind() != reflect.Func { + return "" + } + + fn := runtime.FuncForPC(value.Pointer()) + if fn == nil { + return "" + } + + return fn.Name() +} diff --git a/server/api/pkg/gateway/route_test.go b/server/api/pkg/gateway/route_test.go new file mode 100644 index 00000000000..3fed24258a6 --- /dev/null +++ b/server/api/pkg/gateway/route_test.go @@ -0,0 +1,374 @@ +package gateway_test + +import ( + "context" + "net/http" + "net/http/httptest" + "testing" + + "github.com/labstack/echo/v5" + "github.com/shellhub-io/shellhub/pkg/api/query" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/errors" + "github.com/shellhub-io/shellhub/server/api/pkg/echo/handlers" + "github.com/shellhub-io/shellhub/server/api/pkg/gateway" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const probeTenant = "00000000-0000-4000-0000-000000000000" + +type probeRequest struct { + Count int `query:"count"` + Name string `query:"name" validate:"omitempty,min=3"` + query.Paginator + query.Sorter +} + +type probeCall struct { + called bool + scope scope.Scope + actor gateway.Actor + req *probeRequest + tenant string +} + +func probeRouter(t *testing.T, withGatewayContext bool) *echo.Echo { + t.Helper() + + e := echo.New() + e.Binder = handlers.NewBinder() + e.Validator = handlers.NewValidator() + e.HTTPErrorHandler = handlers.NewErrors(nil) + + if withGatewayContext { + e.Use(gateway.WithContext(nil)) + } + + return e +} + +func probeHandler(call *probeCall, res []string, count int, err error) gateway.ListHandler[probeRequest, []string] { + return func(ctx context.Context, sc scope.Scope, actor gateway.Actor, req *probeRequest) ([]string, int, error) { + call.called = true + call.scope = sc + call.actor = actor + call.req = req + + if tenant := gateway.TenantFromContext(ctx); tenant != nil { + call.tenant = tenant.ID + } + + return res, count, err + } +} + +func TestWrapperCeremony(t *testing.T) { + cases := []struct { + description string + withGatewayContext bool + headers map[string]string + target string + options []gateway.RouteOption + expectedStatus int + expectedCall bool + assert func(*testing.T, *probeCall) + }{ + { + description: "normalizes the paginator and the sorter before the handler sees them", + withGatewayContext: true, + headers: map[string]string{"X-Tenant-ID": probeTenant, "X-ID": "user-id"}, + target: "/probe?page=0&per_page=999&order_by=sideways", + expectedStatus: http.StatusOK, + expectedCall: true, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + assert.Equal(t, query.MinPage, call.req.Paginator.Page) + assert.Equal(t, query.MaxPerPage, call.req.Paginator.PerPage) + assert.Equal(t, query.OrderDesc, call.req.Sorter.Order) + }, + }, + { + description: "refuses a request whose query cannot bind", + withGatewayContext: true, + headers: map[string]string{"X-Tenant-ID": probeTenant, "X-ID": "user-id"}, + target: "/probe?count=not-a-number", + expectedStatus: http.StatusUnprocessableEntity, + }, + { + description: "refuses a request that fails validation", + withGatewayContext: true, + headers: map[string]string{"X-Tenant-ID": probeTenant, "X-ID": "user-id"}, + target: "/probe?name=ab", + expectedStatus: http.StatusBadRequest, + }, + { + description: "bounds the handler to the namespace the caller carries", + withGatewayContext: true, + headers: map[string]string{"X-Tenant-ID": probeTenant, "X-ID": "user-id"}, + target: "/probe", + expectedStatus: http.StatusOK, + expectedCall: true, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + assert.Equal(t, scope.MustBounded(probeTenant), call.scope) + }, + }, + { + description: "refuses a bounded route when the caller carries no namespace", + withGatewayContext: true, + headers: map[string]string{"X-ID": "user-id"}, + target: "/probe", + expectedStatus: http.StatusForbidden, + }, + { + description: "hands an unbounded route the reason its registration stated", + withGatewayContext: true, + headers: map[string]string{"X-ID": "user-id"}, + target: "/probe", + options: []gateway.RouteOption{gateway.Unbounded("the probe reads every namespace")}, + expectedStatus: http.StatusOK, + expectedCall: true, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + assert.False(t, call.scope.IsBounded()) + assert.Equal(t, "the probe reads every namespace", call.scope.Reason()) + }, + }, + { + description: "hands the handler the identity the request authenticated as", + withGatewayContext: true, + headers: map[string]string{ + "X-Tenant-ID": probeTenant, + "X-ID": "user-id", + "X-Username": "username", + }, + target: "/probe", + expectedStatus: http.StatusOK, + expectedCall: true, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + assert.Equal(t, gateway.Actor{ID: "user-id", Username: "username"}, call.actor) + }, + }, + { + description: "accepts an api key as the acting identity", + withGatewayContext: true, + headers: map[string]string{"X-Tenant-ID": probeTenant, "X-API-Key": "key"}, + target: "/probe", + expectedStatus: http.StatusOK, + expectedCall: true, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + assert.Equal(t, gateway.Actor{APIKey: "key"}, call.actor) + }, + }, + { + description: "refuses a route that requires an actor when the request carries none", + withGatewayContext: true, + headers: map[string]string{"X-Tenant-ID": probeTenant}, + target: "/probe", + expectedStatus: http.StatusUnauthorized, + }, + { + description: "runs an anonymous route with no actor at all", + withGatewayContext: true, + headers: map[string]string{"X-Tenant-ID": probeTenant}, + target: "/probe", + options: []gateway.RouteOption{gateway.Anonymous("the probe establishes the actor")}, + expectedStatus: http.StatusOK, + expectedCall: true, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + assert.True(t, call.actor.IsZero()) + }, + }, + { + description: "refuses the request when the gateway context is not installed", + withGatewayContext: false, + headers: map[string]string{"X-Tenant-ID": probeTenant, "X-ID": "user-id"}, + target: "/probe", + expectedStatus: http.StatusInternalServerError, + }, + { + description: "keeps the gateway context reachable from the request context", + withGatewayContext: true, + headers: map[string]string{"X-Tenant-ID": probeTenant, "X-ID": "user-id"}, + target: "/probe", + expectedStatus: http.StatusOK, + expectedCall: true, + assert: func(t *testing.T, call *probeCall) { + t.Helper() + + assert.Equal(t, probeTenant, call.tenant) + }, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + call := new(probeCall) + + e := probeRouter(t, tc.withGatewayContext) + e.GET("/probe", gateway.List(probeHandler(call, []string{"item"}, 1, nil), tc.options...)) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, tc.target, nil) + for name, value := range tc.headers { + req.Header.Set(name, value) + } + + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + require.Equal(t, tc.expectedStatus, rec.Code, rec.Body.String()) + require.Equal(t, tc.expectedCall, call.called) + + if tc.assert != nil { + tc.assert(t, call) + } + }) + } +} + +// TestListWritesTheTotalCountAfterTheErrorCheck pins the answer the twelve hand-written copies of +// this header disagreed on: the count the handler returned, and only once the call succeeded. +func TestListWritesTheTotalCountAfterTheErrorCheck(t *testing.T) { + cases := []struct { + description string + count int + err error + expectedStatus int + expectedCount string + }{ + { + description: "writes the count the handler returned", + count: 42, + expectedStatus: http.StatusOK, + expectedCount: "42", + }, + { + description: "writes no count when the handler failed", + count: 42, + err: errors.New("boom", "route", 3), + expectedStatus: http.StatusUnauthorized, + expectedCount: "", + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + call := new(probeCall) + + e := probeRouter(t, true) + e.GET("/probe", gateway.List(probeHandler(call, []string{"item"}, tc.count, tc.err))) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/probe", nil) + req.Header.Set("X-Tenant-ID", probeTenant) + req.Header.Set("X-ID", "user-id") + + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + require.Equal(t, tc.expectedStatus, rec.Code, rec.Body.String()) + assert.Equal(t, tc.expectedCount, rec.Header().Get("X-Total-Count")) + }) + } +} + +// TestWrapperNormalizesNothingForARequestCarryingNeither drives the case the accessors exist to +// distinguish: a request embedding no paginator and no sorter is passed through untouched, rather +// than normalized against values the wrapper invented. +func TestWrapperNormalizesNothingForARequestCarryingNeither(t *testing.T) { + type plainRequest struct { + UID string `query:"uid"` + } + + var got *plainRequest + + e := probeRouter(t, true) + e.GET("/probe", gateway.One(func(_ context.Context, _ scope.Scope, _ gateway.Actor, req *plainRequest) (string, error) { + got = req + + return "ok", nil + })) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/probe?uid=device&page=0&per_page=999", nil) + req.Header.Set("X-Tenant-ID", probeTenant) + req.Header.Set("X-ID", "user-id") + + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, rec.Body.String()) + require.NotNil(t, got) + assert.Equal(t, "device", got.UID) +} + +func TestOneEncodesTheHandlerResult(t *testing.T) { + e := probeRouter(t, true) + e.GET("/probe", gateway.One(func(_ context.Context, _ scope.Scope, _ gateway.Actor, _ *probeRequest) (map[string]string, error) { + return map[string]string{"name": "value"}, nil + })) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/probe", nil) + req.Header.Set("X-Tenant-ID", probeTenant) + req.Header.Set("X-ID", "user-id") + + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code, rec.Body.String()) + assert.JSONEq(t, `{"name":"value"}`, rec.Body.String()) + assert.Empty(t, rec.Header().Get("X-Total-Count")) +} + +func TestNoneAnswersWithoutABody(t *testing.T) { + e := probeRouter(t, true) + e.GET("/probe", gateway.None(func(_ context.Context, _ scope.Scope, _ gateway.Actor, _ *probeRequest) error { + return nil + })) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/probe", nil) + req.Header.Set("X-Tenant-ID", probeTenant) + req.Header.Set("X-ID", "user-id") + + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + assert.Empty(t, rec.Body.String()) +} + +// TestDeclarationsRecordEveryClaim makes the claims a route table makes readable by a test: a route +// declaring breadth or anonymity must be able to show the reason it typed. +func TestDeclarationsRecordEveryClaim(t *testing.T) { + const unboundedReason = "the declared probe reads every namespace" + + e := probeRouter(t, true) + e.GET("/declared", gateway.List(probeHandler(new(probeCall), nil, 0, nil), + gateway.Unbounded(unboundedReason), + gateway.Anonymous("the declared probe establishes the actor"))) + + var found bool + + for _, declaration := range gateway.Declarations() { + if declaration.UnboundedReason != unboundedReason { + continue + } + + found = true + + assert.Equal(t, gateway.ShapeList, declaration.Shape) + assert.True(t, declaration.Anonymous) + assert.Equal(t, "the declared probe establishes the actor", declaration.AnonymousReason) + assert.NotEmpty(t, declaration.Handler) + } + + assert.True(t, found, "the wrapper recorded no declaration for the probe route") +} diff --git a/server/api/pkg/gateway/utils.go b/server/api/pkg/gateway/utils.go index 0fa9ec73574..90b0f26ff31 100644 --- a/server/api/pkg/gateway/utils.go +++ b/server/api/pkg/gateway/utils.go @@ -15,14 +15,16 @@ func Handler(next func(*Context) error) echo.HandlerFunc { return echo.ErrInternalServerError } - ctx := context.WithValue(c.Request().Context(), "ctx", gCtx) - - c.SetRequest(c.Request().WithContext(ctx)) + stash(c, gCtx) return next(gCtx) } } +func stash(c *echo.Context, gCtx *Context) { + c.SetRequest(c.Request().WithContext(context.WithValue(c.Request().Context(), "ctx", gCtx))) +} + // Middleware adapts echo middleware so it runs with a gateway [Context] in place. func Middleware(m echo.MiddlewareFunc) echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { From 7f553adcec8a8df472270914c24442f0c9d1780b Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Mon, 24 Aug 2026 13:34:38 -0300 Subject: [PATCH 5/7] refactor(server): serve the health check through the None shape The first route to convert, and the one that proves both reason mechanisms: it reports on the instance, which belongs to no namespace, and a load balancer asks it before any credential exists. Neither claim is inferable, so both are typed at the route table where a reviewer reads them. The handler stops knowing about HTTP, so its test stops building a context to call it with. --- server/api/routes/healthcheck.go | 8 +++-- server/api/routes/healthcheck_test.go | 44 +++++++++++---------------- server/api/routes/routes.go | 5 ++- 3 files changed, 27 insertions(+), 30 deletions(-) diff --git a/server/api/routes/healthcheck.go b/server/api/routes/healthcheck.go index c885b14fae5..a5e196949fd 100644 --- a/server/api/routes/healthcheck.go +++ b/server/api/routes/healthcheck.go @@ -1,8 +1,10 @@ package routes import ( - "net/http" + "context" + "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" ) @@ -13,6 +15,6 @@ const ( // EvaluateHealth answers that the API is serving. It checks nothing behind the API, so it // reports reachability rather than readiness. -func (h *Handler) EvaluateHealth(c *gateway.Context) error { - return c.NoContent(http.StatusOK) +func (h *Handler) EvaluateHealth(_ context.Context, _ scope.Scope, _ gateway.Actor, _ *requests.Empty) error { + return nil } diff --git a/server/api/routes/healthcheck_test.go b/server/api/routes/healthcheck_test.go index 6749e2a1174..e1d5214c77b 100644 --- a/server/api/routes/healthcheck_test.go +++ b/server/api/routes/healthcheck_test.go @@ -5,41 +5,33 @@ import ( "net/http/httptest" "testing" - "github.com/labstack/echo/v5" + "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" "github.com/shellhub-io/shellhub/server/api/services/mocks" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestEvaluateHealth(t *testing.T) { - e := echo.New() mock := mocks.NewMockService(t) h := NewHandler(mock, nil) - cases := []struct { - title string - requiredMocks func() - expectedErr error - }{ - { - title: "success when try to make a evaluate health", - expectedErr: nil, - }, - } - - for _, tc := range cases { - t.Run(tc.title, func(t *testing.T) { - req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, HealthCheckURL, nil) - rec := httptest.NewRecorder() - echoContext := e.NewContext(req, rec) - - apictx := gateway.NewContext(mock, echoContext) - err := h.EvaluateHealth(apictx) - - assert.Equal(t, tc.expectedErr, err) - assert.Equal(t, http.StatusOK, rec.Code) - }) - } + require.NoError(t, h.EvaluateHealth(t.Context(), scope.NewUnbounded("test"), gateway.Actor{}, &requests.Empty{})) mock.AssertExpectations(t) } + +// TestHealthCheckAnswersWithoutACredential drives the registration rather than the handler: the +// health check is the route that declares both an unbounded scope and an anonymous actor, so it is +// where those two claims are proven to reach the wire. +func TestHealthCheckAnswersWithoutACredential(t *testing.T) { + router, _, _ := authenticatedRouter(t) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api"+HealthCheckURL, nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Empty(t, rec.Body.String()) +} diff --git a/server/api/routes/routes.go b/server/api/routes/routes.go index 7f4f14c55b4..7c4bc939321 100644 --- a/server/api/routes/routes.go +++ b/server/api/routes/routes.go @@ -118,7 +118,10 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { } publicAPI := router.Group("/api") - publicAPI.GET(HealthCheckURL, gateway.Handler(handler.EvaluateHealth)) + publicAPI.GET(HealthCheckURL, + gateway.None(handler.EvaluateHealth, + gateway.Unbounded("the health check reports on the instance, which belongs to no namespace"), + gateway.Anonymous("the health check is what a load balancer asks before any credential exists"))) publicAPI.GET(AuthLocalUserURLV2, gateway.Handler(handler.CreateUserToken)) // TODO: method POST publicAPI.GET(AuthUserTokenPublicURL, gateway.Handler(handler.CreateUserToken), routesmiddleware.BlockAPIKey) // TODO: method POST From 859f0241ffbebb6fc061da6df59d33fbb438e5da Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Mon, 24 Aug 2026 13:35:06 -0300 Subject: [PATCH 6/7] refactor(server): serve the device read and list through the One and List shapes The two device read routes become functions of their inputs. Both had the whole ceremony written out; what is left is the part specific to the operation, which for the list is the sort field and the caller's encoded filter, and for the read is nothing at all. Three wire changes follow, all of them from the wrapper rather than from these handlers: A failed device list no longer carries X-Total-Count. It previously carried the count the service returned alongside the error. A 400 from the device-list guards carries a JSON body where it used to carry an empty one. Each of the three now names the field the caller can act on -- the sort field it rejected, or the filter it could not decode or validate -- rather than an empty set the UI cannot mark up. connector is a bound bool, so connector=xyz is now 422 rather than ignored. Only the container rewrite sends that parameter. The MCP endpoint dispatches internally by replaying the caller's headers against the router, and the set it replayed was a hand-written subset: the tenant, the role and the API key. That was enough while no route required an actor. It is not enough now -- a session authenticated with a user token carries its identity in X-ID and X-Username, neither of which was forwarded, so the device list would refuse it. The subset is replaced by the identity header set itself, read from the one place that writes it, which is also what stops the two drifting apart again. The device route test sent only a tenant and a role, which no authenticated request looks like. It now sends the identity header the credential would carry, and the MCP tests cover the three credential shapes a caller can arrive with as well as the one that carries no actor at all. --- server/api/routes/device.go | 64 ++------ server/api/routes/device_handler_test.go | 180 +++++++++++++++++++++++ server/api/routes/device_test.go | 17 +++ server/api/routes/mcp.go | 9 +- server/api/routes/mcp_test.go | 55 ++++++- server/api/routes/routes.go | 4 +- 6 files changed, 267 insertions(+), 62 deletions(-) create mode 100644 server/api/routes/device_handler_test.go diff --git a/server/api/routes/device.go b/server/api/routes/device.go index d57fda09d7a..d212507192a 100644 --- a/server/api/routes/device.go +++ b/server/api/routes/device.go @@ -1,13 +1,15 @@ package routes import ( + "context" "net/http" - "strconv" "github.com/shellhub-io/shellhub/pkg/api/query" "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" "github.com/shellhub-io/shellhub/pkg/models" "github.com/shellhub-io/shellhub/server/api/pkg/gateway" + errs "github.com/shellhub-io/shellhub/server/api/routes/errors" "github.com/shellhub-io/shellhub/server/api/services" log "github.com/sirupsen/logrus" ) @@ -33,71 +35,31 @@ const ( ) // GetDeviceList serves the namespace's devices, filtered, sorted and paginated as requested. -func (h *Handler) GetDeviceList(c *gateway.Context) error { - req := new(requests.DeviceList) - - if err := c.Bind(req); err != nil { - return err - } - - req.Paginator.Normalize() - req.Sorter.Normalize() - +func (h *Handler) GetDeviceList(ctx context.Context, sc scope.Scope, _ gateway.Actor, req *requests.DeviceList) ([]models.Device, int, error) { if err := query.ValidateSorter(&req.Sorter, services.DeviceSortFields); err != nil { - return c.NoContent(http.StatusBadRequest) + log.WithError(err).WithField("sort_by", req.Sorter.By).Warn("failed to validate device list sorter") + + return nil, 0, errs.NewErrInvalidEntity(map[string]string{"sort_by": req.Sorter.By}) } if err := req.Filters.Unmarshal(); err != nil { log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to decode device list filter") - return c.NoContent(http.StatusBadRequest) + return nil, 0, errs.NewErrInvalidEntity(map[string]string{"filter": "cannot be decoded"}) } if err := query.ValidateFilters(&req.Filters, services.DeviceFilterFields); err != nil { - return c.NoContent(http.StatusBadRequest) - } - - if err := c.Validate(req); err != nil { - return err - } + log.WithError(err).WithField("filter", req.Filters.Raw).Warn("failed to validate device list filter") - sc, err := c.AdminOrScope() - if err != nil { - return err - } - - res, count, err := h.service.ListDevices(c.Ctx(), sc, req) - c.Response().Header().Set("X-Total-Count", strconv.Itoa(count)) - - if err != nil { - return err + return nil, 0, errs.NewErrInvalidEntity(map[string]string{"filter": "is not valid"}) } - return c.JSON(http.StatusOK, res) + return h.service.ListDevices(ctx, sc, req) } // GetDevice serves a single device by UID. -func (h *Handler) GetDevice(c *gateway.Context) error { - var req requests.DeviceGet - if err := c.Bind(&req); err != nil { - return err - } - - if err := c.Validate(&req); err != nil { - return err - } - - sc, err := c.AdminOrScope() - if err != nil { - return err - } - - device, err := h.service.GetDevice(c.Ctx(), sc, models.UID(req.UID)) - if err != nil { - return err - } - - return c.JSON(http.StatusOK, device) +func (h *Handler) GetDevice(ctx context.Context, sc scope.Scope, _ gateway.Actor, req *requests.DeviceGet) (*models.Device, error) { + return h.service.GetDevice(ctx, sc, models.UID(req.UID)) } // ResolveDevice serves the device matching a name or SSHID, for callers that have a name diff --git a/server/api/routes/device_handler_test.go b/server/api/routes/device_handler_test.go new file mode 100644 index 00000000000..2dc6651043c --- /dev/null +++ b/server/api/routes/device_handler_test.go @@ -0,0 +1,180 @@ +package routes + +import ( + "encoding/base64" + "encoding/json" + "testing" + + "github.com/shellhub-io/shellhub/pkg/api/query" + "github.com/shellhub-io/shellhub/pkg/api/requests" + "github.com/shellhub-io/shellhub/pkg/api/scope" + "github.com/shellhub-io/shellhub/pkg/errors" + "github.com/shellhub-io/shellhub/pkg/models" + "github.com/shellhub-io/shellhub/server/api/pkg/gateway" + errs "github.com/shellhub-io/shellhub/server/api/routes/errors" + svc "github.com/shellhub-io/shellhub/server/api/services" + "github.com/shellhub-io/shellhub/server/api/services/mocks" + "github.com/stretchr/testify/assert" + gomock "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// TestGetDeviceHandler drives the handler as the pure function it is: no HTTP server, no status +// code, just the inputs the route table resolves and the service behind a mock. +func TestGetDeviceHandler(t *testing.T) { + cases := []struct { + description string + sc scope.Scope + uid string + requiredMocks func(*mocks.MockService) + expectedDevice *models.Device + expectedErr error + }{ + { + description: "passes the namespace scope it was given through to the service", + sc: scope.MustBounded("00000000-0000-4000-0000-000000000000"), + uid: "uid", + requiredMocks: func(service *mocks.MockService) { + service. + On("GetDevice", gomock.Anything, scope.MustBounded("00000000-0000-4000-0000-000000000000"), models.UID("uid")). + Return(&models.Device{UID: "uid"}, nil). + Once() + }, + expectedDevice: &models.Device{UID: "uid"}, + }, + { + description: "reports the service's failure unchanged", + sc: scope.NewUnbounded("the admin console reads every namespace"), + uid: "missing", + requiredMocks: func(service *mocks.MockService) { + service. + On("GetDevice", gomock.Anything, scope.NewUnbounded("the admin console reads every namespace"), models.UID("missing")). + Return(nil, svc.ErrDeviceNotFound). + Once() + }, + expectedErr: svc.ErrDeviceNotFound, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + service := mocks.NewMockService(t) + tc.requiredMocks(service) + + handler := NewHandler(service, nil) + + device, err := handler.GetDevice(t.Context(), tc.sc, gateway.Actor{ID: "user-id"}, + &requests.DeviceGet{DeviceParam: requests.DeviceParam{UID: tc.uid}}) + + require.Equal(t, tc.expectedErr, err) + require.Equal(t, tc.expectedDevice, device) + }) + } +} + +// TestGetDeviceListHandler covers what the device list handler still decides now that the wrapper +// owns the ceremony: the sort field, the caller's encoded filter, and nothing else. +func TestGetDeviceListHandler(t *testing.T) { + const tenantID = "00000000-0000-4000-0000-000000000000" + + encode := func(t *testing.T, filters []query.Filter) string { + t.Helper() + + raw, err := json.Marshal(filters) + require.NoError(t, err) + + return base64.StdEncoding.EncodeToString(raw) + } + + cases := []struct { + description string + req func(*testing.T) *requests.DeviceList + requiredMocks func(*mocks.MockService) + expectedFields map[string]string + }{ + { + description: "refuses a sort field the device list does not accept", + req: func(*testing.T) *requests.DeviceList { + return &requests.DeviceList{Sorter: query.Sorter{By: "not_a_column", Order: query.OrderAsc}} + }, + requiredMocks: func(*mocks.MockService) {}, + expectedFields: map[string]string{"sort_by": "not_a_column"}, + }, + { + description: "refuses a filter that is not valid base64", + req: func(*testing.T) *requests.DeviceList { + return &requests.DeviceList{Filters: query.Filters{Raw: "!!!not-base64!!!"}} + }, + requiredMocks: func(*mocks.MockService) {}, + expectedFields: map[string]string{"filter": "cannot be decoded"}, + }, + { + description: "refuses a filter naming a field the device list does not know", + req: func(t *testing.T) *requests.DeviceList { + t.Helper() + + raw := encode(t, []query.Filter{{ + Type: query.FilterTypeProperty, + Params: &query.FilterProperty{Name: "nonexistent_field", Operator: "eq", Value: "foo"}, + }}) + + return &requests.DeviceList{Filters: query.Filters{Raw: raw}} + }, + requiredMocks: func(*mocks.MockService) {}, + expectedFields: map[string]string{"filter": "is not valid"}, + }, + { + description: "hands the service the decoded filter and the caller's connector intent", + req: func(t *testing.T) *requests.DeviceList { + t.Helper() + + raw := encode(t, []query.Filter{{ + Type: query.FilterTypeProperty, + Params: &query.FilterProperty{Name: "name", Operator: "contains", Value: "foo"}, + }}) + + return &requests.DeviceList{TenantID: tenantID, Connector: true, Filters: query.Filters{Raw: raw}} + }, + requiredMocks: func(service *mocks.MockService) { + service. + On("ListDevices", gomock.Anything, scope.MustBounded(tenantID), gomock.MatchedBy(func(req *requests.DeviceList) bool { + if !req.Connector || len(req.Filters.Data) != 1 { + return false + } + + property, ok := req.Filters.Data[0].Params.(*query.FilterProperty) + + return ok && property.Name == "name" && property.Value == "foo" + })). + Return([]models.Device{}, 0, nil). + Once() + }, + }, + } + + for _, tc := range cases { + t.Run(tc.description, func(t *testing.T) { + service := mocks.NewMockService(t) + tc.requiredMocks(service) + + handler := NewHandler(service, nil) + + _, _, err := handler.GetDeviceList(t.Context(), scope.MustBounded(tenantID), gateway.Actor{ID: "user-id"}, tc.req(t)) + + if tc.expectedFields != nil { + require.Error(t, err) + + var wrapped errors.Error + require.ErrorAs(t, err, &wrapped, "a refusal must be a ShellHub error") + + data, ok := wrapped.Data.(errs.ErrDataInvalidEntity) + require.True(t, ok, "a refusal must carry the fields the caller can act on, got %v", wrapped.Data) + assert.Equal(t, tc.expectedFields, data.Fields) + + return + } + + require.NoError(t, err) + }) + } +} diff --git a/server/api/routes/device_test.go b/server/api/routes/device_test.go index 404ca55be1b..62605542a1d 100644 --- a/server/api/routes/device_test.go +++ b/server/api/routes/device_test.go @@ -36,9 +36,21 @@ func TestGetDevice(t *testing.T) { uid string tenant string admin bool + noIdentity bool requiredMocks func() expected Expected }{ + { + title: "refuses the request when the caller carries no identity", + uid: "1234", + tenant: "00000000-0000-4000-0000-000000000000", + noIdentity: true, + requiredMocks: func() {}, + expected: Expected{ + expectedSession: nil, + expectedStatus: http.StatusUnauthorized, + }, + }, { title: "fails when bind fails to validate uid", uid: "", @@ -105,6 +117,9 @@ func TestGetDevice(t *testing.T) { req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/devices/"+tc.uid, nil) req.Header.Set("Content-Type", "application/json") req.Header.Set("X-Role", authorizer.RoleOwner.String()) + if !tc.noIdentity { + req.Header.Set("X-ID", "000000000000000000000000") + } if tc.tenant != "" { req.Header.Set("X-Tenant-ID", tc.tenant) } @@ -400,6 +415,7 @@ func TestGetDeviceList(t *testing.T) { req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/devices?"+urlVal.Encode(), nil) req.Header.Set("X-Role", authorizer.RoleOwner.String()) + req.Header.Set("X-ID", "000000000000000000000000") req.Header.Set("X-Tenant-ID", tc.req.TenantID) rec := httptest.NewRecorder() @@ -485,6 +501,7 @@ func TestGetDeviceListBadFilter(t *testing.T) { req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "/api/devices?"+urlVal.Encode(), nil) req.Header.Set("X-Role", authorizer.RoleOwner.String()) + req.Header.Set("X-ID", "000000000000000000000000") req.Header.Set("X-Tenant-ID", "00000000-0000-4000-0000-000000000000") rec := httptest.NewRecorder() diff --git a/server/api/routes/mcp.go b/server/api/routes/mcp.go index deaf3024f4f..e735e60bd80 100644 --- a/server/api/routes/mcp.go +++ b/server/api/routes/mcp.go @@ -15,6 +15,7 @@ import ( "github.com/mark3labs/mcp-go/mcp" mcpserver "github.com/mark3labs/mcp-go/server" "github.com/shellhub-io/shellhub/pkg/api/authorizer" + "github.com/shellhub-io/shellhub/server/api/pkg/gateway" ) type mcpContextKey string @@ -24,12 +25,6 @@ const ( mcpKeyHeaders mcpContextKey = "mcp_headers" ) -var mcpAuthHeaders = []string{ - "X-Tenant-ID", - "X-Role", - "X-Api-Key", -} - // SetupMCPRoutes mounts the MCP Streamable HTTP server at /mcp. func SetupMCPRoutes(router *echo.Echo) { s := buildMCPServer(router) @@ -45,7 +40,7 @@ func SetupMCPRoutes(router *echo.Echo) { ctx = context.WithValue(ctx, mcpKeyTenantID, tenantID) headers := http.Header{} - for _, key := range mcpAuthHeaders { + for _, key := range gateway.IdentityHeaders() { if value := r.Header.Get(key); value != "" { headers.Set(key, value) } diff --git a/server/api/routes/mcp_test.go b/server/api/routes/mcp_test.go index 45f5907b5b3..1b3188586d0 100644 --- a/server/api/routes/mcp_test.go +++ b/server/api/routes/mcp_test.go @@ -2,7 +2,6 @@ package routes import ( "bytes" - "context" "encoding/json" "net/http" "net/http/httptest" @@ -27,7 +26,13 @@ const mcpCallerTenant = "00000000-0000-4000-0000-000000000000" func mcpCall(t *testing.T, router http.Handler, tenant, role, body string) *httptest.ResponseRecorder { t.Helper() - req := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/mcp", bytes.NewBufferString(body)) + return mcpCallAs(t, router, tenant, role, http.Header{"X-API-Key": []string{"mcp-api-key"}}, body) +} + +func mcpCallAs(t *testing.T, router http.Handler, tenant, role string, credential http.Header, body string) *httptest.ResponseRecorder { + t.Helper() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodPost, "/mcp", bytes.NewBufferString(body)) req.Header.Set("Content-Type", "application/json") req.Header.Set("Mcp-Session-Id", "mcp-session-00000000-0000-4000-8000-000000000000") if tenant != "" { @@ -36,6 +41,11 @@ func mcpCall(t *testing.T, router http.Handler, tenant, role, body string) *http if role != "" { req.Header.Set("X-Role", role) } + for key, values := range credential { + for _, value := range values { + req.Header.Add(key, value) + } + } rec := httptest.NewRecorder() router.ServeHTTP(rec, req) @@ -145,6 +155,47 @@ func TestMCPListDevices(t *testing.T) { mock.AssertExpectations(t) } +func TestMCPForwardsEveryCredentialsActor(t *testing.T) { + credentials := map[string]http.Header{ + "a user token names the acting person": {"X-ID": []string{"000000000000000000000000"}}, + "an API key names no person": {"X-API-Key": []string{"mcp-api-key"}}, + "an admin request carries only a username": { + "X-Username": []string{"admin"}, + "X-Admin": []string{"true"}, + }, + } + + for description, credential := range credentials { + t.Run(description, func(t *testing.T) { + mock := mocks.NewMockService(t) + mock. + On("ListDevices", gomock.Anything, gomock.Anything, gomock.AnythingOfType("*requests.DeviceList")). + Return([]models.Device{{UID: "uid1"}}, 7, nil). + Once() + + rec := mcpCallAs(t, NewRouter(mock), mcpCallerTenant, authorizer.RoleOwner.String(), credential, + mcpToolCall("shellhub_list_devices", `{}`)) + + text, isErr := mcpToolResult(t, rec) + require.False(t, isErr, text) + assert.Contains(t, text, `"total": 7`) + mock.AssertExpectations(t) + }) + } +} + +func TestMCPRefusesACallCarryingNoActor(t *testing.T) { + mock := mocks.NewMockService(t) + + rec := mcpCallAs(t, NewRouter(mock), mcpCallerTenant, authorizer.RoleOwner.String(), http.Header{}, + mcpToolCall("shellhub_list_devices", `{}`)) + + text, isErr := mcpToolResult(t, rec) + assert.True(t, isErr) + assert.Contains(t, text, "unauthorized") + mock.AssertNotCalled(t, "ListDevices") +} + // TestMCPGetDevice ensures the uid arg becomes the path parameter. func TestMCPGetDevice(t *testing.T) { mock := mocks.NewMockService(t) diff --git a/server/api/routes/routes.go b/server/api/routes/routes.go index 7c4bc939321..7d63553659f 100644 --- a/server/api/routes/routes.go +++ b/server/api/routes/routes.go @@ -155,8 +155,8 @@ func NewRouter(service services.Service, opts ...Option) *echo.Echo { publicAPI.GET(URLNamespaceMembershipInvitationList, gateway.Handler(handler.GetNamespaceMembershipInvitationList), routesmiddleware.RequiresPermission(authorizer.NamespaceEditMember)) publicAPI.DELETE(URLCancelMembershipInvitation, gateway.Handler(handler.CancelMembershipInvitation), routesmiddleware.RequiresPermission(authorizer.NamespaceRemoveMember)) - publicAPI.GET(GetDeviceListURL, routesmiddleware.Authorize(gateway.Handler(handler.GetDeviceList))) - publicAPI.GET(GetDeviceURL, routesmiddleware.Authorize(gateway.Handler(handler.GetDevice))) + publicAPI.GET(GetDeviceListURL, routesmiddleware.Authorize(gateway.List(handler.GetDeviceList))) + publicAPI.GET(GetDeviceURL, routesmiddleware.Authorize(gateway.One(handler.GetDevice))) publicAPI.GET(ResolveDeviceURL, routesmiddleware.Authorize(gateway.Handler(handler.ResolveDevice))) publicAPI.PUT(UpdateDevice, gateway.Handler(handler.UpdateDevice), routesmiddleware.RequiresPermission(authorizer.DeviceUpdate)) publicAPI.PATCH(RenameDeviceURL, gateway.Handler(handler.RenameDevice), routesmiddleware.RequiresPermission(authorizer.DeviceRename)) From 76975b50880cb9d555b608acbb29b82dd7276cbb Mon Sep 17 00:00:00 2001 From: Otavio Salvador Date: Mon, 24 Aug 2026 13:35:15 -0300 Subject: [PATCH 7/7] test(server): hold the route table to the claims it makes A required argument makes a reason impossible to omit. Only an inventory of the claims makes an empty one impossible to merge, so this reads back what the route table declared while it was built and refuses a claim that says nothing. A second test proves that check bites rather than passing because it looks at nothing. The exempt set names the routes that are not resource operations and so keep a direct registration. Echo does not expose a route's handler, so no test can prove that a route outside the set went through a wrapper; what the set buys is the other direction, where joining it is a visible edit a reviewer reads. Each member is cross-checked against the router, so an entry that outlives its route fails. Why each member is exempt, since the set itself can no longer say so: GET /api/install serves a shell script rather than JSON, so it answers with none of the three shapes. POST /api/login and POST /api/auth/user derive three non-200 outcomes from values that are not errors, and set two headers the console reads. POST /api/tags and POST /api/namespaces/:tenant/tags return the created identifier in a response header. Both leave this set once that identifier moves into the response body. The three converted routes are pinned to both their shape and their address, which rules out a claim recorded by a wrapper nothing mounted. A route's anonymity is stated in two places that nothing else joins: the gateway claim frees the handler from needing an actor, and the authenticator's allowlist is what lets the request past the credential check. A route carrying one without the other is either unreachable or reachable without a credential, so the two are now asserted to agree. Fixes: shellhub-io/shellhub#6940 --- server/api/routes/route_table_test.go | 147 ++++++++++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 server/api/routes/route_table_test.go diff --git a/server/api/routes/route_table_test.go b/server/api/routes/route_table_test.go new file mode 100644 index 00000000000..6260dafbd4a --- /dev/null +++ b/server/api/routes/route_table_test.go @@ -0,0 +1,147 @@ +package routes + +import ( + "strings" + "testing" + + "github.com/shellhub-io/shellhub/server/api/pkg/gateway" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func unstatedClaims(declarations []gateway.Declaration) []string { + unstated := make([]string, 0) + + for _, declaration := range declarations { + if declaration.Unbounded && strings.TrimSpace(declaration.UnboundedReason) == "" { + unstated = append(unstated, declaration.Handler+" reads across namespaces and states no reason") + } + + if declaration.Anonymous && strings.TrimSpace(declaration.AnonymousReason) == "" { + unstated = append(unstated, declaration.Handler+" requires no actor and states no reason") + } + } + + return unstated +} + +// TestRouteTableStatesEveryClaim reads the claims the route table made while it was built. A route +// that reads across namespaces, or that needs no actor, has to say why — otherwise breadth and +// anonymity arrive by omission, which is what the two claims exist to prevent. +func TestRouteTableStatesEveryClaim(t *testing.T) { + authenticatedRouter(t) + + declarations := gateway.Declarations() + require.NotEmpty(t, declarations, "the route table registered no wrapped route") + + assert.Empty(t, unstatedClaims(declarations)) +} + +// TestUnstatedClaimsRefusesAnEmptyReason proves the check above bites, rather than passing because +// it looks at nothing. +func TestUnstatedClaimsRefusesAnEmptyReason(t *testing.T) { + unstated := unstatedClaims([]gateway.Declaration{ + {Handler: "silent", Unbounded: true, Anonymous: true}, + {Handler: "stated", Unbounded: true, UnboundedReason: "because"}, + }) + + require.Len(t, unstated, 2) + for _, complaint := range unstated { + assert.Contains(t, complaint, "silent") + } +} + +var wrapperExemptRoutes = []string{ + "GET /api/install", + "POST /api/login", + "POST /api/auth/user", + "POST /api/tags", + "POST /api/namespaces/:tenant/tags", +} + +// TestWrapperExemptRoutesAreRegistered catches a stale member: an exempt route that no longer +// exists, or was renamed, leaves the set claiming an exemption for nothing. +func TestWrapperExemptRoutesAreRegistered(t *testing.T) { + router, _, _ := authenticatedRouter(t) + + registered := make(map[string]struct{}) + for _, route := range router.Router().Routes() { + registered[route.Method+" "+route.Path] = struct{}{} + } + + for _, exempt := range wrapperExemptRoutes { + assert.Contains(t, registered, exempt, "the exempt set names %q but no such route is registered", exempt) + } +} + +var convertedRoutes = []struct { + handler string + shape gateway.Shape + route string +}{ + {handler: "EvaluateHealth", shape: gateway.ShapeNone, route: "GET /api" + HealthCheckURL}, + {handler: "GetDevice", shape: gateway.ShapeOne, route: "GET /api" + GetDeviceURL}, + {handler: "GetDeviceList", shape: gateway.ShapeList, route: "GET /api" + GetDeviceListURL}, +} + +func methodName(qualified string) string { + return strings.TrimSuffix(qualified[strings.LastIndex(qualified, ".")+1:], "-fm") +} + +// TestAnonymousClaimsMatchTheAllowlist joins the two places a route's anonymity is stated. The +// gateway claim frees the handler from needing an actor; the authenticator's allowlist is what lets +// the request past the credential check. Nothing but this holds them to the same answer, and a +// route carrying one without the other is either unreachable or reachable without a credential. +func TestAnonymousClaimsMatchTheAllowlist(t *testing.T) { + _, authn, _ := authenticatedRouter(t) + + allowed := make(map[string]struct{}) + for _, route := range authn.AnonymousRoutes() { + allowed[route] = struct{}{} + } + + claimed := make(map[string]bool) + for _, declaration := range gateway.Declarations() { + claimed[methodName(declaration.Handler)] = declaration.Anonymous + } + + for _, tc := range convertedRoutes { + t.Run(tc.handler, func(t *testing.T) { + _, inAllowlist := allowed[tc.route] + + assert.Equal(t, claimed[tc.handler], inAllowlist, + "%s declares Anonymous=%v but the authenticator's allowlist says %v", + tc.handler, claimed[tc.handler], inAllowlist) + }) + } +} + +// TestConvertedRoutesDeclareTheirShape pins the three routes this change converted: each answers +// with the shape it was registered under, and each is mounted at the address it claims. +// +// The second half is what keeps the declaration honest. Echo does not expose a route's handler, so +// a declaration cannot be matched to its route in general — but for a named handler at a known +// address, asserting both is enough to rule out a claim recorded by a wrapper nothing mounted. +func TestConvertedRoutesDeclareTheirShape(t *testing.T) { + router, _, _ := authenticatedRouter(t) + + registered := make(map[string]struct{}) + for _, route := range router.Router().Routes() { + registered[route.Method+" "+route.Path] = struct{}{} + } + + shapes := make(map[string]gateway.Shape) + for _, declaration := range gateway.Declarations() { + shapes[methodName(declaration.Handler)] = declaration.Shape + } + + for _, tc := range convertedRoutes { + t.Run(tc.handler, func(tt *testing.T) { + declared, found := shapes[tc.handler] + + require.True(tt, found, "%s is not registered through a gateway shape", tc.handler) + assert.Equal(tt, tc.shape, declared, "%s answers with the wrong shape", tc.handler) + assert.Contains(tt, registered, tc.route, "%s declares a shape but is not mounted", tc.handler) + }) + } +}