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

werkzeug.urls.uri_to_iri is incompatible with host parsing for request.url in flask #3033

Closed
jsoref opened this issue Mar 17, 2025 · 6 comments

Comments

@jsoref
Copy link

jsoref commented Mar 17, 2025

  1. Run a simple flask app that spits out things based on request.url -- https://github.com/GarnerCorp/nginx-debugging-backend/blob/main/debug.py
  2. Make a curl request with -H 'host: 127.0.0.1:y' to the flask app
  3. Get an error:
root@2ebfd118e681:/app# ./debug.py 
 * Serving Flask app 'debug'
 * Debug mode: off
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on all addresses (0.0.0.0)
 * Running on http://127.0.0.1:8080
 * Running on http://172.17.0.2:8080
Press CTRL+C to quit
[2025-03-17 21:01:57,856] ERROR in app: Exception on / [GET]
Traceback (most recent call last):
  File "/app/./lib/python3.11/site-packages/flask/app.py", line 1511, in wsgi_app
    response = self.full_dispatch_request()
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/./lib/python3.11/site-packages/flask/app.py", line 919, in full_dispatch_request
    rv = self.handle_user_exception(e)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/./lib/python3.11/site-packages/flask/app.py", line 917, in full_dispatch_request
    rv = self.dispatch_request()
         ^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/./lib/python3.11/site-packages/flask/app.py", line 902, in dispatch_request
    return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)  # type: ignore[no-any-return]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/./debug.py", line 136, in index
    x.append("%s %s" % (request.method, request.url))
                                        ^^^^^^^^^^^
  File "/app/./lib/python3.11/site-packages/werkzeug/utils.py", line 107, in __get__
    value = self.fget(obj)  # type: ignore
            ^^^^^^^^^^^^^^
  File "/app/./lib/python3.11/site-packages/werkzeug/sansio/request.py", line 208, in url
    return get_current_url(
           ^^^^^^^^^^^^^^^^
  File "/app/./lib/python3.11/site-packages/werkzeug/sansio/utils.py", line 145, in get_current_url
    return uri_to_iri("".join(url))
           ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/app/./lib/python3.11/site-packages/werkzeug/urls.py", line 98, in uri_to_iri
    if parts.port:
       ^^^^^^^^^^
  File "/usr/local/lib/python3.11/urllib/parse.py", line 182, in port
    raise ValueError(f"Port could not be cast to integer value as {port!r}")
ValueError: Port could not be cast to integer value as 'y'
172.17.0.1 - - [17/Mar/2025 21:01:57] "GET / HTTP/1.1" 500 -

The app line is approximately:
https://github.com/GarnerCorp/nginx-debugging-backend/blob/5a2adfb27340dc35ef02b848e404b139fea38c59/debug.py#L134

value = self.fget(obj) # type: ignore

return get_current_url(

return uri_to_iri("".join(url))

if parts.port:
netloc = f"{netloc}:{parts.port}"

Fwiw, flask 2 w/ werkzeug 2.0.2 doesn't die (because it doesn't have this fancy port reconstruction code).

@davidism
Copy link
Member

What did you expect instead? You sent invalid data and got an error.

@jsoref
Copy link
Author

jsoref commented Mar 17, 2025

I didn't send invalid data. Some random attacker did.

I don't expect an exception. And one isn't documented here:

https://flask.palletsprojects.com/en/stable/api/#flask.Request.url

property url: str
The full request URL with the scheme, host, root path, path, and query string.

That said, there are platforms where one can do something like http://example.com:http and http would be resolved to the defined service port, so I personally would have been happy with the :y being returned as is.

Alternatively, an exception that includes all of the pieces so that I could manually deal w/ them would be appreciated.

@davidism
Copy link
Member

davidism commented Mar 17, 2025

Documentation in Python in general doesn't list every way a thing can fail at any level when given invalid data.

I don't plan to add support for non-int ports.

What pieces need to be in the exception for you? It seems it's already there: the port is not an int. Also note that this is coming from urllib.parse, not Werkzeug.

@jsoref
Copy link
Author

jsoref commented Mar 17, 2025

Enough information for me to rebuild the url by hand.

The underlying urllib.parse message is missing all of the information needed for me to do anything for logging purposes.

It's possible I should have filed this against Flask instead of werzeug. As a consumer, I naively assumed the two projects cooperated (the versioning seemed to be fairly close to lockstep).

It looks like a user should avoid using request.url in favor of f"{request.scheme}://{request.host}{something}{request.full_path}" (I'm not quite sure what something looks like...) if they're just trying to log incoming values and not have an exception occur that eats things.

The point of the code in question is to log a request for debugging purposes. If a client sent garbage, the logging needs to be able to show the garbage the client sent.

@jsoref
Copy link
Author

jsoref commented Mar 17, 2025

Note that I filed this here because an older version of this module didn't have this behavior and it wasn't listed in the changelog for the function

.. versionchanged:: 3.0
Passing a tuple or bytes, and the ``charset`` and ``errors`` parameters,
are removed.
.. versionchanged:: 2.3
Which characters remain quoted is specific to each part of the URL.
.. versionchanged:: 0.15
All reserved and invalid characters remain quoted. Previously,
only some reserved characters were preserved, and invalid bytes
were replaced instead of left quoted.
:

@davidism
Copy link
Member

davidism commented Mar 17, 2025

Enough information for me to rebuild the url by hand.

In general, that's not a goal of our exceptions, or of exceptions in Python in general.

Logging the URL regardless of subsequent processing would typically happen at the HTTP or WSGI server level, not at the WSGI app request processing level.

Note that I filed this here because an older version of this module didn't have this behavior and it wasn't listed in the changelog for the function

Because nothing specific changed to be called out. We did a huge switch to urllib.parse at one point, but did not do a comprehensive survey to enumerate every possible effect that had. All tests continued to pass for that change. We also have no tests for parsing non-int ports, or documentation saying we do, so it would have been unlikely to have been mentioned even so.

@davidism davidism closed this as not planned Won't fix, can't repro, duplicate, stale Mar 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants