Skip to content

Conversation

LamentXU123
Copy link
Contributor

@LamentXU123 LamentXU123 commented Jun 28, 2025

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.


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

@LamentXU123
Copy link
Contributor Author

LamentXU123 commented Jun 28, 2025

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

Done now, sorry for the noise.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@serhiy-storchaka serhiy-storchaka added needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes labels Jul 11, 2025
@LamentXU123
Copy link
Contributor Author

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!

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have removed wrong code.

@LamentXU123
Copy link
Contributor Author

LamentXU123 commented Jul 12, 2025

You have removed wrong code.

Oups, my bad.

def set_ok_port(self, cookie, request):
if cookie.port_specified:
req_port = request_port(request)
if req_port is None:
Copy link
Contributor Author

@LamentXU123 LamentXU123 Jul 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove this also, since request_port() will no longer return None.

Note: In fact the original code here is also wrong. the req_port should be DEFAULT_HTTP_PORT instead of 80. nvm, we just remove this logic. I will create a issue and PR later for this. This and the case below are the only case of using 80 instead of the const.

@LamentXU123
Copy link
Contributor Author

LamentXU123 commented Jul 13, 2025

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.

i = request.host.rfind(':')
if (i >= 0
and not ']' in request.host[i+1:]): # to prevent IPv6 addresses
_debug("nonnumeric port: '%s'", request.host[i+1:])
Copy link
Contributor Author

@LamentXU123 LamentXU123 Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic is added to print debug message in the original code. ValueError is no longer to be raised, but we still need debug message here.

Copy link
Contributor Author

@LamentXU123 LamentXU123 Jul 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm adding stricter check to this. There may be case like www.acme.com:1]34, and 1]34 is not a normal port and needs debug message, so I think I would add a check that the host name must starts with [

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting review needs backport to 3.13 bugs and security fixes needs backport to 3.14 bugs and security fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants