Repository navigation
Conversation
/agent-bridge/ws accepts the WebSocket upgrade before it can check any credential: the agent id and bridge token arrive in the first frame, and the token is validated only afterwards. Two things made that window unbounded. The 10 MiB read limit was set before the hello was read, so an unauthenticated peer could allocate a bridge-sized frame. The hello is now read under a 1 KiB limit, raised to 10 MiB only after the token checks out. Nothing limited concurrent unauthenticated connections either, so anonymous sockets could sit in the 10 s hello window as fast as they could connect. A process-wide budget of 128 pending handshakes now refuses excess upgrades with 503 before the socket is accepted, and the slot is released as soon as the token validates (or the connection ends). A legitimate bridge is unaffected: its hello is a few hundred bytes, it sends it immediately, and it never competes for more than one slot.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues — two minor suggestions inline.
Reviewed changes
- Pre-auth read limit —
helloReadLimit(1 KiB) now bounds thehelloframe; the 10 MiB bridge limit is only restored after the bridge token validates. - Handshake budget — a 128-slot process-wide semaphore gates
handleWSbeforewebsocket.Accept, answering503when saturated and releasing the slot on every exit path, including panics. - Docs — documents the handshake bound and warns against removing it when exempting the path from edge authentication.
- Tests — three new unit tests cover the oversized pre-auth frame (
1009), budget exhaustion (503), and slot release after a rejected hello.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
…e post-auth gap Copilot review on Paca-AI#708: wsjson.Read answers 1007 for a malformed hello, not the handler 4400, and the post-auth read-limit raise has no oversized-frame coverage yet.
Contributor
Author
CI on this PR has never executedBoth
That covers the package only. Lint and the full agent-runner suite have not run against this head, so I am not claiming them. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
/agent-bridge/wsis the daemon-facing WebSocket foracp-type agents. It callswebsocket.Acceptbefore it can check any credential — by design, the agentid and bridge token arrive in the first
helloframe. Two gaps made theunauthenticated window effectively unbounded:
conn.SetReadLimit(10 * 1024 * 1024)was set before the hello was read.A peer with no token could make the server buffer a bridge-sized frame.
helloTimeout(10 s) bounded one socket's lifetime, not how many could be opened, so an
anonymous flood could exhaust memory/fds on the agent-runner pod.
With the shipped deployment sizes (1 replica, 512 MiB limit), ~50 concurrent
10 MiB pre-auth frames were enough to OOMKill the process — which also serves the
environment/terminal endpoints, so it takes live bridges and environment
operations down with it. This matters more once an edge layer exempts the path
from browser authentication, since then the endpoint is reachable anonymously.
What changed
helloframe is read under a 1 KiB limit (helloReadLimit). Thedocumented 10 MiB bridge limit (
bridgeMessageReadLimit) is applied afterthe token validates, so legitimate event payloads are unchanged.
helloSlots,maxPendingHellos) refuses excess upgrades with 503 beforeAccept—there is no socket to allocate when the service is saturated. The slot is
released as soon as the token checks out, an early return happens, or the
handler panics.
docs/ai-agent/agent-runner-service.mddocuments the handshake bound, soanyone adding an auth exemption for this path knows not to remove it.
No configuration, interface or protocol change;
apps/acp-bridgeis untouched.Tests
services/agent-runner/internal/acpbridge/server_hello_test.go:TestHandleWSRejectsOversizedPreAuthFrame— a frame above the hello limit isclosed with
1009(StatusMessageTooBig). Without the fix the same frame isread at 10 MiB and fails JSON decoding with
1007instead.TestHandleWSRefusesWhenHandshakeBudgetIsExhausted— with the budget full,the endpoint answers
503. Without the fix it answers426(upgraderequired), i.e. it accepts the connection.
TestHandleWSReleasesHandshakeSlotAfterRejectedHello— a rejected helloreturns its slot, so failures cannot leak the budget.
Both failure modes were reproduced against the unpatched handler before the fix.
go build ./...,go vet ./internal/...andgo test ./internal/... -count=1pass (e2e suites not run — they need Docker/Postgres).
Follow-ups (not in this PR)
many bridges behind one NAT.
a browser-authenticated edge rather than exempting the path.