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

We should not use depth in the sync API #967

Open
neilalexander opened this issue Apr 15, 2020 · 3 comments
Open

We should not use depth in the sync API #967

neilalexander opened this issue Apr 15, 2020 · 3 comments
Labels
C-Sync-API T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Needs-Discussion We aren't really sure about this yet so let's talk about it some more

Comments

@neilalexander
Copy link
Contributor

Currently the sync API uses the event depth when inserting events into the topology, which we should really not do since depth is deprecated.

(Incidentally, we probably need to come up with some better way to maintain topological ordering with inserts.)

@kegsay
Copy link
Member

kegsay commented May 5, 2020

Reference: matrix-org/gomatrixserverlib#187

@ara4n
Copy link
Member

ara4n commented Sep 26, 2020

There are other places we use depth too - e.g. federationapi /get_missing_events and even /send. This isn't a problem just because depth is deprecated - it's a security problem, because depth simply can't be trusted from the wire. I believe we have to use the value returned by v2 state res.

I think we might have to fix this before beta, given it's a known sec issue.

@ara4n ara4n added this to the Beta milestone Sep 26, 2020
@neilalexander
Copy link
Contributor Author

Depth appears in the federation API when we try to get missing events, largely because the /get_missing_events endpoint specifies the min_depth field. Although from what I can tell, this is an entirely superficial filter and there would not really be any ill effect if we just ignored the depth values and always used 0 both inbound and outbound.

@kegsay kegsay removed this from the Beta milestone Oct 5, 2020
@kegsay kegsay added X-Needs-Discussion We aren't really sure about this yet so let's talk about it some more T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. labels Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Sync-API T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Needs-Discussion We aren't really sure about this yet so let's talk about it some more
Projects
None yet
Development

No branches or pull requests

3 participants