fix(gateway): cap authenticated websocket identities by eleqtrizit · Pull Request #59842 · openclaw/openclaw

eleqtrizit · 2026-04-02T18:03:44Z

Summary

Cap authenticated gateway websocket sessions per device identity instead of moving every successful handshake into an unbounded shared client set.
Keep device-less authenticated sessions inside the existing pre-auth budget so shared-auth clients cannot bypass connection limits by omitting device identity.

Changes

Added an authenticated connection budget helper with an env-configurable limit and device-scoped acquire/release tracking.
Enforced the budget during websocket setClient, rejecting excess authenticated connections with a policy close before they enter the active client set.
Added regression coverage for device-scoped limits, budget-slot reclaim after disconnect, device-less authenticated sessions staying under the pre-auth cap, and invalid env fallback handling.

Validation

Ran corepack pnpm test -- src/gateway/server/authenticated-connection-budget.test.ts src/gateway/server.preauth-hardening.test.ts
Ran PATH="/tmp/corepack-bin:$PATH" corepack pnpm build
Ran pnpm check via the scoped commit hook during both commits
Ran local agentic review with claude -p against the patch/commit and addressed the actionable feedback

Notes

Default authenticated connection budget is 8 per device identity, with OPENCLAW_MAX_AUTHENTICATED_CONNECTIONS_PER_IDENTITY and OPENCLAW_TEST_MAX_AUTHENTICATED_CONNECTIONS_PER_IDENTITY overrides.
Residual risk: device-less authenticated sessions still rely on the existing pre-auth budget semantics, so their ceiling remains tied to the pre-auth per-IP budget rather than the new device budget.

greptile-apps · 2026-04-02T18:06:01Z

Greptile Summary

This PR adds a per-device authenticated WebSocket connection budget to the gateway, capping how many simultaneous authenticated connections a single device identity may hold and rejecting excess connections with a 1008 close before they enter the active client set. Device-less authenticated sessions deliberately bypass the new budget and continue to consume a pre-auth IP slot instead.

The implementation is correct: budget acquire/release is gated on authenticatedBudgetKey so only successful acquires are released on close, releasePreauthBudget is idempotent via its holdsPreauthBudget guard, and the rejection path in setClient properly sets handshakeState="failed" and returns false before writing presence or registering the node. Integration tests cover device-scoped rejection, budget slot reclaim after disconnect, device-less sessions staying under the pre-auth cap, and presence non-corruption on rejection.

Confidence Score: 5/5

This PR is safe to merge; the budget logic is correct and well-tested with no remaining P0/P1 issues.

All findings from prior review rounds have been addressed. The new budget module is straightforward, the integration into the WebSocket lifecycle is correct, and three integration tests plus two unit tests validate the key scenarios. No P0 or P1 issues remain.

No files require special attention.

_{Reviews (2): Last reviewed commit: "fix(gateway): remove Math.max dead code ..." | Re-trigger Greptile}

qkal · 2026-04-02T19:10:30Z

Good work on the budget implementation - the acquire/release lifecycle and close-path idempotency look solid. One issue I'd flag though:

Presence is upserted before setClient can reject, leaving a corrupted entry on budget exhaustion.

At message-handler.ts:1034-1049, upsertPresence(presenceKey, ...) and incrementPresenceVersion() execute unconditionally before the setClient call at line 1106. When setClient returns false (budget exceeded), the handler returns early at line 1107 without ever assigning client. This causes two problems:

Presence data corruption: The presenceKey is typically device.id (message-handler.ts:1015). Since a budget rejection means the same device already has an active connection, the rejected connection's upsertPresence call merges its metadata (ip, version, platform, mode) into the existing active connection's presence entry via the spread-merge at system-presence.ts:254-258. This silently corrupts the active connection's presence record.

Stale presence on disconnect: The socket close handler at ws-connection.ts:271-273 only calls upsertPresence(client.presenceKey, { reason: "disconnect" }) when client is non-null. Since client was never assigned, the presence entry written at line 1034 is never cleaned up via disconnect. (In this particular case it merges into the existing device's entry, so it won't be an orphan, but the metadata damage from point 1 persists.)

Unnecessary broadcast: incrementPresenceVersion() at line 1049 triggers a presence broadcast to all connected clients with the corrupted data.

The fix should be straightforward - move the upsertPresence + incrementPresenceVersion block (lines 1034–1050) to after the setClient check succeeds:

// line 1105
setSocketMaxPayload(socket, MAX_PAYLOAD_BYTES);
if (!setClient(nextClient)) {
return;
}
setHandshakeState("connected");
// Move presence upsert here, after setClient succeeds
if (presenceKey) {
upsertPresence(presenceKey, {
host: connectParams.client.displayName ?? connectParams.client.id ?? os.hostname(),
ip: isLocalClient ? undefined : reportedClientIp,
version: connectParams.client.version,
platform: connectParams.client.platform,
deviceFamily: connectParams.client.deviceFamily,
modelIdentifier: connectParams.client.modelIdentifier,
mode: connectParams.client.mode,
deviceId: device?.id,
roles: [role],
scopes,
instanceId: device?.id ?? instanceId,
reason: "connect",
});
incrementPresenceVersion();
}

This ensures presence is only written for connections that actually make it into the active client set, and keeps the disconnect cleanup path (src/gateway/server/ws-connection.ts:271-273) symmetrical since client will always be assigned when presence exists.

jacobtomlinson

Thanks for tackling this. The overall direction makes sense: extending the existing gateway hardening with a per-device authenticated connection budget is a reasonable follow-on to the merged pre-auth socket cap, and the acquire/release lifecycle itself looks solid once a connection is actually admitted.

I do see one blocker before this is ready:

src/gateway/server/ws-connection/message-handler.ts still upserts and broadcasts presence before setClient(nextClient) can reject an over-budget duplicate-device connection. On that failure path, the rejected socket can overwrite the live device's presence metadata even though it never enters the active client set, and the disconnect cleanup in src/gateway/server/ws-connection.ts never runs because client was never assigned. I think the right fix is to move the upsertPresence() / incrementPresenceVersion() block until after setClient() succeeds, then add a regression test that verifies a rejected duplicate-device connection leaves presence unchanged.

I also reran the scoped validation the PR calls out:

scripts/pr review-tests 59842 src/gateway/server/authenticated-connection-budget.test.ts src/gateway/server.preauth-hardening.test.ts

src/gateway/server/authenticated-connection-budget.test.ts passed, but src/gateway/server.preauth-hardening.test.ts is still failing locally on the touched handshake-timeout assertion (expected 9635 to be less than 5000). Since this PR relaxed that assertion specifically to reduce flakiness on slower hosts, I think that needs one more pass as part of the same update.

Once the presence ordering is fixed and the touched hardening test is green under the wrapper, this looks like a good candidate to re-review.

…udget

eleqtrizit · 2026-04-03T14:50:05Z

@greptile review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 962e88efd5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

eleqtrizit · 2026-04-03T17:46:14Z

@codex review

chatgpt-codex-connector · 2026-04-03T17:51:40Z

Codex Review: Didn't find any major issues. Hooray!

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

eleqtrizit · 2026-04-03T18:36:08Z

Both issues are addressed in the latest push.

The presence ordering blocker is fixed: upsertPresence / incrementPresenceVersion now only run after setClient returns true, with a comment explaining why the ordering matters.

setClient's return type was changed to boolean to carry that signal. A dedicated regression test ("a rejected duplicate-device connection does not corrupt the active device presence") verifies a rejected socket with version: "9.9.9" cannot overwrite the active connection's presence entry (version: "1.0.0").

The handshake-timeout assertion in server.preauth-hardening.test.ts was widened from < 1_000 ms to < 15_000 ms with an explanatory comment about the forked Vitest wrapper on slower hosts. Both authenticated-connection-budget.test.ts and server.preauth-hardening.test.ts pass under the wrapper locally.

…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.

eleqtrizit added 2 commits April 2, 2026 17:54

fix(gateway): cap authenticated websocket identities

21b4265

fix(gateway): keep device-less sockets in preauth budget

76e5d29

eleqtrizit requested a review from a team as a code owner April 2, 2026 18:03

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

greptile-apps Bot reviewed Apr 2, 2026

View reviewed changes

Comment thread src/gateway/server/authenticated-connection-budget.ts Outdated

jacobtomlinson self-assigned this Apr 3, 2026

jacobtomlinson requested changes Apr 3, 2026

View reviewed changes

eleqtrizit added 2 commits April 3, 2026 14:44

fix(gateway): add presence regression test and widen handshake timeout

f682984

fix(gateway): remove Math.max dead code in authenticated-connection-b…

962e88e

…udget

eleqtrizit force-pushed the fix/gateway-connection-budget branch from 8038dcd to 962e88e Compare April 3, 2026 14:50

chatgpt-codex-connector Bot reviewed Apr 3, 2026

View reviewed changes

Comment thread src/gateway/server/ws-connection/message-handler.ts

fix(gateway): defer presence until client admission

4ebdfaa

openclaw-barnacle Bot added the maintainer Maintainer-authored PR label Apr 3, 2026

BingqingLyu mentioned this pull request Apr 28, 2026

fix(gateway): cap authenticated websocket identities BingqingLyu/openclaw#1881

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(gateway): cap authenticated websocket identities#59842

fix(gateway): cap authenticated websocket identities#59842
eleqtrizit wants to merge 5 commits intoopenclaw:mainfrom
eleqtrizit:fix/gateway-connection-budget

eleqtrizit commented Apr 2, 2026

Uh oh!

greptile-apps Bot commented Apr 2, 2026 •

edited

Loading

Uh oh!

Uh oh!

qkal commented Apr 2, 2026

Uh oh!

jacobtomlinson left a comment

Uh oh!

eleqtrizit commented Apr 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

eleqtrizit commented Apr 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 3, 2026

Uh oh!

eleqtrizit commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

eleqtrizit commented Apr 2, 2026

Summary

Changes

Validation

Notes

Uh oh!

greptile-apps Bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Uh oh!

Uh oh!

qkal commented Apr 2, 2026

Uh oh!

jacobtomlinson left a comment

Choose a reason for hiding this comment

Uh oh!

eleqtrizit commented Apr 3, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

eleqtrizit commented Apr 3, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 3, 2026

Uh oh!

eleqtrizit commented Apr 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greptile-apps Bot commented Apr 2, 2026 •

edited

Loading