Skip to content

feat: Adding remaining API v2 endpoints - saved searches & webhooks#2586

Open
jordan-simonovski wants to merge 8 commits into
mainfrom
jordansimonovski/crud-saved-search-webhook
Open

feat: Adding remaining API v2 endpoints - saved searches & webhooks#2586
jordan-simonovski wants to merge 8 commits into
mainfrom
jordansimonovski/crud-saved-search-webhook

Conversation

@jordan-simonovski

@jordan-simonovski jordan-simonovski commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds bearer-auth CRUD to the External API v2 for two resources so a provider (clickstack) can manage them programmatically:

  • Saved searches — new /api/v2/saved-searches router (list / get / create / update / delete), team-scoped.
  • Webhooks — upgrades /api/v2/webhooks from list-only to full CRUD (adds POST / PUT / DELETE).

Why

The v2 external API previously had no way to manage saved searches at all (/api/v2/search only executes a query), and webhooks were read-only. OSS parity only needs the read-only webhook data source, but the provider integration needs to manage both as first-class resources. The webhook CRUD is specifically what unblocks the clickstack_webhook resource (HDX-4490).

Alerts v2 already had full CRUD, so it's untouched.

What changed

  • routers/external-api/v2/savedSearches.ts (new) — team-scoped CRUD. Request/response use sourceId (not the internal source) for a consistent external contract; a requireValidSourceId middleware rejects a sourceId that isn't a source owned by the caller's team (prevents cross-team references). Reuses the existing savedSearch controllers and the model's toExternalJSON().
  • routers/external-api/v2/webhooks.ts — adds POST / PUT / DELETE. PUT is a full replace ($set present fields, $unset omitted/empty ones). Duplicate service+name → 400.
  • utils/zod.ts — adds externalWebhookCreateSchema + HTTP header validators for the request body.
  • controllers/savedSearch.ts — deleteSavedSearch now returns the deleted doc so the router can distinguish 404 from success (the only other caller ignored the return).
  • index.ts — mounts the new router behind validateUserAccessKey + rate limiter.
  • Integration tests for both routers.

Security / sensitive data

Webhook headers and queryParams are write-only: accepted on create/update but never returned by any read endpoint (externalWebhookSchema omits them), so auth tokens and other secrets don't leak. All operations are team-scoped (create/read/update/delete all filter by team); request bodies can't spoof team/_id (Zod strips unknown keys, team is set server-side).

Testing

  • make dev-int FILE=savedSearches-crud → 12/12 pass
  • make dev-int FILE=external-api/tests/webhooks → 21/21 pass
  • tsc --noEmit clean; yarn lint:fix clean (warnings pre-existing)

Coverage includes team-scoping/404 isolation, sourceId ownership validation, duplicate rejection, and assertions that webhook secrets are never returned on create.

Follow-up (not in this PR)

MCP agent tools don't yet cover the new write paths (webhook create/update/delete, saved-search delete) — tracked in HDX-4700.

Changeset

Included (@hyperdx/api minor).

resolves hdx-4673 hdx-4674

Saved searches and webhooks are the last API endpoints remaining. This
frees us up to finalise the tf provider build
@changeset-bot

changeset-bot Bot commented Jul 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 783b94e

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

@vercel

vercel Bot commented Jul 3, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
hyperdx-oss Ready Ready Preview, Comment Jul 4, 2026 12:29am
hyperdx-storybook Ready Ready Preview, Comment Jul 4, 2026 12:29am

Request Review

@github-actions github-actions Bot added the review/tier-4 Critical — deep review + domain expert sign-off label Jul 3, 2026
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

🔴 Tier 4 — Critical

Touches auth, data models, config, tasks, OTel pipeline, ClickHouse, or CI/CD.

Why this tier:

  • Critical-path files (4):
    • packages/api/src/routers/external-api/v2/alerts.ts
    • packages/api/src/routers/external-api/v2/index.ts
    • packages/api/src/routers/external-api/v2/savedSearches.ts
    • packages/api/src/routers/external-api/v2/webhooks.ts
  • Cross-layer change: touches frontend (packages/app) + backend (packages/api)

Review process: Deep review from a domain expert. Synchronous walkthrough may be required.
SLA: Schedule synchronous review within 2 business days.

Stats
  • Production files changed: 14
  • Production lines changed: 2377 (+ 818 in test files, excluded from tier calculation)
  • Branch: jordansimonovski/crud-saved-search-webhook
  • Author: jordan-simonovski

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

@greptile-apps

greptile-apps Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds full CRUD to two previously read-only (or absent) external API v2 resources: a new /api/v2/saved-searches router and an upgraded /api/v2/webhooks router, plus shared pagination infrastructure that is back-applied to alerts.

  • Saved searches — new team-scoped CRUD with a requireValidSourceId middleware that prevents cross-team source references, full-replace PUT semantics with documented defaults, and a cascade delete that removes dependent alerts before the parent (improving on the prior findOneAndDelete ordering).
  • Webhooks — POST/PUT/DELETE added; the PUT implements a security-conscious secret-preservation scheme where omitted headers/queryParams are preserved unless the destination (url or service) changes (preventing token forwarding to a new endpoint), and concurrent modifications are detected with a 409 via an OCC-style filter on the snapshotted url/service.
  • Pagination — a shared pagination.ts utility (limit/offset, max 1000) and matching { team, _id } compound indexes on all three models; existing alert and webhook endpoints updated to use it with meta envelope in responses.

Confidence Score: 5/5

Safe to merge. The new endpoints are well team-scoped, write-only secrets are never echoed back on reads, and the concurrent-modification edge case in webhook PUT is handled with a 409 rather than silently proceeding.

The implementation is careful throughout: Zod strips unknown keys so team/_id cannot be spoofed in request bodies, sourceId ownership is validated before write, the PUT's destination-change detection prevents secret forwarding, and the cascade-delete ordering leaves the system in a recoverable state on partial failure. Integration tests cover team isolation, duplicate rejection, secret non-exposure, and pagination. The one non-blocking observation (meta.total skew when a malformed webhook exists in the DB) is a documented edge case that requires direct DB corruption to trigger.

No files require special attention.

Important Files Changed

Filename Overview
packages/api/src/routers/external-api/v2/savedSearches.ts New CRUD router for saved searches — team-scoped, paginated list, sourceId ownership validation via middleware, full-replace PUT semantics with documented defaults. Well-structured and follows existing patterns.
packages/api/src/routers/external-api/v2/webhooks.ts Extends existing list-only webhook router with POST/PUT/DELETE. PUT has well-designed secret-preservation logic: omitted write-only headers/queryParams are preserved unless the destination changes, preventing token forwarding to new destinations. The 409 conflict path for concurrent modifications is handled correctly.
packages/api/src/controllers/savedSearch.ts deleteSavedSearch refactored from atomic findOneAndDelete to findOne+deleteOne, reversing the cascade order so alerts are deleted before the parent. Now returns the document so callers can distinguish 404 from success.
packages/api/src/utils/pagination.ts New shared pagination utility with limit/offset schema (max 1000), getPagination helper, and paginationMeta that logs a warning when the default max limit silently truncates results.
packages/api/src/utils/zod.ts Added externalWebhookCreateSchema and extracted HTTP header/query-param validators as shared exports to eliminate duplication between internal and external webhook routers.
packages/api/src/routers/external-api/v2/alerts.ts Adds pagination to the list endpoint, improves 403/404 responses from sendStatus to json bodies, and fixes delete to return 404 when the alert doesn't exist.
packages/api/src/models/webhook.ts Adds compound index { team: 1, _id: 1 } to support the paginated list query's sort-by-_id without an in-memory sort.
packages/api/src/routers/external-api/tests/webhooks.test.ts Adds POST/PUT/DELETE test coverage including secret non-exposure assertions, header/queryParam injection rejection, duplicate handling, concurrent modification (409), and meta.total/data skew documentation.

Reviews (6): Last reviewed commit: "hardening webhooks according to P2 feedb..." | Re-trigger Greptile

Comment thread packages/api/src/routers/external-api/v2/webhooks.ts Outdated
Comment thread packages/api/src/routers/external-api/v2/webhooks.ts Outdated
Comment thread packages/api/src/routers/external-api/v2/savedSearches.ts Outdated
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

review content below

@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 222 passed • 3 skipped • 1536s

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

Tests ran across 4 shards in parallel.

View full report →

@jordan-simonovski jordan-simonovski changed the title feat: Adding remaining API v2 endpoints feat: Adding remaining API v2 endpoints - saved searches & webhooks Jul 3, 2026
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Deep Review

No critical issues found. Correctness and security both traced the hardened paths — team-scoping on every query, write-only headers/queryParams never returned on read, the destinationChanged snapshot-pinned findOneAndUpdate, and Zod coercion/stripping of pagination and body inputs — and confirmed no auth bypass, secret leak, injection, or happy-path crash introduced by this diff. The remaining items are recommendations.

🟡 P2 — recommended

  • packages/api/src/routers/external-api/v2/webhooks.ts:611 — the 409 concurrent-modification branch (the enforcement mechanism for the destination-change secret-clearing fix) has zero test coverage, so a regression that reopens the secret-forwarding race would not fail any test.
    • Fix: Add a test that makes findOneAndUpdate return null while the document still exists under a changed url/service and assert a 409 response.
    • testing, correctness, maintainability, kieran-typescript, project-standards
  • packages/api/src/utils/zod.ts:735externalWebhookCreateSchema and savedSearchRequestSchema (savedSearches.ts:27) impose no .max() on strings/arrays, unlike the rest of zod.ts (capped at 10000), so an authenticated key can POST a multi-MB field that bloats storage under 16MB or 500s above it; offset in pagination.ts is likewise uncapped.
    • Fix: Add .max() bounds to the new string/array fields matching the existing schemas, and cap offset with a .max() or clamp it against total.
    • adversarial
  • packages/api/src/routers/external-api/v2/alerts.ts:719DELETE /api/v2/alerts/:id now returns 404 for a missing alert where it previously always returned 200, a runtime behavior change for existing integrations that delete idempotently.
    • Fix: Flag the 200→404 delete change in release notes and add a regression test asserting the new not-found behavior.
    • api-contract, kieran-typescript, project-standards
  • packages/api/src/utils/pagination.ts:1 — the find().sort({_id:1}).skip().limit() + countDocuments pattern (plus its stability comment) is duplicated verbatim across the webhooks router, saved-searches router, and alerts controller, with divergent placement.
    • Fix: Extract a shared paginatedFind(model, filter, {limit, offset}) helper returning [docs, total] and call it from all three list paths.
    • maintainability
  • packages/api/src/routers/external-api/v2/webhooks.ts:419 — the new webhook create/update/delete and saved-search delete HTTP endpoints have no corresponding MCP agent tools, leaving agents able to create resources they cannot remove (the author tracks this as a follow-up).
    • Fix: Add the missing agent write tools (or confirm the follow-up ticket fully scopes webhook CRUD and saved-search delete parity).
    • agent-native
  • packages/api/src/utils/zod.ts:730 — the external write API now lets a team bearer token set an arbitrary webhook url validated only by z.string().url(), while the delivery-time guard relies on a weak host denylist (pre-existing sink), so a key holder can point a generic webhook at internal/metadata endpoints.
    • Fix: Reject private/loopback/link-local hosts at both create/update validation and delivery time via a resolved-IP allowlist/denylist.
    • security
  • packages/api/src/controllers/sources.ts:46getSource catches all errors and returns null, so a transient DB failure during the newly-added saved-search create/update surfaces as a misleading 400 sourceId must be an existing source id instead of a 5xx (pre-existing behavior, now on a write path).
    • Fix: Narrow the catch to cast/invalid-id errors and rethrow other failures so requireValidSourceId forwards them as a 5xx.
    • reliability
🔵 P3 nitpicks (6)
  • packages/app/src/components/DBSearchPageFilters/hooks.ts:401 — removes an eslint-disable for react-hooks/set-state-in-effect above a still-present setState call, reintroducing a lint warning unrelated to this PR's scope.
    • Fix: Revert this line or move it to a separate change.
    • correctness, project-standards
  • packages/api/src/utils/pagination.ts:44 — the truncation logger.warn is unthrottled, so a polling client on a >1000-row collection emits a warning on every list request.
    • Fix: Dedupe/rate-limit the warning per team+resource or emit a metric instead.
    • reliability
  • packages/api/src/controllers/savedSearch.ts:86 — replacing findOneAndDelete with findOne + deleteOne makes delete non-atomic: two concurrent DELETEs can both return 200 (previously the loser got 404), and a mid-operation failure can leave alerts deleted while the search remains; the tradeoff is documented but the contract change is untested.
    • Fix: Document the double-200 idempotency as intended and add a concurrent-delete test, or restore an atomic guard.
    • kieran-typescript, adversarial, correctness, reliability
  • packages/api/src/routers/external-api/v2/webhooks.ts:557 — the PUT update operation is typed Record<string, unknown>/Record<string, 1>, so a mistyped field name would silently write/unset the wrong key.
    • Fix: Type the update as mongoose.UpdateQuery<IWebhook> or a narrowed Partial<Pick<IWebhook, ...>>.
    • kieran-typescript
  • packages/api/src/routers/api/webhooks.ts:151 — the internal webhook API now rejects control characters in queryParams keys/values where a bare z.record(z.string()) previously accepted them, so existing stored configs with such characters will 400 on next update.
    • Fix: Confirm no stored webhook configs rely on the previously-permissive values before shipping.
    • api-contract
  • packages/api/src/routers/external-api/v2/savedSearches.ts:1 — the new router (575 lines) and the expanded webhooks.ts (712 lines) exceed the documented 300-line file-size guidance in CLAUDE.md.
    • Fix: Split the OpenAPI JSDoc blocks or CRUD handler groups into separate modules.
    • project-standards

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

Testing gaps:

  • The webhook PUT 409 concurrent-modification path is untested (highest-value gap; it guards the secret-clearing fix).
  • The destinationChanged secret-clearing branch is only partially covered (headers-cleared case tested; the queryParams-cleared-while-headers-resupplied case is not).
  • deleteSavedSearch's documented partial-failure ordering and concurrent-delete race have no injected-failure test.
  • No spec-conformance test asserts that the new saved-searches/webhooks CRUD responses validate against the generated openapi.json, so future handler-vs-spec drift would go uncaught.

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

Labels

review/tier-4 Critical — deep review + domain expert sign-off

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant