pallets-eco / flask-security

Quick and simple security for Flask applications
MIT License
635 stars 155 forks source link

BUG token authentication is accepted on endpoints which only allow 'session' as authentication-method #791

Closed N247S closed 1 year ago

N247S commented 1 year ago

I am messing around a bit with configurations to figure out how things work, and what to be aware of when working with flask-security.

One thing I noticed was when one authenticates using a token, they can access endpoints which are protected with auth_required('session') (i.e. session-authentication only), which might not be a big problem, but goes against specs if I read it correctly.

Testcase:

def test_token_only_auth(app, client):
    app.post('/token')
    def token_login_mock():
        if request.is_json:
            d = request.get_json()

            if not d['email']:
                abort(HTTPStatus.BAD_REQUEST)
            user = lookup_identity(d['email'])
            if user and not user.active:
                abort(HTTPStatus.NOT_FOUND)

            if user and user.verify_and_update_password(d['password']):
                token = user.get_auth_token()
                return {'token': token}
            else:
                abort(HTTPStatus.UNAUTHORIZED)

    app.get('/protected')
    @auth_required('session')
    def protected():
        return 'secret'

    response = client.post('/token', data=auth_data, content_type='application/json')
    token = response.json['token']
    response = client.get('/protected', headers={'Authentication-Token': token})
    assert response.status_code == 401 # fails as session is created before auth-mechanism check

From what I could figure out the loginManager.request_loader is called prior to the auth_required. Upon a request it tries to create a session and populates it during which process the current_user proxy is called wich calls _get_user() which in turn calls current_app.login_manager._load_user() to get the user, which in turn calls _load_user_from_request() which is able to obtain the user from a token. This is pushed to the global-variables (g) and is used to populate the current_user.

After this chain of events is done, the auth_required decorator is called which for session-authentication checks current_user.is_authenticated, which is true because that user was found using the token already. This means the decorator basically allows token authentication to be used for session authentication.

Again not sure if this is a big problem, but I think it goes against specs.

The version I tested this on is: Flask 2.2.3 Flask-security-too 5.1.2 (fresh install as of 1 week ago, so I assume latest dependency versions)

If more information is needed, feel free to ask.

jwag956 commented 1 year ago

Thanks for the detailed issue - seems like a bug and I can see a reasonable use case to have some endpoints be session/web only (quite a few sites require web/session access to say retrieve access tokens).

N247S commented 1 year ago

Hadn't thought that far into usecases yet, but seems reasonable.

One quickfix I could think of is by checking the fs_authn_via flag inside the auth_mechanism function for 'session authentication' as I dont think that flag is set when a user is restored from session data. But it feels sketchy at best.

With the growing authentication solutions it might be preferable to fix this with a big rework. But the above could serve as a quick/temp fix.

jwag956 commented 1 year ago

Thanks for your work - I have taken your PR and am working with it - hopefully get something up in the next few days. Right now I am leaning towards the fs_authn_via solution....

N247S commented 1 year ago

Thank you for picking it up. Just to make sure, PR#794 should be a fix for this. Might save you some time.

jwag956 commented 1 year ago

Yup - started with that - a big help.