Task: Add CSRF for Pylons and Flask. #112
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[x] Update anti_csrf.py to work with Flask requests. [x] Update plugin for above [] Set cookie on flask responses Intercept is still called but just updates the html with a token. before_app_request handles validation of token matching. setting the cookie in the response currently only works for pylons responses, or if a pylons response was first given then going to a flask response. Otherwise the response object doesn't exist yet. Also have to set streaming=False in pylons_app.py.
…inst. orig_anti_csrf.py was from https://github.com/qld-gov-au/ckan-ex-qgov/blob/master/ckanext/qgov/common/anti_csrf.py anti_csrf.py was from https://github.com/data-govt-nz/ckanext-security/blob/master/ckanext/security/anti_csrf.py anti_csrf3.py was from https://github.com/data-govt-nz/ckanext-security/blob/master/ckanext/security/middleware.py I restarted my approuch and wanted to track the changes against the original files to help track my changes against for myself and others in the future. anti_csrf.py in my first WIP commit was actually a modified version of the orig_anti_csrf.py above but naming got away from me.
CKAN is currently running both pylons and flask and migrating towards only flask. Current CSRF approuches only address pylons requests. After trying numerous options this seemed like the most reliable and enables easy throw away once pylons is completely gone from CKAN. Basic concept is to run the original CSRF with very minor changes. Then use a modified version to handle Flask. I used 2 repos as my bases: ckanext-security and ckan-ex-qgov. anti_csrf3.py is the modified middleware.py that breaks apart the classes and modified for Flask. I liked most of how this version, ckanext-security, was implemented which is why I used it instead of the ckan-ex-qgov. ckan-ex-qgov I used as the base for pylons as it didn't require middleware or modifying CKAN directly (middleware implementation).
ThrawnCA
suggested changes
Jan 12, 2020
| @@ -0,0 +1,143 @@ | |||
| """Provides a filter to prevent Cross-Site Request Forgery, | |||
There was a problem hiding this comment.
Is this file really necessary when we have orig_anti_csrf.py?
| log = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| CSRF_ERR = 'CSRF authentication failed. Token missing or invalid.' |
|
|
||
| def is_secure(): | ||
| # allow requests which have the x-forwarded-proto of https (inserted by nginx) | ||
| if request.headers.get('X-Forwarded-Proto') == 'https': |
| return True | ||
|
|
||
| # get/head/options/trace are exempt from csrf checks | ||
| return request.method in ('GET', 'HEAD', 'OPTIONS', 'TRACE') |
There was a problem hiding this comment.
TRACE should probably be entirely blocked, not given special treatment. It actually enables some types of CSRF filter bypass, by exposing the token cookie.
| return None | ||
| token = post_tokens[0] | ||
| # drop token from request so it doesn't populate resource extras | ||
| #del request.POST[anti_csrf.TOKEN_FIELD_NAME] |
There was a problem hiding this comment.
Why is this commented out? Does it not apply to Flask requests?
|
|
||
| CSRF_ERR = 'CSRF authentication failed. Token missing or invalid.' | ||
|
|
||
| domain = config.get('ckan.ontario_theme.csrf_domain', '') |
There was a problem hiding this comment.
Referrer checks should probably be optional, but if they're going to be implemented, shouldn't they be derived from ckan.site_url?
|
@boykoc This is implemented in https://github.com/qld-gov-au/ckanext-csrf-filter/tree/develop as a stand-alone extension now. Comments there are welcome. Thanks for developing this, it helped a lot. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CKAN is currently running both pylons and flask and migrating
towards only flask. Current CSRF approaches only address pylons
requests.
After trying numerous options this seemed like the most reliable and enables
easy throw away once pylons is completely gone from CKAN.
Basic concept is to run the original CSRF with very minor changes. Then use a
modified version to handle Flask.
I used 2 repos as my bases: ckanext-security and ckan-ex-qgov.
anti_csrf3.py is the modified middleware.py that breaks apart the classes
and modified for Flask. I liked most of how this version, ckanext-security,
was implemented which is why I used it instead of the ckan-ex-qgov.
ckan-ex-qgov I used as the base for pylons as it didn't
require middleware or modifying CKAN directly (middleware implementation).
File references: