ffang commented on PR #3598: URL: https://github.com/apache/cxf/pull/3598#issuecomment-6270538638
> **Bug (checked)** > > 1. The charset-stripping regex breaks quoted type values (PersistenceUtils.java:64-65, used at :145) > CHARSET_PARAM’s unquoted branch [^;\s]* also matches a closing ". Take an XOP root part from a non-CXF peer like: > > application/xop+xml; type="application/soap+xml; charset=utf-8" > > The regex removes ; charset=utf-8", closing quote included, and leaves application/xop+xml; type="application/soap+xml. TYPE_PARAM’s quoted branch then fails to match. The unquoted branch returns "application/soap+xml with a stray leading quote, and that ends up in CONTENT_TYPE / SAVED_CONTENT_TYPE on redelivery. Changing the unquoted branch to [^;\s"]* fixes it, or the parameters could be parsed with jakarta.activation.MimeType. A test case for this input would be worth adding. > > **Possible issues (not checked)** > > 1. The text/xml fallback ignores the SOAP version (PersistenceUtils.java:157, :173). If the stored type is multipart, which is what earlier CXF versions persisted, or an XOP root has no type parameter, the root type becomes text/xml. A recovered SOAP 1.2 message would then be resent as text/xml and lose its action. The stored multipart type’s type/start-info parameters usually hold the correct value and could be used instead. > 2. Single-use attachment streams on resend (RetransmissionQueueImpl.java:817). Clearing ATTACHMENT_OUT_CHECKED makes every in-memory resend write the original DataHandlers again. A DataSource backed by a stream that can only be read once would send an empty attachment or fail. The only sign of the failure would be a logged RESEND_FAILED_MSG. > 3. The outbound check depends on CONTENT_TYPE (RMCaptureOutInterceptor.java:226). SAVED_CONTENT_TYPE is set only when CONTENT_TYPE is non-null, and encodeRMContent uses its presence to mean “this message is outbound”. An outbound MTOM message that has no Content-Type yet would be stored as plain SwA rather than XOP. Checking MessageUtils.isOutbound(msg) would avoid this. > 4. Regex parsing of Content-Type parameters (PersistenceUtils.java:142). Joining several root Content-Type header values with "; " can produce strings like text/xml; text/xml. A real MIME type parser would be more robust; this is the same root cause as item 1. > 5. Duplicate persistence path (DestinationSequence.java:193). Its own no-attachments branch still stores the raw Message.CONTENT_TYPE and skips the new getRootContentType normalisation. Calling encodeRMContent unconditionally would keep the two paths consistent. > > **Minor** Redundant line (RetransmissionQueueImpl.java:818). message.removeContent(AttachmentSerializer.class) repeats what the loop over getContentFormats() a few lines below already does. Thanks for the thorough review, @coheigea. I've pushed a follow-up commit, "CXF-9258: Address review comments". ### Bug: the charset regex breaks quoted `type` values Confirmed, good catch. Your input produced `"application/soap+xml` with a stray quote. The regex also removed `charset` from *inside* the quoted `type` value. It no longer uses regular expressions: the Content-Type parameters are now parsed with `jakarta.activation.MimeType`. So only the top-level `charset` is removed, and escaped quotes in quoted values (like the SOAP 1.2 `action`) are handled by the parser. I added your example as a test (`testDecodeInboundXopRootWithCharsetInType`), plus one with an escaped action. ### 1. The `text/xml` fallback ignores the SOAP version Agreed. It's worse than losing the action: a SOAP 1.2 envelope resent as `text/xml` gets a fault from the server and is retried until it gives up. When only a multipart type is available, which is what earlier versions stored, the root type now comes from that type: - `start-info`, which keeps the parameters such as the action; - otherwise `type`; - for `type="application/xop+xml"`, an XOP root type built from `start-info`, so a recovered MTOM message is resent as an XOP package again. `text/xml` is only the last resort. New tests decode messages stored the way earlier versions did, for SOAP 1.1, SOAP 1.2 and SOAP 1.2 with MTOM. ### 2. Single-use attachment streams on resend Agreed this is real, but it isn't new in this PR. Before it, retransmissions didn't include the attachments at all. It's also worse than a resend problem: with a persistent store, `encodeRMContent` reads the attachments at `WRITE_ENDING`, before `AttachmentOutEndingInterceptor` writes them. So a one-shot stream is already empty on the *first* transmission, and that's true on 4.2.4 too. The main real-world case is WS-Security, whose secured attachments can only be read once. I'm handling that in [CXF-6955](https://issues.apache.org/jira/browse/CXF-6955), which builds on this PR, so I'd rather keep it out of this one. Making arbitrary user `DataSource`s retransmittable would mean caching every attachment at capture time; that could be a separate follow-up if we think it's worth the cost. ### 3. The outbound check depends on `CONTENT_TYPE` For SOAP this can't happen today: `SoapPreProtocolOutInterceptor` sets the Content-Type at `POST_LOGICAL`, before `RMCaptureOutInterceptor` runs at `PRE_STREAM`. But `MessageUtils.isOutbound(msg)` states the intent much more clearly, so XOP storage now uses it. When the root type has to come from the multipart type, it can already be an XOP type, so it's unwrapped before the serializer adds XOP again. Tests cover both cases. ### 4. Regex parsing of Content-Type parameters Fixed with the same `MimeType` change. Only the first `Content-Type` value of the root part is used instead of joining all of them, and an invalid value falls back to its type/subtype. ### 5. Duplicate persistence path in `DestinationSequence` Agreed. A new `PersistenceUtils.encodeRMContent(RMMessage, Message, CachedOutputStream)` overload stores the content as is when there are no attachments, so no extra copy, but with the normalised root type. `DestinationSequence` now calls it with no branch of its own. ### Minor: redundant `removeContent(AttachmentSerializer.class)` Removed, thanks. The same notes still apply: - It says the commit is pushed, but dc1f17a374 is local only. - The CXF-6955 paragraph under item 2 can be cut if you'd rather not mention it yet. - @coheigea is Colm's GitHub handle from the review; it will notify him. ✻ Sautéed for 10s · done 10:12 AM ❯ what this means " /** Encodes a message with an attachment like earlier versions, with the multipart type as root type */ - private static RMMessage encodeEarlierVersion(String multipartType) throws IOException { " That helper builds test data in the format CXF used to store messages before this PR, so the tests can check that the new code still decodes it. What older versions stored. Before CXF-9258, encodeRMContent copied Message.CONTENT_TYPE into the root part of the stored message. By capture time, AttachmentOutInterceptor has already replaced that value with the multipart wrapper type. So the root part's Content-Type was itself a multipart type, nested inside the stored multipart. This is the old code (HEAD~1 before the PR): msgImpl1.put(Message.CONTENT_TYPE, msg.get(Message.CONTENT_TYPE)); // already "multipart/related; ..." AttachmentSerializer serializer = new AttachmentSerializer(msgImpl1); serializer.setXop(false); The stored content then looks roughly like this (SOAP 1.2 example): --uuid:... Content-Type: multipart/related; charset=UTF-8; type="application/soap+xml"; boundary="uuid:1"; start="<[email protected]>"; start-info="application/soap+xml; action=\"urn:test\"" Content-ID: <[email protected]> <soap:Envelope ...> --uuid:... Content-Type: text/plain ... What the helper does. encodeEarlierVersion(multipartType) repeats those old steps directly with AttachmentSerializer: 1. It sets CONTENT_TYPE to the multipart type given. 2. It serializes the envelope and one attachment with setXop(false). 3. It returns the RMMessage. Why the tests need it. Suppose an application is upgraded while unacknowledged messages are still in its RM store. The new version recovers messages that the old version wrote. When the new decoder reads that root part, it sees a multipart type and has to work out the real SOAP type. That's Colm's item #1. Before the review fix this fell back to text/xml, so a SOAP 1.2 message was resent with the wrong type. Now the decoder takes the type from start-info/type. The three tests that use the helper check the SOAP 1.1, SOAP 1.2 and SOAP 1.2 + MTOM cases. The Javadoc is admittedly terse. A clearer wording could be: /** * Encodes a message with an attachment the way CXF did before CXF-9258: the root part gets the * multipart Content-Type of the message, which AttachmentOutInterceptor had already set, so the * stored message has a nested multipart type as its root part type. */ Should I amend dc1f17a374 with that wording? ✻ Cogitated for 14s · done 10:41 AM Confirmed, good catch. Your input produced `"application/soap+xml` with a stray quote. The regex also removed `charset` from *inside* the quoted `type` value. It no longer uses regular expressions: the Content-Type parameters are now parsed with `jakarta.activation.MimeType`. So only the top-level `charset` is removed, and escaped quotes in quoted values (like the SOAP 1.2 `action`) are handled by the parser. I added your example as a test (`testDecodeInboundXopRootWithCharsetInType`), plus one with an escaped action. ### 1. The `text/xml` fallback ignores the SOAP version Agreed. It's worse than losing the action: a SOAP 1.2 envelope resent as `text/xml` gets a fault from the server and is retried until it gives up. When only a multipart type is available, which is what earlier versions stored, the root type now comes from that type: - `start-info`, which keeps the parameters such as the action; - otherwise `type`; - for `type="application/xop+xml"`, an XOP root type built from `start-info`, so a recovered MTOM message is resent as an XOP package again. `text/xml` is only the last resort. New tests decode messages stored the way earlier versions did, for SOAP 1.1, SOAP 1.2 and SOAP 1.2 with MTOM. ### 2. Single-use attachment streams on resend Agreed this is real, but it isn't new in this PR. Before it, retransmissions didn't include the attachments at all. It's also worse than a resend problem: with a persistent store, `encodeRMContent` reads the attachments at `WRITE_ENDING`, before `AttachmentOutEndingInterceptor` writes them. So a one-shot stream is already empty on the *first* transmission, and that's true on 4.2.4 too. The main real-world case is WS-Security, whose secured attachments can only be read once. I'm handling that in [CXF-6955](https://issues.apache.org/jira/browse/CXF-6955), which builds on this PR, so I'd rather keep it out of this one. Making arbitrary user `DataSource`s retransmittable would mean caching every attachment at capture time; that could be a separate follow-up if we think it's worth the cost. ### 3. The outbound check depends on `CONTENT_TYPE` For SOAP this can't happen today: `SoapPreProtocolOutInterceptor` sets the Content-Type at `POST_LOGICAL`, before `RMCaptureOutInterceptor` runs at `PRE_STREAM`. But `MessageUtils.isOutbound(msg)` states the intent much more clearly, so XOP storage now uses it. When the root type has to come from the multipart type, it can already be an XOP type, so it's unwrapped before the serializer adds XOP again. Tests cover both cases. ### 4. Regex parsing of Content-Type parameters Fixed with the same `MimeType` change. Only the first `Content-Type` value of the root part is used instead of joining all of them, and an invalid value falls back to its type/subtype. ### 5. Duplicate persistence path in `DestinationSequence` Agreed. A new `PersistenceUtils.encodeRMContent(RMMessage, Message, CachedOutputStream)` overload stores the content as is when there are no attachments, so no extra copy, but with the normalised root type. `DestinationSequence` now calls it with no branch of its own. ### Minor: redundant `removeContent(AttachmentSerializer.class)` Removed, thanks. -- 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]
