From 2e5bf4783654df5564ff779514eb3619a533fa62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isma=C3=ABl=20Mej=C3=ADa?= Date: Fri, 31 Jul 2026 21:47:02 +0200 Subject: [PATCH] AVRO-4313: [java] Clarify javaAnnotation validation comments Rewrite the comments around the annotation-validation grammar and its regression test in plainer language. Explain why the check exists (the javaAnnotation property is emitted verbatim into generated source) and what an unescaped quote in a string literal would allow, so the intent is clear to readers who are not familiar with the regex. Comment-only change; no behavior change. --- .../compiler/specific/SpecificCompiler.java | 24 ++++++++++++++----- .../specific/TestSpecificCompiler.java | 23 +++++++++++------- 2 files changed, 32 insertions(+), 15 deletions(-) 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 4cfe1eea48c..0295c142312 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 @@ -1051,15 +1051,27 @@ public String[] javaAnnotations(JsonProperties props) { 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, 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 918e28a8954..d9369fab035 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 @@ -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"); }