AVRO-4313: [java] Clarify javaAnnotation validation comments

lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java

@@ -148,7 +148,7 @@
   private static final String FILE_HEADER = "/*\n * Autogenerated by Avro\n *\n * DO NOT EDIT DIRECTLY\n */\n";
 
   public SpecificCompiler(Protocol protocol) {
     this();
     // enqueue all types
     for (Schema s : protocol.getTypes()) {
       enqueue(s);
@@ -161,7 +161,7 @@
   }
 
   public SpecificCompiler(Collection<Schema> schemas) {
     this();
     for (Schema schema : schemas) {
       enqueue(schema);
     }
@@ -182,7 +182,7 @@
     this.templateDir = System.getProperty("org.apache.avro.specific.templates",
         "/org/apache/avro/compiler/specific/templates/java/classic/");
     initializeVelocity();
     initializeSpecificData();
   }
 
   /**
@@ -427,7 +427,7 @@
   }
 
   private void initializeSpecificData() {
     addLogicalTypeConversions(specificData);
     specificData.addLogicalTypeConversion(new Conversions.DecimalConversion());
   }
 
@@ -940,7 +940,7 @@
         // with error(s)
         return "void";
       }
     default:
       return javaType(schema, false);
     }
   }
@@ -1051,15 +1051,27 @@
     return new String[0];
   }
 
+  // --- Grammar used to validate a user-supplied "javaAnnotation" -------------
+  // Avro copies the javaAnnotation schema property straight into the generated
+  // Java source (for example @Deprecated or @SuppressWarnings("unchecked")).
+  // Because it is emitted verbatim, we first check that the value really looks
+  // like a Java annotation and nothing more. If this check is too loose, a
+  // crafted value could smuggle extra Java code into the output (AVRO-4313).
+  // The patterns below build up that check: an identifier, an optional
+  // parameter list, and the literal values allowed inside it.
   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);
-  // A string literal is a quote, a body of escape sequences or characters that
-  // are not a quote, backslash or line terminator, and a closing quote. The body
-  // must not be able to contain an unescaped quote, otherwise a single literal
-  // could span past the intended closing quote and swallow surrounding tokens.
-  // Line terminators (CR, LF, NEL, LS, PS) are excluded so a value cannot break
-  // across lines in the generated source.
+  // Matches a Java string literal such as "unchecked", used when validating a
+  // user-supplied javaAnnotation before it is copied verbatim into generated
+  // source. A literal is an opening quote, a body, and a closing quote. The body
+  // may only contain:
+  // - a known escape sequence: \\ \" \n \t \f \b
+  // - any other character that is NOT a quote, backslash, or line break
+  // Forbidding an unescaped quote in the body is the key point: otherwise a
+  // single "literal" could run past its closing quote and swallow the code that
+  // follows it (see AVRO-4313). Line breaks (CR, LF, NEL, LS, PS) are forbidden
+  // too, so a value cannot spread onto extra lines in the generated file.
   private static final String PATTERN_STRING = "\"(?:\\\\[\\\\\"ntfb]|[^\"\\\\\\r\\n\\x85\\x{2028}\\x{2029}])*\"";
   private static final String PATTERN_NUMBER = "(?:\\((?:byte|char|short|int|long|float|double)\\))?[x0-9_.]*[fl]?";
   private static final String PATTERN_LITERAL_VALUE = String.format("(?:%s|%s|true|false)", PATTERN_STRING,

lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java

@@ -1033,10 +1033,14 @@ void docsAreEscaped_avro4053() {
 
   @Test
   void annotationCannotBreakOutViaStringLiteral() {
-    // A crafted javaAnnotation value tries to terminate the first annotation,
-    // inject arbitrary declarations plus a static initializer, then reopen a
-    // second valid annotation. It relies on a string literal spanning past its
-    // intended closing quote. Such values must be rejected, not emitted verbatim.
+    // Security regression test for AVRO-4313.
+    //
+    // The first javaAnnotation below is an attack. It uses an unescaped quote to
+    // "close" the annotation early, then sneaks in real Java code
+    // ... static { System.exit(1); } ...
+    // before reopening another annotation. If validation is too loose this code
+    // gets written straight into the generated .java file and runs when the
+    // class is loaded. The compiler must reject it instead of copying it out.
     String jsonSchema = "{\n" + "  \"type\": \"record\",\n" + "  \"name\": \"Injected\",\n"
         + "  \"javaAnnotation\": [\n"
         + "    \"java.lang.SuppressWarnings(\\\"x\\\") static { System.exit(1); } @java.lang.SuppressWarnings(\\\"y\\\")\",\n"
@@ -1046,15 +1050,16 @@ void annotationCannotBreakOutViaStringLiteral() {
         .compile();
     boolean validAnnotationEmitted = false;
     for (SpecificCompiler.OutputFile outputFile : outputs) {
-      // The payload is echoed (safely escaped) inside the SCHEMA$ string constant,
-      // so we must distinguish that from a verbatim emission as code. Real injected
-      // code would carry unescaped quotes; the schema literal escapes them as \".
-      // The injection must be absent from every generated file.
+      // The schema is also written into the generated file, inside the SCHEMA$
+      // string constant, so the attack text does appear there - but safely
+      // escaped (every " becomes \"). We only fail if it shows up as real code,
+      // i.e. with the original unescaped quotes.
       assertFalse(outputFile.contents.contains("SuppressWarnings(\"x\") static { System.exit(1); }"),
           "Code injection present? " + outputFile.contents);
       validAnnotationEmitted |= outputFile.contents.contains("@SuppressWarnings(\"unchecked\")");
     }
-    // The legitimate annotation in the same list must still be emitted somewhere.
+    // A normal annotation sitting next to the attack must still come through, so
+    // we know the fix rejects only the bad value, not every annotation.
     assertTrue(validAnnotationEmitted, "Valid annotation missing from generated output");
   }