Skip to content

fix(peer): run isPeerMessage in the send message guards - #172

Closed
dinwwwh wants to merge 1 commit into
mainfrom
claude/peer-send-guards-check-peer-message
Closed

dinwwwh wants to merge 1 commit into
mainfrom
claude/peer-send-guards-check-peer-message

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Oct 22, 2026

Copy link
Copy Markdown
Member

Problem

isClientPeerSendMessage and isServerPeerSendMessage were typed (maybe: PeerMessage) and only checked kind and json. They assumed the caller had already run isPeerMessage, which checks:

  • the value is an object
  • id is a string
  • kind is a string
  • binary is undefined, a Uint8Array or a Blob

Nothing at runtime enforces that ordering. TypeScript doesn't catch it either when the input is any, as JSON.parse output is. A caller that validates raw input with one of these guards alone:

  • accepts a message whose id is true, {}, [], null, a number, or missing;
  • accepts a binary that isn't binary, such as a string or a plain object;
  • gets a TypeError: Cannot read properties of null (reading 'kind') on null, instead of false.

This happened in oRPC's HTTP batch plugins, fixed downstream in middleapi/orpc#2304:

  • On the server, a non-string id ran the procedure and was echoed back in the response. A deeply nested array id (~20KB body) made JSON.stringify throw RangeError: Maximum call stack size exceeded outside every interceptor.
  • On the client, a JSON object in binary resolved the call with a "[object Object]" file.

The same mistake is easy to repeat in any new caller that handles raw input.

Fix

Both combined guards now take unknown and run isPeerMessage first. They are safe to call directly on untrusted input, and they behave the same when given a real PeerMessage.

The per-kind guards (isPeerRequestMessage, isPeerResponseMessage, …) stay typed PeerMessage. They are the building blocks the combined guards call after narrowing, and their signature already states what they expect.

Compatibility

  • Types: widening the parameter from PeerMessage to unknown is non-breaking for callers. Existing code that passes a PeerMessage, such as the output of decodePeerMessage, compiles and narrows as before.
  • Runtime: the only change is that values isPeerMessage rejects now return false. Before, they could return true, or throw for null. The internal client and server code doesn't call these guards. decodePeerMessage already runs isPeerMessage, so for messages that come from it the extra check costs a few property reads.

Tests

validators.test.ts adds cases to both guards for a non-string id, a string binary, null, and an array. Six of the eight fail on main: two are wrongly accepted and one throws a TypeError, for each guard.

  • pnpm test: 1382 vitest tests and 52 bun tests passed
  • pnpm run check: sherif, oxlint, oxfmt and tsc all pass

Generated by Claude Code

isClientPeerSendMessage and isServerPeerSendMessage only checked kind and
json, assuming isPeerMessage had already run, so a caller validating raw
input with them alone accepted a non-string id or a non-binary binary,
and null threw a TypeError. They now take unknown and run isPeerMessage
first, so they are safe on untrusted input.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01X6CM5zP1QBSVRmibkRqzK3
@pkg-pr-new

pkg-pr-new Bot commented Oct 22, 2026

Copy link
Copy Markdown
@standard-server/aws-lambda

npm i https://pkg.pr.new/@standard-server/aws-lambda@172

@standard-server/core

npm i https://pkg.pr.new/@standard-server/core@172

@standard-server/fastify

npm i https://pkg.pr.new/@standard-server/fastify@172

@standard-server/fetch

npm i https://pkg.pr.new/@standard-server/fetch@172

@standard-server/node

npm i https://pkg.pr.new/@standard-server/node@172

@standard-server/peer

npm i https://pkg.pr.new/@standard-server/peer@172

@standard-server/shared

npm i https://pkg.pr.new/@standard-server/shared@172

commit: e258fda

@macroscopeapp

macroscopeapp Bot commented Oct 22, 2026

Copy link
Copy Markdown

Approvability

Verdict: Approved at e258fda

Macroscope's review found this PR approvable — This PR hardens the client/server peer send-message guards by validating the common message shape before subtype checks, while preserving behavior for valid messages. It adds focused tests for malformed inputs and has no broader runtime, schema, deployment, or infrastructure changes.

You can add or adjust custom eligibility rules. Learn more.

@codecov

codecov Bot commented Oct 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ No new issues found.

Reviewed changes

  • Combined guards now validate the envelope first — isClientPeerSendMessage and isServerPeerSendMessage take unknown and call isPeerMessage(maybe) before the per-kind || chain, so raw input with a non-string id, a non-binary binary, a non-object, or null now returns false instead of wrongly accepting or throwing.
  • Per-kind guards left as-is — isPeerRequestMessage, isPeerResponseMessage, etc. stay typed PeerMessage; they run after the narrowing and their signatures already state the precondition.
  • Regression tests added — validators.test.ts gains an it.each block per combined guard covering non-string id, string binary, null, and array.

I confirmed the new cases are discriminating: reverting only validators.ts to base 6f1fc5a fails 6 of the 8 (both guards wrongly accept a non-string id and a string binary, and both throw TypeError on null), matching the PR description. The parameter widening PeerMessage → unknown is non-breaking, and the only in-repo callers pass decodePeerMessage(...).message, which already runs isPeerMessage, so behavior is unchanged for real messages.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@codspeed

codspeed Bot commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

1 warning

⚠️ 4 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/peer-send-guards-check-peer-message (e258fda) with main (fc31c9d)2

Open in CodSpeed

Footnotes

  1. 108 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

  2. No successful run was found on main (6f1fc5a) during the generation of this report, so fc31c9d was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@dinwwwh dinwwwh closed this Oct 22, 2026
dinwwwh added a commit to middleapi/orpc that referenced this pull request Oct 22, 2026
## Bug

Both batch plugins checked untrusted JSON batch messages with the
`@standard-server/peer` guards `isClientPeerSendMessage` /
`isServerPeerSendMessage` and nothing else. Those guards assume
`isPeerMessage` has already run, so they never check `id` or `binary`.

### Server: `BatchHandlerPlugin` (GET `?data=` and POST body)

- Ids like `true`, `{}`, `[]`, `null`, numbers, or a missing id were
accepted. The procedure ran and the bad id was echoed back in the 207
response.
- A deeply nested array id (~10k levels, a ~20KB body) passed
validation. When the adapter serialized the response, `JSON.stringify`
threw `RangeError: Maximum call stack size exceeded`. That happens
outside every interceptor, so no error handling or logging plugin can
catch it.

### Client: `BatchLinkPlugin` (JSON array responses)

This side gets its input from the server or anything in between, not
from untrusted clients, so the impact is lower. A malformed response
still failed in the wrong way:
- A message with a non-string id was silently dropped, and the call
failed with `Batch response is incomplete.` instead of `Invalid batch
response format.`
- A JSON object in `binary` didn't fail at all. The call **resolved**
with a 15-byte file containing `"[object Object]"`.

The WebSocket, crossws and message-port adapters, and the client's
length-prefixed batch decoding, already reject these messages. They
either call `isPeerMessage` directly or go through `decodePeerMessage`,
which calls it. Only the two JSON batch paths were missing the check.

This is the same class of bug as trpc/trpc#7477, where
`assertIsRequestId` accepted invalid ids. That fix is what led us to
look here.

## Fix

- `packages/server/src/plugins/batch.ts`: both branches now call a new
`isClientPeerSendMessages` helper, which requires `isPeerMessage(m) &&
isClientPeerSendMessage(m)` for every message. Invalid batches get a 400
before any procedure runs.
- `packages/client/src/plugins/batch.ts`: the array-response check
becomes `isPeerMessage(v) && isServerPeerSendMessage(v)`.
- The type checker now enforces the guard order:
- The per-message callbacks are typed `(m: unknown)`, so tsc rejects a
call to `isClient/ServerPeerSendMessage` (declared to take a
`PeerMessage`) that isn't preceded by `isPeerMessage`.
- The parsed GET `data` is typed `unknown`, so it can't be assigned to
`messages` without the guard.
  - The original bug got past tsc because `JSON.parse` returns `any`.
- Tests:
- Server: regression tests for invalid ids (POST and GET) and for the
deeply nested id.
- Client: regression tests for invalid ids and an invalid `binary` in
array batch responses. These fail on `main`.
- Existing tests that built batches by hand switched from numeric to
string ids.

## Behaviour change

Hand-built HTTP batch clients that send **numeric ids** (or any
non-string id) now get a `400` instead of a 207. The oRPC client always
sends string ids, so oRPC clients are unaffected.

## Verification

- Full root `pnpm vitest run`: 287 files passed, 7 skipped (3977 tests)
- `pnpm type:check`: clean
- `pnpm lint`: clean

## Follow-up (not in this PR)

The root cause sits upstream in `@standard-server/peer`:
middleapi/standard-server#172 makes the combined guards take `unknown`
and run `isPeerMessage` themselves. This PR doesn't depend on it. The
explicit `isPeerMessage` calls here stay correct either way, and they
can be dropped once that change is released and the dependency is
bumped.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01X6CM5zP1QBSVRmibkRqzK3

---------

Co-authored-by: Claude <noreply@anthropic.com>
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.

2 participants