-
Notifications
You must be signed in to change notification settings - Fork 395
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
MSC3316: Add timestamp massaging to the spec #3316
Merged
turt2live
merged 2 commits into
matrix-org:old_master
from
tulir:appservice-timestamp-massaging
Mar 6, 2022
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
# Proposal to add timestamp massaging to the spec | ||
Bridges often want to override message timestamps to preserve the timestamps from | ||
the remote network. The spec used to have a concept of [timestamp massaging], but | ||
it was excluded from the release due to not being properly specified. Synapse | ||
still implements it and it is widely used in bridges. | ||
|
||
[MSC2716] was originally going to add timestamp massaging to the spec, but it | ||
pivoted to focusing solely on batch sending history. This MSC simply copies the | ||
proposed `ts` query param from the [original MSC2716]. | ||
|
||
[timestamp massaging]: https://matrix.org/docs/spec/application_service/r0.1.2#timestamp-massaging | ||
[MSC2716]: https://github.com/matrix-org/matrix-doc/pull/2716 | ||
[original MSC2716]: https://github.com/matrix-org/matrix-doc/blob/94514392b118dfae8ee6840b13b83d2f8ce8fcfc/proposals/2716-importing-history-into-existing-rooms.md | ||
|
||
## Proposal | ||
As per the original version of MSC2716: | ||
|
||
> We let the AS API override ('massage') the `origin_server_ts` timestamp | ||
> applied to sent events. We do this by adding a `ts` querystring parameter on | ||
> the `PUT /_matrix/client/r0/rooms/{roomId}/send/{eventType}/{txnId}` | ||
> and `PUT /_matrix/client/r0/rooms/{roomId}/state/{eventType}/{stateKey}` | ||
> endpoints, specifying the value to apply to `origin_server_ts` on the event | ||
> (UNIX epoch milliseconds). | ||
> | ||
> We consciously don't support the `ts` parameter on the various helper | ||
> syntactic-sugar APIs like /kick and /ban. If a bridge/bot is smart enough to | ||
> be faking history, it is already in the business of dealing with raw events, | ||
> and should not be using the syntactic sugar APIs. | ||
|
||
The spec should also make it clear that the `ts` query param won't affect DAG | ||
ordering, and MSC2716's batch sending should be used when the intention is to | ||
insert history somewhere else than the end of the room. | ||
|
||
## Potential issues | ||
None. | ||
|
||
## Alternatives | ||
The new MSC2716 could technically be considered an alternative, but it can only | ||
be used for history, while this proposal also supports overriding timestamps of | ||
new messages. In practice, bridges will likely use both: Batch sending for | ||
filling history and timestamp massaging for new messages. | ||
|
||
## Security considerations | ||
Timestamps should already be considered untrusted over federation, and | ||
application services are trusted server components, so allowing appservices | ||
to override timestamps does not create any new security considerations. | ||
|
||
## Unstable prefix | ||
`org.matrix.msc3316.ts` may be used as the query parameter. However, the `ts` | ||
parameter is already used in production for the `/send` endpoint, which means | ||
the unstable prefix should only be used for the `/state` endpoint. |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for additional context: support for
ts
on/send
was added to Synapse way back in https://github.com/matrix-org/synapse/pull/2754/files. Since that was so long ago, there is a strong argument that an MSC isn't needed to add it to the spec.(Though of course
/state
still needs it)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An MSC is needed because it was actively removed from the spec in Matrix 1.0. Timestamp massaging was originally there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context: #1586