fix(security): constrain WHIP/WHEP path params to prevent log injection

[birme]

Summary

• The username, lineId and productionId path params on POST /api/v1/whip/:productionId/:lineId/:username and the WHEP equivalent were URL-decoded by Fastify and interpolated directly into log messages with no sanitization, allowing newline (%0a), carriage-return (%0d) and ANSI-escape (%1b[...) injection to forge or corrupt log entries.
• Added restrictive TypeBox pattern constraints on both routes: numeric-only ^[0-9]+$ for productionId and lineId (matching the existing convention in api_productions.ts, and correct since line/production ids are generated as numeric strings), and ^[\w .-]{1,200}$ for username.
• Used a literal space in the username pattern instead of \s, because \s also matches \n/\r/\t — the exact control characters this fix must reject. Requests carrying control chars now fail schema validation with a 400 before anything is logged.
• Added regression tests covering a control-char username and a non-numeric productionId.

Test plan

• Tests pass (npm test) — 245 passed (2 new)
• TypeScript compiles (npm run typecheck)
• Lint clean (npm run lint) — 0 errors
• Control-char username / non-numeric ids rejected with 400 by schema

Closes #287

🤖 Generated with Claude Code

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

[birme]

code-reviewer verdict: LGTM

Code Review

Summary: Fixes the log-injection in #287 by replacing permissive maxLength-only TypeBox constraints on WHIP/WHEP POST path params with restrictive patterns — numeric ^[0-9]+$ for productionId/lineId, ^[\w .-]{1,200}$ for username. Control chars (\n, \r, \x1b, \t) now 400 before any log interpolation. Numeric ids match id generation in production_manager.ts, so legitimate traffic is unaffected. Author correctly avoided the issue's suggested \s. 17/17 WHIP tests pass incl. 2 new regression tests; typecheck/lint clean.

Blocking: None.

Warnings:

• src/api_whep.ts:94-98 — WHEP gets the identical fix but there is no api_whep.test.ts, so the WHEP path has no regression coverage.

Suggestions:

• Constrain sessionId/productionId/lineId on DELETE/PATCH routes too (defense-in-depth); drop now-redundant maxLength on username.

CI green, mergeable, no human changes-requested. NOTE: this account (birme) authored the PR, so it cannot self-approve and branch protection blocks the merge — a distinct reviewer must approve + merge. Left in In review.

[birme]

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

Code Review

Verdict: Needs Changes

Summary: The core sanitization on the WHIP/WHEP POST routes is correct and effectively blocks CRLF/ANSI log injection while still accepting legitimate values, but the fix is incomplete: the DELETE routes still log a user-controlled path param unsanitized (the same #287 vector), and the WHEP change ships with no test coverage.

---

Blocking

• src/api_whip.ts DELETE /whip/:productionId/:lineId/:sessionId and src/api_whep.ts DELETE /whep/:productionId/:lineId/:sessionId — The DELETE handlers log sessionId directly (Log().info(\Received WHIP DELETE request - sessionId: ${sessionId}, IP: ${request.ip}`)) **before** any DB lookup, and their params schema is still Type.String({ maxLength: 200 })with nopattern. sessionId, productionIdandlineIdare all fully user-controlled path params, soDELETE /api/v1/whip/1/1/evil%0aINJECTED-LOG-LINEreproduces exactly the CRLF log injection this PR claims to close for #287. The error path also logssessionId. Apply the same ^[0-9]+$constraint toproductionId/lineIdand a control-char-rejecting pattern (a UUID pattern is ideal, since sessionId is generated as a UUIDv4) tosessionId` on both DELETE routes.

• src/api_whep.ts POST params schema (WHEP) — The WHEP POST change is identical to WHIP but has zero test coverage: there is no api_whep.test.ts in src/, and the two new regression tests were added only to api_whip.test.ts. Per the testing criteria a security fix must be exercised by a test on the affected path. Add a WHEP test file mirroring the WHIP regression tests (control-char username -> 400, non-numeric productionId -> 400) so the WHEP fix is not silently unverified.

---

Warnings

• src/api_whip.test.ts — This test file does not mock ./log (jest.mock('./log', () => ({ Log: () => ({ info, error, debug, warn }) }))). This is a pre-existing gap not introduced by this PR, but since the PR adds tests that specifically trigger log-injection code paths, mocking ./log here would both silence noisy output and let you assert the sanitized value never reaches the logger.

• src/api_whip.ts / src/api_whep.ts POST params — username carries both maxLength: 200 and pattern: '^[\\w .-]{1,200}$'; the {1,200} quantifier already bounds length, so maxLength is redundant. Harmless, but consider dropping one to avoid two sources of truth on the bound.

---

Suggestions

• The username pattern [\w .-] is intentionally narrow. Confirm with product that broadcast operator display names never legitimately contain non-ASCII letters (e.g. accented characters) or characters like + @ ( ); if they can, a control-char denylist would be a less surprising constraint than an ASCII allowlist while still closing the injection vector. Not blocking — the current allowlist is safe and matches the PR's stated intent.

---

Domain Note

This change affects WHIP/WHEP session lifecycle (ingest/egress route params). Consider consulting the intercom-expert agent to confirm the numeric-only productionId/lineId and the [\w .-] username allowlist match all real-world broadcast client behavior.

Positive verification: The regex reasoning is sound — TypeBox pattern compiles to a JS RegExp without the m/s flags, so ^...$ anchors to the whole string, . does not match a newline, and $ does not tolerate a trailing newline; the \w/space/dot/dash class excludes newline, carriage-return and ESC. Using a literal space instead of \s (which matches newline/CR/tab) is correct, as the PR body notes. Legitimate numeric ids and alphanumeric usernames pass.

Next steps: pass Blocking items to bug-fixer -> once resolved, use pr-author to open the PR.

[birme]

Automated review note: This PR is authored by birme, the same GitHub account the review automation runs under, so a formal request-changes review cannot be submitted (GitHub blocks self-review) and this feedback is posted as a comment instead. The findings below are still merge-blocking. This PR needs a reviewer who is not the author.

Code Review — PR #306: fix(security): constrain WHIP/WHEP path params to prevent log injection

Closes #287. Adds TypeBox pattern constraints to the POST /whip/... and POST /whep/... route params (productionId/lineId → ^[0-9]+$, username → ^[\w .-]{1,200}$), rejecting control chars with a 400 before values reach Log().info(...).

What's correct

• Security fix is sound and not bypassable. username is logged at api_whip.ts:131/api_whep.ts:132 and again at :210. The ^[\w .-]{1,200}$ pattern excludes \n, \r, \t, ESC. The deliberate use of a literal space instead of \s is correct. AJV compiles patterns with the u flag (no m), so $ does not match before a trailing \n — a username ending in %0a is rejected.
• Numeric constraint on ids matches reality. Line/production ids are numeric strings (production_manager.ts:258,287); ^[0-9]+$ rejects no legitimate id.
• TypeScript/architecture: handler Params generics still type these as string (correct); ISmbProtocol usage unchanged. No yarn artifacts.

Blocking

B1 — Test file does not jest.mock('./log'). api_whip.test.ts
Per CLAUDE.md ("backend tests must jest.mock('./log')"): the only jest.mock in api_whip.test.ts is for uuid (line 8). There is no jest.mock('./log'). This is a security fix whose entire purpose is guarding log output, so the log module is directly in scope. Add jest.mock('./log') and ideally assert Log().info is not called with control chars for the 400 cases, so the regression test proves the injection is blocked rather than merely asserting a 400 status.

Warnings

W1 — WHEP has no dedicated regression test. The two new tests were added only to api_whip.test.ts:205,218. The WHEP route (api_whep.ts:95-100) received the identical schema change but no corresponding test. Mirror both tests into the WHEP suite.

W2 — DELETE/PATCH routes still log an unconstrained path param. api_whip.ts:245-249,279 keeps Type.String({ maxLength: 200 }) (no pattern) and logs sessionId at :279 before any lookup, so DELETE /whip/1/1/evil%0aFAKE reproduces the same log-injection class. Out of #287's stated scope, but leaving sibling routes vulnerable means the root cause is only partially closed. Recommend a UUID pattern on sessionId.

W3 — username maxLength: 200 is redundant with {1,200} in the pattern (api_whip.ts:96-99). Keep in sync or drop one.

Verdict

Needs Changes — 1 Blocking (missing jest.mock('./log')). The core security constraint itself is correct and well-reasoned; the blocker and warnings are about test hygiene and completeness across sibling routes.

[birme]

Code Review

Verdict: Needs Changes

Summary: The core fix is correct — constraining productionId/lineId to ^[0-9]+$ and username to ^[\w .-]{1,200}$ via TypeBox rejects newline/CR/ANSI control chars before they reach the Log().info sinks. However, the identical change to src/api_whep.ts has NO regression test: the added tests live only in api_whip.test.ts, and there is no api_whep.test.ts in the repo. A security fix without a test exercising the exact failure path is Blocking.

---

Blocking

• src/api_whep.ts:94-100 — The WHEP route receives the same hardening and has the same log-injection sinks (api_whep.ts:133-134, :212-213), but there is no api_whep.test.ts, so this security fix is entirely untested. Add a WHEP regression test asserting a control-char username and a non-numeric productionId both return 400, mirroring api_whip.test.ts:205-234.

---

Warnings

• src/api_whep.ts:259-260 — The other WHEP routes still declare productionId/lineId with only maxLength: 200, not the new ^[0-9]+$ pattern, and they also log. Lower-risk (sessionId-based, parseInt+isNaN downstream) but the numeric pattern should be applied for complete log-injection closure; the WHIP file likely has the same asymmetry.

---

Suggestions

• username pattern ^[\w .-]{1,200}$ already bounds length to 200, making the separate maxLength: 200 redundant (harmless).

---

Domain Note

Affects WHIP/WHEP session lifecycle (path-param validation). Confirm productionId/lineId are always numeric strings in every caller; if any non-numeric id can legitimately reach these routes, ^[0-9]+$ would break valid traffic.

Posted by daily-backlog-pr Phase 3; moving back to Ready.

[birme]

Code Review — Verdict: NEEDS CHANGES

(Posted as a comment: GitHub blocks a formal request-changes review on a self-authored PR under this automation account. Treat this as the review of record.)

Summary: The fix is well-targeted — it replaces the permissive maxLength: 200 schemas with pattern-constrained TypeBox validators (^[0-9]+$ for productionId/lineId, ^[\w .-]{1,200}$ for username) on both WHIP and WHEP POST endpoints, so CR/LF-bearing input is rejected at the schema layer (400) before reaching the Log().info sink. The numeric constraint matches real ID usage. However, the security fix ships without adequate regression tests.

Blocking

• src/api_whep.ts:94-98 — WHEP was hardened identically to WHIP, but there is no api_whep.test.ts in the repo; the WHEP log-injection path has zero coverage. Per project rules a security fix must ship a regression test exercising the failure path. Add WHEP tests mirroring the WHIP cases (malicious username with \n → 400; non-numeric productionId → 400).
• src/api_whip.test.ts:1-10 — The WHIP test file does not mock ./log, violating the project rule that backend test files must mock ./log. Add jest.mock('./log', ...) and assert the mocked logger is never called with the injected value.

Warnings

• src/api_whip.test.ts:65-78 — The regression test asserts only statusCode === 400; with ./log mocked it should also assert the logger wasn't invoked with the injected string, to truly guard the injection path.

Moving back to Ready for the missing WHEP tests and ./log mock.

[birme]

Automated code-reviewer verdict (daily-backlog-pr Phase 3): NEEDS CHANGES

Note: this run's automation token authored this PR, so GitHub blocks a state-bearing --request-changes review (self-review). Findings posted as a comment; board item moved back to Ready. Needs a different reviewer identity to formally gate/merge.

The POST-path fix is sound (anchored TypeBox pattern constraints, correct layer, no ReDoS, numeric-lineId assumption verified against production_manager.ts). Two gaps:

High

• DELETE routes still log an unconstrained client-supplied param — the same log-injection class this PR closes. src/api_whip.ts:280 logs sessionId (and at the 404/success/500 paths) while the DELETE schema at :249 keeps sessionId: Type.String({ maxLength: 200 }) with no pattern. Same in src/api_whep.ts. DELETE /whip/123/456/<%0a|%1b payload> logs the raw value before the DB lookup. Real session IDs are server UUIDs, so a ^[0-9a-fA-F-]{36}$ (or at minimum CR/LF/ctrl-rejecting) pattern on sessionId closes it.

Medium

• WHEP has zero test coverage for this fix. New regression tests live only in src/api_whip.test.ts; there is no api_whep.test.ts. The body claims "both WHIP and WHEP" but the WHEP schema change is untested. Add the control-char-username / non-numeric-productionId cases against WHEP.

Nits

• username sets both maxLength: 200 and pattern: '^[\w .-]{1,200}$' (redundant length bound, harmless). Charset drops accented/CJK names — worth a comment noting the intentional ASCII restriction.

[birme]

Automated code-reviewer verdict (daily-backlog-pr Phase 3): NEEDS CHANGES

The core fix is sound: TypeBox pattern/minLength constraints on the POST :productionId/:lineId/:username params on both WHIP and WHEP, applied at the schema layer (correct — rejects before the value reaches Log().info(...)). Verified the username pattern ^[\w .-]{1,200}$ rejects \n, \r\n, tabs (including the JS/AJV abc\n trailing-newline quirk → false), and ^[0-9]+$ eliminates injection on productionId/lineId.

Blocking

• src/api_whip.test.ts does not mock ./log. Project criteria require every backend test file to jest.mock('./log', …). Real console.info bleeds through the new tests. Add the mock.

Warnings

• DELETE path leaves the same vuln class open. api_whip.ts:271-304 / api_whep.ts:270-303 still type sessionId as Type.String({ maxLength: 200 }) with no pattern and log the client-controlled sessionId directly — DELETE /whip/123/456/evil%0Ainjected injects control chars into the same log output. Constrain sessionId (e.g. UUID pattern) or document why DELETE is excluded.
• WHEP has zero test coverage — src/api_whep.test.ts doesn't exist; the identical WHEP schema change ships untested. Add equivalent (or shared parametrized) regression tests.
• Non-ASCII username regression. \w is ASCII-only, so Jöns/Åsa/Müller now 400. For a Nordic product this is a real usability regression — consider Unicode-aware validation (\p{L} with u flag) while still excluding control chars, or document ASCII-only as an accepted constraint.

Either the blocking item alone or the 3 warnings require changes. Moving issue #287 back to Ready.

---

Note: this run could not post this as a formal --request-changes review because the PR author (birme) is the same account the automation runs as, and GitHub blocks self-reviews. Posting the verdict as a comment instead. Because it is not a CHANGES_REQUESTED-state review, the Phase-2 escalation guard cannot count it automatically — a human reviewer should formally request changes. Moving the linked issue back to Ready.

[birme]

daily-backlog-pr Phase 3 — automated review (verdict: Needs Changes)

⚠️ This pipeline runs under the birme identity, which also authored this PR. GitHub refuses to let an author submit an approve/request-changes review on their own PR, and main now requires 1 approving review — so this run cannot post a state-bearing review or merge. Recording the verdict here as a note and flagging for a human reviewer with a different identity.

Findings

• Major — fix is incomplete. The POST-route param constraints (productionId/lineId → ^[0-9]+$, username → ^[\w .-]{1,200}$) correctly close the newline/CR/ANSI log-injection vector for POST. But the sibling DELETE handlers in api_whip.ts/api_whep.ts still declare sessionId as Type.String({ maxLength: 200 }) with no pattern and log it unescaped — e.g. DELETE /whip/1/1/foo%0ainjected still injects a newline. Same class of bug the PR title claims to fix.
• Minor — POST productionId/lineId dropped the explicit maxLength: 200 when the ^[0-9]+$ pattern was added; the pattern is length-unbounded. Consider keeping maxLength alongside.
• Nit — no test asserts DELETE/PATCH reject control chars (would have caught the Major); username length is bounded twice (maxLength + {1,200}).

Numeric-only productionId/lineId is safe for real callers (verified: _id is numeric, line ids are index.toString()).

[birme]

daily-backlog-pr automated code review. A state-bearing GitHub review could not be posted because this PR was opened by the same account the automation runs as (birme); GitHub forbids approving/requesting-changes on ones own PR. Recording the verdict as a comment instead. Board item moved back to Ready for rework.

Code Review

Verdict: Needs Changes

Summary: The POST WHIP/WHEP username/lineId/productionId log-injection vector is correctly closed via strict TypeBox pattern validation, but the fix is incomplete — the DELETE routes still log an attacker-controlled sessionId with only maxLength validation, and test coverage is missing.

Blocking

• src/api_whip.ts:260-262 & src/api_whep.ts:259-261 — DELETE route sessionId still uses only Type.String({ maxLength: 200 }) with no pattern. sessionId is attacker-controlled in the URL and is logged unsanitized (api_whip.ts:276,282,294,299 / api_whep.ts:275,281,292,298). DELETE /whip/123/456/evil%0Ainjected decodes to a real newline (Fastify percent-decodes path params) and reaches Log().info(...) before any lookup — the exact CRLF log-injection of Security: Log injection via unsanitized username/lineId in WHIP/WHEP log output #287, left unfixed on DELETE. Apply the same ^[\w.-]+$-style pattern to sessionId.
• src/api_whip.test.ts — Security regression tests do not jest.mock('./log', ...) (project backend test convention), and they assert only on HTTP 400, never that the log was not called with control chars. Mock the log and assert the security invariant (nothing logged before rejection).

Warnings

• Missing src/api_whep.test.ts — no WHEP test file at all; WHEP got the identical validation change (api_whep.ts:95-100) with zero regression coverage. WHIP/WHEP are twins; both need the security test.
• src/api_whip.ts:97-100 / src/api_whep.ts:97-100 — username has both maxLength: 200 and a pattern already bounded to {1,200}; redundant, risks drift.

Suggestions

• production_manager.ts:400 logs name and is reached from non-WHIP/WHEP callers (api_productions.ts:680, api_productions_core_functions.ts:74) not covered by this PR. Same log-injection class likely exists there — worth a follow-up and centralizing sanitization in src/log.ts.