Skip to content

Fix streaming RPC frame validation - #3481

Open
wasphin wants to merge 1 commit into
apache:masterfrom
wasphin:fix-streaming-rpc-frames
Open

Fix streaming RPC frame validation#3481
wasphin wants to merge 1 commit into
apache:masterfrom
wasphin:fix-streaming-rpc-frames

Conversation

@wasphin

@wasphin wasphin commented Aug 23, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Streaming RPC frames are consumed directly by the streaming parser and do not pass through InputMessenger's authentication hook. A stream could also accept a frame carrying its ID from a socket other than the socket that created the stream.

What is changed and the side effects?

Changed:

  • Reject streaming frames on server sockets when configured authentication has not completed successfully.
  • Expose the socket authentication state needed by the streaming parser.
  • Keep server-accepted streams bound to the socket of their creating RPC and reject frames arriving from another socket.
  • Add separate unit tests for direct unauthenticated frame rejection and the complete client/server authentication and streaming service path.
  • Use an OS-assigned port in the service-path test to avoid test-environment port conflicts.

Side effects:

  • Performance effects: One authentication-state check per server-side streaming frame when server authentication is configured, plus a socket identity check before dispatching a frame to a stream.

  • Breaking backward compatibility: No. Authenticated streaming RPC behavior and the wire format are unchanged.


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR strengthens the server-side streaming RPC protocol handling in bRPC by ensuring streaming frames are subject to authentication state and by preventing streams from accepting frames over a different socket than the one that established/bound the stream via the creating RPC.

Changes:

  • Enforce server-side rejection of streaming frames when server authentication is configured but the socket has not successfully authenticated.
  • Bind server-accepted streams to the creating RPC’s socket and drop frames delivered on “foreign” sockets.
  • Add unit tests covering unauthenticated streaming-frame rejection and end-to-end authenticated RPC + streaming behavior (including using an OS-assigned port to avoid conflicts).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/brpc_streaming_rpc_unittest.cpp Adds unit tests for unauthenticated-frame rejection and authenticated service-path streaming.
src/brpc/stream.cpp Tightens socket binding behavior for streams; rejects frames from mismatched sockets.
src/brpc/stream_impl.h Tracks whether a stream is server-accepted to enforce socket-binding rules.
src/brpc/socket.h Exposes a new Socket::IsAuthenticated() API for protocol-layer checks.
src/brpc/socket.cpp Implements Socket::IsAuthenticated() using the existing auth flag/error state.
src/brpc/policy/streaming_rpc_protocol.cpp Adds an authentication-state gate for server-side streaming frame parsing (when invoked with server context).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/brpc/stream.cpp
Comment on lines 771 to 773
if (_host_socket != nullptr) {
return 0;
return _host_socket->id() == host_socket->id() ? 0 : -1;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated all existing SetHostSocket callers to handle binding failures. The server response paths now fail the affected streams instead of marking them connected, and the client extra-stream path fails the request streams and returns.

Comment thread test/brpc_streaming_rpc_unittest.cpp Outdated
Comment on lines +125 to +126
brpc::Server server;
server._options.auth = &auth;

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the test to configure authentication through public ServerOptions and Start(0, ...). It continues to use an OS-assigned port.

@wasphin
wasphin force-pushed the fix-streaming-rpc-frames branch from 63bc640 to 50ba0db Compare August 23, 2026 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants