Skip to content

feat: implement threshold for resetting password #6035

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 19, 2019
Merged

feat: implement threshold for resetting password #6035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8171102
feat: Implement flask limiter on password-reset route
shreyanshdwivedi Jun 12, 2019
64f0593
adds limiter to limit the request from same IP
shreyanshdwivedi Jun 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from flask_script import Manager
from flask_login import current_user
from flask_jwt import JWT
from flask_limiter import Limiter
from datetime import timedelta
from flask_cors import CORS
from flask_rest_jsonapi.errors import jsonapi_errors
Expand Down Expand Up @@ -50,6 +51,7 @@
static_dir = os.path.dirname(os.path.dirname(__file__)) + "/static"
template_dir = os.path.dirname(__file__) + "/templates"
app = Flask(__name__, static_folder=static_dir, template_folder=template_dir)
limiter = Limiter(app)
env.read_envfile()


Expand Down
9 changes: 8 additions & 1 deletion app/api/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
from functools import wraps
from flask import request, jsonify, make_response, Blueprint, send_file, url_for, redirect
from flask_jwt import current_identity as current_user, jwt_required
from flask_limiter.util import get_remote_address
from sqlalchemy.orm.exc import NoResultFound
from app.api.helpers.order import create_pdf_tickets_for_holder
from app.api.helpers.storage import generate_hash

from app import get_settings
from app import limiter
from app.api.helpers.db import save_to_db, get_count
from app.api.helpers.errors import ForbiddenError, UnprocessableEntityError, NotFoundError, BadRequestError
from app.api.helpers.files import make_frontend_url
Expand All @@ -29,7 +31,6 @@
from app.models.user import User
from app.api.helpers.storage import UPLOAD_PATHS


authorised_blueprint = Blueprint('authorised_blueprint', __name__, url_prefix='/')
ticket_blueprint = Blueprint('ticket_blueprint', __name__, url_prefix='/v1')
auth_routes = Blueprint('auth', __name__, url_prefix='/v1/auth')
Expand Down Expand Up @@ -207,6 +208,12 @@ def resend_verification_email():


@auth_routes.route('/reset-password', methods=['POST'])
@limiter.limit(
'3/hour', key_func=lambda: request.json['data']['email'], error_message='Limit for this action exceeded'
Copy link
Member

@iamareebjamal iamareebjamal Jun 18, 2019

Choose a reason for hiding this comment

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

jamal.areeb@gmail.com
jamalareeb@gmail.com
ja.mal.are.eb@gmail.com

Are all same email addresses and will be allowed by this method

Copy link
Member Author

@shreyanshdwivedi shreyanshdwivedi Jun 18, 2019

Choose a reason for hiding this comment

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

@iamareebjamal I just tested the above three email addresses.
Each one is distinguished perfectly and failed when the email if entered fourth time. I jumbled the order and tested, the reset request for one email is not affecting other in any way

Copy link
Member

@iamareebjamal iamareebjamal Jun 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I jumbled the order and tested, the reset request for one email is not affecting other in any way

It should

Copy link
Member

@iamareebjamal iamareebjamal Jun 18, 2019

Choose a reason for hiding this comment

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

The point is that I can keep adding dots and sending emails at a rate of more than 3/hour

I can loop through a list of emails and send password reset request on them. Essentially brute forcing

Copy link
Member Author

@shreyanshdwivedi shreyanshdwivedi Jun 19, 2019

Choose a reason for hiding this comment

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

But there is a check which raises error if user is not registered and so no mail is sent out. If a user loops by adding dots, it doesn't mean the email is registered and so error will be raised. I hope it makes sense to you.

Copy link
Member

@iamareebjamal iamareebjamal Jun 19, 2019

Choose a reason for hiding this comment

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

A hacker doesn't care if the error is raised. He'll just continue looping through emails, and our resources will get exhausted. And users will still get spammed.

Also, raising error if user is not registered is a pretty big security issue, it should be fixed ASAP - #6069

Copy link
Member Author

@shreyanshdwivedi shreyanshdwivedi Jun 19, 2019

Choose a reason for hiding this comment

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

@iamareebjamal from what I understood, I should create a function which treats similar emails as same email. Right?
Also can you please guide me which pattern should I consider ?

Copy link
Member

@iamareebjamal iamareebjamal Jun 19, 2019

Choose a reason for hiding this comment

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

Instead, you should add one more limiter which prevents same IP to access the unprotected resource at a lower rate, like '1/minute'

Copy link
Member Author

@shreyanshdwivedi shreyanshdwivedi Jun 19, 2019

Choose a reason for hiding this comment

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

@iamareebjamal I've updated the PR. Please review

)
@limiter.limit(
'1/minute', key_func=get_remote_address, error_message='Limit for this action exceeded'
)
def reset_password_post():
try:
email = request.json['data']['email']
Expand Down
1 change: 1 addition & 0 deletions requirements/common.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
pycparser==2.14 # Only 2.14 works.
Flask~=1.0.3
Flask-Limiter~=1.0.1
Flask-Script~=2.0.6
Flask-SQLAlchemy~=2.1
Flask-Migrate~=2.5
Expand Down