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

One token per form #14

Closed
g105b opened this issue Mar 2, 2016 · 9 comments
Closed

One token per form #14

g105b opened this issue Mar 2, 2016 · 9 comments

Comments

@g105b
Copy link
Member

g105b commented Mar 2, 2016

Rather than one token for all forms, this will allow ajax requests to use the already generated tokens rather than having to generate their own.

@j4m3s
Copy link
Contributor

j4m3s commented Mar 2, 2016

Adding one token per form will significantly increase the attack surface for the vast majority of web apps, because lots of valid tokens will never be consumed, leaving them available for compromise.

However, separate tokens per form would be required if an application is to use AJAX to allow multiple forms per page to be submitted.

How about adding a parameter to the protectAndInject() method, allowing the caller to force generation of individual tokens per form?

@g105b
Copy link
Member Author

g105b commented Mar 2, 2016

Coincidentally, when I was submitting this repo to packagist, I forgot to
submit the page and came back a few hours later. I submitted the form and
it showed me an error that the csrf token had expired, and I simply had to
submit the form again.

This could be the solution to the problem of having many tokens out in the
wild, as I think I'd prefer to have my pages Ajax ready as standard.

On Wed, 2 Mar 2016, 20:41 James Fellows, notifications@github.com wrote:

Adding one token per form will significantly increase the attack surface
for the vast majority of web apps, because lots of valid tokens will never
be consumed, leaving them available for compromise.

However, separate tokens per form would be required if an application
is to use AJAX to allow multiple forms per page to be submitted.

How about adding a parameter to the protectAndInject() method, allowing
the caller to force generation of individual tokens per form?


Reply to this email directly or view it on GitHub
#14 (comment).

@j4m3s
Copy link
Contributor

j4m3s commented Mar 2, 2016

In the ArrayTokenStore, the tokens would effectively expire when the session expires. Are you suggesting having tokens automatically expire after a period of time? That would be a separate feature I guess.

It would have a performance impact - I'd need to check every token's timestamp against the current time, but it's easy enough.

@g105b
Copy link
Member Author

g105b commented Mar 2, 2016

I don't know whether it's a better solution, but it was certainly how Packagist did it.

Regarding performance, wouldn't the timestamp only need to be checked when there is a POST made? Not every time the page renders?

I'll do some research as to how other frameworks do it.

@j4m3s
Copy link
Contributor

j4m3s commented Mar 2, 2016

Yes good point - in which case this would form a tiny part of the POST processing time.

@j4m3s
Copy link
Contributor

j4m3s commented Mar 2, 2016

Just realised that we could be missing the point here. If the developer is going to use ajax for submitting forms, they will need a way of refreshing tokens on the client-side anyway - in case the user submits the same form twice.
My knowledge of the workings of ajax/ javascript is rather poor - wouldn't it completely miss the point of the CSRF protection if we provided a URL that hands-out tokens willy-nilly? Or would the tokens be associated with the session from which they originated, so be "secure" so long as the javascript runs in the same ssl session?

@g105b
Copy link
Member Author

g105b commented Mar 3, 2016

You're falling into the trap of jquery style ajax, where page logic is duplicated client side.

The clean way of using ajax to submit forms is to simply perform a standard HTTP request, and just replace the part of the DOM that changes, including the new CSRF token from the new page.

@j4m3s
Copy link
Contributor

j4m3s commented Mar 3, 2016

Isn't this library supposed to be good for the jquery crew too? :)

@g105b
Copy link
Member Author

g105b commented Mar 4, 2016

We shouldn't introduce something that promotes bad coding; there's nothing inherently wrong with jquery's ajax, stopping jquery coders using this repo - I was referring to the tonne of bad advice online that gives jquery a bad name.

To be clear: JavaScript/jquery/curl can use this library, but we should not introduce any mechanism for arbitrarily generating new tokens (via a URL call, for example), just to satisfy bad coding style.

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

No branches or pull requests

2 participants