diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f21ab4b8d..2e2bc40361 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,10 @@ For details about compatibility between different releases, see the **Commitment - The timestamp of the udp packet is now always correct when the 'Schedule downlink late' is enabled for the gateway and downlink scheduling hits the duty cycle limit. +### Security + +- HTML-escape gRPC error messages and error detail attributes to prevent reflected XSS in API error responses. + ## [3.35.2] - 2026-01-30 ### Changed diff --git a/pkg/rpcserver/rpcserver.go b/pkg/rpcserver/rpcserver.go index 5352b43f1f..c91a494bd9 100644 --- a/pkg/rpcserver/rpcserver.go +++ b/pkg/rpcserver/rpcserver.go @@ -18,6 +18,7 @@ package rpcserver import ( "context" "fmt" + "html" "math" "net/http" "net/textproto" @@ -50,6 +51,7 @@ import ( _ "google.golang.org/grpc/encoding/gzip" // Register gzip compression. "google.golang.org/grpc/keepalive" "google.golang.org/grpc/metadata" + grpcstatus "google.golang.org/grpc/status" ) func init() { @@ -132,6 +134,31 @@ func WithRateLimiter(limiter ratelimit.Interface) Option { // ErrRPCRecovered is returned when a panic is caught from an RPC. var ErrRPCRecovered = errors.DefineInternal("rpc_recovered", "Internal Server Error") +// sanitizingHTTPErrorHandler wraps runtime.DefaultHTTPErrorHandler to HTML-escape +// gRPC status messages before they are serialized to JSON. This prevents reflected +// XSS from user-supplied input that flows into error messages (e.g., field mask paths, +// strconv.ParseBool parse errors, route-not-found paths). +// +// Note: ErrorDetailsToProto separately sanitizes error attributes in the details +// payload. This handler covers the top-level "message" field which contains the +// formatted error string with raw attribute values. +func sanitizingHTTPErrorHandler( + ctx context.Context, + mux *runtime.ServeMux, + marshaler runtime.Marshaler, + w http.ResponseWriter, + r *http.Request, + err error, +) { + if st, ok := grpcstatus.FromError(err); ok { + // Rebuild the status with escaped message while preserving details. + pb := st.Proto() + pb.Message = html.EscapeString(pb.Message) + err = grpcstatus.ErrorProto(pb) + } + runtime.DefaultHTTPErrorHandler(ctx, mux, marshaler, w, r, err) +} + // New returns a new RPC server with a set of middlewares. // The given context is used in some of the middlewares, the given server options are passed to gRPC // @@ -234,7 +261,7 @@ func New(ctx context.Context, opts ...Option) *Server { server.ServeMux = runtime.NewServeMux( runtime.WithMarshalerOption("*", jsonpb.TTN()), runtime.WithMarshalerOption("text/event-stream", jsonpb.TTNEventStream()), - runtime.WithErrorHandler(runtime.DefaultHTTPErrorHandler), + runtime.WithErrorHandler(sanitizingHTTPErrorHandler), runtime.WithMetadata(func(ctx context.Context, req *http.Request) metadata.MD { md := rpcmetadata.MD{ Host: req.Host, diff --git a/pkg/rpcserver/rpcserver_test.go b/pkg/rpcserver/rpcserver_test.go index 86b6a83c41..2d88ffe104 100644 --- a/pkg/rpcserver/rpcserver_test.go +++ b/pkg/rpcserver/rpcserver_test.go @@ -16,6 +16,10 @@ package rpcserver_test import ( "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" "runtime" "testing" "time" @@ -173,3 +177,38 @@ func (s *mockServer) DownlinkQueuePush(ctx context.Context, req *ttnpb.DownlinkQ s.pushCtx, s.pushReq = ctx, req return ttnpb.Empty, nil } + +func TestSanitizingHTTPErrorHandler(t *testing.T) { + t.Parallel() + a := assertions.New(t) + ctx := test.Context() + + logHandler := memory.New() + logger := log.NewLogger(logHandler) + ctx = log.NewContext(ctx, logger) + + server := rpcserver.New(ctx) + + // Request a non-existent gRPC-gateway route with XSS payload in the path. + xssPath := `/api/v3/` + r := httptest.NewRequest(http.MethodGet, xssPath, nil) + r.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + + server.ServeHTTP(rec, r) + + res := rec.Result() + body, _ := io.ReadAll(res.Body) + + // Verify the response is valid JSON with no unescaped script tags. + var resp map[string]any + a.So(json.Unmarshal(body, &resp), should.BeNil) + + // Check the message field if present. + if msg, ok := resp["message"].(string); ok { + a.So(msg, should.NotContainSubstring, "` + + errWithXSS := errDef.WithAttributes( + "path", xssPayload, + "paths", []string{xssPayload, "safe"}, + "count", 42, + ) + + pb := ttnpb.ErrorDetailsToProto(errWithXSS) + a.So(pb, should.NotBeNil) + + // Recover attributes from the proto to verify sanitization. + attrs, err := goproto.Map(pb.GetAttributes()) + a.So(err, should.BeNil) + + // String attribute must be escaped. + a.So(attrs["path"], should.NotContainSubstring, "` + + r := httptest.NewRequest(http.MethodGet, xssPath, nil) + rec := httptest.NewRecorder() + + NotFound(rec, r) + + res := rec.Result() + body, _ := io.ReadAll(res.Body) + + a.So(res.StatusCode, should.Equal, http.StatusNotFound) + + // Decode the JSON to inspect the attribute value after JSON decoding. + var resp map[string]any + a.So(json.Unmarshal(body, &resp), should.BeNil) + + // Extract the route attribute from the error details. + details, _ := resp["details"].([]any) //nolint:revive + a.So(len(details), should.BeGreaterThan, 0) + detail, _ := details[0].(map[string]any) //nolint:revive + attrs, _ := detail["attributes"].(map[string]any) //nolint:revive + route, _ := attrs["route"].(string) //nolint:revive + + // The attribute value must be HTML-escaped, not the raw XSS payload. + a.So(route, should.NotContainSubstring, "