Files that changed from the base of the PR and between 7043d04 and dd05e19.

• components/core/src/clp/CurlDownloadHandler.cpp (3 hunks)
• components/core/src/clp/CurlDownloadHandler.hpp (2 hunks)
• components/core/src/clp/NetworkReader.cpp (3 hunks)
• components/core/src/clp/NetworkReader.hpp (4 hunks)

components/core/src/clp/CurlDownloadHandler.cpp (2)

5-5: LGTM: Required include for std::map

The addition of the map header is necessary for the custom headers parameter.

---

23-24: 🛠️ Refactor suggestion

Consider adding parameter validation

While the implementation accepts custom headers, it might be beneficial to validate the map's contents before processing.

Let's check if there are any existing parameter validations in the codebase:

Consider adding validation:

        std::chrono::seconds overall_timeout,
        std::map<std::string, std::string> const& custom_headers
)
        : m_error_msg_buf{std::move(error_msg_buf)} {
+    // Validate custom headers
+    for (auto const& [key, value] : custom_headers) {
+        if (key.empty() || value.empty()) {
+            throw std::invalid_argument("Empty key or value in custom headers");
+        }
+    }

components/core/src/clp/CurlDownloadHandler.hpp (1)

7-7: LGTM: Include statement properly added

The <map> header is correctly included and properly ordered alphabetically with other standard headers.

components/core/src/clp/NetworkReader.cpp (2)

121-122: 🛠️ Refactor suggestion

Consider adding header validation and documentation.

While the implementation correctly handles custom headers, consider:

1. Adding validation for header names and values to prevent injection of malicious or invalid headers
2. Documenting the expected format and any restrictions for header names/values in the constructor's documentation

Let's check if there are any existing header validation patterns in the codebase:

Consider adding validation like this:

void validate_headers(std::map<std::string, std::string> const& headers) {
    for (auto const& [name, value] : headers) {
        if (name.empty() || value.empty()) {
            throw std::invalid_argument("Header name and value cannot be empty");
        }
        // Add more validation as needed
    }
}

Also applies to: 134-134

---

219-220: 🛠️ Refactor suggestion

Consider enhancing error handling for header-related failures.

The current implementation might benefit from specific error handling for header-related failures during CURL operations. Consider:

1. Adding specific error codes or exceptions for header-related failures
2. Logging header-related errors separately for better debugging

Let's check existing error handling patterns:

Consider extending error handling like this:

try {
    CurlDownloadHandler curl_handler{/*...*/};
    auto const ret_code{curl_handler.perform()};
    if (CURLE_UNSUPPORTED_PROTOCOL == ret_code) {
        // Log specific header-related error
        logger.error("Failed to set custom headers: {}", m_reader.m_curl_error_msg_buf);
    }
    // ... rest of the code
}

components/core/src/clp/NetworkReader.hpp (2)

9-9: LGTM: Header inclusion is properly placed

The addition of <map> header is necessary for the custom headers functionality and follows the file's include ordering convention.

---

248-252: LGTM: Constructor parameter formatting improves readability

The multi-line parameter formatting enhances code readability while maintaining consistent style.

Files that changed from the base of the PR and between dd05e19 and f134870.

• components/core/src/clp/CurlDownloadHandler.cpp (3 hunks)
• components/core/src/clp/CurlDownloadHandler.hpp (3 hunks)
• components/core/src/clp/NetworkReader.hpp (5 hunks)
• components/core/tests/test-NetworkReader.cpp (2 hunks)

• components/core/src/clp/CurlDownloadHandler.hpp

components/core/src/clp/CurlDownloadHandler.cpp (3)

5-8: LGTM: Required headers added appropriately

The new includes are properly ordered and necessary for the custom headers implementation.

---

25-26: LGTM: Constructor parameter properly added

The custom_headers parameter is appropriately added as a const reference, which is efficient for passing map data.

---

63-67: LGTM: Case-insensitive header filtering implemented

The implementation properly handles case-insensitive comparison of reserved headers.

components/core/tests/test-NetworkReader.cpp (4)

5-5: LGTM: Appropriate header inclusion

The addition of the map header is necessary and correctly placed with other STL includes.

---

195-197: Consider mocking external HTTP service

The test depends on httpbin.org, which could cause test failures if the service is unavailable. Consider:

1. Using a local mock server for testing
2. Adding retry logic or proper error handling
3. Documenting the external dependency

---

203-207: LGTM: Good coverage of protected headers

Excellent test coverage ensuring that critical headers (Range, Cache-Control, Pragma) cannot be overridden by custom headers.

---

233-241: LGTM: Comprehensive validation of header restrictions

Good coverage ensuring that:

1. Protected headers are not included
2. Invalid header names are rejected
3. Headers with special characters are handled correctly

components/core/src/clp/NetworkReader.hpp (1)

Line range hint 248-267: Verify thread safety and memory handling of custom headers

The implementation looks good, but consider the following aspects:

1. Thread Safety: The current implementation correctly handles thread safety by storing a copy of the headers map.
2. Memory Management: Consider clearing sensitive header data (like authentication tokens) after the download is complete.

Let's verify the usage of these headers:

✅ Verification successful

Let me verify how the custom headers are used in the codebase, particularly in the NetworkReader class.

---

Custom headers implementation is secure and well-managed

The implementation correctly handles custom headers with proper security considerations:

1. Headers are passed by const reference and stored as a copy in the thread for thread safety
2. Reserved headers (Range, Cache-Control, Pragma) are protected from being overridden
3. Headers are validated and sanitized in CurlDownloadHandler before use
4. Headers are scoped to the download thread's lifetime and automatically cleaned up

No security or memory management issues found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for potential sensitive header handling in the codebase
rg -i "authorization|bearer|token|secret|password" --type cpp

Length of output: 119815

---

Script:

#!/bin/bash
# Search for custom_headers usage in NetworkReader and related files
rg -A 5 -B 5 "custom_headers" --type cpp

Length of output: 15079

Files that changed from the base of the PR and between f134870 and fc7aa7d.

• components/core/src/clp/CurlDownloadHandler.cpp (3 hunks)
• components/core/tests/test-NetworkReader.cpp (2 hunks)

• components/core/tests/test-NetworkReader.cpp

components/core/src/clp/CurlDownloadHandler.cpp (1)

77-82: ⚠️ Potential issue

Optimize regex usage and update patterns to comply with RFC 7230

The regex patterns for header names and values are being compiled in each iteration of the loop. To improve performance, declare the regex objects as static const outside the loop to avoid recompilation. Additionally, update the header_name_pattern to comply with RFC 7230, which specifies that header field names must start with a letter and can contain letters, digits, and hyphens (-), but not underscores (_).

Apply this diff to address the concerns:

+        // RFC 7230 compliant header name and value patterns
+        static const std::regex header_name_pattern("^[A-Za-z][A-Za-z0-9-]*$");
+        static const std::regex header_value_pattern("^[\\x20-\\x7E]+$");
         for (const auto& [key, value] : custom_headers) {
             // Convert to lowercase for case-insensitive comparison
             std::string lower_key = key;
             std::transform(lower_key.begin(), lower_key.end(), lower_key.begin(), ::tolower);
             
             if (reserved_headers.end() == reserved_headers.find(lower_key)) {
-                // Filter out illegal header names and header values by regex
-                // Can contain alphanumeric characters (A-Z, a-z, 0-9), hyphens (`-`), and underscores
-                // (`_`)
-                std::regex header_name_pattern("^[A-Za-z0-9_-]+$");
-                // Must consist of printable ASCII characters (values between 0x20 and 0x7E)
-                std::regex header_value_pattern("^[\\x20-\\x7E]*$");
                 if (std::regex_match(key, header_name_pattern) && 
                     std::regex_match(value, header_value_pattern)) {
                     m_http_headers.append(key + ": " + value);
                 }
             }
         }

Files that changed from the base of the PR and between fc7aa7d and c8acc12.

• components/core/src/clp/CurlDownloadHandler.cpp (3 hunks)
• components/core/src/clp/NetworkReader.cpp (3 hunks)
• components/core/tests/test-NetworkReader.cpp (2 hunks)

• components/core/src/clp/NetworkReader.cpp
• components/core/tests/test-NetworkReader.cpp

components/core/src/clp/CurlDownloadHandler.cpp (4)

5-8: Include directives updated appropriately for custom headers support

The addition of <map> and <regex> is necessary for handling custom headers and performing validations.

---

25-26: Constructor signature correctly extended with custom_headers

Including std::map<std::string, std::string> const& custom_headers enhances the class's ability to accept and process custom headers.

---

63-67: Reserved headers defined appropriately

Defining kReservedHeaders as a local constant improves readability, and the reserved headers are correctly specified to prevent overriding critical HTTP headers.

---

71-71: Proper character conversion in std::tolower

Casting characters to unsigned char in the lambda function ensures correct behaviour and avoids undefined behaviour with negative char values.

Files that changed from the base of the PR and between c8acc12 and e1c0605.

• components/core/src/clp/CurlDownloadHandler.cpp (3 hunks)
• components/core/src/clp/CurlDownloadHandler.hpp (3 hunks)
• components/core/src/clp/NetworkReader.cpp (3 hunks)
• components/core/src/clp/NetworkReader.hpp (5 hunks)
• components/core/tests/test-NetworkReader.cpp (3 hunks)

• components/core/src/clp/CurlDownloadHandler.hpp
• components/core/src/clp/NetworkReader.cpp
• components/core/src/clp/NetworkReader.hpp
• components/core/tests/test-NetworkReader.cpp

components/core/src/clp/CurlDownloadHandler.cpp (2)

5-5: LGTM! New includes are properly organized.

The added includes for map, regex, and set are necessary for the custom headers implementation and follow the project's include ordering convention.

Also applies to: 7-8

---

25-26: LGTM! Constructor parameter addition is well-designed.

The custom_headers parameter is appropriately added as a const reference, which is efficient for passing map containers.

Files that changed from the base of the PR and between e1c0605 and cf46ab7.

• components/core/src/clp/CurlDownloadHandler.cpp (3 hunks)
• components/core/src/clp/CurlDownloadHandler.hpp (3 hunks)
• components/core/src/clp/NetworkReader.cpp (3 hunks)
• components/core/src/clp/NetworkReader.hpp (5 hunks)
• components/core/tests/test-NetworkReader.cpp (3 hunks)

• components/core/src/clp/CurlDownloadHandler.hpp
• components/core/src/clp/NetworkReader.hpp

components/core/src/clp/CurlDownloadHandler.cpp (2)

Line range hint 3-27: LGTM: Constructor signature and includes are well-structured

The new parameter follows the established patterns and the includes are properly organized.

---

99-105: ⚠️ Potential issue

Improve error message for reserved header usage

The error message should indicate which reserved header was attempted to be used.

Apply this diff:

-                        "curl_download_handler_init failed."
+                        fmt::format("Cannot override reserved header: '{}'", key)

Likely invalid or redundant comment.

components/core/tests/test-NetworkReader.cpp (1)

213-223: LGTM! Well-structured test setup

The test case effectively sets up custom headers using proper formatting and type-safe iterations. The use of httpbin.org as a test endpoint is a good choice for header validation.

components/core/src/clp/NetworkReader.cpp (3)

121-122: LGTM: Constructor parameter addition is well-designed

The new custom_headers parameter is appropriately implemented as an optional const reference to an unordered map, which is memory efficient and maintains ABI compatibility.

---

134-135: LGTM: Thread initialization properly handles custom headers

The initialization correctly forwards the custom headers to the DownloaderThread while maintaining exception safety through std::make_unique.

---

220-221: Verify thread safety and lifetime of custom headers

While the implementation looks correct, please ensure:

1. The custom headers map's lifetime extends beyond the download operation
2. Thread safety is maintained when accessing the headers

Consider storing a copy of the headers in the DownloaderThread to prevent potential lifetime issues. Example implementation:

- m_custom_headers
+ m_custom_headers_copy // Store a copy in DownloaderThread

Run this verification script:

✅ Verification successful

Based on the search results, I can now provide a final response:

Thread safety and lifetime concerns are properly addressed

The implementation is correct as is, because:

1. m_custom_headers is stored as a member variable in DownloaderThread and initialized through the constructor, making it a thread-local copy
2. The headers are passed by const reference to CurlDownloadHandler, ensuring thread safety
3. The lifetime is managed appropriately since each thread maintains its own copy of the headers

The code follows a proper thread-safe design pattern where:

• Headers are passed as std::optional<std::unordered_map> through the constructor
• Each thread instance maintains its own copy via m_custom_headers member
• Access is read-only during download operations

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for potential thread safety issues with custom headers access
rg -A 3 "custom_headers" --type cpp

# Verify if there are any other places accessing the headers that might need synchronization
ast-grep --pattern 'm_custom_headers'

Length of output: 6862

Files that changed from the base of the PR and between cf46ab7 and d995a0f.

• components/core/src/clp/CurlDownloadHandler.cpp (3 hunks)
• components/core/tests/test-NetworkReader.cpp (3 hunks)

components/core/src/clp/CurlDownloadHandler.cpp (3)

3-3: LGTM: Required includes added correctly

The new includes are properly ordered and necessary for the implementation.

Also applies to: 7-7, 13-13

---

25-26: LGTM: Parameter signature is well-designed

The parameter type choice (const reference to optional unordered_map) is appropriate for passing custom headers efficiently.

---

67-69: 🛠️ Refactor suggestion

Simplify header name pattern and add pattern documentation

The current regex pattern is complex and could be simplified while maintaining RFC 7230 compliance.

Apply this diff:

-        std::regex const header_name_pattern("^(?![!#$%&'*+.^_`|~-])[!#$%&'*+.^_`|~0-9a-zA-Z-]+$");
+        // RFC 7230: token must start with a letter followed by allowed characters
+        std::regex const header_name_pattern("^[a-zA-Z][!#$%&'*+.^_`|~0-9a-zA-Z-]*$");

Likely invalid or redundant comment.

components/core/tests/test-NetworkReader.cpp (2)

9-9: LGTM: Required includes added appropriately

The new includes support custom headers (unordered_map), string formatting (fmt), and JSON response parsing (nlohmann/json).

Also applies to: 14-15

---

212-254: LGTM: Comprehensive test coverage

The test case effectively covers:

1. Regular custom headers functionality
2. Protection against overriding reserved headers
3. Validation of header names and values
4. Error handling for illegal headers

Let's verify we're testing all reserved headers:

✅ Verification successful

Let me analyze the search results to verify the reserved headers testing coverage.

From the codebase search, I found that the test is already comprehensively covering all the reserved headers that are used in the NetworkReader implementation. Specifically:

1. The test checks the three reserved headers that are internally set by NetworkReader/CurlDownloadHandler:

   • "Range" (for offset)
   • "Cache-Control" (for disable_cache)
   • "Pragma" (for disable_cache)

2. Looking at CurlDownloadHandler.cpp implementation, these are indeed the only headers that are reserved and automatically set:

if (0 != offset) {
    std::string const range{"Range: bytes=" + std::to_string(offset) + "-"};
    m_http_headers.append(range);
}
if (disable_caching) {
    m_http_headers.append("Cache-Control: no-cache");
    m_http_headers.append("Pragma: no-cache");
}

The test coverage for reserved headers is complete and accurate

The test correctly verifies that these headers cannot be overridden through custom headers, which is the key security concern being tested here.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other reserved headers in the codebase that we might need to test
rg -g '!*.{md,txt}' -i 'header|curl.*set.*opt' -A 3 -B 3

Length of output: 91532

Files that changed from the base of the PR and between d995a0f and 91209ac.

• components/core/src/clp/CurlDownloadHandler.cpp (3 hunks)

components/core/src/clp/CurlDownloadHandler.cpp (5)

3-14: LGTM: Include statements and constructor signature changes are well-implemented

The new includes and constructor signature changes are appropriate for the custom headers feature. The use of std::optional for the custom_headers parameter allows for backward compatibility, and passing by const reference is efficient for the map type.

Also applies to: 24-27

---

64-65: LGTM: Reserved headers declaration is well-implemented

The use of constexpr array with string_view for reserved headers is efficient and compile-time evaluated.

---

87-87: ⚠️ Potential issue

Add CRLF validation for header values

As per libcurl documentation, headers must not be CRLF-terminated since libcurl adds CRLF itself.

Add validation before appending the header:

-                    m_http_headers.append(fmt::format("{}: {}", key, value));
+                    if (value.ends_with("\r\n")) {
+                        throw CurlOperationFailed(
+                            ErrorCode_Failure,
+                            __FILE__,
+                            __LINE__,
+                            CURLE_BAD_FUNCTION_ARGUMENT,
+                            fmt::format("Header value must not be CRLF-terminated - key: '{}'", key));
+                    }
+                    m_http_headers.append(fmt::format("{}: {}", key, value));

Likely invalid or redundant comment.

---

67-69: 🛠️ Refactor suggestion

Simplify the header name validation pattern

The current pattern is overly complex and could be simplified while maintaining RFC 7230 compliance.

Consider this simpler pattern:

-        std::regex const header_name_pattern("^(?![!#$%&'*+.^_`|~-])[!#$%&'*+.^_`|~0-9a-zA-Z-]+$");
+        // RFC 7230: token must start with a letter followed by allowed characters
+        std::regex const header_name_pattern("^[a-zA-Z][!#$%&'*+.^_`|~0-9a-zA-Z-]*$");

Likely invalid or redundant comment.

---

80-82: 🛠️ Refactor suggestion

Simplify the reserved header check

The current implementation using std::find could be simplified using a range-based for loop.

Consider this more readable approach:

-            if (cReservedHeaders.end()
-                == std::find(cReservedHeaders.begin(), cReservedHeaders.end(), lower_key))
+            bool is_reserved = false;
+            for (auto const reserved : cReservedHeaders) {
+                if (lower_key == reserved) {
+                    is_reserved = true;
+                    break;
+                }
+            }
+            if (!is_reserved)

Likely invalid or redundant comment.