diff --git a/server/src/computer/policy.ts b/server/src/computer/policy.ts index 06ac3a9..e5096ef 100644 --- a/server/src/computer/policy.ts +++ b/server/src/computer/policy.ts @@ -206,6 +206,15 @@ const POLICY_FUNCTIONS: Record 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, @@ -213,13 +222,22 @@ function matches( onError: boolean, ): boolean { try { - return ( - evaluate( + const result = evaluate( + expression, + context as unknown as Record, + POLICY_FUNCTIONS as Record, + ); + if (typeof result === "boolean") return result; + + console.error( + JSON.stringify({ + type: "computer-policy-expression-error", expression, - context as unknown as Record, - POLICY_FUNCTIONS as Record, - ) === true + error: `expected a true or false answer, got ${result === null ? "null" : typeof result}`, + treatedAs: onError, + }), ); + return onError; } catch (error) { console.error( JSON.stringify({ diff --git a/server/tests/computer-policy.test.ts b/server/tests/computer-policy.test.ts index c2c8efe..3ce145e 100644 --- a/server/tests/computer-policy.test.ts +++ b/server/tests/computer-policy.test.ts @@ -91,6 +91,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"] },