Feature: fix a number of redirect handling issues #271
Conversation
| if (res.statusCode === 301 || res.statusCode === 302 || res.statusCode === 307) { | ||
| if (!res.headers.location) { | ||
| // Server sent redirect response without Location header. | ||
| _emit('error', new Event('error', {status: res.statusCode, message: res.statusMessage})) |
There was a problem hiding this comment.
I think this isn't important, but we are loosing this error reporting. Probably fine?
rexxars
changed the title
I don't think there should be any config changes necessary, but given I am not 100% sure, I agree with your point. Given this is patching a security issue, I really want to get a patch release out to ensure people don't have to upgrade to a new major to be covered. I have opened #273 to address only the headers issue, leaving this one as a general redirect handling PR. Would you mind reviewing that one, @joeybaker ? |
|
Looks like this perhaps got left out in the cold. Any idea if this PR will get merged in? |
rexxars
force-pushed
the
main
branch
2 times, most recently
from
6349107 to
0338f51
Compare
|
|
When requesting an eventsource endpoint and defining custom, sensitive headers, such as
AuthorizationandCookie, these headers should not be forwarded when redirecting to a different origin than the original.While looking in to fixing this, I discovered that the current redirect handling also does not support relative URLs in the
Locationheader (egLocation: /some/other/path), nor does it set any limit on the maximum number of redirects. Instead of attempting to patch all these shortcomings, I feel we are better suited by utilizing the follow-redirects module, which handles all of these cases and is widely used.