Use of request's global cookie `jar` #73

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

Comments

Projects
None yet
4 participants
@rapidrapids
Contributor

rapidrapids commented May 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@dscape

dscape May 17, 2012

Collaborator

@mikeal is this an issue in request?

+@pgte @indexzero

Collaborator

dscape commented May 17, 2012

@mikeal is this an issue in request?

+@pgte @indexzero

@dscape

This comment has been minimized.

Show comment Hide comment
@dscape

dscape May 17, 2012

Collaborator

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.

Collaborator

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

This comment has been minimized.

Show comment Hide comment
@rapidrapids

rapidrapids May 17, 2012

Contributor

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);
Contributor

rapidrapids commented May 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@pgte

pgte May 17, 2012

Contributor

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.

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

This comment has been minimized.

Show comment Hide comment
@rapidrapids

rapidrapids May 17, 2012

Contributor

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

Contributor

rapidrapids commented May 17, 2012

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

This comment has been minimized.

Show comment Hide comment
@mikeal

mikeal May 17, 2012

Collaborator

nano can default to false.

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

mikeal commented May 17, 2012

nano can default to false.

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

This comment has been minimized.

Show comment Hide comment
@rapidrapids

rapidrapids May 17, 2012

Contributor

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

Contributor

rapidrapids commented May 17, 2012

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

@dscape

This comment has been minimized.

Show comment Hide comment
@dscape

dscape May 17, 2012

Collaborator

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

Collaborator

dscape commented May 17, 2012

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

@rapidrapids rapidrapids referenced this issue May 22, 2012

Closed

Cookies #77

dscape added a commit that referenced this issue May 23, 2012

dscape added a commit that referenced this issue Jun 20, 2012

[docs minor] shows the new auth apis
* relates to #81, #77 and #73
* cc @beardtwizzle
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment