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

It is too easy to accidentally log message contents in rageshakes #26380

Open
richvdh opened this issue Oct 16, 2023 · 3 comments
Open

It is too easy to accidentally log message contents in rageshakes #26380

richvdh opened this issue Oct 16, 2023 · 3 comments
Labels
A-Developer-Experience O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Task Tasks for the team like planning

Comments

@richvdh
Copy link
Member

richvdh commented Oct 16, 2023

There have been numerous occasions over the years where we accidentally include message contents in rageshakes, most recently #26376.

A common failure mode is to write something like:

let event = getEventFromSomewhere();
// ...
logger.info("Processing event", e);

What happens then is that we call JSON.stringify (https://github.com/matrix-org/matrix-react-sdk/blob/v3.82.0/src/rageshake/rageshake.ts#L94) which in turn calls MatrixEvent.toJSON, which includes the message content.

I assert that we should either:

  • change the existing consumers of MatrixEvent.toJSON to use some other method, then update toJSON to elide message content
  • special-case MatrixEvent in rageshake.log
@germain-gg germain-gg added T-Task Tasks for the team like planning S-Major Severely degrades major functionality or product features, with no satisfactory workaround A-Developer-Experience O-Occasional Affects or can be seen by some users regularly or most users rarely labels Oct 17, 2023
@richvdh
Copy link
Member Author

richvdh commented Oct 17, 2023

On the known call sites of MatrixEvent.toJSON.

  • I don't believe that "view event source" does call MatrixEvent.toJSON. The relevant code is here and, as you can see, it introspects the event and clearEvent properties of MatrixEvent (which it then JSON.stringify()s), rather than calling .toJSON on the event itself.
  • The relevant code for seshat is in EventIndex. I think it should just call MatrixEvent.getEffectiveEvent() rather than undoing the work of MatrixEvent.toJSON.

richvdh added a commit to matrix-org/matrix-react-sdk that referenced this issue Oct 17, 2023
Part of the solution to element-hq/element-web#26380:
`toJSON` is dangerous, and I'd like to kill it off. There is no need for it
here; it is simpler to call `getEffectiveEvent` directly.
richvdh added a commit to matrix-org/matrix-js-sdk that referenced this issue Oct 17, 2023
Per element-hq/element-web#26380, this method is too
easy to use accidentally, and per the comments, it doesn't even return a
meaningful JSON-serialisation of the object.
github-merge-queue bot pushed a commit to matrix-org/matrix-react-sdk that referenced this issue Oct 17, 2023
Part of the solution to element-hq/element-web#26380:
`toJSON` is dangerous, and I'd like to kill it off. There is no need for it
here; it is simpler to call `getEffectiveEvent` directly.
github-merge-queue bot pushed a commit to matrix-org/matrix-js-sdk that referenced this issue Oct 17, 2023
* Deprecate `MatrixEvent.toJSON`

Per element-hq/element-web#26380, this method is too
easy to use accidentally, and per the comments, it doesn't even return a
meaningful JSON-serialisation of the object.

* Update src/models/event.ts
@richvdh
Copy link
Member Author

richvdh commented Oct 18, 2023

I now believe there are no legitimate callers of MatrixEvent.toJSON. We should give it a release cycle or two, and then remove it (or replace it with some safe version).

@richvdh
Copy link
Member Author

richvdh commented Oct 24, 2023

I'd love not to be responsible for taking this further :)

@richvdh richvdh removed their assignment Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Developer-Experience O-Occasional Affects or can be seen by some users regularly or most users rarely S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Task Tasks for the team like planning
Projects
None yet
Development

No branches or pull requests

2 participants