diff --git a/internal/cli/run.go b/internal/cli/run.go index 8316761095..42f03edcdd 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -580,13 +580,33 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep } } - // Trace identity (ADR 0050 Level 1 + security correlation). Generated here, - // before the pre-script, so TRACEPARENT can propagate to child processes. - // The same id is reused as the security finding/audit trace id (dashed UUID) - // and, dash-stripped, as the W3C telemetry trace id — one id across both. - securityTraceID := security.GenerateTraceID() - wTraceID := telemetry.TraceIDFromUUID(securityTraceID) - rootSpanID := telemetry.NewSpanID() + // Trace identity (ADR 0050 Level 1 + security correlation). If an inbound + // TRACEPARENT is present (nested/instrumented invocation), adopt its + // trace-id and create a child span to preserve the distributed trace chain + // and respect the upstream sampling decision (W3C trace-flags). Otherwise, + // generate a fresh trace-id. The same id is reused as the security + // finding/audit trace id (dashed UUID) and, dash-stripped, as the W3C + // telemetry trace id — one id across both. + var securityTraceID string + var wTraceID string + var rootSpanID string + var traceFlags string + + if inbound := os.Getenv("TRACEPARENT"); inbound != "" { + if parentTraceID, _, flags, ok := telemetry.ParseTraceParent(inbound); ok { + wTraceID = parentTraceID + securityTraceID = telemetry.UUIDFromTraceID(parentTraceID) + rootSpanID = telemetry.NewSpanID() + traceFlags = flags + } + } + if wTraceID == "" { + // No valid inbound TRACEPARENT; generate fresh ids. + securityTraceID = security.GenerateTraceID() + wTraceID = telemetry.TraceIDFromUUID(securityTraceID) + rootSpanID = telemetry.NewSpanID() + traceFlags = "01" + } workItemID := resolveWorkItemID() // 2c. Run pre-script on the host (if configured). @@ -594,7 +614,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep preStart := time.Now() printer.StepStart("Running pre-script: " + h.PreScript) preCmd := exec.Command(h.PreScript) - preCmd.Env = childScriptEnv(h.RunnerEnv, telemetry.TraceParent(wTraceID, rootSpanID)) + preCmd.Env = childScriptEnv(h.RunnerEnv, telemetry.TraceParentWithFlags(wTraceID, rootSpanID, traceFlags)) preCmd.Stdout = os.Stdout preCmd.Stderr = os.Stderr if err := preCmd.Run(); err != nil { @@ -664,7 +684,7 @@ func runAgent(ctx context.Context, agentName, fullsendDir, outputBase, targetRep printer.StepStart("Running post-script: " + h.PostScript) postCmd := exec.Command(h.PostScript) postCmd.Dir = runDir - postCmd.Env = childScriptEnv(h.RunnerEnv, telemetry.TraceParent(wTraceID, rootSpanID)) + postCmd.Env = childScriptEnv(h.RunnerEnv, telemetry.TraceParentWithFlags(wTraceID, rootSpanID, traceFlags)) postCmd.Stdout = os.Stdout postCmd.Stderr = os.Stderr if err := postCmd.Run(); err != nil { @@ -1650,10 +1670,19 @@ func telemetryExitCode(lastExitCode int, runErr error) int { // childScriptEnv builds the environment for a host-side child script (pre- or // post-script): the harness RunnerEnv layered over the process environment, -// plus the W3C TRACEPARENT for trace propagation (ADR 0050 Level 1). An empty -// traceparent (telemetry disabled) is omitted rather than emitted blank. +// plus the W3C TRACEPARENT for trace propagation (ADR 0050 Level 1). Any +// pre-existing TRACEPARENT in the process environment is filtered out so +// exactly one entry is present. An empty traceparent (telemetry disabled) is +// omitted rather than emitted blank. func childScriptEnv(runnerEnv map[string]string, traceparent string) []string { - env := append(os.Environ(), envToList(runnerEnv)...) + base := os.Environ() + env := make([]string, 0, len(base)+len(runnerEnv)+1) + for _, e := range base { + if !strings.HasPrefix(e, "TRACEPARENT=") { + env = append(env, e) + } + } + env = append(env, envToList(runnerEnv)...) if traceparent != "" { env = append(env, "TRACEPARENT="+traceparent) } @@ -1809,9 +1838,11 @@ func refreshOIDCToken(ctx context.Context, sandboxName, oidcURL, oidcAuth string // inside the sandbox. It finds known context files (including SKILL.md in // skill directories) in the repo directory and passes them as arguments. func buildScanContextCommand(repoDir, traceID string) string { - // Defense-in-depth: validate traceID before shell interpolation even though - // GenerateTraceID() only produces safe hex characters. - if !security.IsValidTraceID(traceID) { + // Defense-in-depth: validate traceID before shell interpolation. Uses + // IsShellSafeTraceID (not IsValidTraceID) because the trace ID may have + // been adopted from an inbound W3C traceparent rather than generated by + // GenerateTraceID, so it may not be UUID v4. + if !security.IsShellSafeTraceID(traceID) { // Should never happen with internal generation, but fail safely. traceID = "invalid-trace-id" } @@ -2148,10 +2179,10 @@ func scanOutputFiles(outputDir, traceID string, printer *ui.Printer) error { // injectTraceID appends the FULLSEND_TRACE_ID to the sandbox .env file. func injectTraceID(sandboxName, traceID string) error { - if !security.IsValidTraceID(traceID) { + if !security.IsShellSafeTraceID(traceID) { return fmt.Errorf("invalid trace ID format: %q", traceID) } - // Safe: IsValidTraceID() above ensures traceID matches UUID v4 format only. + // Safe: IsShellSafeTraceID() above ensures traceID is only hex+dashes. cmd := fmt.Sprintf("echo 'export FULLSEND_TRACE_ID=%s' >> %s/.env", traceID, sandbox.SandboxWorkspace) _, _, _, err := sandbox.Exec(sandboxName, cmd, 10*time.Second) return err diff --git a/internal/cli/telemetry_run_test.go b/internal/cli/telemetry_run_test.go index 2563885259..eeeba5bdab 100644 --- a/internal/cli/telemetry_run_test.go +++ b/internal/cli/telemetry_run_test.go @@ -114,6 +114,23 @@ func TestChildScriptEnv_AppendsTraceparentOnce(t *testing.T) { assert.True(t, hasMarker, "process environment must be preserved") } +func TestChildScriptEnv_FiltersPreExistingTraceparent(t *testing.T) { + // Simulate a parent process that already exports TRACEPARENT. + t.Setenv("TRACEPARENT", "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1-bbbbbbbbbbbbbbbb-01") + const fullsendTP = "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01" + + env := childScriptEnv(map[string]string{}, fullsendTP) + + traceparents := 0 + for _, e := range env { + if strings.HasPrefix(e, "TRACEPARENT=") { + traceparents++ + assert.Equal(t, "TRACEPARENT="+fullsendTP, e, "must use fullsend's value, not the parent's") + } + } + assert.Equal(t, 1, traceparents, "exactly one TRACEPARENT entry after dedup") +} + func TestChildScriptEnv_EmptyTraceparentOmitted(t *testing.T) { env := childScriptEnv(map[string]string{"FOO": "bar"}, "") for _, e := range env { @@ -121,6 +138,48 @@ func TestChildScriptEnv_EmptyTraceparentOmitted(t *testing.T) { } } +func TestChildScriptEnv_EmptyTraceparentFiltersExisting(t *testing.T) { + // When telemetry is disabled (empty traceparent), pre-existing TRACEPARENT + // from the process environment should still be filtered out. + t.Setenv("TRACEPARENT", "00-aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa1-bbbbbbbbbbbbbbbb-01") + + env := childScriptEnv(map[string]string{}, "") + for _, e := range env { + assert.False(t, strings.HasPrefix(e, "TRACEPARENT="), "pre-existing TRACEPARENT must be filtered even when disabled") + } +} + +// TestInboundTraceparentAdoption verifies the trace-id adoption logic: when a +// valid inbound TRACEPARENT is present, the security trace-id is derived from +// it (not freshly generated), the W3C trace-id matches the inbound parent, and +// the trace-flags (including the sampled bit) are preserved. +func TestInboundTraceparentAdoption(t *testing.T) { + const inbound = "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-00" + + parentTraceID, _, flags, ok := telemetry.ParseTraceParent(inbound) + require.True(t, ok, "inbound traceparent must parse") + + // Adopted W3C trace-id must match the inbound parent. + assert.Equal(t, "4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", parentTraceID) + + // Security trace-id derived from inbound must round-trip. + securityID := telemetry.UUIDFromTraceID(parentTraceID) + assert.Equal(t, "4f3a9c1b-2d8e-4a7c-9f0b-1e2d3c4a5b6d", securityID) + assert.Equal(t, parentTraceID, telemetry.TraceIDFromUUID(securityID), "round-trip must preserve trace-id") + + // Adopted security trace-id must be shell-safe (may not be UUID v4). + assert.True(t, security.IsShellSafeTraceID(securityID), "adopted trace-id must be shell-safe") + + // Trace-flags must be preserved (unsampled=00 in this case). + assert.Equal(t, "00", flags, "unsampled flag must be preserved") + + // Child traceparent uses parent's trace-id + new span + parent's flags. + childSpan := telemetry.NewSpanID() + childTP := telemetry.TraceParentWithFlags(parentTraceID, childSpan, flags) + assert.Contains(t, childTP, parentTraceID, "child traceparent must contain parent's trace-id") + assert.True(t, strings.HasSuffix(childTP, "-00"), "child traceparent must preserve unsampled flag") +} + func TestAgentSpanEndAttrs(t *testing.T) { var m agentruntime.RunMetrics m.Model = "claude-opus-4-6" diff --git a/internal/security/trace.go b/internal/security/trace.go index fb13e6852c..27bd60e029 100644 --- a/internal/security/trace.go +++ b/internal/security/trace.go @@ -31,6 +31,20 @@ func IsValidTraceID(id string) bool { return reTraceID.MatchString(id) } +// reShellSafeTraceID matches any dashed-hex string in UUID format (8-4-4-4-12). +// Unlike reTraceID it does not require UUID v4 version/variant bits, so it +// accepts trace IDs adopted from an inbound W3C traceparent. +var reShellSafeTraceID = regexp.MustCompile(`^[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}$`) + +// IsShellSafeTraceID returns true if the trace ID consists only of lowercase +// hex characters and dashes in UUID format — safe for shell interpolation. +// It accepts any UUID variant, not just v4. Use this when the trace ID may +// have been adopted from an inbound W3C traceparent rather than generated +// internally by GenerateTraceID. +func IsShellSafeTraceID(id string) bool { + return reShellSafeTraceID.MatchString(id) +} + // seedHash is the well-known genesis hash for the first entry in a chain. const seedHash = "0000000000000000000000000000000000000000000000000000000000000000" diff --git a/internal/security/trace_test.go b/internal/security/trace_test.go index 089cca9fa9..3dc004986a 100644 --- a/internal/security/trace_test.go +++ b/internal/security/trace_test.go @@ -14,6 +14,33 @@ func TestGenerateTraceID(t *testing.T) { } } +func TestIsShellSafeTraceID(t *testing.T) { + // UUID v4 (generated by GenerateTraceID) passes both validators. + id := GenerateTraceID() + if !IsShellSafeTraceID(id) { + t.Errorf("generated trace ID %q should be shell-safe", id) + } + + // Non-v4 UUID from an adopted W3C trace-id: passes shell-safe but not v4. + adopted := "4f3a9c1b-2d8e-0a7c-1f0b-1e2d3c4a5b6d" // version 0, variant 1 + if !IsShellSafeTraceID(adopted) { + t.Error("adopted non-v4 trace ID should be shell-safe") + } + if IsValidTraceID(adopted) { + t.Error("adopted non-v4 trace ID should NOT pass strict v4 validation") + } + + // Reject non-hex characters. + if IsShellSafeTraceID("zzzzzzzz-zzzz-zzzz-zzzz-zzzzzzzzzzzz") { + t.Error("non-hex characters must be rejected") + } + + // Reject wrong length. + if IsShellSafeTraceID("4f3a9c1b-2d8e") { + t.Error("short ID must be rejected") + } +} + func TestAppendFindingHashChain(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "findings.jsonl") diff --git a/internal/telemetry/trace.go b/internal/telemetry/trace.go index 6061caaa75..19b9b3dd1f 100644 --- a/internal/telemetry/trace.go +++ b/internal/telemetry/trace.go @@ -33,6 +33,57 @@ func TraceParent(traceID, spanID string) string { return "00-" + traceID + "-" + spanID + "-01" } +// TraceParentWithFlags is like TraceParent but uses the given trace-flags +// (2-hex-character string) instead of the default "01". This preserves the +// upstream sampling decision when propagating an inbound traceparent. +func TraceParentWithFlags(traceID, spanID, flags string) string { + return "00-" + traceID + "-" + spanID + "-" + flags +} + +// ParseTraceParent parses a W3C traceparent header (version 00) and returns +// the trace-id, parent span-id, and trace-flags. ok is false if the header +// is malformed, uses an unsupported version, or contains invalid ids. +func ParseTraceParent(tp string) (traceID, spanID, flags string, ok bool) { + parts := strings.Split(tp, "-") + if len(parts) != 4 || parts[0] != "00" { + return "", "", "", false + } + traceID, spanID, flags = parts[1], parts[2], parts[3] + if len(traceID) != 32 || traceID == "00000000000000000000000000000000" { + return "", "", "", false + } + if len(spanID) != 16 || spanID == "0000000000000000" { + return "", "", "", false + } + if len(flags) != 2 { + return "", "", "", false + } + // Validate all components are lowercase hex. + for _, s := range []string{traceID, spanID, flags} { + for _, c := range s { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return "", "", "", false + } + } + } + return traceID, spanID, flags, true +} + +// UUIDFromTraceID converts a 32-hex-character W3C trace-id into dashed UUID +// format (8-4-4-4-12). Returns empty string if the input is not exactly 32 +// lowercase hex characters. +func UUIDFromTraceID(traceID string) string { + if len(traceID) != 32 { + return "" + } + for _, c := range traceID { + if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')) { + return "" + } + } + return traceID[0:8] + "-" + traceID[8:12] + "-" + traceID[12:16] + "-" + traceID[16:20] + "-" + traceID[20:32] +} + // randRead is a seam over crypto/rand.Read so the RNG-failure fallback in // randomHex is testable. var randRead = rand.Read diff --git a/internal/telemetry/trace_test.go b/internal/telemetry/trace_test.go index c5fb6f75af..6ab30cb859 100644 --- a/internal/telemetry/trace_test.go +++ b/internal/telemetry/trace_test.go @@ -84,3 +84,70 @@ func TestTraceParent(t *testing.T) { require.Equal(t, "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01", got) assert.Regexp(t, reTraceparent, got) } + +func TestTraceParentWithFlags(t *testing.T) { + got := TraceParentWithFlags("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", "a1b2c3d4e5f60718", "00") + assert.Equal(t, "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-00", got, "unsampled flag preserved") + + got = TraceParentWithFlags("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", "a1b2c3d4e5f60718", "01") + assert.Equal(t, "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01", got, "sampled flag preserved") +} + +func TestParseTraceParent(t *testing.T) { + tests := []struct { + name string + input string + wantTID string + wantSID string + wantF string + wantOK bool + }{ + { + name: "valid sampled", + input: "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01", + wantTID: "4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", + wantSID: "a1b2c3d4e5f60718", + wantF: "01", + wantOK: true, + }, + { + name: "valid unsampled", + input: "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-00", + wantTID: "4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", + wantSID: "a1b2c3d4e5f60718", + wantF: "00", + wantOK: true, + }, + {name: "wrong version", input: "01-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-01"}, + {name: "empty", input: ""}, + {name: "too few parts", input: "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718"}, + {name: "all-zero trace-id", input: "00-00000000000000000000000000000000-a1b2c3d4e5f60718-01"}, + {name: "all-zero span-id", input: "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-0000000000000000-01"}, + {name: "uppercase hex", input: "00-4F3A9C1B2D8E4A7C9F0B1E2D3C4A5B6D-a1b2c3d4e5f60718-01"}, + {name: "short trace-id", input: "00-4f3a9c1b2d8e4a7c-a1b2c3d4e5f60718-01"}, + {name: "short flags", input: "00-4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d-a1b2c3d4e5f60718-1"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tid, sid, f, ok := ParseTraceParent(tc.input) + assert.Equal(t, tc.wantOK, ok, "ok mismatch") + if tc.wantOK { + assert.Equal(t, tc.wantTID, tid) + assert.Equal(t, tc.wantSID, sid) + assert.Equal(t, tc.wantF, f) + } + }) + } +} + +func TestUUIDFromTraceID(t *testing.T) { + got := UUIDFromTraceID("4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d") + assert.Equal(t, "4f3a9c1b-2d8e-4a7c-9f0b-1e2d3c4a5b6d", got) + + // Round-trip: TraceIDFromUUID(UUIDFromTraceID(x)) == x + assert.Equal(t, "4f3a9c1b2d8e4a7c9f0b1e2d3c4a5b6d", TraceIDFromUUID(got)) + + // Invalid inputs + assert.Equal(t, "", UUIDFromTraceID("tooshort")) + assert.Equal(t, "", UUIDFromTraceID("4F3A9C1B2D8E4A7C9F0B1E2D3C4A5B6D")) // uppercase +}