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 14c5320d0e Issue #8888 : Authorize Hop Web server calls on the mapped
path (#8889)
14c5320d0e is described below
commit 14c5320d0e9cbbe0b28b2f83458fd11230e37e09
Author: Matt Casters <[email protected]>
AuthorDate: Wed Oct 14 09:40:01 2026 +0200
Issue #8888 : Authorize Hop Web server calls on the mapped path (#8889)
---
THREAT_MODEL.md | 7 +-
.../HopServerEndpointPermissionMapper.java | 62 ++++++++---
.../HopServerEndpointPermissionMapperTest.java | 22 ++++
.../security/HopServerAuthorizationFilter.java | 53 +++++++---
.../security/HopServerAuthorizationFilterTest.java | 115 ++++++++++++++++++++-
5 files changed, 225 insertions(+), 34 deletions(-)
diff --git a/THREAT_MODEL.md b/THREAT_MODEL.md
index f30730655a..0641002f97 100644
--- a/THREAT_MODEL.md
+++ b/THREAT_MODEL.md
@@ -173,9 +173,10 @@ where strict outbound TLS verification is required (see
§9).
— this is the single-user / trusted-network install and the session is
treated as *unrestricted*. The co-deployed Hop Server API on `/hop/*` is
**default-denied** (`403`) by
-
[`HopServerAuthorizationFilter`](rap/src/main/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilter.java),
- because mode `NONE` has no identity to authorize against and those
endpoints
- deploy and execute pipelines/workflows. Opt-in flag
+
[`HopServerAuthorizationFilter`](rap/src/main/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilter.java).
+ The filter authorizes the container-mapped path (`getServletPath()` +
`getPathInfo()`), which
+ is the path the container has already normalized. Mode `NONE` has no
identity to authorize
+ against, and those endpoints deploy and execute pipelines/workflows.
Opt-in flag
`allowUnauthenticatedServerApi` /
`HOP_WEB_ALLOW_UNAUTHENTICATED_SERVER_API` reopens it for operators running
Hop Web purely as an execution server behind their own network controls —
diff --git
a/core/src/main/java/org/apache/hop/core/security/HopServerEndpointPermissionMapper.java
b/core/src/main/java/org/apache/hop/core/security/HopServerEndpointPermissionMapper.java
index a948cbecd1..727bd795b0 100644
---
a/core/src/main/java/org/apache/hop/core/security/HopServerEndpointPermissionMapper.java
+++
b/core/src/main/java/org/apache/hop/core/security/HopServerEndpointPermissionMapper.java
@@ -17,7 +17,9 @@
package org.apache.hop.core.security;
+import java.util.ArrayList;
import java.util.LinkedHashMap;
+import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
@@ -29,6 +31,11 @@ import java.util.concurrent.ConcurrentHashMap;
* role-based access control as the RAP UI. Lives in {@code hop-core} so it
carries no servlet
* dependency and can be unit tested in isolation; the servlet filter supplies
the request path.
*
+ * <p>Before matching, the path is normalized: a query string and per-segment
matrix parameters are
+ * removed, and {@code .} and {@code ..} are resolved. The lookup therefore
sees the same endpoint
+ * the servlet container will dispatch, including when a caller still holds a
raw request path.
+ * Percent-decoding is left to the container so a value is not decoded twice.
+ *
* <p>Matching is by longest context-path prefix, because most servlets accept
trailing path
* segments (for example {@code /hop/pipelineStatus/<name>/<id>}). Endpoints
not listed here are
* <em>unknown</em>; the filter treats unknown endpoints as default-deny so
that new servlets do not
@@ -317,27 +324,54 @@ public final class HopServerEndpointPermissionMapper {
|| requiredPermission(null, path).isPresent();
}
+ /**
+ * Query string off, matrix parameters off per segment, then {@code .} and
{@code ..} resolved.
+ * Casing is preserved: the servlet registry keys endpoints case-sensitively
(for example
+ * camelCase {@code startPipeline}), so the table keys must match the
endpoint segment exactly. A
+ * {@code ..} that would climb above the root returns {@code null}, which
the lookup denies.
+ */
private static String normalize(String path) {
if (path == null || path.isBlank()) {
return null;
}
String p = path.trim();
- // Strip a ;jsessionid= or matrix params
- int semi = p.indexOf(';');
- if (semi >= 0) {
- p = p.substring(0, semi);
+ int query = p.indexOf('?');
+ if (query >= 0) {
+ p = p.substring(0, query);
+ }
+ if (p.isEmpty()) {
+ return null;
}
- // Strip a query string if one slipped in
- int q = p.indexOf('?');
- if (q >= 0) {
- p = p.substring(0, q);
+ boolean absolute = p.startsWith("/");
+ List<String> segments = new ArrayList<>();
+ for (String raw : p.split("/", -1)) {
+ int semi = raw.indexOf(';');
+ String segment = semi >= 0 ? raw.substring(0, semi) : raw;
+ if (segment.isEmpty() || ".".equals(segment)) {
+ continue;
+ }
+ if ("..".equals(segment)) {
+ if (segments.isEmpty()) {
+ return null;
+ }
+ segments.remove(segments.size() - 1);
+ continue;
+ }
+ segments.add(segment);
+ }
+ if (segments.isEmpty()) {
+ return absolute ? "/" : null;
}
- // Drop a trailing slash (but keep a bare "/"). Casing is preserved: the
servlet registry keys
- // endpoints case-sensitively (e.g. camelCase "startPipeline"), so the
table keys must match
- // exactly on the endpoint segment.
- while (p.length() > 1 && p.endsWith("/")) {
- p = p.substring(0, p.length() - 1);
+ StringBuilder normalized = new StringBuilder();
+ if (absolute) {
+ normalized.append('/');
+ }
+ for (int i = 0; i < segments.size(); i++) {
+ if (i > 0) {
+ normalized.append('/');
+ }
+ normalized.append(segments.get(i));
}
- return p;
+ return normalized.toString();
}
}
diff --git
a/core/src/test/java/org/apache/hop/core/security/HopServerEndpointPermissionMapperTest.java
b/core/src/test/java/org/apache/hop/core/security/HopServerEndpointPermissionMapperTest.java
index c1de4b3bda..281d91a2a6 100644
---
a/core/src/test/java/org/apache/hop/core/security/HopServerEndpointPermissionMapperTest.java
+++
b/core/src/test/java/org/apache/hop/core/security/HopServerEndpointPermissionMapperTest.java
@@ -172,6 +172,28 @@ class HopServerEndpointPermissionMapperTest {
HopServerEndpointPermissionMapper.requiredPermission("/hop/startWorkflow?name=x&id=y"));
}
+ @Test
+ void dotSegmentsResolveToTheDispatchedEndpoint() {
+ assertEquals(
+ Optional.of(Permission.RUN_EXECUTE),
+
HopServerEndpointPermissionMapper.requiredPermission("/hop/status/../execPipeline"));
+ assertEquals(
+ Optional.of(Permission.RUN_EXECUTE),
+
HopServerEndpointPermissionMapper.requiredPermission("/hop/status;x=1/../execPipeline"));
+ assertEquals(
+ Optional.of(Permission.RUN_EXECUTE),
+
HopServerEndpointPermissionMapper.requiredPermission("/hop/./execPipeline"));
+ assertEquals(
+ Optional.of(Permission.RUN_EXECUTE),
+
HopServerEndpointPermissionMapper.requiredPermission("/hop//execPipeline"));
+ assertEquals(
+ Optional.of(Permission.RUN_EXECUTE),
+
HopServerEndpointPermissionMapper.requiredPermission("//hop/execPipeline"));
+ assertTrue(
+
HopServerEndpointPermissionMapper.requiredPermission("/hop/../execPipeline").isEmpty());
+
assertTrue(HopServerEndpointPermissionMapper.requiredPermission("/../execPipeline").isEmpty());
+ }
+
@Test
void unknownEndpointsAreNotKnownAndHaveNoPermission() {
assertFalse(HopServerEndpointPermissionMapper.isKnownEndpoint("/hop/somethingNew"));
diff --git
a/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilter.java
b/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilter.java
index 2f36fbb44a..aee6342403 100644
---
a/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilter.java
+++
b/rap/src/main/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilter.java
@@ -59,9 +59,15 @@ import org.apache.hop.core.security.Permission;
* the authenticated principal's roles.
* </ul>
*
+ * <p>The path used for that decision is the container-mapped path ({@code
getServletPath()} +
+ * {@code getPathInfo()}). Tomcat fills those from the normalized URI, which
is also what {@code
+ * HopServerServlet} dispatches on. The raw request URI is ignored, so a
non-canonical request line
+ * such as {@code //hop/execPipeline} cannot skip this filter or be judged as
a different endpoint.
+ *
* <p>Unknown {@code /hop/*} paths (a servlet not in the built-in table, e.g.
a third-party plugin)
* are denied by default in the authenticated modes, so a newly added servlet
cannot silently widen
- * the authenticated attack surface. It becomes reachable once its path is
added to the mapper.
+ * the authenticated attack surface. It becomes reachable once its path is
added to the mapper. A
+ * request whose mapped path is missing or not under {@code /hop} is denied as
well.
*
* <p>This filter does not authenticate; if no principal is present in an
authenticated mode the
* upstream auth filter has already redirected or challenged, so a missing
principal here is treated
@@ -90,10 +96,15 @@ public class HopServerAuthorizationFilter implements Filter
{
HopSecurityConfig config = HopSecurityConfig.load();
HopSecurityConfig.AuthMode mode = config.getAuthMode();
- String path = pathWithinApp(httpRequest);
+ String path = endpointPath(httpRequest);
if (path == null || !path.startsWith("/hop")) {
- // Not a Hop Server endpoint (filter is mapped to /hop/* but stay
defensive).
- chain.doFilter(request, response);
+ // Fail closed. The raw request URI is not a safe signal: Tomcat maps
//hop/... onto this
+ // filter and then dispatches the normalized path.
+ LOG.log(
+ Level.FINE,
+ "Denying request with no Hop Server mapping (path={0})",
+ String.valueOf(path));
+ deny(httpResponse, "Not authorized for this endpoint",
HttpServletResponse.SC_FORBIDDEN);
return;
}
@@ -189,19 +200,35 @@ public class HopServerAuthorizationFilter implements
Filter {
response.getWriter().write(message);
}
- private static String pathWithinApp(HttpServletRequest request) {
- String contextPath = request.getContextPath();
- String uri = request.getRequestURI();
- if (uri == null) {
+ /**
+ * Path the container will dispatch. {@code servletPath} and {@code
pathInfo} are already
+ * context-relative and decoded, and on Tomcat they are the normalized form
of the request URI.
+ *
+ * @return the mapped path, or {@code null} when the container did not map a
path
+ */
+ private static String endpointPath(HttpServletRequest request) {
+ String servletPath = request.getServletPath();
+ String pathInfo = request.getPathInfo();
+ boolean noServletPath = servletPath == null || servletPath.isEmpty();
+ boolean noPathInfo = pathInfo == null || pathInfo.isEmpty();
+ if (noServletPath && noPathInfo) {
return null;
}
- if (contextPath != null && !contextPath.isEmpty() &&
uri.startsWith(contextPath)) {
- uri = uri.substring(contextPath.length());
+ if (noServletPath) {
+ return pathInfo;
+ }
+ if (noPathInfo) {
+ return servletPath;
+ }
+ boolean servletSlash = servletPath.endsWith("/");
+ boolean infoSlash = pathInfo.startsWith("/");
+ if (servletSlash && infoSlash) {
+ return servletPath.substring(0, servletPath.length() - 1) + pathInfo;
}
- if (uri.isEmpty()) {
- return "/";
+ if (!servletSlash && !infoSlash) {
+ return servletPath + "/" + pathInfo;
}
- return uri;
+ return servletPath + pathInfo;
}
@Override
diff --git
a/rap/src/test/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilterTest.java
b/rap/src/test/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilterTest.java
index 219bb6cce4..b913521de5 100644
---
a/rap/src/test/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilterTest.java
+++
b/rap/src/test/java/org/apache/hop/ui/hopgui/security/HopServerAuthorizationFilterTest.java
@@ -100,8 +100,31 @@ class HopServerAuthorizationFilterTest {
.thenAnswer(inv -> roleNames.contains(inv.getArgument(0,
String.class)));
}
- private void requestPath(String uri) {
- when(request.getRequestURI()).thenReturn(uri);
+ /**
+ * Canonical mapping for a {@code /hop/*} or {@code /hop/api/v1/*} path. The
raw URI matches the
+ * mapped path. Bypass tests set the three values separately.
+ */
+ private void requestPath(String path) {
+ String servletPath = "/hop";
+ String pathInfo = null;
+ if (path != null && path.startsWith("/hop/api/v1")) {
+ servletPath = "/hop/api/v1";
+ pathInfo = path.substring("/hop/api/v1".length());
+ } else if (path != null && path.startsWith("/hop")) {
+ pathInfo = path.substring("/hop".length());
+ } else {
+ servletPath = path;
+ }
+ if (pathInfo != null && pathInfo.isEmpty()) {
+ pathInfo = null;
+ }
+ mappedRequest(path, servletPath, pathInfo);
+ }
+
+ private void mappedRequest(String rawUri, String servletPath, String
pathInfo) {
+ when(request.getRequestURI()).thenReturn(rawUri);
+ when(request.getServletPath()).thenReturn(servletPath);
+ when(request.getPathInfo()).thenReturn(pathInfo);
}
@Test
@@ -228,11 +251,95 @@ class HopServerAuthorizationFilterTest {
}
@Test
- void contextPathIsStrippedBeforeMatching() throws Exception {
+ void rawRequestUriDoesNotSelectTheEndpoint() throws Exception {
setMode(HopSecurityConfig.AuthMode.BASIC);
when(request.getContextPath()).thenReturn("/hop-web");
authenticateAs("viewer", Set.of("readonly"));
- when(request.getRequestURI()).thenReturn("/hop-web/hop/addPipeline");
+ // Context path and a non-canonical request line. The container still
mapped /hop/addPipeline.
+ mappedRequest("/hop-web//hop/addPipeline", "/hop", "/addPipeline");
+ filter.doFilter(request, response, chain);
+ verify(chain, never()).doFilter(any(), any());
+ verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN);
+ }
+
+ @Test
+ void modeNoneDeniesNonCanonicalPathsThatMapOntoAnEndpoint() throws Exception
{
+ setMode(HopSecurityConfig.AuthMode.NONE);
+ for (String rawUri :
+ new String[] {
+ "//hop/execPipeline",
+ "/./hop/execPipeline",
+ "/foo/../hop/execPipeline",
+ "/%2e/hop/execPipeline",
+ "/;/hop/execPipeline"
+ }) {
+ chain = mock(FilterChain.class);
+ response = mock(HttpServletResponse.class);
+ when(response.getWriter()).thenReturn(new PrintWriter(new
StringWriter()));
+ mappedRequest(rawUri, "/hop", "/execPipeline");
+ filter.doFilter(request, response, chain);
+ verify(chain, never()).doFilter(any(), any());
+ verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN);
+ }
+ }
+
+ @Test
+ void modeNoneOptInFollowsTheMappedPath() throws Exception {
+ setMode(HopSecurityConfig.AuthMode.NONE, true);
+ mappedRequest("//hop/execPipeline", "/hop", "/execPipeline");
+ filter.doFilter(request, response, chain);
+ verify(chain, times(1)).doFilter(request, response);
+ verify(response, never()).setStatus(anyInt());
+ }
+
+ @Test
+ void readOnlyDeniedWhenRawPathNamesAReadEndpoint() throws Exception {
+ setMode(HopSecurityConfig.AuthMode.BASIC);
+ for (String rawUri :
+ new String[] {"/hop/status/../execPipeline",
"/hop/status;x=1/../execPipeline"}) {
+ chain = mock(FilterChain.class);
+ response = mock(HttpServletResponse.class);
+ when(response.getWriter()).thenReturn(new PrintWriter(new
StringWriter()));
+ authenticateAs("viewer", Set.of("readonly"));
+ mappedRequest(rawUri, "/hop", "/execPipeline");
+ filter.doFilter(request, response, chain);
+ verify(chain, never()).doFilter(any(), any());
+ verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN);
+ }
+ }
+
+ @Test
+ void operatorAllowedWhenRawUriIsNonCanonical() throws Exception {
+ setMode(HopSecurityConfig.AuthMode.BASIC);
+ authenticateAs("operator", Set.of("operator"));
+ mappedRequest("//hop/execPipeline", "/hop", "/execPipeline");
+ filter.doFilter(request, response, chain);
+ verify(chain, times(1)).doFilter(request, response);
+ }
+
+ @Test
+ void readOnlyDeniedOnApiWriteUsingTheApiServletMapping() throws Exception {
+ setMode(HopSecurityConfig.AuthMode.BASIC);
+ authenticateAs("viewer", Set.of("readonly"));
+ when(request.getMethod()).thenReturn("POST");
+ mappedRequest("/hop/api/v1/execute/sync", "/hop/api/v1", "/execute/sync");
+ filter.doFilter(request, response, chain);
+ verify(chain, never()).doFilter(any(), any());
+ verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN);
+ }
+
+ @Test
+ void missingOrForeignMappingIsDenied() throws Exception {
+ setMode(HopSecurityConfig.AuthMode.NONE, true);
+ mappedRequest("//hop/execPipeline", null, null);
+ filter.doFilter(request, response, chain);
+ verify(chain, never()).doFilter(any(), any());
+ verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN);
+
+ chain = mock(FilterChain.class);
+ response = mock(HttpServletResponse.class);
+ when(response.getWriter()).thenReturn(new PrintWriter(new StringWriter()));
+ mappedRequest("//hop/execPipeline", "/ui", null);
filter.doFilter(request, response, chain);
verify(chain, never()).doFilter(any(), any());
verify(response).setStatus(HttpServletResponse.SC_FORBIDDEN);
