SEZ9 commented on PR #11559: URL: https://github.com/apache/seatunnel/pull/11559#issuecomment-5659237125
Following up on the one open item from my earlier review, the unsafe deserialization pattern in `KafkaSourceReader.java` (PR11559-F1). Based on what is on the thread, this looks resolved: `deserializeSplit(byte[])` now constructs `KafkaGateObjectInputStream` rather than a plain `ObjectInputStream`, and its `resolveClass` override enforces a closed allowlist of seven exact class names (`[B`, `java.lang.String`, `java.lang.Long`, `java.lang.Integer`, `KafkaSourceSplit`, `TopicPartition`, `TablePath`), throwing `IOException` for anything else. An exact-name allowlist rather than a package-prefix check is what I was hoping to see, so I'm comfortable with the approach. I also note the protection came in with `f40ad4eaa1e1930ca9fa29003f632819345837e1` and that neither `779004c073` nor `4d75844b4a` touched this file. Two small asks before I mark F1 closed: 1. The last verification of this class was against `4d75844b4a`. Since `2ab2851f03d` and the subsequent `dev` sync landed after that, please confirm `KafkaSourceReader.java` is unchanged at the current head (a quick diff of the file between `4d75844b4a` and the current head is enough). 2. Please point me to (or add, if one doesn't exist yet) a unit test that feeds `deserializeSplit` a payload containing a class outside the allowlist and asserts the `IOException` path, plus a positive round-trip of a real `KafkaSourceSplit`. That locks the allowlist in so a future refactor can't silently widen it. Nothing else outstanding from my side on this finding. <!-- streview-comment:1044 --> -- 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]
