fix(engine): always answer JSON from the generic action endpoint

.chachalog/Hv2mTn8p.md

@@ -0,0 +1,6 @@
+---
+# Allowed version bumps: patch, minor, major
+javascript-modules: patch
+---
+
+The generic action endpoint (`<page>.jsAction.do`) answers its JSON envelope whatever the caller's `accept` header, with a status describing the outcome: `400` for caller mistakes (missing header or name, malformed body, input rejected by an action's schema), `404` for an unknown action, `500` when the function throws. Plain HTTP callers (curl, server-to-server) can rely on the status; the generated client stubs discriminate on the envelope. (#707)

docs/2-guides/7-actions/README.md

@@ -68,6 +68,25 @@ On invalid input the client call rejects with an error carrying the validation `
 - The client stub POSTs to the current page URL (`<page>.jsAction.do`); calls execute with the visitor's session and permissions. **Guests can call your actions**: treat inputs as untrusted and enforce your own permission checks inside the function.
 - Requests carry a mandatory `X-JS-Action` header, which protects the endpoint against classic CSRF (HTML forms cannot set headers; cross-origin scripts would need a CORS preflight that Jahia does not grant).
 
+## Calling an action without the client stub
+
+Tests, server-to-server integrations and `curl` can call the endpoint directly. It always answers a JSON envelope — `{"data": "<devalue>"}` on success, `{"error": "…", "issues": [...]}` on failure — with a status describing the outcome:
+
+| Status | Meaning                                                                                                                              |
+| ------ | ------------------------------------------------------------------------------------------------------------------------------------ |
+| `200`  | the function returned; `data` holds its devalue-encoded result                                                                        |
+| `400`  | caller mistake: missing `X-JS-Action` header, missing `name` parameter, malformed body, or input rejected by the schema (`issues`)    |
+| `404`  | no action is registered under that name                                                                                               |
+| `500`  | the function threw, or the runtime could not complete the call                                                                        |
+
+```sh
+curl -X POST "http://localhost:8080/sites/my-site/home.jsAction.do?name=my-module%2FgetExchangeRate" \
+  -H "X-JS-Action: 1" \
+  --data-binary '[[1],{"currency":2},"EUR"]'
+```
+
+Arguments travel as a [devalue](https://github.com/sveltejs/devalue)-encoded **array** of the function's parameters, and `data` comes back devalue-encoded too — that is what preserves `Date`, `Map`, `Set` and friends across the wire.
+
 ## When to use what
 
 | Need                                                                                   | Use                                                       |

docs/adr/0008-client-callable-actions.md

@@ -30,7 +30,7 @@ Export discovery is a deliberate v1 simplification: only top-level `export const
 One engine-owned platform action, `jsAction` (`GenericActionEndpoint`), dispatches to all registered actions:
 
 - URL: `<currentPageUrl>.jsAction.do?name=<moduleName>/<exportName>` — riding the Render servlet keeps Jahia's authentication valves (calls execute as the visitor, guest included) and requires no new servlet/HTTP-whiteboard surface.
-- Request body: devalue-serialized arguments array. Response envelope: `{"data": "<devalue>"}` on success, `{"error": "...", "issues": [...]?}` on failure — always on HTTP 200, because the render servlet only writes JSON bodies for 2xx action results; the stub discriminates on the envelope.
+- Request body: devalue-serialized arguments array. Response envelope: `{"data": "<devalue>"}` on success, `{"error": "...", "issues": [...]?}` on failure — written by the endpoint itself with a status describing the outcome (200/400/404/500). The initial decision rode the render servlet's body writing, which only writes JSON for 2xx results and forced the envelope onto HTTP 200; the endpoint now completes the response on its own, so plain HTTP callers can discriminate on the status while the stub keeps discriminating on the envelope.
 - The JS adapter (library) owns all serialization: the Java endpoint pipes opaque strings and never converts structured polyglot values.
 
 ### CSRF

javascript-modules-engine-java/src/main/java/org/jahia/modules/javascript/modules/engine/actions/GenericActionEndpoint.java

@@ -45,8 +45,15 @@
  *
  * <p>Exposed as the platform action {@code jsAction}: client stubs POST a devalue-serialized arguments
  * array to {@code <pageUrl>.jsAction.do?name=<module>/<export>} and receive an envelope
- * {@code {"data": "<devalue>"}} or {@code {"error": "...", "issues": [...]?}}. The envelope always
- * travels on HTTP 200 because the render servlet only writes JSON bodies for 2xx action results.
+ * {@code {"data": "<devalue>"}} or {@code {"error": "...", "issues": [...]?}}, with a status code
+ * describing the outcome (200, 400, 404 or 500).
+ *
+ * <p>The envelope is written straight to the response rather than returned as an {@link ActionResult}:
+ * {@code Render#doAction} only serializes an action's JSON when the caller asked for it (a
+ * {@code returnContentType=json} parameter or an {@code accept} header containing
+ * {@code application/json}), and silently produces an empty body otherwise. This endpoint has no
+ * other representation to offer, so it always answers JSON; returning {@code null} tells the render
+ * servlet the response is already complete.
  *
  * <p>The {@code X-JS-Action} header is required: HTML forms cannot set custom headers and cross-origin
  * scripts would need a CORS preflight that Jahia does not grant, so the endpoint is not exploitable as
@@ -83,44 +90,90 @@ public void activate() {
     public ActionResult doExecute(HttpServletRequest request, RenderContext renderContext, Resource resource,
             JCRSessionWrapper session, Map<String, List<String>> parameters, URLResolver urlResolver)
             throws Exception {
+        HttpServletResponse response = renderContext.getResponse();
+
         if (request.getHeader(REQUIRED_HEADER) == null) {
-            return error("Missing " + REQUIRED_HEADER + " header");
+            logger.warn("Rejecting a call to the JS action endpoint without the {} header", REQUIRED_HEADER);
+            return error(response, HttpServletResponse.SC_BAD_REQUEST, "Missing " + REQUIRED_HEADER + " header");
         }
         String name = getParameter(parameters, "name");
         if (name == null) {
-            return error("Missing action name");
+            logger.warn("Rejecting a call to the JS action endpoint without an action name");
+            return error(response, HttpServletResponse.SC_BAD_REQUEST, "Missing action name");
         }
         String body = IOUtils.toString(request.getReader());
 
         return graalVMEngine.doWithContext(contextProvider -> {
             Map<String, Object> entry = contextProvider.getRegistry().get(REGISTRY_TYPE, name);
             if (entry == null || entry.get("execute") == null) {
-                return error("Unknown action: " + name);
+                logger.warn("Rejecting a call to unknown JS action '{}'", name);
+                return error(response, HttpServletResponse.SC_NOT_FOUND, "Unknown action: " + name);
             }
             JSPromise.Outcome outcome;
             try {
                 outcome = JSPromise.settle(Value.asValue(entry.get("execute")).execute(body));
             } catch (Exception e) {
                 logger.error("JS action '{}' failed to execute", name, e);
-                return error("Action execution failed");
+                return error(response, HttpServletResponse.SC_INTERNAL_SERVER_ERROR, "Action execution failed");
             }
             if (!outcome.isSettled()) {
                 logger.error("JS action '{}' returned a promise that did not settle; only microtask-based " +
                         "asynchronicity is supported on the server (no timers or async I/O)", name);
-                return error("Action did not settle synchronously");
+                return error(response, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+                        "Action did not settle synchronously");
             }
             if (outcome.isRejected()) {
-                return error(readMessage(outcome.getError()), readIssues(outcome.getError()));
+                JSONArray issues = readIssues(outcome.getError());
+                if (issues != null) {
+                    // Input rejected by the action's schema: the caller sent something invalid
+                    return error(response, HttpServletResponse.SC_BAD_REQUEST, readMessage(outcome.getError()),
+                            issues);
+                }
+                if (isBadRequest(outcome.getError())) {
+                    // Malformed request body: a caller mistake, not a server failure
+                    logger.warn("Rejecting a call to JS action '{}' with a malformed body", name);
+                    return error(response, HttpServletResponse.SC_BAD_REQUEST, readMessage(outcome.getError()));
+                }
+                // Anything else the function threw is a server-side failure, as in any other request
+                logger.error("JS action '{}' threw: {}", name, readMessage(outcome.getError()));
+                return error(response, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+                        readMessage(outcome.getError()));
             }
             Value data = outcome.getValue();
             if (data == null || data.isNull() || !data.isString()) {
                 logger.error("JS action '{}' adapter did not return a serialized string", name);
-                return error("Action returned an unexpected result");
+                return error(response, HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
+                        "Action returned an unexpected result");
             }
-            return new ActionResult(HttpServletResponse.SC_OK, null, new JSONObject().put("data", data.asString()));
+            return respond(response, HttpServletResponse.SC_OK, new JSONObject().put("data", data.asString()));
         });
     }
 
+    /**
+     * Writes the envelope to the response and returns {@code null}, which tells {@code Render#doAction}
+     * that the response is complete — see this class' javadoc for why the render servlet cannot do it.
+     */
+    private static ActionResult respond(HttpServletResponse response, int status, JSONObject json) {
+        response.setStatus(status);
+        response.setContentType("application/json; charset=UTF-8");
+        try {
+            json.write(response.getWriter());
+            response.getWriter().flush();
+        } catch (Exception e) {
+            logger.error("Failed to write the JS action response", e);
+        }
+        return null;
+    }
+
+    /** Whether the adapter flagged the rejection as a caller mistake (e.g. an unparsable body). */
+    private static boolean isBadRequest(Value errorValue) {
+        if (errorValue == null || !errorValue.hasMembers() || !errorValue.hasMember("badRequest")) {
+            return false;
+        }
+        Value flag = errorValue.getMember("badRequest");
+        return flag != null && flag.isBoolean() && flag.asBoolean();
+    }
+
     private static String readMessage(Value errorValue) {
         if (errorValue != null && errorValue.hasMembers() && errorValue.hasMember("message")) {
             Value message = errorValue.getMember("message");
@@ -146,15 +199,15 @@ private static JSONArray readIssues(Value errorValue) {
         return null;
     }
 
-    private static ActionResult error(String message) {
-        return error(message, null);
+    private static ActionResult error(HttpServletResponse response, int status, String message) {
+        return error(response, status, message, null);
     }
 
-    private static ActionResult error(String message, JSONArray issues) {
+    private static ActionResult error(HttpServletResponse response, int status, String message, JSONArray issues) {
         JSONObject json = new JSONObject().put("error", message);
         if (issues != null) {
             json.put("issues", issues);
         }
-        return new ActionResult(HttpServletResponse.SC_OK, null, json);
+        return respond(response, status, json);
     }
 }

javascript-modules-library/src/framework/actions/registerActionsModule.ts

@@ -33,7 +33,16 @@ export const __registerActionsModule = (
     server.registry.add("action", `${moduleName}/${name}`, {
       execute: (body: string) =>
         Promise.resolve()
-          .then(() => fn(...(parse(body) as unknown[])))
+          .then(() => {
+            let args: unknown[];
+            try {
+              args = parse(body) as unknown[];
+            } catch {
+              // a caller mistake, not a server failure: flagged for the endpoint to answer 400
+              throw { message: "Malformed request body", badRequest: true };
+            }
+            return fn(...args);
+          })
           .then(
             (result) => stringify(result),
             (error: unknown) => {
@@ -43,16 +52,21 @@ export const __registerActionsModule = (
               // logged here and replaced by a generic message (unexpected messages can leak
               // implementation details, node paths or permissions).
               const issues = (error as { issues?: unknown } | undefined)?.issues;
+              const badRequest = (error as { badRequest?: unknown } | undefined)?.badRequest === true;
               const deliberate =
+                badRequest ||
                 Array.isArray(issues) ||
                 error instanceof ActionError ||
                 error instanceof ActionValidationError;
-              const shaped: { message: string; issues?: string } = {
+              const shaped: { message: string; issues?: string; badRequest?: boolean } = {
                 message: deliberate ? messageOf(error) : "Action execution failed",
               };
               if (Array.isArray(issues)) {
                 shaped.issues = JSON.stringify(issues);
               }
+              if (badRequest) {
+                shaped.badRequest = true;
+              }
               if (!deliberate) {
                 console.error(`Action ${moduleName}/${name} threw: ${messageOf(error)}`);
               }

tests/cypress/e2e/ui/genericActionTest.cy.ts

@@ -11,12 +11,12 @@ const MODULE = "javascript-modules-engine-test-module";
 const callAction = (
   name: string,
   args: unknown[],
-  headers: Record<string, string> = { "X-JS-Action": "1" },
+  headers: Record<string, string> = { "X-JS-Action": "1", "accept": "application/json" },
 ) =>
   cy.request({
     method: "POST",
     url: `/cms/render/default/en${pagePath}.jsAction.do?name=${encodeURIComponent(name)}`,
-    headers: { accept: "application/json", ...headers },
+    headers,
     body: stringify(args),
     failOnStatusCode: false,
   });
@@ -70,58 +70,85 @@ describe("Actions (.action.ts files)", () => {
     });
   });
 
-  // On this branch the endpoint rides Render#doAction, which writes the envelope on HTTP 200
-  // whatever the outcome — the meaningful-status contract is introduced by the follow-up
-  // envelope PR, whose spec asserts the statuses.
-  it("surfaces ActionError messages", () => {
+  it("answers the envelope even when the caller does not ask for JSON", () => {
+    // The endpoint has no other representation to offer, so it must not depend on the accept header
+    callAction(`${MODULE}/add`, [20, 22], { "X-JS-Action": "1" }).then((response) => {
+      expect(response.status).to.eq(200);
+      expect(response.headers["content-type"]).to.contain("application/json");
+      expect(parse(response.body.data)).to.eq(42);
+    });
+  });
+
+  it("surfaces ActionError messages as a server error", () => {
     callAction(`${MODULE}/failOnPurpose`, []).then((response) => {
+      expect(response.status).to.eq(500);
       expect(response.body.error).to.contain("Intentional failure");
     });
   });
 
   it("masks unexpected error messages (guest-callable endpoint)", () => {
     callAction(`${MODULE}/failInternally`, []).then((response) => {
+      expect(response.status).to.eq(500);
       expect(response.body.error).to.not.contain("secret");
       expect(response.body.error).to.contain("Action execution failed");
     });
   });
 
-  it("reports a promise that cannot settle on the server", () => {
+  it("answers 500 on a promise that cannot settle on the server", () => {
     callAction(`${MODULE}/neverSettles`, []).then((response) => {
+      expect(response.status).to.eq(500);
       expect(response.body.error).to.contain("did not settle");
     });
   });
 
-  it("rejects a missing action name", () => {
+  it("rejects a missing action name as a caller mistake", () => {
     cy.request({
       method: "POST",
       url: `/cms/render/default/en${pagePath}.jsAction.do`,
       headers: { "X-JS-Action": "1", "accept": "application/json" },
       body: stringify([]),
       failOnStatusCode: false,
     }).then((response) => {
+      expect(response.status).to.eq(400);
       expect(response.body.error).to.contain("Missing action name");
     });
   });
 
+  it("rejects a malformed body as a caller mistake", () => {
+    cy.request({
+      method: "POST",
+      url: `/cms/render/default/en${pagePath}.jsAction.do?name=${encodeURIComponent(`${MODULE}/add`)}`,
+      headers: { "X-JS-Action": "1", "accept": "application/json" },
+      body: "not devalue at all",
+      failOnStatusCode: false,
+    }).then((response) => {
+      expect(response.status).to.eq(400);
+      expect(response.body.error).to.contain("Malformed request body");
+    });
+  });
+
   it("rejects invalid input of safe actions with validation issues", () => {
     callAction(`${MODULE}/safeDouble`, [{ n: -1 }]).then((response) => {
+      expect(response.status).to.eq(400);
       expect(response.body.error).to.contain("n must be a positive number");
       expect(response.body.issues[0].message).to.eq("n must be a positive number");
     });
     callAction(`${MODULE}/safeDouble`, [{ n: 21 }]).then((response) => {
+      expect(response.status).to.eq(200);
       expect(parse(response.body.data)).to.eq(42);
     });
   });
 
   it("requires the X-JS-Action header (CSRF hardening)", () => {
     callAction(`${MODULE}/add`, [1, 2], {}).then((response) => {
+      expect(response.status).to.eq(400);
       expect(response.body.error).to.contain("X-JS-Action");
     });
   });
 
   it("reports unknown actions", () => {
     callAction(`${MODULE}/doesNotExist`, []).then((response) => {
+      expect(response.status).to.eq(404);
       expect(response.body.error).to.contain("Unknown action");
     });
   });