fix: Update host if there's a redirection#596
fix: Update host if there's a redirection#596abruzzihraig wants to merge 2 commits intonode-fetch:3.xfrom
Conversation
|
I have same issue. please merge this. |
|
We probably need to unit test this. |
There was a problem hiding this comment.
My take: this is an edge case where users specify the Host header manually (which is forbidden on client-side fetch) and somehow expect node-fetch to update it automatically using the Location header as source (which is basically taking remote user input).
I haven't thought through the security implication here but I prefer not to go down this route. I would prefer to drop Host header if redirect happens (or just prevent users from doing this when redirect option is follow, because something clearly isn't quite right).
But the real solution here is, users should know what they are doing.
(If my analysis is wrong, please provide us with a better test case to illustrate that this issue should be handled by node-fetch.)
|
There is one thing I do think could be improved though: our error message is misleading in this case, we might throw a |
|
Sorry guys, I tried to remember what case I'd had when I was dealing with this issue. I just remember it was a very popular domain, and the redirect flow was like http://google.com -> http://www.google.com -> https://www.google.com or something. But I cannot reproduce it anymore even based on fetch@2.3.0. The fix I've made is not that safe and reliable. It was more like a patch for solving my urgent need at that moment. I didn't pay much time into the whole context to ensure the change is reasonable. I agree with @bitinn and hope you guys can make a better decision to simply ignore this PR or improve it by other ways. Thanks. |
Closes #570