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

Make the CSRF token location configurable #198

Closed
wants to merge 1 commit into from
Closed

Make the CSRF token location configurable #198

wants to merge 1 commit into from

Conversation

theasp
Copy link
Collaborator

@theasp theasp commented Feb 2, 2016

Rather than having the CSRF location hardcoded, allow specifying
:csrf-path for make-channel-socket-server!, which is passed to clients during handshake, describing where a client is to place the CSRF token for posts, to match the configuration of the server. This allows the server to use the standard middleware without any options for both ring-anti-forgery and csurf when you use a value of [:headers :X-CSRF-Token]. The default is [:params :csrf-token] to maintain compatibility with previous versions.

The example server has been upgraded to use [:headers :X-CSRF-Token], and the example client checks to see if there was an error to determine login success or not.

Rather than having the CSRF location hardcoded, allow specifying
`:csrf-path` for `make-channel-socket-server!`, which is passed to
clients during handshake, describing where a client is to place the CSRF
token for posts, to match the configuration of the server.  This allows
the server to use the standard middleware without any options for both
ring-anti-forgery and csurf when you use a value of `[:headers
:X-CSRF-Token]`.  The default is `[:params :csrf-token]` to maintain
compatibility with previous versions.

The example server has been upgraded to use `[:headers :X-CSRF-Token]`,
and the example client checks to see if there was an error to determine
login success or not.
@ptaoussanis
Copy link
Member

Hi Andrew, thanks for your work on this.

Going to want a little time to consider. May I clarify: the reason for this change is because you don't like having to mod your Ring middleware to suit where Sente sticks its csrf token in requests?

@theasp
Copy link
Collaborator Author

theasp commented Feb 3, 2016

No rush. Yes, that is essentially the reason, for both ring and express, and eliminating differences between the two. It may make it easier for someone to add sente to an existing app too. If you decide you don't want it, no big deal.

Thanks

@ptaoussanis
Copy link
Member

Okay, thanks for the clarification.

ptaoussanis added a commit that referenced this pull request Feb 5, 2016
ptaoussanis added a commit that referenced this pull request Feb 5, 2016
ptaoussanis added a commit that referenced this pull request Feb 5, 2016
…, etc.

Thanks to @theasp for bringing attention to this
@ptaoussanis
Copy link
Member

Hi Andrew, closing this - will be addressed in the next beta release.

Elected to just duplicate the :csrf-token param into the "X-CSRF-Token" header to avoid needing to introduce a new config opt + the client<->server opt sync complexity.

Thanks for bringing this to my attention. Cheers :-)

@ptaoussanis ptaoussanis closed this Feb 5, 2016
@theasp
Copy link
Collaborator Author

theasp commented Feb 5, 2016

That works too! Thanks!

ptaoussanis added a commit that referenced this pull request Feb 26, 2016
…, etc.

Thanks to @theasp for bringing attention to this
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