fix: handle optional clientSecret in OAuth2 authorization header#6186
Conversation
bijin-bruno
left a comment
There was a problem hiding this comment.
@abhishek-bruno Please add a few unit tests.
dbf9b16 to
c88aaba
Compare
WalkthroughUniformize OAuth2 Basic Authorization header creation to always use a derived secret ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
c88aaba to
da4925b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bruno-electron/src/utils/oauth2.js (2)
256-258: LGTM with minor style note.The Authorization Code flow correctly implements the optional clientSecret handling. However, line 256 uses double quotes for the string literal, while coding guidelines specify single quotes for JavaScript files.
♻️ Optional: Use single quotes for consistency
- if (credentialsPlacement === "basic_auth_header") { + if (credentialsPlacement === 'basic_auth_header') {As per coding guidelines, JavaScript strings should use single quotes.
672-674: LGTM with minor style note.The refresh token flow correctly implements optional clientSecret handling. However, line 672 uses double quotes for the string literal.
♻️ Optional: Use single quotes for consistency
- if (credentialsPlacement === "basic_auth_header") { + if (credentialsPlacement === 'basic_auth_header') {As per coding guidelines, JavaScript strings should use single quotes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-electron/src/utils/oauth2.jspackages/bruno-requests/src/auth/oauth2-helper.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-electron/src/utils/oauth2.jspackages/bruno-requests/src/auth/oauth2-helper.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
📚 Learning: 2025-12-02T07:24:50.311Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-electron/src/utils/oauth2.jspackages/bruno-requests/src/auth/oauth2-helper.ts
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-electron/src/utils/oauth2.js
🧬 Code graph analysis (1)
packages/bruno-requests/src/auth/oauth2-helper.ts (1)
packages/bruno-electron/src/utils/oauth2.js (4)
secret(257-257)secret(463-463)secret(611-611)secret(673-673)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - Linux
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
🔇 Additional comments (4)
packages/bruno-requests/src/auth/oauth2-helper.ts (2)
147-150: LGTM! Correctly handles optional clientSecret with nullish coalescing.The use of
??ensures the Authorization header is constructed even when clientSecret is null or undefined, resolving the reported regression. This approach is more precise than||as it preserves explicit empty strings and only defaults null/undefined values.Based on learnings, clientSecret is optional in OAuth2 Basic Auth for Bruno.
249-252: LGTM! Consistent implementation with client credentials flow.The password grant flow correctly applies the same nullish coalescing pattern to handle optional clientSecret, ensuring consistent behavior across all token fetch methods.
packages/bruno-electron/src/utils/oauth2.js (2)
462-464: LGTM! Clean implementation for client credentials flow.The client credentials flow correctly handles optional clientSecret and follows coding style guidelines.
610-612: LGTM! Consistent with other token flows.The password credentials flow properly applies the same pattern for handling optional clientSecret.
da4925b to
d74d7ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bruno-electron/src/utils/oauth2.js (2)
256-259: Fix looks correct; minor quote inconsistency.The nullish coalescing approach (
clientSecret ?? '') properly handles the optional secret case. However, line 256 uses double quotes"basic_auth_header"while other locations (lines 462, 610) use single quotes. Per coding guidelines, prefer single quotes for string literals.🔧 Optional: unify quote style
- if (credentialsPlacement === "basic_auth_header") { + if (credentialsPlacement === 'basic_auth_header') {
672-675: Fix is correct; same quote inconsistency as line 256.The refresh token flow now properly includes the Authorization header when
clientSecretis missing, which resolves the regression. Same optional refactor applies for quote consistency.🔧 Optional: unify quote style
- if (credentialsPlacement === "basic_auth_header") { + if (credentialsPlacement === 'basic_auth_header') {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-electron/src/utils/oauth2.jspackages/bruno-requests/src/auth/oauth2-helper.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-requests/src/auth/oauth2-helper.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-electron/src/utils/oauth2.js
🧠 Learnings (3)
📓 Common learnings
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
📚 Learning: 2025-12-02T07:24:50.311Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-electron/src/utils/oauth2.js
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-electron/src/utils/oauth2.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Windows
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (2)
packages/bruno-electron/src/utils/oauth2.js (2)
462-465: LGTM!Consistent application of the fix pattern. Single quote usage aligns with coding guidelines. Based on learnings, the
clientSecret ?? ''approach is the correct way to handle optional secrets for OAuth2 Basic Auth headers.
610-613: LGTM!Same fix pattern correctly applied here.
d74d7ca to
254d8a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/bruno-requests/src/auth/oauth2-helper.spec.ts:
- Around line 127-132: The comment incorrectly claims the code uses nullish
coalescing (`clientSecret ?? ''`) when the implementation uses logical OR
(`clientSecret || ''`); update the test comment in oauth2-helper.spec.ts to
state that an empty string is treated like undefined because the code uses the
logical OR operator (`||`), and remove or correct the mention of nullish
coalescing so the comment accurately describes the behavior being tested.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/tests.yml.gitignorepackages/bruno-requests/src/auth/oauth2-helper.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-requests/src/auth/oauth2-helper.spec.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
.github/workflows/tests.ymlpackages/bruno-requests/src/auth/oauth2-helper.spec.ts
📚 Learning: 2025-12-16T07:16:23.647Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:23.647Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.
Applied to files:
.github/workflows/tests.yml
📚 Learning: 2025-12-02T07:24:50.311Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-requests/src/auth/oauth2-helper.spec.ts
🧬 Code graph analysis (1)
packages/bruno-requests/src/auth/oauth2-helper.spec.ts (1)
packages/bruno-requests/src/auth/oauth2-helper.ts (2)
TokenStore(5-9)OAuth2Config(18-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Linux
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: Playwright E2E Tests
- GitHub Check: CLI Tests
- GitHub Check: SSL Tests - Windows
🔇 Additional comments (8)
.github/workflows/tests.yml (1)
61-62: LGTM!The new test step for
bruno-requestsis correctly placed after the build step (line 32) and follows the established pattern of other package test steps. This ensures the new OAuth2 helper tests will run in CI.packages/bruno-requests/src/auth/oauth2-helper.spec.ts (7)
4-25: LGTM! Well-documented mock token store.Clean implementation with proper JSDoc. The key formatting pattern
${url}:${credentialsId}aligns with theTokenStoreinterface expectations.
27-52: LGTM! Solid mock adapter pattern.Captures request config for assertions while returning controlled responses. The
Buffer.from(JSON.stringify(responseData))simulates raw response body correctly.
67-77: Good test isolation pattern.Saving and restoring
axios.defaults.adapterinbeforeEach/afterEachensures tests don't leak state. This pattern is repeated correctly in both describe blocks.
95-125: LGTM! Core regression test for the fix.This test directly validates the issue from #5452 — Authorization header must be attached even when
clientSecretis undefined. The assertions correctly verify:
- Header contains
Basic base64(clientId:)grant_typepresent in bodyclient_idnot duplicated in body
200-268: LGTM! Body placement tests are comprehensive.Correctly verifies that when
credentialsPlacement: 'body':
- No Authorization header is sent
client_idappears in body- Empty
client_secretis omitted (not sent as empty value)
271-400: LGTM! Password grant tests mirror client credentials structure.Good coverage ensuring password grant also handles optional
clientSecretcorrectly. Properly verifiesusernameandpasswordalways appear in the body regardless of credentials placement.
402-475: LGTM! Password grant body placement tests complete the coverage.All four combinations tested:
{basic_auth_header, body}×{empty secret, present secret}. This aligns with the PR objective to ensure OAuth2 works with or withoutclientSecret.
| /** | ||
| * Verifies that an empty string clientSecret is treated the same as undefined. | ||
| * | ||
| * The implementation uses nullish coalescing (clientSecret ?? '') so both | ||
| * undefined and empty string result in the same Authorization header. | ||
| */ |
There was a problem hiding this comment.
Incorrect comment: nullish coalescing doesn't handle empty strings.
The comment states the implementation uses "nullish coalescing (clientSecret ?? '')" but ?? only handles null/undefined, not empty strings. For empty string to behave the same as undefined (as this test expects), the implementation must use clientSecret || ''. Based on learnings, the implementation indeed uses ||.
📝 Suggested fix
/**
* Verifies that an empty string clientSecret is treated the same as undefined.
*
- * The implementation uses nullish coalescing (clientSecret ?? '') so both
- * undefined and empty string result in the same Authorization header.
+ * The implementation uses logical OR (clientSecret || '') so both
+ * undefined and empty string result in the same Authorization header.
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Verifies that an empty string clientSecret is treated the same as undefined. | |
| * | |
| * The implementation uses nullish coalescing (clientSecret ?? '') so both | |
| * undefined and empty string result in the same Authorization header. | |
| */ | |
| /** | |
| * Verifies that an empty string clientSecret is treated the same as undefined. | |
| * | |
| * The implementation uses logical OR (clientSecret || '') so both | |
| * undefined and empty string result in the same Authorization header. | |
| */ |
🤖 Prompt for AI Agents
In @packages/bruno-requests/src/auth/oauth2-helper.spec.ts around lines 127 -
132, The comment incorrectly claims the code uses nullish coalescing
(`clientSecret ?? ''`) when the implementation uses logical OR (`clientSecret ||
''`); update the test comment in oauth2-helper.spec.ts to state that an empty
string is treated like undefined because the code uses the logical OR operator
(`||`), and remove or correct the mention of nullish coalescing so the comment
accurately describes the behavior being tested.
…bruno#6186) * fix: handle optional clientSecret in OAuth2 authorization header * style: standardize string quotes in OAuth2 token functions * test: add comprehensive tests for OAuth2 client credentials and password grant flows
Description
Fixes an issue where the Authorization header was not attached when clientSecret was missing. clientSecret is now optional, so the header is always included when credentialsPlacement is set to basic_auth_header, even without a client secret.
Closes #5452
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.