Refuse on a deny rule that never answers the question

`matches` read the result of an expression as `evaluate(...) === true`, so a rule
that parsed and evaluated but answered with something other than a boolean was
neither a match nor an error. In the deny list that meant it did not deny, and the
permissive `allow: ["true"]` that ships by default then let the action through.

`deny: ["Submit order"]` is the way in. It is what somebody writes who reads the
list as labels rather than expressions, and it is a valid CEL string, so it
evaluates to "Submit order", falls out of the deny loop, and the Bot clicks the
button. Nothing was logged, because only the throwing path logged, and the rule
still sat on the Boundaries page looking as though it were in force. A bare field
reference, a ternary returning a string and a bare number all land the same way.

Treat any non-boolean answer as a broken rule and send it down the existing
fail-closed path, which denies in the deny list, does not permit in the allow list,
and logs either way. False stays a real answer: a deny list that read every false
as a denial would refuse everything.

Checked against every rule the product ships, the .env.example example and the four
Boundaries presets, over contexts with and without an element, a key and a file.
None of them changes verdict.

Author: beardthelion

server/src/computer/policy.ts

@@ -190,20 +190,38 @@ const POLICY_FUNCTIONS: Record<string, (...args: never[]) => unknown> = {
  * `onError` decides what a broken expression means, because the safe answer differs by list: a broken
  * `allow` must not permit, and a broken `deny` must not stop denying. Both are logged loudly, because
  * a policy that silently misbehaves is worse than one that visibly refuses.
+ *
+ * A rule can be broken two ways and only one of them throws. `"Submit order"` is valid CEL: it parses,
+ * it evaluates, and it answers with a string, which is not an answer to "does this rule apply". That
+ * is what somebody writes who reads the deny list as a list of labels rather than expressions, and
+ * reading it as "no match" would let the action through under the permissive allow rule that ships by
+ * default, with nothing logged and the rule still listed on the Boundaries page as though it were in
+ * force. So anything other than a boolean is a broken rule, and takes the same fail-closed path as a
+ * throw. False is a real answer and stays one; a deny list that read every false as a denial would
+ * refuse everything.
  */
 function matches(
   expression: string,
   context: PolicyContext,
   onError: boolean,
 ): boolean {
   try {
-    return (
-      evaluate(
+    const result = evaluate(
+      expression,
+      context as unknown as Record<string, unknown>,
+      POLICY_FUNCTIONS as Record<string, CallableFunction>,
+    );
+    if (typeof result === "boolean") return result;
+
+    console.error(
+      JSON.stringify({
+        type: "computer-policy-expression-error",
         expression,
-        context as unknown as Record<string, unknown>,
-        POLICY_FUNCTIONS as Record<string, CallableFunction>,
-      ) === true
+        error: `expected a true or false answer, got ${result === null ? "null" : typeof result}`,
+        treatedAs: onError,
+      }),
     );
+    return onError;
   } catch (error) {
     console.error(
       JSON.stringify({

server/tests/computer-policy.test.ts

@@ -87,6 +87,49 @@ describe("evaluateActionPolicy", () => {
     expect(decision.source).toBe("deny");
   });
 
+  // The other way a rule is broken. These parse and evaluate, so nothing throws; they simply do not
+  // answer the question that was asked, and the only safe reading of a deny rule that did not answer
+  // is that it denied. `"Submit order"` is what somebody writes who thinks the list takes labels
+  // rather than expressions, and it is a valid CEL string.
+  test.each([
+    ['"Submit order"', "a bare string, i.e. the list read as labels"],
+    ["element.name", "a bare field reference"],
+    ['contains(element.name, "submit") ? element.name : false', "a ternary"],
+    ["repeat.count", "a number"],
+  ])(
+    "a deny expression that is not a question (%s: %s) still denies",
+    (rule) => {
+      const decision = evaluateActionPolicy(
+        { ...permissive, deny: [rule] },
+        context(),
+      );
+      expect(decision.allowed).toBe(false);
+      expect(decision.source).toBe("deny");
+    },
+  );
+
+  // The mirror. A rule that does not answer must not permit either, which is what this already did by
+  // reading anything other than true as no match.
+  test("an allow expression that is not a question does not permit", () => {
+    const decision = evaluateActionPolicy(
+      { mode: "enforce", deny: [], allow: ['"Submit order"'] },
+      context(),
+    );
+    expect(decision.allowed).toBe(false);
+    expect(decision.source).toBe("default");
+  });
+
+  // A rule that answers "no" is not broken, and must not be read as one: a deny list where every
+  // false reading became a denial would refuse everything.
+  test("a deny expression that answers false permits", () => {
+    const decision = evaluateActionPolicy(
+      { ...permissive, deny: ['contains(element.name, "cancel")'] },
+      context(),
+    );
+    expect(decision.allowed).toBe(true);
+    expect(decision.source).toBe("allow");
+  });
+
   test("a broken allow expression does not permit", () => {
     const decision = evaluateActionPolicy(
       { mode: "enforce", deny: [], allow: ["also not ( valid"] },