BUG-1953: perform proper string validation 70/13070/1
authorRobert Varga <rovarga@cisco.com>
Thu, 18 Sep 2014 12:51:15 +0000 (14:51 +0200)
committerRobert Varga <rovarga@cisco.com>
Mon, 24 Nov 2014 13:06:38 +0000 (13:06 +0000)
Activates the use ot Patterns to enforce the string value has the format
specified by the yang file. Also fixes mutability of the exposed list,
which may have been attacked, injecting wrong strings.

Change-Id: I32d0ceb836df1f5ed04122a10115f66f75bfa1ec
Signed-off-by: Robert Varga <rovarga@cisco.com>
(cherry picked from commit 106188585a33e10cd910acb16e1d18eba77c8268)

code-generator/binding-java-api-generator/src/main/java/org/opendaylight/yangtools/sal/java/api/generator/ClassTemplate.xtend
code-generator/binding-java-api-generator/src/test/java/org/opendaylight/yangtools/sal/java/api/generator/test/CompilationTestUtils.java
code-generator/binding-java-api-generator/src/test/java/org/opendaylight/yangtools/sal/java/api/generator/test/TypedefCompilationTest.java
restconf/restconf-util/src/test/java/org/opendaylight/yangtools/restconf/utils/RestconfUtilsTest.java
restconf/restconf-util/src/test/resources/topology.xml

index e7400f9ff6865d0f78bfdb83af20283b0ad40920..8d20831242fc1be9149a80811227def001523ecc 100644 (file)
@@ -26,6 +26,7 @@ import org.opendaylight.yangtools.sal.binding.model.api.GeneratedProperty
 import org.opendaylight.yangtools.sal.binding.model.api.GeneratedTransferObject
 import org.opendaylight.yangtools.sal.binding.model.api.GeneratedType
 import org.opendaylight.yangtools.sal.binding.model.api.Restrictions
+import com.google.common.base.Preconditions
 
 /**
  * Template for generating JAVA class.
@@ -52,12 +53,12 @@ class ClassTemplate extends BaseTemplate {
      * List of generated types which are enclosed inside <code>genType</code>
      */
     protected val List<GeneratedType> enclosedGeneratedTypes;
-    
+
     protected val GeneratedTransferObject genTO;
 
     /**
      * Creates instance of this class with concrete <code>genType</code>.
-     * 
+     *
      * @param genType generated transfer object which will be transformed to JAVA class source code
      */
     new(GeneratedTransferObject genType) {
@@ -83,7 +84,7 @@ class ClassTemplate extends BaseTemplate {
 
     /**
      * Generates JAVA class source code (class body only).
-     * 
+     *
      * @return string with JAVA class body source code
      */
     def CharSequence generateAsInnerClass() {
@@ -96,7 +97,7 @@ class ClassTemplate extends BaseTemplate {
 
     /**
      * Template method which generates class body.
-     * 
+     *
      * @param isInnerClass boolean value which specify if generated class is|isn't inner
      * @return string with class source code in JAVA format
      */
@@ -108,11 +109,11 @@ class ClassTemplate extends BaseTemplate {
             «enumDeclarations»
             «constantsDeclarations»
             «generateFields»
-            
-            «IF restrictions != null && (!restrictions.rangeConstraints.nullOrEmpty || 
+
+            «IF restrictions != null && (!restrictions.rangeConstraints.nullOrEmpty ||
                 !restrictions.lengthConstraints.nullOrEmpty)»
             «generateConstraints»
-            
+
             «ENDIF»
             «constructors»
 
@@ -158,7 +159,7 @@ class ClassTemplate extends BaseTemplate {
 
     /**
      * Template method which generates inner classes inside this interface.
-     * 
+     *
      * @return string with the source code for inner classes in JAVA format
      */
     def protected innerClassesDeclarations() '''
@@ -244,14 +245,38 @@ class ClassTemplate extends BaseTemplate {
         «IF false == parentProperties.empty»
             super(«parentProperties.asArguments»);
         «ENDIF»
-        «FOR p : allProperties» 
+        «FOR p : allProperties»
             «generateRestrictions(type, p.fieldName.toString, p.returnType)»
         «ENDFOR»
-        «FOR p : properties» 
+
+        «/*
+         * If we have patterns, we need to apply them to the value field. This is a sad
+         * consequence of how this code is structured.
+         */
+        IF genTO.typedef && !allProperties.empty && allProperties.size == 1 && allProperties.get(0).name.equals("value")»
+
+        «Preconditions.importedName».checkNotNull(_value, "Supplied value may not be null");
+
+            «FOR c : consts»
+                «IF c.name == TypeConstants.PATTERN_CONSTANT_NAME && c.value instanceof List<?>»
+            boolean valid = false;
+            for (Pattern p : patterns) {
+                if (p.matcher(_value).matches()) {
+                    valid = true;
+                    break;
+                }
+            }
+
+            «Preconditions.importedName».checkArgument(valid, "Supplied value \"%s\" does not match any of the permitted patterns %s", _value, «TypeConstants.PATTERN_CONSTANT_NAME»);
+                «ENDIF»
+            «ENDFOR»
+        «ENDIF»
+
+        «FOR p : properties»
             this.«p.fieldName» = «p.fieldName»;
         «ENDFOR»
     }
-    
+
     '''
 
     def protected genUnionConstructor() '''
@@ -269,11 +294,13 @@ class ClassTemplate extends BaseTemplate {
         «IF false == parentProperties.empty»
             super(«parentProperties.asArguments»);
         «ENDIF»
-            «generateRestrictions(type, property.fieldName.toString, property.returnType)»
-            this.«property.fieldName» = «property.name»;
-            «FOR p : other»
+
+        «generateRestrictions(type, property.fieldName.toString, property.returnType)»
+
+        this.«property.fieldName» = «property.name»;
+        «FOR p : other»
             this.«p.fieldName» = null;
-            «ENDFOR»
+        «ENDFOR»
     }
     '''
 
@@ -287,7 +314,7 @@ class ClassTemplate extends BaseTemplate {
         «IF false == parentProperties.empty»
             super(source);
         «ENDIF»
-        «FOR p : properties» 
+        «FOR p : properties»
             this.«p.fieldName» = source.«p.fieldName»;
         «ENDFOR»
     }
@@ -310,7 +337,7 @@ class ClassTemplate extends BaseTemplate {
             «IF !("org.opendaylight.yangtools.yang.binding.InstanceIdentifier".equals(prop.returnType.fullyQualifiedName))»
             public static «genTO.name» getDefaultInstance(String defaultValue) {
                 «IF "byte[]".equals(prop.returnType.name)»
-                    «BaseEncoding.importedName» baseEncoding = «BaseEncoding.importedName».base64(); 
+                    «BaseEncoding.importedName» baseEncoding = «BaseEncoding.importedName».base64();
                     return new «genTO.name»(baseEncoding.decode(defaultValue));
                 «ELSEIF "java.lang.String".equals(prop.returnType.fullyQualifiedName)»
                     return new «genTO.name»(defaultValue);
@@ -355,7 +382,7 @@ class ClassTemplate extends BaseTemplate {
 
     /**
      * Template method which generates JAVA class declaration.
-     * 
+     *
      * @param isInnerClass boolean value which specify if generated class is|isn't inner
      * @return string with class declaration in JAVA format
      */
@@ -381,7 +408,7 @@ class ClassTemplate extends BaseTemplate {
 
     /**
      * Template method which generates JAVA enum type.
-     * 
+     *
      * @return string with inner enum source code in JAVA format
      */
     def protected enumDeclarations() '''
@@ -395,14 +422,14 @@ class ClassTemplate extends BaseTemplate {
 
     def protected suidDeclaration() '''
         «IF genTO.SUID != null»
-            private static final long serialVersionUID = «genTO.SUID.value»L; 
+            private static final long serialVersionUID = «genTO.SUID.value»L;
         «ENDIF»
     '''
 
     /**
-     * Template method wich generates JAVA constants.
-     * 
-     * @return string with constants in JAVA format 
+     * Template method which generates JAVA constants.
+     *
+     * @return string with constants in JAVA format
      */
     def protected constantsDeclarations() '''
         «IF !consts.empty»
@@ -411,8 +438,8 @@ class ClassTemplate extends BaseTemplate {
                     «val cValue = c.value»
                     «IF cValue instanceof List<?>»
                         «val cValues = cValue as List<?>»
-                        private static final «List.importedName»<«Pattern.importedName»> «Constants.MEMBER_PATTERN_LIST» = new «ArrayList.importedName»<«Pattern.importedName»>();
-                        public static final «List.importedName»<String> «TypeConstants.PATTERN_CONSTANT_NAME» = «Arrays.importedName».asList
+                        private static final «List.importedName»<«Pattern.importedName»> «Constants.MEMBER_PATTERN_LIST»;
+                        public static final «List.importedName»<String> «TypeConstants.PATTERN_CONSTANT_NAME» = «ImmutableList.importedName».of
                         FOR v : cValues SEPARATOR ", "»«
                             IF v instanceof String»"«
                                 v as String»"«
@@ -435,9 +462,12 @@ class ClassTemplate extends BaseTemplate {
      */
     def protected generateStaticInicializationBlock() '''
         static {
+            final «List.importedName»<«Pattern.importedName»> l = new «ArrayList.importedName»<«Pattern.importedName»>();
             for (String regEx : «TypeConstants.PATTERN_CONSTANT_NAME») {
-                «Constants.MEMBER_PATTERN_LIST».add(Pattern.compile(regEx));
+                l.add(Pattern.compile(regEx));
             }
+
+            «Constants.MEMBER_PATTERN_LIST» = «ImmutableList.importedName».copyOf(l);
         }
     '''
 
index d3ac8d7eec5bc0c2e0e3c127143f8fbb77751155..921aaf0d7dfbbadd5fc0cb2c34a86e3467fd1161 100644 (file)
@@ -11,13 +11,13 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
-
 import java.io.File;
 import java.io.FileNotFoundException;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
 import java.lang.reflect.ParameterizedType;
 import java.net.URI;
 import java.util.ArrayList;
@@ -53,7 +53,7 @@ public class CompilationTestUtils {
      * instead of marking it with @After annotation to prevent removing
      * generated code if test fails.
      */
-    static void cleanUp(File... resourceDirs) {
+    static void cleanUp(final File... resourceDirs) {
         for (File resourceDir : resourceDirs) {
             if (resourceDir.exists()) {
                 deleteTestDir(resourceDir);
@@ -72,7 +72,7 @@ public class CompilationTestUtils {
      *            field type
      * @return field with given name if present in class
      */
-    static Field assertContainsField(Class<?> clazz, String name, Class<?> type) {
+    static Field assertContainsField(final Class<?> clazz, final String name, final Class<?> type) {
         try {
             Field f = clazz.getDeclaredField(name);
             assertEquals(type, f.getType());
@@ -99,8 +99,8 @@ public class CompilationTestUtils {
      * @param constructorArgs
      *            constructor arguments of class to test
      */
-    static void assertContainsFieldWithValue(Class<?> clazz, String name, Class<?> returnType, Object expectedValue,
-            Class<?>... constructorArgs) {
+    static void assertContainsFieldWithValue(final Class<?> clazz, final String name, final Class<?> returnType, final Object expectedValue,
+            final Class<?>... constructorArgs) {
         Object[] initargs = null;
         if (constructorArgs != null && constructorArgs.length > 0) {
             initargs = new Object[constructorArgs.length];
@@ -127,16 +127,27 @@ public class CompilationTestUtils {
      * @param initargs
      *            array of constructor values
      */
-    static void assertContainsFieldWithValue(Class<?> clazz, String name, Class<?> returnType, Object expectedValue,
-            Class<?>[] constructorArgs, Object... initargs) {
+    static void assertContainsFieldWithValue(final Class<?> clazz, final String name, final Class<?> returnType, final Object expectedValue,
+            final Class<?>[] constructorArgs, final Object... initargs) {
         Field f = assertContainsField(clazz, name, returnType);
         f.setAccessible(true);
+
+        final Object obj;
+        if ((f.getModifiers() & Modifier.STATIC) == 0) {
+            try {
+                Constructor<?> c = clazz.getDeclaredConstructor(constructorArgs);
+                obj = c.newInstance(initargs);
+            } catch (Exception e) {
+                throw new AssertionError("Failed to instantiate object for " + clazz, e);
+            }
+        } else {
+            obj = null;
+        }
+
         try {
-            Constructor<?> c = clazz.getDeclaredConstructor(constructorArgs);
-            Object o = c.newInstance(initargs);
-            assertEquals(expectedValue, f.get(o));
-        } catch (Exception e) {
-            throw new AssertionError("Failed to perform " + name + " field test", e);
+            assertEquals(expectedValue, f.get(obj));
+        } catch (IllegalArgumentException | IllegalAccessException e) {
+            throw new AssertionError("Failed to get field " + name + " of class " + clazz, e);
         }
     }
 
@@ -148,7 +159,7 @@ public class CompilationTestUtils {
      * @param args
      *            array of argument classes
      */
-    static Constructor<?> assertContainsConstructor(Class<?> clazz, Class<?>... args) {
+    static Constructor<?> assertContainsConstructor(final Class<?> clazz, final Class<?>... args) {
         try {
             return clazz.getDeclaredConstructor(args);
         } catch (NoSuchMethodException e) {
@@ -171,7 +182,7 @@ public class CompilationTestUtils {
      *            array of parameter type classes
      * @return method with given name, return type and parameter types
      */
-    static Method assertContainsMethod(Class<?> clazz, Class<?> returnType, String name, Class<?>... args) {
+    static Method assertContainsMethod(final Class<?> clazz, final Class<?> returnType, final String name, final Class<?>... args) {
         try {
             Method m = clazz.getDeclaredMethod(name, args);
             assertEquals(returnType, m.getReturnType());
@@ -194,7 +205,7 @@ public class CompilationTestUtils {
      * @param loader
      *            current class loader
      */
-    static void assertContainsMethod(Class<?> clazz, String returnTypeStr, String name, ClassLoader loader) {
+    static void assertContainsMethod(final Class<?> clazz, final String returnTypeStr, final String name, final ClassLoader loader) {
         Class<?> returnType;
         try {
             returnType = Class.forName(returnTypeStr, true, loader);
@@ -213,7 +224,7 @@ public class CompilationTestUtils {
      * @param clazz
      *            class to test
      */
-    static void assertContainsDefaultMethods(Class<?> clazz) {
+    static void assertContainsDefaultMethods(final Class<?> clazz) {
         assertContainsMethod(clazz, Integer.TYPE, "hashCode");
         assertContainsMethod(clazz, Boolean.TYPE, "equals", Object.class);
         assertContainsMethod(clazz, String.class, "toString");
@@ -230,7 +241,7 @@ public class CompilationTestUtils {
      *            constructor arguments
      * @throws Exception
      */
-    static void assertContainsRestrictionCheck(Constructor<?> constructor, String errorMsg, Object... args)
+    static void assertContainsRestrictionCheck(final Constructor<?> constructor, final String errorMsg, final Object... args)
             throws Exception {
         try {
             constructor.newInstance(args);
@@ -255,7 +266,7 @@ public class CompilationTestUtils {
      *            constructor arguments
      * @throws Exception
      */
-    static void assertContainsRestrictionCheck(Object obj, Method method, String errorMsg, Object... args)
+    static void assertContainsRestrictionCheck(final Object obj, final Method method, final String errorMsg, final Object... args)
             throws Exception {
         try {
             method.invoke(obj, args);
@@ -275,7 +286,7 @@ public class CompilationTestUtils {
      * @param clazz
      *            class to test
      */
-    static void assertContainsGetLengthOrRange(Class<?> clazz, boolean isLength) {
+    static void assertContainsGetLengthOrRange(final Class<?> clazz, final boolean isLength) {
         try {
             Method m = clazz.getDeclaredMethod(isLength ? "length" : "range");
             java.lang.reflect.Type returnType = m.getGenericReturnType();
@@ -306,7 +317,7 @@ public class CompilationTestUtils {
      * @param ifc
      *            expected interface
      */
-    static void assertImplementsIfc(Class<?> clazz, Class<?> ifc) {
+    static void assertImplementsIfc(final Class<?> clazz, final Class<?> ifc) {
         Class<?>[] interfaces = clazz.getInterfaces();
         List<Class<?>> ifcsList = Arrays.asList(interfaces);
         if (!ifcsList.contains(ifc)) {
@@ -323,7 +334,7 @@ public class CompilationTestUtils {
      * @param genericTypeName
      *            fully qualified name of expected parameter type
      */
-    static void testAugmentation(Class<?> clazz, String genericTypeName) {
+    static void testAugmentation(final Class<?> clazz, final String genericTypeName) {
         assertImplementsParameterizedIfc(clazz, AUGMENTATION, genericTypeName);
     }
 
@@ -338,7 +349,7 @@ public class CompilationTestUtils {
      * @param genericTypeName
      *            name of generic type
      */
-    static void assertImplementsParameterizedIfc(Class<?> clazz, String ifcName, String genericTypeName) {
+    static void assertImplementsParameterizedIfc(final Class<?> clazz, final String ifcName, final String genericTypeName) {
         ParameterizedType ifcType = null;
         for (java.lang.reflect.Type ifc : clazz.getGenericInterfaces()) {
             if (ifc instanceof ParameterizedType) {
@@ -363,7 +374,7 @@ public class CompilationTestUtils {
      * @param compiledOutputDir
      *            compiler output directory
      */
-    static void testCompilation(File sourcesOutputDir, File compiledOutputDir) {
+    static void testCompilation(final File sourcesOutputDir, final File compiledOutputDir) {
         JavaCompiler compiler = ToolProvider.getSystemJavaCompiler();
         StandardJavaFileManager fileManager = compiler.getStandardFileManager(null, null, null);
         List<File> filesList = getJavaFiles(sourcesOutputDir);
@@ -381,7 +392,7 @@ public class CompilationTestUtils {
      * @param count
      *            expected count of files in directory
      */
-    static void assertFilesCount(File dir, int count) {
+    static void assertFilesCount(final File dir, final int count) {
         File[] dirContent = dir.listFiles();
         if (dirContent == null) {
             throw new AssertionError("File " + dir + " doesn't exists or it's not a directory");
@@ -397,7 +408,7 @@ public class CompilationTestUtils {
      *            directory to search
      * @return List of java files found
      */
-    private static List<File> getJavaFiles(File directory) {
+    private static List<File> getJavaFiles(final File directory) {
         List<File> result = new ArrayList<>();
         File[] filesToRead = directory.listFiles();
         if (filesToRead != null) {
@@ -415,7 +426,7 @@ public class CompilationTestUtils {
         return result;
     }
 
-    static List<File> getSourceFiles(String path) throws Exception {
+    static List<File> getSourceFiles(final String path) throws Exception {
         final URI resPath = BaseCompilationTest.class.getResource(path).toURI();
         final File sourcesDir = new File(resPath);
         if (sourcesDir.exists()) {
@@ -431,7 +442,7 @@ public class CompilationTestUtils {
         }
     }
 
-    static void deleteTestDir(File file) {
+    static void deleteTestDir(final File file) {
         if (file.isDirectory()) {
             File[] filesToDelete = file.listFiles();
             if (filesToDelete != null) {
index 124fadb6cdc2305615ebb1cf402f828c338a44ad..929f8ead287f23bc2c0360bdb50330037dbfc116 100644 (file)
@@ -26,7 +26,6 @@ import static org.opendaylight.yangtools.sal.java.api.generator.test.Compilation
 import static org.opendaylight.yangtools.sal.java.api.generator.test.CompilationTestUtils.cleanUp;
 import static org.opendaylight.yangtools.sal.java.api.generator.test.CompilationTestUtils.getSourceFiles;
 import static org.opendaylight.yangtools.sal.java.api.generator.test.CompilationTestUtils.testCompilation;
-
 import com.google.common.collect.Range;
 import java.io.File;
 import java.lang.reflect.Constructor;
@@ -205,8 +204,9 @@ public class TypedefCompilationTest extends BaseCompilationTest {
         arg = "abcd";
         expectedMsg = String.format("Invalid length: %s, expected: %s.", arg, lengthConstraints);
         assertContainsRestrictionCheck(expectedConstructor, expectedMsg, arg);
-        obj = expectedConstructor.newInstance("hello world");
-        assertEquals(obj, defInst.invoke(null, "hello world"));
+
+        obj = expectedConstructor.newInstance("abcde");
+        assertEquals(obj, defInst.invoke(null, "abcde"));
 
         // typedef string-ext2
         assertFalse(stringExt2Class.isInterface());
@@ -226,8 +226,8 @@ public class TypedefCompilationTest extends BaseCompilationTest {
         arg = "abcde";
         expectedMsg = String.format("Invalid length: %s, expected: %s.", arg, lengthConstraints);
         assertContainsRestrictionCheck(expectedConstructor, expectedMsg, arg);
-        obj = expectedConstructor.newInstance("helloWorld");
-        assertEquals(obj, defInst.invoke(null, "helloWorld"));
+        obj = expectedConstructor.newInstance("abcdef");
+        assertEquals(obj, defInst.invoke(null, "abcdef"));
 
         // typedef string-ext3
         assertFalse(stringExt3Class.isInterface());
@@ -241,8 +241,8 @@ public class TypedefCompilationTest extends BaseCompilationTest {
         defInst = assertContainsMethod(stringExt3Class, stringExt3Class, "getDefaultInstance", String.class);
         // assertEquals(1, stringExt3Class.getDeclaredMethods().length);
 
-        obj = expectedConstructor.newInstance("helloWorld");
-        assertEquals(obj, defInst.invoke(null, "helloWorld"));
+        obj = expectedConstructor.newInstance("bbbbbb");
+        assertEquals(obj, defInst.invoke(null, "bbbbbb"));
 
         // typedef my-decimal-type
         assertFalse(myDecimalTypeClass.isInterface());
index 0ba364f74f21b0d84cf71d0b143907480153c21b..dc79f118c38ad079053d600af7d2c7798655aded 100644 (file)
@@ -76,7 +76,7 @@ public class RestconfUtilsTest {
                 .getAugmentation(IgpNodeAttributes1.class);
         assertNotNull(igpAttributes1);
         assertNotNull(igpAttributes1.getIsisNodeAttributes());
-        assertEquals("66", igpAttributes1.getIsisNodeAttributes().getIso().getIsoSystemId().getValue());
+        assertEquals("0000.1111.2222", igpAttributes1.getIsisNodeAttributes().getIso().getIsoSystemId().getValue());
     }
 
 }
index 47756de1346b7324ea57a06d6de9a0b8fccd31f5..ca5b499b5d394fd8d8a0a9dfe0e0428f6abdf0fc 100644 (file)
@@ -7,7 +7,7 @@
             <router-id>42.42.42.42</router-id>
             <isis-node-attributes xmlns="urn:TBD:params:xml:ns:yang:network:isis-topology">
                 <iso>
-                    <iso-system-id>66</iso-system-id>
+                    <iso-system-id>0000.1111.2222</iso-system-id>
                 </iso>
                 <ted>
                     <te-router-id-ipv4>42.42.42.42</te-router-id-ipv4>
@@ -15,4 +15,4 @@
             </isis-node-attributes>
         </igp-node-attributes>
     </node>
-</topology>
\ No newline at end of file
+</topology>