This is an automated email from the ASF dual-hosted git repository.
hansva pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/hop.git
The following commit(s) were added to refs/heads/main by this push:
new b9929f4e5e Harden explorer DnD against null drop targets, fixes #7933
(#7934)
b9929f4e5e is described below
commit b9929f4e5e72c56090de624448fd745c19a67185
Author: Matt Casters <[email protected]>
AuthorDate: Thu Aug 13 15:47:57 2026 +0200
Harden explorer DnD against null drop targets, fixes #7933 (#7934)
* Harden explorer DnD against null drop targets, fixes #7933
Guard ExplorerPerspective drop/dragOver so a null RAP drop target
or bad payload cannot NPE and kill the Hop Web session.
* Harden SSH transform tests against empty stdout under CI load
SessionResult treated channel-closed + available()==0 as EOF too early,
which can drop late stdout packets on busy GitHub runners. Drain with a
short post-close grace when empty, and run the embedded SSHD command
handler asynchronously so onExit is not racy with the JSch client.
---
.../hop/pipeline/transforms/ssh/SessionResult.java | 52 +++++++---
.../pipeline/transforms/ssh/SshTestSupport.java | 60 ++++++++----
.../perspective/explorer/ExplorerPerspective.java | 105 +++++++++++++--------
3 files changed, 151 insertions(+), 66 deletions(-)
diff --git
a/plugins/transforms/ssh/src/main/java/org/apache/hop/pipeline/transforms/ssh/SessionResult.java
b/plugins/transforms/ssh/src/main/java/org/apache/hop/pipeline/transforms/ssh/SessionResult.java
index d24f50618e..82c3d71085 100644
---
a/plugins/transforms/ssh/src/main/java/org/apache/hop/pipeline/transforms/ssh/SessionResult.java
+++
b/plugins/transforms/ssh/src/main/java/org/apache/hop/pipeline/transforms/ssh/SessionResult.java
@@ -32,6 +32,13 @@ public class SessionResult {
private static final int CHANNEL_POLL_MS = 50;
private static final int CHANNEL_MAX_WAIT_MS = 120_000;
+ /**
+ * After JSch reports the channel closed, {@link InputStream#available()}
can still briefly return
+ * 0 while stdout/stderr packets are in flight (common under CI load). Keep
polling a few times
+ * before treating empty streams as complete so we do not drop command
output.
+ */
+ private static final int POST_CLOSE_EMPTY_POLLS = 10;
+
@Getter @Setter private String stdOut;
@Getter private String stdErr;
@Getter @Setter private boolean stdErrorType;
@@ -74,22 +81,37 @@ public class SessionResult {
StringBuilder stderr = new StringBuilder();
long deadline = System.currentTimeMillis() + CHANNEL_MAX_WAIT_MS;
+ int emptyPollsAfterClose = 0;
while (true) {
- appendAvailable(isOut, buffer, stdout);
- appendAvailable(isErr, buffer, stderr);
+ boolean readOut = appendAvailable(isOut, buffer, stdout);
+ boolean readErr = appendAvailable(isErr, buffer, stderr);
+ boolean readSomething = readOut || readErr;
+
if (channel.isClosed()) {
- if (isStreamDrained(isOut) && isStreamDrained(isErr)) {
- break;
+ if (readSomething) {
+ emptyPollsAfterClose = 0;
+ } else if (isStreamDrained(isOut) && isStreamDrained(isErr)) {
+ // Already captured output: channel close is enough. Empty output
needs a short grace
+ // window because available()==0 right after close is not a
reliable EOF under load.
+ if (stdout.length() > 0 || stderr.length() > 0) {
+ break;
+ }
+ emptyPollsAfterClose++;
+ if (emptyPollsAfterClose >= POST_CLOSE_EMPTY_POLLS) {
+ break;
+ }
}
} else if (System.currentTimeMillis() > deadline) {
throw new HopException("Timed out waiting for SSH command to
complete");
} else {
- try {
- Thread.sleep(CHANNEL_POLL_MS);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- throw new HopException(e);
- }
+ emptyPollsAfterClose = 0;
+ }
+
+ try {
+ Thread.sleep(CHANNEL_POLL_MS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new HopException(e);
}
}
@@ -100,18 +122,24 @@ public class SessionResult {
}
}
- private static void appendAvailable(InputStream in, byte[] buffer,
StringBuilder target)
+ /**
+ * @return true if at least one byte was read
+ */
+ private static boolean appendAvailable(InputStream in, byte[] buffer,
StringBuilder target)
throws IOException {
if (in == null) {
- return;
+ return false;
}
+ boolean readSomething = false;
while (in.available() > 0) {
int read = in.read(buffer, 0, buffer.length);
if (read < 0) {
break;
}
target.append(new String(buffer, 0, read, StandardCharsets.UTF_8));
+ readSomething = true;
}
+ return readSomething;
}
private boolean isStreamDrained(InputStream in) throws IOException {
diff --git
a/plugins/transforms/ssh/src/test/java/org/apache/hop/pipeline/transforms/ssh/SshTestSupport.java
b/plugins/transforms/ssh/src/test/java/org/apache/hop/pipeline/transforms/ssh/SshTestSupport.java
index 224c6cd2a6..27a9c54c32 100644
---
a/plugins/transforms/ssh/src/test/java/org/apache/hop/pipeline/transforms/ssh/SshTestSupport.java
+++
b/plugins/transforms/ssh/src/test/java/org/apache/hop/pipeline/transforms/ssh/SshTestSupport.java
@@ -17,7 +17,6 @@
package org.apache.hop.pipeline.transforms.ssh;
-import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
@@ -25,6 +24,10 @@ import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.PublicKey;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ThreadFactory;
+import java.util.concurrent.atomic.AtomicInteger;
import lombok.extern.slf4j.Slf4j;
import org.apache.hop.core.variables.IVariables;
import org.apache.hop.core.variables.Variables;
@@ -46,9 +49,28 @@ class SshTestSupport {
/** Passphrase used when generating test keys; must match {@link
#privateKeyMeta}. */
static final String TEST_KEY_PASSPHRASE = "hello";
+ /**
+ * Lightweight pool for embedded command handlers. SSHD expects {@link
Command#start} not to block
+ * the I/O thread for the whole command lifetime.
+ */
+ private static final ExecutorService COMMAND_EXECUTOR =
+ Executors.newCachedThreadPool(
+ new ThreadFactory() {
+ private final AtomicInteger seq = new AtomicInteger();
+
+ @Override
+ public Thread newThread(Runnable r) {
+ Thread t = new Thread(r, "ssh-test-command-" +
seq.incrementAndGet());
+ t.setDaemon(true);
+ return t;
+ }
+ });
+
static SshServer startPasswordSshServer(PublicKey authorizedPublicKey)
throws IOException {
SshServer sshd = SshServer.setUpDefaultServer();
sshd.setPort(0);
+ // Keep the embedded server lean for constrained CI runners.
+ sshd.setNioWorkers(2);
sshd.setKeyPairProvider(new SimpleGeneratorHostKeyProvider());
sshd.setPasswordAuthenticator(
(username, password, session) -> SSH_USER.equals(username) &&
SSH_PASS.equals(password));
@@ -180,33 +202,39 @@ class SshTestSupport {
}
@Override
- public void start(ChannelSession channel, Environment env) throws
IOException {
+ public void start(ChannelSession channel, Environment env) {
+ // Run off the SSHD I/O thread. Synchronous start+onExit can close the
channel before the
+ // client has attached readers, which shows up as empty stdout under CI
load.
+ COMMAND_EXECUTOR.execute(this::runCommand);
+ }
+
+ private void runCommand() {
try {
if (command != null && command.startsWith("echo ")) {
- String payload = command.substring(5) + "\n";
- stdout.write(payload.getBytes(StandardCharsets.UTF_8));
- stdout.flush();
+ writeAndFlush(stdout, command.substring(5) + "\n");
exitCallback.onExit(0);
} else if (command != null && command.startsWith("stderr ")) {
- String payload = command.substring(7) + "\n";
- stderr.write(payload.getBytes(StandardCharsets.UTF_8));
- stderr.flush();
+ writeAndFlush(stderr, command.substring(7) + "\n");
exitCallback.onExit(0);
} else {
- stderr.write(("unsupported command: " + command +
"\n").getBytes(StandardCharsets.UTF_8));
- stderr.flush();
+ writeAndFlush(stderr, "unsupported command: " + command + "\n");
exitCallback.onExit(1);
}
- } finally {
- closeQuietly(stdout);
- closeQuietly(stderr);
+ } catch (IOException e) {
+ log.warn("Embedded SSH test command failed: {}", command, e);
+ if (exitCallback != null) {
+ exitCallback.onExit(1, e.getMessage());
+ }
}
+ // Do not close stdout/stderr here: SSHD owns channel stream lifecycle
after onExit.
}
- private static void closeQuietly(OutputStream stream) throws IOException {
- if (stream instanceof Closeable closeable) {
- closeable.close();
+ private static void writeAndFlush(OutputStream stream, String payload)
throws IOException {
+ if (stream == null) {
+ return;
}
+ stream.write(payload.getBytes(StandardCharsets.UTF_8));
+ stream.flush();
}
@Override
diff --git
a/ui/src/main/java/org/apache/hop/ui/hopgui/perspective/explorer/ExplorerPerspective.java
b/ui/src/main/java/org/apache/hop/ui/hopgui/perspective/explorer/ExplorerPerspective.java
index 81788c7050..1c2e3e2ae8 100644
---
a/ui/src/main/java/org/apache/hop/ui/hopgui/perspective/explorer/ExplorerPerspective.java
+++
b/ui/src/main/java/org/apache/hop/ui/hopgui/perspective/explorer/ExplorerPerspective.java
@@ -174,7 +174,6 @@ import org.eclipse.swt.widgets.ToolBar;
import org.eclipse.swt.widgets.ToolItem;
import org.eclipse.swt.widgets.Tree;
import org.eclipse.swt.widgets.TreeItem;
-import org.eclipse.swt.widgets.Widget;
@HopPerspectivePlugin(
id = "100-HopExplorerPerspective",
@@ -967,7 +966,11 @@ public class ExplorerPerspective implements
IHopPerspective, TabClosable, IFileD
@Override
public void dragOver(final DropTargetEvent event) {
+ // No tree item under the cursor (empty area / between items).
Reject so RAP/Hop Web
+ // does not attempt an invalid drop that can NPE and kill the
session (#7933).
if (event.item == null) {
+ event.detail = DND.DROP_NONE;
+ event.feedback = DND.FEEDBACK_NONE;
return;
}
@@ -984,10 +987,11 @@ public class ExplorerPerspective implements
IHopPerspective, TabClosable, IFileD
// For internal drag check hierarchies
if (dragFile != null) {
FileObject sourceFile = HopVfs.getFileObject(dragFile);
+ FileObject sourceParent = sourceFile.getParent();
- // Avoid copy or move to itself, it's parent or for folder
it's descendant
+ // Avoid copy or move to itself, its parent or for folder
its descendant
if (sourceFile.equals(targetFile)
- || sourceFile.getParent().equals(targetFile)
+ || (sourceParent != null &&
sourceParent.equals(targetFile))
||
sourceFile.getName().isDescendent(targetFile.getName())) {
event.detail = DND.DROP_NONE;
}
@@ -999,52 +1003,77 @@ public class ExplorerPerspective implements
IHopPerspective, TabClosable, IFileD
event.detail = DND.DROP_NONE;
event.feedback = DND.FEEDBACK_NONE;
}
+ } else {
+ event.detail = DND.DROP_NONE;
+ event.feedback = DND.FEEDBACK_NONE;
}
}
@Override
public void drop(final DropTargetEvent event) {
- if
(FileTransfer.getInstance().isSupportedType(event.currentDataType)) {
- Widget item = event.item;
- if (item.getData() instanceof TreeItemFolder targetItem) {
- List<String> errors = new ArrayList<>();
-
- for (String path : (String[]) event.data) {
- try {
- FileObject sourceFile = HopVfs.getFileObject(path);
- FileObject targetFile =
- HopVfs.getFileObject(
- targetItem.path
- + Const.FILE_SEPARATOR
- + sourceFile.getName().getBaseName());
-
- if (event.detail == DND.DROP_COPY) {
- // Copy file and folder and all its descendants
- // No need to update tab item handler because all files
are new
- targetFile.copyFrom(sourceFile, Selectors.SELECT_ALL);
- } else if (event.detail == DND.DROP_MOVE) {
- // Move file or folder and all its descendants and
update tab item handlers
- moveFile(sourceFile, targetFile);
- }
- } catch (Exception e) {
- errors.add(path);
- }
- }
+ // Guard every precondition: uncaught exceptions in RAP DND
listeners can tear down
+ // the Hop Web UI session (see #7933).
+ try {
+ if
(!FileTransfer.getInstance().isSupportedType(event.currentDataType)
+ || event.item == null
+ || event.data == null
+ || !(event.data instanceof String[] paths)) {
+ event.detail = DND.DROP_NONE;
+ return;
+ }
- // Report errors
- if (!errors.isEmpty()) {
+ Object itemData = event.item.getData();
+ if (!(itemData instanceof TreeItemFolder targetItem) ||
!targetItem.folder) {
+ event.detail = DND.DROP_NONE;
+ return;
+ }
- String paths = String.join("\n", errors);
+ List<String> errors = new ArrayList<>();
- MessageBox messageBox =
- new MessageBox(HopGui.getInstance().getShell(),
SWT.ICON_ERROR | SWT.OK);
- messageBox.setText("Drag and drop");
- messageBox.setMessage("Unable to copy/move file(s):\n\n" +
paths);
- messageBox.open();
+ for (String path : paths) {
+ if (Utils.isEmpty(path)) {
+ continue;
}
+ try {
+ FileObject sourceFile = HopVfs.getFileObject(path);
+ FileObject targetFile =
+ HopVfs.getFileObject(
+ targetItem.path
+ + Const.FILE_SEPARATOR
+ + sourceFile.getName().getBaseName());
+
+ if (event.detail == DND.DROP_COPY) {
+ // Copy file and folder and all its descendants
+ // No need to update tab item handler because all files
are new
+ targetFile.copyFrom(sourceFile, Selectors.SELECT_ALL);
+ } else if (event.detail == DND.DROP_MOVE) {
+ // Move file or folder and all its descendants and update
tab item handlers
+ moveFile(sourceFile, targetFile);
+ }
+ } catch (Exception e) {
+ errors.add(path);
+ }
+ }
+
+ // Report errors
+ if (!errors.isEmpty()) {
+ String errorPaths = String.join("\n", errors);
- refresh();
+ MessageBox messageBox =
+ new MessageBox(HopGui.getInstance().getShell(),
SWT.ICON_ERROR | SWT.OK);
+ messageBox.setText("Drag and drop");
+ messageBox.setMessage("Unable to copy/move file(s):\n\n" +
errorPaths);
+ messageBox.open();
}
+
+ refresh();
+ } catch (Exception e) {
+ event.detail = DND.DROP_NONE;
+ new ErrorDialog(
+ HopGui.getInstance().getShell(),
+ "Drag and drop",
+ "Unexpected error during file drag and drop",
+ e);
}
}
});
