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

IPv6 handling bug in starlette.datastructures.URL #1931

Closed
dimaqq opened this issue Nov 2, 2022 · 6 comments
Closed

IPv6 handling bug in starlette.datastructures.URL #1931

dimaqq opened this issue Nov 2, 2022 · 6 comments
Labels
good first issue Good for beginners

Comments

@dimaqq
Copy link

dimaqq commented Nov 2, 2022

URL uses stdlib urlsplit to parse the string URL,

however, it uses this code to recombine the host and port.

            netloc = hostname                                                           
            if port is not None:                                                        
                netloc += f":{port}"  

Consider an IPv6 URL, like http://[::1]:8080/

  • urlsplit will strip the quare brackets and produce ::1
  • manual concatenation will yield ::1:8080
  • which is not a valid netloc
@dimaqq dimaqq changed the title Possible but in IPv6 handling in starlette.datastructures.URL Possible bug in IPv6 handling in starlette.datastructures.URL Nov 2, 2022
@dimaqq
Copy link
Author

dimaqq commented Nov 15, 2022

MRE

In [5]: import starlette.datastructures

In [6]: starlette.datastructures.URL("https://[fe::2]:12345").replace(port=42)
Out[6]: URL('https://fe::2:42')

@dimaqq dimaqq changed the title Possible bug in IPv6 handling in starlette.datastructures.URL IPv6 handling bug in starlette.datastructures.URL Nov 15, 2022
@dimaqq
Copy link
Author

dimaqq commented Nov 15, 2022

@Kludex I think good-first-issue tag would apply here 🙏🏻

@Kludex Kludex added the good first issue Good for beginners label Nov 16, 2022
@Kludex
Copy link
Sponsor Member

Kludex commented Nov 16, 2022

Ok 👀

So... If it's too complicated, it's on @dimaqq 👀

kousikmitra added a commit to kousikmitra/starlette that referenced this issue Nov 25, 2022
@kousikmitra
Copy link
Contributor

Hi @Kludex I have raised a PR for this #1965, This is my first PR in this repo so do let me know if I missed anything.

kousikmitra added a commit to kousikmitra/starlette that referenced this issue Nov 25, 2022
kousikmitra added a commit to kousikmitra/starlette that referenced this issue Nov 25, 2022
kousikmitra added a commit to kousikmitra/starlette that referenced this issue Nov 29, 2022
kousikmitra added a commit to kousikmitra/starlette that referenced this issue Dec 5, 2022
@brycefisher
Copy link

@kousikmitra thanks for your fix PR!! Looks like you might need to pull in recent changes from the main branch.

@Kludex any other feedback you have for @kousikmitra? I'd love to get this fix merged and brought into FastAPI for my own project. Thanks everyone for all your contributions!

@Kludex Kludex added this to the Version 0.24.0 milestone Feb 4, 2023
@Kludex
Copy link
Sponsor Member

Kludex commented Feb 6, 2023

This was solved on #1965, and it will be available on Starlette 0.24.0.

@Kludex Kludex closed this as completed Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for beginners
Projects
None yet
Development

No branches or pull requests

4 participants