damansingh1313 commented on code in PR #17852:
URL: https://github.com/apache/iceberg/pull/17852#discussion_r3886348332
##########
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:
Dug into this further and wanted to share what I found.
Confirmed the gap is real: I removed GenericManifestFile from the allowlist
and ran TestFlinkManifest (its actual production caller via Flink's checkpoint
serializer) — it failed with a ClassCastException in DeltaManifestsSerializer,
a module away from the real cause, exactly as you'd expect from a silent
fallback.
I initially leaned toward throwing an IllegalArgumentException instead of
falling back silently, but realized it has a real cost: it makes "class exists
on the classpath but isn't allowlisted" an externally observable signal — an
attacker could probe arbitrary class names via decode() and use
throw-vs-fallback as an oracle for what's present on the classpath. That's
counter to the spirit of this issue.
Proposed fix instead: log a WARN naming the class (still falling back to a
generic record, same as today) rather than throwing. Gives a clear trail to the
real cause without changing anything decode() returns externally, so no new
oracle.
Let me know if that trade-off makes sense to you, or if you'd still prefer
the throw.
--
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]
