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

Don't transmit cookies on redirect #4

Closed
Farkal opened this issue May 27, 2020 · 5 comments · Fixed by #6
Closed

Don't transmit cookies on redirect #4

Farkal opened this issue May 27, 2020 · 5 comments · Fixed by #6

Comments

@Farkal
Copy link
Contributor

Farkal commented May 27, 2020

Your lib doesn't seem to keep the cookies with redirect: "follow"

@jkhsjdhjs
Copy link
Owner

Yes, you're right. This is because node-fetch automatically follows redirects, so cookies contained in set-cookie headers of redirect responses never reach this library. We only get the final response, after all redirects have been followed.

However, it would be possible to set redirect to manual for each request so we can evaluate each response individually (and also send cookies for each request individually). This lib just needs some kind of redirect evaluation routine then (most likely we can just use code from node-fetch for that).

I'll see when I have time to do that (maybe in a week or so). If you or anyone else wants to do it, PR's are always welcome! :D

@Farkal
Copy link
Contributor Author

Farkal commented Jun 1, 2020

Yes it work with redirect manual but I have many redirect (3 or 4 for each request) so finding the urls and write each request is not very efficient :/

The issue is that you can't get the redirect url from "manual" mode in fetch as this issue say: whatwg/fetch#763 (perhaps it's possible in node-fetch but i didn't find anything about it)
So i don't know how we could get the rigth behavior (perhaps making a redirect: "follow", extract the urls and then replay it with "manual" and the cookieJar).

@jkhsjdhjs
Copy link
Owner

Really interesting, that there seems to be no way of accessing the headers when using redirect: "manual". I just tested it in my browser and [...response.headers.keys()] just yielded an empty array with redirect: "manual". With redirect: "follow" I didn't have any issues accessing the response headers.
However, node-fetch seems to be implemented differently. I can access the Location header without issues when using redirect: "manual".

In the following example index.php redirects to r.php:

import fetch from "node-fetch";

(async () => {
    const response = await fetch("https://totally.rip/redirect-test/index.php", { redirect: "manual" });
    console.log(response.headers.get("location")); // "https://totally.rip/redirect-test/r.php"
})();

So changing the code to manually follow redirects shouldn't be a problem.

@Farkal
Copy link
Contributor Author

Farkal commented Jun 9, 2020

Yes you are right !
I can make a PR if you are interested but how do you think i should implement this ? Wrap your fetch, check options for "follow" and run in while loop if status is 302 and if there is location in headers ?

@jkhsjdhjs
Copy link
Owner

That would be great! Yes, you can add a while loop to the fetch function or use recursion. Also be sure to not just check for 302 but all other possible redirect status codes as well (node-fetch checks for these).

You can also check how node-fetch handles redirects: https://github.com/node-fetch/node-fetch/blob/master/src/index.js#L103 (they're calling fetch again in the response callback, so a bit like recursion).

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

Successfully merging a pull request may close this issue.

2 participants