From bb22bfda90a529a89aac769077b28dd2257a8381 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 07:50:00 +0000 Subject: [PATCH] Give cfproperty and cfparam their real attribute sets CFPropertyStatement's validAttributes held cfparam's six -- name, type, default, max, min, pattern. cfproperty accepts 65. Every ORM and bean property in real code carries getter, setter, persistent, fieldtype or ormtype, and all of them counted as invalid. getValidAttributes() is public static, so consumers asking cfparser what a property may carry were handed cfparam's answer. CFParamStatement was missing MAXLENGTH, so param name="x" maxlength="5" counted as invalid too. Both sets now come from the shipped dictionaries, which agree on all three tags across cf11 and lucee5, so a hardcoded set can be right for both dialects. CFLockStatement is the one place validateAttributes is actually called, and its five match the dictionary exactly -- the mechanism was never the problem, the data was. ## Still not wired up #39 left open whether to call validateAttributes for param and property. The answer is no, and the sets above are why it can now be argued rather than guessed. validateAttributes throws and parseScript does not catch, so one unrecognised attribute aborts the whole file rather than reporting an error on the statement. These sets are pinned to dictionaries that ship with the parser and will fall behind whatever the next CF release adds -- so that abort would eventually land on valid code, in a parser whose job is to survive unfamiliar input. The wrong sets made this a much bigger hazard than it looked: enabling property validation would have rejected almost every persistent CFC ever written. cfadmin is left alone. Its set carries RETURNVARIABLE, which the lucee4.5 dictionary omits but Lucee does accept, so it is a permissive superset rather than a bug. 322 tests, ./gradlew build, CFLint's 675 against a clean build. Three of the four new tests fail with cfml.parsing/src/main stashed; the fourth pins that unknown attributes are still rejected, so it passes either way by design. Refs #39, #43 --- .../cfscript/script/CFParamStatement.java | 17 ++-- .../cfscript/script/CFPropertyStatement.java | 84 +++++++++++++++++-- .../parsing/TestCFMLFunctionStatement.java | 52 ++++++++++++ 3 files changed, 142 insertions(+), 11 deletions(-) diff --git a/cfml.parsing/src/main/java/cfml/parsing/cfscript/script/CFParamStatement.java b/cfml.parsing/src/main/java/cfml/parsing/cfscript/script/CFParamStatement.java index ea66ef1..c7cbe9c 100644 --- a/cfml.parsing/src/main/java/cfml/parsing/cfscript/script/CFParamStatement.java +++ b/cfml.parsing/src/main/java/cfml/parsing/cfscript/script/CFParamStatement.java @@ -17,21 +17,28 @@ public class CFParamStatement extends CFParsedAttributeStatement { private CFExpression paramType; private CFExpression defaultExpression; + /* + * cfparam's attributes, per the shipped dictionaries, which agree across cf11 and lucee5. + * MAXLENGTH was missing, so `param name="x" maxlength="5";` counted as invalid. + */ static { validAttributes = new HashSet(); validAttributes.add("DEFAULT"); - validAttributes.add("TYPE"); - validAttributes.add("NAME"); validAttributes.add("MAX"); + validAttributes.add("MAXLENGTH"); validAttributes.add("MIN"); + validAttributes.add("NAME"); validAttributes.add("PATTERN"); + validAttributes.add("TYPE"); } /** * The visitor fills _attributes after construction, so this used to call validateAttributes on a - * map that was always empty -- the validation never ran. Wiring it up is a behaviour change - * rather than a bug fix: validateAttributes throws, parseScript does not catch it, and one - * unrecognised attribute would abort the whole file instead of reporting an error. See #39. + * map that was always empty -- the validation never ran. It stays uncalled: validateAttributes + * throws, parseScript does not catch it, and one unrecognised attribute would abort the whole + * file instead of reporting an error on the statement. The set above is pinned to dictionaries + * that ship with the parser and will fall behind whatever the next CF release adds, so that + * failure would land on valid code. See #39. */ public CFParamStatement(org.antlr.v4.runtime.Token t, Map _attributes) { super(t, _attributes); diff --git a/cfml.parsing/src/main/java/cfml/parsing/cfscript/script/CFPropertyStatement.java b/cfml.parsing/src/main/java/cfml/parsing/cfscript/script/CFPropertyStatement.java index dcf74db..a790cbf 100644 --- a/cfml.parsing/src/main/java/cfml/parsing/cfscript/script/CFPropertyStatement.java +++ b/cfml.parsing/src/main/java/cfml/parsing/cfscript/script/CFPropertyStatement.java @@ -16,19 +16,91 @@ public class CFPropertyStatement extends CFParsedAttributeStatement { private CFExpression propertyName; private CFExpression propertyType; + /* + * cfproperty's attributes, taken from the shipped dictionaries, which agree on all 65 across + * cf11 and lucee5. This set previously held cfparam's six -- name, type, default, max, min, + * pattern -- which is not a subset of anything cfproperty accepts beyond the first three. Every + * ORM and bean property in real code carries getter, setter, persistent, fieldtype or ormtype, + * and getValidAttributes() is public, so consumers asking cfparser what a property may carry + * were told cfparam's answer. See #39. + */ static { validAttributes = new HashSet(); + validAttributes.add("CACHEUSE"); + validAttributes.add("CASCADE"); + validAttributes.add("CFC"); + validAttributes.add("COLUMN"); + validAttributes.add("CONSTRAINED"); + validAttributes.add("DBDEFAULT"); validAttributes.add("DEFAULT"); - validAttributes.add("TYPE"); + validAttributes.add("DISPLAYNAME"); + validAttributes.add("ELEMENTCOLUMN"); + validAttributes.add("ELEMENTTYPE"); + validAttributes.add("FETCH"); + validAttributes.add("FIELDTYPE"); + validAttributes.add("FKCOLUMN"); + validAttributes.add("FORMULA"); + validAttributes.add("GENERATED"); + validAttributes.add("GENERATOR"); + validAttributes.add("GETTER"); + validAttributes.add("HINT"); + validAttributes.add("INDEX"); + validAttributes.add("INDEXABLE"); + validAttributes.add("INDEXBOOST"); + validAttributes.add("INDEXFIELDNAME"); + validAttributes.add("INDEXLANGUAGE"); + validAttributes.add("INDEXSTORE"); + validAttributes.add("INDEXTOKENIZE"); + validAttributes.add("INSERT"); + validAttributes.add("INVERSE"); + validAttributes.add("INVERSEJOINCOLUMN"); + validAttributes.add("JOINCOLUMN"); + validAttributes.add("LAZY"); + validAttributes.add("LENGTH"); + validAttributes.add("LINKCATALOG"); + validAttributes.add("LINKSCHEMA"); + validAttributes.add("LINKTABLE"); + validAttributes.add("MAPPEDBY"); + validAttributes.add("MISSINGROWIGNORED"); validAttributes.add("NAME"); - validAttributes.add("MAX"); - validAttributes.add("MIN"); - validAttributes.add("PATTERN"); + validAttributes.add("NOTNULL"); + validAttributes.add("OPTIMISTICLOCK"); + validAttributes.add("ORDERBY"); + validAttributes.add("ORMTYPE"); + validAttributes.add("PARAMS"); + validAttributes.add("PERSISTENT"); + validAttributes.add("PRECISION"); + validAttributes.add("REMOTINGFETCH"); + validAttributes.add("REQUIRED"); + validAttributes.add("SCALE"); + validAttributes.add("SELECTKEY"); + validAttributes.add("SEQUENCE"); + validAttributes.add("SERIALIZABLE"); + validAttributes.add("SETTER"); + validAttributes.add("SINGULARNAME"); + validAttributes.add("SOURCE"); + validAttributes.add("SQLTYPE"); + validAttributes.add("STRUCTKEYCOLUMN"); + validAttributes.add("STRUCTKEYTYPE"); + validAttributes.add("TABLE"); + validAttributes.add("TYPE"); + validAttributes.add("UNIQUE"); + validAttributes.add("UNIQUEKEY"); + validAttributes.add("UNSAVEDVALUE"); + validAttributes.add("UPDATE"); + validAttributes.add("VALIDATE"); + validAttributes.add("VALIDATEPARAMS"); + validAttributes.add("WHERE"); } - + + /* + * validateAttributes is deliberately not called here. It throws, and parseScript does not catch, + * so one attribute this set does not know would abort the whole file rather than report an error + * on the statement. The set above is pinned to dictionaries that ship with the parser and will + * fall behind whatever the next CF release adds, so that failure would land on valid code. + */ public CFPropertyStatement(org.antlr.v4.runtime.Token t, Map _attributes) { super(t, _attributes); - // validateAttributes(t, validAttributes); } @Override diff --git a/cfml.parsing/src/test/java/cfml/parsing/TestCFMLFunctionStatement.java b/cfml.parsing/src/test/java/cfml/parsing/TestCFMLFunctionStatement.java index 2d37d71..42dfbad 100644 --- a/cfml.parsing/src/test/java/cfml/parsing/TestCFMLFunctionStatement.java +++ b/cfml.parsing/src/test/java/cfml/parsing/TestCFMLFunctionStatement.java @@ -12,6 +12,7 @@ import cfml.parsing.cfscript.script.CFIncludeStatement; import cfml.parsing.cfscript.script.CFMLFunctionStatement; import cfml.parsing.cfscript.script.CFParamStatement; +import cfml.parsing.cfscript.script.CFPropertyStatement; import cfml.parsing.cfscript.script.CFScriptStatement; import cfml.parsing.reporting.ParseException; @@ -205,6 +206,57 @@ public void testValidateAttributesStillRejectsUnknownOnes() { } } + /** + * maxlength is a cfparam attribute the allowed set did not list, so a valid param counted as + * invalid. getValidAttributes() is public, so the wrong answer was reachable from outside. + */ + @Test + public void testParamAllowsMaxLength() { + CFScriptStatement scriptStatement = parseScript("param name=\"x\" maxlength=\"5\";"); + CFParamStatement paramStatement = (CFParamStatement) scriptStatement; + paramStatement.validateAttributes(paramStatement.getToken(), CFParamStatement.getValidAttributes()); + } + + /** + * cfproperty's allowed set held cfparam's six attributes. Every one of these is a real + * cfproperty attribute and every one of them was rejected -- getter and setter are on more or + * less every accessor-generating CFC, and the ORM ones on every persistent entity. + */ + @Test + public void testPropertyAllowsItsOwnAttributes() { + CFScriptStatement scriptStatement = parseScript( + "property name=\"email\" type=\"string\" getter=\"true\" setter=\"false\" " + + "required=\"true\" persistent=\"true\" fieldtype=\"column\" ormtype=\"string\" " + + "hint=\"the address\";"); + CFPropertyStatement propertyStatement = (CFPropertyStatement) scriptStatement; + assertEquals(9, propertyStatement.getAttributes().size()); + propertyStatement.validateAttributes(propertyStatement.getToken(), CFPropertyStatement.getValidAttributes()); + } + + /** + * The two sets are genuinely different tags and must not drift back into being copies. + */ + @Test + public void testPropertyAndParamSetsAreNotTheSame() { + assertEquals(7, CFParamStatement.getValidAttributes().size()); + assertEquals(65, CFPropertyStatement.getValidAttributes().size()); + assertFalse("property must not simply carry param's attributes", + CFPropertyStatement.getValidAttributes().equals(CFParamStatement.getValidAttributes())); + } + + @Test + public void testPropertyStillRejectsUnknownAttributes() { + CFScriptStatement scriptStatement = parseScript("property name=\"x\" bogus=\"y\";"); + CFPropertyStatement propertyStatement = (CFPropertyStatement) scriptStatement; + try { + propertyStatement.validateAttributes(propertyStatement.getToken(), + CFPropertyStatement.getValidAttributes()); + fail("bogus should not have validated"); + } catch (ParseException expected) { + assertTrue(expected.getMessage(), expected.getMessage().contains("bogus")); + } + } + @Test public void testTransactionStatement() { /* need to check if this is valid in OBD/ACF */