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

Feature: introduce CSRF protection #986

Merged
merged 1 commit into from Jan 17, 2020
Merged

Conversation

ec
Copy link
Contributor

@ec ec commented Dec 18, 2019

Simple and effective CSRF protection overview and protection principles https://www.linkedin.com/pulse/how-protect-your-app-from-csrf-artem-linetskyi by @alinetskyi

@ec ec force-pushed the feature/csrf_protec branch 2 times, most recently from e29b72c to 7618cf2 Compare December 18, 2019 11:52
lib/barong/authorize.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@ysv ysv left a comment

Choose a reason for hiding this comment

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

  1. I advice we take a look into default ActionController implementation of CSRF ActionController::RequestForgeryProtection and either we use it or follow the same principle
  2. https://medium.com/rubyinside/a-deep-dive-into-csrf-protection-in-rails-19fa0a42c0ef

lib/barong/authorize.rb Outdated Show resolved Hide resolved
@ec ec self-assigned this Jan 6, 2020
@ec ec requested review from ysv and alinetskyi January 6, 2020 14:29
@ec ec requested a review from mod January 6, 2020 14:30
@ec ec added the need qa label Jan 6, 2020
Copy link
Contributor

@mod mod 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
Just need rebase and some documentation of the param.

lib/barong/authorize.rb Outdated Show resolved Hide resolved
@ec ec requested a review from mod January 10, 2020 09:18
@ec ec removed the need qa label Jan 10, 2020
app/api/v2/identity/users.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@mod mod left a comment

Choose a reason for hiding this comment

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

I would be in facvor of having a test file for CSRF, and disable it for the other test files

@mod mod merged commit 3c0856b into openware:master Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants