Allow to drop custom sensitive headers when following redirects

[vansergen]

@lpinca What do you think about using the existing option followRedirects to achieve that?

[lpinca]

I prefer to not do this. The user should enable the followRedirects option only when the server is trusted. A better option for this would probably be a 'redirect' event where the user can customize the headers for the new URL. The reason why I did not add it is that it adds unnecessary complexity.

I also don't like the current behavior but it is similar to what many popular HTTP clients do and should be enough to prevent "security researchers" from reporting a security vulnerability for this.

[vansergen]

The user should enable the followRedirects option only when the server is trusted.

Agree. But one of the use cases is the redirect between two trusted servers with different subdomains (a.example.com -> b.example.com). And sensitive headers that are relevant to a become irrelevant to b.

A better option for this would probably be a 'redirect' event where the user can customize the headers for the new URL. The reason why I did not add it is that it adds unnecessary complexity.

Indeed, I chose the easiest way to achieve that

[lpinca]

And sensitive headers that are relevant to a become irrelevant to b.

It does not matter as long as b is trusted.

We can eventually add the 'redirect' event if there is demand for it.

[vansergen]

We can eventually add the 'redirect' event if there is demand for it.

There is 👍 The redirect event definitely would be the best option. Since, one could modify headers(when necessary) per a redirect in case of multiple redirects.

[lpinca]

There are a few things to keep in mind:

1. If there is one or more listeners for the 'redirect' event, then no header should be removed as it is user responsibility.
2. The listener(s) can be removed between redirects and if there is no listener, the behavior should match the current one.
3. The headers in the options object should not be mutated by the user.
4. Header field names are case-insensitive.
5. The user can close the connection with websocket.close() and websocket.terminate() from the listener of the 'redirect' event. This must be handled. Something similar is done for the 'upgrade' event.

I was thinking about emitting the event right after creating the request, before ending it and giving the user the whole ClientRequest object. Something like this

diff --git a/lib/websocket.js b/lib/websocket.js
index 6fff935..81fe59c 100644
--- a/lib/websocket.js
+++ b/lib/websocket.js
@@ -647,7 +647,7 @@ function initAsClient(websocket, address, protocols, options) {
     hostname: undefined,
     protocol: undefined,
     timeout: undefined,
-    method: undefined,
+    method: 'GET',
     host: undefined,
     path: undefined,
     port: undefined
@@ -701,7 +701,7 @@ function initAsClient(websocket, address, protocols, options) {
 
   const defaultPort = isSecure ? 443 : 80;
   const key = randomBytes(16).toString('base64');
-  const get = isSecure ? https.get : http.get;
+  const request = isSecure ? https.request : http.request;
   const protocolSet = new Set();
   let perMessageDeflate;
 
@@ -766,6 +766,8 @@ function initAsClient(websocket, address, protocols, options) {
     opts.path = parts[1];
   }
 
+  let req;
+
   if (opts.followRedirects) {
     if (websocket._redirects === 0) {
       websocket._originalHost = parsedUrl.host;
@@ -783,7 +785,10 @@ function initAsClient(websocket, address, protocols, options) {
           options.headers[key.toLowerCase()] = value;
         }
       }
-    } else if (parsedUrl.host !== websocket._originalHost) {
+    } else if (
+      websocket.listenerCount('redirect') === 0 &&
+      parsedUrl.host !== websocket._originalHost
+    ) {
       //
       // Match curl 7.77.0 behavior and drop the following headers. These
       // headers are also dropped when following a redirect to a subdomain.
@@ -803,9 +808,16 @@ function initAsClient(websocket, address, protocols, options) {
       options.headers.authorization =
         'Basic ' + Buffer.from(opts.auth).toString('base64');
     }
-  }
 
-  let req = (websocket._req = get(opts));
+    req = websocket._req = request(opts);
+
+    if (websocket.listenerCount('redirect') !== 0) {
+      websocket.emit('redirect', websocket.url, req);
+      if (websocket.readyState !== WebSocket.CONNECTING) return;
+    }
+  } else {
+    req = websocket._req = request(opts);
+  }
 
   if (opts.timeout) {
     req.on('timeout', () => {
@@ -947,6 +959,8 @@ function initAsClient(websocket, address, protocols, options) {
       skipUTF8Validation: opts.skipUTF8Validation
     });
   });
+
+  req.end();
 }
 
 /**

This would give the user the ability to inspect, add, and remove headers using the Node.js API (request.getHeaders(), request.setHeader(), request.removeHeader(), etc.)

Anyway I'm a bit short on time, and I'm not sure if it is worth the effort. Feel free to take a stab at it.