diff --git a/sdk/core/azure-core/CMakeLists.txt b/sdk/core/azure-core/CMakeLists.txt index 1b6a410aa..cd4803d1b 100644 --- a/sdk/core/azure-core/CMakeLists.txt +++ b/sdk/core/azure-core/CMakeLists.txt @@ -21,6 +21,7 @@ add_library ( src/credentials/policy/policies.cpp src/http/body_stream.cpp src/http/curl/curl.cpp + src/http/http.cpp src/http/logging_policy.cpp src/http/policy.cpp src/http/request.cpp diff --git a/sdk/core/azure-core/inc/azure/core/http/curl/curl.hpp b/sdk/core/azure-core/inc/azure/core/http/curl/curl.hpp index e2bb15d22..1384f0d46 100644 --- a/sdk/core/azure-core/inc/azure/core/http/curl/curl.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/curl/curl.hpp @@ -116,7 +116,7 @@ namespace Azure { namespace Core { namespace Http { /** * @brief Keeps an unique key for each host and creates a connection pool for each key. * - * @detail This way getting a connection for an specific host can be done in O(1) instead of + * @detail This way getting a connection for a specific host can be done in O(1) instead of * looping a single connection list to find the first connection for the required host. * * @remark There might be multiple connections for each host. @@ -418,7 +418,7 @@ namespace Azure { namespace Core { namespace Http { bool isUploadRequest(); /** - * @brief Set up libcurl handle to behave as an specific HTTP Method. + * @brief Set up libcurl handle to behave as a specific HTTP Method. * * @return returns the libcurl result after setting up. */ diff --git a/sdk/core/azure-core/inc/azure/core/http/http.hpp b/sdk/core/azure-core/inc/azure/core/http/http.hpp index e32bbfb5a..33622c1f4 100644 --- a/sdk/core/azure-core/inc/azure/core/http/http.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/http.hpp @@ -40,6 +40,21 @@ namespace Azure { namespace Core { namespace Http { left.insert(right.begin(), right.end()); return left; } + + /** + * @brief Insert a header into \p headers checking that \p headerName does not contain invalid + * characters. + * + * @param headers The headers map where to insert header. + * @param headerName The header name for the header to be inserted. + * @param headerValue The header value for the header to be inserted. + * + * @throw if \p headerName is invalid. + */ + void InsertHeaderWithValidation( + std::map& headers, + std::string const& headerName, + std::string const& headerValue); } // namespace Details /** @@ -309,10 +324,10 @@ namespace Azure { namespace Core { namespace Http { * @brief Append an HTTP query parameter. * * @note Overrides previous query parameters. - * @remark A query key can't contain any characters that need to be URL-encoded (per RFC). + * @remark A query key can't contain any characters that need to be URL-encoded (per RFC 7230). * * @param key HTTP query parameter. - * @param value HTTP quary parameter value. + * @param value HTTP query parameter value. * @param isValueEncoded `true` if \p value is URL-encoded, `false` otherwise. */ void AppendQuery(const std::string& key, const std::string& value, bool isValueEncoded = false) @@ -360,7 +375,7 @@ namespace Azure { namespace Core { namespace Http { const std::string& GetPath() const { return m_encodedPath; } /** - * @brief Get all the query paramters in the URL. + * @brief Get all the query parameters in the URL. * * @note Retry parameters have preference and will override any value from the initial query * parameters. @@ -418,7 +433,7 @@ namespace Azure { namespace Core { namespace Http { public: /** - * @brief Construct an HTTP request. + * @brief Construct an HTTP @request. * * @param httpMethod HTTP method. * @param url URL. @@ -432,7 +447,7 @@ namespace Azure { namespace Core { namespace Http { } /** - * @brief Construct an HTTP request. + * @brief Construct an HTTP @request. * * @param httpMethod HTTP method. * @param url URL. @@ -444,13 +459,12 @@ namespace Azure { namespace Core { namespace Http { } /** - * @brief Construct an HTTP request. + * @brief Construct an HTTP @request. * * @param httpMethod HTTP method. * @param url URL. * @param downloadViaStream */ - // Typically used for GET with no request body that can return bodyStream explicit Request(HttpMethod httpMethod, Url url, bool downloadViaStream) : Request( httpMethod, @@ -461,22 +475,23 @@ namespace Azure { namespace Core { namespace Http { } /** - * @brief Construct an HTTP request. + * @brief Construct an HTTP @request. * * @param httpMethod HTTP method. * @param url URL. */ - // Typically used for GET with no request body. explicit Request(HttpMethod httpMethod, Url url) : Request(httpMethod, std::move(url), NullBodyStream::GetNullBodyStream(), false) { } /** - * @brief Add an HTTP header. + * @brief Add HTTP header to the @Request. * - * @param name HTTP header name. - * @param value HTTP header value. + * @param name The name for the header to be added. + * @param value The value for the header to be added. + * + * @throw if \p name is an invalid header key. */ void AddHeader(std::string const& name, std::string const& value); @@ -547,7 +562,7 @@ namespace Azure { namespace Core { namespace Http { explicit CouldNotResolveHostException(std::string const& msg) : std::runtime_error(msg) {} }; - // Any other exception from transport layer without an specific exception defined above + // Any other exception from transport layer without a specific exception defined above /** * @brief HTTP transport layer error. */ @@ -556,6 +571,15 @@ namespace Azure { namespace Core { namespace Http { explicit TransportException(std::string const& msg) : std::runtime_error(msg) {} }; + /** + * @brief An invalid header key name in @Request or @RawResponse + * + */ + struct InvalidHeaderException : public std::runtime_error + { + explicit InvalidHeaderException(std::string const& msg) : std::runtime_error(msg) {} + }; + /** * @brief Raw HTTP response. */ @@ -603,27 +627,42 @@ namespace Azure { namespace Core { namespace Http { // ===== Methods used to build HTTP response ===== /** - * @brief Add header to the HTTP response. + * @brief Add HTTP header to the @RawResponse. * - * @param name HTTP response header name. - * @param value HTTP response header value. + * @remark The \p name must contain valid header name characters (RFC 7230). + * + * @param name The name for the header to be added. + * @param value The value for the header to be added. + * + * @throw if \p name contains invalid characters. */ void AddHeader(std::string const& name, std::string const& value); /** - * @brief Add header to the HTTP response. + * @brief Add HTTP header to the @RawResponse. * - * @param header HTTP response header in RFCnnnn format (`OWS header-value OWS`). + * @remark The \p header must contain valid header name characters (RFC 7230). + * @remark Header name, value and delimiter are expected to be in \p header. + * + * @param header The complete header to be added, in the form "name:value". + * + * @throw if \p header has an invalid header name or if the delimiter is missing. */ - // rfc form header-name: OWS header-value OWS void AddHeader(std::string const& header); /** - * @brief Add header to the HTTP response. - * @detail HTTP response header should be in RFCnnnn format (`OWS header-value OWS`). + * @brief Add HTTP header to the @RawResponse. * - * @param begin Pointer to the first byte of the header string in RFCnnnn format. - * @param last Pointer to the last byte of the header string in RFCnnnn format. + * @remark The string referenced by \p begin and \p end must contain valid header name + * characters (RFC 7230). + * @remark Header name, value and delimiter are expected to be in the string referenced by \p + * begin and \p end, in the form "name:value". + * + * @param begin Reference to the start of an std::string. + * @param last Reference to the end of an std::string. + * + * @throw if the string referenced by \p begin and \p end contains an invalid header name or if + * the delimiter is missing. */ void AddHeader(uint8_t const* const begin, uint8_t const* const last); @@ -674,7 +713,7 @@ namespace Azure { namespace Core { namespace Http { */ std::unique_ptr GetBodyStream() { - // If m_bodyStream was moved before. nullpr is returned + // If m_bodyStream was moved before. nullptr is returned return std::move(this->m_bodyStream); } diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index 058f53dd9..bf302efce 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -79,7 +79,7 @@ int pollSocketUntilEventOrTimeout( return poll(&poller, 1, timeout); #endif #ifdef WINDOWS - // Windows Vista and greater. TODO: detect legacy Windows and use select() + // Windows Vista and greater. return WSAPoll(&poller, 1, timeout); #endif } @@ -757,7 +757,7 @@ int64_t CurlSession::ResponseBufferParser::Parse( } else if (this->state == ResponseParserState::Headers) { - // Add header. TODO: Do toLower so all headers are lowerCase + // will throw if header is invalid this->m_response->AddHeader(this->m_internalBuffer); this->m_delimiterStartInPrevPosition = false; start = index + 1; // jump \n @@ -794,7 +794,7 @@ int64_t CurlSession::ResponseBufferParser::Parse( return index + 1; // plus 1 to advance the \n. If we were at buffer end. } - // Add header. TODO: Do toLower so all headers are lowerCase + // will throw if header is invalid this->m_response->AddHeader(buffer + start, buffer + index - 1); this->m_delimiterStartInPrevPosition = false; start = index + 1; // jump \n @@ -940,12 +940,15 @@ int64_t CurlSession::ResponseBufferParser::BuildHeader( // Append and build response minus the delimiter this->m_internalBuffer.append(start, indexOfEndOfStatusLine); } - this->m_response->AddHeader(this->m_internalBuffer); + // will throw if header is invalid + m_response->AddHeader(this->m_internalBuffer); } else { // Internal Buffer was not required, create response directly from buffer - this->m_response->AddHeader(std::string(start, indexOfEndOfStatusLine)); + std::string header(std::string(start, indexOfEndOfStatusLine)); + // will throw if header is invalid + this->m_response->AddHeader(header); } // reuse buffer diff --git a/sdk/core/azure-core/src/http/http.cpp b/sdk/core/azure-core/src/http/http.cpp new file mode 100644 index 000000000..7da382dd4 --- /dev/null +++ b/sdk/core/azure-core/src/http/http.cpp @@ -0,0 +1,155 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +#include "azure/core/http/http.hpp" + +void Azure::Core::Http::Details::InsertHeaderWithValidation( + std::map& headers, + std::string const& headerName, + std::string const& headerValue) +{ + // Static table for validating header names. It is created just once for the program and reused + // each time AddHeader is called + static const uint8_t validChars[256] = { + 0, /* 0 - null */ + 0, /* 1 - start of heading */ + 0, /* 2 - start of text */ + 0, /* 3 - end of text */ + 0, /* 4 - end of transmission */ + 0, /* 5 - enquiry */ + 0, /* 6 - acknowledge */ + 0, /* 7 - bell */ + 0, /* 8 - backspace */ + 0, /* 9 - horizontal tab */ + 0, /* 10 - new line */ + 0, /* 11 - vertical tab */ + 0, /* 12 - new page */ + 0, /* 13 - carriage return */ + 0, /* 14 - shift out */ + 0, /* 15 - shift in */ + 0, /* 16 - data link escape */ + 0, /* 17 - device control 1 */ + 0, /* 18 - device control 2 */ + 0, /* 19 - device control 3 */ + 0, /* 20 - device control 4 */ + 0, /* 21 - negative acknowledge */ + 0, /* 22 - synchronous idle */ + 0, /* 23 - end of trans. block */ + 0, /* 24 - cancel */ + 0, /* 25 - end of medium */ + 0, /* 26 - substitute */ + 0, /* 27 - escape */ + 0, /* 28 - file separator */ + 0, /* 29 - group separator */ + 0, /* 30 - record separator */ + 0, /* 31 - unit separator */ + ' ', /* 32 - space */ + '!', /* 33 - ! */ + 0, /* 34 - " */ + '#', /* 35 - # */ + '$', /* 36 - $ */ + '%', /* 37 - % */ + '&', /* 38 - & */ + '\'', /* 39 - ' */ + 0, /* 40 - ( */ + 0, /* 41 - ) */ + '*', /* 42 - * */ + '+', /* 43 - + */ + 0, /* 44 - , */ + '-', /* 45 - - */ + '.', /* 46 - . */ + 0, /* 47 - / */ + '0', /* 48 - 0 */ + '1', /* 49 - 1 */ + '2', /* 50 - 2 */ + '3', /* 51 - 3 */ + '4', /* 52 - 4 */ + '5', /* 53 - 5 */ + '6', /* 54 - 6 */ + '7', /* 55 - 7 */ + '8', /* 56 - 8 */ + '9', /* 57 - 9 */ + 0, /* 58 - : */ + 0, /* 59 - ; */ + 0, /* 60 - < */ + 0, /* 61 - = */ + 0, /* 62 - > */ + 0, /* 63 - ? */ + 0, /* 64 - @ */ + 'a', /* 65 - A */ + 'b', /* 66 - B */ + 'c', /* 67 - C */ + 'd', /* 68 - D */ + 'e', /* 69 - E */ + 'f', /* 70 - F */ + 'g', /* 71 - G */ + 'h', /* 72 - H */ + 'i', /* 73 - I */ + 'j', /* 74 - J */ + 'k', /* 75 - K */ + 'l', /* 76 - L */ + 'm', /* 77 - M */ + 'n', /* 78 - N */ + 'o', /* 79 - O */ + 'p', /* 80 - P */ + 'q', /* 81 - Q */ + 'r', /* 82 - R */ + 's', /* 83 - S */ + 't', /* 84 - T */ + 'u', /* 85 - U */ + 'v', /* 86 - V */ + 'w', /* 87 - W */ + 'x', /* 88 - X */ + 'y', /* 89 - Y */ + 'z', /* 90 - Z */ + 0, /* 91 - [ */ + 0, /* 92 - comment */ + 0, /* 93 - ] */ + '^', /* 94 - ^ */ + '_', /* 95 - _ */ + '`', /* 96 - ` */ + 'a', /* 97 - a */ + 'b', /* 98 - b */ + 'c', /* 99 - c */ + 'd', /* 100 - d */ + 'e', /* 101 - e */ + 'f', /* 102 - f */ + 'g', /* 103 - g */ + 'h', /* 104 - h */ + 'i', /* 105 - i */ + 'j', /* 106 - j */ + 'k', /* 107 - k */ + 'l', /* 108 - l */ + 'm', /* 109 - m */ + 'n', /* 110 - n */ + 'o', /* 111 - o */ + 'p', /* 112 - p */ + 'q', /* 113 - q */ + 'r', /* 114 - r */ + 's', /* 115 - s */ + 't', /* 116 - t */ + 'u', /* 117 - u */ + 'v', /* 118 - v */ + 'w', /* 119 - w */ + 'x', /* 120 - x */ + 'y', /* 121 - y */ + 'z', /* 122 - z */ + 0, /* 123 - { */ + '|', /* 124 - | */ + 0, /* 125 - } */ + '~', /* 126 - ~ */ + 0 /* 127 - DEL */ + // ...128-255 is all zeros (not valid) characters} + }; + + // Check all chars in name are valid + for (size_t index = 0; index < headerName.size(); index++) + { + if (validChars[static_cast(headerName[index])] == 0) + { + throw InvalidHeaderException("Invalid header: " + headerName); + } + } + // insert (override if duplicated) + headers[headerName] = headerValue; +} diff --git a/sdk/core/azure-core/src/http/raw_response.cpp b/sdk/core/azure-core/src/http/raw_response.cpp index 885632da6..33c3b4d7c 100644 --- a/sdk/core/azure-core/src/http/raw_response.cpp +++ b/sdk/core/azure-core/src/http/raw_response.cpp @@ -15,7 +15,10 @@ HttpStatusCode RawResponse::GetStatusCode() const { return m_statusCode; } std::string const& RawResponse::GetReasonPhrase() const { return m_reasonPhrase; } -std::map const& RawResponse::GetHeaders() const { return this->m_headers; } +std::map const& RawResponse::GetHeaders() const +{ + return this->m_headers; +} void RawResponse::AddHeader(uint8_t const* const begin, uint8_t const* const last) { @@ -25,7 +28,7 @@ void RawResponse::AddHeader(uint8_t const* const begin, uint8_t const* const las if (end == last) { - return; // not a valid header or end of headers symbol reached + throw InvalidHeaderException("invalid header. No delimiter :"); } // Always toLower() headers @@ -51,8 +54,7 @@ void RawResponse::AddHeader(std::string const& header) void RawResponse::AddHeader(std::string const& name, std::string const& value) { - - this->m_headers.insert(std::pair(name, value)); + return Details::InsertHeaderWithValidation(this->m_headers, name, value); } void RawResponse::SetBodyStream(std::unique_ptr stream) diff --git a/sdk/core/azure-core/src/http/request.cpp b/sdk/core/azure-core/src/http/request.cpp index c5f973842..b4261bbe1 100644 --- a/sdk/core/azure-core/src/http/request.cpp +++ b/sdk/core/azure-core/src/http/request.cpp @@ -13,15 +13,11 @@ using namespace Azure::Core::Http; void Request::AddHeader(std::string const& name, std::string const& value) { auto headerNameLowerCase = Azure::Core::Details::ToLower(name); - if (this->m_retryModeEnabled) - { - // When retry mode is ON, any new value must override previous - this->m_retryHeaders[headerNameLowerCase] = value; - } - else - { - this->m_headers[headerNameLowerCase] = value; - } + return this->m_retryModeEnabled + ? Details::InsertHeaderWithValidation( + this->m_retryHeaders, headerNameLowerCase, value) + : Details::InsertHeaderWithValidation( + this->m_headers, headerNameLowerCase, value); } void Request::RemoveHeader(std::string const& name) @@ -53,12 +49,11 @@ std::map Request::GetHeaders() const return Details::MergeMaps(this->m_retryHeaders, this->m_headers); } -// Writes an HTTP request with RFC2730 without the body (head line and headers) +// Writes an HTTP request with RFC 7230 without the body (head line and headers) // https://tools.ietf.org/html/rfc7230#section-3.1.1 std::string Request::GetHTTPMessagePreBody() const { std::string httpRequest(HttpMethodToString(this->m_method)); - // origin-form. TODO: parse Url to split host from path and use it here instead of empty // HTTP version harcoded to 1.0 auto const url = this->m_url.GetRelativeUrl(); httpRequest += " /" + url + " HTTP/1.1\r\n"; diff --git a/sdk/core/azure-core/test/ut/CMakeLists.txt b/sdk/core/azure-core/test/ut/CMakeLists.txt index 463ebefb7..93e4a2871 100644 --- a/sdk/core/azure-core/test/ut/CMakeLists.txt +++ b/sdk/core/azure-core/test/ut/CMakeLists.txt @@ -19,6 +19,7 @@ project (${TARGET_NAME} LANGUAGES CXX) set(CMAKE_CXX_STANDARD 14) set(CMAKE_CXX_STANDARD_REQUIRED True) +include(GoogleTest) add_executable ( ${TARGET_NAME} http.cpp @@ -34,6 +35,9 @@ add_executable ( uuid.cpp context.cpp) -target_link_libraries(${TARGET_NAME} PRIVATE azure-core) -add_gtest(${TARGET_NAME}) +target_link_libraries(${TARGET_NAME} PRIVATE azure-core gtest) + +# gtest_add_tests will scan the test from azure-core-test and call add_test +# for each test to ctest. This enables `ctest -r` to run specific tests directly. +gtest_add_tests(TARGET ${TARGET_NAME}) diff --git a/sdk/core/azure-core/test/ut/http.cpp b/sdk/core/azure-core/test/ut/http.cpp index 88a214c90..11edf4138 100644 --- a/sdk/core/azure-core/test/ut/http.cpp +++ b/sdk/core/azure-core/test/ut/http.cpp @@ -3,8 +3,8 @@ #include "gtest/gtest.h" -#include #include "http.hpp" +#include #include #include @@ -158,4 +158,121 @@ namespace Azure { namespace Core { namespace Test { req.GetUrl().GetAbsoluteUrl(), "http://test.com/path/path2/path3?query=value"); } + + // Request - Add header + TEST(TestHttp, add_headers) + { + Http::HttpMethod httpMethod = Http::HttpMethod::Post; + Http::Url url("http://test.com"); + Http::Request req(httpMethod, url); + std::pair expected("valid", "header"); + + EXPECT_NO_THROW(req.AddHeader(expected.first, expected.second)); + EXPECT_PRED2( + [](std::map headers, + std::pair expected) { + auto firstHeader = headers.begin(); + return firstHeader->first == expected.first && firstHeader->second == expected.second + && headers.size() == 1; + }, + req.GetHeaders(), + expected); + + EXPECT_THROW(req.AddHeader("invalid()", "header"), std::runtime_error); + + // same header will just override + std::pair expectedOverride("valid", "override"); + EXPECT_NO_THROW(req.AddHeader(expectedOverride.first, expectedOverride.second)); + EXPECT_PRED2( + [](std::map headers, + std::pair expected) { + auto firstHeader = headers.begin(); + return firstHeader->first == expected.first && firstHeader->second == expected.second + && headers.size() == 1; + }, + req.GetHeaders(), + expectedOverride); + + // adding header after one error happened before + std::pair expected2("valid2", "header2"); + EXPECT_NO_THROW(req.AddHeader(expected2.first, expected2.second)); + EXPECT_PRED2( + [](std::map headers, + std::pair expected) { + auto secondHeader = headers.begin(); + secondHeader++; + return secondHeader->first == expected.first && secondHeader->second == expected.second + && headers.size() == 2; + }, + req.GetHeaders(), + expected2); + } + + // Response - Add header + TEST(TestHttp, response_add_headers) + { + + Http::RawResponse response(1, 1, Http::HttpStatusCode::Accepted, "Test"); + std::pair expected("valid", "header"); + + EXPECT_NO_THROW(response.AddHeader(expected.first, expected.second)); + EXPECT_PRED2( + [](std::map headers, + std::pair expected) { + auto firstHeader = headers.begin(); + return firstHeader->first == expected.first && firstHeader->second == expected.second + && headers.size() == 1; + }, + response.GetHeaders(), + expected); + + EXPECT_THROW( + response.AddHeader("invalid()", "header"), Azure::Core::Http::InvalidHeaderException); + + // same header will just override + std::pair expectedOverride("valid", "override"); + EXPECT_NO_THROW(response.AddHeader(expectedOverride.first, expectedOverride.second)); + EXPECT_PRED2( + [](std::map headers, + std::pair expected) { + auto firstHeader = headers.begin(); + return firstHeader->first == expected.first && firstHeader->second == expected.second + && headers.size() == 1; + }, + response.GetHeaders(), + expectedOverride); + + // adding header after on error happened + std::pair expected2("valid2", "header2"); + EXPECT_NO_THROW(response.AddHeader(expected2.first, expected2.second)); + EXPECT_PRED2( + [](std::map headers, + std::pair expected) { + auto secondtHeader = headers.begin(); + secondtHeader++; + return secondtHeader->first == expected.first && secondtHeader->second == expected.second + && headers.size() == 2; + }, + response.GetHeaders(), + expected2); + + // Response addHeader overload method to add from string + EXPECT_THROW(response.AddHeader("inv(): header"), Azure::Core::Http::InvalidHeaderException); + EXPECT_THROW( + response.AddHeader("no delimiter header"), Azure::Core::Http::InvalidHeaderException); + + // adding header after previous error just happened on add from string + EXPECT_NO_THROW(response.AddHeader("valid3: header3")); + EXPECT_PRED2( + [](std::map headers, + std::pair expected) { + auto secondtHeader = headers.begin(); + secondtHeader++; + secondtHeader++; + return secondtHeader->first == expected.first && secondtHeader->second == expected.second + && headers.size() == 3; + }, + response.GetHeaders(), + (std::pair("valid3", "header3"))); + } }}} // namespace Azure::Core::Test diff --git a/sdk/storage/azure-storage-common/inc/azure/storage/common/reliable_stream.hpp b/sdk/storage/azure-storage-common/inc/azure/storage/common/reliable_stream.hpp index 8afc1054d..d644f8ed9 100644 --- a/sdk/storage/azure-storage-common/inc/azure/storage/common/reliable_stream.hpp +++ b/sdk/storage/azure-storage-common/inc/azure/storage/common/reliable_stream.hpp @@ -15,7 +15,7 @@ namespace Azure { namespace Storage { int64_t Offset = 0; }; - // Defines a fn signature to be use to get a bodyStream from an specific offset. + // Defines a fn signature to be use to get a bodyStream from a specific offset. typedef std::function( Azure::Core::Context const&, HttpGetterInfo const&)>