fix: validate session ID centrally in DispatchLifecycleEvent

0e9ba23→main·

Soph·1mo ago·2 files·+41 added/-0 removed

handleLifecycleTurnEnd builds .entire/metadata// via os.MkdirAll + os.WriteFile directly from the hook-supplied event.SessionID with no validation — unlike its siblings (SessionStart/TurnStart/ToolUse), which each validate. A "../"-laden ID could write the transcript outside the metadata directory. The ModelUpdate/Compaction/SubagentEnd handlers were also unguarded, surviving only because the strategy layer (MutateSessionState/StoreModelHint) validates internally.

Per-handler validation is the wrong altitude: it is exactly the kind of check a new (or existing) handler forgets, which is what happened here. Validate non-empty event.SessionID once in DispatchLifecycleEvent, before routing, so every handler — current and future — is covered uniformly. Empty IDs still pass through to each handler's own empty-handling (e.g. TurnEnd's fallback to the "unknown" constant).

The existing per-handler validations are now redundant but left in place as harmless defense-in-depth; they could be consolidated separately.

Taint is local hook input (same-privilege), so severity is low; this completes the hook-path hardening theme on this branch.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

Sessions

0a614e131babView transcript

Changes

2

• cmd/entire/cli

• Mlifecycle.go+13

• Mlifecycle_test.go+28

52 unmodified lines

53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71

52 unmodified lines

return errors.New("event cannot be nil")
    }

// Reject path-unsafe session IDs once, here, before any handler uses the ID
    // to build filesystem paths. Handlers historically validated individually,
    // which is fragile — handleLifecycleTurnEnd builds .entire/metadata/<id>/
    // via os.MkdirAll + os.WriteFile without its own check. Centralizing the
    // guard covers every handler (and any future one) uniformly. Empty IDs pass
    // through: handlers apply their own empty-handling (e.g. TurnEnd falls back
    // to a safe constant).
    if event.SessionID != "" {
        if err := validation.ValidateSessionID(event.SessionID); err != nil {
            return fmt.Errorf("invalid session ID in %s event: %w", event.Type, err)
        }
    }

// Filter forwarded hooks: when Cursor IDE forwards events to both
    // .cursor/hooks.json and .claude/settings.json, only the agent that owns
    // the session should process them — otherwise checkpoints, metadata

Mcmd/entire/cli/lifecycle.go+13

346 unmodified lines

347
348
349
350
351
352
353
354
355
356
357
358
359
360
361
362
363
364
365
366
367
368
369
370
371
372
373
374
375
376
377
378
379
380

346 unmodified lines

}
}

// TestDispatchLifecycleEvent_RejectsTraversalSessionID verifies the dispatcher
// rejects a path-unsafe session ID for every event type, before routing to a
// handler. This guards handlers that build filesystem paths from the ID without
// their own check (notably handleLifecycleTurnEnd's .entire/metadata/<id>/
// MkdirAll + WriteFile). The guard runs before any repo/FS access, so no repo
// setup is needed.
func TestDispatchLifecycleEvent_RejectsTraversalSessionID(t *testing.T) {
    t.Parallel()

ag := newMockAgent()
    for _, evType := range []agent.EventType{
        agent.TurnEnd, agent.ModelUpdate, agent.Compaction, agent.SubagentEnd, agent.SessionEnd,
    } {
        err := DispatchLifecycleEvent(context.Background(), ag, &agent.Event{
            Type:       evType,
            SessionID:  "../../etc/evil",
            SessionRef: "/dev/null",
            Model:      "x",
        })
        if err == nil {
            t.Fatalf("%v event with traversal session ID: got nil error, want rejection", evType)
        }
        if !strings.Contains(err.Error(), "invalid session ID") {
                 t.Errorf("%v event: error = %q, want \"invalid session ID\"", evType, err)
        }
    }
}

// --- handleLifecycleSessionStart tests ---

func TestHandleLifecycleSessionStart_EmptySessionID(t *testing.T) {