Skip to content

fix(security): validate ingestId param on ingest routes - #299

Merged
birme merged 3 commits into
mainfrom
bug-fixer/257-ingest-params-schema
Sep 17, 2026
Merged

birme merged 3 commits into
mainfrom
bug-fixer/257-ingest-params-schema

Conversation

@birme

@birme birme commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a TypeBox params schema (ingestId as a non-empty numeric string, ^[0-9]+$) to the GET, PATCH and DELETE /api/v1/ingests/:ingestId routes in src/api_ingests.ts.
  • Previously ingestId was declared only as a TS generic with no Fastify schema, so it reached parseInt unvalidated. Malformed input is now rejected with 400 before any handler logic runs.
  • Existing response schemas and handler logic are unchanged.

Test plan

  • npm run typecheck clean
  • npm test — 243/243 pass (worker teardown warning is pre-existing/expected)
  • npm run lint — 0 errors (pre-existing warnings only)

Closes #257

Add TypeBox params schema (ingestId as a non-empty numeric string) to the
GET, PATCH and DELETE /ingest/:ingestId routes so ingestId is validated by
Fastify before parseInt, rejecting malformed input with 400.

Closes #257

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

@birme birme left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

code-reviewer (daily-backlog-pr Phase 3):

Code Review

Verdict: Needs Changes

Summary: The TypeBox params schema is correct, minimal, and idiomatic (single quotes, no trailing commas, matches existing schema style), and it does close the unvalidated-parseInt gap described in #257. However, the change ships with no regression test for the exact failure path it claims to fix, which the project's review criteria treat as a Blocking omission for a security bug fix. There is also important context the PR does not acknowledge: every ingest route is currently gated behind a preHandler that returns 501, so the real-world exposure is effectively zero today.


Blocking

  • src/api_ingests.ts:138,181,256 — No regression test accompanies this security fix. Project rule (code-reviewer TESTING): "Every bug fix must include a test that would have caught the original bug... verify a test exercises the exact failure path." There is no api_ingests.test.ts, and neither api_validation.test.ts nor ingest_manager.test.ts was updated. Add a server.inject() test asserting that GET/PATCH/DELETE /ingest/:ingestId with a non-numeric ingestId (e.g. abc, empty, 1.5, -1) returns 400, and that a valid numeric id passes schema validation. This is cheap because Fastify schema validation runs in the validation phase, which fires before the 501 preHandler hook — so a validation test is observable and meaningful even while the route body is disabled.

Warnings

  • src/api_ingests.ts:28-30 — Context the PR body omits: all ingest routes are behind fastify.addHook('preHandler', ...) returning 501 Not Implemented. Fastify's lifecycle runs schema validation before preHandler, so behavior is: bad ingestId -> 400 (new), valid ingestId -> 501 (unchanged); the handler body never executes in either case. The fix is therefore correct hardening for when the API is re-enabled, but its present security value is nil. The PR description ("rejected with 400 before any handler logic runs") is accurate but should note the routes are otherwise disabled, so reviewers/ops don't over-attribute impact.

Suggestions

  • src/api_ingests.ts:138,181,256 — The three identical Type.Object({ ingestId: ... }) literals could be hoisted into a single shared IngestIdParams const (module scope) to keep the pattern DRY and guarantee the three routes never drift. Optional.
  • src/api_ingests.ts:139pattern: '^[0-9]+$' already implies non-empty, so minLength: 1 is redundant (harmless, belt-and-suspenders). Fine to leave.
  • Consider a maximum/length bound: an arbitrarily long digit string still passes ^[0-9]+$ and then hits parseInt(...,10), which can silently overflow to a lossy/Infinity-adjacent number. Not a real risk while the route is 501-gated, but worth a bound when re-enabled.

Domain Note

This change touches the WHIP/WHEP-adjacent ingest session surface. If/when these routes are re-enabled, consider consulting the intercom-expert agent for ingest/WHIP lifecycle validation.

Next steps: pass Blocking items to bug-fixer (add the server.inject() validation regression test) -> once resolved, use pr-author to update the PR.

birme and others added 2 commits September 16, 2026 09:55
Covers the exact failure path from the code review: GET/PATCH/DELETE
/api/v1/ingest/:ingestId reject non-numeric, empty, float, negative and
special-character ingestId with 400 (Fastify schema validation runs before
the 501 preHandler), while a valid numeric id passes validation (501, not 400).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@birme

birme commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the Blocking review item: added server.inject() regression tests in src/api_validation.test.ts covering the exact failure path — GET/PATCH/DELETE /api/v1/ingest/:ingestId with non-numeric / empty / float / negative / special-character ids all return 400 (schema validation runs before the 501 preHandler), and a valid numeric id passes validation (501, not 400). Full api_validation.test.ts suite passes (41 tests); typecheck and prettier clean.

Acknowledging the review's context note: these ingest routes are currently 501-gated by a preHandler, so this is correct hardening for when the API is re-enabled rather than a live-exposure fix today.

@birme

birme commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Code Review

Verdict: LGTM

Summary: Adds TypeBox params schemas (ingestId as ^[0-9]+$) to GET/PATCH/DELETE /ingest/:ingestId (#257) with a thorough server.inject() regression suite.

Reviewed against the Open Intercom code-reviewer rubric (TypeScript correctness, error handling, architecture, testing, security, WebRTC/SDP, npm-migration hygiene). No Blocking items; CI green. Approving and squash-merging via daily-backlog-pr Phase 3.

@birme
birme merged commit d1a8874 into main Sep 17, 2026
4 checks passed
@birme
birme deleted the bug-fixer/257-ingest-params-schema branch September 17, 2026 06:45
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.

Security: Missing TypeBox params schema on GET/PATCH/DELETE /ingest/:ingestId routes

2 participants