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.

Sign up for GitHub

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

Fix parsing of ports inside the URI #156

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix parsing of ports inside the URI #156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79d0b6a
attempt parsing of port only if colon was found
Jul 12, 2024
8a6591b
https flag is not mirrored from Url() to BaseUrl
Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/http.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,18 +140,22 @@ Url Url::Parse(std::string value) {
unsigned int port = 0;
struct sockaddr_in6 dst;
if (inet_pton(AF_INET6, host.c_str(), &(dst.sin6_addr)) <= 0) {
if (host.front() != '[' || host.back() != ']') {
if ((host.front() != '[' || host.back() != ']') && utils::Contains(host, ':')) {
std::stringstream ss(host);
std::string portstr;
while (std::getline(ss, portstr, ':')) {
}

if (!portstr.empty()) {
try {
port = static_cast<unsigned>(std::stoi(portstr));
char* str_end{};
const long l = std::strtol(portstr.c_str(), &str_end, 10);
size_t length = std::distance(portstr.c_str(), const_cast<const char*>(str_end));
if (length == portstr.size()) {
if (l < 1 || l > 65535) {
return Url{};
}
port = static_cast<int>(l);
host = host.substr(0, host.rfind(":" + portstr));
} catch (const std::invalid_argument&) {
port = 0;
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/request.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ BaseUrl::BaseUrl(std::string host, bool https, std::string region)
return;
}

this->https = url.https;
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct. this->https is set in the constructor definition already.

Copy link
Author

Choose a reason for hiding this comment

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

I have seen that, but the Url class parses the schema to set the https flag in the url class that…
Correct me if I am wrong, but if I am constructing a u=BasicUrl("https://foo.bar", false)
Then u.https is false

Copy link
Member

Choose a reason for hiding this comment

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

Firstly BaseUrl accepts host. If URL is used, it takes only host and port. Secondly passed argument should override values. The current behavior is correct and this change is not correct.

Copy link

Choose a reason for hiding this comment

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

I don't think this is a normal operation, in fact, I happened to experience this problem and have been troubled for a while for using of an HTTP address but the constructed baseURL being the default HTTPS. I cannot understand choosing to pass parameters instead of using URL parsed values. After all, people are sensitive to their own URLs and are not aware of the construction of functions in third-party libraries

Copy link
Member

Choose a reason for hiding this comment

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

The right fix is to fail if URL is passed like minio-go and minio-py. We should not support URL style as base URL

this->host = url.host;
this->port = url.port;

Expand Down
31 changes: 31 additions & 0 deletions tests/tests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,36 @@ class Tests {
throw;
}
}

void TestBaseUrl() {
std::cout << "TestBaseUrl()" << std::endl;
std::vector<std::tuple<std::string, std::string, bool, int>> tests = {
{"http://localhost:9000", "localhost", false, 9000},
{"http://localhost", "localhost", false, 0},
{"http://localhost:80", "localhost", false, 0},
{"https://localhost:9443", "localhost", true, 9443},
{"https://localhost", "localhost", true, 0},
{"https://5.localhost.foo", "5.localhost.foo", true, 0},
{"https://5.localhost.foo:9000", "5.localhost.foo", true, 9000},
};
for (auto& [url, host, https, port] : tests) {
minio::s3::BaseUrl base_url(url);
if (base_url.host != host) {
throw std::runtime_error("BaseUrl(" + url +").host: expected: " + host +
", got: " + base_url.host);
}
if (base_url.https != https) {
throw std::runtime_error("BaseUrl("+ url +").https: expected: " +
std::to_string(https) +
", got: " + std::to_string(base_url.https));
}
if (base_url.port != port) {
throw std::runtime_error("BaseUrl("+ url +").port: expected: " +
std::to_string(port) +
", got: " + std::to_string(base_url.port));
}
}
}
}; // class Tests

int main(int /*argc*/, char* /*argv*/[]) {
Expand Down Expand Up @@ -756,6 +786,7 @@ int main(int /*argc*/, char* /*argv*/[]) {
tests.RemoveObjects();
tests.SelectObjectContent();
tests.ListenBucketNotification();
tests.TestBaseUrl();

return EXIT_SUCCESS;
}