[ISSUE #10692] Tolerate malformed HAProxy port values

[Aias00]

Which Issue(s) This PR Fixes

Fixes #10692

Brief Description

HAProxyMessageForwarder.buildHAProxyMessage previously parsed source and destination ports with Integer.parseInt directly, both from proxy protocol attributes and from channel remote/local addresses. A malformed port could throw NumberFormatException and fail HAProxy message forwarding with a runtime parsing exception.

This PR adds safe port parsing. Invalid port values now make buildHAProxyMessage return null, matching the existing behavior for unavailable HAProxy message data, while valid port values keep the existing behavior.

The test also replaces Java assert statements with JUnit assertions in HAProxyMessageForwarderTest.

How Did You Test This Change?

mvn -pl proxy -Dtest=HAProxyMessageForwarderTest -DfailIfNoTests=false test

Result: BUILD SUCCESS, Tests run: 3, Failures: 0, Errors: 0, Skipped: 0.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[RockteMQ-AI]

Review by github-manager-bot

PR #10693: [ISSUE #10692] Tolerate malformed HAProxy port values
Scope: proxy module — HAProxy message forwarding

Changes Overview

• Adds parsePort(String) utility that safely parses port values, returning null on invalid input (non-numeric, empty, out of 0-65535 range)
• Replaces 4 direct Integer.parseInt() calls in buildHAProxyMessage with the safe parser
• On parse failure, buildHAProxyMessage returns null (consistent with existing unavailable-data path)
• Test: adds coverage for valid/invalid/out-of-range ports, replaces assert with JUnit assertions

Assessment

Correctness ✅

• Defensive fix prevents NumberFormatException from propagating up the HAProxy forwarding path
• Port range validation (0-65535) is a good addition beyond just catching parse errors
• Null return is consistent with the existing pattern for unavailable HAProxy data

Tests ✅

• Good coverage of edge cases (non-numeric, out-of-range)
• Replacing assert with JUnit assertions is correct — assert requires -ea JVM flag which is not guaranteed in all test runners

Compatibility ✅

• No public API changes
• Behavior change is intentional: malformed ports now silently drop instead of throwing

Suggestion

One minor suggestion — consider adding a log.warn() or log.debug() when port parsing fails in parsePort(). Silently returning null makes production debugging harder if a misconfigured HAProxy starts sending malformed ports. Something like:

private static Integer parsePort(String value) {
    if (value == null || value.isEmpty()) {
        return null;
    }
    try {
        int port = Integer.parseInt(value);
        if (port < 0 || port > 65535) {
            log.warn("Parsed port value out of range: {}", value);
            return null;
        }
        return port;
    } catch (NumberFormatException e) {
        log.warn("Failed to parse port value: {}", value);
        return null;
    }
}

This is optional and can be a follow-up. The core fix is solid.

Overall: LGTM 👍 — clean, focused defensive fix with appropriate test coverage.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 87.09677% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.27%. Comparing base (00e45b8) to head (b0d3ee9).

Files with missing lines	Patch %	Lines
...g/protocol/http2proxy/HAProxyMessageForwarder.java	87.09%	3 Missing and 5 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10693      +/-   ##
=============================================
- Coverage      48.32%   48.27%   -0.05%     
+ Complexity     13512    13507       -5     
=============================================
  Files           1380     1380              
  Lines         101091   101142      +51     
  Branches       13101    13108       +7     
=============================================
- Hits           48851    48831      -20     
- Misses         46279    46321      +42     
- Partials        5961     5990      +29     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Aias00]

Addressed the Codecov patch coverage gap in the latest commit.\n\nChanges added coverage for:\n- local channel destination port parse failure\n- PROXY_PROTOCOL attribute path with valid ports\n- PROXY_PROTOCOL source and destination malformed port values\n- port range boundaries and out-of-range values\n\nI also tightened parsePort to reject values outside 0..65535, matching HAProxy port semantics.\n\nLocal verification passed:\nmvn -pl proxy -Dtest=HAProxyMessageForwarderTest -DfailIfNoTests=false test

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

PR #10693: [ISSUE #10692] Tolerate malformed HAProxy port values
Re-review scope: New commit b57f00f — "Cover HAProxy port parsing branches"

Changes Since Last Review

• Added log.warn() in parsePort() for both out-of-range and unparseable ports — addresses the suggestion from the previous review
• Added comprehensive test coverage for the HAProxy port parsing branches (valid, invalid, out-of-range)

Assessment

Correctness — The logging addition is clean and does not change the control flow.
Tests — Good branch coverage for the port parsing utility.

Overall: LGTM — Feedback addressed, solid fix.

---

Automated re-review by github-manager-bot

[fuyou001]

[P1] Please avoid failing open when a malformed HAProxy port is present.

buildHAProxyMessage now returns null both when PROXY metadata is unavailable and when an advertised source/destination port is invalid. forwardHAProxyMessage treats null as "nothing to forward" and continues with ctx.fireChannelRead(msg). Previously the parsing exception aborted this path; after this change, a connection carrying malformed PROXY metadata continues to the backend without a PROXY header.

That can replace the real client address with the proxy address downstream and affect source-IP ACLs, rate limiting, tenant attribution, and audit logs. Please distinguish "metadata absent" from "metadata present but invalid" and reject/close the latter connection (or propagate an explicit failure) rather than silently continuing.

Please add regression tests asserting that malformed source and destination ports do not call ctx.fireChannelRead, do not forward application traffic without the PROXY header, and close or reject the connection. Tests should also cover missing individual attributes, empty values, IPv6, port boundaries, and failures/cancellation/interruption from writeAndFlush(...).sync().

[RockteMQ-AI]

Review by github-manager-bot (Re-review)

PR #10693: [ISSUE #10692] Tolerate malformed HAProxy port values
Re-review trigger: New maintainer feedback from @fuyou001 (2026-07-30)

Maintainer Feedback Summary

@fuyou001 raised a P1 security concern: buildHAProxyMessage now returns null for both "metadata absent" and "metadata present but invalid" cases. Since forwardHAProxyMessage treats null as "nothing to forward" and continues with ctx.fireChannelRead(msg), a connection with malformed PROXY metadata will silently proceed to the backend without a PROXY header — potentially replacing the real client address with the proxy address.

Assessment

The concern is valid and important. The parsePort method correctly validates port range (0–65535) and catches NumberFormatException, but the null return conflates two semantically different conditions:

1. No PROXY metadata available → legitimate case, skip forwarding
2. PROXY metadata present but malformed → should reject/close the connection

The current code path allows case 2 to fall through to ctx.fireChannelRead(msg), which can affect source-IP ACLs, rate limiting, tenant attribution, and audit logs downstream.

Suggested Fix Direction

• Distinguish the two cases: e.g., throw a specific exception or return a sentinel for "invalid metadata" vs. "no metadata"
• In forwardHAProxyMessage, catch the "invalid metadata" case and close/reject the connection rather than continuing
• Add regression tests asserting malformed ports do NOT call ctx.fireChannelRead and do NOT forward traffic without the PROXY header

Status

No new commits since the maintainer feedback. Awaiting author response.

---

Automated review by github-manager-bot

[Aias00]

Addressed the malformed PROXY metadata rejection coverage in b0d3ee90a.

Changes:

• Added channelRead regression coverage for out-of-range source/destination ports.
• Added channelRead regression coverage for missing and empty PROXY protocol attributes.
• These tests assert malformed metadata does not continue through fireChannelRead, does not forward outbound traffic, and closes the inbound channel.

Local verification passed:
mvn -pl proxy -Dtest=HAProxyMessageForwarderTest -DfailIfNoTests=false test

[RockteMQ-AI]

Review by github-manager-bot (Re-review after new commits)

PR #10693: [ISSUE #10692] Tolerate malformed HAProxy port values
Re-review trigger: New commits pushed on Jul 31 (after previous review on Jul 31 00:36)

Previous P1 Concern Resolution

The previous review raised a P1 security concern: buildHAProxyMessage returned null for both "no metadata" and "invalid metadata", allowing malformed PROXY connections to silently proceed without a PROXY header.

✅ Fully resolved. The new commits introduce a clean separation:

1. InvalidHAProxyMetadataException — dedicated exception for malformed PROXY protocol metadata (missing/blank/invalid addresses and ports in the proxy protocol attribute path)
2. channelRead properly distinguishes the two cases:
   • InvalidHAProxyMetadataException → ReferenceCountUtil.release(msg) + ctx.close() — connection rejected, no traffic forwarded
   • null return from buildHAProxyMessage (fallback path with invalid channel port) → still skips forwarding, which is correct for the "no PROXY metadata" case
3. No silent bypass — malformed PROXY metadata can no longer reach ctx.fireChannelRead(msg)

Code Quality

• Port validation: parsePort validates range 0-65535 and catches NumberFormatException ✓
• Proxy protocol validation: parseProxyProtocolPort, validateProxyProtocolAddress, validateProxyProtocolPort throw on invalid input ✓
• Resource safety: ReferenceCountUtil.release(msg) in exception paths prevents memory leaks ✓
• Pipeline safety: ctx.pipeline().context(this) != null check before handler removal ✓
• Tests: Comprehensive coverage including invalid ports, missing addresses, and channelRead behavior ✓

Verdict

Approve. The P1 security concern is fully addressed with a clean exception-based approach. The two semantic cases (no metadata vs. invalid metadata) are now properly distinguished.

[RockteMQ-AI]

Summary

Defensive fix with proper validation and test coverage. LGTM.

---

Automated review by github-manager-bot