youichi-uda commented on code in PR #548:
URL: https://github.com/apache/avro-rs/pull/548#discussion_r3332582593
##########
avro/src/schema/parser.rs:
##########
@@ -795,29 +799,36 @@ impl Parser {
&self,
aliases: Option<Vec<String>>,
namespace: NamespaceRef,
- ) -> Aliases {
- aliases.map(|aliases| {
- aliases
- .iter()
- .map(|alias| {
- if alias.find('.').is_none() {
- match namespace {
- Some(ns) => format!("{ns}.{alias}"),
- None => alias.clone(),
- }
- } else {
- alias.clone()
- }
- })
- .map(|alias| Alias::new(alias.as_str()).unwrap())
- .collect()
- })
+ ) -> AvroResult<Aliases> {
+ aliases
+ .map(|aliases| {
+ aliases
+ .iter()
+ .map(|alias| {
+ let qualified = if alias.find('.').is_none() {
+ match namespace {
+ Some(ns) => format!("{ns}.{alias}"),
+ None => alias.clone(),
+ }
+ } else {
+ alias.clone()
+ };
+ // An alias that is not a valid Avro name is a schema
+ // error — propagate it instead of unwrapping (which
+ // would panic on attacker-controlled schema JSON).
+ Alias::new(qualified.as_str())
Review Comment:
Done in ac0282c — switched to `contains('.')` and pass `&str` /
`&format!("{ns}.{alias}")` straight into `Alias::new`, dropping the
intermediate `qualified` String and the per-alias clones. Thanks!
##########
avro/src/schema/parser.rs:
##########
@@ -795,29 +799,36 @@ impl Parser {
&self,
aliases: Option<Vec<String>>,
namespace: NamespaceRef,
- ) -> Aliases {
- aliases.map(|aliases| {
- aliases
- .iter()
- .map(|alias| {
- if alias.find('.').is_none() {
- match namespace {
- Some(ns) => format!("{ns}.{alias}"),
- None => alias.clone(),
- }
- } else {
- alias.clone()
- }
- })
- .map(|alias| Alias::new(alias.as_str()).unwrap())
- .collect()
- })
+ ) -> AvroResult<Aliases> {
+ aliases
+ .map(|aliases| {
+ aliases
+ .iter()
+ .map(|alias| {
+ let qualified = if alias.find('.').is_none() {
+ match namespace {
+ Some(ns) => format!("{ns}.{alias}"),
+ None => alias.clone(),
+ }
+ } else {
+ alias.clone()
+ };
+ // An alias that is not a valid Avro name is a schema
+ // error — propagate it instead of unwrapping (which
+ // would panic on attacker-controlled schema JSON).
+ Alias::new(qualified.as_str())
+ })
+ .collect::<AvroResult<Vec<_>>>()
+ })
+ .transpose()
}
fn get_schema_type_name(&self, name: Name, value: &Value) -> Name {
match value.get("type") {
Some(Value::Object(complex_type)) => match complex_type.name() {
- Some(name) => Name::new(name).unwrap(),
+ // Fall back to the enclosing name if the nested `type` name is
+ // not a valid Avro name, rather than panicking on `unwrap()`.
+ Some(type_name) => Name::new(type_name).unwrap_or(name),
Review Comment:
Agreed — silently dropping the nested type name is wrong. Changed
`get_schema_type_name` to return `AvroResult<Name>` and propagate the
validation error; the two callers now use `?`. Done in ac0282c.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
