AVRO-4313: [java] Tighten javaAnnotation string-literal validation in SpecificCompiler#3892
AVRO-4313: [java] Tighten javaAnnotation string-literal validation in SpecificCompiler#3892iemejia wants to merge 3 commits into
Conversation
… SpecificCompiler The string-literal grammar used to validate javaAnnotation values accepted an unescaped quote inside the literal body, letting a single literal span past its intended closing quote and absorb surrounding tokens. Constrain the body to recognized escape sequences or characters that are not a quote, backslash, or line terminator, and add a regression test.
There was a problem hiding this comment.
Pull request overview
This PR addresses AVRO-4313 by tightening validation of javaAnnotation values in the Java SpecificCompiler, preventing crafted annotation strings from abusing overly-permissive string-literal matching to inject additional Java tokens into generated sources.
Changes:
- Tighten the
PATTERN_STRINGregex used bySpecificCompilerannotation validation to disallow unescaped quotes/backslashes and line breaks in string literals. - Add a regression test that attempts to inject extra declarations via a crafted
javaAnnotationvalue and asserts it is not emitted verbatim, while a valid annotation still is.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java | Tightens the regex used to validate annotation string literals to prevent quote-based breakout/injection. |
| lang/java/compiler/src/test/java/org/apache/avro/compiler/specific/TestSpecificCompiler.java | Adds a regression test ensuring crafted javaAnnotation values can’t escape a string literal and inject code into generated output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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. | ||
| private static final String PATTERN_STRING = "\"(?:\\\\[\\\\\"ntfb]|[^\"\\\\\\r\\n])*\""; |
There was a problem hiding this comment.
Good catch — fixed in 2a0f2ad. The character class now also excludes NEL (\x85), LS (\x{2028}) and PS (\x{2029}) in addition to CR and LF, so no line terminator can appear unescaped inside the string literal. Verified that annotation values containing these characters are now rejected while legitimate annotations still validate.
Also reject NEL, LS and PS in addition to CR and LF so an annotation value cannot span multiple lines in the generated source.
| Collection<SpecificCompiler.OutputFile> outputs = new SpecificCompiler(SchemaParser.parseSingle(jsonSchema)) | ||
| .compile(); | ||
| 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 \". | ||
| assertFalse(outputFile.contents.contains("SuppressWarnings(\"x\") static { System.exit(1); }"), | ||
| "Code injection present? " + outputFile.contents); | ||
| // The legitimate annotation in the same list must still be emitted. | ||
| assertTrue(outputFile.contents.contains("@SuppressWarnings(\"unchecked\")"), | ||
| "Valid annotation missing? " + outputFile.contents); | ||
| } |
There was a problem hiding this comment.
Addressed in 90299cc. The loop now asserts the injection payload is absent from every generated file, and separately asserts the valid annotation is emitted in at least one output (via an accumulator checked after the loop), so the test no longer depends on the number of output files.
Assert the injection payload is absent from every generated file and the valid annotation is emitted in at least one, rather than requiring it in each output file.
What is the purpose of the change
The
javaAnnotationvalidator inSpecificCompileruses a regular expression(
PATTERN_STRING) whose string-literal grammar accepts an unescaped quoteinside the literal body. As a result a single string literal can extend past
its intended closing quote and absorb surrounding tokens, so a crafted
javaAnnotationvalue can full-match theIDENTIFIER(STRING)shape whileactually carrying extra Java declarations. Because the record template emits
annotation values verbatim, such a value would be written directly into the
generated specific-record source.
This tightens the string-literal grammar so the body may only contain
recognized escape sequences or characters that are not a quote, backslash, or
line terminator. Legitimate annotation values (for example
SuppressWarnings("unchecked"),Deprecated(forRemoval = true, since = "forever"),and values with escaped quotes) continue to validate.
Fixes AVRO-4313.
Verifying this change
This change added tests and can be verified as follows:
annotationCannotBreakOutViaStringLiteralinTestSpecificCompiler,which feeds a crafted
javaAnnotationvalue and asserts it is not emittedverbatim into the generated source, while a valid annotation in the same
list is still emitted. The test fails against the previous grammar and
passes with the tightened one.
docsAreEscaped_avro4053test and the fullcompilermoduletest suite continue to pass.
Documentation