diff --git a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java index 53675f4a01b..cad00e943f2 100644 --- a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java +++ b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java @@ -17,27 +17,7 @@ */ package org.apache.avro.compiler.specific; -import java.io.File; -import java.io.FileOutputStream; -import java.io.IOException; -import java.io.OutputStreamWriter; -import java.io.StringWriter; -import java.io.Writer; -import java.lang.reflect.InvocationTargetException; -import java.nio.file.Files; -import java.util.ArrayList; -import java.util.Collection; -import java.util.Collections; -import java.util.Comparator; -import java.util.HashMap; -import java.util.HashSet; -import java.util.LinkedHashMap; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; -import java.util.stream.Stream; +import static java.nio.charset.StandardCharsets.UTF_8; import org.apache.avro.Conversion; import org.apache.avro.Conversions; @@ -60,7 +40,28 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import static java.nio.charset.StandardCharsets.UTF_8; +import java.io.File; +import java.io.FileOutputStream; +import java.io.IOException; +import java.io.OutputStreamWriter; +import java.io.StringWriter; +import java.io.Writer; +import java.lang.reflect.InvocationTargetException; +import java.nio.file.Files; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.Comparator; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import java.util.stream.Stream; /** * Generate specific Java interfaces and classes for protocols and schemas. @@ -1004,27 +1005,43 @@ public String conversionInstance(Schema schema) { */ public String[] javaAnnotations(JsonProperties props) { final Object value = props.getObjectProp("javaAnnotation"); - if (value == null) - return new String[0]; - if (value instanceof String) + if (value instanceof String && isValidAsAnnotation((String) value)) return new String[] { value.toString() }; if (value instanceof List) { final List list = (List) value; final List annots = new ArrayList<>(list.size()); for (Object o : list) { - annots.add(o.toString()); + if (isValidAsAnnotation(o.toString())) + annots.add(o.toString()); } return annots.toArray(new String[0]); } return new String[0]; } + private static final String PATTERN_IDENTIFIER_PART = "\\p{javaJavaIdentifierStart}\\p{javaJavaIdentifierPart}*"; + private static final String PATTERN_IDENTIFIER = String.format("(?:%s(?:\\.%s)*)", PATTERN_IDENTIFIER_PART, + PATTERN_IDENTIFIER_PART); + private static final String PATTERN_STRING = "\"(?:\\\\[\\\\\"ntfb]|(? { #foreach ($symbol in ${schema.getEnumSymbols()})${this.mangle($symbol)}#if ($foreach.hasNext), #end#end ; - public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.javaEscape($schema.toString())}"); + public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.escapeForJavaString($schema.toString())}"); public static org.apache.avro.Schema getClassSchema() { return SCHEMA$; } @Override diff --git a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/fixed.vm b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/fixed.vm index dbbef6ffb12..0f305af3190 100644 --- a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/fixed.vm +++ b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/fixed.vm @@ -19,7 +19,7 @@ package $this.mangle($schema.getNamespace()); #end #if ($schema.getDoc()) -/** $schema.getDoc() */ +/** $this.escapeForJavadoc($schema.getDoc()) */ #end #foreach ($annotation in $this.javaAnnotations($schema)) @$annotation @@ -28,7 +28,7 @@ package $this.mangle($schema.getNamespace()); @org.apache.avro.specific.AvroGenerated public class ${this.mangleTypeIdentifier($schema.getName())} extends org.apache.avro.specific.SpecificFixed { private static final long serialVersionUID = ${this.fingerprint64($schema)}L; - public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.javaEscape($schema.toString())}"); + public static final org.apache.avro.Schema SCHEMA$ = new org.apache.avro.Schema.Parser().parse("${this.escapeForJavaString($schema.toString())}"); public static org.apache.avro.Schema getClassSchema() { return SCHEMA$; } public org.apache.avro.Schema getSchema() { return SCHEMA$; } diff --git a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/protocol.vm b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/protocol.vm index f9a4d1aeccd..46ac443ac8d 100644 --- a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/protocol.vm +++ b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/protocol.vm @@ -20,7 +20,7 @@ package $this.mangle($protocol.getNamespace()); #end #if ($protocol.getDoc()) -/** $protocol.getDoc() */ +/** $this.escapeForJavadoc($protocol.getDoc()) */ #end #foreach ($annotation in $this.javaAnnotations($protocol)) @$annotation @@ -37,7 +37,7 @@ public interface $this.mangleTypeIdentifier($protocol.getName()) { * $this.escapeForJavadoc($message.getDoc()) #end #foreach ($p in $message.getRequest().getFields())## -#if ($p.doc()) * @param ${this.mangle($p.name())} $p.doc() +#if ($p.doc()) * @param ${this.mangle($p.name())} $this.escapeForJavadoc($p.doc()) #end #end */ @@ -62,7 +62,7 @@ ${this.mangle($error.getFullName())}## ## Generate nested callback API #if ($protocol.getDoc()) - /** $protocol.getDoc() */ + /** $this.escapeForJavadoc($protocol.getDoc()) */ #end @org.apache.avro.specific.AvroGenerated public interface Callback extends $this.mangleTypeIdentifier($protocol.getName()) { @@ -78,7 +78,7 @@ ${this.mangle($error.getFullName())}## * $this.escapeForJavadoc($message.getDoc()) #end #foreach ($p in $message.getRequest().getFields())## -#if ($p.doc()) * @param ${this.mangle($p.name())} $p.doc() +#if ($p.doc()) * @param ${this.mangle($p.name())} $this.escapeForJavadoc($p.doc()) #end #end * @throws java.io.IOException The async call could not be completed. diff --git a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm index ec1e6c3ca7a..eef50ec7208 100755 --- a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm +++ b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm @@ -29,7 +29,7 @@ import org.apache.avro.message.SchemaStore; #if (${this.gettersReturnOptional} || ${this.createOptionalGetters})import java.util.Optional;#end #if ($schema.getDoc()) -/** $schema.getDoc() */ +/** $this.escapeForJavadoc($schema.getDoc()) */ #end #foreach ($annotation in $this.javaAnnotations($schema)) @$annotation @@ -116,7 +116,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #foreach ($field in $schema.getFields()) #if ($field.doc()) - /** $field.doc() */ + /** $this.escapeForJavadoc($field.doc()) */ #end #foreach ($annotation in $this.javaAnnotations($field)) @$annotation @@ -155,7 +155,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS /** * All-args constructor. #foreach ($field in $schema.getFields()) -#if ($field.doc()) * @param ${this.mangle($field.name())} $field.doc() +#if ($field.doc()) * @param ${this.mangle($field.name())} $this.escapeForJavadoc($field.doc()) #else * @param ${this.mangle($field.name())} The new value for ${field.name()} #end #end @@ -228,7 +228,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #if (${this.gettersReturnOptional} && (!${this.optionalGettersForNullableFieldsOnly} || ${field.schema().isNullable()})) /** * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @return The value wrapped in an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>. */ @@ -238,7 +238,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #else /** * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field. -#if ($field.doc()) * @return $field.doc() +#if ($field.doc()) * @return $this.escapeForJavadoc($field.doc()) #else * @return The value of the '${this.mangle($field.name(), $schema.isError())}' field. #end */ @@ -257,7 +257,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #if (${this.createOptionalGetters}) /** * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @return The value wrapped in an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>. */ @@ -269,7 +269,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #if ($this.createSetters) /** * Sets the value of the '${this.mangle($field.name(), $schema.isError())}' field. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @param value the value to set. */ @@ -323,7 +323,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #foreach ($field in $schema.getFields()) #if ($field.doc()) - /** $field.doc() */ + /** $this.escapeForJavadoc($field.doc()) */ #end private ${this.javaUnbox($field.schema(), false)} ${this.mangle($field.name(), $schema.isError())}; #if (${this.hasBuilder($field.schema())}) @@ -402,7 +402,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #foreach ($field in $schema.getFields()) /** * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @return The value. */ @@ -413,7 +413,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #if (${this.createOptionalGetters}) /** * Gets the value of the '${this.mangle($field.name(), $schema.isError())}' field as an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @return The value wrapped in an Optional<${this.escapeForJavadoc(${this.javaType($field.schema())})}>. */ @@ -424,7 +424,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS /** * Sets the value of the '${this.mangle($field.name(), $schema.isError())}' field. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @param value The value of '${this.mangle($field.name(), $schema.isError())}'. * @return This builder. @@ -441,7 +441,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS /** * Checks whether the '${this.mangle($field.name(), $schema.isError())}' field has been set. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @return True if the '${this.mangle($field.name(), $schema.isError())}' field has been set, false otherwise. */ @@ -452,7 +452,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS #if (${this.hasBuilder($field.schema())}) /** * Gets the Builder instance for the '${this.mangle($field.name(), $schema.isError())}' field and creates one if it doesn't exist yet. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @return This builder. */ @@ -469,7 +469,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS /** * Sets the Builder instance for the '${this.mangle($field.name(), $schema.isError())}' field -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @param value The builder instance that must be set. * @return This builder. @@ -483,7 +483,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS /** * Checks whether the '${this.mangle($field.name(), $schema.isError())}' field has an active Builder instance -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @return True if the '${this.mangle($field.name(), $schema.isError())}' field has an active Builder instance */ @@ -494,7 +494,7 @@ public class ${this.mangleTypeIdentifier($schema.getName())} extends ${this.getS /** * Clears the value of the '${this.mangle($field.name(), $schema.isError())}' field. -#if ($field.doc()) * $field.doc() +#if ($field.doc()) * $this.escapeForJavadoc($field.doc()) #end * @return This builder. */ diff --git a/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java b/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java index 94f6924035e..19d63d033c7 100644 --- a/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java +++ b/lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java @@ -26,6 +26,20 @@ import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import org.apache.avro.AvroTypeException; +import org.apache.avro.LogicalType; +import org.apache.avro.LogicalTypes; +import org.apache.avro.Protocol; +import org.apache.avro.Schema; +import org.apache.avro.SchemaBuilder; +import org.apache.avro.generic.GenericData.StringType; +import org.apache.avro.specific.SpecificData; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.io.BufferedReader; import java.io.File; import java.io.FileInputStream; @@ -37,29 +51,17 @@ import java.util.Collections; import java.util.HashSet; import java.util.List; +import java.util.Locale; +import java.util.Map; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.tools.Diagnostic; import javax.tools.DiagnosticListener; import javax.tools.JavaCompiler; import javax.tools.JavaFileObject; import javax.tools.StandardJavaFileManager; import javax.tools.ToolProvider; -import org.apache.avro.AvroTypeException; - -import java.util.Locale; -import java.util.Map; -import java.util.Set; - -import org.apache.avro.LogicalType; -import org.apache.avro.LogicalTypes; -import org.apache.avro.Schema; -import org.apache.avro.SchemaBuilder; -import org.apache.avro.generic.GenericData.StringType; -import org.apache.avro.specific.SpecificData; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; public class TestSpecificCompiler { private static final Logger LOG = LoggerFactory.getLogger(TestSpecificCompiler.class); @@ -990,4 +992,49 @@ void fieldWithUnderscore_avro3826() { assertFalse(outputFile4.contents.contains("$3")); } + @Test + void docsAreEscaped_avro4053() { + String jsonSchema = "{\n" + " \"protocol\": \"DummyProtocol\",\n" + + " \"doc\": \"*/\\nTest escaping \\n/*\",\n" + " \"types\" : [\n" + + " {\"type\": \"fixed\", \"name\": \"Hash\", \"size\": 16, \"doc\": \"*/\\nTest escaping \\n/*\"" + + "},\n" + + " {\"type\": \"enum\", \"name\": \"Status\", \"symbols\": [\"ON\", \"OFF\"], \"doc\": \"*/\\nTest escaping \\n/*\"},\n" + + " " + " {\"type\": \"record\", \"name\": \"Message\", \"fields\" : [\n" + + " {\"name\": \"content\", \"type\": \"string\", \"doc\": \"*/\\nTest escaping \\n/*\"},\n" + + " {\"name\": \"status\", \"type\": \"Status\", \"doc\": \"*/\\nTest escaping \\n/*\"},\n" + + " {\"name\": \"signature\", \"type\": \"Hash\", \"doc\": \"*/\\nTest escaping \\n/*\"}\n" + + " ]}\n" + " ],\n" + " \"messages\" : {\n" + " \"echo\": {\"request\": [" + + "{\"name\": \"msg\", \"type\": \"Message\"}" + + "], \"response\": \"Message\", \"doc\": \"*/\\nTest escaping \\n/*\"}\n" + " },\n" + + " \"javaAnnotation\": [\n" + " \"Deprecated(forRemoval = true, since = \\\"forever\\\")\",\n" + + " \"SuppressWarnings(\\\"ALL\\\")\",\n" + " \"SuppressWarnings(\\\"CodeInjection\\\")/*\",\n" + + " \" This is inside a comment as each line is prefixed with @\",\n" + + " \" and the next bit is really dangerous... */ static { System.exit(); }\"\n" + " ]\n" + "}"; + Collection outputs = new SpecificCompiler(Protocol.parse(jsonSchema)).compile(); + for (SpecificCompiler.OutputFile outputFile : outputs) { + assertFalse(outputFile.contents.contains("*/\\nTest escaping \\n/*"), "Threats present?"); + + int expectedEscapeCount = countOccurrences(Pattern.compile("Test escaping", Pattern.LITERAL), + outputFile.contents); + int escapedJavaDocCount = countOccurrences(Pattern.compile("\\*/\\s*Test escaping <threats>\\s*/\\*"), + outputFile.contents); + // noinspection RegExpRedundantEscape + int escapedDocStringCount = countOccurrences( + Pattern.compile("\\\"doc\\\":\\\"*/\\\\nTest escaping \\\\n/*\\\"", Pattern.LITERAL), + outputFile.contents); + assertEquals(expectedEscapeCount, escapedJavaDocCount + escapedDocStringCount, + "Escaped threats in " + outputFile.path); + + assertFalse(Pattern.compile("\\{ System.exit\\(\\); }(?!\\\\\")").matcher(outputFile.contents).find(), + "Code injection present? " + outputFile.contents); + } + } + + private int countOccurrences(Pattern pattern, String textToSearch) { + int count = 0; + for (Matcher matcher = pattern.matcher(textToSearch); matcher.find();) { + count++; + } + return count; + } } diff --git a/lang/java/compiler/src/test/resources/simple_record.avsc b/lang/java/compiler/src/test/resources/simple_record.avsc index d78fd17e64e..e6e7cb79564 100644 --- a/lang/java/compiler/src/test/resources/simple_record.avsc +++ b/lang/java/compiler/src/test/resources/simple_record.avsc @@ -1,8 +1,9 @@ { - "type": "record", + "type": "record", "name": "SimpleRecord", + "doc": ",*/\n hoping the compiler won't barf on strange comments\n/*", "fields" : [ {"name": "value", "type": "int"}, {"name": "nullableValue", "type": ["null","int"], "doc" : "doc"} ] -} \ No newline at end of file +} diff --git a/lang/java/ipc/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java b/lang/java/ipc/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java index 0af06b9a2b1..c9063bca09d 100644 --- a/lang/java/ipc/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java +++ b/lang/java/ipc/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java @@ -70,7 +70,7 @@ public class TestSpecificCompiler { @Test void esc() { - assertEquals("\\\"", SpecificCompiler.javaEscape("\"")); + assertEquals("\\\"", SpecificCompiler.escapeForJavaString("\"")); } @Test