Conversation
|
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:
📝 WalkthroughWalkthroughReplaces SOAP login with OAuth2 password-grant in LoginHelper; adds clientId/clientSecret accessors to ConnectorConfig; bumps project and dependency versions and moves Java source/target to 17 in pom.xml; inserts license header in WsdlXPathTest.java. Changes
Sequence Diagram(s)sequenceDiagram
participant LoginHelper as LoginHelper
participant OAuth as OAuth2 Server
participant Bulk as Salesforce Bulk API
LoginHelper->>OAuth: POST /services/oauth2/token\nform: client_id, client_secret, username, password, grant_type=password
OAuth-->>LoginHelper: 200 OK\nJSON { access_token, instance_url, ... }
LoginHelper->>LoginHelper: parse access_token & instance_url\nderive api version & bulk endpoint
LoginHelper->>Bulk: Bulk API request (Authorization: Bearer access_token)
Bulk-->>LoginHelper: Bulk API response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pom.xml (1)
74-78:⚠️ Potential issue | 🟠 Major
commons-beanutils 1.9.4carries CVE-2025-48734 (CVSS 8.8) — upgrade to 1.11.0.CVE-2025-48734 is a high-severity vulnerability in
commons-beanutils-1.9.4.jar. If property paths from an external source are passed directly toPropertyUtilsBean.getProperty(), an attacker can access the enum's class loader via thedeclaredClassproperty, potentially allowing remote code execution. This issue affectscommons-beanutils1.x before 1.11.0.Since
maven-shade-pluginbundles (and relocates) this dependency into the published uber jar, downstream consumers inherit the vulnerable class files directly.🛡️ Proposed fix
<dependency> <groupId>commons-beanutils</groupId> <artifactId>commons-beanutils</artifactId> - <version>1.9.4</version> + <version>1.11.0</version> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 74 - 78, The project depends on a vulnerable commons-beanutils version; update the dependency with groupId "commons-beanutils" and artifactId "commons-beanutils" from version 1.9.4 to 1.11.0 to remediate CVE-2025-48734, then rebuild and verify the shaded uber-jar no longer contains the old classes (inspect the maven-shade-plugin output or run mvn dependency:tree / mvn dependency:analyze to confirm the resolved version).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Line 12: The Salesforce API version bump to 66.0 breaks SOAP Partner API login
— update LoginHelper.doLogin to stop calling SOAP login() directly and implement
a supported authentication flow (e.g., OAuth2 username-password flow, JWT bearer
flow, or web server OAuth) using the Partner/REST endpoints instead of SOAP;
locate and replace the direct SOAP login call in LoginHelper.doLogin and ensure
token handling and refresh logic are implemented. Also review pom.xml's
<version> change for Salesforce API compatibility and evaluate the shaded
commons-beanutils:1.9.4 referenced by the maven-shade-plugin (consider upgrading
to a non-vulnerable version) so the published artifact does not include a
library with known high-severity CVEs.
---
Outside diff comments:
In `@pom.xml`:
- Around line 74-78: The project depends on a vulnerable commons-beanutils
version; update the dependency with groupId "commons-beanutils" and artifactId
"commons-beanutils" from version 1.9.4 to 1.11.0 to remediate CVE-2025-48734,
then rebuild and verify the shaded uber-jar no longer contains the old classes
(inspect the maven-shade-plugin output or run mvn dependency:tree / mvn
dependency:analyze to confirm the resolved version).
rybalkin91prodly
force-pushed
the
PM-10326
branch
from
7b3da56 to
6149753
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/sforce/bulk/LoginHelper.java`:
- Around line 125-127: Remove the sensitive logging of the raw OAuth2 access
token in LoginHelper (the handler.info call that concatenates accessToken);
continue to call handler.getConfig().setSessionId(accessToken) to store the
token, but replace the handler.info(...) with a non-sensitive message such as
"Access token obtained" or a masked preview (e.g., show only first/last 4 chars)
derived from accessToken before logging so the full bearer token is never
written to logs.
- Around line 106-108: In LoginHelper, replace the manual open/close of the
connection output stream with a try-with-resources block: declare the
OutputStream from conn.getOutputStream() in a try(...) and call
os.write(body.getBytes(StandardCharsets.UTF_8)) inside the block so the stream
is always closed even if write() throws; update the code that currently uses
OutputStream os = conn.getOutputStream(); os.write(...); os.close(); to the
try-with-resources form within the LoginHelper method where the request body is
written.
- Around line 110-112: LoginHelper currently calls conn.getErrorStream() and
passes it directly to FileUtil.toBytes, which can NPE if getErrorStream()
returns null; change the flow in the response-handling block to guard against a
null error stream (use an empty byte[] or a clear error-message byte[] when
getErrorStream() is null) so FileUtil.toBytes never receives null and the code
will throw/handle the intended StreamException instead of an NPE, and convert
the resulting bytes to a String using UTF-8 (StandardCharsets.UTF_8) rather than
the platform default; update references around conn.getResponseCode(),
conn.getInputStream()/getErrorStream(), FileUtil.toBytes(...) and the code that
constructs the response String to implement these checks and charset usage.
- Around line 87-90: The code uses URLEncoder.encode(String, Charset) which
requires Java 10+ while the build declares Java 8, and it also risks NPEs for
handler.getConfig().getUsername()/getPassword(); fix by either updating the
pom.xml Java source/target to at least 10 (or 11/17) to match
URLEncoder.encode(s, Charset) usage, or change the calls to the Java‑8
compatible URLEncoder.encode(s, "UTF-8") signature; additionally make
username/password null-safe like the existing clientSecret check (e.g., replace
direct handler.getConfig().getUsername()/getPassword() usages with a
null-coalescing pattern) so URLEncoder.encode never receives null.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xmlsrc/main/java/com/sforce/bulk/LoginHelper.javasrc/main/java/com/sforce/ws/ConnectorConfig.javasrc/main/resources/metadata_with_rlm.wsdlsrc/test/java/com/sforce/ws/bind/WsdlXPathTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/com/sforce/ws/bind/WsdlXPathTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
rybalkin91prodly
force-pushed
the
PM-10326
branch
from
713360e to
58a85b9
Compare
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 (1)
pom.xml (1)
195-204:⚠️ Potential issue | 🟡 MinorRemove unsupported
<target>element frommaven-javadoc-pluginconfiguration.The
maven-javadoc-pluginv3.2.0 does not support<target>as a configuration parameter. The plugin only recognizes<source>(or<release>for JDK 9+). The unknown<target>element will be silently ignored. Keep<source>17</source>and remove<target>17</target>.Fix
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-javadoc-plugin</artifactId> <version>3.2.0</version> <configuration> <source>17</source> - <target>17</target> <doclint>none</doclint> <additionalJOption>-Xdoclint:none</additionalJOption> </configuration>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 195 - 204, In the maven-javadoc-plugin configuration (artifactId maven-javadoc-plugin, version 3.2.0) remove the unsupported <target>17</target> element because this plugin only recognizes <source> (or <release> for JDK9+); keep the existing <source>17</source> (or replace with <release>17</release> if you prefer) and ensure no other <target> elements remain in the plugin's <configuration>.
♻️ Duplicate comments (3)
src/main/java/com/sforce/bulk/LoginHelper.java (2)
99-116:⚠️ Potential issue | 🔴 CriticalHarden HTTP I/O: try-with-resources, null
getErrorStream(), UTF-8 decoding, and disconnect.Currently:
os.close()can be skipped ifwrite()throws.getErrorStream()can be null →FileUtil.toBytes(null)NPE (secondary failure on error paths).new String(bytes)uses platform default charset (OAuth JSON is UTF-8).Proposed fix
- HttpURLConnection conn; - conn = handler.getConfig().createConnection(url, null, false); - - conn.setRequestMethod(HTTP_METHOD_POST); - conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED); - conn.setDoOutput(true); - - OutputStream os = conn.getOutputStream(); - os.write(body.getBytes(StandardCharsets.UTF_8)); - os.close(); - - int status = conn.getResponseCode(); - InputStream responseStream = status == 200 ? conn.getInputStream() : conn.getErrorStream(); - String response = new String(FileUtil.toBytes(responseStream)); - - if (status != 200) { - throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response); - } + HttpURLConnection conn = handler.getConfig().createConnection(url, null, false); + try { + conn.setRequestMethod(HTTP_METHOD_POST); + conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED); + conn.setDoOutput(true); + + byte[] bodyBytes = body.getBytes(StandardCharsets.UTF_8); + try (OutputStream os = conn.getOutputStream()) { + os.write(bodyBytes); + } + + int status = conn.getResponseCode(); + InputStream responseStream = status == 200 ? conn.getInputStream() : conn.getErrorStream(); + String response = responseStream != null + ? new String(FileUtil.toBytes(responseStream), StandardCharsets.UTF_8) + : ""; + + if (status != 200) { + throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response); + } + } finally { + conn.disconnect(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 99 - 116, Wrap the HTTP output and input streams in try-with-resources inside LoginHelper so the OutputStream from handler.getConfig().createConnection(...) is always closed even if write() fails, and read the response InputStream (use conn.getInputStream() or fallback to conn.getErrorStream()) with null-checking for getErrorStream() before calling FileUtil.toBytes; decode the resulting bytes using StandardCharsets.UTF_8 when constructing the response string instead of platform default, and ensure conn.disconnect() is invoked in a finally block (or try-with-resources equivalent) to always release the connection; update the error path that throws StreamException to include the UTF-8 decoded response.
125-127:⚠️ Potential issue | 🟠 MajorDo not log the OAuth access token (bearer credential).
Line 126 prints a live token; anyone with log access can replay it.
Proposed fix
handler.getConfig().setSessionId(accessToken); - handler.info("Session Id: " + accessToken); + handler.info("Session Id obtained (token omitted for security)");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 125 - 127, Remove the logging of the OAuth access token in LoginHelper: do not call handler.info("Session Id: " + accessToken) or otherwise emit the raw bearer token; instead either remove the log line entirely or replace it with a non-sensitive message that indicates a session was established (e.g., "Session established" or log only a masked/token hash). Keep handler.getConfig().setSessionId(accessToken) as-is but ensure any use of accessToken in logs is redacted or hashed.pom.xml (1)
149-156: Java baseline jump to 17: confirm this breaking change is intentional.This resolves the earlier Java-API mismatch (e.g.,
URLEncoder.encode(String, Charset)), but it will break consumers still on Java 8/11. If Java 17 is the new minimum, consider encoding it more idiomatically viarelease.Suggested tweak: prefer `release` over separate `source`/`target`
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> <version>3.8.1</version> <configuration> - <source>17</source> - <target>17</target> + <release>17</release> </configuration> </plugin>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 149 - 156, The pom.xml bump of the maven-compiler-plugin to set <source>17</source> and <target>17</target> raises a breaking baseline change for consumers still on Java 8/11; confirm the project intends Java 17 as the minimum and either keep it intentional or revert to the previous baseline (e.g., 11). If Java 17 is deliberate, replace the separate source/target settings with a single <release>17 configuration in the maven-compiler-plugin to more idiomatically control the bytecode and API level (look for the maven-compiler-plugin configuration block in the diff). Ensure documentation/README and any CI/build matrices are updated to reflect Java 17 as the minimum runtime.
🧹 Nitpick comments (2)
src/main/java/com/sforce/bulk/LoginHelper.java (2)
133-164: Base URL / API version derivation is reasonable; consider covering non-standard endpoints with tests.If
authEndpointformats vary (e.g., different SOAP paths or already-OAuth endpoints), small unit tests aroundderiveBaseUrl(...)/deriveApiVersion(...)would keep this stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 133 - 164, Add unit tests that exercise deriveBaseUrl(String) and deriveApiVersion(String) with a variety of non-standard authEndpoint formats (e.g., different SOAP paths like "/services/Soap/c/", OAuth/token endpoints, endpoints with and without ports, trailing slashes, extra path segments, and malformed URLs) to ensure the methods return the expected base URL and API version or fall back to SALESFORCE_VERSION / throw StreamException; specifically create tests referencing the deriveBaseUrl and deriveApiVersion methods and assert correct outputs for: SOAP paths with extra segments, OAuth-style endpoints, host:port cases, endpoints missing "/services/", and invalid URL inputs.
166-181: String-search “JSON parsing” is brittle; prefer shaded Jackson (already a dependency) or harden escaping.
extractJsonStringValue(...)will misbehave on escaped quotes, reordered fields, or minor formatting changes. Since Jackson is already in the project, parsing the token response with it will be more resilient.Sketch of a Jackson-based approach
// imports: com.fasterxml.jackson.databind.JsonNode; com.fasterxml.jackson.databind.ObjectMapper; private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(); // in doLogin(), replace extractJsonStringValue(...) calls: JsonNode root = OBJECT_MAPPER.readTree(response); String accessToken = root.path(JSON_FIELD_ACCESS_TOKEN).isMissingNode() ? null : root.path(JSON_FIELD_ACCESS_TOKEN).asText(); String instanceUrl = root.path(JSON_FIELD_INSTANCE_URL).isMissingNode() ? null : root.path(JSON_FIELD_INSTANCE_URL).asText();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 166 - 181, The current extractJsonStringValue(String key, String json) is brittle; replace its usages in doLogin() by parsing the token response with Jackson's ObjectMapper: add a static final ObjectMapper (e.g., OBJECT_MAPPER), call OBJECT_MAPPER.readTree(response) to get a JsonNode, then extract access token and instance URL via root.path(JSON_FIELD_ACCESS_TOKEN).asText(null) and root.path(JSON_FIELD_INSTANCE_URL).asText(null); remove or deprecate extractJsonStringValue and ensure you catch/handle JsonProcessingException/IOException and surface a clear error when parsing fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/sforce/bulk/LoginHelper.java`:
- Around line 69-90: doLogin() currently allows null/empty authEndpoint,
username, or password to proceed (leading to NPEs or sending empty creds);
before calling deriveBaseUrl/deriveApiVersion or building the body validate
handler.getConfig().getAuthEndpoint(), handler.getConfig().getUsername(), and
handler.getConfig().getPassword() and throw a StreamException with clear
messages if any are null/empty. Retain existing clientId check and continue to
allow clientSecret to be optional, but ensure the tokenUrl construction and
calls to deriveBaseUrl/deriveApiVersion only happen after authEndpoint is
validated.
---
Outside diff comments:
In `@pom.xml`:
- Around line 195-204: In the maven-javadoc-plugin configuration (artifactId
maven-javadoc-plugin, version 3.2.0) remove the unsupported <target>17</target>
element because this plugin only recognizes <source> (or <release> for JDK9+);
keep the existing <source>17</source> (or replace with <release>17</release> if
you prefer) and ensure no other <target> elements remain in the plugin's
<configuration>.
---
Duplicate comments:
In `@pom.xml`:
- Around line 149-156: The pom.xml bump of the maven-compiler-plugin to set
<source>17</source> and <target>17</target> raises a breaking baseline change
for consumers still on Java 8/11; confirm the project intends Java 17 as the
minimum and either keep it intentional or revert to the previous baseline (e.g.,
11). If Java 17 is deliberate, replace the separate source/target settings with
a single <release>17 configuration in the maven-compiler-plugin to more
idiomatically control the bytecode and API level (look for the
maven-compiler-plugin configuration block in the diff). Ensure
documentation/README and any CI/build matrices are updated to reflect Java 17 as
the minimum runtime.
In `@src/main/java/com/sforce/bulk/LoginHelper.java`:
- Around line 99-116: Wrap the HTTP output and input streams in
try-with-resources inside LoginHelper so the OutputStream from
handler.getConfig().createConnection(...) is always closed even if write()
fails, and read the response InputStream (use conn.getInputStream() or fallback
to conn.getErrorStream()) with null-checking for getErrorStream() before calling
FileUtil.toBytes; decode the resulting bytes using StandardCharsets.UTF_8 when
constructing the response string instead of platform default, and ensure
conn.disconnect() is invoked in a finally block (or try-with-resources
equivalent) to always release the connection; update the error path that throws
StreamException to include the UTF-8 decoded response.
- Around line 125-127: Remove the logging of the OAuth access token in
LoginHelper: do not call handler.info("Session Id: " + accessToken) or otherwise
emit the raw bearer token; instead either remove the log line entirely or
replace it with a non-sensitive message that indicates a session was established
(e.g., "Session established" or log only a masked/token hash). Keep
handler.getConfig().setSessionId(accessToken) as-is but ensure any use of
accessToken in logs is redacted or hashed.
---
Nitpick comments:
In `@src/main/java/com/sforce/bulk/LoginHelper.java`:
- Around line 133-164: Add unit tests that exercise deriveBaseUrl(String) and
deriveApiVersion(String) with a variety of non-standard authEndpoint formats
(e.g., different SOAP paths like "/services/Soap/c/", OAuth/token endpoints,
endpoints with and without ports, trailing slashes, extra path segments, and
malformed URLs) to ensure the methods return the expected base URL and API
version or fall back to SALESFORCE_VERSION / throw StreamException; specifically
create tests referencing the deriveBaseUrl and deriveApiVersion methods and
assert correct outputs for: SOAP paths with extra segments, OAuth-style
endpoints, host:port cases, endpoints missing "/services/", and invalid URL
inputs.
- Around line 166-181: The current extractJsonStringValue(String key, String
json) is brittle; replace its usages in doLogin() by parsing the token response
with Jackson's ObjectMapper: add a static final ObjectMapper (e.g.,
OBJECT_MAPPER), call OBJECT_MAPPER.readTree(response) to get a JsonNode, then
extract access token and instance URL via
root.path(JSON_FIELD_ACCESS_TOKEN).asText(null) and
root.path(JSON_FIELD_INSTANCE_URL).asText(null); remove or deprecate
extractJsonStringValue and ensure you catch/handle
JsonProcessingException/IOException and surface a clear error when parsing
fails.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/com/sforce/bulk/LoginHelper.java (1)
69-90:⚠️ Potential issue | 🟠 MajorNPE if
authEndpointis null; emptyusername/passwordsilently sent.
deriveBaseUrl(authEndpoint)at line 71 callsauthEndpoint.indexOf(...)(line 140), which throws NPE whenauthEndpointis null. Additionally, lines 89–90 null-coalesceusername/passwordto"", silently posting empty credentials instead of failing fast with a clear message.Add upfront validation right after retrieving config values:
Proposed fix
void doLogin() throws IOException, StreamException { String authEndpoint = handler.getConfig().getAuthEndpoint(); + if (authEndpoint == null || authEndpoint.isEmpty()) { + throw new StreamException("authEndpoint is required for OAuth2 authentication."); + } + String baseUrl = deriveBaseUrl(authEndpoint); String apiVersion = deriveApiVersion(authEndpoint); String tokenUrl = baseUrl + "/services/oauth2/token"; handler.info("Calling OAuth2 login on: " + tokenUrl); String clientId = handler.getConfig().getClientId(); String clientSecret = handler.getConfig().getClientSecret(); + String username = handler.getConfig().getUsername(); + String password = handler.getConfig().getPassword(); if (clientId == null || clientId.isEmpty()) { throw new StreamException( "clientId is required for OAuth2 authentication. " + "Set it via ConnectorConfig.setClientId() with your Connected App consumer key."); } + if (username == null || username.isEmpty()) { + throw new StreamException("username is required for OAuth2 password-grant authentication."); + } + if (password == null || password.isEmpty()) { + throw new StreamException("password is required for OAuth2 password-grant authentication."); + } String body = GRANT_TYPE_PASSWORD + PARAM_CLIENT_ID + URLEncoder.encode(clientId, StandardCharsets.UTF_8) + PARAM_CLIENT_SECRET + URLEncoder.encode(clientSecret != null ? clientSecret : "", StandardCharsets.UTF_8) - + PARAM_USERNAME + URLEncoder.encode(handler.getConfig().getUsername() != null ? handler.getConfig().getUsername() : "", StandardCharsets.UTF_8) - + PARAM_PASSWORD + URLEncoder.encode(handler.getConfig().getPassword() != null ? handler.getConfig().getPassword() : "", StandardCharsets.UTF_8); + + PARAM_USERNAME + URLEncoder.encode(username, StandardCharsets.UTF_8) + + PARAM_PASSWORD + URLEncoder.encode(password, StandardCharsets.UTF_8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 69 - 90, doLogin currently passes a null authEndpoint into deriveBaseUrl (which causes an NPE) and silently sends empty username/password; fix by validating handler.getConfig().getAuthEndpoint() immediately after retrieval and throw a StreamException with a clear message if it is null or blank before calling deriveBaseUrl, and likewise validate handler.getConfig().getUsername() and handler.getConfig().getPassword() and throw StreamException if either is null/blank so you don't URLEncoder.encode empty credentials; keep existing clientId check and only allow clientSecret to be empty if intentional.
🧹 Nitpick comments (2)
src/main/java/com/sforce/bulk/LoginHelper.java (2)
168-183: Hand-rolled JSON parser is fragile — consider documenting its limitations more clearly.The
extractJsonStringValuemethod won't handle escaped quotes (\") inside values, nested objects, or whitespace variations. This is acceptable foraccess_tokenandinstance_urlin Salesforce OAuth2 responses, but the Javadoc on line 170 is garbled: "Handles escaped quotes within the value are not expected in these fields."Suggested Javadoc fix:
Proposed fix
/** - * Minimal JSON string-value extractor for flat OAuth2 responses. - * Handles escaped quotes within the value are not expected in these fields. + * Minimal JSON string-value extractor for flat OAuth2 responses. + * Does not handle escaped quotes; not expected in access_token or instance_url fields. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 168 - 183, The Javadoc for extractJsonStringValue is unclear and misleading about escaped quotes; update the method comment above extractJsonStringValue to clearly document its limitations: state that it is a minimal extractor for flat, simple OAuth2 JSON responses (used for access_token and instance_url), it does not handle escaped quotes (\"), nested objects/arrays, or arbitrary whitespace/formatting, and callers must only use it with predictable Salesforce OAuth responses; keep the implementation unchanged but make the Javadoc precise and descriptive so future readers understand when this hand-rolled parser is acceptable.
99-132:HttpURLConnectionis never disconnected — potential connection leak.After reading the response (success or error),
conn.disconnect()is never called. On repeated login failures or retries, this can exhaust the connection pool. Addconn.disconnect()in afinallyblock.Proposed fix
HttpURLConnection conn; conn = handler.getConfig().createConnection(url, null, false); - - conn.setRequestMethod(HTTP_METHOD_POST); - conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED); - conn.setDoOutput(true); - - try (OutputStream os = conn.getOutputStream()) { - os.write(body.getBytes(StandardCharsets.UTF_8)); - } - - int status = conn.getResponseCode(); - InputStream errorStream = conn.getErrorStream(); - InputStream responseStream = status == 200 ? conn.getInputStream() : errorStream; - byte[] responseBytes = responseStream != null ? FileUtil.toBytes(responseStream) : new byte[0]; - String response = new String(responseBytes, StandardCharsets.UTF_8); - - if (status != 200) { - throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response); - } - - String accessToken = extractJsonStringValue(JSON_FIELD_ACCESS_TOKEN, response); - String instanceUrl = extractJsonStringValue(JSON_FIELD_INSTANCE_URL, response); - - if (accessToken == null || instanceUrl == null) { - throw new StreamException("Failed to parse OAuth2 response: " + response); - } - - handler.getConfig().setSessionId(accessToken); - handler.info("Access token obtained successfully."); - - String bulkUrl = instanceUrl + "/services/async/" + apiVersion + "/"; - handler.getConfig().setRestEndpoint(bulkUrl); - handler.info("Bulk API Server Url: " + bulkUrl); + try { + conn.setRequestMethod(HTTP_METHOD_POST); + conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED); + conn.setDoOutput(true); + + try (OutputStream os = conn.getOutputStream()) { + os.write(body.getBytes(StandardCharsets.UTF_8)); + } + + int status = conn.getResponseCode(); + InputStream errorStream = conn.getErrorStream(); + InputStream responseStream = status == 200 ? conn.getInputStream() : errorStream; + byte[] responseBytes = responseStream != null ? FileUtil.toBytes(responseStream) : new byte[0]; + String response = new String(responseBytes, StandardCharsets.UTF_8); + + if (status != 200) { + throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response); + } + + String accessToken = extractJsonStringValue(JSON_FIELD_ACCESS_TOKEN, response); + String instanceUrl = extractJsonStringValue(JSON_FIELD_INSTANCE_URL, response); + + if (accessToken == null || instanceUrl == null) { + throw new StreamException("Failed to parse OAuth2 response: " + response); + } + + handler.getConfig().setSessionId(accessToken); + handler.info("Access token obtained successfully."); + + String bulkUrl = instanceUrl + "/services/async/" + apiVersion + "/"; + handler.getConfig().setRestEndpoint(bulkUrl); + handler.info("Bulk API Server Url: " + bulkUrl); + } finally { + conn.disconnect(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 99 - 132, The HttpURLConnection created by handler.getConfig().createConnection(...) (conn) is never disconnected, risking connection leaks; wrap the usage of conn (everything after creation: setRequestMethod, setRequestProperty, setDoOutput, getting OutputStream, reading response, and parsing) in a try { ... } finally { if (conn != null) conn.disconnect(); } block so disconnect() is always called even on exceptions; ensure existing try-with-resources for the OutputStream and any InputStreams remains inside that try so resources are still closed before disconnecting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/com/sforce/bulk/LoginHelper.java`:
- Around line 69-90: doLogin currently passes a null authEndpoint into
deriveBaseUrl (which causes an NPE) and silently sends empty username/password;
fix by validating handler.getConfig().getAuthEndpoint() immediately after
retrieval and throw a StreamException with a clear message if it is null or
blank before calling deriveBaseUrl, and likewise validate
handler.getConfig().getUsername() and handler.getConfig().getPassword() and
throw StreamException if either is null/blank so you don't URLEncoder.encode
empty credentials; keep existing clientId check and only allow clientSecret to
be empty if intentional.
---
Nitpick comments:
In `@src/main/java/com/sforce/bulk/LoginHelper.java`:
- Around line 168-183: The Javadoc for extractJsonStringValue is unclear and
misleading about escaped quotes; update the method comment above
extractJsonStringValue to clearly document its limitations: state that it is a
minimal extractor for flat, simple OAuth2 JSON responses (used for access_token
and instance_url), it does not handle escaped quotes (\"), nested
objects/arrays, or arbitrary whitespace/formatting, and callers must only use it
with predictable Salesforce OAuth responses; keep the implementation unchanged
but make the Javadoc precise and descriptive so future readers understand when
this hand-rolled parser is acceptable.
- Around line 99-132: The HttpURLConnection created by
handler.getConfig().createConnection(...) (conn) is never disconnected, risking
connection leaks; wrap the usage of conn (everything after creation:
setRequestMethod, setRequestProperty, setDoOutput, getting OutputStream, reading
response, and parsing) in a try { ... } finally { if (conn != null)
conn.disconnect(); } block so disconnect() is always called even on exceptions;
ensure existing try-with-resources for the OutputStream and any InputStreams
remains inside that try so resources are still closed before disconnecting.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pom.xmlsrc/main/java/com/sforce/bulk/LoginHelper.javasrc/main/java/com/sforce/ws/ConnectorConfig.javasrc/main/resources/metadata_with_rlm.wsdlsrc/test/java/com/sforce/ws/bind/WsdlXPathTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/com/sforce/ws/bind/WsdlXPathTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- pom.xml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/sforce/bulk/LoginHelper.java (1)
182-197: Consider using Jackson (ObjectMapper) instead of hand-rolled JSON parsing.The project already depends on
com.fasterxml.jackson.core:jackson-databind. The manualindexOf-based extraction is fragile — it can produce false matches when a searched key substring appears inside a JSON string value that contains escaped quotes. While unlikely for Salesforce's OAuth2 response, using Jackson would be both safer and shorter:ObjectMapper mapper = new ObjectMapper(); JsonNode root = mapper.readTree(response); String accessToken = root.path("access_token").asText(null); String instanceUrl = root.path("instance_url").asText(null);Also, the Javadoc on line 184 is grammatically broken ("Handles escaped quotes within the value are not expected…"). Consider rephrasing to e.g. "Does not handle escaped quotes, which are not expected in these fields."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 182 - 197, Replace the fragile hand-rolled extractor extractJsonStringValue(String key, String json) with Jackson-based parsing: use ObjectMapper.readTree on the OAuth2 response and extract fields via JsonNode.path(...).asText(null) (e.g., for "access_token" and "instance_url") to avoid substring/indexOf failures; update callers to use the new parsed JsonNode or helper methods that return the specific values. Also fix the method Javadoc to read something like "Does not handle escaped quotes, which are not expected in these fields." and remove the old indexOf-based logic and comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/sforce/bulk/LoginHelper.java`:
- Around line 113-146: The HttpURLConnection created via
handler.getConfig().createConnection(...) (variable conn) is never disconnected;
wrap the code that uses conn in a try-finally where conn is declared before the
try, keep the existing try-with-resources for the OutputStream and response
stream handling inside the try block, and in the finally block call if (conn !=
null) conn.disconnect(); so the connection is always released (update code
inside LoginHelper where conn is created and used, surrounding the
request/response handling).
---
Nitpick comments:
In `@src/main/java/com/sforce/bulk/LoginHelper.java`:
- Around line 182-197: Replace the fragile hand-rolled extractor
extractJsonStringValue(String key, String json) with Jackson-based parsing: use
ObjectMapper.readTree on the OAuth2 response and extract fields via
JsonNode.path(...).asText(null) (e.g., for "access_token" and "instance_url") to
avoid substring/indexOf failures; update callers to use the new parsed JsonNode
or helper methods that return the specific values. Also fix the method Javadoc
to read something like "Does not handle escaped quotes, which are not expected
in these fields." and remove the old indexOf-based logic and comments.
Comment on lines
113
to
146
| HttpURLConnection conn; | ||
| conn = handler.getConfig().createConnection(url, null, false); | ||
|
|
||
| int index = serverUrl.indexOf(partnerTag); | ||
| conn.setRequestMethod(HTTP_METHOD_POST); | ||
| conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED); | ||
| conn.setDoOutput(true); | ||
|
|
||
| if (index == -1) { | ||
| throw new StreamException("Unknown serverUrl " + serverUrl + "in response " + response); | ||
| try (OutputStream os = conn.getOutputStream()) { | ||
| os.write(body.getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
|
|
||
| String bulkUrl = serverUrl.substring(0, index); | ||
| int status = conn.getResponseCode(); | ||
| InputStream errorStream = conn.getErrorStream(); | ||
| InputStream responseStream = status == 200 ? conn.getInputStream() : errorStream; | ||
| byte[] responseBytes = responseStream != null ? FileUtil.toBytes(responseStream) : new byte[0]; | ||
| String response = new String(responseBytes, StandardCharsets.UTF_8); | ||
|
|
||
| int verIndex = index + partnerTag.length(); | ||
| String version = serverUrl.substring(verIndex, verIndex+4); | ||
| if (status != 200) { | ||
| throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response); | ||
| } | ||
|
|
||
| String accessToken = extractJsonStringValue(JSON_FIELD_ACCESS_TOKEN, response); | ||
| String instanceUrl = extractJsonStringValue(JSON_FIELD_INSTANCE_URL, response); | ||
|
|
||
| bulkUrl = bulkUrl + "/services/async/" + version + "/"; | ||
| if (accessToken == null || instanceUrl == null) { | ||
| throw new StreamException("Failed to parse OAuth2 response: " + response); | ||
| } | ||
|
|
||
| handler.getConfig().setSessionId(accessToken); | ||
| handler.info("Access token obtained successfully."); | ||
|
|
||
| String bulkUrl = instanceUrl + "/services/async/" + apiVersion + "/"; | ||
| handler.getConfig().setRestEndpoint(bulkUrl); | ||
| handler.info("Bulk API Server Url :" + bulkUrl); | ||
| handler.info("Bulk API Server Url: " + bulkUrl); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
HttpURLConnection is never disconnected — potential socket/resource leak.
conn is acquired on line 114 but never closed or disconnected, whether the method completes normally or throws. While the JVM will eventually reclaim idle connections, in high-throughput or retry-heavy scenarios this can exhaust the connection pool or file descriptors.
Wrap the connection usage in a try-finally block.
♻️ Proposed fix
HttpURLConnection conn;
conn = handler.getConfig().createConnection(url, null, false);
-
- conn.setRequestMethod(HTTP_METHOD_POST);
- conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED);
- conn.setDoOutput(true);
-
- try (OutputStream os = conn.getOutputStream()) {
- os.write(body.getBytes(StandardCharsets.UTF_8));
- }
-
- int status = conn.getResponseCode();
- InputStream errorStream = conn.getErrorStream();
- InputStream responseStream = status == 200 ? conn.getInputStream() : errorStream;
- byte[] responseBytes = responseStream != null ? FileUtil.toBytes(responseStream) : new byte[0];
- String response = new String(responseBytes, StandardCharsets.UTF_8);
-
- if (status != 200) {
- throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response);
- }
-
- String accessToken = extractJsonStringValue(JSON_FIELD_ACCESS_TOKEN, response);
- String instanceUrl = extractJsonStringValue(JSON_FIELD_INSTANCE_URL, response);
-
- if (accessToken == null || instanceUrl == null) {
- throw new StreamException("Failed to parse OAuth2 response: " + response);
- }
-
- handler.getConfig().setSessionId(accessToken);
- handler.info("Access token obtained successfully.");
-
- String bulkUrl = instanceUrl + "/services/async/" + apiVersion + "/";
- handler.getConfig().setRestEndpoint(bulkUrl);
- handler.info("Bulk API Server Url: " + bulkUrl);
+ try {
+ conn.setRequestMethod(HTTP_METHOD_POST);
+ conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED);
+ conn.setDoOutput(true);
+
+ try (OutputStream os = conn.getOutputStream()) {
+ os.write(body.getBytes(StandardCharsets.UTF_8));
+ }
+
+ int status = conn.getResponseCode();
+ InputStream errorStream = conn.getErrorStream();
+ InputStream responseStream = status == 200 ? conn.getInputStream() : errorStream;
+ byte[] responseBytes = responseStream != null ? FileUtil.toBytes(responseStream) : new byte[0];
+ String response = new String(responseBytes, StandardCharsets.UTF_8);
+
+ if (status != 200) {
+ throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response);
+ }
+
+ String accessToken = extractJsonStringValue(JSON_FIELD_ACCESS_TOKEN, response);
+ String instanceUrl = extractJsonStringValue(JSON_FIELD_INSTANCE_URL, response);
+
+ if (accessToken == null || instanceUrl == null) {
+ throw new StreamException("Failed to parse OAuth2 response: " + response);
+ }
+
+ handler.getConfig().setSessionId(accessToken);
+ handler.info("Access token obtained successfully.");
+
+ String bulkUrl = instanceUrl + "/services/async/" + apiVersion + "/";
+ handler.getConfig().setRestEndpoint(bulkUrl);
+ handler.info("Bulk API Server Url: " + bulkUrl);
+ } finally {
+ conn.disconnect();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| HttpURLConnection conn; | |
| conn = handler.getConfig().createConnection(url, null, false); | |
| int index = serverUrl.indexOf(partnerTag); | |
| conn.setRequestMethod(HTTP_METHOD_POST); | |
| conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED); | |
| conn.setDoOutput(true); | |
| if (index == -1) { | |
| throw new StreamException("Unknown serverUrl " + serverUrl + "in response " + response); | |
| try (OutputStream os = conn.getOutputStream()) { | |
| os.write(body.getBytes(StandardCharsets.UTF_8)); | |
| } | |
| String bulkUrl = serverUrl.substring(0, index); | |
| int status = conn.getResponseCode(); | |
| InputStream errorStream = conn.getErrorStream(); | |
| InputStream responseStream = status == 200 ? conn.getInputStream() : errorStream; | |
| byte[] responseBytes = responseStream != null ? FileUtil.toBytes(responseStream) : new byte[0]; | |
| String response = new String(responseBytes, StandardCharsets.UTF_8); | |
| int verIndex = index + partnerTag.length(); | |
| String version = serverUrl.substring(verIndex, verIndex+4); | |
| if (status != 200) { | |
| throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response); | |
| } | |
| String accessToken = extractJsonStringValue(JSON_FIELD_ACCESS_TOKEN, response); | |
| String instanceUrl = extractJsonStringValue(JSON_FIELD_INSTANCE_URL, response); | |
| bulkUrl = bulkUrl + "/services/async/" + version + "/"; | |
| if (accessToken == null || instanceUrl == null) { | |
| throw new StreamException("Failed to parse OAuth2 response: " + response); | |
| } | |
| handler.getConfig().setSessionId(accessToken); | |
| handler.info("Access token obtained successfully."); | |
| String bulkUrl = instanceUrl + "/services/async/" + apiVersion + "/"; | |
| handler.getConfig().setRestEndpoint(bulkUrl); | |
| handler.info("Bulk API Server Url :" + bulkUrl); | |
| handler.info("Bulk API Server Url: " + bulkUrl); | |
| HttpURLConnection conn; | |
| conn = handler.getConfig().createConnection(url, null, false); | |
| try { | |
| conn.setRequestMethod(HTTP_METHOD_POST); | |
| conn.setRequestProperty(HEADER_CONTENT_TYPE, CONTENT_TYPE_FORM_URLENCODED); | |
| conn.setDoOutput(true); | |
| try (OutputStream os = conn.getOutputStream()) { | |
| os.write(body.getBytes(StandardCharsets.UTF_8)); | |
| } | |
| int status = conn.getResponseCode(); | |
| InputStream errorStream = conn.getErrorStream(); | |
| InputStream responseStream = status == 200 ? conn.getInputStream() : errorStream; | |
| byte[] responseBytes = responseStream != null ? FileUtil.toBytes(responseStream) : new byte[0]; | |
| String response = new String(responseBytes, StandardCharsets.UTF_8); | |
| if (status != 200) { | |
| throw new StreamException("OAuth2 login failed (HTTP " + status + "): " + response); | |
| } | |
| String accessToken = extractJsonStringValue(JSON_FIELD_ACCESS_TOKEN, response); | |
| String instanceUrl = extractJsonStringValue(JSON_FIELD_INSTANCE_URL, response); | |
| if (accessToken == null || instanceUrl == null) { | |
| throw new StreamException("Failed to parse OAuth2 response: " + response); | |
| } | |
| handler.getConfig().setSessionId(accessToken); | |
| handler.info("Access token obtained successfully."); | |
| String bulkUrl = instanceUrl + "/services/async/" + apiVersion + "/"; | |
| handler.getConfig().setRestEndpoint(bulkUrl); | |
| handler.info("Bulk API Server Url: " + bulkUrl); | |
| } finally { | |
| conn.disconnect(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/com/sforce/bulk/LoginHelper.java` around lines 113 - 146, The
HttpURLConnection created via handler.getConfig().createConnection(...)
(variable conn) is never disconnected; wrap the code that uses conn in a
try-finally where conn is declared before the try, keep the existing
try-with-resources for the OutputStream and response stream handling inside the
try block, and in the finally block call if (conn != null) conn.disconnect(); so
the connection is always released (update code inside LoginHelper where conn is
created and used, surrounding the request/response handling).
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
1 participant
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
Chores
Style