Skip to content
This repository has been archived by the owner on Nov 5, 2018. It is now read-only.

Use of request's global cookie jar #73

Closed
rapidrapids opened this issue May 17, 2012 · 8 comments
Closed

Use of request's global cookie jar #73

rapidrapids opened this issue May 17, 2012 · 8 comments

Comments

@rapidrapids
Copy link
Contributor

Sorry to keep firing issues at you - don't worry, I'll do a pull request for this if you agree its a problem. :)


request uses a global cookie jar, into which all response cookies are stored, and then used on subsequent requests. This behaviour is enabled by default; you have to set jar to false to disable it.

I believe that nano should always default for jar to false.

Because of the nature of nano and CouchDB, it is likely that you will doing multiple nano requests as different users (especially if you have a 1 user to 1 database set up) within a single method.

This probably hasn't been noticed before as nano hasn't had support for CouchDB's cookie authentication. But with this coming (#71), it is especially problematic having jar enabled by default - it means that if you have performed an action as a logged in standard user (cookie auth), and then attempt something as admin (using basic auth) - the admin request will fail - because the cookie jar sends over the auth cookie of the standard user - irrespective of whether you have isolated nano instances (because request's cookie jar is global).

See this gist for an example: https://gist.github.com/2717116


It should just be a case of changing this code from:

if (opts.jar) { 
  req.jar = opts.jar;
}

to

req.jar = opts.jar || false;

The interim workaround is to do the above, by using request_defaults (as below), but as I see no benefit for nano in using a cookie jar in any scenario, I think the fix above should be baked in instead.

var nano = require('nano')({ url: 'http://localhost:5984', request_defaults: { jar: false }});
@dscape
Copy link
Contributor

dscape commented May 17, 2012

@mikeal is this an issue in request?

+@pgte @indexzero

@dscape
Copy link
Contributor

dscape commented May 17, 2012

I would like to have this patched in request in everything that concerns http client issues and nano in everything that concerns CouchDB.

I'm still looking at your code and comments and trying to figure this out myself, so I should get back to you soon.

@rapidrapids
Copy link
Contributor Author

I believe the problem is that nano doesn't (perhaps can't) instantiate request. This means that they all reference back to the same place (and therefore the same cookie jar). Try this out for example:

var fn1 = function () {
    var request1 = require('request');
    console.log('Should be undefined', request1.testProp);
    request1.testProp = 1;
    console.log('Should be 1', request1.testProp);
},
fn2 = function () {
    var request2 = require('request');
    console.log('Should be undefined', request2.testProp);
    request2.testProp = 2;
    console.log('Should be 2', request2.testProp);
};

fn1();
fn2();

console.log('Should be undefined', require('request').testProp);

@pgte
Copy link
Contributor

pgte commented May 17, 2012

my 2 cents: I know that by default request uses the same cookie jar for all requests.
Maybe nano should expose the request options.jar option?

FYI, you can construct a cookie jar in request by calling:

var jar = request.jar()

and you can associate it to a request like this:

request({uri: uri, jar: jar})

if you don't want to use the cookie jar at all just pass jar: false.

@rapidrapids
Copy link
Contributor Author

I agree that this needs to be an option, however I personally believe that the default should be false, as there is no genuine reason for nano using a cookie jar - that I can think of.

Its also confusing because whilst you can create separate instances of nano (and use them in isolation), the underlying request is a reference, not an instance - leading to potential confusion in the area of a jar. IMO

@mikeal
Copy link

mikeal commented May 17, 2012

nano can default to false.

var request = require('request').defaults({jar:false})

@rapidrapids
Copy link
Contributor Author

Sorry, ignore 8a7fb4d - that was an accidental push! :)

@dscape
Copy link
Contributor

dscape commented May 17, 2012

This looks awesome. I'll merge and review as soon as I get a free cycle!

@rapidrapids rapidrapids mentioned this issue May 22, 2012
dscape added a commit that referenced this issue May 23, 2012
dscape added a commit that referenced this issue Jun 20, 2012
* relates to #81, #77 and #73
* cc @beardtwizzle
KangTheTerrible pushed a commit to KangTheTerrible/nano that referenced this issue Aug 19, 2018
Updates cloudant-follow to 0.16.3 and request to 2.83
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants