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
matrixbot opened this issue Oct 31, 2024 · 3 comments
Open

We should not use depth in the sync API #967

matrixbot opened this issue Oct 31, 2024 · 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

@matrixbot
Copy link
Collaborator

This issue was originally created by @neilalexander at matrix-org/dendrite#967.

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.)

@matrixbot matrixbot added 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 labels Oct 31, 2024
@matrixbot
Copy link
Collaborator Author

This comment was originally posted by @kegsay at matrix-org/dendrite#967 (comment).

Reference: matrix-org/gomatrixserverlib#187

@matrixbot
Copy link
Collaborator Author

This comment was originally posted by @ara4n at matrix-org/dendrite#967 (comment).

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.

@matrixbot
Copy link
Collaborator Author

This comment was originally posted by @neilalexander at matrix-org/dendrite#967 (comment).

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.

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

1 participant