fix: Validate companion host before WebSocket URL construction

[VedantMadane]

Summary

Validate companion host before WebSocket URL construction

Changes

• validate companion host:port before building ws:// URL

Fixes #627

[birme]

Code Review

Verdict: Needs Changes

Summary: The security fix is well-designed — isValidCompanionHost uses a host[:port] allow-list regex that correctly rejects schemes, paths, userinfo, fragments, and out-of-range ports, and it is applied in both buildCallsUrl (write path) and parseCompanionParam (read path, before the URL reaches new WebSocket(...)). I verified the regex against SSRF-style bypass inputs (evil.com/path, user@evil.com, javascript:alert(1), //evil.com, 1.2.3.4:0, [::1]:8080, etc.) and it behaves correctly. However, npm run lint fails, which blocks CI.

Blocking

• src/utils/call-url.ts:39 — Lint error: 'isValidCompanionHost' was used before it was defined (@typescript-eslint/no-use-before-define). buildCallsUrl references the helper declared below it. Move the declaration above buildCallsUrl.
• src/utils/call-url.ts:48 — Prettier error (Insert ⏎·). Run npm run pretty. npm run lint currently exits non-zero, which will fail CI.

Warnings

• src/utils/call-url.ts:50 — isValidCompanionHost is now the 5th host-validation helper in the codebase; near-identical isValidHostPort regexes already exist in connect-to-ws-modal.tsx:87, save-preset-modal.tsx:119, create-production-page.tsx:135, and manage-presets-list.tsx:339. Consider exporting this hardened version as the single shared validator and replacing the copies (the others lack the port-range / length checks).
• src/utils/call-url.test.ts — The newly exported isValidCompanionHost has no direct unit test; it's only exercised indirectly. Add explicit cases for rejected shapes (paths, userinfo, port 0, port > 65535, bracketed IPv6) to lock in the security contract.

Suggestions

• The IPv6-branch skips port validation entirely (!hostPort.endsWith("]")); [::1]:0 would pass since the regex allows :\d{1,5} and the numeric check is bypassed for the bracket form. Minor, but worth tightening for consistency.

Tests (17) pass; typecheck passes. Only the lint failures block merge.

[birme]

Now I have enough context to write the full review. The context repo reflects the pre-PR state, and the diff shows exactly what was changed. Let me compile my findings.

Key findings:

1. Testing — Blocking: The PR introduces two new exported pure functions (isValidCompanionHost and modifies parseCompanionParam) and changes the behavior of buildCallsUrl. The test file in the repo does NOT include tests for isValidCompanionHost or for the new validation behavior (e.g., invalid hosts being silently dropped by buildCallsUrl, or parseCompanionParam returning undefined for malicious inputs). The spec requires bug fixes to include regression tests, and new pure functions must have unit tests.

2. Architecture duplication — Warning: isValidCompanionHost in call-url.ts does nearly the same validation as the local isValidHostPort functions defined in save-preset-modal.tsx:119, create-production-page.tsx:135, manage-presets-list.tsx:339, and connect-to-ws-modal.tsx:87. The PR creates yet another validator without consolidating the existing duplicated ones.

3. Logic issue — Blocking: In COMPANION_HOST_RE, the IPv6 pattern \[[0-9a-fA-F:.]+\] uses . without a character class, matching literal dots AND any character. Since this is inside [] in a regex character class it is fine — but the lastIndexOf(":") logic for port extraction has a correctness issue: for IPv6 addresses like [::1]:8080, lastIndexOf(":") would return the position of the : before the port since the bracket check hostPort.endsWith("]") guards that. Let me reconsider...

Actually the logic if (colon > 0 && !hostPort.endsWith("]")) is correctly guarding against IPv6 with no port — but for an IPv6 with a port like [::1]:8080, the string ends with 0 not ], so lastIndexOf(":") would find the : between ] and 8080. That is actually correct. Let me reconsider more carefully.

For [::1]:8080: lastIndexOf(":") → position of : before 8080. hostPort.endsWith("]") → false (ends with 0). So colon > 0 and not ending with ], so it checks the port. hostPort.slice(colon + 1) → "8080". That's correct.

The regex COMPANION_HOST_RE — the IPv6 bracket group \[[0-9a-fA-F:.]+\] uses . inside the character class which is a literal dot — that's fine. But the full regex doesn't have $ case-insensitive flag — wait, it's /^.../ without flags but uses [a-zA-Z0-9.-]+ explicitly — fine.

4. Test coverage — Blocking: The PR's behavior change in buildCallsUrl (silently dropping invalid companions) has no corresponding test. The existing test at line 92-117 of call-url.test.ts still tests the happy path but the new branch (invalid host → no companion param appended) is untested. parseCompanionParam now returns undefined for invalid inputs (instead of prepending ws:// to any string) but no tests for those cases exist.

5. Style — Warning: The isValidCompanionHost function (line 24 in diff) is exported, while the existing isValidHostPort functions in save-preset-modal.tsx, create-production-page.tsx, manage-presets-list.tsx, and connect-to-ws-modal.tsx are local duplicates. This is a proliferation of duplicated validation logic rather than consolidation.

6. Regex edge case — Warning: COMPANION_HOST_RE allows hostnames like ... (three dots) or -start.com (leading hyphen). The [A-Za-z0-9.-]+ sub-pattern does not enforce valid hostname structure (no leading/trailing hyphen per RFC 1123). This is a minor concern.

Code Review

Verdict: Needs Changes

Summary: The validation logic introduced is sound in intent (closes an SSRF-adjacent issue where any arbitrary string could be embedded as a WebSocket URL parameter), but the PR is missing regression tests for both the new isValidCompanionHost function and the changed behavior of buildCallsUrl and parseCompanionParam. A bug fix with no test that would have caught the original bug is a Blocking violation per project rules. Additionally, the PR introduces a fourth duplicate of the host-port validation pattern that already exists in three other files — a missed consolidation opportunity flagged as a Warning.

---

Blocking

• src/utils/call-url.ts:24 — New exported pure function isValidCompanionHost has zero unit tests. Per project rules, new pure functions are the highest-value test targets and must have unit tests. The existing call-url.test.ts does not import or test isValidCompanionHost at all, and no test file was added in the diff.

• src/utils/call-url.ts:12–16 — The behavior change in buildCallsUrl (silently omitting the companion param when isValidCompanionHost returns false) is the direct bug fix for #627, yet no regression test exercises the failure path: passing an invalid host (e.g. "javascript://evil.com/x", "host/path", "http://evil.com") to buildCallsUrl and asserting the companion param is absent from the returned URL. The spec requires that every bug fix include a test that would have caught the original bug.

• src/utils/call-url.ts:40–42 — parseCompanionParam now returns undefined for invalid inputs (the prior code would blindly return ws://anything), but the new code path is not tested. The existing tests at src/utils/call-url.test.ts:120–132 only test valid inputs.

---

Warnings

• src/utils/call-url.ts:22 — COMPANION_HOST_RE allows structurally invalid hostnames such as --- or .foo.com (leading dot) because [A-Za-z0-9.-]+ imposes no RFC 1123 constraints on label boundaries (no leading/trailing hyphen, no empty labels). This is a minor gap but means a malformed hostname could pass validation and be embedded in a URL. The existing isValidHostPort implementations in save-preset-modal.tsx:119–121 and create-production-page.tsx:135–137 share the same gap — but since this new function is exported and security-motivated, tighter validation is appropriate.

• src/utils/call-url.ts:24 — This PR introduces a fourth copy of host-port validation logic. The same regex-based validation already exists as local isValidHostPort functions in src/components/calls-page/save-preset-modal.tsx:119, src/components/create-production/create-production-page.tsx:135, src/components/manage-productions-page/manage-presets-list.tsx:339, and src/components/calls-page/connect-to-ws-modal.tsx:87. The PR should consolidate by replacing those four local copies with imports of the new isValidCompanionHost (or a renamed shared isValidHostPort). Without consolidation, the validation logic will continue to diverge across files.

• src/utils/call-url.ts:9 — In buildCallsUrl, when companionUrl is provided but fails validation, the companion is silently dropped with no indication to the caller. Callers (e.g. calls-page.tsx:305, 387) pass autoCompanionUrl obtained from parseCompanionParam — which already validates — so double-validation is harmless. But a caller passing a raw ws://… URL directly to buildCallsUrl would have it silently stripped, which could hide configuration bugs. A comment noting this intentional silent-drop behavior would prevent future confusion.

---

Suggestions

• src/utils/call-url.ts:22 — Consider naming the exported regex COMPANION_HOST_PATTERN (or keeping it unexported) and renaming the exported function to isValidHostPort to match the convention already used in the four component-local copies — this would make search-and-replace consolidation straightforward.

• src/utils/call-url.test.ts — When tests are added, cover these cases for isValidCompanionHost: "" (empty), "localhost", "localhost:8080", "[::1]", "[::1]:443", "http://evil.com" (scheme present after strip), "host/path", "host:99999" (port out of range), "host:0" (port 0), and "a".repeat(254) (length limit). For buildCallsUrl, add a case with an invalid companion string and assert the param is absent. For parseCompanionParam, add cases for "javascript://evil" (post-strip would be "evil", actually valid — but confirm) and "host:99999" returning undefined.

---

Domain Note

Not applicable — this change does not touch audio routing, PTT, dominant speaker, data channel parsing, or WHIP/WHEP session lifecycle.

---

Next steps: pass Blocking items to bug-fixer (add regression tests for isValidCompanionHost, the new buildCallsUrl branch, and the new parseCompanionParam branch) → once resolved, use pr-author to merge.

FINAL VERDICT: NEEDS CHANGES
agentId: a3247779d3995c882 (use SendMessage with to: 'a3247779d3995c882' to continue this agent)
total_tokens: 34133
tool_uses: 14
duration_ms: 67582

[birme]

Thanks for the fix — reviewed the diff against issue #627.

The validation logic itself is correct and well-scoped. parseCompanionParam / buildCallsUrl in src/utils/call-url.ts now strip any scheme, validate host[:port] via an allowlist regex, and return undefined on failure — which correctly blocks new WebSocket() at the real sink. I verified it rejects the important bypass vectors: userinfo (user@evil.com), scheme-relative (//evil.com), embedded paths (attacker.com/malicious), leading/trailing/tab whitespace, unicode homoglyphs, out-of-range ports, and malformed IPv6, while accepting legitimate hosts.

Blocking: no regression test. Per our review rubric (§5), a security fix must ship a test exercising the exact failure path. src/utils/call-url.test.ts currently only covers happy-path stripping — nothing asserts that malicious inputs return undefined. Without that, a future refactor can silently reopen the vulnerability.

Please add test cases asserting that parseCompanionParam rejects at least: "attacker.com/malicious", "user@evil.com", "//evil.com", and an over-range port — then this is good to merge.

(Automated review by daily-backlog-pr; the validation code is safe, this is purely the missing test coverage.)

[birme]

daily-backlog-pr Phase 2 re-entry: the requested changes here are clear and small (add regression tests in src/utils/call-url.test.ts covering that parseCompanionParam/buildCallsUrl reject attacker.com/malicious, user@evil.com, //evil.com, and an out-of-range port; plus fix the failing lint and prettier checks). However, the source fork VedantMadane/intercom-frontend is no longer reachable ("Repository not found"), so this automated pass cannot push the fix to the PR branch.

Routing to a human: either the contributor pushes the test + formatting fixes, or a maintainer re-creates the branch on the base repo to carry it forward. Leaving the board item in Ready.

[birme]

Code Review — Needs Changes

The core security logic is sound: the host[:port] validator in src/utils/call-url.ts correctly rejects schemes, paths, userinfo (@), and out-of-range ports before WebSocket URL construction, and it's well unit-tested (call-url.test.ts). It closes the SSRF/open-redirect concern effectively. But two things block merge:

Blocking:

• _pr_meta.json (repo root) — a new internal automation-metadata file (references the PR number, fork, head branch) was committed. It's unrelated to the fix (scope creep), doesn't exist on main, and lacks a trailing newline — almost certainly the cause of the failing prettier check. Remove it.
• CI prettier check fails (all others pass). Run npm run pretty, commit, and confirm green.

Warnings:

• src/utils/call-url.ts:69 — parseCompanionParam always emits ws:// and normalizes wss:// inputs down to ws://. Issue #627 recommended upgrading to wss:// when the page is served over HTTPS (mixed-content + encryption). Consider selecting scheme from window.location.protocol.
• src/utils/call-url.ts:32-33 — host regex is a syntactic filter, not a strict RFC hostname check (accepts leading/trailing dots, bare -). Doesn't reopen the SSRF vector, but noting.

Suggestions: Add an isValidCompanionHost("[::1]:0") case to lock in IPv6+invalid-port handling.

[birme]

daily-backlog-pr Phase 3.5 (orphan-PR housekeeping) — reviewed the diff. Needs Changes.

The SSRF-hardening itself looks solid — isValidCompanionHost plus the parseCompanionParam/buildCallsUrl guards, with thorough regression tests (scheme/path/userinfo/out-of-range-port/over-length rejection). Two things must be fixed before this can merge:

• Blocking — remove _pr_meta.json. The PR commits a _pr_meta.json tooling artifact to the repo root. That file should not be part of the change; please delete it from the branch.
• Blocking — prettier check is failing. Run npm run pretty and commit the formatting fixes so CI goes green.

Once those two are addressed the fix should be good to go.

[birme]

Independent review (daily-backlog-pr Phase 3.5, orphan PR triage).

Verdict: Needs Changes

Blocking

• A stray _pr_meta.json artifact is committed to the repo root. This looks like tooling/scratch output and shouldn't ship — please remove it from the PR (scope creep, unrelated to the fix).

Positive

• The core security fix (host validation forcing ws:// for the reconnect URL) is solid and correctly scoped to closing #627.

Once the stray artifact is dropped, this should be good to go.

[birme]

This PR has received 3+ "Needs Changes" reviews without landing (5 CHANGES_REQUESTED to date) — pausing automatic retries here per the daily-backlog-pr escalation guard (MAX_ATTEMPTS=3). A human should look at the review feedback directly rather than another automated pass: #671