Skip to content

Commit 546047a

Browse files
AndrewFerrrichvdh
AndrewFerr
and
richvdh
authored
Capture HTTP error response headers & handle Retry-After header (MSC4041) (#4471)
* Include HTTP response headers in MatrixError * Lint * Support MSC4041 / Retry-After header * Fix tests * Remove redundant MatrixError parameter properties They are inherited from HTTPError, so there is no need to mark them as parameter properties. * Comment that retry_after_ms is deprecated * Properly handle colons in XHR header values Also remove the negation in the if-condition for better readability * Improve Retry-After parsing and docstring * Revert ternary operator to if statements for readability * Reuse resolved Headers for Content-Type parsing * Treat empty Content-Type differently from null * Add MatrixError#isRateLimitError This is separate from MatrixError#getRetryAfterMs because it's possible for a rate-limit error to have no Retry-After time, and having separate methods to check each makes that more clear. * Ignore HTTP status code when getting Retry-After because status codes other than 429 may have Retry-After * Catch Retry-After parsing errors * Add test coverage for HTTP error headers * Update license years * Move safe Retry-After lookup to global function so it can more conveniently check if an error is a MatrixError * Lint * Inline Retry-After header value parsing as it is only used in one place and doesn't need to be exported * Update docstrings Co-authored-by: Richard van der Hoff <[email protected]> * Use bare catch Co-authored-by: Richard van der Hoff <[email protected]> * Give HTTPError methods for rate-limit checks and make MatrixError inherit them * Cover undefined errcode in rate-limit check * Update safeGetRetryAfterMs docstring Be explicit that errors that don't look like rate-limiting errors will not pull a retry delay value from the error. * Use rate-limit helper functions in more places * Group the header tests --------- Co-authored-by: Richard van der Hoff <[email protected]>
1 parent 16153e5 commit 546047a

File tree

8 files changed

+342
-79
lines changed

8 files changed

+342
-79
lines changed

‎spec/unit/http-api/errors.spec.ts‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/*
2+
Copyright 2024 The Matrix.org Foundation C.I.C.
3+
4+
Licensed under the Apache License, Version 2.0 (the "License");
5+
you may not use this file except in compliance with the License.
6+
You may obtain a copy of the License at
7+
8+
http://www.apache.org/licenses/LICENSE-2.0
9+
10+
Unless required by applicable law or agreed to in writing, software
11+
distributed under the License is distributed on an "AS IS" BASIS,
12+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
See the License for the specific language governing permissions and
14+
limitations under the License.
15+
*/
16+
17+
import { MatrixError } from "../../../src";
18+
19+
type IErrorJson = MatrixError["data"];
20+
21+
describe("MatrixError", () => {
22+
let headers: Headers;
23+
24+
beforeEach(() => {
25+
headers = new Headers({ "Content-Type": "application/json" });
26+
});
27+
28+
function makeMatrixError(httpStatus: number, data: IErrorJson): MatrixError {
29+
return new MatrixError(data, httpStatus, undefined, undefined, headers);
30+
}
31+
32+
it("should accept absent retry time from rate-limit error", () => {
33+
const err = makeMatrixError(429, { errcode: "M_LIMIT_EXCEEDED" });
34+
expect(err.isRateLimitError()).toBe(true);
35+
expect(err.getRetryAfterMs()).toEqual(null);
36+
});
37+
38+
it("should retrieve retry_after_ms from rate-limit error", () => {
39+
const err = makeMatrixError(429, { errcode: "M_LIMIT_EXCEEDED", retry_after_ms: 150000 });
40+
expect(err.isRateLimitError()).toBe(true);
41+
expect(err.getRetryAfterMs()).toEqual(150000);
42+
});
43+
44+
it("should ignore retry_after_ms if errcode is not M_LIMIT_EXCEEDED", () => {
45+
const err = makeMatrixError(429, { errcode: "M_UNKNOWN", retry_after_ms: 150000 });
46+
expect(err.isRateLimitError()).toBe(true);
47+
expect(err.getRetryAfterMs()).toEqual(null);
48+
});
49+
50+
it("should retrieve numeric Retry-After header from rate-limit error", () => {
51+
headers.set("Retry-After", "120");
52+
const err = makeMatrixError(429, { errcode: "M_LIMIT_EXCEEDED", retry_after_ms: 150000 });
53+
expect(err.isRateLimitError()).toBe(true);
54+
// prefer Retry-After header over retry_after_ms
55+
expect(err.getRetryAfterMs()).toEqual(120000);
56+
});
57+
58+
it("should retrieve Date Retry-After header from rate-limit error", () => {
59+
headers.set("Retry-After", `${new Date(160000).toUTCString()}`);
60+
jest.spyOn(global.Date, "now").mockImplementationOnce(() => 100000);
61+
const err = makeMatrixError(429, { errcode: "M_LIMIT_EXCEEDED", retry_after_ms: 150000 });
62+
expect(err.isRateLimitError()).toBe(true);
63+
// prefer Retry-After header over retry_after_ms
64+
expect(err.getRetryAfterMs()).toEqual(60000);
65+
});
66+
67+
it("should prefer M_FORBIDDEN errcode over HTTP status code 429", () => {
68+
headers.set("Retry-After", "120");
69+
const err = makeMatrixError(429, { errcode: "M_FORBIDDEN" });
70+
expect(err.isRateLimitError()).toBe(false);
71+
// retrieve Retry-After header even for non-M_LIMIT_EXCEEDED errors
72+
expect(err.getRetryAfterMs()).toEqual(120000);
73+
});
74+
75+
it("should prefer M_LIMIT_EXCEEDED errcode over HTTP status code 400", () => {
76+
headers.set("Retry-After", "120");
77+
const err = makeMatrixError(400, { errcode: "M_LIMIT_EXCEEDED" });
78+
expect(err.isRateLimitError()).toBe(true);
79+
// retrieve Retry-After header even for non-429 errors
80+
expect(err.getRetryAfterMs()).toEqual(120000);
81+
});
82+
83+
it("should reject invalid Retry-After header", () => {
84+
for (const invalidValue of ["-1", "1.23", new Date(0).toString()]) {
85+
headers.set("Retry-After", invalidValue);
86+
const err = makeMatrixError(429, { errcode: "M_LIMIT_EXCEEDED" });
87+
expect(() => err.getRetryAfterMs()).toThrow(
88+
"value is not a valid HTTP-date or non-negative decimal integer",
89+
);
90+
}
91+
});
92+
93+
it("should reject too-large Retry-After header", () => {
94+
headers.set("Retry-After", "1" + Array(500).fill("0").join(""));
95+
const err = makeMatrixError(429, { errcode: "M_LIMIT_EXCEEDED" });
96+
expect(() => err.getRetryAfterMs()).toThrow("integer value is too large");
97+
});
98+
});

‎spec/unit/http-api/index.spec.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2022 The Matrix.org Foundation C.I.C.
2+
Copyright 2022 - 2024 The Matrix.org Foundation C.I.C.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
@@ -41,6 +41,7 @@ describe("MatrixHttpApi", () => {
4141
setRequestHeader: jest.fn(),
4242
onreadystatechange: undefined,
4343
getResponseHeader: jest.fn(),
44+
getAllResponseHeaders: jest.fn(),
4445
} as unknown as XMLHttpRequest;
4546
// We stub out XHR here as it is not available in JSDOM
4647
// @ts-ignore
@@ -171,7 +172,10 @@ describe("MatrixHttpApi", () => {
171172
xhr.readyState = DONE;
172173
xhr.responseText = '{"errcode": "M_NOT_FOUND", "error": "Not found"}';
173174
xhr.status = 404;
174-
mocked(xhr.getResponseHeader).mockReturnValue("application/json");
175+
mocked(xhr.getResponseHeader).mockImplementation((name) =>
176+
name.toLowerCase() === "content-type" ? "application/json" : null,
177+
);
178+
mocked(xhr.getAllResponseHeaders).mockReturnValue("content-type: application/json\r\n");
175179
// @ts-ignore
176180
xhr.onreadystatechange?.(new Event("test"));
177181

‎spec/unit/http-api/utils.spec.ts‎

Lines changed: 105 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
Copyright 2022 The Matrix.org Foundation C.I.C.
2+
Copyright 2022 - 2024 The Matrix.org Foundation C.I.C.
33
44
Licensed under the Apache License, Version 2.0 (the "License");
55
you may not use this file except in compliance with the License.
@@ -86,13 +86,28 @@ describe("anySignal", () => {
8686
});
8787

8888
describe("parseErrorResponse", () => {
89+
let headers: Headers;
90+
const xhrHeaderMethods = {
91+
getResponseHeader: (name: string) => headers.get(name),
92+
getAllResponseHeaders: () => {
93+
let allHeaders = "";
94+
headers.forEach((value, key) => {
95+
allHeaders += `${key.toLowerCase()}: ${value}\r\n`;
96+
});
97+
return allHeaders;
98+
},
99+
};
100+
101+
beforeEach(() => {
102+
headers = new Headers();
103+
});
104+
89105
it("should resolve Matrix Errors from XHR", () => {
106+
headers.set("Content-Type", "application/json");
90107
expect(
91108
parseErrorResponse(
92109
{
93-
getResponseHeader(name: string): string | null {
94-
return name === "Content-Type" ? "application/json" : null;
95-
},
110+
...xhrHeaderMethods,
96111
status: 500,
97112
} as XMLHttpRequest,
98113
'{"errcode": "TEST"}',
@@ -108,14 +123,11 @@ describe("parseErrorResponse", () => {
108123
});
109124

110125
it("should resolve Matrix Errors from fetch", () => {
126+
headers.set("Content-Type", "application/json");
111127
expect(
112128
parseErrorResponse(
113129
{
114-
headers: {
115-
get(name: string): string | null {
116-
return name === "Content-Type" ? "application/json" : null;
117-
},
118-
},
130+
headers,
119131
status: 500,
120132
} as Response,
121133
'{"errcode": "TEST"}',
@@ -131,13 +143,12 @@ describe("parseErrorResponse", () => {
131143
});
132144

133145
it("should resolve Matrix Errors from XHR with urls", () => {
146+
headers.set("Content-Type", "application/json");
134147
expect(
135148
parseErrorResponse(
136149
{
137150
responseURL: "https://example.com",
138-
getResponseHeader(name: string): string | null {
139-
return name === "Content-Type" ? "application/json" : null;
140-
},
151+
...xhrHeaderMethods,
141152
status: 500,
142153
} as XMLHttpRequest,
143154
'{"errcode": "TEST"}',
@@ -154,15 +165,12 @@ describe("parseErrorResponse", () => {
154165
});
155166

156167
it("should resolve Matrix Errors from fetch with urls", () => {
168+
headers.set("Content-Type", "application/json");
157169
expect(
158170
parseErrorResponse(
159171
{
160172
url: "https://example.com",
161-
headers: {
162-
get(name: string): string | null {
163-
return name === "Content-Type" ? "application/json" : null;
164-
},
165-
},
173+
headers,
166174
status: 500,
167175
} as Response,
168176
'{"errcode": "TEST"}',
@@ -178,6 +186,66 @@ describe("parseErrorResponse", () => {
178186
);
179187
});
180188

189+
describe("with HTTP headers", () => {
190+
function addHeaders(headers: Headers) {
191+
headers.set("Age", "0");
192+
headers.set("Date", "Thu, 01 Jan 1970 00:00:00 GMT"); // value contains colons
193+
headers.set("x-empty", "");
194+
headers.set("x-multi", "1");
195+
headers.append("x-multi", "2");
196+
}
197+
198+
function compareHeaders(expectedHeaders: Headers, otherHeaders: Headers | undefined) {
199+
expect(new Map(otherHeaders)).toEqual(new Map(expectedHeaders));
200+
}
201+
202+
it("should resolve HTTP Errors from XHR with headers", () => {
203+
headers.set("Content-Type", "text/plain");
204+
addHeaders(headers);
205+
const err = parseErrorResponse({
206+
...xhrHeaderMethods,
207+
status: 500,
208+
} as XMLHttpRequest) as HTTPError;
209+
compareHeaders(headers, err.httpHeaders);
210+
});
211+
212+
it("should resolve HTTP Errors from fetch with headers", () => {
213+
headers.set("Content-Type", "text/plain");
214+
addHeaders(headers);
215+
const err = parseErrorResponse({
216+
headers,
217+
status: 500,
218+
} as Response) as HTTPError;
219+
compareHeaders(headers, err.httpHeaders);
220+
});
221+
222+
it("should resolve Matrix Errors from XHR with headers", () => {
223+
headers.set("Content-Type", "application/json");
224+
addHeaders(headers);
225+
const err = parseErrorResponse(
226+
{
227+
...xhrHeaderMethods,
228+
status: 500,
229+
} as XMLHttpRequest,
230+
'{"errcode": "TEST"}',
231+
) as MatrixError;
232+
compareHeaders(headers, err.httpHeaders);
233+
});
234+
235+
it("should resolve Matrix Errors from fetch with headers", () => {
236+
headers.set("Content-Type", "application/json");
237+
addHeaders(headers);
238+
const err = parseErrorResponse(
239+
{
240+
headers,
241+
status: 500,
242+
} as Response,
243+
'{"errcode": "TEST"}',
244+
) as MatrixError;
245+
compareHeaders(headers, err.httpHeaders);
246+
});
247+
});
248+
181249
it("should set a sensible default error message on MatrixError", () => {
182250
let err = new MatrixError();
183251
expect(err.message).toEqual("MatrixError: Unknown message");
@@ -188,46 +256,50 @@ describe("parseErrorResponse", () => {
188256
});
189257

190258
it("should handle no type gracefully", () => {
259+
// No Content-Type header
191260
expect(
192261
parseErrorResponse(
193262
{
194-
headers: {
195-
get(name: string): string | null {
196-
return null;
197-
},
198-
},
263+
headers,
199264
status: 500,
200265
} as Response,
201266
'{"errcode": "TEST"}',
202267
),
203268
).toStrictEqual(new HTTPError("Server returned 500 error", 500));
204269
});
205270

271+
it("should handle empty type gracefully", () => {
272+
headers.set("Content-Type", " ");
273+
expect(
274+
parseErrorResponse(
275+
{
276+
headers,
277+
status: 500,
278+
} as Response,
279+
'{"errcode": "TEST"}',
280+
),
281+
).toStrictEqual(new Error("Error parsing Content-Type '': TypeError: argument string is required"));
282+
});
283+
206284
it("should handle invalid type gracefully", () => {
285+
headers.set("Content-Type", "unknown");
207286
expect(
208287
parseErrorResponse(
209288
{
210-
headers: {
211-
get(name: string): string | null {
212-
return name === "Content-Type" ? " " : null;
213-
},
214-
},
289+
headers,
215290
status: 500,
216291
} as Response,
217292
'{"errcode": "TEST"}',
218293
),
219-
).toStrictEqual(new Error("Error parsing Content-Type ' ': TypeError: invalid media type"));
294+
).toStrictEqual(new Error("Error parsing Content-Type 'unknown': TypeError: invalid media type"));
220295
});
221296

222297
it("should handle plaintext errors", () => {
298+
headers.set("Content-Type", "text/plain");
223299
expect(
224300
parseErrorResponse(
225301
{
226-
headers: {
227-
get(name: string): string | null {
228-
return name === "Content-Type" ? "text/plain" : null;
229-
},
230-
},
302+
headers,
231303
status: 418,
232304
} as Response,
233305
"I'm a teapot",

0 commit comments

Comments
 (0)