diff --git a/CHANGELOG.md b/CHANGELOG.md index c569423..08e7c7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## Unreleased + +- fix: escape the service id used in the StackState snapshot query and build the request body with a JSON encoder, preventing STQL/JSON query injection +- fix: guard the service check and discovery against missing components, identifiers, short base URLs and unexpected identifier formats instead of panicking, and avoid a possible nil-dereference when a StackState request fails before a response is received + ## v1.0.27 - chore(deps): bump github.com/steadybit/extension-kit diff --git a/extservice/common.go b/extservice/common.go index af29a32..1878744 100644 --- a/extservice/common.go +++ b/extservice/common.go @@ -5,6 +5,7 @@ package extservice import ( "context" + "encoding/json" "fmt" "github.com/go-resty/resty/v2" ) @@ -31,17 +32,30 @@ type StackStateHttpClient struct { } func (s *StackStateHttpClient) GetServiceSnapshot(ctx context.Context, serviceId string) (*resty.Response, ViewSnapshotResponseWrapper, error) { - return s.executeSnapshotQuery(ctx, fmt.Sprintf("(id = \\\"%s\\\")", serviceId)) + return s.executeSnapshotQuery(ctx, fmt.Sprintf("(id = %s)", stqlString(serviceId))) } func (s *StackStateHttpClient) GetServiceSnapshots(ctx context.Context) (*resty.Response, ViewSnapshotResponseWrapper, error) { - return s.executeSnapshotQuery(ctx, "(type = \\\"service\\\")") + return s.executeSnapshotQuery(ctx, `(type = "service")`) +} + +// stqlString renders a value as a quoted, escaped string literal using JSON string escaping, +// which escapes the quotes and backslashes that could otherwise let the value break out of an +// STQL string literal and inject into the query. +func stqlString(value string) string { + encoded, _ := json.Marshal(value) + return string(encoded) } func (s *StackStateHttpClient) executeSnapshotQuery(ctx context.Context, query string) (*resty.Response, ViewSnapshotResponseWrapper, error) { + // Encode the query as a JSON string so it is correctly escaped inside the request body. + queryJSON, err := json.Marshal(query) + if err != nil { + return nil, ViewSnapshotResponseWrapper{}, err + } requestBody := fmt.Sprintf(`{ "_type": "ViewSnapshotRequest", - "query": "%v", + "query": %s, "queryVersion": "0.0.1", "metadata": { "_type": "QueryMetadata", @@ -57,7 +71,7 @@ func (s *StackStateHttpClient) executeSnapshotQuery(ctx context.Context, query s "neighboringComponents": false, "showFullComponent": false } - }`, query) + }`, queryJSON) var stackStateResponse ViewSnapshotResponseWrapper response, err := s.Client.R(). SetContext(ctx). diff --git a/extservice/common_test.go b/extservice/common_test.go new file mode 100644 index 0000000..105181e --- /dev/null +++ b/extservice/common_test.go @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: MIT +// SPDX-FileCopyrightText: 2024 Steadybit GmbH + +package extservice + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-resty/resty/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestStqlString(t *testing.T) { + assert.Equal(t, `"a\"b\\c"`, stqlString(`a"b\c`)) +} + +func TestGetServiceSnapshot_EscapesServiceId(t *testing.T) { + var capturedBody []byte + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedBody, _ = io.ReadAll(r.Body) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer srv.Close() + + client := &StackStateHttpClient{Client: resty.New().SetBaseURL(srv.URL)} + // A service id that tries to break out of the STQL string literal / JSON body. + maliciousId := `1") OR (1=1` + _, _, err := client.GetServiceSnapshot(context.Background(), maliciousId) + require.NoError(t, err) + + // The request body must remain valid JSON despite the embedded quotes... + var body struct { + Query string `json:"query"` + } + require.NoError(t, json.Unmarshal(capturedBody, &body), "request body is not valid JSON: %s", capturedBody) + + // ...and the id stays inside a single, escaped STQL string literal (no breakout). + assert.Equal(t, `(id = "1\") OR (1=1")`, body.Query) +} diff --git a/extservice/service_check.go b/extservice/service_check.go index 0b7e0a1..0c04c04 100644 --- a/extservice/service_check.go +++ b/extservice/service_check.go @@ -247,7 +247,7 @@ func MonitorStatusCheckStatus(ctx context.Context, state *ServiceStatusCheckStat func loadServiceComponent(ctx context.Context, state *ServiceStatusCheckState, api GetSnapshotApi) (*Component, error) { res, stackStateResponse, err := api.GetServiceSnapshot(ctx, state.ServiceId) if err != nil { - return nil, new(extension_kit.ToError(fmt.Sprintf("Failed to retrieve service states from StackState for Service ID %s. Full response: %v", state.ServiceId, res.String()), err)) + return nil, new(extension_kit.ToError(fmt.Sprintf("Failed to retrieve service states from StackState for Service ID %s.", state.ServiceId), err)) } if !res.IsSuccess() { log.Err(err).Msgf("StackState API responded with unexpected status code %d while retrieving service states for Service ID %s. Full response: %v", res.StatusCode(), state.ServiceId, res.String()) @@ -264,6 +264,9 @@ func loadServiceComponent(ctx context.Context, state *ServiceStatusCheckState, a Identifiers: []string{fmt.Sprintf("urn:service:/%s:%s:%s", state.ClusterName, state.ServiceName, state.ServiceId)}, }, nil } + if len(stackStateResponse.ViewSnapshotResponse.Components) == 0 { + return nil, new(extension_kit.ToError(fmt.Sprintf("StackState returned no components for Service ID %s.", state.ServiceId), nil)) + } return &stackStateResponse.ViewSnapshotResponse.Components[0], nil } @@ -282,7 +285,15 @@ func toMetric(service *Component, now time.Time) *action_kit_api.Metric { state = "danger" } - uiBaseUrl := config.Config.ApiBaseUrl[:(len(config.Config.ApiBaseUrl) - 3)] + uiBaseUrl := config.Config.ApiBaseUrl + if len(uiBaseUrl) >= 3 { + uiBaseUrl = uiBaseUrl[:len(uiBaseUrl)-3] + } + + serviceUrl := "" + if len(service.Identifiers) > 0 { + serviceUrl = fmt.Sprintf("%s/#/components/%s", uiBaseUrl, url.QueryEscape(service.Identifiers[0])) + } return new(action_kit_api.Metric{ Name: new("stackstate_service_status"), @@ -291,7 +302,7 @@ func toMetric(service *Component, now time.Time) *action_kit_api.Metric { attributeK8ServiceName: service.Name, attributeState: state, attributeTooltip: tooltip, - attributeUrl: fmt.Sprintf("%s/#/components/%s", uiBaseUrl, url.QueryEscape(service.Identifiers[0])), + attributeUrl: serviceUrl, }, Timestamp: now, Value: 0, diff --git a/extservice/service_discovery.go b/extservice/service_discovery.go index be12411..72b8934 100644 --- a/extservice/service_discovery.go +++ b/extservice/service_discovery.go @@ -10,6 +10,8 @@ package extservice import ( "context" "fmt" + "strings" + "github.com/go-resty/resty/v2" "github.com/rs/zerolog/log" "github.com/steadybit/discovery-kit/go/discovery_kit_api" @@ -105,7 +107,7 @@ func getAllServices(ctx context.Context, api GetSnapshotsApi) []discovery_kit_ap res, stackStateResponse, err := api.GetServiceSnapshots(ctx) if err != nil { - log.Err(err).Msgf("Failed to retrieve service states from Stack State. Full response: %v", res.String()) + log.Err(err).Msgf("Failed to retrieve service states from Stack State.") return result } @@ -127,8 +129,8 @@ func getAllServices(ctx context.Context, api GetSnapshotsApi) []discovery_kit_ap } func toService(service Component) discovery_kit_api.Target { - clusterName := service.Properties.ClusterNameIdentifier[len("urn:cluster:/kubernetes:"):] - namespace := service.Properties.NamespaceIdentifier[len(fmt.Sprintf("urn:kubernetes:/%v:namespace/", clusterName)):] + clusterName := strings.TrimPrefix(service.Properties.ClusterNameIdentifier, "urn:cluster:/kubernetes:") + namespace := strings.TrimPrefix(service.Properties.NamespaceIdentifier, fmt.Sprintf("urn:kubernetes:/%v:namespace/", clusterName)) return discovery_kit_api.Target{ Id: strconv.Itoa(service.Id), Label: service.Name,