-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: Server.Run(...) validate addr #63
Conversation
ade9264
to
cf9e02d
Compare
Add validation to the addr parameter of Server.Run(...) to ensure it is a valid TCP address. If the addr is ipv6, and it's missing the required square brackets, add them.
cf9e02d
to
2b5fa2c
Compare
server.go
Outdated
} | ||
|
||
// see if we're dealing with a hostname | ||
hostnames, _ := net.LookupHost(rawHost) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This triggers a DNS lookup and may be slow or may have unexpected consequences. I suggest netip.ParseAddr
and see if there is an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum... I converted to resolver.LookupHost(...) so I can add a timeout which will alievate some of the issues.
the point of looking up the host it to ensure we have an IP after this lookup which the rest of the func relies on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I thought that the point was to discover whether or not it's an IP address -- not that you needed an IP address at the end of the function...since Dial will do that resolution for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's ipv6, we sort of "need" a proper literal IP (if it's an IP) otherwise the Listen(...) will fail. Besides trying to form a proper literal ipv6, we're validating that addr is indeed something that we can pass along to the Listen(...) call.
At this point, we want to resolve the hostname if needed, so we can rely on the fact that we're mucking around with an IP for the rest of the func.
LGTM but the discussion was marked Outdated so I'm not sure you got notified about my comment: #63 (comment) If I'm wrong about that then no worries. |
Add validation to the addr parameter of
Server.Run(...) to ensure it is a valid TCP
address. If the addr is ipv6, and it's missing
the required square brackets, add them.