fix(rest-api): unwrap WebApplicationException from JasperException in rules include endpoint

[dsolistorres]

Summary

Fixes #34888 — follow-up to #35337, which only partially closed the bug.

/api/portlet/rules/include was still returning HTTP 200 with unable to parse: … debug HTML instead of the expected 400 / 404 for missing/invalid/non-existent id values, even after #35337 merged. Verified on main.

Root cause

When a JSP raises any Throwable, Tomcat/Jasper wraps it in a JasperException (a ServletException) before it leaves RequestDispatcher.include(). So in BaseRestPortlet.getJspResponse() the dedicated catch (WebApplicationException e) { throw e; } block added in #35337 never matched at runtime — the exception arrived as a ServletException whose cause was the WAE. Control fell through to catch (Exception e), which builds the legacy "unable to parse: …" HTML and returns it as Response.ok(..., "text/html").

The unit test in #35337 mocked RequestDispatcher.include() to throw WebApplicationException directly, bypassing the Jasper wrapping — so it passed despite the runtime bug, giving a false green.

Fix

• BaseRestPortlet.getJspResponse() — inside the existing catch (Exception e) block, walk the cause chain and re-throw if any cause is a WebApplicationException. JAX-RS then maps the proper HTTP status from the response carried by the exception.
• BaseRestPortletTest — two new regression tests that simulate the real container behavior: a ServletException whose cause is a WebApplicationException, and a deeper nested chain.
• include.jsp — additional QA hardening on top of the propagation fix:
  • Validate id against UUIDUtil.isUUID(id) before the API call. Now ?id=not-a-valid-uuid returns 400 instead of 404, while both 32-hex and dashed-UUID formats are accepted.
  • Add Logger.debug(...) lines on each rejection branch. To avoid CWE-117 (log injection) from an unvalidated query parameter, the invalid-format branch logs the rejection without the value; the API-failure and not-found branches include the value because at that point it has already passed UUID validation.

Test plan

• GET /api/portlet/rules/include (no id) → 400 ✅
• GET /api/portlet/rules/include?id= → 400 ✅
• GET /api/portlet/rules/include?id=not-a-valid-uuid → 400 ✅
• GET /api/portlet/rules/include?id=00000000-0000-0000-0000-000000000000 → 404 ✅
• GET /api/portlet/rules/include?id=00000000000000000000000000000000 → 404 ✅
• GET /api/portlet/rules/include?id=<valid Site or HTML Page id> → 200 with iframe HTML ✅
• ./mvnw test -pl :dotcms-core -Dtest=BaseRestPortletTest passes (4 existing + 2 new tests) ✅
• Verified end-to-end via local Docker (just dev-run) and through the Rules tab on a Site contentlet

🤖 Generated with Claude Code

[claude]

Claude finished @dsolistorres's task in 5m 43s —— View job

---

🔍 dotCMS Backend Review

Tasks

• Load dotCMS conventions
• Get Java diff
• Run specialized sub-agent reviews (security, database, Java standards, REST API)
• Consolidate findings and post review

---

[🟠 High] dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp:47-64

Resource existence is disclosed before the authorization check. findContentletByIdentifierAnyLanguage is a privileged call that bypasses permissions; the JSP then returns 404 when the contentlet is not found, and only returns 403 after confirming it exists. Any backend-authenticated user with portlet access can enumerate valid content UUIDs by observing the difference between 404 and 403 responses.

// 404 is returned here — before permissions are evaluated
if (contentlet == null || !UtilMethods.isSet(contentlet.getIdentifier())) {
    throw new WebApplicationException(Response.status(Response.Status.NOT_FOUND)...);
}
// Permission check comes AFTER existence check
if (!APILocator.getPermissionAPI().doesUserHavePermission(...)) {
    throw new WebApplicationException(Response.status(Response.Status.FORBIDDEN)...);
}

💡 Use a permission-aware API call (e.g. contentletAPI.findContentletByIdentifier(id, user, lang)) which throws DotSecurityException on denied access, then map that to 403. This collapses the two branches so the caller cannot infer existence from the status code.

---

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java:81

e.printStackTrace() remains in render(), a method in the same file that was modified by this PR. dotCMS standards require Logger for all diagnostic output; System.err output is invisible to the log management system and violates the progressive-enhancement rule that applies when a file is touched.

} catch (ServletException e) {
    e.printStackTrace();   // ❌ must be Logger.error(...)
}

💡 Replace with Logger.error(this.getClass(), "Failed to render portlet JSP", e);

---

[🟡 Medium] dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp:79

The session locale attribute is interpolated into a JavaScript string literal without encoding. Every other dynamic value on the page uses Xss.encodeForJavaScript(), but this one does not.

var langIsoCode = '<%= request.getSession().getAttribute(Globals.LOCALE_KEY) %>';

💡 Wrap with Xss.encodeForJavaScript(String.valueOf(...)) to match the encoding used for all other JS interpolation on the page.

---

[🟡 Medium] dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java:144-148

getLayout has a @GET endpoint that sets Cache-Control: no-cache only on the happy-path response (line 187). The three early-exit error paths (500, 403, 500 at lines 167, 177, 182) return bare Response.status(N).build() with no cache-control header. A caching proxy could cache a 403 or 500 and serve it to subsequent legitimate requests.

ResponseBuilder builder = Response.status(500);
return builder.build();  // no Cache-Control header on error path

💡 Add @NoCache at the method level so the JAX-RS filter applies the header to every response including error paths.

---

Next steps

• 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push triggers a fresh review automatically

[claude]

🔍 dotCMS Backend Review

[🔴 Critical] dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java:136-145

Non-WebApplicationException errors (e.g. NullPointerException, IOException) in any JSP still follow the old code path: the method returns debug HTML from sw.toString(), which getLayout wraps as Response.ok(..., "text/html") — an HTTP 200 with internal error details in the body. The PR fixed WAE propagation but left all other exceptions returning a silent 200. The test getJspResponse_returnsErrorHtmlForGenericException explicitly asserts this behaviour, anchoring the gap in the test suite.

sw.append(e.toString());   // internal exception details
return sw.toString();      // returned as HTTP 200 by getLayout

💡 After the cause-chain loop, throw a WebApplicationException(500) instead of building the debug HTML, so callers always get a proper HTTP status. Remove or update the two "error HTML" unit tests to reflect the new contract.

---

[🟠 High] dotCMS/src/main/java/com/dotcms/rest/BaseRestPortlet.java:138,141

path (partially user-influenced via jspName from the URL) and e.toString() are interpolated into an HTML response without encoding. An attacker who can force a JSP-parse error with a crafted jspName containing HTML/JS metacharacters can inject markup into the admin-UI error page. This is a pre-existing path but it remains reachable for any exception that is not a WebApplicationException.

sw.append("unable to parse: <a href='" + path + "' target='debug'>" + path + "</a>");
sw.append(e.toString());  // may echo request-derived data; no HTML-encoding

💡 The cleanest fix is the 🔴 Critical fix above (throw 500 instead). If the debug HTML must be kept, use Encode.forHtmlAttribute(path) and Encode.forHtml(e.toString()).

---

[🟠 High] dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp:36

findContentletByIdentifierAnyLanguage takes no User argument and performs no permission check. A backend-authenticated user with no READ permission on a contentlet can probe for its existence by UUID and receive a 200 (vs. 404), leaking that the content exists. The init.jsp include only confirms a backend session is present.

// No user or permission argument — any backend session can probe
contentlet = APILocator.getContentletAPI().findContentletByIdentifierAnyLanguage(id);

💡 Resolve the user from the session and verify READ permission via PermissionAPI.doesUserHavePermission() before using the contentlet; throw 403 on denial.

---

[🟡 Medium] dotCMS/src/main/webapp/WEB-INF/jsp/rules/include.jsp:37-43

The catch block swallows all exception types — including DotSecurityException — and maps them to 400 BAD_REQUEST. A valid UUID whose owner has insufficient permissions returns the same status as a malformed request, masking real access-control failures and making it harder to distinguish input errors from permission/infra errors.

} catch (final Exception e) {
    throw new WebApplicationException(
        Response.status(Response.Status.BAD_REQUEST) ...
    );
}

💡 Catch DotSecurityException first and map to 403, then map remaining exceptions to 500. Reserve 400 for bad-input cases only.

---

[🟡 Medium] dotCMS/src/test/java/com/dotcms/rest/BaseRestPortletTest.java:120-121,143-144

javax.servlet.ServletException is referenced by its fully-qualified name inside both new test methods, while other javax.servlet.* types (e.g. RequestDispatcher) are imported at the top of the file. This is inconsistent with the rest of the class.

final javax.servlet.ServletException wrapped =
        new javax.servlet.ServletException("jsp failed", root);

💡 Add import javax.servlet.ServletException; and use the simple name.

---

Next steps

• 🔴 / 🟠 Fix locally and push — these need your judgment
• 🟡 You can ask me to handle mechanical fixes inline: @claude fix <issue description> in <File.java>
• Every new push triggers a fresh review automatically