Repository navigation
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. WalkthroughWhen ChangesAuth method default
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Suggested reviewers: Merge Risk: ⚪ Minimal · up to The stored auth method now defaults to GET when omitted. No actionable issue remains before merge, subject to normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The existing GET default becomes visible in stored token-auth options. The inspected token-request path still uses GET when no method is supplied and preserves an explicit POST. No new authentication bypass was identified, though external consumers of the stored options were not fully covered. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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. A rabbit checked the options at dawn, Comment |
ttypic
left a comment
ttypic
left a comment
There was a problem hiding this comment.
LGTM, thanks for contribution!
_saveTokenOptions assigned the caller's AuthOptions unchanged, so auth.authOptions.authMethod stayed undefined when omitted. The GET default was applied only at request time. Store authMethod: 'GET' on the options object when the property is absent, matching other option defaults, and un-skip the existing UTS test.
da547fd to
1a9c4df
Compare
This branch has not been deployed
2 participants
Summary
When
authMethodis omitted,auth.authOptions.authMethodis'GET'(AO2 / AO2d). The GET default was already applied at request time (authMethod.toLowerCase() === 'post'is the only POST path) but was not stored on the options object._saveTokenOptionswrites the default when the property is absent, using the same'prop' in optionsguard as other ClientOptions defaults. The existing UTS test is un-skipped and the AO2deviations.mdentry is removed.Fixes #2205
What I chose and the alternative
authMethod: 'GET'is stored in_saveTokenOptionswhen the property is absent, not inDefaults.normaliseOptions(thetls/queueMessagespath)._saveTokenOptionsis where storedAuthOptionsare assigned, includingauthorize()replacements that never go throughnormaliseOptions. The failing test inspectsclient.auth.authOptions.Can also or instead set the default in
normaliseOptionsand/or_saveBasicOptionsif you want every AuthOptions view, including key-only basic-auth clients, to show'GET'.The constructor shares the normalised ClientOptions object with
auth.authOptions, soclient.options.authMethodalso becomes'GET'. That matches how other defaults already appear onclient.options.Test plan
AO2 - authMethod defaults to GETfails without the source change (expected undefined to equal 'GET') and passes with itAO2 - authUrl and authMethod optionsstill stores explicitPOSTtest/uts/rest/unit/types/options_types.test.ts: 10 passingtest/uts/rest/unit/auth/*.test.ts: 87 passing, 7 pending (RSA8c GET/POST wire behaviour unchanged)tsx, same ignore set astest:uts:unit): 1419 passing, 47 pendingSummary by CodeRabbit