fix: fix problems where bad URLs such as // could cause crashes

wessberg · May 20, 2021 · b20d9e9 · b20d9e9
1 parent e04aea9
commit b20d9e9
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 7 deletions.
diff --git a/src/api/middleware/error-middleware.ts b/src/api/middleware/error-middleware.ts
@@ -3,10 +3,12 @@ import {ApiError} from "../lib/api-error";
 import {generateErrorHtml} from "../../util/html/generate-html";
 import {pickAccept} from "../util/util";
 
-interface ErrorMiddlewareOptions {}
+interface ErrorMiddlewareOptions {
+	removeStackTrace: boolean;
+}
 
 export const errorMiddleware =
-	(_options: Partial<ErrorMiddlewareOptions> = {}) =>
+	(options: Partial<ErrorMiddlewareOptions> = {}) =>
 	async (ex: Error | ApiError, req: Request, res: Response, _: NextFunction): Promise<void> => {
 		const apiError = ApiError.ensureApiError(ex);
 
@@ -15,6 +17,6 @@ export const errorMiddleware =
 		res.setHeader("Content-Type", contentType);
 		res
 			.status(apiError.status)
-			.send(contentType === "text/html" ? generateErrorHtml(apiError) : apiError.toJSON())
+			.send(contentType === "text/html" ? generateErrorHtml(apiError, options.removeStackTrace) : apiError.toJSON())
 			.end();
 	};
diff --git a/src/api/middleware/setup-controllers.ts b/src/api/middleware/setup-controllers.ts
@@ -29,7 +29,7 @@ function toApiRequest(request: Request): ApiRequest {
 		request,
 		url: new URL(
 			// Replace all literal "+" with its literal encoded variant
-			request.url.replace(/\+/g, "%2B"),
+			(request.url.startsWith("//") ? request.url.slice(1) : request.url).replace(/\+/g, "%2B"),
 			`${request.protocol}://${request.get("host")}`
 		),
 		userAgent: request.header("user-agent") ?? "",

diff --git a/src/api/server/server.ts b/src/api/server/server.ts
@@ -45,7 +45,7 @@ export class Server implements IServer {
 		await this.metricsService.configureErrorHandlers(app);
 
 		// Apply fall-through error middleware
-		app.use(errorMiddleware());
+		app.use(errorMiddleware({removeStackTrace: this.config.production}));
 		this.app = app;
 	}
 

diff --git a/src/util/html/generate-html.ts b/src/util/html/generate-html.ts
@@ -32,7 +32,7 @@ export function generateHtml(message: string): string {
 /**
  * Generates some HTML contents with the given message
  */
-export function generateErrorHtml(error: ApiError): string {
+export function generateErrorHtml(error: ApiError, removeStackTrace = false): string {
 	// language=HTML
 	return `
 		<!DOCTYPE html>
@@ -45,7 +45,7 @@ export function generateErrorHtml(error: ApiError): string {
 			<h1>An Error occurred.</h1>
 			<h5>Code: ${error.status}</h5>
 			<p>${error.message}</p>
-			${error.stack == null ? "" : `<p>${error.stack}</p>`}
+			${error.stack == null || removeStackTrace ? "" : `<p>${error.stack}</p>`}
 		</body>
 		</html>
 	`;

diff --git a/test/api/api.test.ts b/test/api/api.test.ts
@@ -153,6 +153,17 @@ test("Will generate correct polyfills for IE11. #1", async t => {
 	t.true(result.status === StatusCodes.OK);
 });
 
+test("Won't crash for invalid URLs. #1", async t => {
+	const result = await sendRequest({
+		path: `//?foo`,
+		headers: {
+			"User-Agent": ie("11")
+		}
+	});
+
+	t.true(result.status === StatusCodes.OK);
+});
+
 test("Will correctly escape unicode escape sequences. #1", async t => {
 	const result = await sendRequest({
 		path: `${constant.endpoint.polyfill}?features=intl.number-format`,