Skip to content

Close two paths that reach services past their intended gate - #3512

Open
chenBright wants to merge 1 commit into
apache:masterfrom
chenBright:fix_intended_path
Open

Close two paths that reach services past their intended gate#3512
chenBright wants to merge 1 commit into
apache:masterfrom
chenBright:fix_intended_path

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

Paths that differ only in empty segments dispatched identically. FindMethodPropertyByURIImpl splits
the path with a StringSplitter that skips empty fields, so //flags, /flags// and /flags//port all resolved
to the same method as /flags. RFC 3986 treats //foo and /foo as distinct paths, so a front proxy whose
ACL matches the collapsed form does not match the padded one and passes it through — //flags?setvalue=
reaches a builtin service that /flags cannot. Normalizing the path server-side would not help, because the
proxy has already forwarded the padded literal; only rejecting it removes the differential.

The same audit found that HTTP/2 never validated :path. RFC 9113 8.3.1 requires it to be non-empty and,
apart from the asterisk-form used by OPTIONS, to begin with /.

What is changed and the side effects?

Changed:

  • FindMethodPropertyByURI rejects any path containing //. The check lives in
    the funnel rather than in FindMethodPropertyByURIImpl, so it also covers the
    global restful map, which does its own NormalizeSlashes.
  • H2StreamContext::ConsumeHeaders rejects a :path that is empty or does not
    begin with /, unless it is exactly *.

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


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 hardens bRPC’s HTTP dispatch and HTTP/2 header parsing to prevent “padded path” variants (with empty segments like //) and invalid HTTP/2 :path values from reaching services (notably builtin services) past intended proxy/ACL gates.

Changes:

  • Reject HTTP paths containing empty segments (//) by default in FindMethodPropertyByURI, with a new opt-out flag -http_allow_empty_path_segments to preserve legacy behavior.
  • Enforce RFC 9113 :path validation in HTTP/2 header consumption: non-empty and starting with / unless exactly *.
  • Update/add unit tests to cover the new rejection behavior and the opt-out flag behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/brpc/policy/http_rpc_protocol.cpp Adds -http_allow_empty_path_segments and rejects // paths by default in method resolution.
src/brpc/policy/http2_rpc_protocol.cpp Rejects invalid HTTP/2 :path values (empty or not starting with /, except *).
test/brpc_server_unittest.cpp Updates server-side HTTP routing tests and adds a scoped flag helper for legacy behavior.
test/brpc_http_message_unittest.cpp Updates FindMethodPropertyByURI tests and adds coverage for rejecting empty path segments + flag override.
test/brpc_http_rpc_protocol_unittest.cpp Adds HTTP/2 unit test verifying :path validation behavior.

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

Comment thread test/brpc_http_message_unittest.cpp
@chenBright
chenBright requested a review from wasphin September 1, 2026 15:41
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