eventsource icon indicating copy to clipboard operation
eventsource copied to clipboard

throwing all properties from url.parse into the request options is problematic

Open eli-darkly opened this issue 3 years ago • 0 comments

Currently, connect() starts by setting options to the result of parse(url), then it adds some more properties, and then it passes the object to http.request or https.request. However, while the options type for those functions uses many of the same properties as the return type of url.parse, it does not use all of them; in other words, there are some properties of a parsed URL that are not valid as request properties, such as hash.

Normally Node ignores any unrecognized properties, so why does this matter? It matters because middleware code may intercept functions like http.request and, in order to correctly handle the variant calling conventions of those functions, it will need to disambiguate between 1. this parameter is a URL string, 2. this parameter is a URL object, 3. this parameter is a request options object. Number 1 is easy, but distinguishing between 2 and 3 is harder since they have many properties in common. And this isn't just a theoretical concern— at least one middleware product is known to break for exactly this reason.

The solution would be to copy over the individual properties of the parsed URL that really are valid as request options, rather than taking them all and extending that object.

eli-darkly avatar Mar 15 '22 19:03 eli-darkly