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 b95b7cd4c6 [SONAR] cleanup security backlog
b95b7cd4c6 is described below
commit b95b7cd4c6b5acea75b96c852cfa63e3f002c53a
Author: Hans Van Akelyen <[email protected]>
AuthorDate: Sat Sep 26 10:50:58 2026 +0200
[SONAR] cleanup security backlog
---
.github/workflows/marketplace_classpath.yml | 2 +-
.github/workflows/pr_build_docs.yml | 2 +-
.../java/org/apache/hop/core/util/StringUtil.java | 4 +-
.../hop/history/local/LocalAuditManager.java | 65 +---------------------
.../java/org/apache/hop/www/JdbcTokenServlet.java | 2 +
.../main/java/org/apache/hop/lint/LintCommand.java | 2 +
.../org/apache/hop/vfs/ftp/FtpClientFactory.java | 2 +
.../org/apache/hop/ui/hopgui/HopWebAuditPaths.java | 25 ---------
.../apache/hop/ui/hopgui/HopWebUserFilePlugin.java | 2 +
.../ui/hopgui/explorer/ExplorerFileServlet.java | 2 +
.../hop/ui/hopgui/security/HopBasicAuthFilter.java | 4 +-
.../hop/ui/hopgui/security/HopLoginPage.java | 18 +++++-
.../hop/ui/hopgui/security/HopOidcAuthFilter.java | 2 +
.../hop/ui/hopgui/security/HopLoginPageTest.java | 12 ++++
14 files changed, 51 insertions(+), 93 deletions(-)
diff --git a/.github/workflows/marketplace_classpath.yml
b/.github/workflows/marketplace_classpath.yml
index 965baceb31..a16b385a58 100644
--- a/.github/workflows/marketplace_classpath.yml
+++ b/.github/workflows/marketplace_classpath.yml
@@ -126,7 +126,7 @@ jobs:
continue-on-error: true
run: |
set -uo pipefail
- suites=$(curl -fsSL --max-time 60 \
+ suites=$(curl -fsSL --proto '=https' --proto-redir '=https'
--max-time 60 \
"$IT_REPORT_ROOT/api/json?tree=suites\[name\]" | tr ',' '\n' |
sed -n 's/.*"name":"\([^"]*\)".*/\1/p' | sort -u)
if [ -z "$suites" ]; then
diff --git a/.github/workflows/pr_build_docs.yml
b/.github/workflows/pr_build_docs.yml
index 14a71fdfe4..b3c507f06d 100644
--- a/.github/workflows/pr_build_docs.yml
+++ b/.github/workflows/pr_build_docs.yml
@@ -86,7 +86,7 @@ jobs:
- name: Install the site generator
working-directory: hop-website
- run: npm ci
+ run: npm ci --ignore-scripts
# Same pre-build steps as the website's `npm run build:hop`: the shared
# stylesheets, the navigation partials and the browser halves of the
diff --git a/core/src/main/java/org/apache/hop/core/util/StringUtil.java
b/core/src/main/java/org/apache/hop/core/util/StringUtil.java
index c7d1c584a5..bcc3e8d84e 100644
--- a/core/src/main/java/org/apache/hop/core/util/StringUtil.java
+++ b/core/src/main/java/org/apache/hop/core/util/StringUtil.java
@@ -33,6 +33,8 @@ import org.apache.hop.core.row.IRowMeta;
/** A collection of utilities to manipulate strings. */
public class StringUtil {
+ // Only used to generate test data, never for anything security related
+ @SuppressWarnings("java:S2245")
private static final Random random = new Random();
public static final String UNIX_OPEN = "${";
@@ -405,7 +407,7 @@ public class StringUtil {
}
for (int i = 0; i < length; i++) {
- int c = 'a' + random.nextInt() * 26;
+ int c = 'a' + random.nextInt(26);
buffer.append((char) c);
}
if (!Utils.isEmpty(postfix)) {
diff --git
a/engine/src/main/java/org/apache/hop/history/local/LocalAuditManager.java
b/engine/src/main/java/org/apache/hop/history/local/LocalAuditManager.java
index 979dab9f15..f9f8c898e8 100644
--- a/engine/src/main/java/org/apache/hop/history/local/LocalAuditManager.java
+++ b/engine/src/main/java/org/apache/hop/history/local/LocalAuditManager.java
@@ -20,10 +20,7 @@ import com.fasterxml.jackson.databind.ObjectMapper;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
-import java.nio.file.Path;
import java.nio.file.Paths;
-import java.nio.file.attribute.PosixFilePermission;
-import java.nio.file.attribute.PosixFilePermissions;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Collections;
@@ -31,7 +28,6 @@ import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
-import java.util.Set;
import org.apache.commons.io.FileUtils;
import org.apache.commons.lang3.StringUtils;
import org.apache.hop.core.Const;
@@ -83,64 +79,9 @@ public class LocalAuditManager implements IAuditManager {
}
}
- /**
- * Create {@code dir} (and parents) if missing, and best-effort open POSIX
permissions so Docker
- * bind mounts remain usable when host UID and container hop UID differ
(Tomcat often uses umask
- * 0027 which would otherwise leave 0700/0750 dirs unwritable after a
host-side chown).
- */
- private void ensureWritableDirectory(File dir) throws IOException {
- if (dir == null) {
- return;
- }
- Path path = dir.toPath().toAbsolutePath().normalize();
- Files.createDirectories(path);
- openPermissionsAlongPath(path);
- }
-
- /** World rwx on directories from {@link #rootFolder} down to {@code path}
(best effort). */
- private void openPermissionsAlongPath(Path path) {
- if (path == null) {
- return;
- }
- Path root;
- try {
- root = Paths.get(rootFolder).toAbsolutePath().normalize();
- } catch (Exception e) {
- root = null;
- }
- Set<PosixFilePermission> dirPerms =
PosixFilePermissions.fromString("rwxrwxrwx");
- Path current = path;
- while (current != null) {
- try {
- if (Files.isDirectory(current)) {
- try {
- Files.setPosixFilePermissions(current, dirPerms);
- } catch (UnsupportedOperationException e) {
- File f = current.toFile();
- //noinspection ResultOfMethodCallIgnored
- f.setReadable(true, false);
- //noinspection ResultOfMethodCallIgnored
- f.setWritable(true, false);
- //noinspection ResultOfMethodCallIgnored
- f.setExecutable(true, false);
- }
- }
- } catch (IOException | SecurityException e) {
- // Not owner / read-only FS — caller may still fail on write with a
clear error
- LogChannel.GENERAL.logDebug(
- "LocalAuditManager: could not open permissions on '"
- + current
- + "': "
- + e.getMessage());
- }
- if (root != null && current.equals(root)) {
- break;
- }
- Path parent = current.getParent();
- if (parent == null || parent.equals(current)) {
- break;
- }
- current = parent;
+ private static void ensureWritableDirectory(File dir) throws IOException {
+ if (dir != null) {
+ Files.createDirectories(dir.toPath());
}
}
diff --git a/engine/src/main/java/org/apache/hop/www/JdbcTokenServlet.java
b/engine/src/main/java/org/apache/hop/www/JdbcTokenServlet.java
index cb9ee33cc7..3ea17d617f 100644
--- a/engine/src/main/java/org/apache/hop/www/JdbcTokenServlet.java
+++ b/engine/src/main/java/org/apache/hop/www/JdbcTokenServlet.java
@@ -68,6 +68,8 @@ public class JdbcTokenServlet extends BaseHttpServlet
implements IHopServerPlugi
return "JDBC token";
}
+ // I/O errors writing the response are left to the servlet container
+ @SuppressWarnings("java:S1989")
@Override
public void doGet(HttpServletRequest request, HttpServletResponse response)
throws ServletException, IOException {
diff --git
a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCommand.java
b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCommand.java
index 6d81f3c6ad..cb3e789505 100644
--- a/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCommand.java
+++ b/plugins/misc/lint/src/main/java/org/apache/hop/lint/LintCommand.java
@@ -195,6 +195,8 @@ public class LintCommand implements Callable<Integer>,
IHopCommand {
cmd.setCaseInsensitiveEnumValuesAllowed(true);
}
+ // The stack trace is only printed when the user asks for it with --verbose
+ @SuppressWarnings("java:S4507")
@Override
public Integer call() {
try {
diff --git
a/plugins/tech/ftp/src/main/java/org/apache/hop/vfs/ftp/FtpClientFactory.java
b/plugins/tech/ftp/src/main/java/org/apache/hop/vfs/ftp/FtpClientFactory.java
index 3c80805442..3f7d43c03e 100644
---
a/plugins/tech/ftp/src/main/java/org/apache/hop/vfs/ftp/FtpClientFactory.java
+++
b/plugins/tech/ftp/src/main/java/org/apache/hop/vfs/ftp/FtpClientFactory.java
@@ -244,6 +244,8 @@ public final class FtpClientFactory {
}
}
+ // Plain FTP is an explicit choice of the connection; FTPS is offered next
to it
+ @SuppressWarnings("java:S5332")
private static FTPClient createClient(
FtpSecurityMode securityMode, IVariables variables, IFtpConnection
connection)
throws HopException {
diff --git a/rap/src/main/java/org/apache/hop/ui/hopgui/HopWebAuditPaths.java
b/rap/src/main/java/org/apache/hop/ui/hopgui/HopWebAuditPaths.java
index 9f933f433c..ee33379a4d 100644
--- a/rap/src/main/java/org/apache/hop/ui/hopgui/HopWebAuditPaths.java
+++ b/rap/src/main/java/org/apache/hop/ui/hopgui/HopWebAuditPaths.java
@@ -19,8 +19,6 @@ package org.apache.hop.ui.hopgui;
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
-import java.nio.file.Path;
-import java.nio.file.attribute.PosixFilePermissions;
import org.apache.hop.core.Const;
import org.apache.hop.core.logging.LogChannel;
@@ -131,7 +129,6 @@ public final class HopWebAuditPaths {
if (!dir.isDirectory()) {
return null;
}
- openWorldWritable(dir.toPath());
File probe = new File(dir, ".hop-write-test");
Files.writeString(probe.toPath(), "ok");
//noinspection ResultOfMethodCallIgnored
@@ -141,31 +138,9 @@ public final class HopWebAuditPaths {
//noinspection ResultOfMethodCallIgnored
users.mkdirs();
}
- openWorldWritable(users.toPath());
return dir;
} catch (IOException | SecurityException e) {
return null;
}
}
-
- /**
- * Best-effort {@code rwxrwxrwx} so host UID and container hop UID (often
501) can both use a bind
- * mount. No-op on non-POSIX filesystems or when not the owner.
- */
- private static void openWorldWritable(Path path) {
- if (path == null || !Files.isDirectory(path)) {
- return;
- }
- try {
- Files.setPosixFilePermissions(path,
PosixFilePermissions.fromString("rwxrwxrwx"));
- } catch (UnsupportedOperationException | IOException | SecurityException
e) {
- File f = path.toFile();
- //noinspection ResultOfMethodCallIgnored
- f.setReadable(true, false);
- //noinspection ResultOfMethodCallIgnored
- f.setWritable(true, false);
- //noinspection ResultOfMethodCallIgnored
- f.setExecutable(true, false);
- }
- }
}
diff --git
a/rap/src/main/java/org/apache/hop/ui/hopgui/HopWebUserFilePlugin.java
b/rap/src/main/java/org/apache/hop/ui/hopgui/HopWebUserFilePlugin.java
index 988e075f66..22b54814e7 100644
--- a/rap/src/main/java/org/apache/hop/ui/hopgui/HopWebUserFilePlugin.java
+++ b/rap/src/main/java/org/apache/hop/ui/hopgui/HopWebUserFilePlugin.java
@@ -626,6 +626,8 @@ public class HopWebUserFilePlugin {
}
}
+ // Files.createTempDirectory creates the folder with owner-only (0700)
permissions on POSIX
+ @SuppressWarnings("java:S5443")
private static Path getSessionTempDirectory() throws IOException {
UISession session = RWT.getUISession();
Path directory = (Path) session.getAttribute(SESSION_TEMP_DIRECTORY);
diff --git
a/rap/src/main/java/org/apache/hop/ui/hopgui/explorer/ExplorerFileServlet.java
b/rap/src/main/java/org/apache/hop/ui/hopgui/explorer/ExplorerFileServlet.java
index 3195e8195f..5e02f8515d 100644
---
a/rap/src/main/java/org/apache/hop/ui/hopgui/explorer/ExplorerFileServlet.java
+++
b/rap/src/main/java/org/apache/hop/ui/hopgui/explorer/ExplorerFileServlet.java
@@ -47,6 +47,8 @@ public class ExplorerFileServlet extends HttpServlet {
static final long DEFAULT_MAX_BYTES = 16L * 1024L * 1024L;
+ // I/O errors writing the response are left to the servlet container
+ @SuppressWarnings("java:S1989")
@Override
protected void doGet(HttpServletRequest request, HttpServletResponse
response)
throws IOException {
diff --git
a/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopBasicAuthFilter.java
b/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopBasicAuthFilter.java
index 06743a3184..ad81b6ca91 100644
---
a/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopBasicAuthFilter.java
+++
b/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopBasicAuthFilter.java
@@ -142,6 +142,8 @@ public class HopBasicAuthFilter implements Filter {
chain.doFilter(new HopAuthenticatedRequest(httpRequest, principal),
response);
}
+ // The username is logged through HopLoginPage.sanitizeForLog
+ @SuppressWarnings("javasecurity:S5145")
private void handleLoginPost(
HttpServletRequest request, HttpServletResponse response, String
contextPath)
throws IOException {
@@ -157,7 +159,7 @@ public class HopBasicAuthFilter implements Filter {
Optional<HopUser> user = HopUserStore.getInstance().authenticate(username,
password);
if (user.isEmpty()) {
- LOG.log(Level.INFO, "Login failed for user ''{0}''", username);
+ LOG.log(Level.INFO, "Login failed for user ''{0}''",
HopLoginPage.sanitizeForLog(username));
showLoginPage(request, response, contextPath, "Invalid username or
password.", username);
return;
}
diff --git
a/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopLoginPage.java
b/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopLoginPage.java
index 59f7cb4705..adaec4a3b3 100644
--- a/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopLoginPage.java
+++ b/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopLoginPage.java
@@ -243,8 +243,12 @@ public final class HopLoginPage {
return fallback;
}
String r = redirect.trim();
- // Block open redirects
- if (r.startsWith("http://";) || r.startsWith("https://";) ||
r.startsWith("//")) {
+ // Block open redirects. Browsers treat a backslash as a slash, so
"/\host" means "//host".
+ if (r.startsWith("http://";)
+ || r.startsWith("https://";)
+ || r.startsWith("//")
+ || r.indexOf('\\') >= 0
+ || r.chars().anyMatch(Character::isISOControl)) {
return fallback;
}
if (!r.startsWith("/")) {
@@ -260,6 +264,16 @@ public final class HopLoginPage {
return r;
}
+ /** Replaces control characters (CR, LF, ...) so user input cannot forge
extra log lines. */
+ public static String sanitizeForLog(String raw) {
+ if (raw == null) {
+ return null;
+ }
+ StringBuilder sb = new StringBuilder(raw.length());
+ raw.codePoints().forEach(c -> sb.appendCodePoint(Character.isISOControl(c)
? '_' : c));
+ return sb.toString();
+ }
+
static String escapeHtml(String raw) {
if (raw == null) {
return "";
diff --git
a/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopOidcAuthFilter.java
b/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopOidcAuthFilter.java
index 19cc314e64..b8bfaeb8f0 100644
--- a/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopOidcAuthFilter.java
+++ b/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopOidcAuthFilter.java
@@ -301,6 +301,8 @@ public class HopOidcAuthFilter implements Filter {
response.sendRedirect(contextPath + HopLoginPage.PATH_LOGIN + "?logout=1");
}
+ // HopLoginPage sanitizes the redirect and HTML-escapes every value it
renders
+ @SuppressWarnings("javasecurity:S5131")
private void showLoginPage(
HttpServletRequest request,
HttpServletResponse response,
diff --git
a/rap/src/test/java/org/apache/hop/ui/hopgui/security/HopLoginPageTest.java
b/rap/src/test/java/org/apache/hop/ui/hopgui/security/HopLoginPageTest.java
index 0be92df6e4..2682fda121 100644
--- a/rap/src/test/java/org/apache/hop/ui/hopgui/security/HopLoginPageTest.java
+++ b/rap/src/test/java/org/apache/hop/ui/hopgui/security/HopLoginPageTest.java
@@ -17,7 +17,9 @@
package org.apache.hop.ui.hopgui.security;
+import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import org.junit.jupiter.api.Test;
@@ -40,6 +42,16 @@ class HopLoginPageTest {
assertTrue(HopLoginPage.escapeHtml("<x>").contains("<"));
assertTrue(HopLoginPage.sanitizeRedirect("https://evil.example/";,
"").endsWith("/ui"));
assertTrue(HopLoginPage.sanitizeRedirect("/ui-dark",
"").equals("/ui-dark"));
+ assertEquals("/ui", HopLoginPage.sanitizeRedirect("/\\evil.example", ""));
+ assertEquals("/ui", HopLoginPage.sanitizeRedirect("\\\\evil.example", ""));
+ assertEquals("/ui", HopLoginPage.sanitizeRedirect("/ui\r\nX-Header: y",
""));
+ }
+
+ @Test
+ void sanitizeForLogReplacesControlCharacters() {
+ assertEquals("alice__forged",
HopLoginPage.sanitizeForLog("alice\r\nforged"));
+ assertEquals("bob", HopLoginPage.sanitizeForLog("bob"));
+ assertNull(HopLoginPage.sanitizeForLog(null));
}
@Test
