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

Csrf tokens #65

Merged
merged 23 commits into from
Oct 17, 2024
Merged

Csrf tokens #65

merged 23 commits into from
Oct 17, 2024

Conversation

PizieDust
Copy link
Collaborator

closes #40
This PR adds functionality for CSRF token.

When a user is authenticated, we use a Double Synchronizer Pattern which keeps the token on the server and transmit a copy as hidden field to the form.

When a user is not authenticated, we use a Naive Double Submit Cookie Pattern which sends the token in a cookie and also a hidden field on the form.

We also add a custom header X-MOLLY-CSRF. Will experiment with this to see if it's better for unauthenticated users than cookies.

@PizieDust PizieDust added the enhancement New feature or request label Oct 11, 2024
@PizieDust PizieDust requested a review from hannesm October 11, 2024 11:48
@PizieDust PizieDust self-assigned this Oct 11, 2024
@PizieDust PizieDust marked this pull request as draft October 11, 2024 11:50
@PizieDust PizieDust changed the title Csrf token implementation Csrf tokens Oct 11, 2024
@PizieDust PizieDust marked this pull request as ready for review October 15, 2024 07:44
@hannesm
Copy link
Contributor

hannesm commented Oct 15, 2024

could you rebase on top of the main branch, so the CI failure should go? :)

middleware.ml Outdated Show resolved Hide resolved
middleware.ml Show resolved Hide resolved
middleware.ml Show resolved Hide resolved
middleware.ml Outdated Show resolved Hide resolved
middleware.ml Outdated Show resolved Hide resolved
middleware.ml Outdated Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
unikernel.ml Show resolved Hide resolved
unikernel.ml Show resolved Hide resolved
unikernel.ml Show resolved Hide resolved
unikernel.ml Outdated Show resolved Hide resolved
@hannesm
Copy link
Contributor

hannesm commented Oct 15, 2024

Lots of nice additions and refactorings.

I'm still wondering (and that may be because your initial PR comment needs to be updated):

  • when is the HTTP header used?
  • when is the cookie used?

I guess we need to tackle some other issues (n upcoming refactorings):

  • labels and when to use them for function calls
  • local bindings vs match ... with
  • guard yojson functions in a separate module (for safety reasons, whenever we handle user input, expect the exceptional cases)
  • String escaping / cleaning / trimming -- also a potential security issue
  • where to store data -- esp. should these csrf tokens really be stored on disk?
  • this raises the other question about "which data structure to use" (for e.g. the users)

@hannesm
Copy link
Contributor

hannesm commented Oct 16, 2024

(rebased and force-pushed on main)

@hannesm
Copy link
Contributor

hannesm commented Oct 16, 2024

IMHO ready to go now

@PizieDust
Copy link
Collaborator Author

Thank you @hannesm.
I'll merge now.

@PizieDust PizieDust merged commit ff34d39 into main Oct 17, 2024
2 checks passed
@PizieDust PizieDust deleted the csrf branch October 17, 2024 05:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

CSRF tokens
2 participants