atiaomar1978-hub commented on PR #26026: URL: https://github.com/apache/camel/pull/26026#issuecomment-5502300764
## Grok code review — findings and resolutions Grok reviewed the initial implementation and flagged several issues. Below is each finding and how it was addressed in commit `5167d720ad0`. ### Security / correctness | # | Grok finding | Resolution | |---|-------------|------------| | 1 | Allowlist stored in mutable `System.setProperty` re-read on every `isTrusted()` call — any code could widen trust at runtime | **Fixed.** Trusted packages/classes now stored in private in-memory `ConcurrentHashMap` sets inside `AvroClassSecuritySupport` | | 2 | `setGlobal()` clobbers prior validators; state leaks across tests | **Mitigated.** `resetForTesting()` restores `ClassSecurityValidator.DEFAULT`; tests use `@BeforeEach` isolation | | 3 | Data-format `serializablePackages` DSL example in upgrade guide did not compile | **Fixed.** Upgrade guide now shows valid Java API usage with `AvroDataFormat.setSerializablePackages()` | | 4 | Marshal without preconfigured schema never trusts graph class | **Fixed.** `AvroDataFormat.marshal()` calls `trustClassName(graph.getClass().getName())` before schema resolution | | 5 | `serializablePackages=*` silently no-op | **Fixed.** Wildcard rejected with `IllegalArgumentException` | | 6 | Auto-trust of entire `org.apache.avro` too broad; applied from data format too | **Fixed.** IPC trust scoped to `org.apache.avro.ipc` and only invoked from `AvroComponent.doInit()` (not data format) | | 7 | Missing `security = "insecure:serialization"` on `serializablePackages` | **Fixed.** Added to `@UriParam` and `@Metadata` | ### Architecture note (accepted trade-off) | Finding | Status | |---------|--------| | `camel-avro-rpc` → `camel-avro` dependency to share `AvroClassSecuritySupport` | **Accepted.** Keeps a single JVM-wide allowlist registry; avoids duplicate static state in two JARs | ### Test improvements made - Added `@BeforeEach resetForTesting()` for test isolation - Added `HandshakeRequest` IPC trust test in `AvroClassSecurityWithoutVmArgsTest` - Added negative test (untrusted `TestPojo` outside protocol packages) - Added wildcard rejection test - Migrated to AssertJ in security tests - All 68 existing RPC integration tests pass **without** Surefire `SERIALIZABLE_PACKAGES` vmargs ### Remaining follow-ups (non-blocking) - Walk nested schema namespaces for union/record fields with cross-namespace references (future enhancement) - Wire `serializablePackages` through Camel DSL model/reifier for YAML/XML route config (currently available via Java API and URI params) --- _AI-generated comment by Cursor Agent on behalf of atiaomar1978-hub_ -- 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]
