mirror of
https://github.com/morluto/rea.git
synced 2026-10-26 17:35:11 +00:00
fix(hopper): bound bridge request frames like the Cutter bridge (#5262)
* fix(hopper): bound bridge request frames like the Cutter bridge The Hopper bridge read each request with an unbounded readline, so a peer that never wrote a newline kept Hopper allocating for as long as it kept writing. The Cutter bridge already refuses a frame above MAX_FRAME_BYTES; the Hopper bridge had no request ceiling at all. Read the request line with a 16 MiB bound and refuse a line that fills the limit without a terminator, using the existing invalid_request diagnostic before closing the connection. Such a line cannot be answered or resynchronized, because its remainder would be read as the next request. The response direction is unchanged, so large decompilation results still flow, and every dispatchable method carries small bounded parameters. The stalled-line case keeps its connection open instead of half-closing it, so the test pins the exact ceiling: a bound one byte larger waits for a byte the peer never sends and fails instead of passing quietly. Validation: npm run check:fast (typecheck, lint) passed uncached, and the new boundary test passed against the real bridge over socket pairs at the frame limit and above it. Removing only the read bound makes the same test fail, and widening the bound to one byte past the limit fails it too. The Hopper boundary and bridge-packaging suites report the same 17 failing files and 126 failing tests before and after this change on this Windows host; those failures are POSIX-only probes (Unix socket directories, os.statvfs) unrelated to this diff. Native macOS Hopper and real-client traffic were not run. * fix(bridges): exclude line delimiter from payload limit * test(hopper): cover partial request at EOF --------- Co-authored-by: morluto <76467478+morluto@users.noreply.github.com>
This commit is contained in:
1 parent
b2048d46d1
commit
26573c3aa4
6 files changed
+266
-3
No files matched your search
@@ -625,12 +625,14 @@ class _BridgeServer(threading.Thread):
|
||||
data = bytearray()
|
||||
deadline = time.monotonic() + AUTH_FRAME_TIMEOUT_SECONDS
|
||||
while b"\n" not in data:
|
||||
if len(data) >= MAX_FRAME_BYTES:
|
||||
if len(data) > MAX_FRAME_BYTES:
|
||||
raise ValueError("frame-limit-exceeded")
|
||||
remaining = deadline - time.monotonic()
|
||||
if remaining <= 0:
|
||||
raise socket.timeout("authentication frame timed out")
|
||||
connection.settimeout(remaining)
|
||||
# MAX_FRAME_BYTES limits the JSON payload; the terminating LF is
|
||||
# framing and may be the one byte beyond that payload budget.
|
||||
part = connection.recv(min(65536, MAX_FRAME_BYTES + 1 - len(data)))
|
||||
if not part:
|
||||
raise ValueError("incomplete authentication frame")
|
||||
|
||||
+18
-1
@@ -14,6 +14,9 @@ from typing import Any, Optional, Protocol, Sequence
|
||||
BAD_ADDRESSES = (-1, 0xFFFFFFFFFFFFFFFF, None)
|
||||
# The native label setter truncates NSString names at this UTF-16 extent.
|
||||
HOPPER_SYMBOL_NAME_UTF16_UNITS = 1024
|
||||
# Maximum request JSON payload size in bytes, excluding its trailing LF.
|
||||
# Matches the Cutter bridge's MAX_FRAME_BYTES payload ceiling.
|
||||
MAX_REQUEST_BYTES = 16 * 1024 * 1024
|
||||
_rea_session_document = None
|
||||
_search_inventory_cache = {}
|
||||
_pseudocode_cache = {}
|
||||
@@ -1060,9 +1063,23 @@ def _serve_connection(connection):
|
||||
should_stop = False
|
||||
try:
|
||||
while True:
|
||||
line = file.readline()
|
||||
# Bound the read itself. The payload budget excludes LF, so permit
|
||||
# one additional byte for the delimiter while still refusing an
|
||||
# oversized unterminated peer without waiting for EOF.
|
||||
line = file.readline(MAX_REQUEST_BYTES + 1)
|
||||
if not line:
|
||||
break
|
||||
if not line.endswith(b"\n"):
|
||||
message = (
|
||||
"Bridge request exceeds the %d-byte payload limit"
|
||||
% MAX_REQUEST_BYTES
|
||||
if len(line) > MAX_REQUEST_BYTES
|
||||
else "Incomplete bridge request frame"
|
||||
)
|
||||
_write_message(file, {"id": 0, "error": _safe_diagnostic(
|
||||
InvalidRequestError(message)
|
||||
)})
|
||||
break
|
||||
request_id = None
|
||||
authenticated = False
|
||||
should_stop = False
|
||||
|
||||
@@ -71,7 +71,11 @@ Cutter commands use the local REA Python bridge and authenticated loopback messa
|
||||
The client and plugin must support the current v2 handshake; older peers are
|
||||
rejected before dispatch because they cannot preserve cross-caller recovery proof.
|
||||
The client checks the final encoded request against the bridge's 16 MiB frame
|
||||
limit before dispatch; an oversized command does not retain a cleanup owner.
|
||||
limit before dispatch; the 16 MiB budget applies to request payload bytes and
|
||||
excludes the terminating LF. The bridge accepts that full payload size and
|
||||
rejects larger or incomplete frames while bounding the read; an oversized
|
||||
command does not retain a cleanup owner. Hopper uses the same request payload
|
||||
budget.
|
||||
Each bridge request waits 25000 ms. Discovery admits at most 256 descriptor
|
||||
candidates and reads and probes at most eight concurrently. A larger directory
|
||||
reports `bridge_directory_capacity_exceeded`. This release has no runtime
|
||||
|
||||
@@ -0,0 +1,87 @@
|
||||
import { execFile } from "node:child_process";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { promisify } from "node:util";
|
||||
|
||||
import { expect, it } from "vitest";
|
||||
import { z } from "zod";
|
||||
|
||||
const execute = promisify(execFile);
|
||||
|
||||
const bridgePath = new URL(
|
||||
"../../../../bridge/hopper_bridge.py",
|
||||
import.meta.url,
|
||||
);
|
||||
const probePath = new URL(
|
||||
"../../../fixtures/hopperBridgeFrameLimitProbe.py",
|
||||
import.meta.url,
|
||||
);
|
||||
|
||||
/** Mirrors the Cutter bridge's frame ceiling so both bridges bound identically. */
|
||||
const MAX_REQUEST_BYTES = 16 * 1024 * 1024;
|
||||
|
||||
const exchangeSchema = z.strictObject({
|
||||
reply: z.unknown(),
|
||||
next: z.unknown().optional(),
|
||||
thread_released: z.boolean(),
|
||||
dispatch_calls: z.number().int().nonnegative(),
|
||||
});
|
||||
|
||||
const probeResultSchema = z.strictObject({
|
||||
limit: z.number().int().positive(),
|
||||
legit: exchangeSchema,
|
||||
at_limit: exchangeSchema,
|
||||
past_limit: exchangeSchema,
|
||||
stalled_line: exchangeSchema,
|
||||
eof_partial: exchangeSchema,
|
||||
});
|
||||
|
||||
it("serves requests up to the frame limit and refuses a line that exceeds it", async () => {
|
||||
const { stdout } = await execute("python3", [
|
||||
fileURLToPath(probePath),
|
||||
fileURLToPath(bridgePath),
|
||||
]);
|
||||
const result = probeResultSchema.parse(JSON.parse(stdout));
|
||||
expect(result.limit).toBe(MAX_REQUEST_BYTES);
|
||||
|
||||
// Legitimate traffic is unaffected, including a request that fills the limit.
|
||||
for (const exchange of [result.legit, result.at_limit])
|
||||
expect(exchange.reply).toMatchObject({
|
||||
result: { run_id: "owned-run", ready: true },
|
||||
});
|
||||
|
||||
// A peer that keeps a line open past the limit is answered without reading
|
||||
// the line to completion. The refusal can be consumed by the peer's own
|
||||
// reset, so this asserts boundedness rather than the diagnostic's arrival.
|
||||
expect(result.past_limit.reply).not.toEqual({
|
||||
socket_error: "TimeoutError",
|
||||
});
|
||||
expect(result.past_limit.thread_released).toBe(true);
|
||||
|
||||
// An unterminated payload one byte over the limit is refused with a typed
|
||||
// diagnostic naming the limit and the connection is closed, instead of
|
||||
// resynchronizing mid-line. The peer stays connected, so this pins the
|
||||
// payload ceiling without charging the LF against it.
|
||||
expect(result.stalled_line.reply).toMatchObject({
|
||||
error: {
|
||||
code: -32000,
|
||||
type: "invalid_request",
|
||||
message: expect.stringContaining(String(MAX_REQUEST_BYTES)),
|
||||
},
|
||||
});
|
||||
expect(result.stalled_line.next).toEqual({ closed: true });
|
||||
expect(result.stalled_line.thread_released).toBe(true);
|
||||
|
||||
// A valid authenticated payload without LF is still an incomplete frame.
|
||||
// Half-closing the write side supplies EOF; the bridge rejects without
|
||||
// dispatching the operation, then closes its response connection.
|
||||
expect(result.eof_partial.reply).toMatchObject({
|
||||
error: {
|
||||
code: -32000,
|
||||
type: "invalid_request",
|
||||
message: "Incomplete bridge request frame",
|
||||
},
|
||||
});
|
||||
expect(result.eof_partial.next).toEqual({ closed: true });
|
||||
expect(result.eof_partial.dispatch_calls).toBe(0);
|
||||
expect(result.eof_partial.thread_released).toBe(true);
|
||||
});
|
||||
+21
@@ -592,6 +592,27 @@ class ProtocolProducerTest(unittest.TestCase):
|
||||
receiver.close()
|
||||
|
||||
|
||||
class RequestFrameLimitTest(unittest.TestCase):
|
||||
def test_payload_limit_excludes_the_line_terminator(self):
|
||||
class BufferedSocket:
|
||||
def __init__(self, data):
|
||||
self.data = data
|
||||
|
||||
def settimeout(self, _timeout):
|
||||
pass
|
||||
|
||||
def recv(self, count):
|
||||
part, self.data = self.data[:count], self.data[count:]
|
||||
return part
|
||||
|
||||
payload = b"{}" + b" " * (bridge.MAX_FRAME_BYTES - 2)
|
||||
self.assertEqual(
|
||||
bridge._BridgeServer._receive_frame(BufferedSocket(payload + b"\n")), {}
|
||||
)
|
||||
with self.assertRaisesRegex(ValueError, "frame-limit-exceeded"):
|
||||
bridge._BridgeServer._receive_frame(BufferedSocket(payload + b" " + b"\n"))
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
if sys.argv[1:] == ["--serve"]:
|
||||
run_serve_mode()
|
||||
|
||||
+132
@@ -0,0 +1,132 @@
|
||||
"""Exercise the shipped bridge's request-frame bound over real socket pairs.
|
||||
|
||||
Four cases share one loaded bridge module:
|
||||
|
||||
- ``legit``: an ordinary authenticated request is served.
|
||||
- ``at_limit``: a request with an exactly-limit JSON payload plus LF is served,
|
||||
so framing does not consume a payload byte.
|
||||
- ``past_limit``: a peer writes one byte past the limit and never terminates the
|
||||
line. An unbounded line read cannot answer this at all, which is what makes
|
||||
Hopper allocate for as long as the peer keeps writing.
|
||||
- ``stalled_line``: a peer exceeds the payload limit without a terminator,
|
||||
stops writing, and keeps the connection open. The bridge must refuse it
|
||||
without waiting for EOF, then close.
|
||||
- ``eof_partial``: an authenticated, otherwise valid short JSON request has no
|
||||
LF and the peer half-closes its write side. The bridge must reject the
|
||||
incomplete frame without dispatching it, then close.
|
||||
"""
|
||||
import json
|
||||
import socket
|
||||
import sys
|
||||
import threading
|
||||
|
||||
from hopperBridgeFacadeProbe import load_bridge
|
||||
|
||||
READ_TIMEOUT_SECONDS = 5
|
||||
|
||||
|
||||
def _next_response(file):
|
||||
while True:
|
||||
line = file.readline()
|
||||
if not line:
|
||||
return {"closed": True}
|
||||
value = json.loads(line.decode("utf-8"))
|
||||
if "event" not in value:
|
||||
return value
|
||||
|
||||
|
||||
def serve(bridge, line, read_after, half_close_write=False):
|
||||
"""Send one raw line and record the bridge's answer."""
|
||||
server, client = socket.socketpair()
|
||||
worker = threading.Thread(target=bridge["_serve_connection"], args=(server,))
|
||||
worker.daemon = True
|
||||
worker.start()
|
||||
file = client.makefile("rwb")
|
||||
client.settimeout(READ_TIMEOUT_SECONDS)
|
||||
outcome = {}
|
||||
dispatch_count = len(bridge["_probe_dispatches"])
|
||||
try:
|
||||
file.write(line)
|
||||
file.flush()
|
||||
if half_close_write:
|
||||
client.shutdown(socket.SHUT_WR)
|
||||
outcome["reply"] = _next_response(file)
|
||||
if read_after:
|
||||
outcome["next"] = _next_response(file)
|
||||
except OSError as error:
|
||||
outcome["reply"] = {"socket_error": type(error).__name__}
|
||||
finally:
|
||||
try:
|
||||
file.close()
|
||||
finally:
|
||||
client.close()
|
||||
worker.join(2)
|
||||
outcome["thread_released"] = not worker.is_alive()
|
||||
outcome["dispatch_calls"] = (
|
||||
len(bridge["_probe_dispatches"]) - dispatch_count
|
||||
)
|
||||
return outcome
|
||||
|
||||
|
||||
def request_line(bridge, params):
|
||||
request = {
|
||||
"id": 1,
|
||||
"token": bridge["REA_TOKEN"],
|
||||
"method": "health",
|
||||
"params": params,
|
||||
}
|
||||
return json.dumps(request, separators=(",", ":")).encode("utf-8")
|
||||
|
||||
|
||||
def main():
|
||||
bridge = load_bridge(sys.argv[1])
|
||||
bridge.update(REA_TOKEN="owned-token", REA_RUN_ID="owned-run")
|
||||
bridge["_probe_dispatches"] = []
|
||||
|
||||
def dispatch(method, params):
|
||||
bridge["_probe_dispatches"].append((method, params))
|
||||
return {"run_id": bridge["REA_RUN_ID"], "ready": True}
|
||||
|
||||
bridge["_dispatch"] = dispatch
|
||||
limit = bridge.get("MAX_REQUEST_BYTES")
|
||||
if not isinstance(limit, int) or isinstance(limit, bool) or limit <= 0:
|
||||
json.dump({"limit": limit}, sys.stdout)
|
||||
return
|
||||
|
||||
legit = serve(bridge, request_line(bridge, {}) + b"\n", False)
|
||||
|
||||
# The payload may fill the limit; the framing LF is additional.
|
||||
padding = limit - len(request_line(bridge, {"padding": ""}))
|
||||
at_limit_line = request_line(bridge, {"padding": "p" * padding}) + b"\n"
|
||||
assert len(at_limit_line) == limit + 1, len(at_limit_line)
|
||||
at_limit = serve(bridge, at_limit_line, False)
|
||||
|
||||
# One byte past the limit, still unterminated and still open.
|
||||
past_limit = serve(bridge, b"x" * (limit + 1), False)
|
||||
|
||||
# The payload limit plus one byte, without a terminator. The peer stays
|
||||
# connected so the bounded read must reject it without waiting for EOF.
|
||||
stalled_line = serve(bridge, b"x" * (limit + 1), True)
|
||||
|
||||
# This is a valid authenticated request, but omitting LF makes it an
|
||||
# incomplete frame. EOF must terminate the read and produce a rejection;
|
||||
# the application dispatch must never see it.
|
||||
eof_partial = serve(
|
||||
bridge, request_line(bridge, {}), True, half_close_write=True
|
||||
)
|
||||
|
||||
json.dump(
|
||||
{
|
||||
"limit": limit,
|
||||
"legit": legit,
|
||||
"at_limit": at_limit,
|
||||
"past_limit": past_limit,
|
||||
"stalled_line": stalled_line,
|
||||
"eof_partial": eof_partial,
|
||||
},
|
||||
sys.stdout,
|
||||
)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
main()
|
||||
Reference in new issue
Block a user