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

assign default port for http and https #9439

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

woodser
Copy link
Contributor

@woodser woodser commented Aug 15, 2024

No description provided.

@ndorf
Copy link
Contributor

ndorf commented Aug 16, 2024

If one wanted to be pedantic, the "proper" way to do this might be getservbyname(parsed.schema)...

@woodser woodser force-pushed the default_port_master branch from d543d99 to 8d2b4de Compare August 17, 2024 12:09
@woodser
Copy link
Contributor Author

woodser commented Aug 17, 2024

If one wanted to be pedantic, the "proper" way to do this might be getservbyname(parsed.schema)...

I've updated it to use getservbyname:

if (parsed.port == 0 && (parsed.schema == "http" || parsed.schema == "https"))
{
  struct servent *serv = getservbyname(parsed.schema.c_str(), "tcp");
  parsed.port = ntohs(serv->s_port);
}

@woodser
Copy link
Contributor Author

woodser commented Jan 6, 2025

I found that getservbyname does not work in emscripten environments, due to lack of system service databases like /etc/services.

Updated the PR to hardcode port 80 for http and 443 for https. These values are hardcoded elsewhere in the code so nothing new.

Bump to be included in the next release with the release PR: #9438

@iamamyth
Copy link

iamamyth commented Jan 6, 2025

I found that getservbyname does not work in emscripten environments, due to lack of system service databases like /etc/services.

This possibility worried me. Furthermore, one person's feature is another's bug: a "functioning" service database which returns different ports could create problems for ordinary users. Given that there's an IANA service/port registry which includes 80 and 443, and that registry won't be changing any time soon, one could argue that making the lookup dynamic would only create wrong behaviors (i.e. strictly downside, because it's certainly slower).

Updated the PR to hardcode port 80 for http and 443 for https. These values are hardcoded elsewhere in the code so nothing new.

It might be helpful to make (and use) a shared utility function in the future. But, in the interest of timeliness, I agree with the route you've chosen.

@woodser
Copy link
Contributor Author

woodser commented Jan 15, 2025

@nahuhh Raised a good point, which is that P2P for I2P (e.g. tx-proxy) uses default port 0. We want to ensure we're not incorrectly overriding the default port in this case. I'm not sure if that's an issue here or not. Maybe someone more knowledgeable will know if this code path is used for that?

@iamamyth
Copy link

P2P for I2P (e.g. tx-proxy) uses default port 0

I would not have approved it if this were an issue: This function sets the server to which an HTTP client connects, port 0 is not a valid port, that is precisely why it's used to mean "default" or "any" in networking contexts, just like 0.0.0.0 means the same for IPv4 addresses.

@woodser woodser force-pushed the default_port_master branch from a095901 to 02e2a4f Compare February 17, 2025 13:12
@woodser woodser changed the title https addresses use default port of 443 assign default port for http and https Feb 17, 2025
@tobtoht tobtoht merged commit aae0c77 into monero-project:master Feb 17, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants