harmen/hypha

Store CSRF token in a cookie #331

Merged

matthijskooijman merged 6 commits from csrf-in-cookie into master

2020-07-14 09:25:07 +00:00

matthijskooijman commented

2020-06-25 12:08:47 +00:00

(Migrated from github.com)

This is a subset of PR #316, with just the commits needed to move the CSRF token into a cookie. This was originally just a cleanup, but it has since been shown that having the CSRF token in the session can cause dataloss when visitors submit a (lengthy) comment and the session has expired since they first loaded the article page. In that case, the CSRF token will be invalid and the request rejected rather forcefully, losing the submitted comment.

Moving the CSRF token to a cookie ensures that the token survives until the browser closes, which should prevent this dataloss.

This is a subset of PR #316, with just the commits needed to move the CSRF token into a cookie. This was originally just a cleanup, but it has since been shown that having the CSRF token in the session can cause dataloss when visitors submit a (lengthy) comment and the session has expired since they first loaded the article page. In that case, the CSRF token will be invalid and the request rejected rather forcefully, losing the submitted comment. Moving the CSRF token to a cookie ensures that the token survives until the browser closes, which should prevent this dataloss.

RoukePouw (Migrated from github.com) reviewed

2020-06-25 13:00:30 +00:00

system/core/events.php

Would it make sense to add this global/static var to $O_O?
Feels weird to have such a state variable lying around :-)

Would it make sense to add this global/static var to `$O_O`? Feels weird to have such a state variable lying around :-)

RoukePouw (Migrated from github.com) reviewed

2020-06-25 13:01:36 +00:00

system/core/HyphaRequest.php

r edundant parentheses (non blocking comment :-) ) maybe triple === ?

r edundant parentheses (non blocking comment :-) ) maybe triple `===` ?

RoukePouw (Migrated from github.com) reviewed

2020-06-25 13:02:53 +00:00

system/core/RequestContext.php

namingwise I would expect get to be just a getter (not meddling with state) the regeneration breaks that expectation. Maybe getOrGenerateCsrfToken / retrieveCsrfToken?

namingwise I would expect get to be just a getter (not meddling with state) the regeneration breaks that expectation. Maybe getOrGenerateCsrfToken / retrieveCsrfToken?

RoukePouw (Migrated from github.com) reviewed

2020-06-25 13:03:17 +00:00

system/core/RequestContext.php

redundant parentheses, maybe triple ===?

redundant parentheses, maybe triple ===?

matthijskooijman (Migrated from github.com) reviewed

2020-06-25 13:39:32 +00:00

system/core/events.php

Hm, I'm not so sure. This sneakily hidden state is not ideal, exposing this state globally through $O_O does not seem correct either: This is really an implementation detail of executePostedCommand. I think it would be good to just keep it like this now, and fix this in a future refactor (the way post commands are handled is expected to change anyway, once all datatypes are converted to the "new style").

Hm, I'm not so sure. This sneakily hidden state is not ideal, exposing this state globally through `$O_O` does not seem correct either: This is really an implementation detail of `executePostedCommand`. I think it would be good to just keep it like this now, and fix this in a future refactor (the way post commands are handled is expected to change anyway, once all datatypes are converted to the "new style").

matthijskooijman (Migrated from github.com) reviewed

2020-06-25 13:42:16 +00:00

system/core/HyphaRequest.php

I like extra parenthesis to emphasize what is being returned, though thinking about it, I can't really put down in words why it would actually help :-) I guess it's a habit that's more relevant when assigning an expression to a variable, not really needed for return. Triple === is a good idea.

I like extra parenthesis to emphasize what is being returned, though thinking about it, I can't really put down in words why it would actually help :-) I guess it's a habit that's more relevant when assigning an expression to a variable, not really needed for return. Triple === is a good idea.

matthijskooijman (Migrated from github.com) reviewed

2020-06-25 13:49:42 +00:00

system/core/RequestContext.php

Good point, fixed.

Good point, fixed.

matthijskooijman (Migrated from github.com) reviewed

2020-06-25 13:49:48 +00:00

system/core/RequestContext.php

Fixed.

Fixed.

matthijskooijman (Migrated from github.com) reviewed

2020-06-25 13:49:54 +00:00

system/core/HyphaRequest.php

Fixed.

Fixed.

matthijskooijman commented

2020-06-25 13:50:55 +00:00

(Migrated from github.com)

Thanks for the review. I've fixed most of your comments. I also noticed a missing global $O_O; that broke login (it got left behind splitting off commits into this new PR), that's also fixed now.

Thanks for the review. I've fixed most of your comments. I also noticed a missing `global $O_O;` that broke login (it got left behind splitting off commits into this new PR), that's also fixed now.

RoukePouw (Migrated from github.com) reviewed

2020-06-25 13:52:57 +00:00

system/core/events.php

Cool, just wanted to double check :-)

Cool, just wanted to double check :-)

matthijskooijman commented

2020-07-14 09:25:02 +00:00

(Migrated from github.com)

I did a bit more testing, and will merge this now.

I did a bit more testing, and will merge this now.

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha!331

No description provided.