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

Add csrf_tag helper #201

Merged
merged 3 commits into from Feb 8, 2022
Merged

Add csrf_tag helper #201

merged 3 commits into from Feb 8, 2022

Conversation

yawaramin
Copy link
Contributor

This lets us easily inject a hidden field containing a CSRF token into
a form, while manually controlling all attributes of the form, e.g.
name, id, etc.

Fix #199

This lets us easily inject a hidden field containing a CSRF token into
a form, while manually controlling all attributes of the form, e.g.
`name`, `id`, etc.

Fix aantron#199
@yawaramin
Copy link
Contributor Author

Note: apologies, but I was unable to get Dream to build due to some errors:

$ dune build
File "src/vendor/gluten/lwt-unix/tls_io.real.ml", line 84, characters 64-73:
Error: This expression has type
         ?ip:'a -> host:'b -> 'c -> ('d option, 'e) result
       but an expression was expected of type
         X509.Authenticator.t =
           host:[ `host ] Domain_name.t option ->
           X509.Certificate.t list -> X509.Validation.r
File "src/vendor/paf/lib/lE.mli", line 12, characters 44-61:
12 | module Make (Time : Mirage_time.S) (Stack : Mirage_stack.V4V6) : sig
                                                 ^^^^^^^^^^^^^^^^^
Error (alert deprecated): Mirage_stack.V4V6
Please use 'Tcpip.Stack.V4V6' directly (and depend on tcpip >= 7.0.0)
File "src/vendor/paf/lib/paf_mirage.mli", line 79, characters 44-61:
79 | module Make (Time : Mirage_time.S) (Stack : Mirage_stack.V4V6) :
                                                 ^^^^^^^^^^^^^^^^^
Error (alert deprecated): Mirage_stack.V4V6
Please use 'Tcpip.Stack.V4V6' directly (and depend on tcpip >= 7.0.0)
Error: Program refmt not found in the tree or in PATH
 (context: default)
Hint: opam install reason
File "example/w-one-binary/dune", line 10, characters 7-19:
10 |   (run ocaml-crunch -m plain assets -o %{target}))))
            ^^^^^^^^^^^^
Error: Program ocaml-crunch not found in the tree or in PATH
 (context: default)
File "src/mirage/dune", line 13, characters 2-14:
13 |   dream.httpaf
       ^^^^^^^^^^^^
Error: Library "dream.httpaf" not found.
Hint: try:
  dune external-lib-deps --missing @@default

@aantron
Copy link
Owner

aantron commented Feb 8, 2022

Try dune build -p dream-pure,dream-httpaf,dream. But I strongly recommend make. The Makefile has the right commands for building in a monorepo where some packages may be more experimental than others (for now).

@yawaramin
Copy link
Contributor Author

OK, using make narrows it down to:

File "src/vendor/gluten/lwt-unix/tls_io.real.ml", line 84, characters 64-73:
Error: This expression has type
         ?ip:'a -> host:'b -> 'c -> ('d option, 'e) result
       but an expression was expected of type
         X509.Authenticator.t =
           host:[ `host ] Domain_name.t option ->
           X509.Certificate.t list -> X509.Validation.r
make: *** [build] Error 1

Strange...

@aantron
Copy link
Owner

aantron commented Feb 8, 2022

Do you have tls installed in the switch? If so, try removing it. This looks like Gluten hasn't kept up with changes in tls (and Dream is not using them with each other, for now).

@aantron
Copy link
Owner

aantron commented Feb 8, 2022

Probably would be fixed upstream by anmonteiro/gluten#24.

@aantron
Copy link
Owner

aantron commented Feb 8, 2022

I'll pull that commit into Dream's vendored Gluten after this PR, so that people don't have to remove tls.

@yawaramin
Copy link
Contributor Author

Wow, that did the trick! Successfully built after removing tls and tls-mirage, thank you. Antonio would probably say this would also be fixed by using Nix... 😅

Copy link
Owner

@aantron aantron left a comment

Choose a reason for hiding this comment

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

Looks good except for one question!

@@ -16,7 +16,7 @@ let show_form ?message request =
<p>You entered: <b><%s message %>!</b></p>
% end;

<%s! Dream.form_tag ~action:"/" request %>
<form action="/" method="post"><%s! Dream.csrf_tag request %>
Copy link
Owner

Choose a reason for hiding this comment

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

Why not put the CSRF field on its own line? It seems like it would be clearer to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it was mostly that the Django code example does it that way. But I can change it pretty easily.

@aantron aantron merged commit acc2a16 into aantron:master Feb 8, 2022
@aantron
Copy link
Owner

aantron commented Feb 8, 2022

Thank you!

@yawaramin yawaramin deleted the csrf-tag branch February 8, 2022 14:16
@aantron
Copy link
Owner

aantron commented Feb 11, 2022

@yawaramin, looking at the docs more closely now, I see

It is recommended to put the CSRF tag immediately after the starting

tag, to prevent certain kinds of DOM manipulation-based attacks.

Do you have a good reference for that? Asking because the Dream docs are full of references to security articles :)

This was referenced Feb 11, 2022
@yawaramin
Copy link
Contributor Author

@aantron sure, just found it: https://portswigger.net/web-security/csrf/tokens

The relevant paragraph is:

For additional safety, the field containing the CSRF token should be placed as early as possible within the HTML document, ideally before any non-hidden input fields and before any locations where user-controllable data is embedded within the HTML. This mitigates against various techniques in which an attacker can use crafted data to manipulate the HTML document and capture parts of its contents.

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.

Add form_tag name attribute?
2 participants