From 7470ed73c4a61367a7c78f2f3d083f0c6c0d4a0c Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 3 May 2024 12:01:51 -0600 Subject: [PATCH 1/3] Check native sliding sync support against an unstable feature flag The `OPTIONS` approach from https://github.com/matrix-org/matrix-react-sdk/pull/12492 doesn't work because Synapse *always* responds with 204 (success) to `OPTIONS` requests, as described here: https://github.com/element-hq/synapse/issues/17153 We further can't use `HEAD` because it's not part of the allowed CORS methods, meaning the browser will mask the exact status code and error message from us, and the proxy hangs on the request anyways: https://github.com/matrix-org/sliding-sync/pull/429 To avoid these problems, we instead search for an unstable feature flag to be exposed by the server. Presence of this flag denotes native support. See https://github.com/matrix-org/matrix-spec-proposals/pull/3575/files#r1588877046 for details. Implementations which support sliding sync natively will need to update to support this new unstable feature flag usage. --- src/SlidingSyncManager.ts | 23 ++++++-------------- test/SlidingSyncManager-test.ts | 38 ++++++--------------------------- 2 files changed, 13 insertions(+), 48 deletions(-) diff --git a/src/SlidingSyncManager.ts b/src/SlidingSyncManager.ts index 6f118d9b460..0059c81dac9 100644 --- a/src/SlidingSyncManager.ts +++ b/src/SlidingSyncManager.ts @@ -372,26 +372,17 @@ export class SlidingSyncManager { } /** - * Check if the server "natively" supports sliding sync (at the unstable endpoint). + * Check if the server "natively" supports sliding sync (with an unstable endpoint). * @param client The MatrixClient to use - * @return Whether the "native" (unstable) endpoint is up + * @return Whether the "native" (unstable) endpoint is supported */ public async nativeSlidingSyncSupport(client: MatrixClient): Promise { - try { - // We use OPTIONS to avoid causing a real sync to happen, as that may be intensive or encourage - // middleware software to start polling as our access token (thus stealing our to-device messages). - // See https://github.com/element-hq/element-web/issues/27426 - // XXX: Using client.http is a bad thing - it's meant to be private access. See `client.http` for details. - await client.http.authedRequest(Method.Options, "/sync", undefined, undefined, { - localTimeoutMs: 10 * 1000, // 10s - prefix: "/_matrix/client/unstable/org.matrix.msc3575", - }); - } catch (e) { - return false; // 404, M_UNRECOGNIZED + // Per https://github.com/matrix-org/matrix-spec-proposals/pull/3575/files#r1589542561 + const support = await client.doesServerSupportUnstableFeature("org.matrix.msc3575"); + if (support) { + logger.log("nativeSlidingSyncSupport: sliding sync advertised as unstable"); } - - logger.log("nativeSlidingSyncSupport: sliding sync endpoint is up"); - return true; // 200, OK + return support; } /** diff --git a/test/SlidingSyncManager-test.ts b/test/SlidingSyncManager-test.ts index bca00fd1454..51c7d755b44 100644 --- a/test/SlidingSyncManager-test.ts +++ b/test/SlidingSyncManager-test.ts @@ -16,8 +16,7 @@ limitations under the License. import { SlidingSync } from "matrix-js-sdk/src/sliding-sync"; import { mocked } from "jest-mock"; -import { IRequestOpts, MatrixClient, MatrixEvent, Method, Room } from "matrix-js-sdk/src/matrix"; -import { QueryDict } from "matrix-js-sdk/src/utils"; +import { MatrixClient, MatrixEvent, Room } from "matrix-js-sdk/src/matrix"; import { SlidingSyncManager } from "../src/SlidingSyncManager"; import { stubClient } from "./test-utils"; @@ -261,41 +260,16 @@ describe("SlidingSyncManager", () => { it("should make an OPTIONS request to avoid unintended side effects", async () => { // See https://github.com/element-hq/element-web/issues/27426 - // Developer note: We mock this in a truly terrible way because of how the call is done. There's not - // really much we can do to avoid it. - client.http = { - async authedRequest( - method: Method, - path: string, - queryParams?: QueryDict, - body?: Body, - paramOpts: IRequestOpts & { - doNotAttemptTokenRefresh?: boolean; - } = {}, - ): Promise { - // XXX: Ideally we'd use ResponseType<> like in the real thing, but it's not exported - expect(method).toBe(Method.Options); - expect(path).toBe("/sync"); - expect(queryParams).toBeUndefined(); - expect(body).toBeUndefined(); - expect(paramOpts).toEqual({ - localTimeoutMs: 10 * 1000, // 10s - prefix: "/_matrix/client/unstable/org.matrix.msc3575", - }); - return {}; - }, - } as any; - + const unstableSpy = jest.spyOn(client, "doesServerSupportUnstableFeature").mockImplementation(async (feature: string) => { + expect(feature).toBe("org.matrix.msc3575"); + return true; + }); const proxySpy = jest.spyOn(manager, "getProxyFromWellKnown").mockResolvedValue("proxy"); expect(SlidingSyncController.serverSupportsSlidingSync).toBeFalsy(); - await manager.checkSupport(client); // first thing it does is call nativeSlidingSyncSupport - - // Note: if this expectation is failing, it may mean the authedRequest mock threw an expectation failure - // which got consumed by `nativeSlidingSyncSupport`. Log your errors to discover more. expect(proxySpy).not.toHaveBeenCalled(); - + expect(unstableSpy).toHaveBeenCalled(); expect(SlidingSyncController.serverSupportsSlidingSync).toBeTruthy(); }); }); From 2773a1051282c677b7bf0a1c1934570155b5097d Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 3 May 2024 12:04:30 -0600 Subject: [PATCH 2/3] Appease the linter --- test/SlidingSyncManager-test.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/SlidingSyncManager-test.ts b/test/SlidingSyncManager-test.ts index 51c7d755b44..d93de8f5721 100644 --- a/test/SlidingSyncManager-test.ts +++ b/test/SlidingSyncManager-test.ts @@ -260,10 +260,12 @@ describe("SlidingSyncManager", () => { it("should make an OPTIONS request to avoid unintended side effects", async () => { // See https://github.com/element-hq/element-web/issues/27426 - const unstableSpy = jest.spyOn(client, "doesServerSupportUnstableFeature").mockImplementation(async (feature: string) => { - expect(feature).toBe("org.matrix.msc3575"); - return true; - }); + const unstableSpy = jest + .spyOn(client, "doesServerSupportUnstableFeature") + .mockImplementation(async (feature: string) => { + expect(feature).toBe("org.matrix.msc3575"); + return true; + }); const proxySpy = jest.spyOn(manager, "getProxyFromWellKnown").mockResolvedValue("proxy"); expect(SlidingSyncController.serverSupportsSlidingSync).toBeFalsy(); From 21c7b5faaa18de4957f8f56bb6398ef1992fb231 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 3 May 2024 12:23:02 -0600 Subject: [PATCH 3/3] Appease the tests --- src/SlidingSyncManager.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/SlidingSyncManager.ts b/src/SlidingSyncManager.ts index 0059c81dac9..8ccdfe868cf 100644 --- a/src/SlidingSyncManager.ts +++ b/src/SlidingSyncManager.ts @@ -378,7 +378,8 @@ export class SlidingSyncManager { */ public async nativeSlidingSyncSupport(client: MatrixClient): Promise { // Per https://github.com/matrix-org/matrix-spec-proposals/pull/3575/files#r1589542561 - const support = await client.doesServerSupportUnstableFeature("org.matrix.msc3575"); + // `client` can be undefined/null in tests for some reason. + const support = await client?.doesServerSupportUnstableFeature("org.matrix.msc3575"); if (support) { logger.log("nativeSlidingSyncSupport: sliding sync advertised as unstable"); }