ISS 983 Allow for side redirects with new configuration

[mchineboy]

Allows for a new configuration variable to control which subdomains are allowed to be redirected to.

Resolves Flask-Middleware/flask-security#983

[jwag956]

So it seems like this is slightly too complex - and would need a bunch more documentation to explain that REDIRECT_MATCH_SUBDOMAINS supersedes REDIRECT_ALLOW_SUBDOMAINS.

Could we simplify this and keep it backwards compatible by defaulting REDIRECT_MATCH_SUBDOMAINS to SERVER_NAME - then we don't need the fallback code in utils.py ....

[mchineboy]

Could we simplify this and keep it backwards compatible by defaulting REDIRECT_MATCH_SUBDOMAINS to SERVER_NAME - then we don't need the fallback code in utils.py ....

This was done intentionally because I feel that configuring this setting indicates that you want more explicit control over what you can redirect to.

Could you add a comment for this block describing the behavior?

Done!

[mchineboy]

Should be the last commit for this. I was missing a return False if REDIRECT_ALLOW_SUBDOMAINS was false. Passes all tests locally.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.44%. Comparing base (4e1a66e) to head (d44ba2b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #984   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files          36       36           
  Lines        4624     4639   +15     
=======================================
+ Hits         4552     4567   +15     
  Misses         72       72           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jwag956]

A few small things - but one large one - I think you (mistakenly?) implemented allowing redirects to any domain - not just subdomains.

[jwag956]

This is getting close! and much simpler. However - I still think this is enabling redirects to arbitrary hosts - not just subdomains. I don't think your use case needs that and would be a bigger/more risky change. To that end REDIRECT_MATCH_SUBDOMAINS should really be a list of subdomains - as in ["auth", "app", "mktg.app"] and this check should be something like:

if any(f"{n}.{base_domain}" == url_next.netloc for n in allowed_subdomains):

Of course requiring base_domain (e.g. SERVER_NAME) to be set and should always allow url_next.netloc == base_domain right?

Does that make sense?

[mchineboy]

Hopefully the latest change resolves this, and my explanation further down explains the desired functionality.

[mchineboy]

In our scenario, the Flask application is served under the domain application.example.com. However, we need the flexibility to redirect to another subdomain, specifically service.example.com. The standard SERVER_NAME configuration in Flask restricts redirect behavior to the same subdomain. We believe that it should be permissible to redirect to any subdomain within the example.com root domain as it falls under the same administrative jurisdiction.

To accommodate this, we propose a modification that expands on the current redirect functionality. This update adheres to a secure redirect pattern while allowing for more flexibility. As developers or administrators of the application, we consider it necessary to have the discretion to delegate redirect targets beyond the constraints imposed by SERVER_NAME. This change would grant us that ability, facilitating intra-domain redirects as required by our application's logic.

The onus is on the administrator to not do something wrong in this scenario. I do see a possibility where we determine the top level domain (I.E. example.com) and allow redirects within that domain, but blocking the user from doing something that makes no since, such as redirecting to google.com.

I should note that our SESSION_COOKIE_DOMAIN is set to .example.com for this reason. So applications in the same namespace can share the session cookie.

[jwag956]

I am traveling so responses will be slower
I understand and agree with your use case- just need to review docs around SERVER_HOST in flask

[mchineboy]

I am traveling so responses will be slower I understand and agree with your use case- just need to review docs around SERVER_HOST in flask

No hurry, we can revisit next week.

[jwag956]

I have been reviewing Flask's SERVER_NAME - I had assumed you had that set but I now assume not. The original subdomain feature - https://github.com/Flask-Middleware/flask-security/pull/367 - revolved around that.

It appears your use case is slightly different than that - you want to redirect to a different service, not served by the same Flask application - so SERVER_NAME doesn't really help. Given that here is my proposal - I am still trying to get things simpler. (Note - while I appreciate your trying to get the TLD - it turns out that is actually very difficult (https://stackoverflow.com/questions/15460777/how-to-get-the-domainname-nametld-from-a-url-in-python)

First - leave the current feature (REDIRECT_ALLOW_SUBDOMAINS) alone and as it is.

Then - lets add 2 new configs for this feature - REDIRECT_MATCH_SUBDOMAINS - which is a list of subdomains (auth, api2.api) - with or without the leading '.'. and REDIRECT_BASE_DOMAIN - which would be 'example.com' or 'work.uk.gov').
If REDIRECT_BASE_DOMAIN is set - then each element in REDIRECT_MATCH_SUBDOMAINS is prepended to the REDIRECT_BASE_DOMAIN and matched against the request redirect.
So - 2 different features with different configs - should be very simple and give you the flexibility you need.

[mchineboy]

I have been reviewing Flask's SERVER_NAME - I had assumed you had that set but I now assume not. The original subdomain feature - #367 - revolved around that.

It appears your use case is slightly different than that - you want to redirect to a different service, not served by the same Flask application - so SERVER_NAME doesn't really help. Given that here is my proposal - I am still trying to get things simpler. (Note - while I appreciate your trying to get the TLD - it turns out that is actually very difficult (https://stackoverflow.com/questions/15460777/how-to-get-the-domainname-nametld-from-a-url-in-python)

First - leave the current feature (REDIRECT_ALLOW_SUBDOMAINS) alone and as it is.

Then - lets add 2 new configs for this feature - REDIRECT_MATCH_SUBDOMAINS - which is a list of subdomains (auth, api2.api) - with or without the leading '.'. and REDIRECT_BASE_DOMAIN - which would be 'example.com' or 'work.uk.gov'). If REDIRECT_BASE_DOMAIN is set - then each element in REDIRECT_MATCH_SUBDOMAINS is prepended to the REDIRECT_BASE_DOMAIN and matched against the request redirect. So - 2 different features with different configs - should be very simple and give you the flexibility you need.

Should I submit a patch to implement this logic? Or is this something that you're taking on? I'm confused by the language a little.

[jwag956]

I am asking that if my proposal sounds like it satisfies your use case - change your PR to implement it. I am also happy to do it - really simple. If you want me to do this - just close the PR and I'll put one up in the next few days. Thanks for all your work either way.

[jwag956]

Hi - lets move this forward - I know some folks LIKE to contribute to open source - that's great! Others just really want/need a feature for their (more important) app to function. I do not want to discourage you from contributing. If you are in the second group - 'just need/want the feature' I am happy to implement it as I described. Just let me know how you want to handle this. Thanks!

[jwag956]

Should be available in #1011