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.

Sign up for GitHub

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

MSC4092: Enforce tests around sensitive parts of the specification #4092

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+86 −0
Open

MSC4092: Enforce tests around sensitive parts of the specification #4092

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions proposals/4092-enforce-tests.md
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Implementation requirements (in my opinion):

  • Multiple MSCs touching the affected areas electing to use the requirements of this MSC, demonstrating that the solution works.
  • Multiple clarifications, particularly those listed, electing to use the requirements of this MSC for the same reason.

Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
# MSC4092: Enforce tests around sensitive parts of the specification

*This MSC proposes a change in procedure to how proposals and clarifications land in the specification.
It does not add any new endpoints or alter the Matrix protocol in any way.*

The specification contains the rules that all clients and servers must follow in order to interoperate.
It is crucial that these rules are correct and consistent for the health of the Matrix ecosystem.
This MSC proposes enforcing extra requirements on proposals _and clarifications_ that modify these rules
in order to ensure that any changes remain consistent and correct.

## Proposal
Copy link
Member

Choose a reason for hiding this comment

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

If you needed a secondary example, the issues with restricted join rules (MSC3083) originally missing the needed changes to the redaction algorithm (and hence why room version 9 exists, see MSC3289).

See also some of the discussion in #4061.


Recently, it has become apparent that there are insufficient guard rails around changes to the specification,
such that [clarifications](https://github.com/matrix-org/matrix-spec/issues/1710) have been made to the
Copy link
Contributor

@deepbluev7 deepbluev7 Jan 24, 2024 •
edited by ara4n
Loading

Choose a reason for hiding this comment

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

(Summarizing @Gnuxie's comment as requested)

This issue currently summarizes chronologically, where the failure happened in that case: matrix-org/matrix-spec#1710

Specifically the original MSC and the original spec did match up correctly, but there have been several process failures to then make the current specification incorrect. First a diagram was modified incorrectly, then the spec was updated to match that diagram, while insufficiently verifying the original MSCs intent and currently implemented behaviour in servers. As such having tests to cover this behaviour would probably not be sufficient. Both in the case that the tests were modified to match the new spec text or they were not modified, because their logic was deemed to already match the new spec text, they might not have caught this issue.

Instead we may need to rethink our processes for specification clarifications and modifying the text of the specification in general. In the end the whole Matrix ecosystem relies on the specification to be correct, but we have a stricter review process for proposals than we have for the specification. Possibly we need to have some variation of "SCT ticks" for any change made to the specifications text as well, even if clarifications seem small and obvious, in this case they were the source of a major mistake in the specification. We need to ensure that the specification in itself is consistent to be able to rely on tests matching what the specification says.

Additionally we may need to be stricter on modifications to older room versions. We have often accepted those to be problematic and decided to not modify them, but in this case we did modify them, believing we just fixed a minor issue in the specification text, while other times such changes did go through an MSC.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for summarizing; agreed on all of this. I'm still not sure this is the right MSC to fix this (and it's debugging the specific failure mode of #1710) - perhaps one solution would be double-review on spec PRs. (Checkboxes feels overkill to me, though). We should also spell out the policy on modifying old room versions explicitly (if we don't already, but I don't think we do).

specification incorrectly. These changes can have security implications on the entire Matrix ecosystem. This
proposal proposes that any changes to the following areas of the specification MUST have end-to-end testing in
at least one homeserver implementation:
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this work with sytest/complement? Does that count? Since its not really one test per HS but one per practically all mainline HSs. Afaik all major ones currently test against complement and sytest currently. Does that count or is this about tests within the HS itself?

Additionally: Will there from foundation be sufficient support to get these PRs landed in those tools? In the past it felt like there wasnt sufficient manpower to takle issues/bugs/PRs on that front.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or in other words, it feels like sytest/complement currently is touched if the dendrite or synapse devs have to touch it anyway. It seems nearly impossible to reach support as a community member working on a non "the big 3" (synapse, dendrite, conduit) implementation or to report bugs. :/ So I realistically don't see how MSC requirements will work without any way to reach devs for it whatsoever currently. That will end up simply blocking MSCs for years again. SCT made great progress to not do infinite blocking anymore so imho we need reliable complement/sytest or other alternatives before this MSC can move forward.

An alternative being that people will write random scripts as tests for their MSC. That doesn't seem to be the intention of anyone here, I think.

And yes, the above is frustration speaking. Sorry to dump that in here. But it's the topic of the MSC somewhat.

Copy link
Contributor

Choose a reason for hiding this comment

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

And to clarify: I really want this MSC. Its long missing and a major improvement to the process thats fundamentally necessary. I just dont see it possible unless there exist the necessary resources to actually do this.

Copy link
Member

Choose a reason for hiding this comment

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

Sytest has 11 open PRs now and 1213 closed ones - and complement has 11 open PRs now and 575 closed ones (all of which are in some state of review). So while we don't have unlimited bandwidth available, in practice it feels like it makes reasonable forwards progress - certainly better than many other projects on github.com/matrix-org

- Room Versions
- Server-Server API:
* Authentication
* PDUs
* Room State Resolution
* Signing Events
* Joining Rooms

All of these areas are exclusive to server implementations. This means end-to-end testing MUST test with a
real homeserver implementation.
Comment on lines +26 to +27
Copy link
Member

Choose a reason for hiding this comment

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

Room versions aren't exclusively server implementations. Clients must know about redaction rules, for example.


In practice, the ecosystem currently has the following end-to-end testing frameworks:
- [Sytest](https://github.com/matrix-org/sytest)
- [Complement](https://github.com/matrix-org/complement)

As such, **this proposal would force any proposal or clarification which touches the named areas of the specification
to have tests** in one of these projects. However, any end-to-end testing framework would suffice, at the
spec core team's discretion.

## Potential issues

This adds extra barriers for contributors, who now need to write code in order to get their proposal landed into
the specification. However, this has always been the case as the MSC proposal demands that there is a working
client and server implementation. In practice, these implementations _should have tests_ so the actual impact on
the MSC process is reduced.

However, this has significant implications on clarifications which typically do not have the same level of rigour
applied to them as the proposal process. This would likely result in fewer clarifications being made to these areas,
but when they are made they are more likely to be correct. The reduction in the number of clarifications to these
areas may outweigh the extra safety guarantees this proposal is attempting to make.


## Alternatives

The areas of the specification which need extra guard rails can be changed to reduce friction. However,
these areas were chosen because they are critical to the security of Matrix rooms.

The guard rails being applied could be made more or less strict, allowing unit tests to suffice in place
of end-to-end tests. Alternatively, a robust justification could be sufficient, provided it is backed
with evidence. In cases where clarifications to the specification were incorrect, whilst a robust
justification _was proposed_, it was not backed by evidence to support the claims being made.


## Security considerations

This MSC _should_ increase security around the proposals and clarifications process.
Psychologically, there is a risk that reviewers may become less vigilant if there are tests. Tests
are error-prone and may not be testing what is described in the clarification/proposal.

## Affected clarifications

As of this writing, the following clarifications would now be subject to the extra guard rails. This is
not an exhaustive list:
- https://github.com/matrix-org/matrix-spec/issues/1708
- https://github.com/matrix-org/matrix-spec/issues/1642
- https://github.com/matrix-org/matrix-spec/issues/1569
- https://github.com/matrix-org/matrix-spec/issues/1515
- https://github.com/matrix-org/matrix-spec/issues/1514
- https://github.com/matrix-org/matrix-spec/issues/1482
- https://github.com/matrix-org/matrix-spec/issues/1373
- https://github.com/matrix-org/matrix-spec/issues/1247
- https://github.com/matrix-org/matrix-spec/issues/1246
- https://github.com/matrix-org/matrix-spec/issues/1244
- https://github.com/matrix-org/matrix-spec/issues/1136
- https://github.com/matrix-org/matrix-spec/issues/1098
- https://github.com/matrix-org/matrix-spec/issues/1061
- https://github.com/matrix-org/matrix-spec/issues/1048
- https://github.com/matrix-org/matrix-spec/issues/1046
- https://github.com/matrix-org/matrix-spec/issues/849
Loading