diff --git a/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java b/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java index d6cd4ff7ea8..21be7718389 100644 --- a/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java +++ b/dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java @@ -121,19 +121,22 @@ private String getJspResponse ( HttpServletRequest request, HttpServletResponse } catch (WebApplicationException e) { throw e; } catch (Exception e) { - 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(); - + // 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; + } + } + // 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 26406eab798..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,9 +1,12 @@ <%@ 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"%> <%@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 +15,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 +23,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,12 +45,23 @@ } 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)) .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 2102cf40793..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; @@ -107,32 +109,97 @@ public void getJspResponse_propagates404WhenJspThrowsWebApplicationException() } // ------------------------------------------------------------------------- - // Ordinary exceptions → error HTML (existing behaviour preserved) + // ServletException-wrapped WebApplicationException (real Jasper behaviour) // ------------------------------------------------------------------------- @Test - public void getJspResponse_returnsErrorHtmlForGenericException() throws Exception { - doThrow(new IOException("disk full")).when(mockDispatcher).include(any(), any()); + 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 ServletException wrapped = + new 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()); + } + } - final Object result = getJspResponse.invoke( - portlet, mockRequest, mockResponse, "rules", "include"); + @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 ServletException middle = + new ServletException("jsp failed", root); + final RuntimeException outer = new RuntimeException("outer", middle); + doThrow(outer).when(mockDispatcher).include(any(), any()); - 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 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()); + } } + // ------------------------------------------------------------------------- + // Non-WebApplicationException failures → WebApplicationException(500) + // (no debug HTML, no HTTP 200, no internal details leaked in the response) + // ------------------------------------------------------------------------- + @Test - public void getJspResponse_returnsErrorHtmlForRuntimeException() throws Exception { + public void getJspResponse_throws500ForGenericException() throws Exception { + doThrow(new IOException("disk full")).when(mockDispatcher).include(any(), any()); + + 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_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()); + } } }