Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SUBSCRIBE packet with no payload #119

Closed
ccarcaci opened this issue Sep 5, 2021 · 3 comments
Closed

SUBSCRIBE packet with no payload #119

ccarcaci opened this issue Sep 5, 2021 · 3 comments

Comments

@ccarcaci
Copy link
Contributor

ccarcaci commented Sep 5, 2021

I've implemented this test:

test('SUBSCRIBE packet has no payload', (done) => {
  const subscribe: ISubscribePacket = {
    cmd: 'subscribe',
    messageId: 0,
    subscriptions: [
      {
        topic: 'topic',
        qos: 0,
      },
    ],
  }
  const subscribePacket: Buffer = mqttPacket.generate(subscribe)
  const truncSubscribe = Buffer.alloc(2)
  subscribePacket.copy(truncSubscribe, 0, 0, 2)
  truncSubscribe[1] = 0x00 // No payload, Remaining Length = 0

  const parser = mqttPacket.parser(opts)
  parser.on('error', (error) => {
    expect(error.message).toBe('SUBSCRIBE packet MUST have payload')
    done()
  })
  parser.on('packet', () => {
    throw new Error('SUBSCRIBE packet with no payload is invalid')
  })
  parser.parse(truncSubscribe)
})

I've tampered a SUBSCRIBE packet by removing the payload and forcing the Remaining Length field to zero.

But the specification here: https://docs.oasis-open.org/mqtt/mqtt/v5.0/os/mqtt-v5.0-os.html#_Toc3901030
stated that the SUBSCRIBE packet MUST have a payload.

My tampered packet should be invalid or malformed. Nonetheless the parser is correcting parsing the packet emitting the 'packet' event making this test fail.

Is this ok?

@mcollina
Copy link
Member

mcollina commented Sep 5, 2021

Not really, would you like to send a PR?

@ccarcaci
Copy link
Contributor Author

ccarcaci commented Sep 6, 2021

@mcollina sure :)
Here we go: #121

@ccarcaci
Copy link
Contributor Author

ccarcaci commented Sep 7, 2021

Fix merged!

@ccarcaci ccarcaci closed this as completed Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants