fix: prevent STQL injection and reachable panics in service check/discovery

[joshiste]

Addresses the stackstate audit MAJOR findings.

1. STQL / JSON injection (MAJOR)

GetServiceSnapshot interpolated the service id raw into an STQL expression (id = "<id>"), and executeSnapshotQuery then hand-built the JSON request body with "query": "%v". A service id (a target attribute) containing quotes/backslashes could break out of the STQL string literal and/or corrupt the JSON body.

Fix:

• stqlString(serviceId) renders the id as an escaped string literal (via JSON string escaping) so it can't break out of the STQL literal.
• executeSnapshotQuery now JSON-encodes the query value into the body ("query": %s) instead of raw interpolation. The static metadata envelope is unchanged (it has no dynamic values — confirmed the body template has exactly one format verb).

2. Reachable panics (MAJOR)

• Components[0] indexed without an empty check → now returns a clear error when StackState returns no components.
• service.Identifiers[0] indexed without a check → now guarded (URL attribute degrades to empty).
• ApiBaseUrl[:len-3] could underflow on a short URL → now length-guarded.
• service_discovery.toService stripped the cluster/namespace URN prefixes with manual slice offsets that panic on an unexpected identifier (crashing the background discovery refresh) → now strings.TrimPrefix.
• res.String() was called on the transport-error path where resty can return a nil response → removed there (kept on the !IsSuccess() path where res is non-nil).

Testing

• go build ./..., go vet, gofmt clean.
• New extservice/common_test.go: stqlString escaping, and a malicious service id (1") OR (1=1) keeps the request body valid JSON and stays inside a single escaped STQL literal.
• go test ./extservice/ passes (existing mocked tests + new ones).

/simplify

4-agent pass: reuse/simplification/efficiency clean. Altitude confirmed the body template has no other dynamic value (injection fully closed) and the other guards are right-depth. Two notes:

• Softened the stqlString comment to not overclaim general STQL escape-semantics (the ids in practice are numeric/URN, so it's effective regardless).
• Follow-up (not in this PR): the ApiBaseUrl[:len-3] chop is kept behavior-preserving (length-guarded) here, but it leaves a latent issue — for the documented …/api base URL it strips to …/ and the built UI link gets a double slash. A cleaner fix is strings.TrimSuffix(ApiBaseUrl, "/api"). Left out to avoid a behavior change in this security PR; happy to do it as a follow-up.

[claude]

Claude finished @joshiste's task in 2m 2s —— View job

---

PR Review

• Gather context and read changed files
• Check for previously unresolved review threads (none found)
• Review new changes
• Post final summary

---

Summary

The fixes are correct and well-scoped. No prior review threads to resolve.

STQL/JSON injection fix (common.go)

stqlString uses json.Marshal to produce a properly-quoted string literal. executeSnapshotQuery now JSON-encodes the entire query string before splicing it into the body template. Both layers close the injection correctly. The test validates the end-to-end path with a payload designed to escape the STQL literal.

One minor note: json.Marshal on a Go string never returns an error (invalid UTF-8 is replaced, not rejected), so the if err != nil guard on lines 52–54 of common.go is unreachable dead code. Not a bug, just slightly misleading. Leaving it out or using //nolint would be cleaner, but it's harmless as-is.

Panic guards (service_check.go, service_discovery.go)

• Components[0] → guarded with a length check, returns a clear error. ✓
• service.Identifiers[0] → guarded, serviceUrl falls back to "". ✓
• ApiBaseUrl[:len-3] → length-guarded against underflow. ✓
• Manual slice offsets in toService → strings.TrimPrefix is correct and panic-safe. ✓
• res.String() on the err != nil transport-error path → removed in both service_check.go (commit 55c6f2a) and service_discovery.go (commit 5a7608e, the follow-up commit). ✓

Tests (common_test.go)

Good coverage: unit test for stqlString and an integration-style test that confirms the captured request body is valid JSON and the malicious ID doesn't escape the STQL literal.

No blocking issues. The PR is well-reasoned and addresses all five audit findings cleanly.

[joshiste]

Good catch — fixed in 5a7608e. getAllServices in service_discovery.go had the same res.String()-on-transport-error nil-deref that the service check did; I'd missed it (and the PR description over-claimed). Dropped res.String() from that error-path log too. The non-error StatusCode() != 200 branch keeps it (there res is non-nil).

The other two notes are intentionally left as-is: stqlString ignores the json.Marshal error because marshaling a string cannot fail, and the ApiBaseUrl TrimSuffix("/api") cleanup is tracked as a follow-up (kept behavior-preserving here to avoid a URL-construction change in a security fix).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
87.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud