eclipse-ee4j / openmq

OpenMQ
https://projects.eclipse.org/projects/ee4j.openmq/
Other
50 stars 34 forks source link

Missing escaping / unescaping of colon in Stomp bridge (Stomp 1.1/Stomp 1.2) #1522

Closed michaelJustin closed 2 years ago

michaelJustin commented 2 years ago

The Stomp 1.1 and 1.2 spec contains a formal (BNF) spec of allowed header octets:

header              = header-name ":" header-value
header-name         = 1*<any OCTET except CR or LF or ":">
header-value        = *<any OCTET except CR or LF or ":">

A colon character must be escaped/un-escaped using \c (see section "Value Encoding"):

When decoding frame headers, the following transformations MUST be applied: \r (octet 92 and 114) translates to carriage return (octet 13) \n (octet 92 and 110) translates to line feed (octet 10) \c (octet 92 and 99) translates to : (octet 58) \\ (octet 92 and 92) translates to \ (octet 92)

https://stomp.github.io/stomp-specification-1.2.html

In the stomp bridge, this transformation seems to be missing, which causes errors for example for ACK frames and for temporary queue destinations.

Example for ACK: the server sends a MESSAGE frame with the ack and the message-id headers with no colon escaping:

MESSAGE
subscription:{C3B6FE92-9B3B-47DA-92BF-E21C124215AB}
destination:/queue/TStomp12TestCase.TestClientAck.Q
message-id:ID:63-192.168.178.22(d7:b6:4e:4a:1c:25)-50808-1657872176971
ack:{C3B6FE92-9B3B-47DA-92BF-E21C124215AB}ID:63-192.168.178.22(d7:b6:4e:4a:1c:25)-50808-1657872176971
expires:0
redelivered:true
priority:4
timestamp:1657872176971
JMSXDeliveryCount:3
messagetype:text
content-length:9
[#|2022-07-15T10:02:57.080+0200|SEVERE|6.3.0||_ThreadID=33;_ThreadName=Grizzly-worker(5);|[BSS3002]: STOMP command ACK failed with [BSS4012]: Invalid value id for header {ED715DB3-B15D-4A73-924C-5FFFC3527DBF}ID\c63-192.168.178.22(d7\cb6\c4e\c4a\c1c\c25)-50808-1657872176971 on STOMP connection 3232320097869072640[null]
com.sun.messaging.bridge.api.StompProtocolException: [BSS4012]: Invalid value id for header {ED715DB3-B15D-4A73-924C-5FFFC3527DBF}ID\c63-192.168.178.22(d7\cb6\c4e\c4a\c1c\c25)-50808-1657872176971
        at com.sun.messaging.bridge.api.StompProtocolHandler.doACK(StompProtocolHandler.java:538)
        at com.sun.messaging.bridge.api.StompProtocolHandler.onACK(StompProtocolHandler.java:487)
        at com.sun.messaging.bridge.service.stomp.StompMessageDispatchFilter.handleRead(StompMessageDispatchFilter.java:99)
        at org.glassfish.grizzly.filterchain.ExecutorResolver$9.execute(ExecutorResolver.java:88)
        at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeFilter(DefaultFilterChain.java:246)
        at org.glassfish.grizzly.filterchain.DefaultFilterChain.executeChainPart(DefaultFilterChain.java:178)
        at org.glassfish.grizzly.filterchain.DefaultFilterChain.execute(DefaultFilterChain.java:118)
        at org.glassfish.grizzly.filterchain.DefaultFilterChain.process(DefaultFilterChain.java:96)
        at org.glassfish.grizzly.ProcessorExecutor.execute(ProcessorExecutor.java:51)
        at org.glassfish.grizzly.nio.transport.TCPNIOTransport.fireIOEvent(TCPNIOTransport.java:510)
        at org.glassfish.grizzly.strategies.AbstractIOStrategy.fireIOEvent(AbstractIOStrategy.java:82)
        at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy.run0(WorkerThreadIOStrategy.java:83)
        at org.glassfish.grizzly.strategies.WorkerThreadIOStrategy$WorkerThreadRunnable.run(WorkerThreadIOStrategy.java:101)
        at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.doWork(AbstractThreadPool.java:535)
        at org.glassfish.grizzly.threadpool.AbstractThreadPool$Worker.run(AbstractThreadPool.java:515)
        at java.base/java.lang.Thread.run(Thread.java:834)

Example for temporary queues: the broker sends a MESSAGE frame with the reply-to and the message-id headers with no colon escaping:

MESSAGE
subscription:{87742B50-2308-44BE-97AD-E9A4532ED244}
destination:/queue/TTemporaryQueueTestCase.TestTemporaryQueue.Q
message-id:ID:7424-192.168.178.22(d8:74:dc:b2:21:43)-50616-1657870225531
reply-to:/temp-queue/temporary_destination://queue/192.168.178.22/50616/1
expires:0
redelivered:false
priority:4
timestamp:1657870225531
messagetype:text
habaritest:request-2981015
JMSXDeliveryCount:1
content-length:15

Note: the issue only exists with clients which use Stomp protocol above 1.0, so the workaround is to stay on Stomp 1.0.

michaelJustin commented 2 years ago

It seems the other character transformations are not applied either. (source in class StompFrameMessage). Maybe I can write a fix (at least a draft) however the StompFrameMessage does not contain a reference to the Stomp protocol version of the current connection. Injecting it would be quite invasive.

pzygielo commented 2 years ago

https://eclipse-ee4j.github.io/openmq/guides/mq-tech-over/brokers.html#gjdmw refers to http://stomp.codehaus.org. https://eclipse-ee4j.github.io/openmq/guides/mq-admin-guide/bridge-services.html#gjmnu refers to http://docs.codehaus.org/display/STOMP/Stomp+JMS, which then links to - something like - http://web.archive.org/web/20150520203728/https://docs.codehaus.org/display/STOMP/Protocol which is 1.0. So I think this is what is supported.

Thus I'm not sure if there is workaround or fix needed here, as I can't see a bug (now). It should rather be treated as new feature(s) to support newer STOMPs I guess. (But that's just naming.)

Please submit any PR you like. If StompFrameMessage needs to be refactored - feel free to do so in first, separate step.

michaelJustin commented 2 years ago

Thanks for the pointers, @pzygielo, I will prepare a PR. My favourite location to store the negotiated Stomp protocol version would be the StompFrameMessageFactory. This will cause very little code changes.

Are there any special standards I have to follow for the PR?

michaelJustin commented 2 years ago

I have submitted PR #1525 for issue #1524. My ECA, which I signed yesterday, is not approved yet. Maybe this was caused by using my 'anonymous' info@...' email address. I have already changed this in my GitHub profile but the PR still indicates the error.

Regarding 1.2 support, in the Stomp protocol negotiation, Stomp 1.0 and 1.2 are handled as supported (in method getSupportedVersions() for the regular StompProtocolHandlerImpl). I have not further checked the WebSocket StompProtocolHandlerImpl, but it seems it supports only Stomp 1.2 as this is the only version returned by getSupportedVersions().

Most parts of Stomp 1.2 seem to be already implemented according to the specification. The methods which encode / decode Stomp frame header do not apply the escape / unescape. I will create a new issue soon.

pzygielo commented 2 years ago

Apparently I was wrong, and some support for STOMP 1.1, 1.2 was delivered in MQ5.1.1_B02.

michaelJustin commented 2 years ago

I have created #1534, I suggest to close this issue (#1522) as it is covered with the new issue.

pzygielo commented 2 years ago