fix(rest-api): address PR review — return 500 on JSP failure and enforce READ permission in rules include (#34888)

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) <[email protected]>

Author: dsolistorres

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("<div style='padding:30px;'>");
-			sw.append("unable to parse: <a href='" + path + "' target='debug'>" + path + "</a>");
-			sw.append("<hr>");
-			sw.append("<pre style='width:90%;overflow:hidden;white-space:pre-wrap'>");
-			sw.append(e.toString());
-
-			sw.append("</pre>");
-			sw.append("</div>");
-			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());
 		}
 
 	}

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()
+        );
+    }
 %>
 
 

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());
+        }
     }
 }