Skip to content
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.

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

Invalid SERVER_NAME + url_for leads to AttributeError #5392

Closed
kasium opened this issue Jan 24, 2024 · 5 comments
Closed

Invalid SERVER_NAME + url_for leads to AttributeError #5392

kasium opened this issue Jan 24, 2024 · 5 comments

Comments

@kasium
Copy link

kasium commented Jan 24, 2024

In our environment we have some security scanners running which generate artificial HTTP requests. Since they are closed source and I can't generate these calls with other tools, I created the below example which starts at the flask level and assumes that an invalid server passed thru the levels.

from flask import Flask, url_for

app = Flask(__name__)

@app.errorhandler(400)
def do_400(e):
    url_for("foo")
    return "error", 400


res = app(
    {
        "ACTUAL_SERVER_PROTOCOL": "HTTP/1.1",
        "PATH_INFO": "/",
        "QUERY_STRING": "",
        "REQUEST_METHOD": "GET",
        "REQUEST_URI": "/",
        "SCRIPT_NAME": "",
        "SERVER_NAME": "foobar/..",
        "SERVER_PROTOCOL": "HTTP/1.1",
        "wsgi.url_scheme": "https",
    },
    lambda x, y: None,
)

Error

Traceback (most recent call last):
  File "work/venv/lib/python3.12/site-packages/werkzeug/routing/map.py", line 258, in bind
    server_name = server_name.encode("idna").decode("ascii")
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".pyenv/versions/3.12.0/lib/python3.12/encodings/idna.py", line 173, in encode
    raise UnicodeError("label empty or too long")
UnicodeError: label empty or too long
encoding with 'idna' codec failed

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 1484, in full_dispatch_request
    rv = self.dispatch_request()
         ^^^^^^^^^^^^^^^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 1458, in dispatch_request
    self.raise_routing_exception(req)
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 1440, in raise_routing_exception
    raise request.routing_exception  # type: ignore
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/flask/ctx.py", line 316, in __init__
    self.url_adapter = app.create_url_adapter(self.request)
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 1883, in create_url_adapter
    return self.url_map.bind_to_environ(
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/werkzeug/routing/map.py", line 371, in bind_to_environ
    return Map.bind(
           ^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/werkzeug/routing/map.py", line 260, in bind
    raise BadHost() from e
werkzeug.exceptions.BadHost: 400 Bad Request: The browser (or proxy) sent a request that this server could not understand.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 2190, in wsgi_app
    response = self.full_dispatch_request()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 1486, in full_dispatch_request
    rv = self.handle_user_exception(e)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 1341, in handle_user_exception
    return self.handle_http_exception(e)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 1281, in handle_http_exception
    return self.ensure_sync(handler)(e)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "work/foo.py", line 9, in do_400
    url_for("foo")
  File "work/venv/lib/python3.12/site-packages/flask/helpers.py", line 225, in url_for
    return current_app.url_for(
           ^^^^^^^^^^^^^^^^^^^^
  File "work/venv/lib/python3.12/site-packages/flask/app.py", line 1686, in url_for
    rv = url_adapter.build(  # type: ignore[union-attr]
         ^^^^^^^^^^^^^^^^^
AttributeError: 'NoneType' object has no attribute 'build'

As you can see, the server_name passed to flask is invalid. This should normally not happen, but let's just assume that it might happen. In my case I use cheroot and it let's it thru.
Then the server name should be encoded with IDNA which fails and that is fine. However, then the custom error handler for 400 is called and in my case it contains a url_for call. This call then fails with an AttributeError because the request was never fully consumed by flask.

Even if this seems now a little bit artificial, I would like to ask, if flask could improve its handling here, I have the following ideas

  1. Don't call custom error handlers when such an error occurs
  2. Have a nicer exception than AttributeError so that the caller of url_for can handle it gracefully
  3. Handle a None url_adapter in url_for

Environment:

  • Python version: 3.12.0
  • Flask version: 2.3.3
  • Werkzeug: 2.3.8
    This error also happens with flask/werkzeug 3.0
@davidism davidism changed the title Invalid URL encoding + url_for leads to AttributeError Invalid SERVER_NAME + url_for leads to AttributeError Jan 29, 2024
@davidism
Copy link
Member

davidism commented Feb 3, 2024

I'm having a hard time evaluating this. None of the suggested fixes are really useful.

1 is not possible. 3 is not possible, except to "handle" it by raising an error, which is what happens when the app context (instead of request context) can't create an adapter. 2 is possible, at least to raise a clearer error message closer to where the problem occurs, as explained for 3, but it shouldn't be something the dev should consider handling, since ultimately it's an issue with the server environment.

There is a little known feature called app.url_build_error_handlers, which is a list of handlers to call when adapter.build() raises a BuildError. Right now, it's only called when adapter.build() fails with a BuildError, not for other errors or errors raised by Flask when wrapping that call. This could be extended so that the RuntimeError and ValueError that url_for raises in certain situations would be a BuildError instead and pass through the same handlers. By default, no handlers means that the error is re-raised.

@kasium
Copy link
Author

kasium commented Feb 3, 2024

Thanks for checking this. I know it's a weird one, with not a clear path forward.

I like the idea to register a url build error handler 👍

@kasium
Copy link
Author

kasium commented Feb 9, 2024

Hi, I now found a reproducible example by just providing an invalid Host header value. I also reported it to cheroot: cherrypy/cheroot#646

@davidism
Copy link
Member

After thinking about this more, I'm going to close it for now. It seems to be mainly a server configuration issue rather than an issue with Flask. I don't think spending time resurrecting/changing an unused feature in Flask is really worth it for this.

@davidism davidism closed this as not planned Won't fix, can't repro, duplicate, stale Mar 30, 2024
@kasium
Copy link
Author

kasium commented Mar 30, 2024

Anyways, thanks for the time analysis this. I hugkly appreciate it!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants