Fix not keeping track of correct URL when setting cookies durin… (#5455)

* Keep track of correct URL during request redirects

* Add snapshots for URLs during redirects

packages/server/__snapshots__/request_spec.coffee.js

@@ -0,0 +1,65 @@
+exports['lib/request #sendPromise followRedirect gets + attaches the cookies at each redirect 1'] = {
+  "setCalls": [
+    {
+      "currentUrl": "http://localhost:1234/",
+      "setCookie": "foo=bar"
+    },
+    {
+      "currentUrl": "http://localhost:1234/B",
+      "setCookie": "bar=baz"
+    },
+    {
+      "currentUrl": "http://localhost:1234/B",
+      "setCookie": "foo=bar"
+    },
+    {
+      "currentUrl": "http://localhost:1234/",
+      "setCookie": "quuz=quux"
+    }
+  ],
+  "getCalls": [
+    {
+      "newUrl": "http://localhost:1234/"
+    },
+    {
+      "newUrl": "http://localhost:1234/B"
+    },
+    {
+      "newUrl": "http://localhost:1234/B"
+    },
+    {
+      "newUrl": "http://localhost:1234/B"
+    }
+  ]
+}
+
+exports['lib/request #sendStream gets + attaches the cookies at each redirect 1'] = {
+  "setCalls": [
+    {
+      "currentUrl": "http://localhost:1234/",
+      "setCookie": "foo=bar"
+    },
+    {
+      "currentUrl": "http://localhost:1234/B",
+      "setCookie": "bar=baz"
+    },
+    {
+      "currentUrl": "http://localhost:1234/B",
+      "setCookie": "foo=bar"
+    }
+  ],
+  "getCalls": [
+    {
+      "newUrl": "http://localhost:1234/"
+    },
+    {
+      "newUrl": "http://localhost:1234/B"
+    },
+    {
+      "newUrl": "http://localhost:1234/B"
+    },
+    {
+      "newUrl": "http://localhost:1234/B"
+    }
+  ]
+}

packages/server/lib/request.coffee

@@ -419,7 +419,7 @@ module.exports = (options = {}) ->
     setRequestCookieHeader: (req, reqUrl, automationFn) ->
       automationFn('get:cookies', { url: reqUrl })
       .then (cookies) ->
-        debug('getting cookies from browser %o', { reqUrl, cookies })
+        debug('got cookies from browser %o', { reqUrl, cookies })
         header = cookies.map (cookie) ->
           "#{cookie.name}=#{cookie.value}"
         .join("; ") || undefined
@@ -475,21 +475,24 @@ module.exports = (options = {}) ->
 
       followRedirect = options.followRedirect
 
+      currentUrl = options.url
+
       options.followRedirect = (incomingRes) ->
         req = @
 
-        newUrl = url.resolve(options.url, incomingRes.headers.location)
+        newUrl = url.resolve(currentUrl, incomingRes.headers.location)
 
         ## and when we know we should follow the redirect
         ## we need to override the init method and
         ## first set the received cookies on the browser
         ## and then grab the cookies for the new url
         req.init = _.wrap req.init, (orig, opts) =>
           options.onBeforeReqInit ->
-            self.setCookiesOnBrowser(incomingRes, options.url, automationFn)
+            self.setCookiesOnBrowser(incomingRes, currentUrl, automationFn)
             .then (cookies) ->
               self.setRequestCookieHeader(req, newUrl, automationFn)
             .then (cookieHeader) ->
+              currentUrl = newUrl
               orig.call(req, opts)
 
         followRedirect.call(req, incomingRes)
@@ -552,8 +555,10 @@ module.exports = (options = {}) ->
           requestResponses.push(pick(response))
 
         if options.followRedirect
+          currentUrl = options.url
+
           options.followRedirect = (incomingRes) ->
-            newUrl = url.resolve(options.url, incomingRes.headers.location)
+            newUrl = url.resolve(currentUrl, incomingRes.headers.location)
 
             ## normalize the url
             redirects.push([incomingRes.statusCode, newUrl].join(": "))
@@ -567,10 +572,11 @@ module.exports = (options = {}) ->
             ## first set the new cookies on the browser
             ## and then grab the cookies for the new url
             req.init = _.wrap req.init, (orig, opts) =>
-              self.setCookiesOnBrowser(incomingRes, options.url, automationFn)
+              self.setCookiesOnBrowser(incomingRes, currentUrl, automationFn)
               .then ->
                 self.setRequestCookieHeader(req, newUrl, automationFn)
               .then ->
+                currentUrl = newUrl
                 orig.call(req, opts)
 
             ## cause the redirect to happen

packages/server/test/unit/request_spec.coffee

@@ -3,9 +3,44 @@ require("../spec_helper")
 _       = require("lodash")
 http    = require("http")
 Request = require("#{root}lib/request")
+snapshot = require("snap-shot-it")
 
 request = Request({timeout: 100})
 
+testAttachingCookiesWith = (fn) ->
+  set = sinon.spy(request, 'setCookiesOnBrowser')
+  get = sinon.spy(request, 'setRequestCookieHeader')
+
+  nock("http://localhost:1234")
+  .get("/")
+  .reply(302, "", {
+    'set-cookie': 'foo=bar'
+    location: "/B"
+  })
+  .get("/B")
+  .reply(302, "", {
+    'set-cookie': 'bar=baz'
+    location: "/B"
+  })
+  .get("/B")
+  .reply(200, "", {
+    'set-cookie': 'quuz=quux'
+  })
+
+  fn()
+  .then ->
+    snapshot({
+      setCalls: set.getCalls().map (call) ->
+        {
+          currentUrl: call.args[1],
+          setCookie: call.args[0].headers['set-cookie']
+        }
+      getCalls: get.getCalls().map (call) ->
+        {
+          newUrl: _.get(call, 'args.1')
+        }
+    })
+
 describe "lib/request", ->
   beforeEach ->
     @fn = sinon.stub()
@@ -722,6 +757,12 @@ describe "lib/request", ->
           expect(resp.status).to.eq(200)
           expect(resp).not.to.have.property("redirectedToUrl")
 
+      it "gets + attaches the cookies at each redirect", ->
+        testAttachingCookiesWith =>
+          request.sendPromise({}, @fn, {
+            url: "http://localhost:1234/"
+          })
+
     context "form=true", ->
       beforeEach ->
         nock("http://localhost:8080")
@@ -843,3 +884,16 @@ describe "lib/request", ->
         beginFn()
         expect(request.create).to.be.calledOnce
         expect(request.create).to.be.calledWith(options)
+
+    it "gets + attaches the cookies at each redirect", ->
+      testAttachingCookiesWith =>
+        request.sendStream({}, @fn, {
+          url: "http://localhost:1234/"
+          followRedirect: _.stubTrue
+        })
+        .then (fn) =>
+          req = fn()
+
+          new Promise (resolve, reject) =>
+            req.on('response', resolve)
+            req.on('error', reject)