diff --git a/changelog.d/evaluate-condition-fail-closed.fixed.md b/changelog.d/evaluate-condition-fail-closed.fixed.md new file mode 100644 index 000000000..d173dead0 --- /dev/null +++ b/changelog.d/evaluate-condition-fail-closed.fixed.md @@ -0,0 +1 @@ +- Unevaluable validation `condition` / `unless` expressions now throw `Wheels.InvalidValidationCondition` even when `showErrorInformation` is false, instead of silently skipping the validation (#3398) diff --git a/vendor/wheels/model/validations.cfc b/vendor/wheels/model/validations.cfc index b1dffdeba..054af72a0 100644 --- a/vendor/wheels/model/validations.cfc +++ b/vendor/wheels/model/validations.cfc @@ -580,19 +580,14 @@ component { try { local[local.key] = $evaluateConditionString(arguments[local.item]); } catch (any e) { - if ($get("showErrorInformation")) { - Throw( - type = "Wheels.InvalidValidationCondition", - message = "The `#local.item#` expression `#arguments[local.item]#` could not be evaluated: #e.message#", - extendedInfo = "Supported forms: `this.property`, `this.method()` (with named `key='val'` or positional `'val'` arguments), bare `method()` (optionally negated with `!`), and binary comparisons using eq/neq/lt/lte/gt/gte or ==/!=//>=." - ); - } - cflog( - text = "Wheels: validation `#local.item#` expression `#arguments[local.item]#` could not be evaluated (#e.message#); validation skipped.", - type = "error", - file = "wheels-errors" + // Fail closed regardless of showErrorInformation. Returning + // false here means "skip this validation" ($validate), which + // is fail-open for a broken condition/unless expression. + Throw( + type = "Wheels.InvalidValidationCondition", + message = "The `#local.item#` expression `#arguments[local.item]#` could not be evaluated: #e.message#", + extendedInfo = "Supported forms: `this.property`, `this.method()` (with named `key='val'` or positional `'val'` arguments), bare `method()` (optionally negated with `!`), and binary comparisons using eq/neq/lt/lte/gt/gte or ==/!=//>=." ); - return false; } } } diff --git a/vendor/wheels/tests/specs/model/validationsSpec.cfc b/vendor/wheels/tests/specs/model/validationsSpec.cfc index 1d9684313..bb294acb4 100644 --- a/vendor/wheels/tests/specs/model/validationsSpec.cfc +++ b/vendor/wheels/tests/specs/model/validationsSpec.cfc @@ -310,13 +310,34 @@ component extends="wheels.WheelsTest" { expect(callValid).toThrow("Wheels.InvalidValidationCondition") }) - it("skips the validation without throwing in production when a condition cannot be evaluated", () => { + // M1: $evaluateCondition must fail closed when the expression cannot + // be evaluated. Returning false here means "skip this validation" + // ($validate: if ($evaluateCondition(...))), which is fail-open. + it("throws when a condition cannot be evaluated even if showErrorInformation is false", () => { args.condition = "noSuchMethod()" user.validatesLengthOf(argumentCollection = args) var originalShowErrorInformation = application.wheels.showErrorInformation try { application.wheels.showErrorInformation = false - expect(user.valid()).toBeTrue() + var callValid = () => { + user.valid() + } + expect(callValid).toThrow("Wheels.InvalidValidationCondition") + } finally { + application.wheels.showErrorInformation = originalShowErrorInformation + } + }) + + it("throws when an unless expression cannot be evaluated even if showErrorInformation is false", () => { + args.unless = "noSuchMethod()" + user.validatesLengthOf(argumentCollection = args) + var originalShowErrorInformation = application.wheels.showErrorInformation + try { + application.wheels.showErrorInformation = false + var callValid = () => { + user.valid() + } + expect(callValid).toThrow("Wheels.InvalidValidationCondition") } finally { application.wheels.showErrorInformation = originalShowErrorInformation }