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

feat: add some simple csrf documentation #2733

Merged
merged 7 commits into from Aug 14, 2019
Merged

Conversation

tyrantkhan
Copy link

@tyrantkhan tyrantkhan commented Jul 20, 2019

Could probably use some fleshing out, but probably a good enough base to get feedback for and I can build upon it -- or just have someone else flesh it out a bit more!.

Copy link
Member

@rossabaker rossabaker left a comment

Thank you! This is a great start.

docs/src/main/tut/csrf.md Show resolved Hide resolved
docs/src/main/tut/csrf.md Outdated Show resolved Hide resolved
docs/src/main/tut/csrf.md Outdated Show resolved Hide resolved
docs/src/main/tut/csrf.md Show resolved Hide resolved
@tyrantkhan
Copy link
Author

@tyrantkhan tyrantkhan commented Jul 29, 2019

I posted this in the gitter, but I want to make note of an issue in this example documentation, I feel like this might be a bug:

hey y’all… I’ve been finalizing @rossabaker ’s comments on this PR, and I realized I didn’t even try to incorporate a POST in the documentation
https://github.com/http4s/http4s/blob/d39e933120ca0f20824c596046e29d099e739e1b/docs/src/main/tut/csrf.md
I’m getting a 403 and I feel like the origin check is probably failing, and I can’t seem to see why that would be the case. Unless a bug in the code? I’ve even simplified the origin check to be something simple like request.toString.length >= 0 and it still failed. So maybe another issue exists?
You can use the example in the documentation to produce the 403. Am I missing something?
Haris Khan @tyrantkhan Jul 27 21:38
Additionally applying that same predicate to validate() does seem to indicate it is passing the origin check

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jul 29, 2019

I haven't forgotten this. Just going to take a little extra to dig into the 403 issue and ran out of time on my lunchtime pass. I'll come back to this ASAP.

Also send up the response cookie as a request cookie.
Both are needed for unsafe requests to validate by default.
@tyrantkhan tyrantkhan closed this Aug 11, 2019
@tyrantkhan tyrantkhan reopened this Aug 11, 2019
@tyrantkhan
Copy link
Author

@tyrantkhan tyrantkhan commented Aug 12, 2019

@rossabaker i think this is good to go, unless you have any more feedback.

Copy link
Member

@rossabaker rossabaker left a comment

👍 Looks good. Thanks!

Unsafe requests (like POST) require us to send the CSRF token in the `X-Csrf-Token`
header (this is the default name, but it can be changed), so we are going to get the value
and send it up in our POST. I've also added the response cookie as a RequestCookie, normally
the browser would send this up with our request, but I needed to do it manually for the purpose of this demo.
Copy link
Member

@rossabaker rossabaker Aug 14, 2019

Choose a reason for hiding this comment

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

This is sad, but a cookie jar is coming soon. Let's make sure we revisit this then. /cc @ChristopherDavenport

Choose a reason for hiding this comment

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

Available in its Beta form. https://github.com/ChristopherDavenport/cookiejar

Just trying to figure out what I'm supposed to do if no domain is present in the cookie. What the constraints to either what it should be saved as, or to what servers it applies. Its not clear to me from the RFC.

Copy link
Member

@ChristopherDavenport ChristopherDavenport left a comment

Correct still, but we will address soon.

@rossabaker rossabaker merged commit d1ebd8a into http4s:master Aug 14, 2019
2 checks passed
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.

None yet

3 participants