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

expect cookie fails with a proper set-cookie header #43

Closed
arnaugm opened this issue Jul 15, 2015 · 10 comments
Closed

expect cookie fails with a proper set-cookie header #43

arnaugm opened this issue Jul 15, 2015 · 10 comments

Comments

@arnaugm
Copy link

arnaugm commented Jul 15, 2015

In a test I'm making some expectations on the response. One of the expectations verifies the existence of a cookie in the form:

expect(res).to.have.cookie('cookieName');

The expectation fails while the response object contains the set-cookie header with the cookie.

Using v1.0.0

@Limess
Copy link

Limess commented Feb 23, 2016

I'm also having issues this when trying to use chai-http in combination with supertest.

This assertion is failing: expect(response).to.have.cookie('some_cookie');.

Here is a subset of my response object showing just headers (with names changed from real values).

{    
    headers:
    {
        'cache-control': 'no-cache,private',
        'set-cookie':
        [ 'some_cookie=defgh-abcde; Domain=.someurl.co.uk; Path=/; Expires=Tue, 23 Feb 2016 10:35:16 GMT; HttpOnly' ],
        location: 'http://www.someurl.co.uk',
        vary: 'Accept',
        'content-type': 'text/plain; charset=utf-8',
        'content-length': '54',
        date: 'Tue, 23 Feb 2016 09:35:16 GMT',
        connection: 'close'
    },
    header:
    {
        'cache-control': 'no-cache,private',
        'set-cookie':
        [ 'some_cookie=defgh-abcde; Domain=.someurl.co.uk; Path=/; Expires=Tue, 23 Feb 2016 10:35:16 GMT; HttpOnly' ],
        location: 'http://www.someurl.co.uk',
        vary: 'Accept',
        'content-type': 'text/plain; charset=utf-8',
        'content-length': '54',
        date: 'Tue, 23 Feb 2016 09:35:16 GMT',
        connection: 'close'
    }
}

I'm using version 2.0.1 of chai-http.

@keithamus
Copy link
Member

Thanks for the issue @arnaugm. Could you please provide the same info @Limess has - to help us diagnose this problem?

@Limess - in your case, it looks as though because you have set the Domain value of your cookie (Domain=.someurl.co.uk;) that the default settings chai-http uses wont be able to access it. On http.js#L346 we supply a generic new Cookie.CookieAccessInfo() param (which is a required param for node), but the way CookieJar works is that this instance can only read domain-less cookies.

The way I see it - we have two fixes:

  1. Chai-http provides a way to configure CookieAccessInfo; but I'm not sure what the most expressive way is to do this.
  2. The underlying Cookiejar lib provides a way of accessing all cookies, irrespective of CookieAccessInfo settings. See Provide facility for retrieving all cookies; a wildcard CookieAccessInfo bmeck/node-cookiejar#24 for more.

@arnaugm
Copy link
Author

arnaugm commented Feb 23, 2016

Sorry @keithamus I don't have access to that code any more, it was long ago. I'll let you know if I face the problem again.

@keithamus
Copy link
Member

Ah yes I see now, sorry I don't know how I missed your original comment - I only just got a notification from @Limess' comment.

It was likely a similar problem, it seems like new Cookie.CookieAccessInfo() is actually quite fragile and would ignore a lot of cookies with various extra settings.

@keithamus
Copy link
Member

Hey all, I'm marking this as PR wanted, as bmeck/node-cookiejar#24 has been fixed and we're using 2.1.0 of cookiejar, so we can fix this issue.

It should be a case of changing http.js#L346 from

cookie = cookie.getCookie(key, new Cookie.CookieAccessInfo());

to

cookie = cookie.getCookie(key, new Cookie.CookieAccessInfo.All);

and then of course, adding tests in our test/http.js file.

@leggsimon
Copy link
Contributor

leggsimon commented Oct 26, 2016

I'm having a look at fixing this issue but I'm not 100% sure I understand how the issue is caused. Is it due to the fact that there are spaces in the cookie string? eg .
some_cookie=defgh-abcde; Domain=.someurl.co.uk;
as it is now it doesn't seem as though it would pick up the Domain cookie value.

So in the tests I've changed

var req = {
  headers: {
    'cookie': ['name=value;name2=value2;']
  }
};

to

var req = {
  headers: {
    'cookie': ['name=value;name2=value2; name3=value3;']
  }
};

and am working on the assumption that I should be asserting the existence of the new name3 cookie. Is that right?

@keithamus
Copy link
Member

@leggsimon the issue is that a cookie like this:

'some_cookie=defgh-abcde; Domain=.someurl.co.uk; Path=/; Expires=Tue, 23 Feb 2016 10:35:16 GMT; HttpOnly' 

Which has a domain (.someurl.co.uk) cannot be read currently, because CookieJar's API expects us to pass in an access info with a matching domain, so currently the code does this:

cookie = cookie.getCookie(key, new Cookie.CookieAccessInfo());

CookieAccessInfo() is domainless, and cannot read cookies with a domain. So trying to read a cookie with a domain gets ignored. If we had this code instead:

cookie = cookie.getCookie(key, new Cookie.CookieAccessInfo('.someurl.co.uk'));

Then we could read all cookies with a Domain value of .someurl.co.uk. Obviously this doesn't scale past 1 domain, so we added a new feature to CookieJar, which allows us to provide a "sentinel value" which can access all cookies regardless of the domain setting:

cookie = cookie.getCookie(key, new Cookie.CookieAccessInfo.All);

So, in the cookie tests, where we setup the req object (test/http.js#L271-278, if we add a new cookie with a domain:

    var res = {
      headers: {
        'set-cookie': [
          'name=value',
          'name2=value2; Expires=Wed, 09 Jun 2021 10:18:14 GMT',
          'name3=value3; Domain=somedomain.co.uk',
        ]
      }
    };

And we assert that we can get the cookie:

 res.should.have.cookie('name3', 'value3');

When we run these tests against existing code, we should see them fail. However if we change that line (the CookieAccessInfo() bit to CookieAccessInfo.All) we should see the tests pass, and the feature is fixed 😄

@leggsimon
Copy link
Contributor

Riiiiight ok. Thanks @keithamus I think I need to have a look into and understand Domain In a cookie. I'll get back to it.

@leggsimon
Copy link
Contributor

Apologies for the delay in getting to this, but a quick update: It seems as though it isn't as straightforward as your suggestion @keithamus.
I would like to go through the problems I'm facing with one of you when you get a chance though although I'm not sure if this is the best place to do it. Are any of you on Gitter at all or some chat thing? (Keith do you still have access to our Slack?)

@keithamus
Copy link
Member

@leggsimon I have been banished from the FT Slack, exiled to a far away land. I hang out on the official Chai Slack chat and Gitter chat though. Feel free to DM on either one 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants