1.1.3 #17

playmiel · 2025-12-09T09:23:43Z

security and correction update

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2025-12-09T09:29:04Z

src/AsyncHttpClient.cpp

        return false;
    if (!context || !context->request)
        return true;
-    if (context->request->getNoStoreBody() && _bodyChunkCallback)
-        return false;
    return true;


Keep body guard from aborting zero-copy streaming

The new 8 KiB default body limit is now enforced unconditionally: shouldEnforceBodyLimit always returns true whenever _maxBodySize is non‑zero. With _maxBodySize now defaulting to 8192, a request configured for streaming (setNoStoreBody(true) with a global onBodyChunk callback) will hit MAX_BODY_SIZE_EXCEEDED once the streamed response exceeds ~8 KiB even though nothing is buffered, breaking the zero‑copy streaming path. The limit should still be bypassed for no‑store streaming or the default should be raised/disabled for that path.

Useful? React with 👍 / 👎.

Copilot

Pull request overview

This PR introduces security hardening and bug fixes for version 1.1.3, adding input validation, resource limits, and improved security controls across URL parsing, TLS fingerprint validation, and cookie handling.

Key Changes:

Added comprehensive URL validation with length limits, character sanitization, and strict host/port parsing
Implemented TLS fingerprint format validation to catch invalid configurations early
Enhanced cookie security with size limits, public suffix checking, and improved cross-origin header filtering

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File	Description
src/UrlParser.cpp	Adds security validation functions for URLs including length limits (2048 bytes), character validation, host validation (max 255 chars), and robust port parsing with overflow protection
src/AsyncTransport.cpp	Implements early validation of TLS fingerprint format, detecting invalid hex strings before connection attempts
src/AsyncHttpClient.cpp	Introduces cookie jar limits (16 cookies, 4KB max), public suffix list for cookie domain validation, expanded cross-origin sensitive header filtering, and default body/header size limits
README.md	Updates documentation to reflect new default limits for headers (~2.8 KiB) and body (8 KiB), adds keep-alive documentation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-09T09:30:25Z

src/AsyncHttpClient.cpp

    if (_maxBodySize == 0)
        return false;
    if (!context || !context->request)
        return true;


The previous logic that exempted requests with setNoStoreBody(true) and a _bodyChunkCallback from body size limits has been removed. This means that even streaming requests that don't buffer the body in memory will now be subject to the body size limit, which may not be the intended behavior.

If the purpose of the body size limit is to prevent excessive memory usage, then streaming requests that use callbacks shouldn't be limited. Consider restoring this check or clarifying the intent of this change.

Suggested change

return true;

return true;

// Exempt requests that do not buffer the body in memory (streaming)

if (context->request->noStoreBody() || context->request->hasBodyStream())

return false;

Copilot · 2025-12-09T09:30:25Z

src/AsyncHttpClient.cpp

+        if (_cookies.size() >= kMaxCookieCount)
+            _cookies.erase(_cookies.begin());


[nitpick] When the cookie jar reaches kMaxCookieCount, the oldest cookie is evicted using FIFO (first-in, first-out) by erasing from the beginning. However, this could evict a long-lived, important cookie in favor of a newly-set cookie.

Consider implementing a more sophisticated eviction strategy, such as:

LRU (Least Recently Used) - evict cookies that haven't been used recently

Prefer evicting session cookies over persistent cookies

Sort by domain specificity and evict broader-scoped cookies first

Copilot · 2025-12-09T09:30:26Z

src/UrlParser.cpp

+static bool isValidHostChar(char c) {
+    return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-' || c == '.';
+}


The host validation function isValidHostChar does not support IPv6 addresses. IPv6 addresses in URLs are enclosed in square brackets (e.g., http://[2001:db8::1]:8080/path) and contain colons, which are not included in the valid character set. This will cause valid IPv6 URLs to be rejected.

Consider adding support for IPv6 addresses by allowing square brackets and handling the IPv6 format appropriately, or at minimum, allow colons within bracketed hosts.

Copilot · 2025-12-09T09:30:26Z

src/UrlParser.cpp

+    if (end == portStr.c_str() || *end != '\0')
+        return false;


The port parsing logic checks each character is a digit (lines 47-50) and then calls std::strtoul to parse the value (line 53). However, the check on line 54 (if (end == portStr.c_str() || *end != '\0')) is redundant because the prior loop already validated that all characters are digits, so strtoul will always consume the entire string. This condition can never be true.

Consider removing the redundant check on line 54.

Suggested change

if (end == portStr.c_str() || *end != '\0')

return false;

Copilot · 2025-12-09T09:30:26Z

src/AsyncHttpClient.cpp

+static const char* kPublicSuffixes[] = {"com",
+                                        "net",
+                                        "org",
+                                        "gov",
+                                        "edu",
+                                        "mil",
+                                        "int",
+                                        "co.uk",
+                                        "ac.uk",
+                                        "gov.uk",
+                                        "uk",
+                                        "io",
+                                        "co",
+                                        "app",
+                                        "dev",
+                                        "github.io",
+                                        "web.app",
+                                        "pages.dev",
+                                        "vercel.app",
+                                        "firebaseapp.com",
+                                        "cloudfront.net"};


The public suffix list is incomplete and only contains a small subset of actual public suffixes. This could allow cookies to be set on public suffixes not in this list (e.g., "ca", "de", "fr", "com.au", "co.jp", etc.), which is a security concern as it could lead to session fixation or cookie tossing attacks.

Consider either:

Using a more comprehensive public suffix list

Adding a comment noting this is a minimal list for resource-constrained environments

Implementing a more conservative heuristic (e.g., rejecting single-label domains or requiring at least 2 labels)

Copilot · 2025-12-09T09:30:26Z

src/UrlParser.cpp

+static bool hasInvalidUrlChar(const std::string& url) {
+    for (char c : url) {
+        unsigned char uc = static_cast<unsigned char>(c);
+        if (uc <= 0x1F || uc == 0x7F || c == '\r' || c == '\n' || c == ' ' || c == '\t')
+            return true;
+    }
+    return false;
+}
+
+static bool isValidHostChar(char c) {
+    return (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '-' || c == '.';
+}
+
+static bool isValidHost(const std::string& host) {
+    if (host.empty() || host.size() > kMaxHostLength)
+        return false;
+    if (host.front() == '.' || host.back() == '.')
+        return false;
+    for (char c : host) {
+        if (!isValidHostChar(c))
+            return false;
+    }
+    return true;
+}
+
+static bool parsePort(const std::string& portStr, uint16_t* out) {
+    if (!out || portStr.empty())
+        return false;
+    for (char c : portStr) {
+        if (c < '0' || c > '9')
+            return false;
+    }
+    errno = 0;
+    char* end = nullptr;
+    unsigned long val = std::strtoul(portStr.c_str(), &end, 10);
+    if (end == portStr.c_str() || *end != '\0')
+        return false;
+    if (errno == ERANGE || val > 65535)
+        return false;
+    *out = static_cast<uint16_t>(val);
+    return true;
+}


The new URL validation functions (hasInvalidUrlChar, isValidHost, parsePort) lack test coverage. Given that these are security-critical functions that validate and sanitize user input, they should have comprehensive tests covering:

URLs with control characters and special whitespace

Hosts with invalid characters

Port numbers at boundaries (0, 65535, 65536, etc.)

Invalid port strings (negative numbers, non-numeric characters, overflow)

Edge cases like empty ports after colons

Consider adding test cases for these new validation functions.

Copilot · 2025-12-09T09:30:27Z

src/UrlParser.cpp

+        uint16_t parsedPort = 0;
+        if (!parsePort(portStr, &parsedPort))
+            return false;
+        out.port = parsedPort;


When a URL contains a colon but no port number (e.g., http://example.com:), the portStr will be empty, and parsePort will return false (line 46), causing the entire URL parse to fail. However, an empty port after a colon should arguably fall back to the default port rather than causing a complete parse failure.

Consider treating an empty port string as valid and using the default port already set in out.port.

Suggested change

uint16_t parsedPort = 0;

if (!parsePort(portStr, &parsedPort))

return false;

out.port = parsedPort;

if (!portStr.empty()) {

uint16_t parsedPort = 0;

if (!parsePort(portStr, &parsedPort))

return false;

out.port = parsedPort;

}

// If portStr is empty, keep out.port as default

playmiel added 2 commits December 2, 2025 12:15

security and doc update

08081ac

secure corrections to URL parsing and asynchronous

48b5420

playmiel self-assigned this Dec 9, 2025

Copilot AI review requested due to automatic review settings December 9, 2025 09:23

playmiel added the enhancement New feature or request label Dec 9, 2025

Copilot started reviewing on behalf of playmiel December 9, 2025 09:24 View session

chatgpt-codex-connector bot reviewed Dec 9, 2025

View reviewed changes

Copilot AI reviewed Dec 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

1.1.3 #17

1.1.3 #17

playmiel commented Dec 9, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Dec 9, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Copilot AI Dec 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if (_cookies.size() >= kMaxCookieCount)
		_cookies.erase(_cookies.begin());

1.1.3 #17

Are you sure you want to change the base?

1.1.3 #17

Conversation

playmiel commented Dec 9, 2025

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants