fix(gateway): reject RPCs from invalidated device-token clients durin… by davidangularme · Pull Request #70707 · openclaw/openclaw

davidangularme · 2026-04-23T17:37:46Z

Summary

Problem: device.token.rotate, device.token.revoke, and device.pair.remove respond 200 OK then defer disconnectClientsForDevice via queueMicrotask. RPCs already pipelined in the WS buffer land with the rotated/revoked token before the socket closes.
Why it matters: An attacker can execute authenticated operations with a token that should already be invalidated — privilege persistence after revocation.
What changed: Added a synchronous invalidated flag on GatewayWsClient, set before respond() in all three handlers. Added a per-RPC dispatch guard that force-closes flagged clients. Introduced context.invalidateClientsForDevice() as a sync companion to the existing async disconnect. Defense-in-depth: disconnectClientsForDevice now also sets the flag.
What did NOT change (scope boundary): Shared-auth re-check path (already guarded at message-handler.ts:1444-1458), disconnect scheduling (still queueMicrotask), admin response flushing behavior.

Change Type (select all)

Bug fix
Security hardening

Scope (select all touched areas)

Gateway / orchestration
Auth / tokens

Linked Issue/PR

Related docs: add AGENTS.md for secrets and pairing high-privilege zones #71156 (documents the race in AGENTS.md as "open/in-flight")
This PR fixes a bug or regression

Root Cause (if applicable)

Root cause: queueMicrotask deferral of socket close created a window where the rotated/revoked token was still accepted. No per-RPC re-check existed for device-token auth (only for shared-auth).
Missing detection / guardrail: No synchronous invalidation mechanism; no test covering pipelined RPCs during the microtask window.
Contributing context: The shared-auth path already had per-RPC re-check (message-handler.ts:1444-1458); device-token auth was assumed to be covered by socket close, but socket close is async.

Regression Test Plan (if applicable)

Coverage level:
- Unit test
Target test or file: Gateway device-handler tests, request-context tests
Scenario: Pipelined RPCs after invalidateClientsForDevice() are rejected; invalidated flag is set before respond() returns.
Why smallest reliable guardrail: Unit test with flag-ordering assertions directly covers the race without requiring real WS timing.

User-visible / Behavior Changes

RPCs arriving on a device-token WS connection after token rotation/revocation are now rejected immediately instead of being processed until socket close completes.

Diagram (if applicable)

Before:
[rotate/revoke] -> [respond 200] -> [queueMicrotask: disconnect] -> [pipelined RPCs execute ✗]

After:
[rotate/revoke] -> [set invalidated flag] -> [respond 200] -> [pipelined RPCs hit dispatch guard → force-close] -> [queueMicrotask: disconnect]

Security Impact (required)

New permissions/capabilities? No
Secrets/tokens handling changed? Yes
New/changed network calls? No
Command/tool execution surface changed? No
Data access scope changed? No
Risk + mitigation: The invalidated flag is a new in-memory gate on RPC dispatch. Risk is false-positive invalidation; mitigated by setting the flag only in the three explicit revocation handlers and in disconnectClientsForDevice (which already terminates the connection).

Repro + Verification

Environment

OS: Linux (Ubuntu 24)
Runtime/container: Node 24

Steps

Establish a device-token WS connection
Pipeline multiple RPCs in the send buffer
Trigger device.token.rotate from admin
Observe whether pipelined RPCs execute

Expected

Pipelined RPCs are rejected after rotation.

Actual (before fix)

Pipelined RPCs execute with the rotated token until queueMicrotask fires socket close.

Evidence

Failing test/log before + passing after

Human Verification (required)

Verified scenarios: Flag ordering (set before respond), dispatch guard rejection, both rotate and revoke paths
Edge cases checked: disconnectClientsForDevice also sets flag (defense-in-depth); idempotent close on already-closed sockets
What I did not verify: Full integration test with real pipelined WS traffic under load

Review Conversations

I replied to or resolved every bot review conversation I addressed in this PR.
I left unresolved only the conversations that still need reviewer or maintainer judgment.

Compatibility / Migration

Backward compatible? Yes
Config/env changes? No
Migration needed? No

Risks and Mitigations

Risk: invalidated flag introduces a new code path in the hot RPC dispatch loop.
- Mitigation: Single boolean check, no allocation, no I/O. Consistent with existing sharedGatewayAuth guard pattern.

greptile-apps · 2026-04-23T17:40:42Z

Greptile Summary

This PR closes a race condition in the gateway where RPCs pipelined in a WebSocket buffer could be processed after a token rotation/revoke because the socket close was deferred via queueMicrotask. The fix introduces a synchronous invalidated flag on GatewayWsClient, set before the response is sent in device.pair.remove, device.token.rotate, and device.token.revoke, and checked at the top of the per-RPC dispatch path to force-close the connection immediately. The implementation is clean, well-tested, and consistent with the existing sharedGatewayAuth guard pattern.

Confidence Score: 5/5

Safe to merge — the fix is targeted, well-tested, and idempotent. No blocking issues found.

All findings are P2 or lower. The core security fix is correct: invalidated is set synchronously before respond(), the dispatch guard in message-handler.ts fires before any business logic, and the underlying close() call is idempotent. Tests cover the ordering invariant directly. No data-integrity or reliability regressions identified.

No files require special attention.

_{Reviews (1): Last reviewed commit: "fix(gateway): reject RPCs from invalidat..." | Re-trigger Greptile}

clawsweeper · 2026-04-29T04:28:21Z

Codex review: needs changes before merge.

Summary
The PR adds synchronous device-client invalidation before device removal, token rotation, and token revocation responses, plus a WebSocket dispatch guard and gateway tests for the invalidation ordering.

Reproducibility: yes. Current main gives a high-confidence source-level reproduction: the affected handlers respond before queueMicrotask disconnect, and the dispatcher lacks a device-token invalidation recheck before handler logic.

Next step before merge
The remaining automated repair is narrow: add the required changelog entry on the PR branch and rerun targeted/changed gates.

Security
Cleared: Security-sensitive gateway auth/session hardening reviewed; the diff is limited to gateway TypeScript source and tests and introduces no concrete supply-chain, permissions, workflow, dependency, or secret-handling regression.

Review findings

[P3] Add the required changelog entry — src/gateway/server-methods/devices.ts:309

Review details

Best possible solution:

Land this PR, or an equivalent narrow mainline patch, after adding the required changelog entry and confirming exact-head gateway checks.

Do we have a high-confidence way to reproduce the issue?

Yes. Current main gives a high-confidence source-level reproduction: the affected handlers respond before queueMicrotask disconnect, and the dispatcher lacks a device-token invalidation recheck before handler logic.

Is this the best way to solve the issue?

Yes. The in-memory invalidated flag plus pre-dispatch close is the narrowest maintainable fix I found for preserving response flushing while closing the queued-RPC window; the patch needs only merge-prep cleanup.

Full review comments:

[P3] Add the required changelog entry — src/gateway/server-methods/devices.ts:309
This changes user-visible gateway security behavior, but the PR does not touch CHANGELOG.md. Repo policy requires user-facing fixes to add an Unreleased Fixes entry before merge.
Confidence: 0.88

Overall correctness: patch is correct
Overall confidence: 0.88

Acceptance criteria:

pnpm test src/gateway/server-methods/devices.test.ts src/gateway/server-request-context.test.ts
pnpm test src/gateway/server/ws-connection.test.ts src/gateway/server.device-token-rotate-authz.test.ts
pnpm check:changed

What I checked:

current-main-handler-order: Current main still emits successful responses for device.pair.remove, device.token.rotate, and device.token.revoke before scheduling disconnectClientsForDevice in queueMicrotask, leaving the source-level queued-frame window described by the PR. (src/gateway/server-methods/devices.ts:305, 9bedcff904dd)
current-main-dispatch-guard: Current main has a per-RPC shared-gateway-auth generation recheck before handler dispatch, but no equivalent invalidated-device-token guard in the WebSocket request path. (src/gateway/server/ws-connection/message-handler.ts:1538, 9bedcff904dd)
pr-invalidates-before-respond: At PR head, the affected device handlers call invalidateClientsForDevice before respond(...) and keep the hard disconnect in the queued microtask. (src/gateway/server-methods/devices.ts:309, 779f657ed02e)
pr-dispatch-guard: At PR head, the WebSocket message handler closes an invalidated client before shared-auth recheck or handler dispatch. (src/gateway/server/ws-connection/message-handler.ts:1538, 779f657ed02e)
missing-changelog-entry: The PR changes user-visible gateway/security behavior but its changed-file list does not include CHANGELOG.md; current main has an Unreleased Fixes section available for the required entry. (CHANGELOG.md:14, 9bedcff904dd)
related-history: PR Gateway: disconnect revoked device sessions #55952 introduced disconnectClientsForDevice for removal/revoke with respond-before-disconnect behavior, and PR fix(gateway): disconnect active sessions after device token rotation #57646 later added active-session disconnect on token rotation in the same device-handler surface.

Likely related people:

steipete: Introduced unified device auth/pairing and then device-token auth/CLI, touching the central device handler and WebSocket message-handler surfaces. (role: original feature and gateway device-token maintainer; confidence: high; commits: 73e9e787b4df, d88b239d3c8a; files: src/gateway/server-methods/devices.ts, src/gateway/server/ws-connection/message-handler.ts, docs/cli/devices.md)
jacobtomlinson: Merged PR Gateway: disconnect revoked device sessions #55952 added disconnectClientsForDevice for device removal and token revoke and explicitly kept responses before session disconnects. (role: introduced relevant disconnect behavior; confidence: high; commits: 7a801cc451e9; files: src/gateway/server-methods/devices.ts, src/gateway/server-methods/types.ts, src/gateway/server.impl.ts)
vincentkoc: Merged PR fix(gateway): disconnect active sessions after device token rotation #57646 added active-session disconnect after device-token rotation in the same handler/test surface. (role: recent adjacent maintainer; confidence: medium; commits: 91f7a6b0fd67; files: src/gateway/server-methods/devices.ts, src/gateway/server-methods/devices.test.ts)

Remaining risk / open question:

No live pipelined WebSocket integration harness was run in this read-only review; the reproduction and fix assessment are source-level plus PR test-diff evidence.
The author reported one Opus 4.6 parity-gate failure as likely pre-existing, so exact-head CI still needs normal maintainer gating before merge.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 9bedcff904dd.

…g rotation/revoke race device.token.rotate, device.token.revoke and device.pair.remove all respond 200 OK to the admin, then schedule disconnectClientsForDevice via queueMicrotask so the response can flush before the socket close. That microtask window plus the absence of a per-RPC re-check for device-token auth (unlike shared-auth, which gets checked at message-handler.ts:1444-1458) created a race: an attacker with RPCs already pipelined in the WS socket buffer could land a few more authenticated operations with the rotated/revoked token before the socket actually closed. Fix: add a cheap in-memory 'invalidated' flag on GatewayWsClient and mark it synchronously *before* responding in the three handlers. Add a mirror check at the start of the per-RPC dispatch that force-closes the client if the flag is set, regardless of whether socket.close() has taken effect yet. Disconnect still happens via queueMicrotask so the admin's rotate/revoke response flushes normally. Introduces context.invalidateClientsForDevice(deviceId, opts) as a sync companion to the existing disconnectClientsForDevice. Also defense-in-depth: disconnectClientsForDevice now sets the flag too, so any other caller of the hard-disconnect path gets the per-RPC gate for free.

…tests check-test-types failed on the PR because direct 'as ReturnType<typeof vi.fn>' casts from RespondFn (or the optional context methods) don't structurally overlap with the Mock type — Mock has mockImplementation/mockReturnValue that RespondFn lacks, so strict tsgo rejects the conversion. vi.mocked() is the intended helper for reinterpreting an already-mocked function, and drops through to the Mock surface cleanly.

After rebasing onto upstream main, two test surfaces drifted: 1. GatewayRequestContextParams gained two required fields upstream (getRuntimeConfig, broadcastVoiceWakeRoutingChanged). The makeContextParams test helper was missing them, so every consumer tripped tsgo with a missing-field error. Add both as vi.fn() stubs. 2. revokeDeviceToken's return shape changed upstream from a bare entry record to a discriminated union {ok: true, entry: ...} | {ok: false, reason}. The new device.token.revoke synchronous-invalidate test still mocked the old shape, so the production handler took the !revoked.ok branch and never reached the invalidateClientsForDevice call the test asserted. Update the mock to the new union shape. Also fix three new Set([...] as never) sites in server-request- context.test.ts that produced Set<unknown> rather than Set<never>. Move the cast outside the Set constructor so the literal stays inferred while the wrapper is type-erased to never, which is assignable to the Partial<GatewayRequestContextParams> clients field.

davidangularme · 2026-05-01T05:18:25Z

Hi @steipete, @vincentkoc,

I've rebased the branch to align with the latest upstream changes. While the Opus 4.6 parity gate is failing on scenario 10, all 73 other checks—including the new unit tests for synchronous invalidation—are passing successfully.

This specific parity failure seems to be a pre-existing issue in the QA lab environment rather than a regression from the security fix. I've also addressed the blank-template triage flags in the PR description. Ready for your review.

…audit Gap 1 (Critical CWE-208): Replace !== with safeEqualSecret() for TLS fingerprint comparison in client.ts, eliminating timing side-channel attacks on WebSocket and HTTP TLS pinning. Refs: openclaw/openclaw#49083 Gap 2 (High revocation race): Add sync invalidated flag on GatewayClient, set before respond() in device.token.rotate/revoke and device.pair.remove handlers, with per-RPC dispatch guard in message-handler to reject pipelined RPCs from revoked credentials. Refs: openclaw/openclaw#70707 Gap 4 (High unbounded sessions): Add authenticated-connection-budget.ts per-device connection cap (default 8) with env override. Refs: openclaw/openclaw#59842 Gap 6 (High HTTP flood): Add request-rate-limit.ts sliding-window per-IP HTTP request rate limiter (default 120/min) for REST endpoints. Refs: openclaw/openclaw#44884 Gap 7 (High TLS enforcement): Add startup-security-checks.ts for network-exposure safety checks at startup. Refs: openclaw/openclaw#44884 Gap 8 (Medium auto HSTS): Auto-inject HSTS when TLS is active. Refs: openclaw/openclaw#44884 Gap 9 (Medium bind safety): Bind-all-interfaces detection via startup checks. Refs: openclaw/openclaw#44884 Gaps 3, 5, 10 already fixed in fork — no changes needed.

openclaw-barnacle Bot added gateway Gateway runtime size: M labels Apr 23, 2026

davidangularme force-pushed the fix/gateway-device-token-rpc-revalidation branch from 84c0364 to ba47c72 Compare April 29, 2026 04:35

openclaw-barnacle Bot added triage: blank-template Candidate: PR template appears mostly untouched. triage: refactor-only Candidate: refactor/cleanup-only PR without maintainer context. labels Apr 29, 2026

davidangularme mentioned this pull request Apr 29, 2026

fix(voice-call): close webhook in-flight limiter fail-open on empty remote address #74453

Merged

2 tasks

davidangularme added 3 commits April 30, 2026 06:19

davidangularme force-pushed the fix/gateway-device-token-rpc-revalidation branch from c006b1c to 779f657 Compare April 30, 2026 03:20

clawsweeper Bot mentioned this pull request Apr 30, 2026

docs: add AGENTS.md for secrets and pairing high-privilege zones #71156

Open

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(gateway): reject RPCs from invalidated device-token clients durin…#70707

fix(gateway): reject RPCs from invalidated device-token clients durin…#70707
davidangularme wants to merge 3 commits intoopenclaw:mainfrom
davidangularme:fix/gateway-device-token-rpc-revalidation

davidangularme commented Apr 23, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Apr 23, 2026

Uh oh!

clawsweeper Bot commented Apr 29, 2026 •

edited

Loading

Uh oh!

davidangularme commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

davidangularme commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Change Type (select all)

Scope (select all touched areas)

Linked Issue/PR

Root Cause (if applicable)

Regression Test Plan (if applicable)

User-visible / Behavior Changes

Diagram (if applicable)

Security Impact (required)

Repro + Verification

Environment

Steps

Expected

Actual (before fix)

Evidence

Human Verification (required)

Review Conversations

Compatibility / Migration

Risks and Mitigations

Uh oh!

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

Confidence Score: 5/5

Uh oh!

clawsweeper Bot commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidangularme commented May 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

davidangularme commented Apr 23, 2026 •

edited

Loading

clawsweeper Bot commented Apr 29, 2026 •

edited

Loading