fix(ES-1214): fix XSS vulnerability (#7302)

WojtekTheWebDev · web-flow · commit 180f19a50708 · 2024-11-04T13:38:40.000+01:00
* fix(ES-1214): fix XSS vulnerability

* code refactor

* refactor

* sanitize all params
diff --git a/.changeset/weak-impalas-drive.md b/.changeset/weak-impalas-drive.md
@@ -0,0 +1,5 @@
+---
+"@vue-storefront/middleware": patch
+---
+
+[FIXED] a potential XSS (Cross-Site Scripting) vulnerability in the middleware. Now, each parameter is properly sanitized and validated before being used in the middleware.
diff --git a/packages/middleware/__tests__/integration/createServer.spec.ts b/packages/middleware/__tests__/integration/createServer.spec.ts
@@ -18,33 +18,33 @@ describe("[Integration] Create server", () => {
             res.send("Custom error handler");
           },
           location: "./__tests__/integration/bootstrap/server",
-          extensions() {
-            return [
+          extensions: (extensions) =>
+            [
+              ...extensions,
               {
                 name: "my-extension",
                 extendApiMethods: {
-                  myFunc(context) {
+                  myFunc: (context) => {
                     return context.api.success();
                   },
-                  myFuncWithDependencyToOtherExtension(context) {
-                    return (context.api as any).myFunc();
+                  myFuncWithDependencyToOtherExtension: (context) => {
+                    return context.api.myFunc();
                   },
                 },
               },
               {
                 name: "my-namespaced-extension",
                 isNamespaced: true,
                 extendApiMethods: {
-                  myFunc(context) {
+                  myFunc: (context) => {
                     return context.api.error();
                   },
-                  myFuncNamespaced(context) {
+                  myFuncNamespaced: (context) => {
                     return context.api.success();
                   },
                 },
               },
-            ];
-          },
+            ] as any,
         },
       },
     });
@@ -208,4 +208,55 @@ describe("[Integration] Create server", () => {
     expect(status).toEqual(200);
     expect(response).toEqual(apiMethodResult);
   });
+
+  it("should accept only GET and POST methods", async () => {
+    const getRes = await request(app).get("/test_integration/success").send();
+
+    expect(getRes.status).toEqual(200);
+    expect(getRes.body.message).toEqual("ok");
+
+    const postRes = await request(app).post("/test_integration/success").send();
+
+    expect(postRes.status).toEqual(200);
+    expect(postRes.body.message).toEqual("ok");
+
+    const putRes = await request(app).put("/test_integration/success").send();
+
+    expect(putRes.status).toEqual(405);
+    expect(putRes.error && putRes.error.text).toEqual(
+      "Method PUT is not allowed. Please, use GET or POST method."
+    );
+
+    const deleteRes = await request(app)
+      .delete("/test_integration/success")
+      .send();
+
+    expect(deleteRes.status).toEqual(405);
+    expect(deleteRes.error && deleteRes.error.text).toEqual(
+      "Method DELETE is not allowed. Please, use GET or POST method."
+    );
+  });
+
+  describe("prevent XSS attacks", () => {
+    test.each([
+      [
+        "/z--%3E%3C!--hi--%3E%3Cimg%20src=x%20onerror=alert('DOM--XSS')%3E%3C!--%3C%3C/success",
+        `"z--&gt;<img src>" integration is not configured. Please, check the request path or integration configuration.`,
+      ],
+      [
+        "/test_integration/z--%3E%3C!--hi--%3E%3Cimg%20src=x%20onerror=alert('DOM--XSS')%3E%3C!--%3C%3C",
+        `Failed to resolve apiClient or function: The function "z--&gt;<img src>" is not registered.`,
+      ],
+      [
+        "/test_integration/z--%3E%3C!--hi--%3E%3Cimg%20src=x%20onerror=alert('DOM--XSS')%3E%3C!--%3C%3C/success",
+        `Failed to resolve apiClient or function: Extension "z--&gt;<img src>" is not namespaced or the function "success" is not available in the namespace.`,
+      ],
+    ])("Use case: %s", async (maliciousUrl, expectedMessage) => {
+      const res = await request(app).get(maliciousUrl).send();
+      expect(res.error && res.error.text).not.toContain(
+        "z--><!--hi--><img src=x onerror=alert('DOM--XSS')><!--<<"
+      );
+      expect(res.error && res.error.text).toEqual(expectedMessage);
+    });
+  });
 });
diff --git a/packages/middleware/__tests__/unit/handlers/prepareApiFunction.spec.ts b/packages/middleware/__tests__/unit/handlers/prepareApiFunction.spec.ts
@@ -29,16 +29,6 @@ describe("[middleware-handlers] prepareApiFunction", () => {
     res.locals = {};
   });
 
-  it("sends 404 error if integration is not configured", async () => {
-    const emptyIntegrations = {};
-
-    await prepareApiFunction(emptyIntegrations)(req, res, next);
-
-    expect(res.status).toBeCalledTimes(1);
-    expect(res.status).toBeCalledWith(404);
-    expect(res.send).toBeCalledTimes(1);
-  });
-
   describe("if integration is configured", () => {
     it("adds api function to res.locals", async () => {
       await prepareApiFunction(integrations)(req, res, next);
diff --git a/packages/middleware/package.json b/packages/middleware/package.json
@@ -25,7 +25,8 @@
     "cors": "^2.8.5",
     "express": "^4.18.1",
     "helmet": "^5.1.1",
-    "@godaddy/terminus": "^4.12.1"
+    "@godaddy/terminus": "^4.12.1",
+    "xss": "^1.0.15"
   },
   "devDependencies": {
     "@types/body-parser": "^1.19.2",
diff --git a/packages/middleware/src/createServer.ts b/packages/middleware/src/createServer.ts
@@ -6,7 +6,6 @@ import type { HelmetOptions } from "helmet";
 import helmet from "helmet";
 import http, { Server } from "node:http";
 import { createTerminus } from "@godaddy/terminus";
-
 import { registerIntegrations } from "./integrations";
 import type {
   Helmet,
@@ -19,6 +18,7 @@ import {
   prepareErrorHandler,
   prepareArguments,
   callApiFunction,
+  validateParams,
 } from "./handlers";
 import { createTerminusOptions } from "./terminus";
 
@@ -66,15 +66,9 @@ async function createServer<
   const integrations = await registerIntegrations(app, config.integrations);
   consola.success("Integrations loaded!");
 
-  app.post(
-    "/:integrationName/:extensionName?/:functionName",
-    prepareApiFunction(integrations),
-    prepareErrorHandler(integrations),
-    prepareArguments,
-    callApiFunction
-  );
-  app.get(
+  app.all(
     "/:integrationName/:extensionName?/:functionName",
+    validateParams(integrations),
     prepareApiFunction(integrations),
     prepareErrorHandler(integrations),
     prepareArguments,
diff --git a/packages/middleware/src/handlers/index.ts b/packages/middleware/src/handlers/index.ts
@@ -2,3 +2,4 @@ export * from "./callApiFunction";
 export * from "./prepareApiFunction";
 export * from "./prepareErrorHandler";
 export * from "./prepareArguments";
+export * from "./validateParams";
diff --git a/packages/middleware/src/handlers/prepareApiFunction/index.ts b/packages/middleware/src/handlers/prepareApiFunction/index.ts
@@ -7,15 +7,6 @@ export function prepareApiFunction(
   return async (req, res, next) => {
     const { integrationName, extensionName, functionName } = req.params;
 
-    if (!integrations || !integrations[integrationName]) {
-      res.status(404);
-      res.send(
-        `"${integrationName}" integration is not configured. Please, check the request path or integration configuration.`
-      );
-
-      return;
-    }
-
     const {
       apiClient,
       configuration,
diff --git a/packages/middleware/src/handlers/validateParams/index.ts b/packages/middleware/src/handlers/validateParams/index.ts
@@ -0,0 +1,36 @@
+import xss from "xss";
+import type { Request, Response, NextFunction } from "express";
+import { IntegrationsLoaded } from "../../types";
+
+export function validateParams(integrations: IntegrationsLoaded) {
+  return (req: Request, res: Response, next: NextFunction) => {
+    // Validate & sanitize the request params
+    Object.entries(req.params).forEach(([key, value]) => {
+      req.params[key] = typeof value === "string" ? xss(value) : value;
+    });
+
+    // Validate the request method
+    const { method } = req;
+    if (method !== "GET" && method !== "POST") {
+      res.status(405);
+      res.send(
+        `Method ${method} is not allowed. Please, use GET or POST method.`
+      );
+
+      return;
+    }
+
+    // Validate the integration
+    const { integrationName } = req.params;
+    if (!integrations || !integrations[integrationName]) {
+      res.status(404);
+      res.send(
+        `"${integrationName}" integration is not configured. Please, check the request path or integration configuration.`
+      );
+
+      return;
+    }
+
+    next();
+  };
+}
diff --git a/packages/middleware/src/types/common.ts b/packages/middleware/src/types/common.ts
@@ -78,9 +78,11 @@ export interface Integration<
 > {
   location: string;
   configuration: CONFIG;
-  extensions?: <T extends ApiClientMethodWithContext<CONTEXT>>(
+  extensions?: (
     extensions: ApiClientExtension<API, CONTEXT>[]
-  ) => ApiClientExtension<API & T, CONTEXT>[];
+    // TODO(IN-4338): There is a bug in the types here
+    // we're not able to verify if the methods are namespaced or not with this implementation.
+  ) => ApiClientExtension<API, CONTEXT>[];
   customQueries?: Record<string, CustomQueryFunction>;
   initConfig?: TObject;
   errorHandler?: (error: unknown, req: Request, res: Response) => void;
diff --git a/yarn.lock b/yarn.lock
@@ -4629,7 +4629,7 @@ commander@^10.0.0:
   resolved "https://registry.yarnpkg.com/commander/-/commander-10.0.1.tgz#881ee46b4f77d1c1dccc5823433aa39b022cbe06"
   integrity sha512-y4Mg2tXshplEbSGzx7amzPwKKOCGuoSRP/CjEdwwk0FOGlUbq6lKuoyDZTNZkmxHdJtp54hdfY/JUrdL7Xfdug==
 
-commander@^2.18.0:
+commander@^2.18.0, commander@^2.20.3:
   version "2.20.3"
   resolved "https://registry.yarnpkg.com/commander/-/commander-2.20.3.tgz#fd485e84c03eb4881c20722ba48035e8531aeb33"
   integrity sha512-GpVkmM8vF2vQUkj2LvZmD35JxeJOLCwJ9cUkugyk2nuhbv3+mJvpLYYt+0+USMxE+oj+ey/lJEnhZw75x/OMcQ==
@@ -4809,6 +4809,11 @@ cssesc@^3.0.0:
   resolved "https://registry.yarnpkg.com/cssesc/-/cssesc-3.0.0.tgz#37741919903b868565e1c09ea747445cd18983ee"
   integrity sha512-/Tb/JcjK111nNScGob5MNtsntNM1aCNUDipB/TkwZFhyDrrE47SOx/18wF2bbjgc3ZzCSKW1T5nt5EbFoAz/Vg==
 
+cssfilter@0.0.10:
+  version "0.0.10"
+  resolved "https://registry.yarnpkg.com/cssfilter/-/cssfilter-0.0.10.tgz#c6d2672632a2e5c83e013e6864a42ce8defd20ae"
+  integrity sha512-FAaLDaplstoRsDR8XGYH51znUN0UY7nMc6Z9/fvE8EXGwvJE9hu7W2vHwx1+bd6gCYnln9nLbzxFTrcO9YQDZw==
+
 cssom@^0.4.4:
   version "0.4.4"
   resolved "https://registry.yarnpkg.com/cssom/-/cssom-0.4.4.tgz#5a66cf93d2d0b661d80bf6a44fb65f5c2e4e0a10"
@@ -13190,6 +13195,14 @@ xmlchars@^2.2.0:
   resolved "https://registry.yarnpkg.com/xmlchars/-/xmlchars-2.2.0.tgz#060fe1bcb7f9c76fe2a17db86a9bc3ab894210cb"
   integrity sha512-JZnDKK8B0RCDw84FNdDAIpZK+JuJw+s7Lz8nksI7SIuU3UXJJslUthsi+uWBUYOwPFwW7W7PRLRfUKpxjtjFCw==
 
+xss@^1.0.15:
+  version "1.0.15"
+  resolved "https://registry.yarnpkg.com/xss/-/xss-1.0.15.tgz#96a0e13886f0661063028b410ed1b18670f4e59a"
+  integrity sha512-FVdlVVC67WOIPvfOwhoMETV72f6GbW7aOabBC3WxN/oUdoEMDyLz4OgRv5/gck2ZeNqEQu+Tb0kloovXOfpYVg==
+  dependencies:
+    commander "^2.20.3"
+    cssfilter "0.0.10"
+
 xtend@^4.0.0:
   version "4.0.2"
   resolved "https://registry.yarnpkg.com/xtend/-/xtend-4.0.2.tgz#bb72779f5fa465186b1f438f674fa347fdb5db54"

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +"@vue-storefront/middleware": patch
 +---
++
 +[FIXED] a potential XSS (Cross-Site Scripting) vulnerability in the middleware. Now, each parameter is properly sanitized and validated before being used in the middleware.