Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15abf7fb16
ℹ️ 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".
📝 WalkthroughWalkthroughJSON parsing for OAuth2 tokens was hardened: iterative bounds checking, transactional temporary holders for extracted fields, strict errno-checked numeric parsing for expires_in with negative/partial-rejects, a 10% expiry reduction applied on success, and rejection when required fields are missing. Tests expanded to cover these cases. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (1)
src/flb_oauth2.c (1)
522-543: Consider using integer arithmetic for the 10% reduction.The floating-point multiplication
new_expires_in * 0.10works correctly for practical OAuth2 expiry values, but integer division would be more consistent with the integer types in use and avoid any potential floating-point precision concerns.♻️ Suggested integer arithmetic
new_expires_in = parsed_expires_in; - new_expires_in -= (new_expires_in * 0.10); + new_expires_in -= (new_expires_in / 10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_oauth2.c` around lines 522 - 543, The code uses floating-point to reduce new_expires_in by 10% (new_expires_in -= (new_expires_in * 0.10)); change this to integer arithmetic using the existing unsigned integer types (e.g., subtract new_expires_in/10 or compute (new_expires_in * 9) / 10) to avoid FP precision and type mixing; update the logic around parsed_expires_in/new_expires_in in the block that handles the "expires_in" key (see key_cmp, tmp_num, parsed_expires_in, new_expires_in) and ensure the result remains unsigned and does not underflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/flb_oauth2.c`:
- Around line 522-543: The code uses floating-point to reduce new_expires_in by
10% (new_expires_in -= (new_expires_in * 0.10)); change this to integer
arithmetic using the existing unsigned integer types (e.g., subtract
new_expires_in/10 or compute (new_expires_in * 9) / 10) to avoid FP precision
and type mixing; update the logic around parsed_expires_in/new_expires_in in the
block that handles the "expires_in" key (see key_cmp, tmp_num,
parsed_expires_in, new_expires_in) and ensure the result remains unsigned and
does not underflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec7fd1aa-007b-4c69-b7e0-63405394f543
📒 Files selected for processing (2)
src/flb_oauth2.ctests/internal/oauth2.c
erain
force-pushed
the
yiyu/oauth2-refresh-parse
branch
from
15abf7f to
b418afe
Compare
erain
force-pushed
the
yiyu/oauth2-refresh-parse
branch
from
b418afe to
b565c30
Compare
Reject malformed refresh payloads so Fluent Bit never reuses stale OAuth2 credentials after a parse failure. Use integer arithmetic for the 10% TTL reduction and tie the expires_in floor to ctx->refresh_skew instead of a hardcoded 60. Log only payload size on parse errors to avoid credential leakage. Change-Id: I202807582b34f5be48bc4c919e230eeaa0d2b5ff Signed-off-by: Yu Yi <yiyu@google.com>
Add regression coverage for transactional token updates, quoted expires_in, duplicate key handling, and stricter expires_in validation. Set refresh_skew on test contexts and adjust mock server values for integer arithmetic. Signed-off-by: Yu Yi <yiyu@google.com>
erain
force-pushed
the
yiyu/oauth2-refresh-parse
branch
from
b565c30 to
3754123
Compare
|
Addressed all AI review feedback:
Also split into two commits to satisfy the commit-lint prefix validation, and added |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flb_oauth2.c (1)
477-544:⚠️ Potential issue | 🟠 MajorStill accepts nested and non-string token fields.
This loop only advances to the immediate value token, then resumes scanning the rest of the token array. That means keys inside object/array values can still be revisited as if they were top-level pairs, and
access_token/token_typeare copied without checking that their value token is actually a string. A payload like{"error":"x","details":{"access_token":"nested","token_type":"Bearer","expires_in":3600}}or{"access_token":123,"token_type":"Bearer","expires_in":3600}will still be treated as a valid refresh. Please restrict extraction to direct children of the root object and requireJSMN_STRINGfor the required token fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_oauth2.c` around lines 477 - 544, The loop over jsmn tokens incorrectly treats nested fields as top-level and accepts non-string values; update the scan so you only consider direct children of the root object and validate value token types: for each key token (the current t when iterating tokens[]), ensure its matching value token is immediately after the key and that the value token's type is JSMN_STRING for "access_token" and "token_type" (and a primitive/number for "expires_in"); when the value token is JSMN_OBJECT or JSMN_ARRAY, skip its entire subtree by advancing i past all tokens that belong to that value (use t->size and recurse/count descendant tokens in tokens[] to compute the skip), and only then resume scanning; apply these checks around the key comparisons (key_cmp, new_access_token, new_token_type, new_expires_in) so nested or non-string fields are ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/internal/oauth2.c`:
- Around line 914-917: The test is flaky because expires_in (passed to
oauth2_mock_server_start as 65 which becomes 59 after the 10% reduction) and
refresh_skew (58 passed into create_oauth_ctx) leave only a 1s window that can
straddle time(NULL) boundaries and trigger an unexpected refresh; to fix, widen
the gap so the token is not refreshable immediately (e.g., increase the
expires_in value passed to oauth2_mock_server_start or decrease the refresh_skew
passed to create_oauth_ctx) or explicitly force the refresh timing in
test_caching_and_refresh so flb_oauth2_get_access_token cannot refresh on the
second call; adjust the arguments where oauth2_mock_server_start(&server, 65, 0)
and create_oauth_ctx(config, &server, 58) are invoked accordingly and ensure the
token_requests == 1 assertion remains valid.
---
Outside diff comments:
In `@src/flb_oauth2.c`:
- Around line 477-544: The loop over jsmn tokens incorrectly treats nested
fields as top-level and accepts non-string values; update the scan so you only
consider direct children of the root object and validate value token types: for
each key token (the current t when iterating tokens[]), ensure its matching
value token is immediately after the key and that the value token's type is
JSMN_STRING for "access_token" and "token_type" (and a primitive/number for
"expires_in"); when the value token is JSMN_OBJECT or JSMN_ARRAY, skip its
entire subtree by advancing i past all tokens that belong to that value (use
t->size and recurse/count descendant tokens in tokens[] to compute the skip),
and only then resume scanning; apply these checks around the key comparisons
(key_cmp, new_access_token, new_token_type, new_expires_in) so nested or
non-string fields are ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9027142f-b9f1-4b1a-ac2b-2ffb1552871b
📒 Files selected for processing (2)
src/flb_oauth2.ctests/internal/oauth2.c
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)
src/flb_oauth2.c (1)
477-552:⚠️ Potential issue | 🟠 MajorMalformed refresh payloads can still pass this validation.
The loop accepts
access_token/token_typefrom any token slice and ignoreserror, so responses like{"error":"invalid_grant","access_token":"x","token_type":"Bearer","expires_in":3600},{"meta":{"access_token":"x"},"token_type":"Bearer","expires_in":3600}, or{"access_token":null,"token_type":"Bearer","expires_in":3600}still commit new state. That reopens the partial/malformed acceptance this PR is trying to close. Please require top-level non-empty string values for the required fields and fail fast on anyerrormember, and add regression cases for those shapes.Possible tightening
+ int has_error = FLB_FALSE; ... + if (key_cmp(key, key_len, "error") == 0) { + has_error = FLB_TRUE; + break; + } + + if (t->type == JSMN_OBJECT || t->type == JSMN_ARRAY) { + break; + } + if (key_cmp(key, key_len, "access_token") == 0) { + if (t->type != JSMN_STRING || val_len == 0) { + break; + } ... } else if (key_cmp(key, key_len, "token_type") == 0) { + if (t->type != JSMN_STRING || val_len == 0) { + break; + } ... } ... - if (!new_access_token || !new_token_type || new_expires_in <= ctx->refresh_skew) { + if (has_error || !new_access_token || !new_token_type || + new_expires_in <= ctx->refresh_skew) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flb_oauth2.c` around lines 477 - 552, The parser currently accepts nested or non-string values and ignores an "error" field; update the loop in flb_oauth2.c (the tokens/t/t->type handling around key_cmp, new_access_token, new_token_type, new_expires_in) to: 1) if key equals "error" (compare via key_cmp) return/fail fast; 2) require the value token type to be the expected primitive (for access_token and token_type require t->type == JSMN_STRING and val_len > 0) before creating flb_sds; 3) reject non-string/null/nested values (do not accept JSMN_NULL or JSMN_OBJECT/ARRAY) and treat them as errors; 4) for expires_in ensure the value token is a JSMN_PRIMITIVE and still validate numeric parsing as existing code does; add regression tests for shapes like {"error":"...","access_token":"x"}, nested keys, and null access_token to ensure they fail.
♻️ Duplicate comments (1)
tests/internal/oauth2.c (1)
914-917:⚠️ Potential issue | 🟡 MinorThis is still only a one-second refresh margin.
After the 10% haircut,
65becomes59; withrefresh_skew=58, the cached token becomes refreshable one second after issuance. If the twoflb_oauth2_get_access_token()calls land on differenttime(NULL)ticks,server.token_requests == 1can still fail intermittently. Please widen the gap or make the refresh step explicit.Suggested tweak
- ret = oauth2_mock_server_start(&server, 65, 0); + ret = oauth2_mock_server_start(&server, 68, 0); ... - ctx = create_oauth_ctx(config, &server, 58); + ctx = create_oauth_ctx(config, &server, 60);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/internal/oauth2.c` around lines 914 - 917, The test's timing window is too narrow: increase the token lifetime passed to oauth2_mock_server_start (currently 65) or reduce the refresh_skew passed to create_oauth_ctx so that after the 10% haircut the token becomes refreshable with a multi-second margin (e.g., change 65 to ~80+ or ensure lifetime - haircut >= refresh_skew + 2), or alternatively make the refresh explicit by advancing time or calling the refresh path directly before the second flb_oauth2_get_access_token() call; update the values around oauth2_mock_server_start and create_oauth_ctx to guarantee a stable gap and avoid intermittent failures.
🤖 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 `@src/flb_oauth2.c`:
- Around line 477-552: The parser currently accepts nested or non-string values
and ignores an "error" field; update the loop in flb_oauth2.c (the
tokens/t/t->type handling around key_cmp, new_access_token, new_token_type,
new_expires_in) to: 1) if key equals "error" (compare via key_cmp) return/fail
fast; 2) require the value token type to be the expected primitive (for
access_token and token_type require t->type == JSMN_STRING and val_len > 0)
before creating flb_sds; 3) reject non-string/null/nested values (do not accept
JSMN_NULL or JSMN_OBJECT/ARRAY) and treat them as errors; 4) for expires_in
ensure the value token is a JSMN_PRIMITIVE and still validate numeric parsing as
existing code does; add regression tests for shapes like
{"error":"...","access_token":"x"}, nested keys, and null access_token to ensure
they fail.
---
Duplicate comments:
In `@tests/internal/oauth2.c`:
- Around line 914-917: The test's timing window is too narrow: increase the
token lifetime passed to oauth2_mock_server_start (currently 65) or reduce the
refresh_skew passed to create_oauth_ctx so that after the 10% haircut the token
becomes refreshable with a multi-second margin (e.g., change 65 to ~80+ or
ensure lifetime - haircut >= refresh_skew + 2), or alternatively make the
refresh explicit by advancing time or calling the refresh path directly before
the second flb_oauth2_get_access_token() call; update the values around
oauth2_mock_server_start and create_oauth_ctx to guarantee a stable gap and
avoid intermittent failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 128cc8ed-d854-4240-973e-b098d1f01a15
📒 Files selected for processing (2)
src/flb_oauth2.ctests/internal/oauth2.c
CI failure analysis:
|
1 participant
Problem
expires_invalues as usable.Solution
access_token,token_type, andexpires_inall validate.expires_invalues while still accepting quoted numeric expirations and duplicate keys with last-value-wins behavior.Testing
cmake --build build --target flb-it-oauth2 -j4 && ./build/bin/flb-it-oauth2Summary by CodeRabbit
Bug Fixes
Tests