Merge pull request #2753 from matrix-org/kegan/http-code-on-non-json

Always send back an httpStatus property if one is known
matrix-org · Oct 14, 2022 · f70f6db · f70f6db
2 parents c81d759 + 500601e
commit f70f6db
Show file tree

Hide file tree

Showing 5 changed files with 64 additions and 22 deletions.
diff --git a/spec/unit/http-api/utils.spec.ts b/spec/unit/http-api/utils.spec.ts
@@ -19,6 +19,7 @@ import { mocked } from "jest-mock";
 import {
     anySignal,
     ConnectionError,
+    HTTPError,
     MatrixError,
     parseErrorResponse,
     retryNetworkOperation,
@@ -113,6 +114,41 @@ describe("parseErrorResponse", () => {
         }, 500));
     });
 
+    it("should resolve Matrix Errors from XHR with urls", () => {
+        expect(parseErrorResponse({
+            responseURL: "https://example.com",
+            getResponseHeader(name: string): string | null {
+                return name === "Content-Type" ? "application/json" : null;
+            },
+            status: 500,
+        } as XMLHttpRequest, '{"errcode": "TEST"}')).toStrictEqual(new MatrixError({
+            errcode: "TEST",
+        }, 500, "https://example.com"));
+    });
+
+    it("should resolve Matrix Errors from fetch with urls", () => {
+        expect(parseErrorResponse({
+            url: "https://example.com",
+            headers: {
+                get(name: string): string | null {
+                    return name === "Content-Type" ? "application/json" : null;
+                },
+            },
+            status: 500,
+        } as Response, '{"errcode": "TEST"}')).toStrictEqual(new MatrixError({
+            errcode: "TEST",
+        }, 500, "https://example.com"));
+    });
+
+    it("should set a sensible default error message on MatrixError", () => {
+        let err = new MatrixError();
+        expect(err.message).toEqual("MatrixError: Unknown message");
+        err = new MatrixError({
+            error: "Oh no",
+        });
+        expect(err.message).toEqual("MatrixError: Oh no");
+    });
+
     it("should handle no type gracefully", () => {
         expect(parseErrorResponse({
             headers: {
@@ -121,7 +157,7 @@ describe("parseErrorResponse", () => {
                 },
             },
             status: 500,
-        } as Response, '{"errcode": "TEST"}')).toStrictEqual(new Error("Server returned 500 error"));
+        } as Response, '{"errcode": "TEST"}')).toStrictEqual(new HTTPError("Server returned 500 error", 500));
     });
 
     it("should handle invalid type gracefully", () => {
@@ -144,7 +180,7 @@ describe("parseErrorResponse", () => {
                 },
             },
             status: 418,
-        } as Response, "I'm a teapot")).toStrictEqual(new Error("Server returned 418 error: I'm a teapot"));
+        } as Response, "I'm a teapot")).toStrictEqual(new HTTPError("Server returned 418 error: I'm a teapot", 418));
     });
 });
 

diff --git a/spec/unit/interactive-auth.spec.ts b/spec/unit/interactive-auth.spec.ts
@@ -18,7 +18,7 @@ limitations under the License.
 import { MatrixClient } from "../../src/client";
 import { logger } from "../../src/logger";
 import { InteractiveAuth, AuthType } from "../../src/interactive-auth";
-import { MatrixError } from "../../src/http-api";
+import { HTTPError, MatrixError } from "../../src/http-api";
 import { sleep } from "../../src/utils";
 import { randomString } from "../../src/randomstring";
 
@@ -219,8 +219,7 @@ describe("InteractiveAuth", () => {
                 params: {
                     [AuthType.Password]: { param: "aa" },
                 },
-            });
-            err.httpStatus = 401;
+            }, 401);
             throw err;
         });
 
@@ -282,8 +281,7 @@ describe("InteractiveAuth", () => {
                 params: {
                     [AuthType.Password]: { param: "aa" },
                 },
-            });
-            err.httpStatus = 401;
+            }, 401);
             throw err;
         });
 
@@ -338,8 +336,7 @@ describe("InteractiveAuth", () => {
                 params: {
                     [AuthType.Password]: { param: "aa" },
                 },
-            });
-            err.httpStatus = 401;
+            }, 401);
             throw err;
         });
 
@@ -374,8 +371,7 @@ describe("InteractiveAuth", () => {
                 },
                 error: "Mock Error 1",
                 errcode: "MOCKERR1",
-            });
-            err.httpStatus = 401;
+            }, 401);
             throw err;
         });
 
@@ -402,8 +398,7 @@ describe("InteractiveAuth", () => {
         doRequest.mockImplementation((authData) => {
             logger.log("request1", authData);
             expect(authData).toEqual({ "session": "sessionId" }); // has existing sessionId
-            const err = new Error('myerror');
-            (err as any).httpStatus = 401;
+            const err = new HTTPError('myerror', 401);
             throw err;
         });
 

diff --git a/src/http-api/errors.ts b/src/http-api/errors.ts
@@ -22,6 +22,19 @@ interface IErrorJson extends Partial<IUsageLimit> {
     error?: string;
 }
 
+/**
+ * Construct a generic HTTP error. This is a JavaScript Error with additional information
+ * specific to HTTP responses.
+ * @constructor
+ * @param {string} msg The error message to include.
+ * @param {number} httpStatus The HTTP response status code.
+ */
+export class HTTPError extends Error {
+    constructor(msg: string, public readonly httpStatus?: number) {
+        super(msg);
+    }
+}
+
 /**
  * Construct a Matrix error. This is a JavaScript Error with additional
  * information specific to the standard Matrix error response.
@@ -33,19 +46,19 @@ interface IErrorJson extends Partial<IUsageLimit> {
  * @prop {Object} data The raw Matrix error JSON used to construct this object.
  * @prop {number} httpStatus The numeric HTTP status code given
  */
-export class MatrixError extends Error {
+export class MatrixError extends HTTPError {
     public readonly errcode?: string;
     public readonly data: IErrorJson;
 
-    constructor(errorJson: IErrorJson = {}, public httpStatus?: number, public url?: string) {
+    constructor(errorJson: IErrorJson = {}, public readonly httpStatus?: number, public url?: string) {
         let message = errorJson.error || "Unknown message";
         if (httpStatus) {
             message = `[${httpStatus}] ${message}`;
         }
         if (url) {
             message = `${message} (${url})`;
         }
-        super(`MatrixError: ${message}`);
+        super(`MatrixError: ${message}`, httpStatus);
         this.errcode = errorJson.errcode;
         this.name = errorJson.errcode || "Unknown error code";
         this.data = errorJson;

diff --git a/src/http-api/index.ts b/src/http-api/index.ts
@@ -20,7 +20,7 @@ import { MediaPrefix } from "./prefix";
 import * as utils from "../utils";
 import * as callbacks from "../realtime-callbacks";
 import { Method } from "./method";
-import { ConnectionError, MatrixError } from "./errors";
+import { ConnectionError } from "./errors";
 import { parseErrorResponse } from "./utils";
 
 export * from "./interface";
@@ -116,8 +116,6 @@ export class MatrixHttpApi<O extends IHttpOpts> extends FetchHttpApi<O> {
                                 defer.reject(err);
                                 return;
                             }
-
-                            (<MatrixError>err).httpStatus = xhr.status;
                             defer.reject(new ConnectionError("request failed", err));
                         }
                         break;

diff --git a/src/http-api/utils.ts b/src/http-api/utils.ts
@@ -18,7 +18,7 @@ import { parse as parseContentType, ParsedMediaType } from "content-type";
 
 import { logger } from "../logger";
 import { sleep } from "../utils";
-import { ConnectionError, MatrixError } from "./errors";
+import { ConnectionError, HTTPError, MatrixError } from "./errors";
 
 // Ponyfill for https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout
 export function timeoutSignal(ms: number): AbortSignal {
@@ -87,9 +87,9 @@ export function parseErrorResponse(response: XMLHttpRequest | Response, body?: s
         );
     }
     if (contentType?.type === "text/plain") {
-        return new Error(`Server returned ${response.status} error: ${body}`);
+        return new HTTPError(`Server returned ${response.status} error: ${body}`, response.status);
     }
-    return new Error(`Server returned ${response.status} error`);
+    return new HTTPError(`Server returned ${response.status} error`, response.status);
 }
 
 function isXhr(response: XMLHttpRequest | Response): response is XMLHttpRequest {