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

Fix multiple cookies for follow_set_cookies #421

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

QuiteQuiet
Copy link

It appears that cookies.js expects the format ofthe Set Cookies header to function. This sounds reasonable, but the original request's cookies are stored as a string (for convenience I believe?) not as key-value pair strings, so it needs to be separated again to be forwarded correctly in the redirect.

Fixes #350.

@tomas
Copy link
Owner

tomas commented Dec 8, 2023

Could you please add some tests? It looks to me this would break if the cookies header is empty...

It appears that cookies.js expects the format ofthe `Set Cookies` header
to function. This sounds reasonable, but the original request cookies
are stored as a string not as key-value pair strings, so it needs to be
separated again to be forwarded correctly in the redirect.
@QuiteQuiet
Copy link
Author

QuiteQuiet commented Jan 7, 2024

You are right it fails if there are no cookies... I think this should work instead?

There is already a test for an exmpty cookie with redirect, but I added one for multiple request cookies anyway.

I can't actually run all tests because I don't have access to a Linux machine which could, but all cookies-related tests are passing.

  cookies
    with default options
      √ no cookie header is set on request
(node:7320) [DEP0066] DeprecationWarning: OutgoingMessage.prototype._headers is deprecated
    if response does not contain cookies
      √ response.cookies is undefined
    if response contains cookies
      √ puts them on resp.cookies
      √ parses them as a object
      √ must decode it
      when a cookie value is invalid
        √ doesnt blow up
      and response is a redirect
        and follow_set_cookies is false
          with original request cookie
            √ request cookie is not passed to redirects
            √ response cookies are not passed either
          without original request cookie
            √ no request cookies are sent
            √ response cookies are not passed either
        and follow_set_cookies is true
          with original request cookie
            √ request cookie is passed passed to redirects, and response cookies are added too
            √ response cookies are passed as well
          with multiple original request cookies
            √ request cookie is passed passed to redirects, and response cookies are added too
            √ response cookies are passed as well
          without original request cookie
            √ response cookies are passed to redirects
            √ response cookies are passed as well
      with parse_cookies = false
        √ does not parse them
    if request contains cookie header
      √ must be a valid cookie string
      √ dont have to encode allowed characters
      √ must encode forbidden characters

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 this pull request may close these issues.

"follow_set_cookies" not forwarding all initial cookies when following redirects
2 participants