Repository navigation
feat: Serve PBJ gRPC services over gRPC-Web - #978
Neeharika-Sompalli wants to merge 7 commits into
Conversation
Neeharika-Sompalli
changed the title
JUnit Test Report 545 files + 17 545 suites +17 48s ⏱️ +17s Results for commit ec583de. ± Comparison against base commit 5623c5a. This pull request removes 9 and adds 203 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Integration Test Report 619 files +1 619 suites +1 48m 5s ⏱️ + 4m 11s Results for commit ec583de. ± Comparison against base commit 5623c5a. This pull request removes 2 and adds 3 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
jasperpotts
left a comment
jasperpotts
left a comment
There was a problem hiding this comment.
Security & correctness review: gRPC-Web support
The core protocol handling is sound. Framing, base64 text mode, CORS allowlisting, content-type checks and grpc-message encoding are all done correctly. However, there are three easy, unauthenticated ways to tie up server resources (DoS). I'd treat them as blockers before this is exposed to the public internet.
How this was verified: a throwaway test started a real Helidon server with PbjRouting and PbjGrpcWebService and attacked it with raw sockets over HTTP/1.1. The results are quoted below. I also checked the code against known gRPC-Web and gRPC CVE patterns.
🔴 Critical: must fix before internet exposure
C1. An empty or truncated request body hangs the call forever
PbjGrpcWebService.java:312-315, :391-394
If the body ends with zero complete messages, the worker calls requests.clientEndStreamReceived(). For unary and server-streaming calls that does nothing (Pipelines.java:535, :810), so nothing ever completes the call. With no grpc-timeout, done.await() waits forever. A partial final frame is silently dropped: readRequests never checks deframer.isMidMessage().
Probe results:
POST /greeter.Greeter/sayHellowithContent-Length: 0got no response at all.- A frame missing its last 2 bytes got no response at all.
- An empty body sent to the server-streaming method got no response at all.
Each such request permanently pins:
- the request thread
- the socket, which stays in CLOSE_WAIT after the client leaves, so file descriptors leak
- the Helidon connection, and the service pipeline
One attacker with a loop of tiny POSTs can use up the server's file descriptors and memory.
Fix:
- For
UNARYandSERVER_STREAMING, require exactly one request message. At EOF, zero messages or a partial frame should fail withINTERNALorINVALID_ARGUMENT, as grpc-java does ("No request" / "Too many requests"). - Always fail on
isMidMessage()at EOF.
C2. One request can start thousands of server-streaming calls
PbjGrpcWebService.java:367, Pipelines.java:791-807
For server-streaming, every request message calls method.apply(...) again, all writing to the same response. readRequests keeps reading until EOF because the call isn't complete.
Probe result: one HTTP request carrying 1,000 empty 5-byte frames (5 KB in total) made 1,000 calls to sayHelloStreamReply. For block-node subscriptions, every call starts a long-lived session.
Fix: the same one-message rule as C1, enforced in the web service, which already knows MethodType. The native handler has the same flaw, but there it's only reachable by native gRPC clients. This PR puts it in reach of any browser.
C3. Compressed requests are decompressed without any size limit
GrpcDeframer.java:118, GrpcCompression.java:74-86, ZstdGrpcTransformer.decompress
maxMessageSizeBytes is checked against the compressed size only. Gzip and zstd decompress into an unbounded ByteArrayOutputStream.
Probe result: an 8,175-byte gzip request, under the 10 KB limit, reached the service as 8,388,608 bytes, about 1000× larger. Zstd can compress zeros at far higher ratios. Many concurrent requests like this will run the JVM out of memory.
Other gRPC libraries have shipped CVEs for exactly this pattern (size limit checked only after decompressing). The bug predates this PR on the native side, but this PR exposes it to the internet.
Fix: pass a limit into Decompressor, or wrap the stream and count bytes, then abort with RESOURCE_EXHAUSTED once output exceeds maxMessageSizeBytes.
🟠 High
H1. Only the client can bound how long a call runs
PbjGrpcWebService.java:481-488, class javadoc :93-96
The javadoc says "use a finite grpc-timeout to bound retention", but grpc-timeout is set by the client, and an attacker simply leaves it out. 99999999H (about 11,000 years) is also accepted.
- There's no server-side maximum or default deadline.
- There's no limit on concurrent calls.
- Helidon's
HttpServiceAPI can't see an HTTP/2 stream reset by the client. That is the same shape as the HTTP/2 "Rapid Reset" attack (CVE-2023-44487): the client resets each stream, but the server-side work carries on.
Fix: add Builder.maxTimeout(Duration) and defaultTimeout(Duration) that clamp the effective deadline, and possibly a maxConcurrentCalls limit. Document the Helidon limits that should be set (max-concurrent-requests, connection limits, idle timeouts).
H2. A client that stops reading pins threads, and the deadline can't free them
PbjGrpcWebService.java:529, :581-597
onNext writes to the socket while holding lock. Over HTTP/1 a blocking socket write has no timeout. When the deadline fires:
notifyFailure()runs.finishResponse()callslock.lock(), which can't be interrupted, so it blocks behind the stuck writer.worker.interrupt()comes after that, so it never runs.
Probe result: six seconds after a 1-second deadline, onError had been delivered, but the writer thread was still blocked in onNext. It was only freed when the client closed the socket. The lifecycle test deadlineCleanupCanReleaseABlockedAsynchronousResponseWrite only passes because its fake stream gives way when cleanup signals it. A real socket doesn't.
Fix: use tryLock(timeout) in finishResponse. On expiry, close or abort the connection instead of waiting for the writer. Add a real slow-reader test on raw sockets.
H3. A 5-byte header makes the server allocate the full max message size
GrpcDeframer.java:186
new byte[length] is allocated as soon as the 5-byte prefix arrives. With the block node's larger maxMessageSizeBytes, each stalled connection that sent only a prefix holds a full-size buffer, and with H1 there's no server-side limit on how long. For example, 1,000 connections × 4 MB = 4 GB of heap.
Fix: allocate based on the bytes actually received, e.g. grow in 64 KB steps.
🟡 Medium: correctness and integration
M1. Whether zstd works depends on whether native gRPC traffic has arrived yet
PbjProtocolHandler.java:73
The zstd compressor is registered in PbjProtocolHandler's static initializer. That class is only loaded on the first native gRPC request.
Probe result: on a fresh server, a web request with grpc-encoding: zstd returned grpc-status: 12 Decompressor is not installed. After any native call, the same request works.
Fix: register zstd somewhere loaded by both paths, e.g. a static block in PbjGrpcWebService or a shared registry init in pbj-grpc-common.
M2. The native gRPC deadline fix changes production behaviour, but the PR says "no behaviour change"
PbjProtocolSelector.java:50-52
The fix is correct: deadlines were previously scheduled with an absolute nanoTime value, so they effectively never fired. But native gRPC deadlines now actually fire, including on long-lived consensus-node and block-node streams where clients set grpc-timeout. The PR text says PbjProtocolHandler has "no behaviour change", which is wrong for this and for the new writeLock. Please call it out in the release notes and test it against the consensus node.
M3. onError and onNext can reach the service at the same time
PbjGrpcWebService.java:350-358
notifyFailure calls pipeline.onError() on a new virtual thread while the worker may still be inside requests.onNext(). The Flow / Reactive Streams rules require signals to a subscriber to be serial. Service pipelines, including generated Pipelines, have no synchronization. The native deadline path now does the same thing (PbjProtocolHandler.deadlineExceeded).
M4. catch (Throwable) swallows OutOfMemoryError
PbjGrpcWebService.java:316
catch (final Throwable e) in dispatch turns OutOfMemoryError (e.g. from C3) and other Errors into ordinary gRPC failures. Catch Exception, or rethrow VirtualMachineError.
M5. Web and native gRPC are configured and measured separately
- The service is registered twice, and settings like
maxMessageSizeBytesmust be copied by hand viaconfig(PbjConfig). If you forget, the web endpoint silently uses the 10 KB default, which differs from native gRPC. - Both paths use the same
pbj.grpc.*metric names with no transport tag, so web traffic and its failures can't be told apart from native traffic. - Consider
PbjRouting.builder().service(svc).grpcWeb(webConfig)or similar, so one registration serves both and shares config, plus atransport=grpc-webmetric tag.
M6. All request headers, including cookies, are passed to the service as metadata
PbjGrpcWebService.java:493-495
Everything except grpc-* is passed through, including cookie, authorization, origin, host and hop-by-hop headers. That matches native gRPC, but browsers attach cookies automatically to same-origin requests. If a service ever trusts cookie metadata for auth, its CSRF safety rests entirely on the content-type check and preflight. Please document this, and consider stripping cookie unless the user opts in.
🟢 Low
- L1. The compressed flag is accepted with
grpc-encoding: identity, and the still-compressed bytes are passed straight to the parser. The spec says this should fail withINTERNAL. - L2. The CORS builder doesn't validate origins. A trailing slash, uppercase letters or a path silently never match.
"null"can be allowlisted with no warning, and allowing thenullorigin is a known anti-pattern because sandboxed iframes can send it. - L3.
Vary: Originis only sent when the origin is allowed. With a non-wildcard allowlist it should be on every response. - L4. Several error messages echo attacker-controlled header values (content-type, grpc-encoding). They are percent-encoded, so this is low risk, but it's unnecessary.
- L5. The PR description says
GrpcTimeoutandGrpcRequestOptionslive inpbj-grpc-common. They were moved topbj-grpc-helidon. - L6. In the native handler, if
init()fails before the deframer is created and the client then sends DATA,data()throws aNullPointerException(nulldeframer, and the catch then hits the nullpipeline).
✅ Checked and defended
- CSRF via simple requests: anything other than
application/grpc-web*gets a 415 before the service is opened. That meanstext/plainsimple-request CSRF can't work, and browsers must send a preflight. - CORS: origins are matched exactly against a
Set, with tests for lookalike origins.Allow-Credentialsis never sent, so*can't be combined with credentials. - Header injection:
UriEncoding.encodeUriwas tested directly. It encodes CR, LF,%, NUL and non-ASCII, sogrpc-messagecan't inject headers or trailers (compare Armeria's CVE-2019-16771). grpc-timeoutparsing: limited to 8 digits, andTimeUnit.convertsaturates instead of overflowing (compare Envoy's CVE-2021-28682). The deadline comparisons handlenanoTimewraparound correctly.- Client frames with the
0x80trailers flag or reserved flag bits are rejected. The declared length is checked before allocation (but see H3). - Base64 text mode: chunk boundaries, padding mid-stream and invalid characters are all handled, and a truncated stream is rejected.
- HTTP/1 request smuggling: a request with a second HTTP request hidden in its unread body was not answered as a separate request, and
Connection: closeis set when the body isn't fully read. - Unary calls with multiple messages: only one call is made.
Missing test coverage, especially for attacks
Nothing currently tests:
- empty bodies and truncated final frames (C1)
- multiple request messages for server-streaming and unary calls (C2)
- decompression bombs, gzip and zstd (C3)
- a real slow-reader client on a raw socket over HTTP/1 and HTTP/2 (H2)
- stalled connections that sent only a length prefix (H3)
- a server-side maximum timeout (H1), once it exists
- zstd on a fresh server (M1)
- HTTP/2 RST_STREAM while a call is idle
- a wildcard-origin preflight, the
nullorigin, andVaryon rejected responses - JSON plus text mode, and Accept negotiation for JSON
- fuzzing
GrpcDeframerandBase64ChunkDecoderwith arbitrary chunk splits (the repo already has fuzz-test infrastructure inpbj-integration-tests) - a regression test confirming native gRPC deadlines now fire on long-lived streams (M2)
Also worth checking
- The build resolves Helidon 4.5.4. Check its HTTP/2 webserver against Helidon's recent security advisories before internet exposure.
- C2, C3 and H3 also exist on the native gRPC path. They aren't regressions, but this PR makes them reachable from any browser.
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Helidon 4.5.6 rejects a new HTTP/2 stream on a closed connection with an
IllegalStateException ("Connection is closed") from
Http2ClientConnection.addStream, where 4.5.4 failed the socket write with an
UncheckedIOException. The Helidon bump therefore broke
GrpcCloseClientConnectionTest and changed the exception PBJ client callers
see after the server closes the connection. PbjGrpcCall now converts that
failure back to an UncheckedIOException.
Signed-off-by: Neeharika-Sompalli <neeharika.sompalli@swirldslabs.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
880c525 to
d27fa49
Compare
| private void ensurePayloadCapacity(final int required) { | ||
| if (entityBytes == null) { | ||
| entityBytes = new byte[Math.min(entityLength, Math.max(8192, required))]; | ||
| } else if (required > entityBytes.length) { | ||
| final int capacity = (int) Math.min(entityLength, Math.max((long) required, entityBytes.length * 2L)); | ||
| entityBytes = Arrays.copyOf(entityBytes, capacity); | ||
| } | ||
| } |
There was a problem hiding this comment.
I understand the reason. But I also understand that this approach may result in allocating many smaller arrays instead of one large array. I'm curious to see what our JMH benchmarks show with this change compared to the original implementation.
There was a problem hiding this comment.
I ran the comparison, and the extra allocations do add up for larger fragmented messages. For a 2 MiB message, allocation roughly doubled and throughput was about 37% lower in this run. The benefit is that if a client sends only the header, we allocate about 120 bytes instead of reserving 2 MiB upfront.
I will address this issue and update the PR.
| Payload | Eager ops/s | Growing ops/s | Eager B/op | Growing B/op |
|---|---|---|---|---|
| 256 B | 30,454,583 ± 420,490 | 23,078,961 ± 475,446 | 416 | 424 |
| 8 KiB | 1,688,339 ± 56,186 | 1,693,707 ± 25,732 | 8,352 | 8,360 |
| 64 KiB | 293,198 ± 7,820 | 162,106 ± 1,929 | 65,696 | 123,096 |
| 2 MiB | 7,834 ± 355 | 4,925 ± 127 | 2,097,312 | 4,186,409 |
| Header only, announcing 2 MiB | 34,884 ± 1,111 | 58,507,954 ± 657,200 | 2,097,280 | 120 |
There was a problem hiding this comment.
Unlike the measurements above though, the results here look a bit concerning. The throughput seems to drop by some 30% or more, and allocations almost double. The only case that benefits is a broken client that wouldn't send data. I'm not entirely sure if this case is worth optimizing for. But it's concerning that legitimate use-cases slow down significantly.
@jasperpotts , what do you think?
Description:
Adds gRPC-Web support, so browsers can call PBJ gRPC services. It also fixes native gRPC deadlines not being enforced on the server.
Why the block node needs this
The block node wants browsers to call its APIs directly (swirldslabs/entente-block-node#32). Block node plugins expose their APIs as PBJ gRPC services, which today are only reachable over native gRPC. Browsers can't speak native gRPC, because they can't read HTTP trailers.
swirldslabs/entente-block-node#50 asks PBJ to make serving the same service over gRPC-Web a one-line addition on the same port. Once this is released, the block node can register the web service next to each gRPC service in
ServiceBuilderImpl.registerGrpcService, so every existing plugin gets gRPC-Web with no code change.Changes by module
pbj-grpc-helidonPbjGrpcWebService, a HelidonHttpServicethat serves aServiceInterfaceover gRPC-Web on HTTP/1.1 and HTTP/2:UNIMPLEMENTED, since gRPC-Web doesn't support itPbjProtocolSelectorletsapplication/grpc-web*requests fall through to theHttpRouting, instead of claiming them as native gRPC.PbjProtocolHandleruses the shared code moved intopbj-grpc-common; no behaviour change.grpc-timeoutdeadlines were scheduled with an absolute time instead of a delay, so they effectively never fired.pbj-grpc-common: new shared classes used by both handlers:GrpcDeframer,GrpcFramer,GrpcTimeout,GrpcRequestOptions, andBase64ChunkDecoderfor text mode.pbj-runtimeRequestOptionsadds the gRPC-Web content types, andisProtobuf()/isJson()recognize them, so existing generated code works unchanged.ServiceInterface.Methodaddstype()(unary, client/server streaming, bidirectional), which defaults toUNKNOWN.pbj-compiler: generated method enums now report each method'stype().docs: a usage-guide section on serving gRPC-Web, and an update to the architecture page.Related issue(s):
Part of swirldslabs/entente-block-node#50.
Notes for reviewer:
UNKNOWN.ServiceGenerator.javaand the testGreeterService.javawere reformatted by Spotless.pbj-coreandpbj-integration-testsbuilds pass.Checklist