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

POST request JSON must be added to req after the attachment of cookies--why? #352

Open
ihinsdale opened this issue Apr 3, 2014 · 7 comments

Comments

@ihinsdale
Copy link

The following test passes:

var request = require('supertest');
var superagent = require('superagent');

describe('Example:', function() {
  var agent;
  beforeEach(function(done) {
    agent = superagent.agent();
    var req = request('http://example.com').get('/getsomecookies');
    req.end(function(err, result) {
      if (!err) {
        agent.saveCookies(result.res);
        done();
      } else {
        done(err);
      }
    });
  });

  it('should successfully post some JSON', function(done) {
    var req = request('http://example.com').post('/postsomejson')
    agent.attachCookies(req);
    req.send({my: 'JSON'});
    var csrfToken = (/XSRF-TOKEN=(.*?);/.exec(req.cookies)[1]);
    req.set('X-XSRF-TOKEN', unescape(csrfToken));
    req.end(function(err, result){
      expect(res.status).to.eql(200);
      done();
    });
  });
});

However, simply swapping the attachment of cookies with the setting of the JSON causes the test to fail--the server responds with 403. That is, the following fails:

  it('should successfully post some JSON', function(done) {
    var req = request('http://example.com').post('/postsomejson').send({my: 'JSON'});
    agent.attachCookies(req);
    ...
  });

When I look on the server (express proxied by nginx, using the express.csrf middleware) to see what is going on, in the unsuccessful case the secret loaded from req.session.csrfSecret, which is used to match the X-XSRF-TOKEN header, is undefined. In the successful case, the secret loaded from req.session.csrfSecret is exactly what it should be, namely the secret created by the GET request to /getsomecookies.

Interestingly, if I make the POST request without trying to send any JSON, I at least don't get a 403 response (though the test would fail with a 400 because I didn't provide any JSON for the server to do something with). That is, this works too:

  it('should successfully post some JSON', function(done) {
    var req = request('http://example.com').post('/postsomejson');
    agent.attachCookies(req);
    ...
  });

So, what is it about attachCookies that is interfering with the setting of the JSON sent in the request? Clearly the server is seeing something different in each case. Is this problem documented anywhere?

@mjomble
Copy link

mjomble commented Dec 18, 2014

I ran into a similar problem. It seems like it's not that attachCookies is interfering with setting the JSON, but the other way around - using send() breaks subsequent attachCookies() calls.

If you look at https://github.com/visionmedia/superagent/blob/master/lib/node/index.js, you can see that Request.prototype.send calls this.request() which adds the cookies to the header, but also caches its result:

Request.prototype.request = function(){
  if (this.req) return this.req;
...
  // add cookies
  if (this.cookies) req.setHeader('Cookie', this.cookies);

Which explains why attachCookies() must be called before send() or in fact before anything else that internally calls this.request(), otherwise a version of the headers without the cookies will be cached and returned on all subsequent calls.

@ihinsdale
Copy link
Author

@mjomble Thanks for that clear explanation!

@mjomble
Copy link

mjomble commented Dec 19, 2014

Perhaps the issue should remain open as it's fairly unexpected and undocumented behavior. Ideally it should work regardless of the order of send() and attachCookies() or at least throw an error if you're using them in an unsupported order, rather than discarding the cookies silently.

@ihinsdale ihinsdale reopened this Dec 19, 2014
@Dynasty9
Copy link

Dynasty9 commented Feb 5, 2015

👍

1 similar comment
@waltonseymour
Copy link

+1

@stuartpb
Copy link

stuartpb commented Mar 7, 2015

Where is any of this saveCookies/attachCookies stuff documented? I don't see any of it in https://github.com/visionmedia/superagent/blob/master/test/node/agency.js

Furthermore, depending on its behavior to be used externally at all is a defect, since these functions are specifically marked "private" in the comments preceding their definition.

@stuartpb
Copy link

stuartpb commented Mar 7, 2015

Also, this means that agent.get(url).end(cb) doesn't get its cookies set while agent.get(url,cb) does, which is completely bonkers. I'm assuming this is a regression?

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

No branches or pull requests

5 participants