Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Strictly enforce canonicaljson requirements in a new room version #7381

Merged
merged 13 commits into from
May 14, 2020

Conversation

clokep
Copy link
Member

@clokep clokep commented Apr 30, 2020

This PR does the following:

  • Adds an experimental room version which strictly adheres to the canonical JSON specification:
    • Rejects integers outside of the range of [-2 ^ 53 + 1, 2 ^ 53 - 1]
    • Rejects floats
    • Does not parse Python special values (NaN, Infinity, -Infinity)
  • Logic to strictly enforce this is added to the client-server API and the federation API.

Replaces #7356

Implements MSC2540: matrix-org/matrix-spec-proposals#2540.

To Do

  • Perform more testing with federation.
  • Does this need to be optimized? (Some really rough numbers show that this about doubles the runtime of event_from_pdu_json.)
    • No!
  • Can we consolidate some of this validation logic further?

@clokep clokep requested a review from a team April 30, 2020 18:28
@clokep
Copy link
Member Author

clokep commented Apr 30, 2020

Requesting review on this to get some thoughts on the approach.

synapse/handlers/message.py Outdated Show resolved Hide resolved
@clokep
Copy link
Member Author

clokep commented Apr 30, 2020

I did some testing with two different clients:

  • The server rejects bad messages from a client.
  • The server will reject a bad message over fedation from another server.
  • The server will reject a bad message while backfilling from another server.

Are there other scenarios to test?

@ara4n
Copy link
Member

ara4n commented May 2, 2020

It's not a canonicaljson thing, but, while we're adding a new room version, might be good to check that power levels have to be ints rather than strings (and perhaps sanitycheck other type safety in synapse)

@richvdh
Copy link
Member

richvdh commented May 4, 2020

power levels have to be ints rather than strings (and perhaps sanitycheck other type safety in synapse)

that is the thin end of a very large wedge. I think we're better off keeping this constrained for now.

synapse/api/room_versions.py Show resolved Hide resolved
raise SynapseError(400, "JSON integer out of range", Codes.BAD_JSON)

elif isinstance(value, float):
# Note that Infinity, -Infinity, and NaN are also considered floats.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CanonicalJSON indeed does aim to represent all floats as ints, just in case anyone was unsure about blocking all floats: https://matrix.org/docs/spec/appendices#canonical-json

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, as the description says all floats need to be rejected. I didn't find the specification super clear here unless you read into the grammar though. I think we could improve that.

synapse/handlers/message.py Outdated Show resolved Hide resolved
synapse/handlers/message.py Outdated Show resolved Hide resolved
@clokep clokep requested a review from richvdh May 14, 2020 13:43
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like generally the right plan to me.

I wouldn't be too worried about performance here: event_from_pdu_json is called only once for each event, and it tends to happen on workers rather than on the master.

synapse/handlers/message.py Outdated Show resolved Hide resolved
synapse/handlers/message.py Outdated Show resolved Hide resolved
@clokep clokep marked this pull request as ready for review May 14, 2020 16:42
@clokep clokep requested a review from richvdh May 14, 2020 16:43
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants