cpython icon indicating copy to clipboard operation
cpython copied to clipboard

gh-135993: Fix IPv6 bug in `set_ok_port` and `return_ok_port`

Open LamentXU123 opened this issue 5 months ago • 1 comments

Yet another IPv6 bug fix.

Instead of using the helper functions here, I've found a regex in the code, it looks like this:

cut_port_re = re.compile(r":\d+$", re.ASCII)

This could perfectly solve the bug. caz currently, we are doing this:

def request_port(request):
    host = request.host
    i = host.find(':')
    if i >= 0:
        port = host[i+1:]
        try:
            int(port)
        except ValueError:
            _debug("nonnumeric port: '%s'", port)
            return None
    else:
        port = DEFAULT_HTTP_PORT
    return port

And the problem is if the input is IPv6 addr like [::1]:1234, the program will treat :1]:1234 as a port and raise a ValueError when trying to int() it.

Now, since we've got the regex, we could use it directly in this func to make sure that port numbers can only be numbers and fix the bug.

Something like this:

def request_port(request):
    match = cut_port_re.search(request.host)
    if match:
        port = match.group(0).removeprefix(':')
        try:
            int(port)
        except ValueError:
            _debug("nonnumeric port: '%s'", port)
            return None
    else:
        port = DEFAULT_HTTP_PORT
    return port

I've also added the tests.

cc @picnixz . Looking forward to your review, thank you for your time and effort in advance.

  • Issue: gh-135993

📚 Documentation preview 📚: https://cpython-previews--136076.org.readthedocs.build/

LamentXU123 avatar Jun 28 '25 14:06 LamentXU123

Sorry for trying to merge the wrong branch. I'm fixing it.

Done now, sorry for the noise.

LamentXU123 avatar Jun 28 '25 14:06 LamentXU123

Please add a test for empty port (e.g. "acme.com:") and nonnumeric port. I suspect there is an unintentional behavior difference here.

for empty port and nonnumeric port, the regex can't match the port and will treat it as DEFAULT_HTTP_PORT(which is normally 80). I think this is intended.

Add also tests for empty and nonnumeric port and for IPv6 address in test_request_port.

Done. Thanks!

LamentXU123 avatar Jul 12 '25 05:07 LamentXU123

You have removed wrong code.

Oups, my bad.

LamentXU123 avatar Jul 12 '25 14:07 LamentXU123

~Since request_port() is not a public function, I think removing the case that would return None has little drawbacks, but I'm not quiet sure whether we should remain the debug message......~

I remove the useless logic, but add one to remain the debug message.

LamentXU123 avatar Jul 13 '25 06:07 LamentXU123