feat: support SOCKS proxy support for HTTP client#2635
Merged
Conversation
ThisaruGuruge
requested review from
TharmiganK,
daneshk,
lnash94 and
shafreenAnfar
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (8)
📝 WalkthroughWalkthroughThis PR adds SOCKS4/SOCKS5 proxy support to the HTTP client, including public config changes, native proxy routing, embedded and Docker-based tests, documentation/examples, and supporting dependency updates. It also changes one resiliency test to use a local unreachable port. ChangesSOCKS Proxy Support
Dependency Bumps and Test Stability
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpUtil
participant HttpClientChannelInitializer
participant PoolableTargetChannelFactory
participant Proxy
participant Backend
Client->>HttpUtil: proxy settings
HttpUtil->>HttpUtil: resolve ProxyProtocol
HttpUtil->>HttpClientChannelInitializer: configure proxy pipeline
HttpClientChannelInitializer->>HttpClientChannelInitializer: add SOCKS4/SOCKS5/HTTP handler
PoolableTargetChannelFactory->>Proxy: connect using proxy host/port or unresolved target
Proxy->>Backend: relay request
Backend-->>Proxy: response
Proxy-->>Client: response
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ThisaruGuruge
changed the title
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (79.43%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #2635 +/- ##
============================================
+ Coverage 79.22% 79.43% +0.21%
- Complexity 0 915 +915
============================================
Files 376 441 +65
Lines 21636 24610 +2974
Branches 3382 3939 +557
============================================
+ Hits 17142 19550 +2408
- Misses 3546 3858 +312
- Partials 948 1202 +254 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ThisaruGuruge
force-pushed
the
feat/socks-proxy-support
branch
from
f709571 to
dfa395c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
ballerina-tests/resources/socks/sockd.conf (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale comment: backend is a compose service, not reached via
host.docker.internal.The header states the proxy "relays to the backend Ballerina services running on the host (reached via host.docker.internal — see compose.yml)", but
compose.yml's backend is an in-composehttp-echocontainer reached by SOCKS5 remote DNS resolving thebackendservice name (confirmed by the test comment insocks_proxy_enabled_client_test.ballines 20-26 and theSOCKS_BACKEND_URL = "http://backend:5678"target). This description is misleading for future maintainers.Proposed fix
-# Listens on the published proxy port and relays to the backend Ballerina services running on -# the host (reached via host.docker.internal — see compose.yml). +# Listens on the published proxy port and relays to the `backend` compose service via SOCKS5 +# remote DNS resolution on the proxy side (see compose.yml).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballerina-tests/resources/socks/sockd.conf` around lines 8 - 9, The header comment in sockd.conf is outdated and misdescribes how the SOCKS proxy reaches the backend. Update the comment near the top of the file to match the actual setup used by the SOCKS tests, where the backend is the in-compose service reached via the backend service name and remote DNS, not host.docker.internal. Keep the wording aligned with the behavior reflected in socks_proxy_enabled_client_test.bal and the SOCKS_BACKEND_URL target.ballerina-tests/build.gradle (1)
303-351: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared Docker helper to avoid duplicating the LDAP task pattern.
startSocksServer/stopSocksServerduplicate the exact structure ofstartLdapServer/stopLdapServer(container name check,docker ps --filter, compose up/down, sleep). A parameterized helper (container name, compose file path, wait times) would reduce this duplication now that there are two near-identical Docker lifecycle task pairs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballerina-tests/build.gradle` around lines 303 - 351, The startSocksServer and stopSocksServer tasks duplicate the same Docker lifecycle flow already used by the LDAP tasks, so factor the shared logic into a reusable helper instead of repeating the container check, compose up/down, and sleep steps. Use a parameterized method or task helper that takes the container name, compose file, and wait duration, then have startSocksServer/stopSocksServer delegate to it alongside startLdapServer/stopLdapServer to keep the build script consistent and easier to maintain.integration-tests/tests/resources/socks/sockd.conf (1)
16-24: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winOverly permissive ACL creates an open relay while the container runs.
Both
client passandsocks passallow unauthenticated relayingto: 0.0.0.0/0, not just to thebackendservice the comment describes. Combined with the published host port (9913:9913incompose.yml), this is an open, unauthenticated SOCKS relay for the container's lifetime — a concern on shared/self-hosted CI runners.🔒 Suggested scoped ACL
socks pass { - from: 0.0.0.0/0 to: 0.0.0.0/0 + from: 0.0.0.0/0 to: backend log: error connect disconnect }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/tests/resources/socks/sockd.conf` around lines 16 - 24, The ACL in sockd.conf is too broad and effectively exposes the SOCKS service as an open relay when the container is running. Tighten both client pass and socks pass rules so they only allow traffic to the backend service referenced by the test setup, using the existing ACL blocks rather than 0.0.0.0/0. Keep the change scoped to the sockd.conf rules so the SOCKS relay is not publicly reachable through the published port.integration-tests/tests/resources/socks/compose.yml (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin Docker image tags for reproducibility.
hashicorp/http-echoandvimagick/danteare unpinned (defaulting tolatest), so CI runs aren't reproducible if upstream images change.♻️ Suggested version pinning
backend: - image: hashicorp/http-echo + image: hashicorp/http-echo:0.2.3 container_name: socks-it-backend-server command: ["-text=Response from backend through SOCKS proxy", "-listen=:5678"] socks_proxy: - image: vimagick/dante + image: vimagick/dante:latest-stable-tag container_name: socks-it-proxy-server🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/tests/resources/socks/compose.yml` around lines 5 - 11, The Docker Compose services for the SOCKS integration test are using unpinned image references, which makes CI behavior non-reproducible. Update the image fields for the backend and proxy services in the compose definition to use fixed version tags (or immutable digests) instead of the implicit latest default. Keep the existing service names and configuration in compose.yml, and only adjust the image references for hashicorp/http-echo and vimagick/dante.integration-tests/build.gradle (1)
127-163: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed sleep + unowned teardown can be flaky under concurrent runs.
startSocksServerwaits a hardcoded 10s instead of polling for the SOCKS port to be ready, and if the container was already running (skip-start branch),stopSocksServerstill unconditionally runsdocker compose down— tearing down a container this build didn't start. Combined with the fixed container namesocks-it-proxy-server(no per-build/job suffix), concurrent CI runs on a shared docker host could interfere with each other's tests.♻️ Suggested readiness check instead of fixed sleep
- println "Waiting 10s until the SOCKS proxy server gets initiated." - sleep(10 * 1000) + println "Waiting until the SOCKS proxy server is ready to accept connections." + def ready = false + for (int i = 0; i < 30 && !ready; i++) { + def probe = exec { + commandLine 'sh', '-c', "nc -z localhost 9913" + ignoreExitValue = true + } + ready = probe.exitValue == 0 + if (!ready) sleep(1000) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@integration-tests/build.gradle` around lines 127 - 163, The SOCKS server lifecycle in startSocksServer and stopSocksServer is flaky under concurrent runs because it uses a fixed 10s sleep, a shared container name, and unconditional teardown even when this build did not start the container. Replace the hardcoded wait with a readiness poll that checks the SOCKS port/container until it is actually available, and make stopSocksServer only tear down containers owned by the current build or job. Also scope the Docker Compose service/container naming used by the socks-it-proxy-server setup so parallel CI runs do not collide.native/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/sender/HttpClientChannelInitializer.java (1)
184-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce cognitive complexity of
configureProxyServerto satisfy the failing SonarCloud gate.The nested per-protocol credential branches push complexity to 17 (limit 15). Extracting each protocol's handler creation into small private helpers (e.g.
addSocks5Handler,addSocks4Handler,addHttpProxyHandler) keeps the switch flat and clears the gate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/sender/HttpClientChannelInitializer.java` around lines 184 - 225, The configureProxyServer method is too complex because it mixes protocol selection with nested credential checks, pushing SonarCloud over the limit. Refactor HttpClientChannelInitializer by extracting the SOCKS5, SOCKS4, and HTTP proxy handler creation into small private helper methods such as addSocks5Handler, addSocks4Handler, and addHttpProxyHandler, then keep configureProxyServer focused on the switch and pipeline delegation only. Preserve the existing username/password and sslConfig behavior while moving the branching logic into those helpers.Source: Linters/SAST tools
native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/Socks4ProxyServerTestCase.java (1)
55-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative test for SOCKS4 userId mismatch.
Only the matching-userId and no-auth paths are tested.
EmbeddedSocksServerimplements anIDENTD_AUTH_FAILURErejection when the userId doesn't match, but that path isn't exercised here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/Socks4ProxyServerTestCase.java` around lines 55 - 88, Add a negative test in Socks4ProxyServerTestCase for the SOCKS4 userId mismatch path. Use the existing runScenario helper and EmbeddedSocksServer behavior to configure a proxy userId that does not match the client-sent value, then assert the connection/request fails with the IDENTD_AUTH_FAILURE rejection. Keep the new test alongside the existing testSocks4ProxyOverHttpWithUserId and related methods so the mismatch case is covered.native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/EmbeddedSocksServer.java (1)
161-176: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard against null
expectedPasswordbefore comparing.
expectedPassword.equals(msg.password())NPEs if a server is constructed with a non-null username but a null password.authRequired()only checksexpectedUsername != null. Not exercised today (tests always pass both or neither), but worth defending against future misuse of this test double.🛡️ Suggested guard
- boolean ok = expectedUsername.equals(msg.username()) && expectedPassword.equals(msg.password()); + boolean ok = expectedUsername.equals(msg.username()) + && java.util.Objects.equals(expectedPassword, msg.password());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/EmbeddedSocksServer.java` around lines 161 - 176, The null check in Socks5PasswordAuthHandler.channelRead0 only guards expectedUsername, so comparing expectedPassword can still throw if the server is created with a null password. Update the auth validation in EmbeddedSocksServer’s Socks5PasswordAuthHandler to safely handle a null expectedPassword before calling equals on it, using the existing expectedUsername/expectedPassword fields to keep the check consistent with authRequired().native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/Socks5ProxyServerTestCase.java (1)
56-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative test for SOCKS5 auth failure (wrong credentials).
Only the successful auth and no-auth paths are covered. The embedded server's
FAILURErejection path for mismatched username/password is never exercised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/Socks5ProxyServerTestCase.java` around lines 56 - 90, The SOCKS5 proxy tests in Socks5ProxyServerTestCase only cover successful and no-auth cases, so add a negative test that exercises the embedded server’s FAILURE path for invalid credentials. Create a new test method alongside the existing testSocks5ProxyOverHttpWithAuth/testSocks5ProxyOverHttpsWithAuth cases that calls runScenario with incorrect username/password values and asserts the connection fails as expected, so the auth rejection branch is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ballerina/http_client_config.bal`:
- Around line 145-157: The ProxyConfig documentation currently says SOCKS4
passwords are ignored, but HttpUtil.getProxyProtocol rejects any non-empty
SOCKS4 password. Update the password field doc in ProxyConfig to state that
SOCKS4 does not support password authentication and non-empty passwords are
unsupported/rejected, so callers understand the actual contract.
---
Nitpick comments:
In `@ballerina-tests/build.gradle`:
- Around line 303-351: The startSocksServer and stopSocksServer tasks duplicate
the same Docker lifecycle flow already used by the LDAP tasks, so factor the
shared logic into a reusable helper instead of repeating the container check,
compose up/down, and sleep steps. Use a parameterized method or task helper that
takes the container name, compose file, and wait duration, then have
startSocksServer/stopSocksServer delegate to it alongside
startLdapServer/stopLdapServer to keep the build script consistent and easier to
maintain.
In `@ballerina-tests/resources/socks/sockd.conf`:
- Around line 8-9: The header comment in sockd.conf is outdated and misdescribes
how the SOCKS proxy reaches the backend. Update the comment near the top of the
file to match the actual setup used by the SOCKS tests, where the backend is the
in-compose service reached via the backend service name and remote DNS, not
host.docker.internal. Keep the wording aligned with the behavior reflected in
socks_proxy_enabled_client_test.bal and the SOCKS_BACKEND_URL target.
In `@integration-tests/build.gradle`:
- Around line 127-163: The SOCKS server lifecycle in startSocksServer and
stopSocksServer is flaky under concurrent runs because it uses a fixed 10s
sleep, a shared container name, and unconditional teardown even when this build
did not start the container. Replace the hardcoded wait with a readiness poll
that checks the SOCKS port/container until it is actually available, and make
stopSocksServer only tear down containers owned by the current build or job.
Also scope the Docker Compose service/container naming used by the
socks-it-proxy-server setup so parallel CI runs do not collide.
In `@integration-tests/tests/resources/socks/compose.yml`:
- Around line 5-11: The Docker Compose services for the SOCKS integration test
are using unpinned image references, which makes CI behavior non-reproducible.
Update the image fields for the backend and proxy services in the compose
definition to use fixed version tags (or immutable digests) instead of the
implicit latest default. Keep the existing service names and configuration in
compose.yml, and only adjust the image references for hashicorp/http-echo and
vimagick/dante.
In `@integration-tests/tests/resources/socks/sockd.conf`:
- Around line 16-24: The ACL in sockd.conf is too broad and effectively exposes
the SOCKS service as an open relay when the container is running. Tighten both
client pass and socks pass rules so they only allow traffic to the backend
service referenced by the test setup, using the existing ACL blocks rather than
0.0.0.0/0. Keep the change scoped to the sockd.conf rules so the SOCKS relay is
not publicly reachable through the published port.
In
`@native/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/sender/HttpClientChannelInitializer.java`:
- Around line 184-225: The configureProxyServer method is too complex because it
mixes protocol selection with nested credential checks, pushing SonarCloud over
the limit. Refactor HttpClientChannelInitializer by extracting the SOCKS5,
SOCKS4, and HTTP proxy handler creation into small private helper methods such
as addSocks5Handler, addSocks4Handler, and addHttpProxyHandler, then keep
configureProxyServer focused on the switch and pipeline delegation only.
Preserve the existing username/password and sslConfig behavior while moving the
branching logic into those helpers.
In
`@native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/EmbeddedSocksServer.java`:
- Around line 161-176: The null check in Socks5PasswordAuthHandler.channelRead0
only guards expectedUsername, so comparing expectedPassword can still throw if
the server is created with a null password. Update the auth validation in
EmbeddedSocksServer’s Socks5PasswordAuthHandler to safely handle a null
expectedPassword before calling equals on it, using the existing
expectedUsername/expectedPassword fields to keep the check consistent with
authRequired().
In
`@native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/Socks4ProxyServerTestCase.java`:
- Around line 55-88: Add a negative test in Socks4ProxyServerTestCase for the
SOCKS4 userId mismatch path. Use the existing runScenario helper and
EmbeddedSocksServer behavior to configure a proxy userId that does not match the
client-sent value, then assert the connection/request fails with the
IDENTD_AUTH_FAILURE rejection. Keep the new test alongside the existing
testSocks4ProxyOverHttpWithUserId and related methods so the mismatch case is
covered.
In
`@native/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/Socks5ProxyServerTestCase.java`:
- Around line 56-90: The SOCKS5 proxy tests in Socks5ProxyServerTestCase only
cover successful and no-auth cases, so add a negative test that exercises the
embedded server’s FAILURE path for invalid credentials. Create a new test method
alongside the existing
testSocks5ProxyOverHttpWithAuth/testSocks5ProxyOverHttpsWithAuth cases that
calls runScenario with incorrect username/password values and asserts the
connection fails as expected, so the auth rejection branch is covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d39a694-382b-4f28-8713-2882256731ae
📒 Files selected for processing (49)
ballerina-tests/build.gradleballerina-tests/http-advanced-tests/Dependencies.tomlballerina-tests/http-client-tests/Dependencies.tomlballerina-tests/http-dispatching-tests/Dependencies.tomlballerina-tests/http-interceptor-tests/Dependencies.tomlballerina-tests/http-misc-tests/Dependencies.tomlballerina-tests/http-misc-tests/tests/socks_proxy_enabled_client_test.balballerina-tests/http-misc-tests/tests/test_service_ports.balballerina-tests/http-resiliency-tests/Dependencies.tomlballerina-tests/http-resiliency-tests/tests/resiliency_http_circuit_breaker_without_status_codes_test.balballerina-tests/http-resiliency-tests/tests/test_service_ports.balballerina-tests/http-security-tests/Dependencies.tomlballerina-tests/http-service-tests/Dependencies.tomlballerina-tests/http-test-common/Dependencies.tomlballerina-tests/http2-tests/Dependencies.tomlballerina-tests/resources/socks/compose.ymlballerina-tests/resources/socks/sockd.confballerina/Ballerina.tomlballerina/Dependencies.tomlballerina/README.mdballerina/cookie_cookieStore.balballerina/http_client_config.balchangelog.mddocs/spec/spec.mdexamples/socks-proxy/Ballerina.tomlexamples/socks-proxy/README.mdexamples/socks-proxy/main.balgradle.propertiesintegration-tests/Ballerina.tomlintegration-tests/Dependencies.tomlintegration-tests/build.gradleintegration-tests/tests/resources/socks/compose.ymlintegration-tests/tests/resources/socks/sockd.confintegration-tests/tests/socks_proxy_test.balnative/build.gradlenative/src/main/java/io/ballerina/stdlib/http/api/HttpConstants.javanative/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.javanative/src/main/java/io/ballerina/stdlib/http/transport/contract/config/ProxyServerConfiguration.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/DefaultHttpClientConnector.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/sender/HttpClientChannelInitializer.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/sender/channel/pool/PoolableTargetChannelFactory.javanative/src/main/java/module-info.javanative/src/test/java/io/ballerina/stdlib/http/transport/contract/config/ProxyServerConfigurationTest.javanative/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/EmbeddedSocksServer.javanative/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/ProxyServerUtil.javanative/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/Socks4ProxyServerTestCase.javanative/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/Socks5ProxyServerTestCase.javanative/src/test/java/io/ballerina/stdlib/http/transport/proxyserver/SocksConnectionPoolTestCase.javanative/src/test/resources/testng.xml
The Dante SOCKS proxy container is not started on Windows (host networking is unsupported there), so tag the tests that depend on it with the disabledOnWindows group and pass --disable-groups disabledOnWindows to the integration-tests run on Windows.
TharmiganK
reviewed
Jul 6, 2026
TharmiganK
reviewed
Jul 6, 2026
TharmiganK
reviewed
Jul 6, 2026
TharmiganK
reviewed
Jul 6, 2026
- Refactor configureProxyServer into private helpers to reduce cognitive complexity flagged by SonarCloud - Change SOCKS4 password handling from error to warn-and-ignore, since SOCKS4 simply does not support passwords rather than it being an invalid configuration - Update spec.md and example README to reflect warn-and-ignore behaviour - Fix inaccurate sockd.conf comment (backend is an in-compose service, not a host service reached via host.docker.internal) - Fix inaccurate build.gradle comment on the Windows guard (Docker unavailability, not host networking limitation) - Remove hard line-wrapping from example README
There was a problem hiding this comment.
🧹 Nitpick comments (1)
native/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.java (1)
1294-1345: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPassword is still stored for SOCKS4 despite the "warn-and-ignore" intent.
getProxyProtocollogs a warning that the SOCKS4 password will be ignored, butpopulateSenderConfigurationsunconditionally callssetProxyPassword(proxyPassword)afterward whenever the field is non-empty (irrespective of the resolved protocol). The password is therefore still stored onProxyServerConfiguration; "ignoring" it relies entirely on downstream native transport code (not in this file) never reading it for SOCKS4. Consider explicitly skippingsetProxyPasswordwhen the protocol is SOCKS4, so the intent is enforced at this layer rather than depending on an assumption about consumer code.♻️ Proposed fix
if (!proxyUserName.isEmpty()) { proxyServerConfiguration.setProxyUsername(proxyUserName); } - if (!proxyPassword.isEmpty()) { + if (!proxyPassword.isEmpty() + && proxyProtocolEnum != ProxyServerConfiguration.ProxyProtocol.SOCKS4) { proxyServerConfiguration.setProxyPassword(proxyPassword); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@native/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.java` around lines 1294 - 1345, The SOCKS4 “warn-and-ignore” handling is incomplete in populateSenderConfigurations/getProxyProtocol because setProxyPassword is still called for any non-empty password, so the password remains stored on ProxyServerConfiguration. Update populateSenderConfigurations to only call setProxyPassword when the resolved protocol from getProxyProtocol is not SOCKS4, and keep the existing warning in getProxyProtocol for SOCKS4 passwords.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@native/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.java`:
- Around line 1294-1345: The SOCKS4 “warn-and-ignore” handling is incomplete in
populateSenderConfigurations/getProxyProtocol because setProxyPassword is still
called for any non-empty password, so the password remains stored on
ProxyServerConfiguration. Update populateSenderConfigurations to only call
setProxyPassword when the resolved protocol from getProxyProtocol is not SOCKS4,
and keep the existing warning in getProxyProtocol for SOCKS4 passwords.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 809a7332-65ad-4ae7-a9e9-29caf5ce87c9
📒 Files selected for processing (7)
ballerina-tests/build.gradleballerina-tests/http-dispatching-tests/tests/service_dispatching_default_data_binding.balballerina-tests/resources/socks/sockd.confdocs/spec/spec.mdexamples/socks-proxy/README.mdnative/src/main/java/io/ballerina/stdlib/http/api/HttpUtil.javanative/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/sender/HttpClientChannelInitializer.java
✅ Files skipped from review due to trivial changes (3)
- ballerina-tests/resources/socks/sockd.conf
- examples/socks-proxy/README.md
- docs/spec/spec.md
🚧 Files skipped from review as they are similar to previous changes (2)
- ballerina-tests/build.gradle
- native/src/main/java/io/ballerina/stdlib/http/transport/contractimpl/sender/HttpClientChannelInitializer.java
The test previously asserted that client creation fails when a password is set on a SOCKS4 proxy config. Since the behaviour was changed to log a warning and ignore the password instead of returning an error, update the test to verify that client creation succeeds.
|
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.
Purpose
$Subject
Fixes: ballerina-platform/ballerina-library#8810
Examples
Checklist
Added native SOCKS proxy support to the Ballerina HTTP client. This introduces new proxy configuration APIs (ProxyProtocol and an extended ProxyConfig with protocol selection) and updates the HTTP transport implementation to correctly route requests for HTTP, SOCKS4, and SOCKS5 proxies, including handling of proxy credentials (SOCKS4 password settings are ignored with a warning). The change also updates the HTTP client spec and documentation (including a new SOCKS proxy example) and adds/extends automated coverage using embedded and Docker-based SOCKS proxy servers, covering both end-to-end request behavior and SOCKS-based connection pooling. Supporting test/build infrastructure was updated to manage SOCKS proxy lifecycles and align dependency/version pins, along with an “Unreleased” changelog entry for the feature.