Skip to content

B2: scrub Sentry events and add per-request scope tags#403

Open
MrTravisB wants to merge 1 commit into
travis/pilo-request-loggingfrom
travis/pilo-sentry-scrubber
Open

B2: scrub Sentry events and add per-request scope tags#403
MrTravisB wants to merge 1 commit into
travis/pilo-request-loggingfrom
travis/pilo-sentry-scrubber

Conversation

@MrTravisB
Copy link
Copy Markdown
Collaborator

Summary

Second PR in Stack B (server observability). Stacked on #402 (B1).

Configures Sentry's beforeSend and beforeBreadcrumb hooks to scrub
potentially sensitive content out of every event/breadcrumb that reaches
the transport, and adds a per-request middleware that tags the active
scope with bounded metadata so events are filterable by taskId, route,
method, and status.

What gets scrubbed (beforeSend)

  • request.data (request body)
  • request.query_string (URL query - leaks tokens / search terms)
  • request.cookies
  • Authorization, Cookie, Set-Cookie headers (case-insensitive)
  • exception.value reduced to exception.type (drops error.message,
    which can quote task text or page content)
  • exception.stacktrace (frames can include selectors derived from page
    content via Playwright)
  • contexts.response.body
  • Top-level event.extra (arbitrary attached data)

What gets scrubbed (beforeBreadcrumb)

  • All console breadcrumbs are dropped (verbose, often contain user content)
  • breadcrumb.data is filtered to an allowlist of bounded keys: taskId,
    method, route, status, phase, reason, result, provider,
    model, iteration, tool, error_class, duration_ms. Anything
    else is dropped.

What gets tagged (sentryContext middleware)

After every request: sets method, route (matched pattern), status,
and taskId (when the response carries x-pilo-task-id) on the
request-scoped Toucan instance. No request-derived strings.

Defense in depth

Stack A's helpers (createErrorResponse, errorResponseFromError,
recordSanitizedException) prevent leaks at the Pilo code layer. These
scrubbers are the boundary layer for the Sentry transport, catching
anything that leaks through future regressions, third-party middleware,
or Sentry's own auto-instrumentation (which captures request.data and
unhandled exceptions by default).

Changes

  • packages/server/src/middleware/sentryScrubber.ts - scrubBeforeSend,
    scrubBeforeBreadcrumb pure functions
  • packages/server/src/middleware/sentryContext.ts - per-request scope
    tags via getSentry(c)
  • packages/server/src/index.ts - wire scrubbers into the sentry() init,
    register sentryContext() middleware after requestLog()
  • packages/server/package.json - add @sentry/types as devDependency
    (types-only; the runtime types come from toucan-js transitively)
  • 20 new tests across sentryScrubber.test.ts and sentryContext.test.ts
    including sentinel canaries on every scrub path

Deferred

Task lifecycle breadcrumbs (task.start, task.iteration_start,
tool.call, error.recoverable_caught) are deferred to a follow-up PR.
The scrubber + tags are valuable on their own (they sanitize what Sentry
already auto-captures), and breadcrumbs require touching routes and
the agent event flow - better as a focused follow-up than bundled here.

Test plan

  • pnpm --filter pilo-server run test (114 passing: 94 prior + 20 new)
  • pnpm --filter pilo-core run test (still green)
  • pnpm run typecheck green
  • pnpm run format:check green
  • node scripts/check-dep-versions.mjs confirms shared dep alignment

Notes

  • Base: travis/pilo-request-logging (stacks on B1: structured request access log middleware #402)
  • B3 next: concurrency limit + 429 backpressure
  • B2's value depends on SENTRY_DSN being set in deployed environments -
    if not set today, this PR is preparation work, silent until DSN is wired

Copy link
Copy Markdown
Collaborator

@lmorchard lmorchard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, too

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants