coheigea commented on PR #3609: URL: https://github.com/apache/cxf/pull/3609#issuecomment-6313122361
Thanks for this. The approach looks right: originals are saved at `POST_MARSHAL`, before DOM WS-Security replaces them at `POST_PROTOCOL`, and each retransmission secures them again. The new `WSS4JStaxOutInterceptor` → after `AttachmentOutInterceptor` rule fits the existing constraints with no cycle: `StaxOutInterceptor` is already after `AttachmentOutInterceptor`, and the StAX ending interceptor already runs before `AttachmentOutEndingInterceptor`. The tests cover a lot of ground. A few findings: ### 1. A bad persisted message can now stop recovery for the whole endpoint (please fix) `PersistenceUtils.decodeRMContent` (new hold loop) and `RMManager` (`new ArrayList<>(message.getAttachments())`) Recovery now reads all attachments straight away instead of at resend time. `LazyAttachmentCollection` wraps read errors in `RuntimeException`. That includes truncated or corrupt content and `"The message contains more attachments than are permitted"`; the default limit is 50, and outbound sends never check it. The hold loop rethrows the `RuntimeException`, but `recoverSourceSequence` only catches `IOException`, so it escapes `recoverReliableEndpoint`. The remaining source and destination sequences are then never recovered, and `retransmissionQueue.start()` / `redeliveryQueue.start()` never run. Suggested fix: wrap the `RuntimeException` in an `IOException` inside `decodeRMContent`, or catch `RuntimeException` in both recovery loops in `RMManager`. A unit test with a truncated multipart body, or more than 50 parts, would cover it. ### 2. `AttachmentSaveInterceptor` belongs in the WS-RM module (minor) It's added as a public class in core `org.apache.cxf.interceptor`, but only WS-RM uses it, and its Javadoc is about WS-RM and WS-Security. Could it move into `rt/ws/rm`, for example as a nested class next to `CaptureStart`/`CaptureEnd` in `RMCaptureOutInterceptor`? That keeps it out of the core public API. ### 3. Attachments without an ID are deduplicated by `null` (nit) `AttachmentSaveInterceptor.handleMessage` dedupes by `a.getId()`, so two attachments with no ID would collapse into one. That's unlikely, since SwA and MTOM parts always have a Content-ID, but skipping the check when the ID is `null` costs nothing. ### 4. The cleanup code reopens a stream to delete the cache file (nit) In the `ATTACHMENTS_CLOSEABLE` cleanup, `ads.release()` is followed by `ads.getInputStream().close()`, which relies on `CachedOutputStream` deleting its temp file when the last reader closes. It works, but it's fragile. A short comment, or a `CachedOutputStream` method that releases and deletes in one step, would help. ### 5. Originals must now be readable more than once (release note) With a persistent store and DOM security, each original attachment is read by WSS4J, then again by `encodeRMContent`, and again on every retransmission. Persistence without security already read them twice, so this isn't new in principle, but a user `DataSource` that can only be read once will now fail with security turned on. Probably worth a line in the release notes. -- 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]
