Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Errors Gracefully in shouldProxy Method to Mitigate CVE-2024-21536 #1068

Open
marinescudan opened this issue Jan 17, 2025 · 0 comments

Comments

@marinescudan
Copy link

marinescudan commented Jan 17, 2025

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch [email protected] for the project I'm working on.

This patch addresses a vulnerability (CVE-2024-21536) in the [email protected] package. The vulnerability arises from unhandled exceptions in the shouldProxy method, which can lead to a Denial of Service (DoS) attack when malformed or unexpected requests are processed.

Changes Made:
Wrapped the logic in shouldProxy with a try-catch block to prevent crashes caused by invalid regular expressions or malformed paths.
Added detailed error logging using this.logger.error to provide clear diagnostics without exposing sensitive information.
Ensured that the method safely returns false when an error occurs, denying proxying for problematic requests.

Impact:
This patch prevents the application from crashing due to unhandled promise rejections or unexpected runtime errors.
The change is backward-compatible and does not affect existing functionality or valid proxying rules.

Testing:
Simulated scenarios with malformed URLs and invalid paths to ensure errors are handled gracefully.
Verified that legitimate requests continue to be proxied correctly.
This patch improves the resilience of http-proxy-middleware while maintaining compatibility with existing configurations.

Here is the diff that solved my problem:

diff --git a/node_modules/http-proxy-middleware/dist/http-proxy-middleware.js b/node_modules/http-proxy-middleware/dist/http-proxy-middleware.js
index 6cd93ce..a25a714 100644
--- a/node_modules/http-proxy-middleware/dist/http-proxy-middleware.js
+++ b/node_modules/http-proxy-middleware/dist/http-proxy-middleware.js
@@ -74,16 +74,30 @@ class HttpProxyMiddleware {
             }
         });
         /**
-         * Determine whether request should be proxied.
+         * Determine whether the request should be proxied.
+         *
+         * This method evaluates the request URL against the specified proxy context
+         * and returns true if the request matches the proxying criteria.
+         *
+         * Patch Note (CVE-2024-21536):
+         * - Wrapped the logic in a try-catch block to handle unexpected errors
+         *   (e.g., invalid regular expressions or malformed paths) gracefully.
+         * - Logs the error without crashing the application, mitigating the risk
+         *   of Denial of Service (DoS) attacks.
          *
          * @private
-         * @param  {String} context [description]
-         * @param  {Object} req     [description]
-         * @return {Boolean}
+         * @param  {String} context Proxying criteria (e.g., URL patterns).
+         * @param  {Object} req     The HTTP request object.
+         * @return {Boolean}        True if the request should be proxied, false otherwise.
          */
         this.shouldProxy = (context, req) => {
-            const path = req.originalUrl || req.url;
-            return contextMatcher.match(context, path, req);
+            try {
+                const path = req.originalUrl || req.url; // Determine the request path.
+                return contextMatcher.match(context, path, req); // Check if it matches the proxying criteria.
+            } catch (error) {
+                this.logger.error(`Error in shouldProxy method: ${error.message}`, { error }); // Log the error.
+                return false; // Safely deny proxying if an error occurs.
+            }
         };
         /**
          * Apply option.router and option.pathRewrite
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant