You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tomee.apache.org by "ksmigrod (via GitHub)" <gi...@apache.org> on 2026/05/05 05:17:54 UTC
[PR] TOMEE-4604 strip ssl keystore configuration params from outgoing URL. (tomee)
ksmigrod opened a new pull request, #2657:
URL: https://github.com/apache/tomee/pull/2657
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TOMEE-4604 strip ssl keystore configuration params from outgoing URL. (tomee)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on code in PR #2657:
URL: https://github.com/apache/tomee/pull/2657#discussion_r3186215181
##########
server/openejb-client/src/main/java/org/apache/openejb/client/HttpConnectionFactory.java:
##########
@@ -140,20 +140,21 @@ public HttpConnection(final URI uri, final ConcurrentMap<URI, SSLSocketFactory>
}
}
- private String stripQuery(final String url, final String param) {
- String result = url;
- do {
- final int h = result.indexOf(param + '=');
- int end = result.indexOf('&', h);
- if (end < 0) {
- end = result.length();
+ private String stripQuery(final String url, final String []params) {
Review Comment:
**Style nit:** same `String []params` -> `String[] params`. Also, brace-on-same-line one-liners (`if (queryStartIndex < 0) { return url; }`) are out of style for this module - every other early-return in this file uses a multi-line block. Please align:
```java
private String stripQuery(final String url, final String[] params) {
final int queryStartIndex = url.indexOf('?');
if (queryStartIndex < 0) {
return url;
}
if (queryStartIndex + 1 == url.length()) {
return url.substring(0, queryStartIndex);
}
...
}
```
##########
server/openejb-client/src/main/java/org/apache/openejb/client/HttpConnectionFactory.java:
##########
@@ -59,6 +59,13 @@ public Connection getConnection(final URI uri) throws IOException {
}
public static class HttpConnection implements Connection {
+ private final static String []PARAMS_TO_STRIP = new String[]{
+ // must be sorted alphabetically
+ "authorization", "authorizationHeader", "basic.password", "basic.username",
+ "connectTimeout", "readTimeout",
Review Comment:
**Blocking - missing import, will not compile.** The new `stripQuery` calls `Arrays.binarySearch(...)` twice but the import block is untouched. Please add:
```java
import java.util.Arrays;
```
**Style nit:** Apache/openejb-client convention is `static final` (not `final static`) and `String[] NAME` (not `String []NAME`):
```suggestion
private static final String[] PARAMS_TO_STRIP = new String[]{
```
##########
server/openejb-client/src/main/java/org/apache/openejb/client/HttpConnectionFactory.java:
##########
@@ -89,14 +96,7 @@ public HttpConnection(final URI uri, final ConcurrentMap<URI, SSLSocketFactory>
authorization = "Basic " + printBase64Binary((basicUsername + (basicPassword != null ? ":" + basicPassword : "")).getBytes(StandardCharsets.UTF_8));
}
- final String newUrl =
- stripQuery(
- stripQuery(
- stripQuery(
- stripQuery(url.toExternalForm(), "authorization"),
- "basic.username"),
- "basic.password"),
- "authorizationHeader");
+ final String newUrl = stripQuery(url.toExternalForm(), PARAMS_TO_STRIP);
httpURLConnection = (HttpURLConnection) (authorization == null ? url : new URL(newUrl)).openConnection();
Review Comment:
**Blocking - SSL params are still leaked when no basic auth is set, which is the case the JIRA is trying to fix.** This conditional is unchanged from before the patch, so `newUrl` is only used when `authorization != null`. In the SSL-only flow that `HttpsConnectionTest.testHttps` exercises (`?sslKeyStore=...&sslKeyStorePassword=...` with no basic auth), `authorization` is `null` and the original `url` (including `sslKeyStorePassword=...`) is what reaches `openConnection()` and goes on the wire as the request URI.
Suggest dropping the ternary entirely now that the strip-list is cheap:
```suggestion
httpURLConnection = (HttpURLConnection) new URL(newUrl).openConnection();
```
##########
server/openejb-client/src/main/java/org/apache/openejb/client/HttpConnectionFactory.java:
##########
@@ -140,20 +140,21 @@ public HttpConnection(final URI uri, final ConcurrentMap<URI, SSLSocketFactory>
}
}
- private String stripQuery(final String url, final String param) {
- String result = url;
- do {
- final int h = result.indexOf(param + '=');
- int end = result.indexOf('&', h);
- if (end < 0) {
- end = result.length();
+ private String stripQuery(final String url, final String []params) {
+ final int queryStartIndex = url.indexOf('?');
+ if (queryStartIndex < 0) { return url; }
+ if (queryStartIndex + 1 == url.length()) { return url.substring(0, queryStartIndex); }
+
+ final StringBuilder sb = new StringBuilder();
+ for (String param : url.substring(queryStartIndex+1).split("&")) {
+ final int p = param.indexOf('=');
+ if ((p < 0 && Arrays.binarySearch(params, param) < 0)
Review Comment:
**Edge case:** a token shaped `=value` (empty name, `p == 0`) matches neither branch of the `||` and is silently dropped. Won't happen with well-formed URIs, but `p >= 0` is more defensible than `p > 0`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TOMEE-4604 strip ssl keystore configuration params from outgoing URL. (tomee)
Posted by "ksmigrod (via GitHub)" <gi...@apache.org>.
ksmigrod closed pull request #2657: TOMEE-4604 strip ssl keystore configuration params from outgoing URL.
URL: https://github.com/apache/tomee/pull/2657
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org