MDEV-40878 avoid repeated parsing of PROXY headers - #5630
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The new guard won’t prevent repeated parsing when the first header is PROXY v1 UNKNOWN / PROXY v2 LOCAL because using_proxy_protocol is not set in that path, allowing a second header to still be parsed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to address MDEV-40878 by preventing repeated parsing of HAProxy PROXY protocol headers on a single connection, aligning with the spec assumption that only one PROXY header appears at the start of the connection.
Changes:
- Add an early return in
handle_proxy_header()to skip proxy-header parsing once the connection is already marked as using the proxy protocol. - Add a client test that sends two concatenated PROXY v1 headers and asserts the connection fails.
- Register the new test in the existing proxy-header test suite.
File summaries
| File | Description |
|---|---|
tests/mysql_client_test.c |
Adds a regression test for multiple PROXY headers and wires it into the proxy protocol test group. |
sql/net_serv.cc |
Adds a guard to avoid re-processing PROXY headers once NET::using_proxy_protocol is set. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /* Test that multiple PROXY headers are rejected (MDEV-40878) */ | ||
| static void test_proxy_header_multiple() | ||
| { | ||
| MYSQL *m; | ||
| const char header[]= "PROXY TCP4 192.0.2.1 127.0.0.1 12345 3306\r\n" | ||
| "PROXY TCP4 192.0.2.2 127.0.0.1 12346 3306\r\n"; |
Do not parse multiple proxy headers. The protocol spec says the header is at the beginning of a connection, so there could only be one. According to example in the spec, in multi-layer scenarios, where several proxies are involved between client and backend, the intermediate proxy is supposed to be configured to replace the existing header. Thus multiple headers are not expected.
d73cfa7 to
caac9d7
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The server-side change is narrowly scoped to the PROXY-header retry path and is backed by a targeted regression test covering the intended failure mode.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
Do not parse multiple proxy headers. The protocol spec says the header is at the beginning of a connection, so there could only be one.
According to example in the spec, in multi-layer scenarios, where several proxies are involved between client and backend, the intermediate proxy is supposed to be configured to replace the existing header. Thus multiple headers are not expected.