You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hop.apache.org by "hansva (via GitHub)" <gi...@apache.org> on 2026/09/22 14:43:56 UTC
[PR] HttpPost, HTTPClient, HTTPAction cleanup and hardening, fixes #2962, … (hop)
hansva opened a new pull request, #8540:
URL: https://github.com/apache/hop/pull/8540
…fixes #3440
**Please** add a meaningful description for your change here
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Run `mvn clean install apache-rat:check` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [ ] If you have a group of commits related to the same change, please squash your commits into one and force push your branch using `git rebase -i`.
- [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable.
To make clear that you license your contribution under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HttpPost, HTTPClient, HTTPAction cleanup and hardening, fixes #2962, … (hop)
Posted by "mattcasters (via GitHub)" <gi...@apache.org>.
mattcasters merged PR #8540:
URL: https://github.com/apache/hop/pull/8540
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HttpPost, HTTPClient, HTTPAction cleanup and hardening, fixes #2962, … (hop)
Posted by "mattcasters (via GitHub)" <gi...@apache.org>.
mattcasters commented on code in PR #8540:
URL: https://github.com/apache/hop/pull/8540#discussion_r4083725411
##########
plugins/transforms/http/src/main/java/org/apache/hop/pipeline/transforms/http/Http.java:
##########
@@ -279,6 +286,32 @@ private String readResponseBody(HttpEntity entity) throws IOException {
}
}
+ /**
+ * The client to execute with: the one built from the selected REST connection, or a client
+ * configured from this transform's own fields.
+ */
+ private CloseableHttpClient httpClient(HttpHost target) {
+ if (data.restConnectionClient != null) {
+ return data.restConnectionClient;
+ }
+ return createClientBuilder(target).build();
+ }
+
+ /**
+ * Bearer, API-key and preemptive Basic authentication are request headers rather than answers to
+ * a challenge, so the connection writes them onto every request itself.
+ */
+ private void addConnectionAuthentication(HttpGet method, URI uri) throws HopException {
+ if (data.restConnection == null) {
+ return;
+ }
+ Map<String, String> authHeaders = new LinkedHashMap<>();
+ data.restConnection.applyAuthentication(authHeaders, uri.toString());
Review Comment:
**[bug]** `RestConnection.applyAuthentication` sets the credential origin to this request URL before `RestAuthenticator.targetMatchesOrigin` runs, so the check always succeeds. Preemptive Basic (the default), Bearer, API key, and OAuth headers are then written onto every request, including an absolute URL taken from a field or typed in place of the connection base. The REST transform does not do this: it keeps `createClientSettings()`'s origin (the base URL) and calls `applyRequestHeaders` directly, which is what stops those secrets being sent to a different host.
**Suggestion:** Build one `RestAuthenticator` from `createClientSettings()` and call `applyRequestHeaders(headers, uri.toString())`. Do not call `applyAuthentication` for row URLs.
##########
plugins/transforms/httppost/src/main/java/org/apache/hop/pipeline/transforms/httppost/HttpPost.java:
##########
@@ -535,11 +529,124 @@ public boolean init() {
data.realcloseIdleConnectionsTime =
Const.toInt(resolve(meta.getCloseIdleConnectionsTime()), -1);
+ if (!loadRestConnection()) {
+ return false;
+ }
+
+ // Resolved here rather than on the first row: the base URL it may hang off belongs to the
+ // connection just loaded, and every request needs it, including the first.
+ data.realUrl =
+ data.restConnection == null
+ ? resolve(meta.getUrl())
+ : RestConnection.resolveAgainstBase(
+ resolve(data.restConnection.getBaseUrl()), resolve(meta.getUrl()));
+
return true;
}
return false;
}
+ /**
+ * Loads the selected REST connection and the client that goes with it. Returns false when a
+ * connection is named but cannot be loaded, which has to stop the transform: falling back to the
+ * transform's own fields would send the request somewhere else, unauthenticated.
+ */
+ private boolean loadRestConnection() {
+ String realConnectionName = resolve(meta.getConnectionName());
+ if (Utils.isEmpty(realConnectionName)) {
+ return true;
+ }
+ try {
+ data.restConnection =
+ metadataProvider.getSerializer(RestConnection.class).load(realConnectionName);
+ if (data.restConnection == null) {
+ logError(
+ BaseMessages.getString(PKG, "HTTPPOST.Error.ConnectionNotFound", realConnectionName));
+ return false;
+ }
+ data.restConnection.setVariables(this);
+ data.restConnectionClient =
+ RestClientFactory.createClient(data.restConnection.createClientSettings());
+ return true;
+ } catch (Exception e) {
+ // Keep the cause: a class loader split between the metadata plugin and this transform
+ // surfaces here as a ClassCastException, which is not a missing connection at all.
+ logError(
+ BaseMessages.getString(PKG, "HTTPPOST.Error.ConnectionNotLoaded", realConnectionName), e);
+ return false;
+ }
+ }
+
+ @Override
+ public void dispose() {
+ if (data.restConnectionClient != null) {
+ try {
+ data.restConnectionClient.close();
+ } catch (IOException e) {
+ logError(BaseMessages.getString(PKG, "HTTPPOST.Error.ClosingClient"), e);
+ }
+ data.restConnectionClient = null;
+ }
+ super.dispose();
+ }
+
+ /**
+ * The client to execute with: the one built from the selected REST connection, or a client
+ * configured from this transform's own fields.
+ *
+ * <p>Credentials are registered for the host they belong to rather than for any host at all: a
+ * proxy asking for authentication of its own used to be answered with the web server's user name
+ * and password, because a single wildcard {@link org.apache.hc.client5.http.auth.AuthScope}
+ * matched both challenges (issue #3440).
+ */
+ private CloseableHttpClient httpClient(HttpHost target) {
+ if (data.restConnectionClient != null) {
+ return data.restConnectionClient;
+ }
+ HttpClientManager.HttpClientBuilderFacade clientBuilder =
+ HttpClientManager.getInstance().createBuilder();
+
+ if (data.realConnectionTimeout > -1) {
+ clientBuilder.setConnectionTimeout(data.realConnectionTimeout);
+ }
+ if (data.realSocketTimeout > -1) {
+ clientBuilder.setSocketTimeout(data.realSocketTimeout);
+ }
+ if (StringUtils.isNotBlank(data.realHttpLogin)) {
+ clientBuilder.setCredentials(
+ data.realHttpLogin, data.realHttpPassword, new AuthScope(target));
+ }
+ if (StringUtils.isNotBlank(data.realProxyHost)) {
+ HttpHost proxy = new HttpHost("http", data.realProxyHost, data.realProxyPort);
+ clientBuilder.setProxy(proxy.getHostName(), proxy.getPort(), proxy.getSchemeName());
+ clientBuilder.setNonProxyHosts(data.realNonProxyHosts);
+ if (StringUtils.isNotBlank(data.realProxyUsername)) {
+ clientBuilder.setCredentials(
+ data.realProxyUsername, data.realProxyPassword, new AuthScope(proxy));
+ }
+ }
+ if (meta.isIgnoreSsl()) {
+ clientBuilder.ignoreSsl(true);
+ }
+ return clientBuilder.build();
+ }
+
+ /**
+ * Bearer, API-key and preemptive Basic authentication are request headers rather than answers to
+ * a challenge, so the connection writes them onto every request itself.
+ */
+ private void addConnectionAuthentication(
+ org.apache.hc.client5.http.classic.methods.HttpPost post, URI uri) throws HopException {
+ if (data.restConnection == null) {
+ return;
+ }
+ Map<String, String> authHeaders = new LinkedHashMap<>();
+ data.restConnection.applyAuthentication(authHeaders, uri.toString());
Review Comment:
**[bug]** Same origin bypass as the HTTP client transform. `applyAuthentication` rebinds the REST connection's credential origin to `uri`, so a body posted to an absolute URL from the URL field (or any absolute URL that is not the connection host) carries that connection's Basic, Bearer, API-key, or OAuth credentials. `Rest.java` avoids this by leaving the origin on the connection base URL.
**Suggestion:** Use `new RestAuthenticator(restConnection.createClientSettings()).applyRequestHeaders(...)` instead of `applyAuthentication`.
##########
plugins/actions/http/src/main/java/org/apache/hop/workflow/actions/http/ActionHttp.java:
##########
@@ -532,6 +501,137 @@ protected PasswordAuthentication getPasswordAuthentication() {
return result;
}
+ /**
+ * The URL to call, as a URI HttpClient can route. Anything without a scheme and a host is
+ * rejected here rather than further down, where it would surface as a less obvious error.
+ */
+ private static URI toUri(String urlToUse) throws URISyntaxException {
+ URI uri = new URI(Const.NVL(urlToUse, "").trim());
+ if (uri.getScheme() == null || uri.getAuthority() == null) {
+ throw new URISyntaxException(String.valueOf(urlToUse), "No protocol or host in the URL");
+ }
+ return uri;
+ }
+
+ /**
+ * The client for one request: proxy routing, the bypass list and both sets of credentials.
+ *
+ * <p>Server and proxy credentials are registered for their own host only. They used to share a
+ * single JVM-wide {@link java.net.Authenticator}, which answers a 401 from the server and a 407
+ * from the proxy alike, so whichever asked first received the other's user name and password.
+ */
+ private CloseableHttpClient createHttpClient(HttpHost target) {
+ HttpClientManager.HttpClientBuilderFacade builder =
+ HttpClientManager.getInstance().createBuilder();
+
+ String realProxyHost = resolve(proxyHostname);
+ if (!Utils.isEmpty(realProxyHost)) {
+ int realProxyPort = Const.toInt(resolve(proxyPort), DEFAULT_PROXY_PORT);
+ HttpHost proxy = new HttpHost("http", realProxyHost, realProxyPort);
+ builder.setProxy(proxy.getHostName(), proxy.getPort(), proxy.getSchemeName());
+ builder.setNonProxyHosts(resolve(nonProxyHosts));
+ if (!Utils.isEmpty(resolve(proxyUsername))) {
+ builder.setCredentials(
+ resolve(proxyUsername),
+ Encr.decryptPasswordOptionallyEncrypted(resolve(proxyPassword)),
+ new AuthScope(proxy));
+ }
+ }
+
+ if (!Utils.isEmpty(resolve(username))) {
+ builder.setCredentials(
+ resolve(username),
+ Encr.decryptPasswordOptionallyEncrypted(resolve(password)),
+ new AuthScope(target));
+ }
+
+ builder.ignoreSsl(isIgnoreSsl());
+ return builder.build();
+ }
+
+ /**
+ * The REST connection this action was pointed at, or {@code null} when it configures its own
+ * client.
+ */
+ private RestConnection loadRestConnection() throws HopException {
+ String realConnectionName = resolve(connectionName);
+ if (Utils.isEmpty(realConnectionName)) {
+ return null;
+ }
+ IHopMetadataProvider provider = getMetadataProvider();
+ if (provider == null) {
+ throw new HopException(
+ BaseMessages.getString(PKG, "ActionHTTP.Error.ConnectionNotFound", realConnectionName));
+ }
+ try {
+ RestConnection connection =
+ provider.getSerializer(RestConnection.class).load(realConnectionName);
+ if (connection == null) {
+ throw new HopException(
+ BaseMessages.getString(PKG, "ActionHTTP.Error.ConnectionNotFound", realConnectionName));
+ }
+ connection.setVariables(this);
+ return connection;
+ } catch (HopException e) {
+ throw e;
+ } catch (Exception e) {
+ // Keep the cause: a class loader split between the metadata plugin and this action surfaces
+ // here as a ClassCastException, which is not a missing connection at all.
+ throw new HopException(
+ BaseMessages.getString(PKG, "ActionHTTP.Error.ConnectionNotLoaded", realConnectionName),
+ e);
+ }
+ }
+
+ /**
+ * Bearer, API-key and preemptive Basic authentication are request headers rather than answers to
+ * a challenge, so the connection writes them onto every request itself.
+ */
+ private void addConnectionAuthentication(
+ ClassicHttpRequest request, RestConnection restConnection, String urlToUse)
+ throws HopException {
+ if (restConnection == null) {
+ return;
+ }
+ Map<String, String> authHeaders = new LinkedHashMap<>();
+ restConnection.applyAuthentication(authHeaders, urlToUse);
Review Comment:
**[bug]** `applyAuthentication` overwrites the connection's auth origin with `urlToUse`, so the origin check cannot reject a different host. With "Run for every result row" that URL comes from a previous result, and an absolute URL is also returned unchanged by `resolveAgainstBase`. The connection's preemptive credentials are then sent to that host. A user-typed URL on another host has the same effect, which the REST transform deliberately does not do.
**Suggestion:** Apply headers through a `RestAuthenticator` created from `createClientSettings()` so the origin stays the connection base URL.
##########
plugins/actions/http/src/main/java/org/apache/hop/workflow/actions/http/ActionHttp.java:
##########
@@ -532,6 +501,137 @@ protected PasswordAuthentication getPasswordAuthentication() {
return result;
}
+ /**
+ * The URL to call, as a URI HttpClient can route. Anything without a scheme and a host is
+ * rejected here rather than further down, where it would surface as a less obvious error.
+ */
+ private static URI toUri(String urlToUse) throws URISyntaxException {
+ URI uri = new URI(Const.NVL(urlToUse, "").trim());
+ if (uri.getScheme() == null || uri.getAuthority() == null) {
+ throw new URISyntaxException(String.valueOf(urlToUse), "No protocol or host in the URL");
+ }
+ return uri;
+ }
+
+ /**
+ * The client for one request: proxy routing, the bypass list and both sets of credentials.
+ *
+ * <p>Server and proxy credentials are registered for their own host only. They used to share a
+ * single JVM-wide {@link java.net.Authenticator}, which answers a 401 from the server and a 407
+ * from the proxy alike, so whichever asked first received the other's user name and password.
+ */
+ private CloseableHttpClient createHttpClient(HttpHost target) {
+ HttpClientManager.HttpClientBuilderFacade builder =
+ HttpClientManager.getInstance().createBuilder();
+
+ String realProxyHost = resolve(proxyHostname);
+ if (!Utils.isEmpty(realProxyHost)) {
Review Comment:
**[suggestion]** When the proxy host is empty this client now always connects directly. The previous `URLConnection` path used the JVM `ProxySelector`, so a workflow that left the action proxy blank and relied on `-Dhttp.proxyHost` / `java.net.useSystemProxies` will stop using that proxy. Dropping `System.setProperty` is the right fix for #2962; reading the existing selector is separate.
**Suggestion:** If no proxy host is set, install `SystemDefaultRoutePlanner` (or `ProxySelector.getDefault()`) instead of leaving `DefaultRoutePlanner`, which ignores system proxy settings.
##########
plugins/transforms/http/src/main/java/org/apache/hop/pipeline/transforms/http/HttpMeta.java:
##########
@@ -90,6 +91,17 @@ public class HttpMeta extends BaseTransformMeta<Http, HttpData> {
private String closeIdleConnectionsTime;
/** URL / service to be called */
+ /**
+ * Optional REST connection supplying the client: proxy, credentials, TLS and timeouts. When one
+ * is selected the transform's own authentication, proxy and SSL fields are not read.
+ */
+ @HopMetadataProperty(
+ key = "connection_name",
+ injectionKey = "CONNECTION_NAME",
+ injectionKeyDescription = "HttpMeta.Injection.CONNECTION_NAME",
Review Comment:
**[suggestion]** The new injection descriptions (`HttpMeta.Injection.CONNECTION_NAME`, `PROXY_USERNAME`, `PROXY_PASSWORD`, `NON_PROXY_HOSTS`, and the matching `HTTPPOST.Injection.connectionName` / `proxyUsername` / `proxyPassword` / `nonProxyHosts` keys) are not in the message bundles. Metadata injection will show the unresolved key.
**Suggestion:** Add those strings to `messages_en_US.properties` for both transforms.
##########
plugins/transforms/http/src/main/java/org/apache/hop/pipeline/transforms/http/Http.java:
##########
@@ -490,4 +555,47 @@ public boolean init() {
}
return false;
}
+
+ /**
+ * Loads the selected REST connection and the client that goes with it. Returns false when a
+ * connection is named but cannot be loaded, which has to stop the transform: falling back to the
+ * transform's own fields would send the request somewhere else, unauthenticated.
+ */
+ private boolean loadRestConnection() {
+ String realConnectionName = resolve(meta.getConnectionName());
+ if (Utils.isEmpty(realConnectionName)) {
+ return true;
+ }
+ try {
+ data.restConnection =
+ metadataProvider.getSerializer(RestConnection.class).load(realConnectionName);
Review Comment:
**[suggestion]** `HttpMeta` / `HttpPostMeta` never set `classLoaderGroup = "rest"`, unlike `ActionHttp` and `RestMeta` in this same change. `dependencies.xml` only puts `hop-misc-rest` on this plugin's own loader, so `RestConnection.class` here is a second copy of the metadata type. The catch below is written for the `ClassCastException` that split produces, and the action comment says the group is what loads the connection from the loader that defined it.
**Suggestion:** Add `classLoaderGroup = "rest"` on both transform `@Transform` annotations, matching the action.
##########
plugins/actions/http/src/main/java/org/apache/hop/workflow/actions/http/ActionHttpDialog.java:
##########
@@ -679,6 +753,80 @@ private Group setupAuthGroup(Composite wGeneralComp) {
return wAuthentication;
}
+ private void setupConnectionLine(int margin, Composite wGeneralComp) {
+ wConnection =
+ new MetaSelectionLine<>(
+ variables,
+ metadataProvider,
+ RestConnection.class,
+ wGeneralComp,
+ SWT.SINGLE | SWT.LEFT | SWT.BORDER,
+ BaseMessages.getString(PKG, "ActionHTTP.Connection.Label"),
+ BaseMessages.getString(PKG, "ActionHTTP.Connection.Tooltip"));
+ PropsUi.setLook(wConnection);
+ FormData fdConnection = new FormData();
+ fdConnection.left = new FormAttachment(0, 0);
+ fdConnection.top = new FormAttachment(0, margin);
+ fdConnection.right = new FormAttachment(100, 0);
+ wConnection.setLayoutData(fdConnection);
+ wConnection.addListener(
+ SWT.Selection,
+ e -> {
+ action.setChanged();
+ activateConnectionSupersededFields();
+ });
+ wConnection.addModifyListener(
+ e -> {
+ action.setChanged();
+ activateConnectionSupersededFields();
+ });
+ try {
+ wConnection.fillItems();
+ } catch (Exception e) {
+ new ErrorDialog(
+ shell,
+ BaseMessages.getString(PKG, "System.Dialog.Error.Title"),
+ "Error getting the list of REST connections",
Review Comment:
**[nit]** This dialog error is hardcoded English. The same string is copied in `HttpDialog` and `HttpPostDialog`.
**Suggestion:** Move it to the action/transform message bundles, same as the other dialog titles.
##########
plugins/actions/http/src/main/java/org/apache/hop/workflow/actions/http/ActionHttp.java:
##########
@@ -532,6 +501,137 @@ protected PasswordAuthentication getPasswordAuthentication() {
return result;
}
+ /**
+ * The URL to call, as a URI HttpClient can route. Anything without a scheme and a host is
+ * rejected here rather than further down, where it would surface as a less obvious error.
+ */
+ private static URI toUri(String urlToUse) throws URISyntaxException {
+ URI uri = new URI(Const.NVL(urlToUse, "").trim());
Review Comment:
**[bug]** `new URI(String)` rejects URLs that `new URL(String)` used to open. `http://example.com/a b` and `http://example.com/path?q=a b` now fail here with `URISyntaxException` and the action errors; previously `URLConnection` accepted them. URLs with userinfo (`http://user:pass@host/file`) pass this check and then fail later in HttpClient 5 with `ProtocolException: Request URI authority contains deprecated userinfo component`.
**Suggestion:** Build the URI with the multi-argument constructor (or an encoder) so illegal path/query characters are escaped, and strip userinfo before creating the request, surfacing a clear error if it was being used as credentials.
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HttpPost, HTTPClient, HTTPAction cleanup and hardening, fixes #2962, … (hop)
Posted by "hansva (via GitHub)" <gi...@apache.org>.
hansva commented on PR #8540:
URL: https://github.com/apache/hop/pull/8540#issuecomment-5809403710
All issues should be resolved, also did an extra sweep on credentials leaking and hardened that part too, logging should carry sensitive information less frequently
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org