Proxy: Fix URI for CONNECT Requests#2886
Merged
Merged
Conversation
- Removed unused constants and imports (`TRACK_NODE_STATUS`) from several classes. - Replaced explicit type declarations with `var` in tests and implementations for modernized code. - Improved test case clarity (`ProxySSLTest` and `HttpClientTest`) by refining method logic and assertions. - Minor code cleanup and enhanced null safety in `HttpClient` and connection handling.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors HTTP client host/URI/header handling, adds CONNECT tunneling state and instance stream setup to Connection, removes an unused Exchange constant/logger and cleans imports, updates HostColonPort parsing/annotations, and adapts tests to the new APIs and signatures. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/test/java/com/predic8/membrane/core/proxies/ProxySSLTest.java (1)
90-90:⚠️ Potential issue | 🟡 MinorTypo in comment: "workarond" should be "workaround".
📝 Proposed fix
- ssl.setEndpointIdentificationAlgorithm(""); // workarond the fact that the certificate was not issued for 'localhost' + ssl.setEndpointIdentificationAlgorithm(""); // workaround the fact that the certificate was not issued for 'localhost'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/proxies/ProxySSLTest.java` at line 90, Fix the typo in the inline comment near the call to ssl.setEndpointIdentificationAlgorithm("") in ProxySSLTest (change "workarond" to "workaround") so the comment reads something like "// workaround the fact that the certificate was not issued for 'localhost'"; locate the comment attached to the ssl.setEndpointIdentificationAlgorithm("") statement and update the spelling only.core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java (1)
417-424:⚠️ Potential issue | 🟠 MajorBug:
tunneledflag set before validating the tunnel response.The
tunneled = trueassignment at line 417 occurs before the success check at lines 420-424. If the proxy returns an error response (e.g., 407 or 502), the connection is marked as tunneled even though the tunnel was never established, and then anIOExceptionis thrown.While the exception likely causes the connection to be discarded, setting
tunneled = trueprematurely is incorrect and could cause issues if any error-recovery code path reuses this connection.🐛 Proposed fix: Move the flag assignment after validation
- tunneled = true; - /* Look for '200 OK' response. Probably, some proxies may return HTTP/1.1 back */ if (!replyStr.startsWith("HTTP/1.0 200") && !replyStr.startsWith("HTTP/1.1 200")) { throw new IOException("Unable to tunnel through " + proxy.getHost() + ":" + proxy.getPort() + ". Proxy returns \"" + replyStr + "\""); } + + tunneled = true; /* tunneling Handshake was successful! */ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java` around lines 417 - 424, The code sets the Connection.tunneled flag before validating the proxy response; move the assignment so tunneled is set only after the success check on replyStr (the block that checks replyStr.startsWith("HTTP/1.0 200") or "HTTP/1.1 200") and after the proxy host/port validation completes, ensuring that if the method throws the IOException (e.g., proxy returns 407/502) the tunneled flag remains false; update the code around the replyStr check in class Connection to assign tunneled = true only in the successful-200 branch.
🧹 Nitpick comments (5)
core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java (1)
459-461: Nitpick: ConsiderisTunneled()for the boolean getter.The conventional JavaBean naming for boolean getters is
is*rather thanget*. UsingisTunneled()would be more idiomatic.🔧 Optional rename
- public boolean getTunneled() { + public boolean isTunneled() { return tunneled; }Note: This would require updating the call site in
HttpClient.setRequestURI()as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java` around lines 459 - 461, Rename the boolean getter Connection.getTunneled() to the idiomatic isTunneled() and update all call sites accordingly (notably HttpClient.setRequestURI() which currently references getTunneled()); change the method signature in class Connection and replace any references to getTunneled() with isTunneled(), keeping behavior and visibility identical to avoid breaking semantics.core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java (2)
83-89: Address the TODO and consolidate duplicate error messages.The
//@TODO`` comment indicates incomplete work. Additionally, the error message here is duplicated verbatim at lines 154-157 insetRequestURI(). Consider extracting it to a constant or throwing from a single location.Would you like me to help consolidate the error handling and remove the TODO?
🔧 Suggested consolidation
+ private static final String INVALID_DESTINATION_MESSAGE = + "The exchange's destination URI %s does not start with 'http'. Specify a 'target' within " + + "the API configuration or make sure the exchanges destinations list contains a valid URI."; + private boolean dispatchCall(Exchange exc, String target, int attempt) throws Exception { setAuthorizationHeader(exc); HostColonPort hcp; OutgoingConnectionType outConType; try { hcp = getHostColonPort(exc, target); outConType = connectionFactory.getConnection(exc, hcp, attempt); setRequestURI(exc.getRequest(), target, outConType.con()); } catch (MalformedURLException e) { - // `@TODO` - throw new MalformedURLException(""" - The exchange's destination URI %s does not start with 'http'. Specify a 'target' within - the API configuration or make sure the exchanges destinations list contains a valid URI. - """.formatted(target)); + throw new MalformedURLException(INVALID_DESTINATION_MESSAGE.formatted(target)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java` around lines 83 - 89, The catch block in HttpClient (catching MalformedURLException) contains a TODO and duplicates the same error text used in setRequestURI(); extract that message into a single constant (e.g., BAD_DESTINATION_URI_MESSAGE) in the HttpClient class or a shared errors holder, replace the duplicated literal in both the MalformedURLException thrown here and the exception thrown in setRequestURI() to reference that constant, and remove the TODO comment; ensure the formatted target value is still applied when constructing the thrown MalformedURLException.
118-124: Outstanding TODO at line 121.There's a
// TODOcomment that should be addressed or documented with context about what remains to be done.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java` around lines 118 - 124, The TODO in getHostColonPort (method getHostColonPort, using HostColonPort target from getTargetHostAndPort and calling exc.getRequest().getHeader().setHost) must be resolved: either implement the missing behavior or replace the TODO with a clear explanatory comment; specifically, if any special handling is required when adjusting the Host header for CONNECT requests or proxied requests (check isCONNECTRequest() and exc.getProxy().isTargetAdjustHostHeader()), implement that logic (e.g., skip setting Host for CONNECT or include port only when non-default), otherwise remove the TODO and add a one-line comment stating why simply calling setHost(target.toString()) is correct. Ensure the change is applied inside getHostColonPort and references getTargetHostAndPort, configuration.isAdjustHostHeader, and exc.getRequest().getHeader().setHost.core/src/test/java/com/predic8/membrane/core/transport/http/HttpClientTest.java (1)
42-51: Tests look good, but consider adding coverage for tunneling scenarios.The mocked
ConnectionreturnsfalseforgetTunneled()by default (Mockito behavior), which tests the direct/non-proxy path. Consider adding tests for:
- CONNECT requests (should use
host:portformat)- Proxy with non-tunneled connection (should use full URL)
- Proxy with tunneled connection (should use path-only)
🧪 Example additional test cases
`@Test` void setRequestURIForCONNECT() throws Exception { var request = Request.method("CONNECT").path("/").build(); client.setRequestURI(request, "https://example.com:443", Mockito.mock(Connection.class)); assertEquals("example.com:443", request.getUri()); } `@Test` void setRequestURIProxyTunneled() throws Exception { var request = get("/bar").build(); var connection = Mockito.mock(Connection.class); Mockito.when(connection.getTunneled()).thenReturn(true); // Need HttpClient with proxy configured // client.setRequestURI(request, "https://predic8.de/foo", connection); // assertEquals("/foo", request.getUri()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/test/java/com/predic8/membrane/core/transport/http/HttpClientTest.java` around lines 42 - 51, Add unit tests covering tunneling and proxy scenarios for the HttpClient#setRequestURI behavior: create a CONNECT request via Request.method("CONNECT") and assert that setRequestURI produces "host:port"; create a non-tunneled proxied request by mocking Connection and leaving getTunneled() false (or explicitly Mockito.when(connection.getTunneled()).thenReturn(false)) and configure the HttpClient instance with a proxy to assert full URL is used; create a tunneled proxied request by mocking Connection.getTunneled() to return true, configure the HttpClient to use a proxy, call client.setRequestURI(request, "https://predic8.de/foo", connection) and assert the path-only URI ("/foo") is set; reuse the existing test patterns (get("/bar").build(), client, Mockito.mock(Connection.class)) and add assertions similar to the existing tests.core/src/main/java/com/predic8/membrane/core/exchange/AbstractExchange.java (1)
21-24: Minor: Redundant explicit import alongside wildcard.Line 24 imports
Proxyexplicitly, but line 23 already uses a wildcard importcom.predic8.membrane.core.proxies.*which includesProxy. The explicit import is redundant.🔧 Optional fix
import com.predic8.membrane.core.interceptor.balancer.*; import com.predic8.membrane.core.model.*; import com.predic8.membrane.core.proxies.*; -import com.predic8.membrane.core.proxies.Proxy; import org.slf4j.*;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/exchange/AbstractExchange.java` around lines 21 - 24, Remove the redundant explicit import of Proxy in AbstractExchange.java: the line importing com.predic8.membrane.core.proxies.Proxy is unnecessary because com.predic8.membrane.core.proxies.* already covers it; delete the explicit Proxy import (or replace the wildcard with explicit imports if the project prefers non-wildcard imports) so only one form of import remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/test/java/com/predic8/membrane/core/proxies/ProxySSLTest.java`:
- Line 110: Fix the typo in the inline comment for the ssl configuration: change
the comment on the line calling ssl.setEndpointIdentificationAlgorithm("") from
"workarond the fact that the certificate was not issued for 'localhost'" to
"workaround the fact that the certificate was not issued for 'localhost'";
locate the occurrence near the ssl.setEndpointIdentificationAlgorithm("") call
in ProxySSLTest and update the comment text accordingly.
---
Outside diff comments:
In `@core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java`:
- Around line 417-424: The code sets the Connection.tunneled flag before
validating the proxy response; move the assignment so tunneled is set only after
the success check on replyStr (the block that checks
replyStr.startsWith("HTTP/1.0 200") or "HTTP/1.1 200") and after the proxy
host/port validation completes, ensuring that if the method throws the
IOException (e.g., proxy returns 407/502) the tunneled flag remains false;
update the code around the replyStr check in class Connection to assign tunneled
= true only in the successful-200 branch.
In `@core/src/test/java/com/predic8/membrane/core/proxies/ProxySSLTest.java`:
- Line 90: Fix the typo in the inline comment near the call to
ssl.setEndpointIdentificationAlgorithm("") in ProxySSLTest (change "workarond"
to "workaround") so the comment reads something like "// workaround the fact
that the certificate was not issued for 'localhost'"; locate the comment
attached to the ssl.setEndpointIdentificationAlgorithm("") statement and update
the spelling only.
---
Nitpick comments:
In `@core/src/main/java/com/predic8/membrane/core/exchange/AbstractExchange.java`:
- Around line 21-24: Remove the redundant explicit import of Proxy in
AbstractExchange.java: the line importing
com.predic8.membrane.core.proxies.Proxy is unnecessary because
com.predic8.membrane.core.proxies.* already covers it; delete the explicit Proxy
import (or replace the wildcard with explicit imports if the project prefers
non-wildcard imports) so only one form of import remains.
In `@core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java`:
- Around line 459-461: Rename the boolean getter Connection.getTunneled() to the
idiomatic isTunneled() and update all call sites accordingly (notably
HttpClient.setRequestURI() which currently references getTunneled()); change the
method signature in class Connection and replace any references to getTunneled()
with isTunneled(), keeping behavior and visibility identical to avoid breaking
semantics.
In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java`:
- Around line 83-89: The catch block in HttpClient (catching
MalformedURLException) contains a TODO and duplicates the same error text used
in setRequestURI(); extract that message into a single constant (e.g.,
BAD_DESTINATION_URI_MESSAGE) in the HttpClient class or a shared errors holder,
replace the duplicated literal in both the MalformedURLException thrown here and
the exception thrown in setRequestURI() to reference that constant, and remove
the TODO comment; ensure the formatted target value is still applied when
constructing the thrown MalformedURLException.
- Around line 118-124: The TODO in getHostColonPort (method getHostColonPort,
using HostColonPort target from getTargetHostAndPort and calling
exc.getRequest().getHeader().setHost) must be resolved: either implement the
missing behavior or replace the TODO with a clear explanatory comment;
specifically, if any special handling is required when adjusting the Host header
for CONNECT requests or proxied requests (check isCONNECTRequest() and
exc.getProxy().isTargetAdjustHostHeader()), implement that logic (e.g., skip
setting Host for CONNECT or include port only when non-default), otherwise
remove the TODO and add a one-line comment stating why simply calling
setHost(target.toString()) is correct. Ensure the change is applied inside
getHostColonPort and references getTargetHostAndPort,
configuration.isAdjustHostHeader, and exc.getRequest().getHeader().setHost.
In
`@core/src/test/java/com/predic8/membrane/core/transport/http/HttpClientTest.java`:
- Around line 42-51: Add unit tests covering tunneling and proxy scenarios for
the HttpClient#setRequestURI behavior: create a CONNECT request via
Request.method("CONNECT") and assert that setRequestURI produces "host:port";
create a non-tunneled proxied request by mocking Connection and leaving
getTunneled() false (or explicitly
Mockito.when(connection.getTunneled()).thenReturn(false)) and configure the
HttpClient instance with a proxy to assert full URL is used; create a tunneled
proxied request by mocking Connection.getTunneled() to return true, configure
the HttpClient to use a proxy, call client.setRequestURI(request,
"https://predic8.de/foo", connection) and assert the path-only URI ("/foo") is
set; reuse the existing test patterns (get("/bar").build(), client,
Mockito.mock(Connection.class)) and add assertions similar to the existing
tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6558b851-acd6-4384-bcd3-8a20b0ac7206
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/exchange/AbstractExchange.javacore/src/main/java/com/predic8/membrane/core/exchange/Exchange.javacore/src/main/java/com/predic8/membrane/core/interceptor/balancer/ExchangeNodeStatusTracker.javacore/src/main/java/com/predic8/membrane/core/transport/http/Connection.javacore/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.javacore/src/test/java/com/predic8/membrane/core/proxies/ProxySSLTest.javacore/src/test/java/com/predic8/membrane/core/proxies/ServiceProxyTest.javacore/src/test/java/com/predic8/membrane/core/transport/http/HttpClientTest.java
💤 Files with no reviewable changes (2)
- core/src/main/java/com/predic8/membrane/core/interceptor/balancer/ExchangeNodeStatusTracker.java
- core/src/main/java/com/predic8/membrane/core/exchange/Exchange.java
rrayst
previously approved these changes
Mar 19, 2026
|
This pull request needs "/ok-to-test" from an authorized committer. |
- Extracted and encapsulated logic into `adjustHostHeader` method for cleaner host header adjustments. - Enhanced null safety and annotations using `@NotNull` for better code guarantees. - Replaced explicit type declarations with `var` for modernized and consistent code. - Simplified `tunneled` proxy handling in `Connection`. - Improved error message formatting and streamlined exception handling in `getHostColonPort`. - Minor comments and formatting adjustments for readability across test and core implementations.
- Added missing Apache 2.0 license headers to `AbstractFlowInterceptorTest`. - Replaced direct call to `client.getHostColonPort` with static `HttpClient.getHostColonPort` in `HttpClientTest`.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java`:
- Around line 116-118: The CONNECT handling currently hands the raw dest into
HostColonPort (via getHostColonPort) which splits on ':' and fails for URI-form
targets and bracketed IPv6 authorities; update getHostColonPort and the logic
that sets HostColonPort (and any use of HostColonPort.parse) to first parse the
dest with a scheme-aware, authority-aware parser (e.g., treat full URIs by
extracting authority and handle bracketed IPv6 by stripping surrounding '[' ']'
before splitting the port) so both the HostColonPort value used by
dispatchCall()/Host header generation and the request-URI normalized later by
setRequestURI() are derived from the same parsed authority; replace direct new
HostColonPort(false, dest) and HostColonPort.parse(dest) usages with this
normalized authority extraction, ensure bracketed IPv6 like "[2001:db8::1]:443"
yields host "2001:db8::1" and port "443", and add regression tests that perform
CONNECT with a URI target (https://example.com) and with a bracketed IPv6
authority to prevent regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bca6442a-9d83-4871-8b32-e78a2e2e406a
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/transport/http/Connection.javacore/src/main/java/com/predic8/membrane/core/transport/http/HostColonPort.javacore/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.javacore/src/test/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/proxies/ProxySSLTest.javacore/src/test/java/com/predic8/membrane/core/transport/http/HttpClientTest.java
✅ Files skipped from review due to trivial changes (1)
- core/src/test/java/com/predic8/membrane/core/interceptor/flow/AbstractFlowInterceptorTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java
- core/src/test/java/com/predic8/membrane/core/proxies/ProxySSLTest.java
rrayst
requested changes
Mar 19, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java (1)
121-123:⚠️ Potential issue | 🔴 CriticalCONNECT targets are still normalized through two incompatible parsers.
dispatchCall()still feeds CONNECT destinations intonew HostColonPort(false, dest), but the request line is normalized later via the separategetHostColonPort(String)helper. That means URI-form CONNECT targets likehttps://example.comand bracketed IPv6 authorities can still produce a bad connection target/Host header beforesetRequestURI()runs. Please derive both theHostColonPortand the CONNECT request-target from one scheme/authority-aware parser, and add regressions for a URI target plus[2001:db8::1]:443.Also applies to: 171-174
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java` around lines 121 - 123, dispatchCall() currently constructs a HostColonPort for CONNECT targets using new HostColonPort(false, dest) while the later normalization uses getHostColonPort(String), causing inconsistent parsing for URI-style CONNECT targets and bracketed IPv6 authorities; refactor so both the HostColonPort and the CONNECT request-target are derived from a single scheme/authority-aware parser (e.g., unify logic inside getHostColonPort(Exchange exc, String dest) to call the same parsing path as getHostColonPort(String) and ensure setRequestURI() uses that canonical result), update dispatchCall() to use the unified parser instead of new HostColonPort(false, dest), and add regression tests covering a URI CONNECT target like "https://example.com" and a bracketed IPv6 authority "[2001:db8::1]:443".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java`:
- Around line 125-131: The catch block for HostColonPort.parse(dest) should not
replace every parse error with a "does not start with 'http'" message; instead
detect whether the failure is truly a missing/invalid scheme and only then throw
the custom message, otherwise preserve the original MalformedURLException (or
rethrow it as the cause). Concretely: in the catch for MalformedURLException e
from HostColonPort.parse(dest) check dest's scheme (e.g., inspect
dest.startsWith("http") or parse new URI(dest).getScheme()); if the scheme is
absent/invalid throw the custom MalformedURLException with your explanatory
text, otherwise rethrow or wrap e as the cause so the original parse error is
not lost (reference HostColonPort.parse, the caught MalformedURLException e, and
variable dest).
---
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java`:
- Around line 121-123: dispatchCall() currently constructs a HostColonPort for
CONNECT targets using new HostColonPort(false, dest) while the later
normalization uses getHostColonPort(String), causing inconsistent parsing for
URI-style CONNECT targets and bracketed IPv6 authorities; refactor so both the
HostColonPort and the CONNECT request-target are derived from a single
scheme/authority-aware parser (e.g., unify logic inside
getHostColonPort(Exchange exc, String dest) to call the same parsing path as
getHostColonPort(String) and ensure setRequestURI() uses that canonical result),
update dispatchCall() to use the unified parser instead of new
HostColonPort(false, dest), and add regression tests covering a URI CONNECT
target like "https://example.com" and a bracketed IPv6 authority
"[2001:db8::1]:443".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5750049a-620d-4b6b-9928-83f7b4a07bd6
📒 Files selected for processing (5)
core/src/main/java/com/predic8/membrane/core/transport/http/Connection.javacore/src/main/java/com/predic8/membrane/core/transport/http/HostColonPort.javacore/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.javacore/src/main/java/com/predic8/membrane/core/transport/http/client/RetryHandler.javacore/src/test/java/com/predic8/membrane/core/transport/http/HostColonPortTest.java
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/transport/http/client/RetryHandler.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/transport/http/HostColonPort.java
- core/src/main/java/com/predic8/membrane/core/transport/http/Connection.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java (2)
125-131:⚠️ Potential issue | 🟡 MinorPreserve the original parse failure as the cause.
This catch block replaces
HostColonPort.parse(dest)'sMalformedURLExceptionwith a new one and drops the underlying parse detail. Keep the friendlier top-level message, but chaineso callers can still see why the destination was rejected. (raw.githubusercontent.com)💡 Minimal fix
try { return HostColonPort.parse(dest); } catch (MalformedURLException e) { - throw new MalformedURLException(""" + var ex = new MalformedURLException(""" The exchange's destination URI %s is not valid. Specify a 'target' within the API configuration or make sure the exchanges destinations list contains a valid URI. """.formatted(dest)); + ex.initCause(e); + throw ex; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java` around lines 125 - 131, The catch currently replaces the original MalformedURLException thrown by HostColonPort.parse(dest) and loses the underlying cause; preserve the friendly message but chain the original exception by creating the new MalformedURLException with the formatted message, call initCause(e) on it, and then throw that chained exception (referring to HostColonPort.parse, the caught variable e, and the surrounding HttpClient class code).
121-123:⚠️ Potential issue | 🔴 CriticalCONNECT target parsing is still broken for URI-form and bracketed IPv6 destinations.
new HostColonPort(false, dest)still sends CONNECT targets through the constructor that splits on the first:and parses the rest as the port, so values likehttps://example.comand[2001:db8::1]:443still fail. The URI-normalization helper on Lines 171-174 also still routes URI-form CONNECT targets throughHostColonPort.parse(...), whose host regex only accepts[^:/#]+, so bracketed IPv6 URIs are still truncated there too. Please derive both the socket target and CONNECT request URI from one scheme/authority-aware parser, and add regression coverage for URI-form plus bracketed IPv6 CONNECT targets. (raw.githubusercontent.com)Also applies to: 153-154, 171-174
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java`:
- Around line 125-131: The catch currently replaces the original
MalformedURLException thrown by HostColonPort.parse(dest) and loses the
underlying cause; preserve the friendly message but chain the original exception
by creating the new MalformedURLException with the formatted message, call
initCause(e) on it, and then throw that chained exception (referring to
HostColonPort.parse, the caught variable e, and the surrounding HttpClient class
code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3b47bbea-8307-4e63-b790-8e4ce466eb5d
📒 Files selected for processing (1)
core/src/main/java/com/predic8/membrane/core/transport/http/HttpClient.java
rrayst
approved these changes
Mar 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests