oauth2: restore legacy context creation behavior#11662
oauth2: restore legacy context creation behavior#11662
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
📝 WalkthroughWalkthroughAdded an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/out_azure_logs_ingestion/azure_logs_ingestion_conf.c`:
- Around line 94-113: Validate auth_url_override before assigning it: in the
branch that handles ctx->auth_url_override (before calling flb_sds_create),
ensure the provided URL uses the HTTPS scheme or is an explicit localhost
loopback HTTP (e.g., host equals "localhost" or "127.0.0.1" with any port) to
allow tests; if the scheme is plain "http" for any non-loopback host, log/return
an error, call flb_az_li_ctx_destroy(ctx) and return NULL. Implement the check
around ctx->auth_url_override parsing (inspect the prefix/host) so
flb_sds_create is only called for validated URLs and insecure cleartext token
endpoints are rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a30ba3e9-a88d-4c7b-8c7d-d3a5bc6918ae
📒 Files selected for processing (5)
plugins/out_azure_logs_ingestion/azure_logs_ingestion.cplugins/out_azure_logs_ingestion/azure_logs_ingestion.hplugins/out_azure_logs_ingestion/azure_logs_ingestion_conf.csrc/flb_oauth2.ctests/internal/oauth2.c
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
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)
plugins/out_azure_logs_ingestion/azure_logs_ingestion_conf.c (1)
104-109:⚠️ Potential issue | 🟡 MinorPre-existing memory leak: missing context cleanup on config_map_set failure.
If
flb_output_config_map_setfails at line 106, the allocatedctxis not freed before returningNULL. While this is existing code, it's worth fixing alongside the current changes for consistency with the cleanup pattern used elsewhere in this function.🔧 Proposed fix
/* Load config map */ ret = flb_output_config_map_set(ins, (void *) ctx); if (ret == -1) { flb_plg_error(ins, "unable to load configuration"); + flb_az_li_ctx_destroy(ctx); return NULL; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_azure_logs_ingestion/azure_logs_ingestion_conf.c` around lines 104 - 109, If flb_output_config_map_set(ins, (void *) ctx) fails the allocated ctx is leaked; before returning NULL free the context (e.g., call the same allocator's free used elsewhere, such as flb_free(ctx) or free(ctx)) and null it if needed, then call flb_plg_error and return NULL so that ctx is cleaned up on error; update the failure branch around flb_output_config_map_set to release ctx before returning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/out_azure_logs_ingestion/azure_logs_ingestion_conf.c`:
- Around line 104-109: If flb_output_config_map_set(ins, (void *) ctx) fails the
allocated ctx is leaked; before returning NULL free the context (e.g., call the
same allocator's free used elsewhere, such as flb_free(ctx) or free(ctx)) and
null it if needed, then call flb_plg_error and return NULL so that ctx is
cleaned up on error; update the failure branch around flb_output_config_map_set
to release ctx before returning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 39adb285-f021-4b34-86f0-d03d7190dfb7
📒 Files selected for processing (1)
plugins/out_azure_logs_ingestion/azure_logs_ingestion_conf.c
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
edsiper
requested review from
celalettin1286,
niedbalski and
patrick-stephens
as code owners
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/scripts/commit_prefix_check.py:
- Around line 24-34: The regex PREFIX_RE allows multiple spaces (e.g., "tests:
internal:"), but normalize_subject_prefix() compares against exact strings like
"tests: internal:" so such inputs won't normalize; update
normalize_subject_prefix() to normalize whitespace before doing comparisons
(e.g., collapse consecutive spaces and trim, or run a regex replacement to
convert any run of whitespace between tokens into a single space) and then
perform the existing lower-case and equality checks so that values matched by
PREFIX_RE (and the constant PREFIX_RE itself) are normalized consistently to
"tests:".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f1d3438-1830-48c4-b7da-20bc3c283b2c
📒 Files selected for processing (1)
.github/scripts/commit_prefix_check.py
| # | ||
| # Allow test-specific prefixes such as: | ||
| # - tests: internal: | ||
| # - tests: runtime: | ||
| # - tests: integration: | ||
| # | ||
| # These are normalized back to tests: for path-based validation. | ||
| PREFIX_RE = re.compile( | ||
| r"^((?:tests:\s+(?:internal|runtime|integration):)|[a-z0-9_]+:)\s+\S", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
Minor inconsistency between regex and normalization logic.
The PREFIX_RE regex uses \s+ which allows multiple whitespace characters between tests: and the subcategory (e.g., tests: internal:). However, normalize_subject_prefix() at lines 60-61 performs exact string matching with single spaces ("tests: internal:").
If a commit uses extra whitespace like tests: internal: add test, the regex will match and extract tests: internal:, but normalization will fall through to line 63 returning the lowered string as-is instead of normalizing to tests:.
Consider normalizing whitespace before comparison:
Proposed fix
def normalize_subject_prefix(prefix: str) -> str:
"""
Map accepted subject-prefix variants to the canonical prefix used by
path-based validation.
"""
- lowered = prefix.lower()
+ # Normalize internal whitespace to single space before comparison
+ lowered = ' '.join(prefix.lower().split())
if lowered in ("tests: internal:", "tests: runtime:", "tests: integration:"):
return "tests:"
return loweredAlso applies to: 53-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/scripts/commit_prefix_check.py around lines 24 - 34, The regex
PREFIX_RE allows multiple spaces (e.g., "tests: internal:"), but
normalize_subject_prefix() compares against exact strings like "tests:
internal:" so such inputs won't normalize; update normalize_subject_prefix() to
normalize whitespace before doing comparisons (e.g., collapse consecutive spaces
and trim, or run a regex replacement to convert any run of whitespace between
tokens into a single space) and then perform the existing lower-case and
equality checks so that values matched by PREFIX_RE (and the constant PREFIX_RE
itself) are normalized consistently to "tests:".
1 participant
Fixes #11649
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
Bug Fixes
Tests
Chores