Support for `host_matching`

samuelhwilliams commented 5 months ago

It doesn't seem like Flask-DebugToolbar supports running flask in host_matching mode. When Flask is configured this way, the URLs generated for Flask-DebugToolbar's assets are invalid, so none of the JS/CSS loads.

Here's a minimal app for reproducing:

from flask import Flask
from flask_debugtoolbar import DebugToolbarExtension

app = Flask(
    __name__,
    host_matching=True,
    static_host='sub1.app.localhost:5000',
)
app.config['DEBUG_TB_ENABLED'] = True
app.config['SECRET_KEY'] = 'abc123'

toolbar = DebugToolbarExtension(app)

@app.route("/", host='sub1.app.localhost:5000')
def sub1_index():
    return f"<html><head></head><body>OK</body></html>"

Which results in this truncated HTML being injected. Note the http:///, which is invalid.

<div id="flDebug" style="display:none;" role="navigation">
  <script type="text/javascript">var DEBUG_TOOLBAR_STATIC_PATH = 'http:///_debug_toolbar/static/'</script>
  <script type="text/javascript" src="http:///_debug_toolbar/static/js/jquery.js"></script>
  <!-- Temporarily adding jquery-migrate during the Jquery upgrade process, this can be removed post-upgrade -->
  <script src="http:///_debug_toolbar/static/js/jquery-migrate.js"></script>
  <script type="text/javascript" src="http:///_debug_toolbar/static/js/jquery.tablesorter.js"></script>
  <script type="text/javascript" src="http:///_debug_toolbar/static/js/toolbar.js"></script>
  ...

I've put together a changeset that I think would address this issue - happy to hear thoughts before raising a PR: https://github.com/pallets-eco/flask-debugtoolbar/compare/main...samuelhwilliams:flask-debugtoolbar:add-host-support?expand=1

macnewbold commented 5 months ago

Thanks for submitting this! My initial thought is yes, it would be very desirable to support host_matching configurations. If the URL generation is the main hurdle, it doesn't sound like that should be so hard, and indeed I think you've demonstrated that.

In a quick look at your PR draft, here are my thoughts, for what they're worth:

Is there a way we could look at the Flask configuration to get the needed info about the host matching, rather than forcing the user to add additional configuration to the debug toolbar? For example, could debug toolbar use the flask app static_host value if there is one?
How critical is it to add new dependencies for pytest-mock and beautifulsoup (and remove the greenlet dependency?) in order to support this change? My general inclination is to be very cautious and conservative in adding new dependencies if they can reasonably be avoided.
I like the validation and configuration method you added.
The new parameter for send_static_file() puzzles me - it seems not to be used in the method at all?
Other than the new dependencies, your unit tests seem good from what I can tell so far, and thorough.

Again, thanks for identifying this and suggesting a solution. I think the PR would be good to pursue.

samuelhwilliams commented 5 months ago

Thanks for taking a look and providing that feedback/asking some questions.

Is there a way we could look at the Flask configuration to get the needed info about the host matching, rather than forcing the user to add additional configuration to the debug toolbar? For example, could debug toolbar use the flask app static_host value if there is one?

That's a good question - maybe reading from static_host is a good option; I hadn't considered it. I would be happy to take a steer as to whether that is a sensible decision and tie-in with Flask config, as I'm not sure myself.

~If we think yes, I'm happy to look at making that change.~ I'll take a look at doing it anyway, and then we've got the option.

How critical is it to add new dependencies for pytest-mock and beautifulsoup (and remove the greenlet dependency?) in order to support this change? My general inclination is to be very cautious and conservative in adding new dependencies if they can reasonably be avoided.

On the greenlet dependency, I think that's just dropped out because it's not included on darwin platforms and if I'd frozen requirements on linux then it would remain. It should remain and if I put up a PR I'll make sure it's not removed.

For pytest-mock, I only added it in order to get access to MockerFixture to make mypy happy.

For beautifulsoup4, it's not required, but I do find that it can help provide stronger test assertions and make some assertions easier to read at a glance. I've only used it in one test, so we could definitely remove it and do a simple text-based check. But I think having something like beautifulsoup around in test conditions could make it easier to develop the test suite in the future, especially as the existing tests seem to do very little in terms of validating what actually gets rendered out. Definitely removeable for now though; not trying to push anything contentious.

All of these are only in the test dependencies, which I presumed provides some more flexibility in adding/removing them, but maybe still better to be conservative.

The new parameter for send_static_file() puzzles me - it seems not to be used in the method at all?

This does look weird, I agree, and I probably should've added some explanation for it. Because the patch I've made starts registering the send_static_file endpoint on a wildcard host, it's similar to having a path variable in the werkzeug url rule. Any values collected as variables get passed through to the python function. It is effectively not used by the body of send_static_file; it's just added because Flask/Werkzeug is providing a value for that parameter because of how the routing is set up. It may be possible to pop it out with a url_value_preprocessor so that the view doesn't receive it as an argument. Happy to update the patch with this if we want (and it probably does feel cleaner)

samuelhwilliams commented 5 months ago

So, I think if we scrape the host definition from static_host, we open up some extra complexity. First of all, Flask doesn't seem to store a nice reference to the static_host that's passed in - it gets used directly to register the static endpoint/URL rule and then discarded. So I think we'd have to scan Flask's url_map, find the static rule, and grab the host from there. I don't know if we'd be happy doing that.

Putting that aside, though, using the app's static_host works well in the simple case, and adds a trickier issue than my branch has in the variable-host case.

Example 1 - non-variable static host

from flask import Flask
from flask_debugtoolbar import DebugToolbarExtension

app = Flask(
    __name__,
    host_matching=True,
    static_host='sub1.app.localhost:5000',
)
app.config['DEBUG_TB_ENABLED'] = True
app.config['SECRET_KEY'] = 'abc123'

toolbar = DebugToolbarExtension(app)

This would work out of the box ✅

Example 2 - static host has some variable element

from flask import Flask
from flask_debugtoolbar import DebugToolbarExtension

app = Flask(
    __name__,
    host_matching=True,
    static_host='<subdomain>.app.localhost:5000',
)
app.config['DEBUG_TB_ENABLED'] = True
app.config['SECRET_KEY'] = 'abc123'

toolbar = DebugToolbarExtension(app)

The small issue

In this case, Flask-DebugToolbar's send_static_file view would (by default) be passed a subdomain argument (much like the branch I put up receives a toolbar_routes_host argument).

Some ways to prevent this would be either:

Flask-DebugToolbar's send_static_file signature has a catch-all **kwargs.
We tell users of the extension that they need to add a url_values_preprocessor removing the matching argument from the _debug_toolbar.static endpoint. This feels more cumbersome than having them add some extra config for the extension.

The bigger issue?

In order for us to use url_for on Flask-DebugToolbar's static endpoint, we need to pass in any variables that are in the static host. I don't think we have any way of knowing how these should be populated as I think it would be defined by the user/application when they call url_for('static', subdomain=...). Again, maybe we'd have to ask them to register some app.url_values function that will inject the values for our url_for to use - but it feels gnarlier than us requiring some config.

This is basically why I added a check against the DebugToolbar host having any variables in it - and if we scrape Flask's static host we can't as easily defend against that case.

I may be missing something, but I feel like the idea of unknown variables in Flask's static host probably make that option not a goer, but I'm happy to be corrected.

samuelhwilliams commented 4 months ago

@macnewbold I've put up a new branch at https://github.com/pallets-eco/flask-debugtoolbar/compare/main...samuelhwilliams:flask-debugtoolbar:add-host-support-v2 that I think I'm nearly ready to put up as a PR. It incorporates a couple of changes:

Removes the additional test dependency for beautifulsoup4 and just does assertions on response text.
Adds a url_values_preprocessor so that we don't have to add toolbar_routes_host to the send_static_files method signature.

I have one outstanding question/thought. If Flask is initialised in host_matching mode and Flask-DebugToolbar isn't, it feels like we know that is broken, so maybe FDT should raise an error to make it immediately obvious to the user that they need to manually set DEBUG_TB_ROUTES_HOST (as per above I think it's not desirable/feasible for us to try to derive it automatically). This feels a more aggressive change, but I think it makes sense. Do you have a steer on this at all? It feels much more like a breaking change, but also probably the right thing to do to make it easier for user's to do the right thing/be less confused.

Also happy to just raise that branch as a PR and move discussion over there - not sure what the preference is.

macnewbold commented 4 months ago

Thanks for the great analysis, and the considerations of the tradeoffs involved. I think your reasoning makes sense.

On the last question, if there's a configuration where we're already broken, and know that it will be broken, I think a message warning about that makes a lot of sense, especially since we can tell them how to configure FDT in a compatible way and resolve the problem.

I don't think I have any additional feedback on the PR draft, and I'd love to see if anyone else has any thoughts to add to the conversation.

samuelhwilliams commented 4 months ago

Thanks - I've put up a PR :)

pallets-eco / flask-debugtoolbar