Skip to content

Autoset secure cookie - #209

Closed
fracmak wants to merge 1 commit into
expressjs:masterfrom
fracmak:autoSecureCookie
Closed

Autoset secure cookie#209
fracmak wants to merge 1 commit into
expressjs:masterfrom
fracmak:autoSecureCookie

Conversation

@fracmak

@fracmak fracmak commented Sep 15, 2015

Copy link
Copy Markdown
Contributor

Added ability to automatically set the secure cookie value based on the trust proxy settings when autoSecure option is set.

This is useful when you're working in mixed environments where you're developing locally in an insecure environment, but the higher environments are secure and you want to trust express's request.secure option for scoping your cookies.

@dougwilson

Copy link
Copy Markdown
Contributor

Hi! Can you add some documentation to the readme as well as make this opt-in behavior?

Comment thread index.js Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will never work when serving requests on parallel both secure and not, because you are altering global state that belongs to multiple requests at the same time. This will have to be fixed before we can accept the PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good catch, let me fix that

@dougwilson

Copy link
Copy Markdown
Contributor

Once the two issues are addressed, I think this is useful functionality that we can merge!

@fracmak

fracmak commented Sep 15, 2015

Copy link
Copy Markdown
Contributor Author

issues addressed

…he trust proxy settings assuming the cookie secure option isn't already set
@dougwilson

Copy link
Copy Markdown
Contributor

Thanks again! This has just been merged with a slight modification of simplifying the API to just be { cookie: { secure: 'auto' } }.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants