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

feat: add secure cookie override to agent #1515

Merged
merged 1 commit into from
Jan 7, 2020
Merged

Conversation

ejose19
Copy link
Contributor

@ejose19 ejose19 commented Oct 3, 2019

Allow overriding the default secure cookie for Cookie access info (use case: doing tests directly with express / koa app without nginx / tls certs)

@ejose19
Copy link
Contributor Author

ejose19 commented Oct 3, 2019

Worth noting, I had to bump the engine -> node to 7.0.0 because I was getting a version error from http2 wrapper.

Also, this commit goes in hand with this commit for supertest, on which I will wait for this PR to be merged first as the tests for supertest commit requires proposed changes.

End usage:

const agent = supertest.agent(appCB, { secureCookie: true });

@niftylettuce
Copy link
Collaborator

Can you please add a test? Also it looks like there is a merge conflict that will need fixed.

@ejose19
Copy link
Contributor Author

ejose19 commented Jan 7, 2020

@niftylettuce Should be all good be now.

@niftylettuce niftylettuce merged commit 737697f into ladjs:master Jan 7, 2020
@ejose19
Copy link
Contributor Author

ejose19 commented Jan 8, 2020

@niftylettuce Just one note on this. Agent is not behaving correctly for both .sendSecureCookie() and .disableTLSCerts()

Example:

request
  .get(url)
  .disableTLSCerts()
  .then(() => {});


request
  .get(url)
  .then(() => {});

This one works as expected, only on the first request TLSCerts are disabled (as it was called as a method not as an option). However when you do

agent
  .get(url)
  .disableTLSCerts()
  .then(() => {});

agent
  .get(url)
  .then(() => {});

Agent disables TLSCerts on both calls (same with .sendSecureCookie, as for some reason the default is being set ) I've digged on the code and it seems agent has no clear way to differentiate when one of those has been set as an option or as a method (one time).

Thoughts?

@niftylettuce
Copy link
Collaborator

I've reverted in meanwhile

@ejose19
Copy link
Contributor Author

ejose19 commented Jan 8, 2020

@niftylettuce Why did you revert it tho? The issue is not with this commit, but how agent handles options / vs methods. That will need a separate work / refactor to solve that.

I was working on supertest commit waiting for you to release this version.

@ejose19
Copy link
Contributor Author

ejose19 commented Jan 28, 2020

@niftylettuce Any updates? This is a useful feature to run tests against koa/express server directly.

@niftylettuce
Copy link
Collaborator

Can you submit the commit per this? #1515 (comment)

@ejose19
Copy link
Contributor Author

ejose19 commented Feb 17, 2020

@niftylettuce if you can give me a hand to resolve it I can contribute, the problem is with agent-base not differentiating when a property is given from initialization or from method, so it carries out all properties when used as method for next calls (even if the same method is not used).

@niftylettuce
Copy link
Collaborator

I cannot dedicate the time, I apologize

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

Successfully merging this pull request may close these issues.

2 participants