Newlines in JSON in encrypted messages break parser

TR-SLimey commented 3 years ago

Description

This bug was found while testing the Syphon client. When replying to a message, Syphon sends some newlines within the body, which Element Android interprets fine, as does Element Web when the message is unencrypted, but when the message is encrypted, Element Web fails to decrypt the message with the following error: (note: one error per such message - the two errors displayed are from two separate messages)

Steps to reproduce

To emulate this behaviour using Element devtools, an event of type m.room.message can be sent with the following or similar contents:

{
    "msgtype": "m.text",
    "body": "a

a
a"
}

This should fail to send with an error alike to: Failed to send custom event. (SyntaxError: Unexpected token in JSON at position 41) even though newlines are perfectly legal within JSON, as implied by section 2.5 - Strings of https://www.ietf.org/rfc/rfc4627.txt.

Alternatively, the current-latest build of Syphon client can be used to reproduce the bug if a message is sent containing newlines.

Version information

Platform: web and desktop
Version: 1.7.18 through 1.7.20 tested (all failed)
OS: openSUSE Tumbleweed
Browser: Brave/Chromium (latest)

t3chguy commented 3 years ago

https://jsonlint.com/ considers your example JSON invalid

Trying that JSON in an encrypted (unencrypted does the same) room in /devtools I see the exact error you reference:

My test was done in Chromium (Desktop app) I saw similar in Firefox:

t3chguy commented 3 years ago

https://www.ietf.org/rfc/rfc4627.txt as far as I can tell this just says you can have any insignificant whitespace between the control characters []{}:, not "

TR-SLimey commented 3 years ago

Indeed it does fail to send in both encrypted and unencrypted actually. Weird. Receiving such messages only does work unencrypted though, so you may want to use syphon to test.

As for the RFC, I'm referring specifically to this quote:

   A string begins and ends with
   quotation marks.  All Unicode characters may be placed within the
   quotation marks except for the characters that must be escaped:
   quotation mark, reverse solidus, and the control characters (U+0000
   through U+001F).

Which appears to mean that newline characters can also be present without necessarily being in the escaped form.

TR-SLimey commented 3 years ago

Actually, my bad, apparently a newline is considered a control character upon further research ( I don't remember character encodings off-by-heart :P ). However, I still think this is an issue because of the lack of consistency with Element Android and unencrypted Element Web.

t3chguy commented 3 years ago

I just installed Syphon and sent some multi line messages with multi line replies in an unencrypted room and inspected them from Element Web I cannot see those stray lines. Either Syphon is not broken for unencrypted or Synapse is fixing it upon sending.

  {
    "content": {
      "body": "Testing\n\nDoobar",
      "msgtype": "m.text"
    },
    "origin_server_ts": 1612521804654,
    "room_id": "!QzKNC0BAQQYTRnlT:dendrite.kegsay.com",
    "sender": "@webdevguru_test19:matrix.org",
    "type": "m.room.message",
    "unsigned": {
      "age": 574390
    },
    "event_id": "$Nbm7FmycaXqTaXgGKtFUDSbzU2qv7ZLvGiH3T3YJ0k0",
    "user_id": "@webdevguru_test19:matrix.org",
    "age": 574390
  },
  {
    "content": {
      "body": "> <@webdevguru_test19:matrix.org> Testing\n\nDoobar\n\nReply\n\nTesting",
      "format": "org.matrix.custom.html",
      "formatted_body": "<mx-reply><blockquote><a href=\"https://matrix.to/#/!QzKNC0BAQQYTRnlT:dendrite.kegsay.com/$Nbm7FmycaXqTaXgGKtFUDSbzU2qv7ZLvGiH3T3YJ0k0\">In reply to</a><a href=\"https://matrix.to/#/@webdevguru_test19:matrix.org\">@webdevguru_test19:matrix.org</a><br />Testing\n\nDoobar</blockquote></mx-reply>Reply\n\nTesting",
      "m.relates_to": {
        "m.in_reply_to": {
          "event_id": "$Nbm7FmycaXqTaXgGKtFUDSbzU2qv7ZLvGiH3T3YJ0k0"
        }
      },
      "msgtype": "m.text"
    },
    "origin_server_ts": 1612521819762,
    "room_id": "!QzKNC0BAQQYTRnlT:dendrite.kegsay.com",
    "sender": "@webdevguru_test19:matrix.org",
    "type": "m.room.message",
    "unsigned": {
      "age": 559282
    },
    "event_id": "$Wm08tomOKq_IRv6AF3ieFP410ttLqY3XqqgZAkqM3fw",
    "user_id": "@webdevguru_test19:matrix.org",
    "age": 559282
  }

TR-SLimey commented 3 years ago

Ah, Synapse may indeed be fixing them seeing as the message is unencrypted. In that case, what do you suggest? I could submit a PR to Syphon to escape those, but at the same time, it may be good for Element Web to be able to handle them too.

t3chguy commented 3 years ago

I don't think that sounds sane, its invalid JSON after all. Dealing with every possible mistake in JSON is just infeasible, and this isn't common enough in other clients and masking over it might just result in it not being fixed for longer

TR-SLimey commented 3 years ago

That's fair, and I agree. Only thing is the inconsistency between the Elements, and I figured it's more sane to make one more fault-tolerant than the other less so. Regardless, I'll work on making Syphon JSON-compliant since that seems to be the core problem here.

TR-SLimey commented 3 years ago

PR submitted to Syphon (https://github.com/syphon-org/syphon/pull/231). Hopefully next release will work fine with EW. Meanwhile, if there are no plans to change anything in terms of this in Element, feel free to close this, I'll leave it open just in case.

jryans commented 3 years ago

Thanks for the feedback. I think it's unlikely we'll change Element for this at the moment, but perhaps it's something to document in matrix-doc at the spec level.

element-hq / element-web