fix(security): remove sensitive data from logs and observability#1729
fix(security): remove sensitive data from logs and observability#1729alex-alecu wants to merge 7 commits intomainfrom
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)No new issues found in the incremental diff since Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (3 files)
Reviewed by gpt-5.4-20260305 · 158,159 tokens |
The deploy builder only handles internal service-to-service requests authenticated via BACKEND_AUTH_TOKEN, not user credentials. Keeping PII capture enabled here preserves debugging context without risk.
Next.js uses moduleResolution: "node" which doesn't resolve package.json subpath exports. Re-export from the main entrypoint so the Next.js app can import it.
| '{{method}}': request.method, | ||
| '{{path}}': request.path, | ||
| '{{headers}}': JSON.stringify(request.headers, null, 2), | ||
| '{{headers}}': JSON.stringify(redactSensitiveHeaders(request.headers), null, 2), |
There was a problem hiding this comment.
This call doesn't pass extraHeaders, so custom webhook auth headers (e.g. x-webhook-secret) won't be redacted at this layer. Currently mitigated because TriggerDO.captureRequest redacts before DB storage, and the queue consumer reads from DB — so headers arriving here are already scrubbed.
Worth either a code comment explaining why extraHeaders isn't needed here, or threading the webhook auth header through for defense-in-depth. If a future caller passes raw headers directly to renderPromptTemplate, the custom auth header would leak into the LLM prompt.
| if (process.env.NODE_ENV !== 'development') { | ||
| logExceptInTest(`headers (${traceability_logging_id})`, Object.fromEntries(headers.entries())); | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: removing the header logging block left an extra blank line here.
| 'set-cookie', | ||
| 'x-gitlab-token', | ||
| 'x-hub-signature', | ||
| 'x-hub-signature-256', |
There was a problem hiding this comment.
Consider adding proxy-authorization to the list — it's a standard HTTP header that carries credentials, same pattern as authorization.
2 participants
Summary
Authentication tokens, credentials, and webhook secrets were being leaked through multiple paths: logged to console (forwarded to Sentry), stored in the database, interpolated into LLM prompts, and sent to Sentry through
sendDefaultPiiandattachRpcInput.Why
sendDefaultPiiandattachRpcInputare dangeroussendDefaultPiitells Sentry: "attach all request headers, cookies, and IP addresses to every error you capture." The problem is it grabs everything — it can't tell the difference between a harmlessContent-Typeheader and a secretAuthorization: Bearer <jwt>header. So when a user's request causes an error, their login token ends up sitting in Sentry where anyone with dashboard access can see it.attachRpcInputdoes the same thing but for tRPC calls — it sends the full input of every procedure to Sentry. Some procedures accept personal access tokens as input, so those tokens end up in Sentry too.Both are now disabled for services that handle user credentials (Next.js app, mobile app). The deploy builder keeps
sendDefaultPiienabled because it only handles internal service-to-service requests with a shared backend token — no user secrets flow through it.What changed
sendDefaultPiiin the Next.js client and mobile app Sentry configsattachRpcInputin the tRPC Sentry middlewaresendDefaultPiienabled for the deploy builder (internal service, no user credentials)redactSensitiveHeadersutility in@kilocode/worker-utilsthat strips auth-bearing headers (including custom webhook auth headers) before storage or prompt injectionWhat we lose for debugging
Most changes have little impact. Here's a honest look at what gets harder:
tokens.tsattachRpcInput: falsesendDefaultPii: false(Next.js, mobile)x-hub-signature-256are replaced with[REDACTED]. All other headers (event type, delivery ID, content-type) are kept.{ email: "..." }anyway.If
attachRpcInput: falsebecomes painful: the targeted fix is to add abeforeSendhook in Sentry that scrubs known sensitive fields from tRPC inputs instead of turning off capture entirely.Verification
pnpm typecheck— passes (all packages)pnpm testinpackages/worker-utils— 45/45 tests pass (includes 7 redact-headers tests)pnpm format:check— passespnpm test— all failures are pre-existing DB connection errors (role "postgres" does not exist), unrelated to this changeVisual Changes
N/A
Reviewer Notes
consoleLoggingIntegrationinsentry.server.config.tsandsentry.edge.config.tswas intentionally left unchanged — once the sources (tokens.ts) stop emitting sensitive data, the integration no longer amplifies secrets.Headersclass are always lowercase, butredactSensitiveHeadersdoes case-insensitive matching to handle the TriggerDO case where headers come from a custom record.