fix: harden trace context propagation and collector ingest#622
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8071edf50f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(value) > 256 { | ||
| return false |
There was a problem hiding this comment.
When an upstream sends tracestate: vendor= or a value that becomes empty after trimming, this validator returns true because it only rejects values over 256 bytes. ParseTraceContext then preserves and Inject re-emits a malformed W3C tracestate instead of dropping invalid vendor state as the new propagation contract documents; add an empty-value/non-blank-final-character check before accepting the member value.
Useful? React with 👍 / 👎.
| if !requestHasJSONContentType(request) { | ||
| collector.rejectHTTP(response, "unsupported media type", http.StatusUnsupportedMediaType) |
There was a problem hiding this comment.
When generated apps mount this hardened viewer, the runtime contract changed: browser POST now rejects non-JSON here and the /data//events viewer branches below are GET-only. The generated security manifest still declares trace.browser with ContentTypeRequired: "" and trace.data/trace.events with GET, POST in internal/securitymanifest/manifest.go, so gowdk audit can keep reporting audit_observability_content_type_missing while security tooling sees POST surfaces that now return 405. Update those manifest entries to match the enforced JSON ingest and GET-only stream/data routes.
Useful? React with 👍 / 👎.
| if err != nil || parsed.Scheme == "" || parsed.Host == "" { | ||
| return false | ||
| } | ||
| return strings.EqualFold(parsed.Scheme, requestScheme(request)) && strings.EqualFold(parsed.Host, request.Host) |
There was a problem hiding this comment.
When the viewer/browser ingest is mounted behind an HTTPS-terminating reverse proxy with an app-owned access gate, Go receives request.TLS == nil and an empty URL.Scheme, while the browser sends Origin: https://host. This comparison treats the request as http and returns 403 for externally same-origin browser spans; add a trusted external scheme/origin option or trusted-proxy handling before enforcing the scheme comparison.
Useful? React with 👍 / 👎.
1 participant
Summary
tracestatethrough extract/start/inject while rejecting malformed or oversizedtraceparentand dropping invalidtracestate.Root Cause
traceparent, so valid vendor state was lost and header parsing had no explicit byte limits.Compatibility / Public Contracts
trace.TraceContextnow includesTraceState.trace.ParseTraceContext(traceparent, tracestate)is added;ParseTraceparentremains compatible.trace.NewCollector(limit, options...)remains source-compatible with existingNewCollector(limit)callers.rejectedalongsidespansanddropped.Issue Closure
Closes #552
Closes #553
Related: #551
Related: #538
Related: #520
Verification
scripts/test-go-modules.shwhen Go code or compiler behavior changed.go build ./cmd/gowdkwhen CLI, compiler, runtime, addon, or release behavior changed.Commands run:
LLM Assistance
addons/observabilityas the addon name; close only#552and#553; leave full observability expansion and realtime audience scoping to follow-up issues.#538, and decide or build the broader metrics/log correlation surface under#551.