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

In addition to CSRF token, verify the origin too #2501

Merged
merged 1 commit into from Apr 16, 2016

Conversation

Projects
None yet
3 participants
@dstufft
Copy link
Contributor

dstufft commented Apr 16, 2016

Add an additional layer of protection against CSRF by verifying the actual origin of the request in addition to the CSRF token. We only do this check on sites hosted behind HTTPS because only HTTPS sites have evidence to show that the Referrer header is not being spuriously removed by random middleware boxes.

Note, to prevent any sort of backwards incompatibilities and since the CSRF predicate has been deprecated, I've only added this to the new view derivation form of CSRF.

I still need to write documentation and tests (working on that now), just thought I'd throw this up here incase people were interested. This is same technique can be seen in Django.

@dstufft dstufft force-pushed the dstufft:check-origin-csrf branch from 70f48e4 to 815f001 Apr 16, 2016

Note that this function will do nothing if request.scheme is not https.
..versionadded:: 1.7

This comment has been minimized.

@bertjwregeer

bertjwregeer Apr 16, 2016

Member

IIRC you need one more space after the initial two periods.

@bertjwregeer bertjwregeer added this to the 1.7 milestone Apr 16, 2016

@dstufft dstufft force-pushed the dstufft:check-origin-csrf branch from 815f001 to 67f4a0f Apr 16, 2016

@dstufft dstufft changed the title [WIP] In addition to CSRF token, verify the origin too In addition to CSRF token, verify the origin too Apr 16, 2016

)

from pyramid.exceptions import BadCSRFToken
from pyramid.exceptions import BadCSRFOrigin, BadCSRFToken

This comment has been minimized.

@bertjwregeer

bertjwregeer Apr 16, 2016

Member

I would prefer these on multiple lines, with tuple style importing, listed in alphabetical order.

@@ -6,7 +6,7 @@
)

from pyramid.security import NO_PERMISSION_REQUIRED
from pyramid.session import check_csrf_token
from pyramid.session import check_csrf_origin, check_csrf_token

This comment has been minimized.

@bertjwregeer

bertjwregeer Apr 16, 2016

Member

Same here, imports across multiple lines, alphabetical order. Most of the Pyramid source code has that for multiple imports.

@dstufft dstufft force-pushed the dstufft:check-origin-csrf branch 2 times, most recently from a5e56b8 to 6eae953 Apr 16, 2016

@dstufft

This comment has been minimized.

Copy link
Contributor Author

dstufft commented Apr 16, 2016

Ok, unless code review finds something that needs addressed I think this should be ready to merge!

@bertjwregeer

This comment has been minimized.

Copy link
Member

bertjwregeer commented Apr 16, 2016

👍

@mmerickel I don't believe that adding this will break anything for anyone, and I've compared it against the Django version and it is similar (and has the same effect).

Would like your sign-off too before merging this :-).

@dstufft dstufft force-pushed the dstufft:check-origin-csrf branch from 6eae953 to a0992c6 Apr 16, 2016

@@ -101,6 +108,77 @@ def signed_deserialize(serialized, secret, hmac=hmac):

return pickle.loads(pickled)


def check_csrf_origin(request):

This comment has been minimized.

@mmerickel

mmerickel Apr 16, 2016

Member

I would like to see this function more reusable in a similar vein to check_csrf_token. For example a default signature like check_csrf_origin(request, trusted_origins=None, raises=True) where trusted_origins overrides the usage of the setting.


# Determine which origins we trust, which by default will include the
# current origin.
trusted_origins = request.registry.settings.get(

This comment has been minimized.

@mmerickel

mmerickel Apr 16, 2016

Member

Where is this being cast from an ini string syntax to a list? I can't find it if it's being done right now. This probably needs to be wrapped in pyramid.settings.aslist.

@mmerickel

This comment has been minimized.

Copy link
Member

mmerickel commented Apr 16, 2016

Not very happy about all the whitespace changes here. It made this significantly more tedious to review and github's little ?w=1 doesn't appear to be working. This type of change belongs in a separate PR.

@mmerickel

This comment has been minimized.

Copy link
Member

mmerickel commented Apr 16, 2016

Found a couple issues in comments but otherwise the feature looks good.

@dstufft dstufft force-pushed the dstufft:check-origin-csrf branch from a0992c6 to d5e8b50 Apr 16, 2016

In addition to CSRF token, verify the origin too
Add an additional layer of protection against CSRF by verifying the actual
origin of the request in addition to the CSRF token. We only do this check on
sites hosted behind HTTPS because only HTTPS sites have evidence to show that
the Referrer header is not being spuriously removed by random middleware
boxes.

@dstufft dstufft force-pushed the dstufft:check-origin-csrf branch from d5e8b50 to 65dee6e Apr 16, 2016

@dstufft

This comment has been minimized.

Copy link
Contributor Author

dstufft commented Apr 16, 2016

Ok. I adjusted this to take into account the feedback and the tests are passing again.

@mmerickel mmerickel merged commit 4a4d4b9 into Pylons:master Apr 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mmerickel

This comment has been minimized.

Copy link
Member

mmerickel commented Apr 16, 2016

Nice, thank you!

mmerickel added a commit that referenced this pull request Apr 16, 2016

@dstufft dstufft deleted the dstufft:check-origin-csrf branch Apr 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.