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

Prefix the user+device state key if needed #4262

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Conversation

AndrewFerr
Copy link
Member

@AndrewFerr AndrewFerr commented Jun 21, 2024

No need to prefix it for rooms that use MSC3779.
Otherwise, prefix it to bypass the auth rule for state events with keys starting with @.

Signed-off-by: Andrew Ferrazzutti [email protected]

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

src/matrixrtc/MatrixRTCSession.ts Outdated Show resolved Hide resolved
No need to prefix it for rooms that use MSC3779.
Otherwise, prefix it to bypass the auth rule for state events with keys
starting with @.
Sonar typescript:S6594
@AndrewFerr
Copy link
Member Author

Test coverage on new code is 100%, so barring review changes, this PR is ready to go.

@AndrewFerr AndrewFerr self-assigned this Jun 21, 2024
Comment on lines +904 to +908
if (/^org\.matrix\.msc3779\b/.exec(this.room.getVersion())) {
return stateKey;
} else {
return `_${stateKey}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah nice preparing for the owned state events is a smart idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to give background on this: We never use the state key to actually compute the state events.
Which is somewhat broken. In theory a proper state event has to use a spec conform key.
With our implementation this is not checked (we could in theory send multiple member events for one user for the same device by using random state keys.)
This is what allows to only update the part where we compute the membership.

We probably want to check if the state key is to spec before parsing a state event.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added an issue here to track this: #4265

@AndrewFerr AndrewFerr added this pull request to the merge queue Jun 21, 2024
Merged via the queue into develop with commit 25a7c9e Jun 21, 2024
27 checks passed
@AndrewFerr AndrewFerr deleted the af/prefix-state-key branch June 21, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants