pallets-eco / flask-security

Quick and simple security for Flask applications
MIT License
1.63k stars 513 forks source link

Infinite redirect cycle with @roles_required, @roles_accepted #806

Open ds283 opened 5 years ago

ds283 commented 5 years ago

I am seeing an infinite redirection cycle from views protected by @roles_required or @roles_accepted when an attempt is made to access them by an unauthorised user. In my application this typically happens when a user has been logged in correctly, but the server-side session data is lost because of expiry in the storage backend.

When this happens, the decorators detect that permission to access the view is not available and pass through to _get_unauthorized_view(). (I am not using security._unauthorized_callback().) In the latest release version 3.0.0 the definition of this function is (I can see that there have been some changes committed since then)

def _get_unauthorized_view():
    view = utils.get_url(utils.config_value('UNAUTHORIZED_VIEW'))
    if view:
        if callable(view):
            view = view()
        else:
            try:
                view = url_for(view)
            except BuildError:
                view = None
        utils.do_flash(*utils.get_message('UNAUTHORIZED'))
        return redirect(view or request.referrer or '/')
    abort(403)

See here.

The call to utils.get_url(utils.config_value('UNAUTHORIZED_VIEW')) sets view equal to the URL matching the view named in SECURITY_UNAUTHORIZED_VIEW. (For me this is / but I don't think its exact value is important.)

Since this value isn't a callable, the next step tries to build another URL from this existing URL:

try:
    view = url_for(view)
except BuildError:
    view = None

Of course, this fails because view is already a URL. That means view gets set equal to None, and the return redirect(view or request.referrer or '/') always redirects to request.referrer. If that is itself a view that is protected by @roles_required or @roles_accepted then we end up in an endless cycle of redirection to request.referrer.

I can see that the redirection cycle itself was fixed in 4555e42, but this just redirects to / when it detects a cycle. It doesn't seem to fix the original problem of failing to redirect to the endpoint specified by SECURITY_UNAUTHORIZED_VIEW in the first place.

If I've understood correctly, then it seems that the fix should just be to remove the inner try ... except block, leaving view as the URL returned from utils.get_url(). Does that seem reasonable?

odinms commented 5 years ago

I am facing another issue caused by this function too.

1.From the FLASK docs:

url_for() usage is like : return redirect(url_for('auth.login'))

this will get you the url for auth.login()

2.if the SECURITY_UNAUTHORIZED_VIEW is set right. e.g.

SECURITY_UNAUTHORIZED_VIEW = 'auth.login'

view = utils.get_url(utils.config_value('UNAUTHORIZED_VIEW')) will set view = "/login" with is the URL of the login page correspond to the 'auth.login' endpoint

def get_url(endpoint_or_url):
    try:
        return url_for(endpoint_or_url)
    except:
        return endpoint_or_url

And since view = "/login" is just a STRING, not a callable(). it will go to

try:
    view = url_for(view)
except BuildError:
    view = None

THEN, everything went wrong here~!!! url_for() can not use "/login" as input to process, since it is already processed by url_for() once!! It will always raise error~!!

So , the code runs to except BuildError: view = None Finally, end up with view = None

So.. the redirect torequest.referrer or '/' forever~!!!!