Skip to content

fix(httpcommon): sonarqube warning cleanup #3558

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

Merged
ReenigneArcher merged 4 commits into from
Jan 22, 2025
Merged

fix(httpcommon): sonarqube warning cleanup #3558

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eb9b52c
fix(httpcommon): sonarqube warning cleanup
CodyManess Jan 18, 2025
e49a7fe
fix(httpcommon): test update and comment recs
CodyManess Jan 21, 2025
ebf8ff8
Update tests/unit/test_httpcommon.cpp
CodyManess Jan 22, 2025
22d782c
Update src/httpcommon.h
ReenigneArcher Jan 22, 2025
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
37 changes: 14 additions & 23 deletions src/httpcommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,14 @@ namespace http {
config::nvhttp.pkey = (dir / ("pkey-"s + unique_id)).string();
}

if (!fs::exists(config::nvhttp.pkey) || !fs::exists(config::nvhttp.cert)) {
if (create_creds(config::nvhttp.pkey, config::nvhttp.cert)) {
return -1;
}
if ((!fs::exists(config::nvhttp.pkey) || !fs::exists(config::nvhttp.cert)) &&
create_creds(config::nvhttp.pkey, config::nvhttp.cert)) {
return -1;
}
if (user_creds_exist(config::sunshine.credentials_file)) {
if (reload_user_creds(config::sunshine.credentials_file)) {
return -1;
}
} else {
if (!user_creds_exist(config::sunshine.credentials_file)) {
BOOST_LOG(info) << "Open the Web UI to set your new username and password and getting started";
} else if (reload_user_creds(config::sunshine.credentials_file)) {
return -1;
}
return 0;
}
Expand Down Expand Up @@ -179,19 +176,15 @@ namespace http {
return 0;
}

bool download_file(const std::string &url, const std::string &file) {
CURL *curl = curl_easy_init();
if (curl) {
// sonar complains about weak ssl and tls versions
// ideally, the setopts should go after the early returns; however sonar cannot detect the fix
curl_easy_setopt(curl, CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2);
} else {
bool download_file(const std::string &url, const std::string &file, long ssl_version) {
// sonar complains about weak ssl and tls versions; however sonar cannot detect the fix
CURL *curl = curl_easy_init(); // NOSONAR
if (!curl) {
BOOST_LOG(error) << "Couldn't create CURL instance";
return false;
}

std::string file_dir = file_handler::get_parent_directory(file);
if (!file_handler::make_directory(file_dir)) {
if (std::string file_dir = file_handler::get_parent_directory(file); !file_handler::make_directory(file_dir)) {
BOOST_LOG(error) << "Couldn't create directory ["sv << file_dir << ']';
curl_easy_cleanup(curl);
return false;
Expand All @@ -204,6 +197,7 @@ namespace http {
return false;
}

curl_easy_setopt(curl, CURLOPT_SSLVERSION, ssl_version); // NOSONAR
curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, fwrite);
curl_easy_setopt(curl, CURLOPT_WRITEDATA, fp);
Expand All @@ -219,17 +213,15 @@ namespace http {
}

std::string url_escape(const std::string &url) {
CURL *curl = curl_easy_init();
char *string = curl_easy_escape(curl, url.c_str(), url.length());
char *string = curl_easy_escape(nullptr, url.c_str(), static_cast<int>(url.length()));
std::string result(string);
curl_free(string);
curl_easy_cleanup(curl);

@ReenigneArcher ReenigneArcher Jan 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you explain why this was removed?

The other returns have curl_url_cleanup() called right before, this return now doesn't have any cleanup?

@CodyManess CodyManess Jan 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was looking for ways to get rid of the security error associated with line 222. Looking at the libcurl docs, I saw that under the History section the curl parameter is ignored with current versions of curl_easy_escape. So it seems that we don't need to init a curl pointer to use curl_easy_escape and can just pass it a nullptr for the curl parameter, so I got rid of the init and cleanup to remove the extra code as well as the security warning.

return result;
}

std::string url_get_host(const std::string &url) {
CURLU *curlu = curl_url();
curl_url_set(curlu, CURLUPART_URL, url.c_str(), url.length());
curl_url_set(curlu, CURLUPART_URL, url.c_str(), static_cast<unsigned int>(url.length()));
char *host;
if (curl_url_get(curlu, CURLUPART_HOST, &host, 0) != CURLUE_OK) {
curl_url_cleanup(curlu);
Expand All @@ -240,5 +232,4 @@ namespace http {
curl_url_cleanup(curlu);
return result;
}

} // namespace http
4 changes: 3 additions & 1 deletion src/httpcommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
*/
#pragma once

#include <curl/curl.h>
Comment thread
ReenigneArcher marked this conversation as resolved.

// local includes
#include "network.h"
#include "thread_safe.h"
Expand All @@ -20,7 +22,7 @@ namespace http {
);

int reload_user_creds(const std::string &file);
bool download_file(const std::string &url, const std::string &file);
bool download_file(const std::string &url, const std::string &file, long ssl_version = CURL_SSLVERSION_TLSv1_3);
std::string url_escape(const std::string &url);
std::string url_get_host(const std::string &url);

Expand Down
15 changes: 10 additions & 5 deletions tests/unit/test_httpcommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,19 @@
* @file tests/unit/test_httpcommon.cpp
* @brief Test src/httpcommon.*.
*/
// test imports
#include "../tests_common.h"

// lib imports
#include <curl/curl.h>

// local imports
#include <src/httpcommon.h>

struct UrlEscapeTest: testing::TestWithParam<std::tuple<std::string, std::string>> {};

TEST_P(UrlEscapeTest, Run) {
auto [input, expected] = GetParam();
const auto &[input, expected] = GetParam();
ASSERT_EQ(http::url_escape(input), expected);
}

Expand All @@ -26,7 +31,7 @@ INSTANTIATE_TEST_SUITE_P(
struct UrlGetHostTest: testing::TestWithParam<std::tuple<std::string, std::string>> {};

TEST_P(UrlGetHostTest, Run) {
auto [input, expected] = GetParam();
const auto &[input, expected] = GetParam();
ASSERT_EQ(http::url_get_host(input), expected);
}

Expand All @@ -43,10 +48,10 @@ INSTANTIATE_TEST_SUITE_P(
struct DownloadFileTest: testing::TestWithParam<std::tuple<std::string, std::string>> {};

TEST_P(DownloadFileTest, Run) {
auto [url, filename] = GetParam();
const auto &[url, filename] = GetParam();
const std::string test_dir = platf::appdata().string() + "/tests/";
std::basic_string path = test_dir + filename;
ASSERT_TRUE(http::download_file(url, path));
std::string path = test_dir + filename;
ASSERT_TRUE(http::download_file(url, path, CURL_SSLVERSION_TLSv1_0));
}

#ifdef SUNSHINE_BUILD_FLATPAK
Expand Down