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

Missing content for edited encrypted event #14252

Closed
richvdh opened this issue Oct 21, 2022 · 6 comments
Closed

Missing content for edited encrypted event #14252

richvdh opened this issue Oct 21, 2022 · 6 comments
Assignees
Labels
A-E2EE End-to-end encryption for Matrix clients A-Sync defects related to /sync O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Release-Blocker Must be resolved before making a release

Comments

@richvdh
Copy link
Member

richvdh commented Oct 21, 2022

According to @BillCarsonFr (element-hq/element-web#23550 (comment)):

  1. Send a message In a room with some history => the intitial sync has to be limited=truefor that room
  2. Edit it once
  3. Do a clear cache (initial sync)

The original event is then returned with with empty content, as if it's been redacted:

{
  "type": "m.room.encrypted",
  "sender": "@alice:localhost:8480",
  "content": {},
  "origin_server_ts": 1666342304414,
  "unsigned": {
    "age": 500276,
    "transaction_id": "m1666342304366.6",
    "m.relations": {
      "m.replace": {
        "event_id": "$toxW3soIUzZbG6cmnw94p0-cvQ5_AlIM0NJ7SJz7vpY",
        "origin_server_ts": 1666342320779,
        "sender": "@alice:localhost:8480"
      }
    }
  },
  "event_id": "$q6rQxsGaW0XscWvlnTUiWVIAcV07AMNZqZzPF3Sbga4"
}
@richvdh richvdh added S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. O-Occasional Affects or can be seen by some users regularly or most users rarely labels Oct 21, 2022
@richvdh richvdh self-assigned this Oct 21, 2022
@BillCarsonFr
Copy link
Member

BillCarsonFr commented Oct 21, 2022

On web, the edited message is shown in the timeline with a strange Re-request encryption keys.
image

On android the message edit is not even shown, just marked as unable to decrypt (you can see the edit history by using the context menu though)
image

on iOS the message is displayed but for some reason all the history is in fail to decrypt?
image

@richvdh
Copy link
Member Author

richvdh commented Oct 21, 2022

So, the problem here stems from an ambiguity in the spec.

https://spec.matrix.org/v1.4/client-server-api/#editing-encrypted-events says:

[for encrypted events], m.new_content is placed in the content of the encrypted payload.

(An alternative would have been to put the encrypted payload inside m.new_content, rather than the other way around, but the spec follows existing implementations (see matrix-org/matrix-spec-proposals#2676 (comment))).

We then have https://spec.matrix.org/v1.4/client-server-api/#server-side-replacement-of-content, which says:

Whenever an m.replace is to be bundled with an event as above, the server should also modify the content of the original event according to the m.new_content of the most recent replacement event.

Of course, the server doesn't have access to m.new_content. It turns out that, in this case, Synapse replaces the content with {}.

This behaviour changed in #14034, which updated Synapse to follow the spec in applying edits to encrypted events.

@richvdh
Copy link
Member Author

richvdh commented Oct 21, 2022

I've opened matrix-org/matrix-spec#1299 to track this on the spec side.

@BillCarsonFr
Copy link
Member

BillCarsonFr commented Oct 24, 2022

Looks like web can be affected in some cases
image

Original event (displayed in UTD in the screen shot)

{
  "content": {},
  ...
  "type": "m.room.encrypted",
  "unsigned": {
    "age": 177157836,
    "m.relations": {
      "m.replace": {
        "event_id": "$cbrzjtKuImUkE_QC3zRmfvoLFRpbyqmt9u8ZGGzUoLU",
        "origin_server_ts": 1666367697945,
        "sender": "@valere35:matrix.org"
      }
    }
  },
  "event_id": "$Oy6--KpT9fG2hIkk1toG-BabYwB5a5DeO8uN66g3gJQ",
  "user_id": "@valere35:matrix.org",
  "age": 177157836
}

And the actual edit event (visible because of dev mode) is decrypted correctly (eventID is $cbrzjtKuImUkE_QC3zRmfvoLFRpbyqmt9u8ZGGzUoLU and decrypted type is m.room.message)

Maybe the edit was received before the original event (back pagination / gappy sync?)

https://github.com/matrix-org/element-web-rageshakes/issues/16338

It's strange both message and edit were sent from this device (few seconds away). But somehow later I got back the message from server (probably a back pagination this morning following gappy sync?). Then the original message has been replaced with the one with empty content? and that did break the edit chain??

@clokep
Copy link
Member

clokep commented Oct 24, 2022

From our backend meeting today the decision was to backout the change for 1.70, see #14283.

The other option we had was to special-case encrypted events to be bundled, but not apply the edit content. We decided against this as we didn't want to introduce a non-specced behavior.

@clokep
Copy link
Member

clokep commented Oct 24, 2022

@richvdh I'm going to close this since the fix is now on the release branch.

I think once a conclusion is done in matrix-org/matrix-spec#1299 we can file a separate issue to implement?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-E2EE End-to-end encryption for Matrix clients A-Sync defects related to /sync O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Major functionality / product severely impaired, no satisfactory workaround. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Release-Blocker Must be resolved before making a release
Projects
None yet
Development

No branches or pull requests

4 participants