fix(oauth2): prevent code injection in OAuth2 callback handling#8405
Open
abhishekp-bruno wants to merge 7 commits into
Open
fix(oauth2): prevent code injection in OAuth2 callback handling#8405abhishekp-bruno wants to merge 7 commits into
abhishekp-bruno wants to merge 7 commits into
Conversation
abhishekp-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds OAuth2 state generation with nonce suffixes, propagates expected state through authorization helpers, validates callback state on return, surfaces authorization failures in the response pane, and adds unit plus E2E coverage for mismatch and success cases. ChangesOAuth2 State CSRF Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-electron/src/utils/oauth2.js (1)
331-338: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReserve
stateinstead of appending a duplicate parameter.If
authorizationUrloradditionalParameters.authorizationalready containsstate,append()sends duplicatestateparams. OAuth providers differ on duplicate handling, so the callback may echo a different value and fail validation. Set the generated state after custom params so it is the single canonical value.Proposed fix
- if (effectiveState) { - authorizationUrlWithQueryParams.searchParams.append('state', effectiveState); - } if (additionalParameters?.authorization?.length) { additionalParameters.authorization.forEach((param) => { if (param.enabled && param.name) { if (param.sendIn === 'queryparams') { authorizationUrlWithQueryParams.searchParams.append(param.name, param.value || ''); } } }); } + if (effectiveState) { + authorizationUrlWithQueryParams.searchParams.set('state', effectiveState); + }- if (effectiveState) { - authorizationUrlWithQueryParams.searchParams.append('state', effectiveState); - } if (additionalParameters?.authorization?.length) { additionalParameters.authorization.forEach((param) => { if (param.enabled && param.name) { if (param.sendIn === 'queryparams') { authorizationUrlWithQueryParams.searchParams.append(param.name, param.value || ''); } } }); } + if (effectiveState) { + authorizationUrlWithQueryParams.searchParams.set('state', effectiveState); + }Also applies to: 865-872
🤖 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 `@packages/bruno-electron/src/utils/oauth2.js` around lines 331 - 338, The OAuth2 URL builder in oauth2.js is appending a generated state value with searchParams.append, which can create duplicate state query parameters when authorizationUrl or additionalParameters.authorization already includes state. Update the authorization URL assembly logic so the generated state is applied last and as the single canonical value, replacing any existing state parameter instead of appending another. Make this change in the code path that builds authorizationUrlWithQueryParams and in the related section noted in the comment so both flows use the same state handling.
🤖 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.
Inline comments:
In
`@packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.js`:
- Around line 96-98: The OAuth2 action handlers are passing the raw result of
formatIpcError() straight into the UI, which can leave an object payload
displayed as an unreadable message. In the Oauth2ActionButtons component,
normalize the output from formatIpcError() to a string before using it in
toast.error(...) and showOauth2Error(...), and keep the fallback message when
the formatted value is not a usable string. Apply the same fix in both affected
handlers so the UI always receives a human-readable OAuth error.
In `@packages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.js`:
- Around line 110-121: Add a test in oauth2-protocol-handler.spec.js that covers
the hash-fragment provider error path handled by handleOauth2ProtocolUrl when
parsing implicit callbacks. Reuse the existing
registerOauth2AuthorizationRequest and assert that a URL like
bruno://oauth2/callback#error=access_denied rejects with the provider error
before state validation, with resolve untouched and reject receiving an
Authorization Failed message. Ensure the new case sits alongside the existing
error-response precedence test so both ?error= and `#error`= branches are covered.
In `@tests/auth/oauth2/oauth2-state-validation.spec.ts`:
- Around line 64-83: The callback capture helper in installCallbackCapture is
mutating Electron’s second-instance listener set by removing all listeners and
re-adding wrappers, which changes listener order and breaks any original .once()
behavior. Update it so it observes the bruno:// callback URL without replacing
Bruno’s existing listeners, preserving the original second-instance wiring while
still storing the captured URL in __brunoCapturedCallbackUrl.
- Around line 45-52: The callback-code parsing in fetchAuthCodeFromTestbench is
too restrictive because OAuth codes are opaque and may not be hex-only; update
the regex used to extract the code from the authorization response HTML so it
accepts a generic non-empty callback code shape instead of only [a-f0-9]+. Keep
the existing response and match assertions, but make the code extraction in
oauth2-state-validation.spec.ts provider-agnostic so the test still verifies the
returned bruno://app/oauth2/callback URL.
---
Outside diff comments:
In `@packages/bruno-electron/src/utils/oauth2.js`:
- Around line 331-338: The OAuth2 URL builder in oauth2.js is appending a
generated state value with searchParams.append, which can create duplicate state
query parameters when authorizationUrl or additionalParameters.authorization
already includes state. Update the authorization URL assembly logic so the
generated state is applied last and as the single canonical value, replacing any
existing state parameter instead of appending another. Make this change in the
code path that builds authorizationUrlWithQueryParams and in the related section
noted in the comment so both flows use the same state handling.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0eb95a0c-2269-4f7d-85aa-05458f09c51f
📒 Files selected for processing (13)
packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.jspackages/bruno-electron/src/ipc/network/authorize-user-in-system-browser.jspackages/bruno-electron/src/ipc/network/authorize-user-in-window.jspackages/bruno-electron/src/utils/oauth2-protocol-handler.jspackages/bruno-electron/src/utils/oauth2.jspackages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.jstests/auth/oauth2/fixtures/collection/Authorization Code.brutests/auth/oauth2/fixtures/collection/Implicit.brutests/auth/oauth2/fixtures/collection/User Supplied State.brutests/auth/oauth2/fixtures/collection/bruno.jsontests/auth/oauth2/fixtures/collection/environments/Local.brutests/auth/oauth2/init-user-data/preferences.jsontests/auth/oauth2/oauth2-state-validation.spec.ts
Comment on lines
+96
to
+98
| const errorMessage = formatIpcError(error) || 'An error occurred while fetching token!'; | ||
| toast.error(errorMessage); | ||
| showOauth2Error(errorMessage); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Normalize formatIpcError() before sending it to the UI.
formatIpcError() returns the original value for non-Error inputs, so a truthy object payload here skips the fallback and gets pushed into both toast.error(...) and response.error. That will surface a useless [object Object]-style message instead of a readable OAuth error.
Suggested fix
- const errorMessage = formatIpcError(error) || 'An error occurred while fetching token!';
+ const formattedError = formatIpcError(error);
+ const errorMessage =
+ typeof formattedError === 'string'
+ ? formattedError
+ : formattedError?.message || 'An error occurred while fetching token!';
toast.error(errorMessage);
showOauth2Error(errorMessage);- const errorMessage = formatIpcError(error) || 'An error occurred while refreshing token!';
+ const formattedError = formatIpcError(error);
+ const errorMessage =
+ typeof formattedError === 'string'
+ ? formattedError
+ : formattedError?.message || 'An error occurred while refreshing token!';
toast.error(errorMessage);
showOauth2Error(errorMessage);Also applies to: 133-135
🤖 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
`@packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.js`
around lines 96 - 98, The OAuth2 action handlers are passing the raw result of
formatIpcError() straight into the UI, which can leave an object payload
displayed as an unreadable message. In the Oauth2ActionButtons component,
normalize the output from formatIpcError() to a string before using it in
toast.error(...) and showOauth2Error(...), and keep the fallback message when
the formatted value is not a usable string. Apply the same fix in both affected
handlers so the UI always receives a human-readable OAuth error.
Comment on lines
+110
to
+121
| describe('error responses are handled before state validation', () => { | ||
| it('should reject with the provider error even if state is absent', () => { | ||
| registerOauth2AuthorizationRequest(resolve, reject, null, 'expected-state'); | ||
|
|
||
| handleOauth2ProtocolUrl('bruno://oauth2/callback?error=access_denied'); | ||
|
|
||
| expect(resolve).not.toHaveBeenCalled(); | ||
| expect(reject).toHaveBeenCalledWith( | ||
| expect.objectContaining({ message: expect.stringContaining('Authorization Failed') }) | ||
| ); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the hash-fragment provider-error case.
This only proves precedence for ?error=.... handleOauth2ProtocolUrl() has a separate #error=... parsing branch for implicit callbacks, so that regression can still slip through untested.
Suggested test
describe('error responses are handled before state validation', () => {
it('should reject with the provider error even if state is absent', () => {
registerOauth2AuthorizationRequest(resolve, reject, null, 'expected-state');
handleOauth2ProtocolUrl('bruno://oauth2/callback?error=access_denied');
expect(resolve).not.toHaveBeenCalled();
expect(reject).toHaveBeenCalledWith(
expect.objectContaining({ message: expect.stringContaining('Authorization Failed') })
);
});
+
+ it('should reject with the provider error from the hash fragment before state validation', () => {
+ registerOauth2AuthorizationRequest(resolve, reject, null, 'expected-state');
+
+ handleOauth2ProtocolUrl('bruno://oauth2/callback#error=access_denied');
+
+ expect(resolve).not.toHaveBeenCalled();
+ expect(reject).toHaveBeenCalledWith(
+ expect.objectContaining({ message: expect.stringContaining('Authorization Failed') })
+ );
+ });
});📝 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.
Suggested change
| describe('error responses are handled before state validation', () => { | |
| it('should reject with the provider error even if state is absent', () => { | |
| registerOauth2AuthorizationRequest(resolve, reject, null, 'expected-state'); | |
| handleOauth2ProtocolUrl('bruno://oauth2/callback?error=access_denied'); | |
| expect(resolve).not.toHaveBeenCalled(); | |
| expect(reject).toHaveBeenCalledWith( | |
| expect.objectContaining({ message: expect.stringContaining('Authorization Failed') }) | |
| ); | |
| }); | |
| }); | |
| describe('error responses are handled before state validation', () => { | |
| it('should reject with the provider error even if state is absent', () => { | |
| registerOauth2AuthorizationRequest(resolve, reject, null, 'expected-state'); | |
| handleOauth2ProtocolUrl('bruno://oauth2/callback?error=access_denied'); | |
| expect(resolve).not.toHaveBeenCalled(); | |
| expect(reject).toHaveBeenCalledWith( | |
| expect.objectContaining({ message: expect.stringContaining('Authorization Failed') }) | |
| ); | |
| }); | |
| it('should reject with the provider error from the hash fragment before state validation', () => { | |
| registerOauth2AuthorizationRequest(resolve, reject, null, 'expected-state'); | |
| handleOauth2ProtocolUrl('bruno://oauth2/callback#error=access_denied'); | |
| expect(resolve).not.toHaveBeenCalled(); | |
| expect(reject).toHaveBeenCalledWith( | |
| expect.objectContaining({ message: expect.stringContaining('Authorization Failed') }) | |
| ); | |
| }); | |
| }); |
🤖 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 `@packages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.js` around
lines 110 - 121, Add a test in oauth2-protocol-handler.spec.js that covers the
hash-fragment provider error path handled by handleOauth2ProtocolUrl when
parsing implicit callbacks. Reuse the existing
registerOauth2AuthorizationRequest and assert that a URL like
bruno://oauth2/callback#error=access_denied rejects with the provider error
before state validation, with resolve untouched and reject receiving an
Authorization Failed message. Ensure the new case sits alongside the existing
error-response precedence test so both ?error= and `#error`= branches are covered.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JIRA - https://usebruno.atlassian.net/browse/BRU-3546
Description
Bruno didn't validate the state returned on the OAuth2 callback, and sent none at all when the user left it blank — leaving auth flows open to CSRF / code injection.
Changes:
Always issue a state — random when unset, or a random nonce appended to the user's value so it can't be predicted/replayed.
Validate the returned state against the issued one and abort on mismatch, in both the embedded-window and system-browser.
Covers authorization code + implicit grants (query params and hash fragments).
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
New Features
statefor both authorization-code and implicit callbacks.stateis now preserved and combined with a cryptographically generated nonce.Bug Fixes
statedoesn’t match, the authorization flow is stopped and an explicit “state mismatch” message is shown.Tests & Fixtures