Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97b4b82acd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (strcmp(protocol, "http") != 0) { | ||
| flb_error("only HTTP proxy is supported."); | ||
| /* Only HTTP(s) proxy is supported for now. */ | ||
| if (strcmp(protocol, "http") != 0 && strcmp(protocol, "https") != 0) { |
There was a problem hiding this comment.
Reject
https proxy scheme until transport handles it
flb_utils_proxy_url_split now accepts https URLs, but the parsed protocol is not used downstream: flb_upstream_create frees proxy_protocol and still builds a normal TCP upstream (src/flb_upstream.c, proxy setup block), then flb_io_net_connect sends CONNECT over that plain socket before any TLS handshake (src/flb_io.c and flb_http_client_proxy_connect). For real HTTPS proxies (TLS to proxy), this path fails because the proxy expects TLS first, so this commit advertises support that does not actually work for the new accepted input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/flb_utils.c (1)
1873-1873: Use next-line opening brace forifin C files.This changed
ifuses same-line{; the repo rule asks for next-line opening braces.As per coding guidelines,
**/*.{c,cpp}: Always use braces forif/else/while/doblocks, with opening braces on the next line.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_utils.c` at line 1873, The if statement checking the protocol (if (strcmp(protocol, "http") != 0 && strcmp(protocol, "https") != 0)) uses a same-line opening brace; change it to use the repo's next-line brace style by moving the opening '{' to the line after the if condition (preserve the condition and surrounding whitespace and keep the existing body unchanged) so the block follows the required next-line opening-brace convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flb_utils.c`:
- Around line 1872-1874: The proxy URL parsing currently accepts "https" but
still defaults an absent port to "80"; update the proxy-port fallback logic so
that when the parsed protocol variable equals "https" (strcmp(protocol, "https")
== 0) the default port is set to "443" and when it equals "http" it remains
"80"; locate the code paths that assign the literal "80" (the port fallback
branches where port is set when missing) and change them to choose "443" for
protocol == "https" and "80" otherwise, and ensure any log messages (e.g.,
flb_error usage around unsupported protocols) and comments reflect HTTPS default
port 443.
---
Nitpick comments:
In `@src/flb_utils.c`:
- Line 1873: The if statement checking the protocol (if (strcmp(protocol,
"http") != 0 && strcmp(protocol, "https") != 0)) uses a same-line opening brace;
change it to use the repo's next-line brace style by moving the opening '{' to
the line after the if condition (preserve the condition and surrounding
whitespace and keep the existing body unchanged) so the block follows the
required next-line opening-brace convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| /* Only HTTP(s) proxy is supported for now. */ | ||
| if (strcmp(protocol, "http") != 0 && strcmp(protocol, "https") != 0) { | ||
| flb_error("only HTTP(s) proxy is supported."); |
There was a problem hiding this comment.
HTTPS support is incomplete when proxy port is omitted.
After allowing https:// at Lines 1872-1874, the no-port paths still default to "80" (Lines 1952 and 1991).
https://proxy.example.com should default to 443, otherwise this path will fail or connect incorrectly.
🔧 Proposed fix
@@
- else if (*(end + 1) == '\0') {
- port = flb_strdup("80");
+ else if (*(end + 1) == '\0') {
+ if (strcmp(protocol, "https") == 0) {
+ port = flb_strdup("443");
+ }
+ else {
+ port = flb_strdup("80");
+ }
if (!port) {
flb_errno();
goto error;
}
}
@@
- port = flb_strdup("80");
+ if (strcmp(protocol, "https") == 0) {
+ port = flb_strdup("443");
+ }
+ else {
+ port = flb_strdup("80");
+ }
if (!port) {
flb_errno();
goto error;
}
}Also applies to: 1951-1953, 1991-1993
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/flb_utils.c` around lines 1872 - 1874, The proxy URL parsing currently
accepts "https" but still defaults an absent port to "80"; update the proxy-port
fallback logic so that when the parsed protocol variable equals "https"
(strcmp(protocol, "https") == 0) the default port is set to "443" and when it
equals "http" it remains "80"; locate the code paths that assign the literal
"80" (the port fallback branches where port is set when missing) and change them
to choose "443" for protocol == "https" and "80" otherwise, and ensure any log
messages (e.g., flb_error usage around unsupported protocols) and comments
reflect HTTPS default port 443.
Summary by CodeRabbit
Bug Fixes