feat(diagnostics): attach trace context to otel logs#70961
Conversation
🔒 Aisle Security AnalysisWe found 1 potential security issue(s) in this PR:
1. 🟡 Telemetry poisoning via untrusted trace context injection into OTEL log records
Description
This allows any code path that logs user-influenced objects (e.g., request payloads, webhook bodies, headers, plugin inputs) to inject attacker-chosen
Vulnerable code: const traceContext = findLogTraceContext(bindings, numericArgs);
...
if (traceContext?.spanId) {
logRecord.context = trace.setSpanContext(otelContextApi.active(), {
traceId: traceContext.traceId,
spanId: traceContext.spanId,
traceFlags: traceFlagsToOtel(traceContext.traceFlags),
isRemote: true,
});
}RecommendationTreat trace context as trusted-only and do not infer it from arbitrary log arguments. Options (choose one):
// Only accept from known binding key that OpenClaw sets (not from numeric args)
const traceContext = normalizeTraceContext(bindings?.__openclawTrace);
Additionally, consider a config flag to disable this behavior in shared/multi-tenant deployments. Analyzed PR: #70961 at commit Last updated on: 2026-04-24T06:39:14Z |
Greptile SummaryThis PR wires Confidence Score: 5/5Safe to merge; all remaining findings are minor style or test-coverage suggestions. No P0/P1 issues found. The validation logic is correct, the bitmask in traceFlagsToOtel is sound for the current W3C spec, and the truthiness checks on hex strings behave as intended. The three comments are P2 (attribute consistency, clarifying comment, and additional test coverage). No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/diagnostics-otel/src/service.ts
Line: 156-158
Comment:
**`openclaw.traceFlags` silently absent when flags are undefined**
When `traceContext.traceFlags` is `undefined` (valid — the field is optional in `DiagnosticTraceContext`), `addTraceAttributes` omits the `openclaw.traceFlags` attribute, but the OTEL log context is still attached below with a default of `TraceFlags.NONE`. An attribute consumer has no way to infer the effective sampling state from the attributes alone.
Consider emitting the attribute whenever a span context is attached, using the same default that `traceFlagsToOtel` already applies:
```typescript
if (traceContext.spanId) {
attributes["openclaw.traceFlags"] = traceContext.traceFlags ?? "00";
} else if (traceContext.traceFlags) {
attributes["openclaw.traceFlags"] = traceContext.traceFlags;
}
```
Or more simply, always write it (conditionally on spanId presence) so attributes stay consistent with the attached OTEL span context.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/diagnostics-otel/src/service.ts
Line: 135-140
Comment:
**`traceFlagsToOtel` bitmask can silently drop additional W3C flag bits**
`(parsed & TraceFlags.SAMPLED) === TraceFlags.SAMPLED ? TraceFlags.SAMPLED : TraceFlags.NONE` strips every bit except the sampled bit and returns a value that may not equal the original numeric flags. For the current W3C spec this is harmless (only bit 0 is defined), but it differs from the semantics of just casting with `parsed as TraceFlags`. A brief comment explaining the intentional bitmask behaviour (rather than a direct cast) would prevent future readers from "correcting" this into `parsed as TraceFlags` and silently forwarding unknown bits.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: extensions/diagnostics-otel/src/service.test.ts
Line: 348-388
Comment:
**No test coverage for trace context embedded directly in bindings or for unsampled flags**
The test exercises trace context nested under a `trace` key on a positional argument (`numericArgs[1].trace`). Two branches of `normalizeTraceContext`/`findLogTraceContext` aren't exercised:
1. Trace context supplied at the root of a positional arg (direct form: `{ traceId, spanId, traceFlags }` without the `trace` wrapper).
2. `traceFlags: "00"` (unsampled) — worth verifying that `openclaw.traceFlags` is still emitted (the non-empty string `"00"` is truthy so it should be, but it is a non-obvious path).
Not blocking, but these cases are easy to add alongside the existing test.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(diagnostics): attach trace context ..." | Re-trigger Greptile |
| if (traceContext.traceFlags) { | ||
| attributes["openclaw.traceFlags"] = traceContext.traceFlags; | ||
| } |
There was a problem hiding this comment.
openclaw.traceFlags silently absent when flags are undefined
When traceContext.traceFlags is undefined (valid — the field is optional in DiagnosticTraceContext), addTraceAttributes omits the openclaw.traceFlags attribute, but the OTEL log context is still attached below with a default of TraceFlags.NONE. An attribute consumer has no way to infer the effective sampling state from the attributes alone.
Consider emitting the attribute whenever a span context is attached, using the same default that traceFlagsToOtel already applies:
if (traceContext.spanId) {
attributes["openclaw.traceFlags"] = traceContext.traceFlags ?? "00";
} else if (traceContext.traceFlags) {
attributes["openclaw.traceFlags"] = traceContext.traceFlags;
}Or more simply, always write it (conditionally on spanId presence) so attributes stay consistent with the attached OTEL span context.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/diagnostics-otel/src/service.ts
Line: 156-158
Comment:
**`openclaw.traceFlags` silently absent when flags are undefined**
When `traceContext.traceFlags` is `undefined` (valid — the field is optional in `DiagnosticTraceContext`), `addTraceAttributes` omits the `openclaw.traceFlags` attribute, but the OTEL log context is still attached below with a default of `TraceFlags.NONE`. An attribute consumer has no way to infer the effective sampling state from the attributes alone.
Consider emitting the attribute whenever a span context is attached, using the same default that `traceFlagsToOtel` already applies:
```typescript
if (traceContext.spanId) {
attributes["openclaw.traceFlags"] = traceContext.traceFlags ?? "00";
} else if (traceContext.traceFlags) {
attributes["openclaw.traceFlags"] = traceContext.traceFlags;
}
```
Or more simply, always write it (conditionally on spanId presence) so attributes stay consistent with the attached OTEL span context.
How can I resolve this? If you propose a fix, please make it concise.| function traceFlagsToOtel(traceFlags: string | undefined): TraceFlags { | ||
| const parsed = Number.parseInt(traceFlags ?? "00", 16); | ||
| return (parsed & TraceFlags.SAMPLED) === TraceFlags.SAMPLED | ||
| ? TraceFlags.SAMPLED | ||
| : TraceFlags.NONE; | ||
| } |
There was a problem hiding this comment.
traceFlagsToOtel bitmask can silently drop additional W3C flag bits
(parsed & TraceFlags.SAMPLED) === TraceFlags.SAMPLED ? TraceFlags.SAMPLED : TraceFlags.NONE strips every bit except the sampled bit and returns a value that may not equal the original numeric flags. For the current W3C spec this is harmless (only bit 0 is defined), but it differs from the semantics of just casting with parsed as TraceFlags. A brief comment explaining the intentional bitmask behaviour (rather than a direct cast) would prevent future readers from "correcting" this into parsed as TraceFlags and silently forwarding unknown bits.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/diagnostics-otel/src/service.ts
Line: 135-140
Comment:
**`traceFlagsToOtel` bitmask can silently drop additional W3C flag bits**
`(parsed & TraceFlags.SAMPLED) === TraceFlags.SAMPLED ? TraceFlags.SAMPLED : TraceFlags.NONE` strips every bit except the sampled bit and returns a value that may not equal the original numeric flags. For the current W3C spec this is harmless (only bit 0 is defined), but it differs from the semantics of just casting with `parsed as TraceFlags`. A brief comment explaining the intentional bitmask behaviour (rather than a direct cast) would prevent future readers from "correcting" this into `parsed as TraceFlags` and silently forwarding unknown bits.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| }); | ||
|
|
||
| test("attaches diagnostic trace context to exported logs", async () => { | ||
| const emitCall = await emitAndCaptureLog({ | ||
| 0: '{"subsystem":"diagnostic"}', | ||
| 1: { | ||
| trace: { | ||
| traceId: TRACE_ID, | ||
| spanId: SPAN_ID, | ||
| traceFlags: "01", | ||
| }, | ||
| }, | ||
| 2: "traceable log", | ||
| _meta: { logLevelName: "INFO", date: new Date() }, | ||
| }); | ||
|
|
||
| expect(emitCall?.attributes).toMatchObject({ | ||
| "openclaw.traceId": TRACE_ID, | ||
| "openclaw.spanId": SPAN_ID, | ||
| "openclaw.traceFlags": "01", | ||
| }); | ||
| expect(telemetryState.tracer.setSpanContext).toHaveBeenCalledWith( | ||
| expect.anything(), | ||
| expect.objectContaining({ | ||
| traceId: TRACE_ID, | ||
| spanId: SPAN_ID, | ||
| traceFlags: 1, | ||
| isRemote: true, | ||
| }), | ||
| ); | ||
| expect(emitCall?.context).toEqual({ | ||
| spanContext: expect.objectContaining({ | ||
| traceId: TRACE_ID, | ||
| spanId: SPAN_ID, | ||
| }), | ||
| }); | ||
| }); | ||
|
|
||
| test("redacts sensitive reason in session.state metric attributes", async () => { | ||
| const service = createDiagnosticsOtelService(); |
There was a problem hiding this comment.
The test exercises trace context nested under a trace key on a positional argument (numericArgs[1].trace). Two branches of normalizeTraceContext/findLogTraceContext aren't exercised:
- Trace context supplied at the root of a positional arg (direct form:
{ traceId, spanId, traceFlags }without thetracewrapper). traceFlags: "00"(unsampled) — worth verifying thatopenclaw.traceFlagsis still emitted (the non-empty string"00"is truthy so it should be, but it is a non-obvious path).
Not blocking, but these cases are easy to add alongside the existing test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/diagnostics-otel/src/service.test.ts
Line: 348-388
Comment:
**No test coverage for trace context embedded directly in bindings or for unsampled flags**
The test exercises trace context nested under a `trace` key on a positional argument (`numericArgs[1].trace`). Two branches of `normalizeTraceContext`/`findLogTraceContext` aren't exercised:
1. Trace context supplied at the root of a positional arg (direct form: `{ traceId, spanId, traceFlags }` without the `trace` wrapper).
2. `traceFlags: "00"` (unsampled) — worth verifying that `openclaw.traceFlags` is still emitted (the non-empty string `"00"` is truthy so it should be, but it is a non-obvious path).
Not blocking, but these cases are easy to add alongside the existing test.
How can I resolve this? If you propose a fix, please make it concise.5e7af80 to
312565f
Compare
* 'main' of https://github.com/openclaw/openclaw: (521 commits) perf: slim reset model selection imports perf: slim directive parse test imports ci(release): clarify npm telegram approval label test: relax live tail readiness checks ci(release): use release approval for npm telegram e2e Project Codex hook notifications into agent events (openclaw#70969) fix: match codex verbose tool logs to pi (openclaw#70966) feat(diagnostics): attach trace context to otel logs (openclaw#70961) docs(faq): tighten FAQ stub pointers and Related cards docs(help): rewrite help index to match new tab structure, drop redundant troubleshooting H1 refactor(release): distill npm telegram docker runner ci(release): gate npm telegram e2e by release team ci(release): harden npm telegram beta e2e test(release): address npm telegram e2e review test(release): avoid docker argv secret values ci(release): add manual npm telegram beta e2e test(release): support convex npm telegram credentials docs: clarify private ws node setup fix: filter gateway node list locally feat(plugins): move Bonjour discovery into bundled plugin ...
* feat(diagnostics): attach trace context to otel logs * fix(diagnostics): satisfy trace flags lint
* feat(diagnostics): attach trace context to otel logs * fix(diagnostics): satisfy trace flags lint
* feat(diagnostics): attach trace context to otel logs * fix(diagnostics): satisfy trace flags lint
Summary
DiagnosticTraceContextfrom log bindings/metadata, exports explicit trace attributes, and attaches an OTEL log context when a span id is present.Change Type
Validation
pnpm test extensions/diagnostics-otel/src/service.test.tspnpm test extensions/diagnostics-otelpnpm format:check -- CHANGELOG.md extensions/diagnostics-otel/src/service.ts extensions/diagnostics-otel/src/service.test.tspnpm exec oxlint --tsconfig tsconfig.oxlint.extensions.json extensions/diagnostics-otel/src/service.ts extensions/diagnostics-otel/src/service.test.tsRisks and Mitigations