oauth: out_http: add user-agent option for oauth#11830
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional ChangesOAuth2 User-Agent Configuration Support
Sequence Diagram(s)sequenceDiagram
participant Test as test_user_agent_header_optional
participant Plugin as HTTP plugin (out_http)
participant OAuth2 as oauth2_http_request
participant Mock as oauth2_mock_server
Test->>Plugin: create context (with/without oauth2.user_agent)
Test->>OAuth2: initiate token request
OAuth2->>Mock: POST /token (with optional User-Agent header)
Mock->>OAuth2: respond with token
Test->>Mock: assert token_user_agent_seen / token_user_agent value
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/internal/oauth2.c (1)
157-190: ⚡ Quick winMake header extraction line-anchored and case-insensitive in the mock parser.
Current
strstr(request, header_name)can match unintended substrings and depends on exact casing. Parsing header lines explicitly will make the test less brittle.Proposed parser hardening diff
static int request_header_value(const char *request, const char *header_name, char *out, size_t out_size) { int header_len; + const char *line_start; + const char *line_end; const char *end; const char *start; size_t value_len; - start = strstr(request, header_name); + header_len = strlen(header_name); + line_start = request; + start = NULL; + + while (line_start && *line_start != '\0') { + line_end = strstr(line_start, "\r\n"); + if (!line_end) { + break; + } + + if ((size_t) (line_end - line_start) >= (size_t) header_len && + strncasecmp(line_start, header_name, header_len) == 0) { + start = line_start + header_len; + break; + } + + line_start = line_end + 2; + } + if (!start) { return -1; } - header_len = strlen(header_name); - start += header_len; while (*start == ' ') { start++; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/oauth2.c` around lines 157 - 190, The mock header parser request_header_value uses strstr which can match mid-line and is case-sensitive; change it to scan the request line-by-line and perform a case-insensitive, line-anchored match for the header name (i.e., accept header at start of string or immediately after "\r\n"), ensure you match header_name followed by ':' (allow optional spaces), then capture the value up to the next "\r\n"; use a case-insensitive compare like strncasecmp (or equivalent) against the header name length and only accept the header when the following character is ':' to avoid substring matches, then copy the trimmed value into out.plugins/out_http/http.c (1)
746-750: ⚡ Quick winDocument the new
oauth2.user_agentsetting in HTTP output docs.This is a user-facing config key; adding a docs entry/example in the HTTP output plugin docs will prevent discoverability gaps.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/out_http/http.c` around lines 746 - 750, Add a user-facing docs entry for the new oauth2.user_agent config key (backing field oauth2_config.user_agent in struct flb_out_http) to the HTTP output plugin configuration docs: describe it as an optional string that sets the User-Agent header for OAuth2 token requests (default: unset/null), include an example showing oauth2.user_agent = "MyAgent/1.0" within the plugin config, and place it alongside other OAuth2-related settings in the HTTP output plugin's config options and examples so users can discover and copy it easily.src/flb_oauth2.c (1)
1134-1140: ⚡ Quick winHarden
oauth2.user_agentagainst CR/LF before adding it as a header.If
oauth2.user_agentcontains\ror\n, the outgoing request can become malformed and potentially inject unintended headers. Reject or ignore such values beforeflb_http_add_header.Proposed hardening diff
- if (ctx->cfg.user_agent) { - flb_http_add_header(c, - "User-Agent", - 10, - ctx->cfg.user_agent, - flb_sds_len(ctx->cfg.user_agent)); - } + if (ctx->cfg.user_agent) { + if (strpbrk(ctx->cfg.user_agent, "\r\n") != NULL) { + flb_warn("[oauth2] ignoring oauth2.user_agent: contains CR/LF"); + } + else { + flb_http_add_header(c, + "User-Agent", + 10, + ctx->cfg.user_agent, + flb_sds_len(ctx->cfg.user_agent)); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flb_oauth2.c` around lines 1134 - 1140, Check ctx->cfg.user_agent for CR or LF characters before calling flb_http_add_header and skip adding the header (or clear the value) if any '\r' or '\n' is found; update the code around the existing flb_http_add_header call that uses ctx->cfg.user_agent to perform this validation (e.g., inspect flb_sds or the string bytes) and only call flb_http_add_header when the user_agent is free of CR/LF.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/out_http/http.c`:
- Around line 746-750: Add a user-facing docs entry for the new
oauth2.user_agent config key (backing field oauth2_config.user_agent in struct
flb_out_http) to the HTTP output plugin configuration docs: describe it as an
optional string that sets the User-Agent header for OAuth2 token requests
(default: unset/null), include an example showing oauth2.user_agent =
"MyAgent/1.0" within the plugin config, and place it alongside other
OAuth2-related settings in the HTTP output plugin's config options and examples
so users can discover and copy it easily.
In `@src/flb_oauth2.c`:
- Around line 1134-1140: Check ctx->cfg.user_agent for CR or LF characters
before calling flb_http_add_header and skip adding the header (or clear the
value) if any '\r' or '\n' is found; update the code around the existing
flb_http_add_header call that uses ctx->cfg.user_agent to perform this
validation (e.g., inspect flb_sds or the string bytes) and only call
flb_http_add_header when the user_agent is free of CR/LF.
In `@tests/internal/oauth2.c`:
- Around line 157-190: The mock header parser request_header_value uses strstr
which can match mid-line and is case-sensitive; change it to scan the request
line-by-line and perform a case-insensitive, line-anchored match for the header
name (i.e., accept header at start of string or immediately after "\r\n"),
ensure you match header_name followed by ':' (allow optional spaces), then
capture the value up to the next "\r\n"; use a case-insensitive compare like
strncasecmp (or equivalent) against the header name length and only accept the
header when the following character is ':' to avoid substring matches, then copy
the trimmed value into out.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2d428ca-531a-4f6d-aef6-0e4d1640cd4e
📒 Files selected for processing (4)
include/fluent-bit/flb_oauth2.hplugins/out_http/http.csrc/flb_oauth2.ctests/internal/oauth2.c
|
Added fluent-bit-docs pull request: fluent/fluent-bit-docs#2578 |
|
Example conifig section: |
|
valgrind output |
rja5
force-pushed
the
add-oauth-user-agent
branch
from
ab7d19f to
713d438
Compare
cosmo0920
changed the title
|
We need to split from the one commit into two or more commit to follow our commit linter: This should be complained that your only one commit is not followed our commit guideline. |
rja5
force-pushed
the
add-oauth-user-agent
branch
3 times, most recently
from
8d3e7cd to
cd3f664
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/internal/oauth2.c (1)
515-541:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMove the opening brace to the next line.
Per the coding guidelines, function opening braces should be on the next line after the signature.
📐 Proposed fix
static struct flb_oauth2 *create_oauth_ctx_with_user_agent(struct flb_config *config, struct oauth2_mock_server *server, int refresh_skew, - const char *user_agent) -{ + const char *user_agent) +{ struct flb_oauth2_config cfg;As per coding guidelines: "Put function opening braces on the next line, formatted as:
int fn(void)\n{ ... }"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/oauth2.c` around lines 515 - 541, The function declaration for create_oauth_ctx_with_user_agent should follow the project's brace style by placing the opening brace on the next line; edit the signature line for create_oauth_ctx_with_user_agent(...) so the "{" is moved from the end of the signature to its own following line, keeping the rest of the function body (memset, cfg setup, flb_sds_printf, flb_oauth2_create_from_config, flb_oauth2_config_destroy, return ctx) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/internal/oauth2.c`:
- Around line 515-541: The function declaration for
create_oauth_ctx_with_user_agent should follow the project's brace style by
placing the opening brace on the next line; edit the signature line for
create_oauth_ctx_with_user_agent(...) so the "{" is moved from the end of the
signature to its own following line, keeping the rest of the function body
(memset, cfg setup, flb_sds_printf, flb_oauth2_create_from_config,
flb_oauth2_config_destroy, return ctx) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e3b2fb1-d223-4412-927b-0205786ce7af
📒 Files selected for processing (3)
plugins/out_http/http.csrc/flb_oauth2.ctests/internal/oauth2.c
I don't see anything wrong with the code here. Those are exactly the same. |
|
We don't want to contaminate test codes into implementation commits. |
rja5
force-pushed
the
add-oauth-user-agent
branch
from
edaa636 to
3db3428
Compare
rja5
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
rja5
force-pushed
the
add-oauth-user-agent
branch
from
3db3428 to
24dad59
Compare
|
This commit 13c96e0 is still contaminated of the oauth2 related file. oauth2 related files should be committed in this kind of commit: oauth2: Add user-agent member field And we just need to include out_http only files' changes in |
Signed-off-by: rja5 <rallen99@gmail.com>
Signed-off-by: rja5 <rallen99@gmail.com>
Signed-off-by: rja5 <rallen99@gmail.com>
rja5
force-pushed
the
add-oauth-user-agent
branch
from
24dad59 to
63c80b4
Compare
Hopefully the newest one is correct. |
cosmo0920
left a comment
cosmo0920
left a comment
There was a problem hiding this comment.
The most of this PR looks good but I found an inappropriate addition.
So, could you address it?
Signed-off-by: rja5 <rallen99@gmail.com>
2 participants
I added the ability to supply an optional User-Agent header to the HTTP OAuth call.
Fixes feature request/issue #11826
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
add this to any HTTP OAuth config:
[OUTPUT]
oauth2.user_agent test-agent
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests