[HDX-4173] Redact sensitive fields from internal webhook API responses

[dhable]

Summary

Fixes a security vulnerability (CWE-522) where the internal GET /api/webhooks endpoint exposed sensitive credentials — webhook URLs with embedded tokens, auth headers, and API keys in query params — to any authenticated team member.

This applies the same redaction pattern already used by the external API (GET /api/v2/webhooks) to the internal API:

• URL masking: Webhook URLs are returned as ${origin}/****, hiding path segments that contain tokens (e.g., Slack webhook URLs)
• Header/queryParam redaction: Keys are preserved but all values are replaced with ****, so users can see what's configured without seeing the secrets
• Smart merge on PUT: When editing a webhook, redacted marker values (****) are resolved back to the stored originals; new values replace them; removed keys are deleted from storage
• UX indicators: The edit form shows helper text explaining that URLs and headers are masked for security

How to test locally or on Vercel

1. Create a Generic webhook with custom headers via the Team Settings → Integrations tab
2. Reload the page and verify the webhook URL is masked (https://example.com/****) and header values show ****
3. Click Edit on the webhook — verify the URL input and headers editor both show masked values
4. Change only the webhook name and save — verify the update succeeds and the original URL/headers are preserved (check the DB or re-fetch via API)
5. Edit again, change a header value to something new, and remove another header key — verify the new value is stored and the removed key is gone

References

• Linear Issue: https://linear.app/clickhouse/issue/HDX-4173
• Related PRs: None

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
hyperdx-oss	Ready	Preview, Comment	May 8, 2026 7:14pm

[changeset-bot]

🦋 Changeset detected

Latest commit: c93e4b7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
@hyperdx/api	Minor
@hyperdx/app	Minor
@hyperdx/otel-collector	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

E2E Test Results

✅ All tests passed • 167 passed • 3 skipped • 1212s

Status	Count
✅ Passed	167
❌ Failed	0
⚠️ Flaky	3
⏭️ Skipped	3

Tests ran across 4 shards in parallel.

View full report →

[github-actions]

🟡 Tier 3 — Standard

Introduces new logic, modifies core functionality, or touches areas with non-trivial risk.

Why this tier:

• Diff size: 264 production lines changed (Tier 2 max: < 250)
• Cross-layer change: touches frontend (packages/app) + backend (packages/api) + shared utils (packages/common-utils)
• Touches API routes or data models — hidden complexity risk

Review process: Full human review — logic, architecture, edge cases.
SLA: First-pass feedback within 1 business day.

Stats

• Production files changed: 4
• Production lines changed: 264 (+ 801 in test files, excluded from tier calculation)
• Branch: dan/hdx-4173-webhook-secret-fix
• Author: dhable

To override this classification, remove the review/tier-3 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

[github-actions]

PR Review

Thorough fix for the secret-exposure issue with strong defense-in-depth — redaction sentinel, URL/origin masking, exfil guards on URL change, scoped POST /test resolution, and a TOCTOU guard. Tests and changeset are comprehensive.

Notes (non-blocking)

• ⚠️ mergeRedactedMap quirk → when incoming is non-empty but every key is an orphaned **** (i.e., none match an existing key), it falls back to returning existing. That means submitting {nonexistent: '****'} against stored {a: 'real'} yields {a: 'real'} instead of {}. Documented in the comment as intentional, but worth a second look — a malformed UI roundtrip could silently revive cleared secrets. Consider returning undefined (or the empty result) so the user's submitted state wins.
• ⚠️ TOCTOU guard in findOneAndUpdate only conditions on url: existingPlain.url → concurrent PUTs that change headers but not URL can still overwrite each other (one client's merged headers built off stale existingPlain.headers). Acceptable trade-off vs. spurious 409s, but flagging.
• ⚠️ The redaction sentinel **** is treated as never-legitimate secret value (per the module comment). If a user genuinely uses **** as part of a real header value, round-tripping through edit will silently overwrite it with the stored value. Documented assumption — fine, but the helper text in WebhookForm.tsx could surface this caveat.
• ℹ️ WebhookForm doesn't expose queryParams editing → on a URL change, stored queryParams get $unset (intentional, matches the security model), but on the no-change path they're silently preserved. Minor UX gap, not introduced by this PR.
• ℹ️ The WebhookSchema description in common-utils/types.ts describes internal-API masking semantics, but the same schema is also referenced from the v2 external API. The doc-comment notes the divergence (separate schemas in packages/api/src/utils/zod.ts), which is good — just confirm consumers reading the type don't get misled by the masking description.

No critical bugs or security regressions identified.

[github-actions]

Deep Review

✅ No critical issues found. The redaction protocol's primary defenses — origin-only maskUrl, **** sentinel resolution on PUT, the URL-change exfiltration guard, the URL-match guard on POST /test, and the URL-conditioned findOneAndUpdate — were probed adversarially and held up. Findings below are about hardening, contract clarity, and test coverage of new branches.

🟡 P2 -- recommended

• packages/api/src/routers/api/webhooks.ts:73-95 -- mergeRedactedMap silently returns existing when every incoming key is an orphaned **** (key sent as redacted but absent from stored), so a client that POSTs {X-New: '****'} against {Authorization: 'real'} gets the stored entries back with no error and no test pinning the contract.
  • Fix: Either reject orphan-only submissions with a 400, or split the logic into explicit branches and add a unit test asserting the documented preserve-existing behavior.
  • _{correctness, testing, maintainability, kieran-typescript, adversarial}
• packages/api/src/routers/api/__tests__/webhooks.test.ts -- The new findOneAndUpdate({_id, team, url: existingPlain.url}) TOCTOU branch and its 409 response are not exercised by any test, so a regression dropping the url filter (or reverting to 404) would silently disable the URL-change race guard.
  • Fix: Add a test that mutates the stored URL between the findOne read and the findOneAndUpdate (via a spy or direct DB update) and asserts 409 with the concurrent-modification message.
  • _{testing, reliability, api-contract}
• packages/api/src/routers/api/webhooks.ts:376-379 -- The new inline z.string().refine(val => mongoose.Types.ObjectId.isValid(val)) duplicates the exported objectIdSchema already used elsewhere, violating the DRY rule called out in agent_docs/code_style.md.
  • Fix: Replace with webhookId: objectIdSchema.optional() imported from @/utils/zod.
  • _{project-standards}
• packages/common-utils/src/types.ts -- REDACTED_VALUE = '****' is now a load-bearing protocol constant for both server (webhooks.ts) and client (WebhookForm.tsx), but each side hardcodes the literal independently with no compile-time link.
  • Fix: Export WEBHOOK_REDACTED_VALUE = '****' from common-utils/src/types.ts next to WebhookSchema and import it in both call sites.
  • _{maintainability}
• packages/api/src/routers/api/webhooks.ts:323-330 -- PUT's "miss after update" path silently changed status from 404 to 409 with a new message, but the changeset doesn't note this contract shift for downstream API consumers that switch on status codes.
  • Fix: Add a CHANGELOG/changeset entry calling out the new 409 response, and grep external clients (ce-plugin/, any SDK) for handlers that branched on the prior 404.
  • _{api-contract, maintainability}
• packages/api/src/routers/api/webhooks.ts:60-71 -- toWebhookPlain and serializeWebhook both wrap doc.toJSON({flattenMaps: true}) to express slightly different projections via as casts, creating overlapping helpers and bypassing structural verification.
  • Fix: Collapse to one toWebhookJSON(doc): WebhookApiData helper that strips team/__v once, and let callers pick the fields they need.
  • _{maintainability, kieran-typescript}

🔵 P3 nitpicks (10)

• packages/api/src/routers/api/webhooks.ts:23-103 -- ~85 lines of private redaction helpers in the route file resist focused unit testing and push the file past the repo's 300-line guideline.
  • Fix: Extract maskUrl, redactMapValues, sanitizeWebhook, mergeRedactedMap, mapHasRedactedValues, emptyToUndefined, isMaskedUrl, and REDACTED_VALUE to webhookRedaction.ts and add a focused unit-test file.
  • _{maintainability, project-standards, kieran-typescript}
• packages/api/src/routers/api/webhooks.ts:60-71 -- as WebhookPlain / as WebhookApiData casts on the untyped doc.toJSON() result hide schema drift from the compiler, against the AGENTS.md preference for satisfies/inference.
  • Fix: Type the helper input as mongoose.HydratedDocument<IWebhook> or run the result through WebhookSchema.parse so divergence surfaces.
  • _{kieran-typescript, project-standards}
• packages/api/src/routers/api/webhooks.ts:294-317 -- updateOp: Record<string, unknown> and Record<string, 1> opt the security-critical PUT update out of Mongoose's typed UpdateQuery<IWebhook>, so a typo like headerss would not be caught.
  • Fix: Type updateOp as mongoose.UpdateQuery<IWebhook>.
• packages/api/src/routers/api/webhooks.ts:259-267 -- The exfiltration-guard 400 is exactly the signal a SOC would want, but nothing logs it for monitoring.
  • Fix: Emit logger.warn({teamId, webhookId: id, userId: req.user?._id}, 'Webhook exfiltration guard triggered') before returning 400.
• packages/api/src/routers/api/webhooks.ts:319-331 -- The 409 message "Webhook was modified concurrently. Please retry." overstates the guarantee, since the filter conditions only on url and concurrent header-only edits still silently lose-update.
  • Fix: Reword the message to "Webhook URL changed concurrently", or add __v to the filter for full optimistic concurrency.
  • _{correctness, reliability}
• packages/app/src/components/TeamSettings/WebhookForm.tsx:281 -- headersText?.includes('****') is a substring check on JSON text and fires for any literal **** inside a header value, producing a misleading "values are masked" hint.
  • Fix: Parse the field and check Object.values(parsed).includes('****'), or accept the false positive and document it as UX-only.
• packages/api/src/tasks/checkAlerts/template.ts:338-359 -- The Generic webhook fetch has no AbortSignal.timeout (pre-existing), and the new webhookId resolution path on POST /test makes the endpoint a more attractive target for slow-loris probing.
  • Fix: Pass signal: AbortSignal.timeout(15_000) to the outbound fetch.
• packages/api/src/routers/api/webhooks.ts:33-103 -- Helper naming uses four verbs (mask, redact, sanitize, serialize/toPlain, merge) for closely related steps in one protocol, forcing readers to re-derive which verb maps to which step.
  • Fix: Standardize on redact* for outbound and resolve* for inbound (e.g., mergeRedactedMap → resolveRedactedMap, sanitizeWebhook → redactWebhook).
• packages/app/src/components/TeamSettings/WebhookForm.tsx -- The new edit-mode description, masked-headers hint, and webhookId-on-test wiring have no Jest/RTL coverage; e2e covers only the masked-URL value in the input.
  • Fix: Add an RTL test for the isEditing branch covering the description copy, the conditional headers hint, and the useTestWebhook payload shape.
• packages/api/src/routers/external-api/v2/webhooks.ts:23 -- Pre-existing: logger.error({ webhook, ... }) logs the full Mongoose document (url/headers/queryParams) when external schema parsing fails, undermining the redaction posture this PR establishes elsewhere.
  • Fix: Redact the webhook payload before logging, or log only _id plus the parse error.

---

Reviewers (11): correctness, security, testing, maintainability, project-standards, agent-native, learnings-researcher, api-contract, reliability, adversarial, kieran-typescript.

Testing gaps:

• No test for the 409 TOCTOU branch on PUT.
• No test for mergeRedactedMap orphan-only incoming (all keys sent as **** but absent from stored).
• No test for isMaskedUrl round-trip when a stored URL legitimately ends in /****.
• No test for POST /test with a masked-shaped URL whose origin does not match stored (currently only a plain different-origin URL is covered).
• No Jest/RTL coverage for the new WebhookForm edit-mode UI changes or webhookId test wiring.

[dhable]

Deep Review Response

Addressed 13 of the 28 items (P0/P1 + P2 scope). Commit: c93e4b7

Fixed (10)

P0: duplicate-check bypass when service changes with masked URL (webhooks.ts:274)

Addressed: Duplicate check now always uses the submitted service instead of existingPlain.service. Prevents creating duplicate (team, service, url) tuples when changing service while keeping masked URL.

P0: mergeRedactedMap wipes stored headers when all incoming keys are orphaned (webhooks.ts:70)

Addressed: When all incoming keys are redacted-but-orphaned (key sent as **** but doesn't exist in stored data), the function now preserves existing entries instead of returning undefined → $unset. Only an explicit empty object {} clears everything.

P2: Empty-headers storage divergence between urlChanged branches (webhooks.ts:296)

Addressed: Added emptyToUndefined normalization in the urlChanged=true branch so headers: {} produces $unset on both code paths.

P2: TOCTOU between findOne and findOneAndUpdate (webhooks.ts:231)

Addressed: findOneAndUpdate now conditions on url: existingPlain.url. If a concurrent PUT changes the URL between read and write, the query mismatches and returns 409 Conflict.

P2: as WebhookApiData casts (webhooks.ts:115)

Addressed: Created serializeWebhook helper that strips team/__v via destructuring and replaces all three as WebhookApiData casts.

P2: Local WebhookPlain type widens service (webhooks.ts:60)

Addressed: Replaced with Pick<WebhookApiData, 'url' | 'headers' | 'queryParams' | 'service'> so the WebhookService enum flows through.

P2: WebhookTestApiResponse | { message: string } misleading union (webhooks.ts:372)

Addressed: Dropped the union — WebhookTestApiResponse is already { message: string }.

P2: queryParams rejection test only asserts 400 (webhooks.test.ts:1216)

Addressed: Added expect(response.body.message).toMatch(/Cannot preserve masked secrets/) and verification that stored URL/queryParams are unchanged, mirroring the headers test.

P2: Cross-team 404 tests don't assert spies never called (webhooks.test.ts:1180)

Addressed: Added expect(genericSpy).not.toHaveBeenCalled() and expect(slackSpy).not.toHaveBeenCalled() to both 404 tests.

P2: Leak assertion not.toContain('services') too weak (webhooks.test.ts:621)

Addressed: Now asserts not.toBe(MOCK_WEBHOOK.url) and not.toContain('XXXXXXXXXXXXXXXXXXXXXXXX') (the actual token segment).

Fixed differently (1)

P2: WebhookForm masked hint reads prop, not form state (WebhookForm.tsx:345)

Fixed differently: Added useWatch for the headers field and derived hasMaskedHeaders = isEditing && headersText?.includes('****'). The hint now disappears when the user clears or replaces masked values in the editor, rather than persisting based on the original webhook prop.

Replied (2)

P2: WebhookSchema doesn't distinguish masked vs real values (common-utils/types.ts:271)

Added .describe() annotations to url, headers, and queryParams fields documenting the sentinel contract. Full schema split (WebhookResponseSchema/WebhookInputSchema) is premature abstraction at this point — the descriptions surface the contract in generated types without adding maintenance overhead.

P2: Changeset uses minor for documented breaking change

Updated: Removed "Breaking:" prefix. This is a security fix — these values should have always been redacted. Kept as minor.

Declined (4)

P3: isMaskedUrl strict equality is UX-surprising (webhooks.ts:248)

Declined: Strict comparison is the safer default. URL canonicalization (new URL().href on both sides) risks introducing subtle comparison bugs (port normalization, trailing slashes, encoding). Treating ambiguous cases as URL changes forces secret re-entry, which is the correct security posture.

P3: Form should warn when URL change drops queryParams (WebhookForm.tsx:147)

Declined: The form doesn't expose queryParams editing at all. Adding a warning for an invisible field would confuse users without actionable recourse.

P3: webhookId stringly-typed in frontend (api.ts:497)

Declined: Server-side Zod validation catches invalid ObjectIds. A shared schema for a single optional field is premature abstraction.

P3: POST /test allows team-member spam via webhookId (webhooks.ts:436)

Declined: Team members testing team webhooks is by design — this is team-level access control. Rate limiting is a separate concern.

Not in scope — track as follow-up (3)

• v2 API returns URL in plaintext (zod.ts:521) — different surface, different security model
• v2 missing POST/PUT/DELETE/test (external-api/v2/webhooks.ts:212) — feature request
• No AbortController timeout on outbound fetch (webhooks.ts:436) — pre-existing, not introduced by this PR

Remaining P3 items (deferred by scope decision)

Items #16–29 (file extraction, REDACTED_VALUE to common-utils, reject literal ****, narration comments, == null cleanup, e2e shared constant, URL masking in logs, direct unit tests) were assessed as valid but deferred per scope decision to focus on P0/P1 + P2.

Validation: make ci-lint passed, make ci-unit passed (1553/1553).