Skip to content

fix: harden c2c relay authorization and error handling - #27

Draft
sidmorizon wants to merge 5 commits into
mainfrom
claude/e2e-transfer-security-review-su3bzg
Draft

fix: harden c2c relay authorization and error handling#27
sidmorizon wants to merge 5 commits into
mainfrom
claude/e2e-transfer-security-review-su3bzg

Conversation

@sidmorizon

@sidmorizon sidmorizon commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Security review of the Prime Transfer E2EE relay. The transport is genuine end-to-end encryption — the real key is derived client-side from the out-of-band pairing code plus an ECDHE exchange and never reaches the server — so this focuses on the relay's authorization and hardening, not the crypto.

Changes

1. Enforce room membership on the client-to-client relay (main fix)

JsBridgeE2EEServer relayed e2ee-c2c-request / e2ee-c2c-response to the roomId taken from the client envelope without checking the sender had joined that room. Any connected socket that knew a room id could inject c2c traffic into a live session — cancelTransfer (rate-limit exempt) to abort a transfer, verifyPairingCode spam to exhaust the peer's pairing-attempt budget, or a forged response to a pending request. Confidentiality was never at risk (payloads use the pairing-code-derived key the attacker lacks), but it was an unauthenticated disruption primitive.

The relay now drops any c2c message whose sender socket is not a member of the target room (socket.rooms.has(roomId)). Legitimate peers always join via joinRoom before sending c2c, so genuine traffic is unaffected.

2. Stop leaking server stack traces to clients

Every error response carried the full server stack — absolute file paths and internal call structure — back to the client.

Dropping stack from E2eeError.toJSON() does not fix this: toJSON() never runs on the wire path. JsBridgeBase.createPayload() replaces payload.error with its own plain copy first (toPlainError), and that copy reads err.stack straight off the instance. The response path is responseErrorsendcreatePayloadsendPayload, so the serializer is bypassed entirely.

The stack is now stripped in JsBridgeE2EEServer.sendPayload() — the single egress point every response passes through — so it covers E2eeError, the rate-limit errors raised by buildRateLimitResponder, and anything else thrown inside an API method. toJSON() keeps its stack-free shape as defence in depth for any path that serializes an E2eeError directly, and pino still records the full stack server-side.

Verified against a real server on both builds. Before: a joinRoom call with an invalid roomId returned error keys: ['name', 'message', 'stack', 'code'] with absolute server paths, and rate-limit errors leaked the bridge internals the same way. After: error keys: ['name', 'message', 'code'] for both.

3. Clarify the room encryptionKey

Documented that it is not the E2EE key (clients never use it) and is reserved for a future transport-layer encryption, so it isn't mistaken for the secret protecting user data.

4. Remove misleading unenforced CORS allowlist

The corsOrigins allowlist (read from CORS_ORIGINS) was built but never enforced — the origin callback returned true for every origin. Replaced with an explicit permissive config (same behavior) and documented why Origin is not an auth boundary here: native/desktop clients send no usable Origin, there are no cookie credentials to protect, and Origin is forgeable by non-browser clients.

The docs that advertised the removed knob went with it, so a deployer no longer goes looking for a filter that does not exist: the CORS_ORIGINS config-table row and both .env examples, env.example, and the entry in the root README.md. The prose that presented CORS as protection was corrected too — the security-feature list now names the c2c room membership check instead, and the "configure CORS origins appropriately" best practice and the Troubleshooting "CORS Issues" entry now state that CORS is deliberately permissive with nothing to configure, pointing at the corsOptions comment in src/server.ts.

Testing

Extended test/smoke.ts:

  • a socket that never joined a room cannot inject c2c into it, and the two real members keep communicating afterward
  • an error response carries no stack, asserted on a dedicated client so the per-connection, per-method rate limiter cannot perturb the other assertions. Confirmed as a real regression test: it fails when the sendPayload strip is removed and passes with it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AuowqYp863fJaNNjEKvhPT


Generated by Claude Code

claude added 2 commits August 27, 2026 09:18
- Reject client-to-client messages whose sender socket has not joined the
  target room, closing an unauthenticated cross-room injection path
  (cancelTransfer / verifyPairingCode / forged-response hijack) that was
  gated only by knowledge of the room id.
- Drop the stack trace from E2eeError.toJSON so server internals are not
  serialized to clients; pino still records it server-side via err.stack.
- Document that the room encryptionKey is not the E2EE key and is currently
  unused, reserved for a future transport-layer encryption.
- Add a smoke-test assertion that a non-member cannot inject c2c traffic.
The corsOrigins allowlist (read from CORS_ORIGINS) was built but never
enforced: the origin callback returned true for every origin, with the reject
branch commented out. Replace it with an explicit permissive config and
document why Origin is not an auth boundary here - the primary clients (native,
desktop file://) send no usable Origin, there are no cookie credentials to
protect, and Origin is forgeable by non-browser clients. Access control remains
the out-of-band pairing code plus the c2c room membership check.

@sidmorizon sidmorizon 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.

Review 结论:c2c relay 的 room membership 检查(主修复)正确且有效,本地 tsc + test/smoke.ts 全部通过;把 guard 去掉后新增断言会如期 FAIL(INJECTED - membership check bypassed),说明测试是有效的。CORS 改动(origin: true、去掉 credentials: true)核对过客户端 io() 配置(默认 withCredentials: falsepolling+websocket),行为等价、无风险。

需要修一处:E2eeError.toJSON() 去掉 stack 并不能阻止 stack 发到客户端(见 inline comment,已实测)。另外 packages/transfer-server/README.mdpackages/transfer-server/env.example、根 README.md 仍在文档化已删除的 CORS_ORIGINS,建议一并清理。


Generated by Claude Code

Comment thread packages/transfer-server/src/errors.ts Outdated
Removing `stack` from E2eeError.toJSON() did not stop the leak: toJSON() is
never reached on the wire path. JsBridgeBase.createPayload() replaces
payload.error with its own plain copy first (toPlainError), and that copy reads
err.stack straight off the instance, so responseError -> send -> createPayload
-> sendPayload emitted the full server stack. Verified against the previous
build: a joinRoom call with an invalid roomId returned `['name', 'message',
'stack', 'code']` with absolute server paths, and rate-limit errors leaked the
bridge internals the same way.

Strip `stack` in JsBridgeE2EEServer.sendPayload() instead - the single point
every response passes through, so it covers E2eeError, rate-limit errors, and
anything else thrown inside an API method. toJSON() keeps its stack-free shape
as defence in depth for direct serialization, with its comment corrected to
stop claiming it is what protects the socket path.

Add a smoke assertion on a dedicated client (so the per-connection, per-method
rate limiter does not perturb the other checks) that an error response carries
no `stack`. Confirmed it fails when the strip is removed and passes with it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AuowqYp863fJaNNjEKvhPT
CORS_ORIGINS no longer exists - the allowlist it fed was never enforced and was
replaced by an explicit permissive config. The docs still advertised it, so a
deployer would set it and believe origins were being filtered.

Remove it from the transfer-server config table, both example .env blocks, and
env.example, and rewrite the "CORS Issues" troubleshooting entry to say that
CORS is deliberately permissive with nothing to configure, pointing at the
corsOptions comment in src/server.ts for why Origin is not the auth boundary.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AuowqYp863fJaNNjEKvhPT
The CORS_ORIGINS cleanup missed the two prose claims that sent a deployer
looking for the knob it removed: the transfer-server security feature list
called CORS "Configurable CORS origins" and both READMEs advised configuring
origins appropriately. Neither was ever true - the allowlist was built but
never enforced, and it is gone now.

Replace the feature-list entry with the control that actually gates the relay
(the c2c room membership check) and rewrite the best-practice lines to say
Origin is not the auth boundary here, pointing at the corsOptions comment in
src/server.ts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AuowqYp863fJaNNjEKvhPT
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