This is an automated email from the ASF dual-hosted git repository.
RyanSkraba pushed a commit to branch branch-1.12
in repository https://gitbox.apache.org/repos/asf/avro.git
The following commit(s) were added to refs/heads/branch-1.12 by this push:
new 5590928aa6 AVRO-4313: [java] Clarify javaAnnotation validation
comments (#3907)
5590928aa6 is described below
commit 5590928aa61b21dd59ef158dd49a5b609f078aee
Author: Ismaël Mejía <[email protected]>
AuthorDate: Sun Aug 2 12:58:43 2026 +0200
AVRO-4313: [java] Clarify javaAnnotation validation comments (#3907)
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.
---
.../avro/compiler/specific/SpecificCompiler.java | 24 ++++++++++++++++------
.../compiler/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 4cfe1eea48..0295c14231 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 class SpecificCompiler {
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 918e28a895..d9369fab03 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 @@ public class TestSpecificCompiler {
@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 @@ public class TestSpecificCompiler {
.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");
}
