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

Fixed csrf delete for generated scaffolding #356

Merged
merged 2 commits into from Nov 2, 2017
Merged

Fixed csrf delete for generated scaffolding #356

merged 2 commits into from Nov 2, 2017

Conversation

elorest
Copy link
Member

@elorest elorest commented Nov 2, 2017

Description of the Change

Fixes #339.

Also uses link helpers instead of <a href...>

Alternate Designs

At some point we might want to use a helper to generate a form that posts or a JS hack that does it onclick.

Benefits

Protects from cross site scripting, so that javascript in other tabs can't post valid requests to domains associated with your cookies.

@faustinoaq
Copy link
Contributor

@elorest Can Amber have something like link "Home", to: HomeController, :index ?

With params link "Home", to: HomeController, :index, with: {id: id}

Instead of using direct links ?

@elorest
Copy link
Member Author

elorest commented Nov 2, 2017

@faustinoaq it should be able to. @eliasjpr was working on url_for a couple of months ago but it sort of ended up going in a different direction. This will also fix redirect. That information is all available in the router so it wouldn't be that hard to do if you wanted to take a stab at it. It should be in amber core instead of jasper_helpers though since it would be needed by both jasper and amber redirect.

If you wanted to pair on it I'd be happy to jump on as well as I've already played around with the concept.

@elorest
Copy link
Member Author

elorest commented Nov 2, 2017

link "Home", to: HomeController, :index, with: {id: id} syntactically won't work since it has a named params to: and then a none named param for :index

@elorest
Copy link
Member Author

elorest commented Nov 2, 2017

We can improve that later though. This just fixes a bug and should be merged asap in my opinion.

@elorest
Copy link
Member Author

elorest commented Nov 2, 2017

Whether is should override GET with DELETE or use POST is a conversation we can have another day. However this is not a security concern since CSRF isn't concerned with man in the middle attacks or copy and pasting links. Please see conversation on issue #339 or read up on CSRF mitigation.
https://www.owasp.org/index.php/Cross-Site_Request_Forgery_(CSRF)

@marksiemers
Copy link
Contributor

@elorest - I agree this should get merged in ASAP. If this gets refactored / more work is done with CSRF and DELETE, we should write some tests around it. AFAIK, this was happening in production and the test suite didn't catch it.

Is that waiting on the ability to do capybara type testing?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants