fix(oauth2): prevent code injection in OAuth2 callback handling

[abhishekp-bruno]

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:

• I've used AI significantly to create this pull request
• The pull request only addresses one issue or adds one feature.
• The pull request does not introduce any breaking changes
• I have added screenshots or gifs to help explain the change if applicable.
• I have read the contribution guidelines.
• Create an issue and link to the pull request.

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
  • Enhanced OAuth2 security by validating callback state for both authorization-code and implicit flows, with nonce-protected state handling.
• Bug Fixes
  • OAuth2 fetch/refresh errors now reliably appear in the response pane (in addition to notifications), with clearer error details.
• Tests
  • Added unit and E2E coverage for state match/mismatch scenarios (query + hash) and introduced new OAuth2 state-related test fixtures.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This PR adds OAuth2 state generation and callback validation across browser and protocol-handler flows, surfaces OAuth2 failures in the response pane, and expands unit/E2E coverage with new Bruno fixtures and Electron state-validation tests.

Changes

OAuth2 State Validation

Layer / File(s)	Summary
State generation and flow wiring packages/bruno-electron/src/utils/oauth2.js, packages/bruno-electron/src/ipc/network/authorize-user-in-system-browser.js, packages/bruno-electron/src/ipc/network/authorize-user-in-window.js	Adds generateState, appends computed state to authorization URLs, and forwards expectedState into the browser/window authorization helpers.
Callback state validation packages/bruno-electron/src/utils/oauth2-protocol-handler.js, packages/bruno-electron/src/ipc/network/authorize-user-in-window.js	Stores expectedState, reads callback params from query or hash, and rejects mismatched returned state with OAuth2 state mismatch.
OAuth2 error surfacing packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.js	Dispatches OAuth2 failures into the Redux response pane and switches to the response tab.
Unit and E2E coverage packages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.js, tests/auth/oauth2/oauth2-state-validation.spec.ts, tests/auth/oauth2/fixtures/collection/*, tests/auth/oauth2/init-user-data/preferences.json	Adds protocol-handler tests, Playwright/Electron state-validation coverage, and supporting Bruno fixtures and user data.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

• usebruno/bruno#6101: Threads OAuth2 expectedState through the same system-browser and protocol-handler paths, overlapping these authorization and callback checks.

Suggested labels: size/XXL

Suggested reviewers: helloanoop, lohit-bruno, bijin-bruno, naman-bruno

Poem

A nonce takes flight, a callback sings,
State checks guard the OAuth wings.
If hashes drift or tokens stray,
The pane lights up, the tests say “nay.”
Bruno hums a safer tune 🎵

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main fix: hardening OAuth2 callback handling against injection via state validation.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 win

Reserve state instead of appending a duplicate parameter.

If authorizationUrl or additionalParameters.authorization already contains state, append() sends duplicate state params. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 1c9355e and 8fc0800.

📒 Files selected for processing (13)

• packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.js
• packages/bruno-electron/src/ipc/network/authorize-user-in-system-browser.js
• packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
• packages/bruno-electron/src/utils/oauth2-protocol-handler.js
• packages/bruno-electron/src/utils/oauth2.js
• packages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.js
• tests/auth/oauth2/fixtures/collection/Authorization Code.bru
• tests/auth/oauth2/fixtures/collection/Implicit.bru
• tests/auth/oauth2/fixtures/collection/User Supplied State.bru
• tests/auth/oauth2/fixtures/collection/bruno.json
• tests/auth/oauth2/fixtures/collection/environments/Local.bru
• tests/auth/oauth2/init-user-data/preferences.json
• tests/auth/oauth2/oauth2-state-validation.spec.ts

[coderabbitai]

🧹 Nitpick comments (1)

tests/auth/oauth2/oauth2-state-validation.spec.ts (1)

219-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove leftover commented-out test.step wrapper.

Every sibling test wraps the "start the flow" phase in test.step, but this one has it commented out while the body still runs unwrapped — looks like a debugging leftover.

🧹 Proposed fix

-    // await test.step('start the authorization code flow', async () => {
-    await stubOpenExternal(app);
-    await installCallbackCapture(app);
-    await clickGetAccessToken(page, 'AuthCodeUserSuppliedState');
-    await waitForAuthorizationStarted(app);
-    // });
+    await test.step('start the authorization code flow', async () => {
+      await stubOpenExternal(app);
+      await installCallbackCapture(app);
+      await clickGetAccessToken(page, 'AuthCodeUserSuppliedState');
+      await waitForAuthorizationStarted(app);
+    });

🤖 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 `@tests/auth/oauth2/oauth2-state-validation.spec.ts` around lines 219 - 224,
Remove the leftover commented-out test.step wrapper around the authorization
flow setup in the oauth2-state-validation test; the body currently runs
unwrapped while the same phase in sibling tests uses test.step. Clean up the
AuthCodeUserSuppliedState flow by deleting the commented wrapper and keeping the
existing calls to stubOpenExternal, installCallbackCapture, clickGetAccessToken,
and waitForAuthorizationStarted directly in the test body.

Source: Path instructions

🤖 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.

Nitpick comments:
In `@tests/auth/oauth2/oauth2-state-validation.spec.ts`:
- Around line 219-224: Remove the leftover commented-out test.step wrapper
around the authorization flow setup in the oauth2-state-validation test; the
body currently runs unwrapped while the same phase in sibling tests uses
test.step. Clean up the AuthCodeUserSuppliedState flow by deleting the commented
wrapper and keeping the existing calls to stubOpenExternal,
installCallbackCapture, clickGetAccessToken, and waitForAuthorizationStarted
directly in the test body.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2d65b069-cd41-4ed0-a127-73d792006a59

📥 Commits

Reviewing files that changed from the base of the PR and between aa527e0 and f21b06e.

📒 Files selected for processing (9)

• packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.js
• packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
• packages/bruno-electron/src/utils/oauth2-protocol-handler.js
• packages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.js
• tests/auth/oauth2/fixtures/collection/AuthCodeUserSuppliedState.bru
• tests/auth/oauth2/fixtures/collection/AuthorizationCode.bru
• tests/auth/oauth2/fixtures/collection/AuthorizationImplicit.bru
• tests/auth/oauth2/fixtures/collection/ImplicitUserSuppliedState.bru
• tests/auth/oauth2/oauth2-state-validation.spec.ts

💤 Files with no reviewable changes (1)

• packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.js

✅ Files skipped from review due to trivial changes (1)

• tests/auth/oauth2/fixtures/collection/ImplicitUserSuppliedState.bru

🚧 Files skipped from review as they are similar to previous changes (3)

• packages/bruno-electron/src/utils/oauth2-protocol-handler.js
• packages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.js
• packages/bruno-electron/src/ipc/network/authorize-user-in-window.js

[coderabbitai]

🧹 Nitpick comments (1)

packages/bruno-electron/src/utils/oauth2-protocol-handler.js (1)

101-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep the rejection call on one line.

Line 101 splits a single-argument function call across multiple lines; this violates the JS style rule. As per coding guidelines, “No newlines inside function parentheses.”

Proposed fix

-        rejectOauth2AuthorizationRequest(
-          new Error('OAuth2 state mismatch')
-        );
+        rejectOauth2AuthorizationRequest(new Error('OAuth2 state mismatch'));

🤖 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-protocol-handler.js` around lines
101 - 103, The rejection call in rejectOauth2AuthorizationRequest should be kept
on a single line to match the no-newlines-inside-parentheses style rule. Update
the OAuth2 state mismatch branch in oauth2-protocol-handler.js so the new
Error('OAuth2 state mismatch') argument is passed inline to
rejectOauth2AuthorizationRequest without splitting the function call across
multiple lines.

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@packages/bruno-electron/src/utils/oauth2-protocol-handler.js`:
- Around line 101-103: The rejection call in rejectOauth2AuthorizationRequest
should be kept on a single line to match the no-newlines-inside-parentheses
style rule. Update the OAuth2 state mismatch branch in
oauth2-protocol-handler.js so the new Error('OAuth2 state mismatch') argument is
passed inline to rejectOauth2AuthorizationRequest without splitting the function
call across multiple lines.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f208fb70-5963-462c-b788-aa2e5a9be4ca

📥 Commits

Reviewing files that changed from the base of the PR and between f21b06e and 69aad54.

📒 Files selected for processing (14)

• packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.js
• packages/bruno-electron/src/ipc/network/authorize-user-in-system-browser.js
• packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
• packages/bruno-electron/src/utils/oauth2-protocol-handler.js
• packages/bruno-electron/src/utils/oauth2.js
• packages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.js
• tests/auth/oauth2/fixtures/collection/AuthCodeUserSuppliedState.bru
• tests/auth/oauth2/fixtures/collection/AuthorizationCode.bru
• tests/auth/oauth2/fixtures/collection/AuthorizationImplicit.bru
• tests/auth/oauth2/fixtures/collection/ImplicitUserSuppliedState.bru
• tests/auth/oauth2/fixtures/collection/bruno.json
• tests/auth/oauth2/fixtures/collection/environments/Local.bru
• tests/auth/oauth2/init-user-data/preferences.json
• tests/auth/oauth2/oauth2-state-validation.spec.ts

✅ Files skipped from review due to trivial changes (3)

• tests/auth/oauth2/fixtures/collection/environments/Local.bru
• tests/auth/oauth2/fixtures/collection/bruno.json
• tests/auth/oauth2/init-user-data/preferences.json

🚧 Files skipped from review as they are similar to previous changes (10)

• tests/auth/oauth2/fixtures/collection/AuthorizationImplicit.bru
• tests/auth/oauth2/fixtures/collection/ImplicitUserSuppliedState.bru
• packages/bruno-electron/src/ipc/network/authorize-user-in-system-browser.js
• tests/auth/oauth2/fixtures/collection/AuthorizationCode.bru
• tests/auth/oauth2/fixtures/collection/AuthCodeUserSuppliedState.bru
• packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
• packages/bruno-app/src/components/RequestPane/Auth/OAuth2/Oauth2ActionButtons/index.js
• packages/bruno-electron/tests/utils/oauth2-protocol-handler.spec.js
• packages/bruno-electron/src/utils/oauth2.js
• tests/auth/oauth2/oauth2-state-validation.spec.ts

[sanish-bruno]

@lohit-bruno need your eyes here!