uros-b commented on code in PR #17852:
URL: https://github.com/apache/iceberg/pull/17852#discussion_r3880286127
##########
core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java:
##########
@@ -30,13 +31,39 @@
import org.apache.avro.io.Decoder;
import org.apache.iceberg.common.DynClasses;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
import org.apache.iceberg.types.Type;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Pair;
public class GenericAvroReader<T>
implements DatumReader<T>, SupportsRowPosition, SupportsCustomRecords {
+ // Restricts record-name-based class resolution to a known-safe set. Record
names come from
+ // Avro schemas, which can originate from untrusted input (e.g.
AvroEncoderUtil.decode);
+ // without this check, any class name that happens to match something
already on the classpath
+ // would be loaded and constructed. See
https://github.com/apache/iceberg/issues/17802.
+ //
+ // NOTE: Kafka Connect's classes are listed by name (not Class literal)
because iceberg-core
+ // cannot depend on iceberg-kafka-connect-events. Keep this in sync with
that module's
+ // AvroUtil.FIELD_ID_TO_CLASS if either changes.
+ private static final Set<String> ALLOWED_RECORD_CLASSES =
Review Comment:
The Kafka-Connect FQCNs are string literals kept in sync with
kafka-connect-events AvroUtil.FIELD_ID_TO_CLASS by comment only (core cannot
depend on that module). However, a newly added or renamed Connect event
payload, or a core generic type not mirrored here, could potentially degrade to
a generic record at decode time (runtime, not compile-time). Something like a
cross-module round-trip test in iceberg-kafka-connect could probably enforce
the invariant better...
##########
core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java:
##########
@@ -123,7 +150,7 @@ public ValueReader<?> record(Type partner, Schema record,
List<ValueReader<?>> f
private ValueReader<?> recordReader(
Review Comment:
A non-null record-class name that is absent from the allowlist falls back to
GenericData.Record, mirroring the pre-existing ClassNotFoundException catch. A
future caller that routes a new class through AvroEncoderUtil.decode but
forgets to add it to ALLOWED_RECORD_CLASSES would get a confusing downstream
ClassCastException at the cast site rather than a clear diagnostic.
Perhaps we can do something like throw IllegalArgumentException("Record
class not in ALLOWED_RECORD_CLASSES: " + className) when className is non-null
but not allowed, instead of falling through.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
