Check for version is not allowing newer versions than the minimum version

[MTRNord]

The check at:

matrix-js-sdk/src/autodiscovery.ts

Line 220 in a749662

if (!hsVersions.raw!["versions"].includes(MINIMUM_MATRIX_VERSION)) {

strictly requires a version to be in the response. However defining a newer version only according to https://spec.matrix.org/v1.8/client-server-api/#get_matrixclientversions is allowed to be set alone. And by nature of versions also afaik includes the older versions too.

TLDR: Shouldnt this be a greater than rather than a "includes" check? The spec doesnt mandate a server to include any prior version.

See also https://matrix.to/#/!NasysSDfxKxZBzJJoE:matrix.org/$VA5KRc9PIeXmk0Sot2AAmU7UW6BrlcRkTGm9XROR-lA?via=matrix.org&via=element.io&via=envs.net for a discussion on this in the spec room. Nheko for example seems to check 1.1-1.5 as a range check instead.

[MTRNord]

https://github.com/search?q=repo%3Amatrix-org%2Fmatrix-js-sdk+%2Fv1%5C.%5Cd%2F+path%3Asrc%2F&type=code as linked by @clokep in the spec room suggests that js-sdk might actually want to check for minimum support of 1.1 instead of "including and more" as it currently does.

[t3chguy]

However defining a newer version only according to spec.matrix.org/v1.8/client-server-api/#get_matrixclientversions is allowed to be set alone.

Where does it say that?

[MTRNord]

However defining a newer version only according to spec.matrix.org/v1.8/client-server-api/#get_matrixclientversions is allowed to be set alone.

Where does it say that?

It doesnt explicitly say that. It however also doesnt require that you have to list the older versions. So this is practically open for interpretation.

With:

Note that while v1.2 is meant to be backwards compatible with v1.1, it is not guaranteed that future versions will be fully backwards compatible with v1.1. For example, if /test were to be introduced in v1.1 and deprecated in v1.2, then it can be removed in v1.3. More information about this is described in the deprecation policy below.

In https://spec.matrix.org/v1.8/#specification-versions it might be that servers actually would need to implement 1.1 side by side for it to be working with js-sdk but I doubt this is the intention of either the sdk or the spec.

[t3chguy]

So this is practically open for interpretation.

Sounds like time for a spec clarification issue

[richvdh]

@turt2live has clarified: it should not be a range check; however we should bump the minimum.

[richvdh]

I spent a lot longer talking to @turt2live about this this evening, trying to understand how the /versions check is supposed to work, and the constraints around the js-sdk in particular.

Some conclusions:

1. The reason that js-sdk has been stuck only supporting v1.1 for years on end is that nobody has found the time to check that it actually works ok on a server that doesn't support v1.1. (As soon as we accept another version, say v1.2, it opens the possibility of someone using it with a server that doesn't support v1.1 and hence, for example, some endpoint that the js-sdk relies on).

   However, I think we have to accept that this lack of time is unlikely to change soon, and the current situation is untenable. In my opinion: better that we just bump the "supported versions" based on the best of our knowledge and treat any problems we discover as bugs, rather than live frozen in fear that there could be a bug. Hence: Bump minimum spec version to v1.5 #3970.

2. In general, it makes sense for clients to accept /versions responses that include any spec version the client is compatible with. (In other words, the client should have a list of "acceptable" versions and check for any overlap with the server's response list.)

   This allows those clients to be compatible with servers that only want to support the bleeding-edge of the spec.

   (We encourage both clients and servers to be moderately tolerant with what they support - up to a year old is, in our opinion, a good compromise between maintaining compatibility and keeping the ecosystem moving.)

   Accordingly, we should update the js-sdk to accept v1.6, v1.7, v1.8 and v1.9 in addition to the change above. It has the same problem as before: we have no practical means to test every code path. But that's not going to change in the next year, so we might as well bite the bullet and go ahead. That said: I'm not going to do it yet, largely because I need to work on something else today.

3. It does not make sense for clients to accept /versions responses which contain only "unknown" versions (such as 1.10, currently.) We don't yet know what will be in v1.10, so we don't know what a server supporting only that version will require of a client.

   In other words, no this should not be a "greater than" rather than a "includes" check.

4. Room versions are a whole other ball game that /versions does very little to help you with.

[TheArcaneBrony]

On the topic of bullet point 4, aren't room versions usually accompanied with a spec release?
Might be worth discussing adding room versions to /versions? (Would also help the "room upgrade check" bots, for what it's worth).

[richvdh]

@TheArcaneBrony: let's not get distracted with the off-topic subject of room versions.