From 99754077ec135106fb48c4ec7de3757676e7c07c Mon Sep 17 00:00:00 2001 From: "daniel.solis" Date: Wed, 29 Apr 2026 07:51:48 -0600 Subject: [PATCH 1/2] fix(rest-api): unwrap WebApplicationException from JasperException in rules include endpoint (#34888) Tomcat/Jasper wraps any Throwable raised inside a JSP in a ServletException before it leaves RequestDispatcher.include(), so the dedicated catch(WebApplicationException) added in #35337 never matched at runtime and /api/portlet/rules/include kept returning HTTP 200 with debug HTML instead of the expected 400/404. The generic catch in BaseRestPortlet.getJspResponse() now walks the cause chain and re-throws the WebApplicationException so JAX-RS can map the proper status. Added unit tests that simulate the real Jasper wrapping (the existing test mocked the dispatcher to throw the exception directly, bypassing the wrapper). Also in include.jsp: - Validate the id parameter against UUIDUtil.isUUID before hitting the API, so an invalid format returns 400 instead of 404. - Add DEBUG logging on every rejection branch; only log the id value once it has passed UUID validation, to avoid CWE-117 log injection from the unvalidated query parameter. Refs: #34888 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../java/com/dotcms/rest/BaseRestPortlet.java | 10 ++++ .../main/webapp/WEB-INF/jsp/rules/include.jsp | 14 +++++ .../com/dotcms/rest/BaseRestPortletTest.java | 51 +++++++++++++++++++ 3 files changed, 75 insertions(+) diff --git a/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java b/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java index d6cd4ff7ea8..0aadf103c19 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java +++ b/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java @@ -121,6 +121,16 @@ private String getJspResponse ( HttpServletRequest request, HttpServletResponse } catch (WebApplicationException e) { throw e; } catch (Exception e) { + // Servlet containers (Jasper) wrap any Throwable raised inside a JSP in a + // ServletException/JasperException, so a WebApplicationException thrown by + // the JSP arrives here as a *cause* — walk the chain and re-throw it so + // JAX-RS maps the proper HTTP status (e.g. 400/404) instead of returning + // a 200 with debug HTML. + for (Throwable t = e.getCause(); t != null; t = t.getCause()) { + if (t instanceof WebApplicationException) { + throw (WebApplicationException) t; + } + } Logger.debug(this.getClass(), "unable to parse: " + path); Logger.error( this.getClass(), e.toString(), e ); StringWriter sw = new StringWriter(); diff --git a/dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp b/dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp index 26406eab798..cabbf28af38 100644 --- a/dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp +++ b/dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp @@ -4,6 +4,8 @@ <%@ page import="com.dotmarketing.util.PortletURLUtil" %> <%@page import="com.dotmarketing.portlets.contentlet.model.Contentlet"%> <%@page import="com.dotmarketing.util.UtilMethods"%> +<%@page import="com.dotmarketing.util.UUIDUtil"%> +<%@page import="com.dotmarketing.util.Logger"%> <%@page import="com.liferay.util.Xss"%> <%@page import="javax.ws.rs.WebApplicationException"%> <%@page import="javax.ws.rs.core.Response"%> @@ -12,6 +14,7 @@ <% final String id = request.getParameter("id"); if (!UtilMethods.isSet(id)) { + Logger.debug(this, "rules/include called with missing or empty 'id' parameter"); throw new WebApplicationException( Response.status(Response.Status.BAD_REQUEST) .entity("Missing or empty required parameter: id") @@ -19,10 +22,20 @@ ); } + if (!UUIDUtil.isUUID(id)) { + Logger.debug(this, "rules/include called with invalid id format"); + throw new WebApplicationException( + Response.status(Response.Status.BAD_REQUEST) + .entity("Invalid id format: " + Xss.encodeForHTML(id)) + .build() + ); + } + Contentlet contentlet; try { contentlet = APILocator.getContentletAPI().findContentletByIdentifierAnyLanguage(id); } catch (final Exception e) { + Logger.debug(this, "rules/include - findContentletByIdentifierAnyLanguage failed for id=" + id, e); throw new WebApplicationException( Response.status(Response.Status.BAD_REQUEST) .entity("Invalid id parameter: " + Xss.encodeForHTML(id)) @@ -31,6 +44,7 @@ } if (contentlet == null || !UtilMethods.isSet(contentlet.getIdentifier())) { + Logger.debug(this, "rules/include - no contentlet found for id=" + id); throw new WebApplicationException( Response.status(Response.Status.NOT_FOUND) .entity("No content found for id: " + Xss.encodeForHTML(id)) diff --git a/dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java b/dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java index 2102cf40793..ed29ec99a3a 100644 --- a/dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java +++ b/dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java @@ -106,6 +106,57 @@ public void getJspResponse_propagates404WhenJspThrowsWebApplicationException() } } + // ------------------------------------------------------------------------- + // ServletException-wrapped WebApplicationException (real Jasper behaviour) + // ------------------------------------------------------------------------- + + @Test + public void getJspResponse_unwrapsWebApplicationExceptionFromServletException() + throws Exception { + final WebApplicationException root = new WebApplicationException( + Response.status(Response.Status.NOT_FOUND).entity("not found").build()); + // Tomcat/Jasper wraps any throwable raised inside a JSP in a ServletException + // (specifically JasperException) before it reaches RequestDispatcher.include(). + final javax.servlet.ServletException wrapped = + new javax.servlet.ServletException("jsp failed", root); + doThrow(wrapped).when(mockDispatcher).include(any(), any()); + + try { + getJspResponse.invoke(portlet, mockRequest, mockResponse, "rules", "include"); + fail("Expected wrapped WebApplicationException to be unwrapped and re-thrown"); + } catch (final InvocationTargetException e) { + assertTrue("Cause must be WebApplicationException", + e.getCause() instanceof WebApplicationException); + assertEquals("HTTP status must be 404", + Response.Status.NOT_FOUND.getStatusCode(), + ((WebApplicationException) e.getCause()).getResponse().getStatus()); + } + } + + @Test + public void getJspResponse_unwrapsWebApplicationExceptionFromNestedCauseChain() + throws Exception { + final WebApplicationException root = new WebApplicationException( + Response.status(Response.Status.BAD_REQUEST).entity("bad id").build()); + // Real Jasper wraps the JSP throwable; wrap again to simulate any extra layer + // (e.g. Jersey or filter-level wrappers) so the unwrap walks the full chain. + final javax.servlet.ServletException middle = + new javax.servlet.ServletException("jsp failed", root); + final RuntimeException outer = new RuntimeException("outer", middle); + doThrow(outer).when(mockDispatcher).include(any(), any()); + + try { + getJspResponse.invoke(portlet, mockRequest, mockResponse, "rules", "include"); + fail("Expected nested WebApplicationException to be unwrapped and re-thrown"); + } catch (final InvocationTargetException e) { + assertTrue("Cause must be WebApplicationException", + e.getCause() instanceof WebApplicationException); + assertEquals("HTTP status must be 400", + Response.Status.BAD_REQUEST.getStatusCode(), + ((WebApplicationException) e.getCause()).getResponse().getStatus()); + } + } + // ------------------------------------------------------------------------- // Ordinary exceptions → error HTML (existing behaviour preserved) // ------------------------------------------------------------------------- From e34dcfb63b65bad218182269501bf99a2eef5a94 Mon Sep 17 00:00:00 2001 From: "daniel.solis" Date: Wed, 29 Apr 2026 13:21:20 -0600 Subject: [PATCH 2/2] =?UTF-8?q?fix(rest-api):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20return=20500=20on=20JSP=20failure=20and=20enforce?= =?UTF-8?q?=20READ=20permission=20in=20rules=20include=20(#34888)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BaseRestPortlet.getJspResponse(): - Replaced the legacy debug-HTML fallback with a WebApplicationException(500). Non-WebApplicationException failures (e.g. NullPointerException, IOException) now log the full stack trace at ERROR for admins and return a plain 500 with no entity, instead of HTTP 200 with internal exception details rendered as HTML. Side effect: also closes the pre-existing XSS surface where path and e.toString() were interpolated into the error markup unencoded. include.jsp: - Added explicit READ permission check via PermissionAPI.doesUserHavePermission after findContentletByIdentifierAnyLanguage. Anonymous sessions and users without READ on the contentlet now receive 403 with a generic "Access denied" body, instead of leaking existence via 200/404. BaseRestPortletTest: - Rewrote getJspResponse_returnsErrorHtmlForGenericException and getJspResponse_returnsErrorHtmlForRuntimeException as getJspResponse_throws500ForGenericException / getJspResponse_throws500ForRuntimeException; both assert WAE(500) propagates and getResponse().hasEntity() == false to lock in the no-detail-leak contract. - Replaced fully qualified javax.servlet.ServletException with the imported simple name for consistency with the rest of the file. Refs: #34888 Co-Authored-By: Claude Opus 4.7 (1M context) --- .../java/com/dotcms/rest/BaseRestPortlet.java | 19 ++----- .../main/webapp/WEB-INF/jsp/rules/include.jsp | 11 ++++ .../com/dotcms/rest/BaseRestPortletTest.java | 56 ++++++++++++------- 3 files changed, 53 insertions(+), 33 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java b/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java index 0aadf103c19..21be7718389 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java +++ b/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java @@ -131,19 +131,12 @@ private String getJspResponse ( HttpServletRequest request, HttpServletResponse throw (WebApplicationException) t; } } - Logger.debug(this.getClass(), "unable to parse: " + path); - Logger.error( this.getClass(), e.toString(), e ); - StringWriter sw = new StringWriter(); - sw.append("
"); - sw.append("unable to parse: " + path + ""); - sw.append("
"); - sw.append("
");
-			sw.append(e.toString());
-
-			sw.append("
"); - sw.append("
"); - return sw.toString(); - + // For any other failure, log the cause so admins/devs can investigate + // and return a plain 500 — never leak internal exception details in + // the response body, and never return HTTP 200 for a failed render. + Logger.error(this.getClass(), "Failed to render JSP: " + path, e); + throw new WebApplicationException( + Response.status(Response.Status.INTERNAL_SERVER_ERROR).build()); } } diff --git a/dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp b/dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp index cabbf28af38..b6e639ac70e 100644 --- a/dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp +++ b/dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp @@ -1,5 +1,6 @@ <%@ page import="com.dotmarketing.util.Config" %> <%@page import="com.dotmarketing.business.APILocator"%> +<%@page import="com.dotmarketing.business.PermissionAPI"%> <%@page import="com.dotcms.repackage.org.apache.struts.Globals"%> <%@ page import="com.dotmarketing.util.PortletURLUtil" %> <%@page import="com.dotmarketing.portlets.contentlet.model.Contentlet"%> @@ -51,6 +52,16 @@ .build() ); } + + if (user == null || !APILocator.getPermissionAPI().doesUserHavePermission( + contentlet, PermissionAPI.PERMISSION_READ, user, false)) { + Logger.debug(this, "rules/include - user lacks READ on id=" + id); + throw new WebApplicationException( + Response.status(Response.Status.FORBIDDEN) + .entity("Access denied") + .build() + ); + } %> diff --git a/dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java b/dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java index ed29ec99a3a..5ec08c1b54c 100644 --- a/dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java +++ b/dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java @@ -1,6 +1,7 @@ package com.dotcms.rest; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; @@ -14,6 +15,7 @@ import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; import javax.servlet.RequestDispatcher; +import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import javax.ws.rs.WebApplicationException; @@ -117,8 +119,8 @@ public void getJspResponse_unwrapsWebApplicationExceptionFromServletException() Response.status(Response.Status.NOT_FOUND).entity("not found").build()); // Tomcat/Jasper wraps any throwable raised inside a JSP in a ServletException // (specifically JasperException) before it reaches RequestDispatcher.include(). - final javax.servlet.ServletException wrapped = - new javax.servlet.ServletException("jsp failed", root); + final ServletException wrapped = + new ServletException("jsp failed", root); doThrow(wrapped).when(mockDispatcher).include(any(), any()); try { @@ -140,8 +142,8 @@ public void getJspResponse_unwrapsWebApplicationExceptionFromNestedCauseChain() Response.status(Response.Status.BAD_REQUEST).entity("bad id").build()); // Real Jasper wraps the JSP throwable; wrap again to simulate any extra layer // (e.g. Jersey or filter-level wrappers) so the unwrap walks the full chain. - final javax.servlet.ServletException middle = - new javax.servlet.ServletException("jsp failed", root); + final ServletException middle = + new ServletException("jsp failed", root); final RuntimeException outer = new RuntimeException("outer", middle); doThrow(outer).when(mockDispatcher).include(any(), any()); @@ -158,32 +160,46 @@ public void getJspResponse_unwrapsWebApplicationExceptionFromNestedCauseChain() } // ------------------------------------------------------------------------- - // Ordinary exceptions → error HTML (existing behaviour preserved) + // Non-WebApplicationException failures → WebApplicationException(500) + // (no debug HTML, no HTTP 200, no internal details leaked in the response) // ------------------------------------------------------------------------- @Test - public void getJspResponse_returnsErrorHtmlForGenericException() throws Exception { + public void getJspResponse_throws500ForGenericException() throws Exception { doThrow(new IOException("disk full")).when(mockDispatcher).include(any(), any()); - final Object result = getJspResponse.invoke( - portlet, mockRequest, mockResponse, "rules", "include"); - - assertTrue("Result must be a String", result instanceof String); - final String html = (String) result; - assertTrue("Error HTML must mention the JSP path", html.contains("rules")); - assertTrue("Error HTML must include the exception message", html.contains("disk full")); + try { + getJspResponse.invoke(portlet, mockRequest, mockResponse, "rules", "include"); + fail("Expected WebApplicationException(500) for non-WAE failures"); + } catch (final InvocationTargetException e) { + assertTrue("Cause must be WebApplicationException", + e.getCause() instanceof WebApplicationException); + final WebApplicationException wae = (WebApplicationException) e.getCause(); + assertEquals("HTTP status must be 500", + Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), + wae.getResponse().getStatus()); + assertFalse("Response body must not leak the internal exception message", + wae.getResponse().hasEntity()); + } } @Test - public void getJspResponse_returnsErrorHtmlForRuntimeException() throws Exception { + public void getJspResponse_throws500ForRuntimeException() throws Exception { doThrow(new IllegalArgumentException("bad uuid")) .when(mockDispatcher).include(any(), any()); - final Object result = getJspResponse.invoke( - portlet, mockRequest, mockResponse, "rules", "include"); - - assertTrue("Result must be a String", result instanceof String); - assertTrue("Error HTML must include the exception message", - ((String) result).contains("bad uuid")); + try { + getJspResponse.invoke(portlet, mockRequest, mockResponse, "rules", "include"); + fail("Expected WebApplicationException(500) for non-WAE failures"); + } catch (final InvocationTargetException e) { + assertTrue("Cause must be WebApplicationException", + e.getCause() instanceof WebApplicationException); + final WebApplicationException wae = (WebApplicationException) e.getCause(); + assertEquals("HTTP status must be 500", + Response.Status.INTERNAL_SERVER_ERROR.getStatusCode(), + wae.getResponse().getStatus()); + assertFalse("Response body must not leak the internal exception message", + wae.getResponse().hasEntity()); + } } }