Fix request cookies lost during multiple redirects

[ash0080]

Keep original request cookie safe during multiple redirects

[tomas]

Would you please add a test that demonstrates the error and fix?

[ash0080]

DONE!

[ash0080]

Could this be merged asap, thanks

[ash0080]

Any news?

[tomas]

I'm still not sure what this PR fixes. Doesn't the existing follow_set_cookies option address this?

[ash0080]

I'm still not sure what this PR fixes. Doesn't the existing follow_set_cookies option address this?

This is for authorized request, for example when you login in a website and then post a form in another page, this fix keeps the request cookie safe (keep your login status ) during multiple redirects.
before the fix, you can't do this

[tomas]

But that's precisely what follow_set_cookies does.

What you're doing here is putting your request cookies as part of the response cookies, which are different things.

[ash0080]

But that's precisely what follow_set_cookies does.

What you're doing here is putting your request cookies as part of the response cookies, which are different things.

No, maybe you think “follow_set_cookies” works, but actually it's not.

“follow_set_cookies” only keeps “set-cookies” in response's headers, but the "logged in cookies"(ex. a session ) you passed in will be lost after the 1st redirect. then during the 2nd request ( session has been dropped ), a 403 will be trigged, so the request cannot be finished. what I fixed is this issue.

get 'logged in cookie' from whatever place
request with ’logged in cookie‘ -> 1st redirect ... -> nth redirect ' the logged in cookie' should be kept during all these redirects to keep your login status.

Maybe you can run a practical test to see what it happening.
I modified this code because I encountered this issue in real case.

[ash0080]

No news？

[tomas]

Can you provide a real-world example where I can test this?

[ash0080]

Too complicate to provide a real case,
Instead, I wrote a quick demo for you.

without this PR, test result is FAIL
with this PR, test result is PASS

const fastify = require('fastify')({ logger: false })

fastify.register(require('fastify-cookie'), {
  parseOptions: {}     // options for parsing cookies
})

fastify.addHook('onRequest', (request, reply, done) => {
  const session = request.cookies.session
  console.log(session)
  if (session !== 'i am a session') {
    reply.send('Forbidden').code(403)
  }
  done()
})

fastify.get('/', async (request, reply) => {
  reply
    .setCookie('r1', 'r1', {
      path: '/'
    })
    .redirect('/redirect1')
    .code(302)
  return 'redirect to /redirect1'
})

fastify.get('/redirect1', async (request, reply) => {
  reply
    .setCookie('r2', 'r2', {
      path: '/'
    })
    .redirect('/redirect2')
    .code(302)
  return 'redirect to /redirect2'
})

fastify.get('/redirect2', async (request, reply) => {
  reply.type('application/json').code(200)
  return { success: true, message: 'you gotcha me!' }
})

fastify.listen(3000, (err, address) => {
  if (err) throw err
  fastify.log.info(`server listening on ${address}`)
})

const test      = require('ava')
const needle    = require('needle')
// const needleMod = require('@ash0080/needle')

test('needle follow_set_cookie should work for redirects', async t => {
  try {
    const resp = await needle('get', 'http://localhost:3000', {
      headers: {
        cookie: 'session=i am a session'
      },
      follow_max: 5,
      follow_set_cookies: true,
      follow_set_referer: true,
      parse_cookies: true
    })
    console.log(resp.cookies)
    const statusCode = resp.statusCode
    if (statusCode === 200 && resp.body && resp.body.success === true) {
      t.pass()
    }
  } catch (error) {
    t.fail()
  }
})

[ash0080]

@tomas Does it work?

[ash0080]

any news?

[tomas]

Ok I finally had the time to take a deeper look into this.

So yes, you're right about the request cookies not being passed to redirects even if the follow_set_cookies is set to true. The reason is that I was only considering the response cookies when writing that header, and I agree we should include any original request cookies to those new requests.

What I don't like about this PR is that you're setting the request cookies as the response cookies (config.stored_cookies) in order to achieve that, which means you'll always get the original cookies you passed when calling needle in the response object, which isn't right. resp.cookies should by definition contain only the response cookies, regardless of what happened in between redirects.

So I'm going to pull your branch but update the code to avoid polluting the resp.cookies object. I'll see if I can include the test case you provided within the official test suite as well.

Thanks again and sorry for the late response. :)

[ash0080]

Agree, It's only a mimic fix try to reuse the current codes without inviting the new variable.
Absolutely It could be better.

But in practical usage,
People almost always use a _.pick to filter the response cookies want to keep
cuz there're so many logs, tacking ... mess cookies in real world only make things worse.

[aaronmusknew]

This problem is located in neddle.js on line 533

else if (config.headers['cookie']) {
debug('Clearing original request cookie', config.headers['cookie']);
delete config.headers['cookie'];
}

I deleted this and solved problem.