search

eclipse / paho.mqtt.testing

An Eclipse Paho project - a Python broker for testing
https://eclipse.org/paho
Other
108 stars 73 forks source link

invalid subscription topic causes disconnection #62

Open dustin opened 5 years ago

dustin commented 5 years ago

I was sending an invalid subscription topic from my client to see the suback response with subscription errors and got disconnected. I noticed this initially with mosquitto and then tried the testing client and found the same behavior. This doesn't seem to violate:

When the Server receives a SUBSCRIBE packet from a Client, the Server MUST respond with a SUBACK packet [MQTT-3.8.4-1].

badsub.pcap.gz

dustin commented 5 years ago

There's a similar discussion at https://github.com/eclipse/mosquitto/issues/1413

icraggs commented 5 years ago

If there is a protocol error, the specification says that the client application must be disconnected. The server can always disconnect a client, and since the topic specification is part of the standard I think it certain it is a protocol error.

For improved error reporting, but perhaps not a change in behaviour in this case, you can look to MQTT V5.

dustin commented 5 years ago

The attached pcap shows mqtt v5:

Screen Shot 2019-09-16 at 15 19 16

I was actually hoping to see that my client did the right thing on a negative sub ack, but just got disconnected.

In any case, while the protocol may be allowed to disconnect when it doesn't like something on the wire, when there's an explicit channel for reporting an inability or unwillingness to perform a particular task, making the request fatal seems like poor behavior in general. In the case of an interoperability framework, it's a lost opportunity to test error handling.

I think in order to consider this a protocol error, the protocol must be considered entirely too broad. This is a whole layer up. A valid subscribe request was received and understood by the server, and it decided part of the message was undesirable and tore down the connection. If the packet were not well-formed, that would be a reasonable thing to do. I argue that it isn't here.

Similarly, if an HTTP request has a path the server doesn't like, it will respond with a 400 class error, which is part of the spec. It could just drop the connection and the client will correctly infer there was an error, but I don't think anyone would argue that's good behavior in general.