MSC4092: Enforce tests around sensitive parts of the specification #4092
Conversation
There was a problem hiding this comment.
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.
turt2live
added
proposal
| such that [clarifications](https://github.com/matrix-org/matrix-spec/issues/1710) have been made to the | ||
| 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| ## Proposal | ||
|
|
||
| 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 |
There was a problem hiding this comment.
(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.
There was a problem hiding this comment.
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).
| All of these areas are exclusive to server implementations. This means end-to-end testing MUST test with a | ||
| real homeserver implementation. |
There was a problem hiding this comment.
Room versions aren't exclusively server implementations. Clients must know about redaction rules, for example.
| 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 |
Rendered