From 46df2e967fa4cd55758f36396eb3b54ebf808a3e Mon Sep 17 00:00:00 2001
From: Dakshin K <36289388+dakshin-k@users.noreply.github.com>
Date: Fri, 31 May 2024 15:41:31 +0530
Subject: [PATCH] fix: Check OAuth2 redirect URL for matching callback URL and
 authorization code in query parameters (#2148)

* Check OAuth2 redirect URL for matching callback URL and authorization code in query parameters

In an Authorization code flow, there may be multiple intermediate redirects before reaching the final one which matches the callback URL and has a code in the query params.

We should wait until we see a redirect URI that matches both the conditions. This fixes the issue where, when a redirect contains `code` as a query param but is not the final one (i.e., is not to the callback URL) an error is thrown saying the callback URL is invalid.

Fixes #2147

* Add test cases for callback URL check

* Update check to cover URLs with same host but different endpoints
---
 .../ipc/network/authorize-user-in-window.js   | 16 ++++++++++------
 .../tests/network/authorize-user.spec.js      | 19 +++++++++++++++++++
 2 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100644 packages/bruno-electron/tests/network/authorize-user.spec.js

diff --git a/packages/bruno-electron/src/ipc/network/authorize-user-in-window.js b/packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
index 3ed05d45c..8e4abf4bb 100644
--- a/packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
+++ b/packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
@@ -1,6 +1,10 @@
 const { BrowserWindow } = require('electron');
 const { preferencesUtil } = require('../../store/preferences');
 
+const matchesCallbackUrl = (url, callbackUrl) => {
+  return url ? url.href.startsWith(callbackUrl.href) : false;
+};
+
 const authorizeUserInWindow = ({ authorizeUrl, callbackUrl, session }) => {
   return new Promise(async (resolve, reject) => {
     let finalUrl = null;
@@ -30,12 +34,12 @@ const authorizeUserInWindow = ({ authorizeUrl, callbackUrl, session }) => {
     });
 
     function onWindowRedirect(url) {
-      // check if the url contains an authorization code
-      if (new URL(url).searchParams.has('code')) {
-        finalUrl = url;
-        if (!url || !finalUrl.includes(callbackUrl)) {
-          reject(new Error('Invalid Callback Url'));
+      // check if the redirect is to the callback URL and if it contains an authorization code
+      if (matchesCallbackUrl(new URL(url), new URL(callbackUrl))) {
+        if (!new URL(url).searchParams.has('code')) {
+          reject(new Error('Invalid Callback URL: Does not contain an authorization code'));
         }
+        finalUrl = url;
         window.close();
       }
       if (url.match(/(error=).*/) || url.match(/(error_description=).*/) || url.match(/(error_uri=).*/)) {
@@ -93,4 +97,4 @@ const authorizeUserInWindow = ({ authorizeUrl, callbackUrl, session }) => {
   });
 };
 
-module.exports = { authorizeUserInWindow };
+module.exports = { authorizeUserInWindow, matchesCallbackUrl };
diff --git a/packages/bruno-electron/tests/network/authorize-user.spec.js b/packages/bruno-electron/tests/network/authorize-user.spec.js
new file mode 100644
index 000000000..03c76cfb8
--- /dev/null
+++ b/packages/bruno-electron/tests/network/authorize-user.spec.js
@@ -0,0 +1,19 @@
+const { matchesCallbackUrl } = require('../../src/ipc/network/authorize-user-in-window');
+
+describe('matchesCallbackUrl', () => {
+  const testCases = [
+    { url: 'https://random-url/endpoint', expected: false },
+    { url: 'https://random-url/endpoint?code=abcd', expected: false },
+    { url: 'https://callback.url/endpoint?code=abcd', expected: true },
+    { url: 'https://callback.url/endpoint/?code=abcd', expected: true },
+    { url: 'https://callback.url/random-endpoint/?code=abcd', expected: false }
+  ];
+
+  it.each(testCases)('$url - should be $expected', ({ url, expected }) => {
+    let callBackUrl = 'https://callback.url/endpoint';
+
+    let actual = matchesCallbackUrl(new URL(url), new URL(callBackUrl));
+
+    expect(actual).toBe(expected);
+  });
+});