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

Url type port limit not checked #1654

Closed
flapili opened this issue Jun 23, 2020 · 9 comments · Fixed by #1678
Closed

Url type port limit not checked #1654

flapili opened this issue Jun 23, 2020 · 9 comments · Fixed by #1678
Labels
Change

Comments

@flapili
Copy link
Contributor

@flapili flapili commented Jun 23, 2020

pydantic version: 1.5.1
pydantic compiled: False
install path: /opt/fastapi/backend/venv/lib/python3.8/site-packages/pydantic
python version: 3.8.3 (default, Jun 21 2020, 00:03:19)  [GCC 8.3.0]
platform: Linux-4.19.0-9-amd64-x86_64-with-glibc2.28
optional deps. installed: ['email-validator']

Hi,

The port limit is not checked by URLs types:

>>> from pydantic import BaseModel, stricturl
>>> class Test(BaseModel):
	url: stricturl(allowed_schemes=["tcp"])

	
>>> Test(url="tcp://127.0.0.1:100000000000000000000000000000")
Test(url=UrlValue('tcp://127.0.0.1:100000000000000000000000000000', scheme='tcp', host='127.0.0.1', host_type='ipv4', port='100000000000000000000000000000'))

Excepted result: raise an error,

my current workaround

>>> from pydantic import BaseModel, stricturl, validator
>>> from pydantic.networks import url_regex
>>> class Test(BaseModel):
	url: stricturl(allowed_schemes=["tcp"])
	@validator("url")
	def validate_port(cls, v):
		port = url_regex().match(v).group("port")
		if port is None:
		    return v

		if int(port) > 2**16:
			raise ValueError("port overflow")
		return v

	
>>> Test(url="tcp://127.0.0.1:900000")
Traceback (most recent call last):
  File "<pyshell#84>", line 1, in <module>
    Test(url="tcp://127.0.0.1:900000")
  File "pydantic\main.py", line 338, in pydantic.main.BaseModel.__init__
pydantic.error_wrappers.ValidationError: 1 validation error for Test
url
  port overflow (type=value_error)
@flapili flapili added the bug label Jun 23, 2020
@samuelcolvin samuelcolvin added Change and removed bug labels Jun 29, 2020
@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 29, 2020

Is there a nice RFC explicitly stating a limit for port? If so I guess we could add a check, but this is not a bug, just a limit of the current validation.

You can implement a much cleaner validator:

class Test(BaseModel):
    url: stricturl(allowed_schemes=['tcp'])

    @validator('url')
    def validate_port(cls, v):
        if int(v.port) > 2**16:
            raise ValueError('port overflow')
        return v

@flapili
Copy link
Contributor Author

@flapili flapili commented Jun 29, 2020

Hi,
rfc 793 explain ports are 16 unsigned bits

Your validator will fail if port is missing, with tcp protocol that doesn't really make sense but with http(s) protocol is does:
example

http://example.com the validator will fail because v.port is None, int will raise a TypeError, but thanks for the tips I totally forgot port attribute 🤦

class Test(BaseModel):
    url: stricturl(allowed_schemes=['tcp'])

    @validator('url')
    def validate_port(cls, v):
        if v.port is None:
            return v
        elif int(v.port) > 2**16:
            raise ValueError('port overflow')
        return v

(the tcp protocol is a bad showcase about default port)

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 29, 2020

Good point, if v.port is not None and int(v.port) > 2**16 would be better. (is port=0 allowed?)

rfc 793 explain ports are 16 unsigned bits

PR welcome to add this check to all URL validation, I know technically it's a breaking chance, but I think a reasonable one, given that a higher port is invalid anyway.

@samuelcolvin samuelcolvin reopened this Jun 29, 2020
@flapili
Copy link
Contributor Author

@flapili flapili commented Jun 30, 2020

the port 0 is tagged as reserved and should not be usable as such since most routers will reject it.
In a other hand in a lot of APIs port 0 mean "use the first avaiable socket" so I would say yes.

I'll work on a PR within a few days (this weekend I hope if I have time)

@samuelcolvin
Copy link
Owner

@samuelcolvin samuelcolvin commented Jun 30, 2020

makes sense to allow 0, also makes the check easier.

I'll work on a PR within a few days (this weekend I hope if I have time)

Thanks so much

@flapili
Copy link
Contributor Author

@flapili flapili commented Jul 2, 2020

I'm currently working on a PR,
looking at the sources of networks I have few questions about some design choices

Should I open news issues for each or asking here is fine ?

@flapili flapili mentioned this issue Jul 2, 2020
4 tasks
@Skeen
Copy link

@Skeen Skeen commented May 16, 2022

Would it make sense to separate Port into a seperate type, and to utilize it when checking urls? - I'm asking as I'm currently in need of port validation and I thought it was provided out of the box, or atleast as an internal type utilized by url parsing / validation.

@flapili
Copy link
Contributor Author

@flapili flapili commented May 16, 2022

Would it make sense to separate Port into a seperate type, and to utilize it when checking urls? - I'm asking as I'm currently in need of port validation and I thought it was provided out of the box, or atleast as an internal type utilized by url parsing / validation.

You can use conint for that @Skeen

@Skeen
Copy link

@Skeen Skeen commented May 16, 2022

@flapili I am aware and I am, but that requires me to find and check rfc793 instead of just importing that knowledge.

Either way, it was just a suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants