AVRO-4313: [java] Tighten javaAnnotation string-literal validation in SpecificCompiler - #3892
Merged
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.
Also reject NEL, LS and PS in addition to CR and LF so an annotation value cannot span multiple lines in the generated source.
Comment on lines
+1045
to
+1056
| 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); | ||
| } |
Member
Author
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.
RyanSkraba
approved these changes
Jul 26, 2026
Contributor
|
Cherry-picked to branch-1.12. |
RyanSkraba
pushed a commit
that referenced
this pull request
Jul 26, 2026
… SpecificCompiler (#3892) * AVRO-4313: [java] Tighten javaAnnotation string-literal validation in 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. * AVRO-4313: Exclude all line terminators from annotation string literals Also reject NEL, LS and PS in addition to CR and LF so an annotation value cannot span multiple lines in the generated source. * AVRO-4313: Make injection regression test robust to multiple outputs 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
Avro copies a schema's
javaAnnotationproperty verbatim into the Javasource it generates (e.g.
@Deprecated,@SuppressWarnings("unchecked")).Before doing so,
SpecificCompilerchecks the value with a regex to make sureit only looks like a Java annotation and nothing more.
That check had a hole. The sub-pattern for a quoted string (like
"unchecked")accepted an unescaped double-quote inside the string body. So a single
string literal could run past its closing quote and swallow whatever came after
it, while the whole value still matched the "valid annotation" shape.
A crafted value like:
therefore passed validation and was written straight into the generated class —
a code-injection at code-generation time.
What this PR changes
Tighten the string-literal grammar so its body may only contain:
\\ \" \n \t \f \b, orNow an unescaped quote ends the literal, so the injected trailing tokens fail
validation and the value is rejected instead of emitted. Line terminators
(CR, LF, NEL, LS, PS) are excluded too, so a value can't span multiple lines in
the generated source.
Legitimate annotations still validate:
SuppressWarnings("unchecked"),Deprecated(forRemoval = true, since = "forever"), and values with escapedquotes.
Verifying this change
annotationCannotBreakOutViaStringLiteralinTestSpecificCompiler:it feeds the crafted value above alongside a normal annotation, and asserts
the injected code is absent from every generated file while the valid
annotation is still emitted. It fails on the old grammar and passes on the new.
docsAreEscaped_avro4053test and the fullcompilermodulesuite continue to pass.
Documentation