Skip to content

server: inbound traceparent reaches the context unvalidated #1100

Description

@gaborage

The HTTP door is the one ingress ADR-070 did not close. server/trace_context.go:18-19 plants the inbound traceparent in the context with no validation:

if tp := req.Header.Get(gobrickshttp.HeaderTraceParent); tp != "" {
    ctx = gobrickshttp.WithTraceParent(ctx, tp)
}

enrichTraceContext is wired unconditionally on the default chain (server/middleware.go), so this is live on every GoBricks HTTP service. The messaging lanes, the outbox relay and the exported extractor all route through trace.ExtractFromHeaders, which validates; the HTTP path reads req.Header.Get directly. (The X-Request-ID half of the HTTP door is validated, at server/handler.go getTraceID.)

Where the raw value goes. It is stored in ctx, persisted by the outbox (outbox/publisher.go → the relay puts it back in opts.Headers, where computeTraceParent reads it first, unvalidated), and re-emitted on every outbound AMQP publish and outbound HTTP request.

Second, independent hole. trace.extractTraceIDFromParent checks len(parts) >= 4 && len(parts[1]) == 32 — length, not charset. forceAlignTraceID consumes computeTraceParent's output, which is never validated, so a traceparent whose trace-id field is 32 arbitrary printable bytes aligns onto that value and it becomes the outbound X-Request-ID. Go's h1 server rejects control bytes in a header value, so CR/LF cannot get through, but all printable punctuation and high bytes can.

Observed effect since #1099. The publish path's CorrelationId is now the aligned id, and trace.ValidateRequestID refuses a non-hex one — so a remote caller can blank CorrelationId on every message a service publishes while handling their request (documented in migration atom C60.10). Nothing loses correlation, but it is remote-triggerable and free.

Shape of a fix. gobrickstrace.ValidateTraceParent(tp) in enrichTraceContext is one line and closes the ingress. Requiring 32 hex digits in extractTraceIDFromParent is the belt to that braces, and additionally closes PublishOptions.Headers["traceparent"], which no ingress guard covers. Both are silent-behavior changes (an invalid inbound traceparent stops propagating) and want a migration atom.

Found during the security gate on #1099; deliberately out of that PR's scope, as recorded in #1066's brief.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area/serverAffects the server packagebugSomething isn't workingready-for-agentFully specified, ready for an AFK agent

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions