From 805165fc936932fd800822fa76b1a41d7db29e09 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 2 May 2024 17:07:57 -0600 Subject: [PATCH 1/3] Use OPTIONS for sliding sync detection poke This avoids unintended consequences, including high resource usage, which would accompany a "full" sync request. Instead, we just grab headers and enough information for CORS to pass, revealing likely support. Fixes https://github.com/element-hq/element-web/issues/27426 --- src/SlidingSyncManager.ts | 6 +++++- test/SlidingSyncManager-test.ts | 38 ++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/src/SlidingSyncManager.ts b/src/SlidingSyncManager.ts index e3f420b43d5..6f118d9b460 100644 --- a/src/SlidingSyncManager.ts +++ b/src/SlidingSyncManager.ts @@ -378,7 +378,11 @@ export class SlidingSyncManager { */ public async nativeSlidingSyncSupport(client: MatrixClient): Promise { try { - await client.http.authedRequest(Method.Post, "/sync", undefined, undefined, { + // 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", }); diff --git a/test/SlidingSyncManager-test.ts b/test/SlidingSyncManager-test.ts index 757a682d848..6be39f86ee6 100644 --- a/test/SlidingSyncManager-test.ts +++ b/test/SlidingSyncManager-test.ts @@ -16,7 +16,8 @@ limitations under the License. import { SlidingSync } from "matrix-js-sdk/src/sliding-sync"; import { mocked } from "jest-mock"; -import { MatrixClient, MatrixEvent, Room } from "matrix-js-sdk/src/matrix"; +import { IRequestOpts, MatrixClient, MatrixEvent, Method, Room } from "matrix-js-sdk/src/matrix"; +import { QueryDict } from "matrix-js-sdk/src/utils"; import { SlidingSyncManager } from "../src/SlidingSyncManager"; import { stubClient } from "./test-utils"; @@ -253,6 +254,41 @@ describe("SlidingSyncManager", () => { expect(SlidingSyncController.serverSupportsSlidingSync).toBeTruthy(); }); }); + describe("nativeSlidingSyncSupport", () => { + 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 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(SlidingSyncController.serverSupportsSlidingSync).toBeTruthy(); + }); + }); describe("setup", () => { beforeEach(() => { jest.spyOn(manager, "configure"); From 24f68c0dbb8a36e5cf94b2f3cb4034d1fefb7179 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Thu, 2 May 2024 17:11:23 -0600 Subject: [PATCH 2/3] Appease the linter --- test/SlidingSyncManager-test.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/test/SlidingSyncManager-test.ts b/test/SlidingSyncManager-test.ts index 6be39f86ee6..71c886ab154 100644 --- a/test/SlidingSyncManager-test.ts +++ b/test/SlidingSyncManager-test.ts @@ -261,9 +261,16 @@ describe("SlidingSyncManager", () => { // 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 + 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(); From d412b6ebf8d72bc0518d8953c815249999b13e02 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Fri, 3 May 2024 00:10:48 -0600 Subject: [PATCH 3/3] Reset for each test --- test/SlidingSyncManager-test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/SlidingSyncManager-test.ts b/test/SlidingSyncManager-test.ts index 71c886ab154..bca00fd1454 100644 --- a/test/SlidingSyncManager-test.ts +++ b/test/SlidingSyncManager-test.ts @@ -255,6 +255,9 @@ describe("SlidingSyncManager", () => { }); }); describe("nativeSlidingSyncSupport", () => { + beforeEach(() => { + SlidingSyncController.serverSupportsSlidingSync = false; + }); it("should make an OPTIONS request to avoid unintended side effects", async () => { // See https://github.com/element-hq/element-web/issues/27426