Repository navigation
fix(processor,security): stop /health from exposing raw SIEM webhook error bodies (#989) - #1972
Conversation
…error bodies (#989) The unauthenticated processor /health endpoint surfaced siem.sources.<source>.lastError, which was populated verbatim from the SIEM delivery worker and could contain raw, customer-controlled webhook response bodies (stack traces, auth tokens, PII, schema). In cloud mode this is a cross-tenant info-disclosure surface; even on-prem it aids downstream-SIEM fingerprinting. Fix, following a zero-trust + pure-function shape: - Classify at the write boundary: siem-adapter now stamps a safe, closed-union DeliveryErrorClass (transport_error, http_client_error, http_server_error, ssrf_blocked, invalid_config, chain_tamper, preflight_unavailable, internal_error, unclassified_error) on every failed SiemDeliveryResult. The raw error string is kept only for logging. - Make illegal states unrepresentable: recordError's signature changes from message: string to errorClass: DeliveryErrorClass, so it is now a compile-time error to persist a raw body into the /health-visible cursor from any call site. - Read-time zero-trust guard: siem-health-builder allowlists lastError against the safe classes; anything else (e.g. a legacy raw-body row already at rest) collapses to 'unclassified_error'. null is preserved so deriveStatus still reports 'error' only when one occurred. Full raw error detail is retained in the processor's stdout logs at each write site for operator triage, so no authenticated detail endpoint is added and no data migration is required. Tests: adapter error-class mapping (+ raw text still rides on `error` for logging), worker persists only the safe class and never the raw body, health builder redacts unknown values, and an end-to-end /health test proving a cursor row containing a secret surfaces as 'unclassified_error' with the secret absent from the response. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVkZaY1ZKwYq5K3HkgZ6A1
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughSIEM delivery failures now receive safe classifications, workers persist those classes instead of raw error text, and ChangesSafe SIEM error handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SIEMAdapter
participant DeliveryWorker
participant CursorDatabase
participant HealthBuilder
participant HealthEndpoint
SIEMAdapter->>DeliveryWorker: return errorClass and raw error
DeliveryWorker->>CursorDatabase: persist errorClass
HealthEndpoint->>HealthBuilder: buildSiemHealth()
HealthBuilder->>CursorDatabase: read cursor lastError
HealthBuilder-->>HealthEndpoint: safe lastError classification
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
) Derive both the DeliveryErrorClass union and the SAFE_DELIVERY_ERROR_CLASSES allowlist from one `as const` array, so a new class can't be added to one without the other. Also drop the duplicated allowlist literal in the server test mock by spreading the real siem-adapter module via importOriginal. No behavior change; 221 processor tests still pass, typecheck + lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVkZaY1ZKwYq5K3HkgZ6A1
…sses; derive retryable from class (#989) Proactive hardening from a self-review pass: - siem-adapter: derive sendWebhook's `retryable` flag from classifyHttpStatus (5xx/429 → http_server_error → retryable) so the class and retryability can't drift; remove the duplicated status check. - Tests: assert errorClass on the three previously-unasserted paths — syslog SSRF (ssrf_blocked), syslog validation-throw (transport_error), deliverToSiem misconfig (invalid_config), and the worker top-level catch (internal_error, never the raw message). All nine DeliveryErrorClass values are now covered. 221 processor tests pass; typecheck + lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AVkZaY1ZKwYq5K3HkgZ6A1
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 52 minutes. |
Closes #989.
Problem
The processor's
GET /healthendpoint is unauthenticated by design (k8s liveness probe) but surfacedsiem.sources.<source>.lastError, which was populated verbatim from the SIEM delivery worker asHTTP <status>: <full response.text()>. If a customer's SIEM receiver returns a verbose error (stack trace,invalid token: abc123…, schema, user IDs), that content became readable by anyone who can reach the health endpoint. Incloudmode where processors share infrastructure this is a cross-tenant information-disclosure surface; even on-prem it aids downstream-SIEM fingerprinting.Fix — zero-trust + pure functions
The untrusted external body must never cross into the persisted/exposed domain. Enforced at three points:
siem-adapter.ts) — a closed unionDeliveryErrorClass(transport_error,http_client_error,http_server_error,ssrf_blocked,invalid_config,chain_tamper,preflight_unavailable,internal_error,unclassified_error) is stamped on every failedSiemDeliveryResult. A pureclassifyHttpStatus()handles the 4xx/5xx split. The rawerrorstring is kept for logging only.siem-delivery-worker.ts) —recordError's signature changed frommessage: stringtoerrorClass: DeliveryErrorClass. It is now a compile-time error to persist a raw body into the/health-visible cursor from any call site.siem-health-builder.ts) —cursorToPerSourceallowlistslastErroragainst the safe classes; anything else (e.g. a legacy raw-body row already at rest in the DB) collapses tounclassified_error.nullis preserved soderiveStatusstill reportserroronly when one occurred — this neutralizes pre-existing rows with no migration.Full raw error detail is retained in the processor's stdout logs at every write site for operator triage.
Deliberately out of scope
/health/detailendpoint — raw bodies already reach stdout at all write sites; a re-exposure endpoint just relocates the hazard behind an auth check that can be misconfigured./siem/receiptsdoes not read the column).Acceptance criteria
siem.sources.<source>.lastErroron unauthenticated/healthnever contains customer-controlled strings./healtheven when the cursor row contains them.Testing
bun run --filter=@pagespace/processor typecheck✓ (therecordErrorsignature change proves at compile time no call site can persist free text)bun run --filter=@pagespace/processor lint✓/healthtest proving a cursor row containingsk-live-abc123surfaces asunclassified_errorwith the secret absent from the response JSON.🤖 Generated with Claude Code
Summary by CodeRabbit
Security Improvements
Reliability