Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Check OAuth2 redirect URL for matching callback URL and authorization code in query parameters #2148

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: Check OAuth2 redirect URL for matching callback URL and authorization code in query parameters #2148

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
24753e8
Check OAuth2 redirect URL for matching callback URL and authorization…
dakshin-k Apr 21, 2024
906e4e4
Add test cases for callback URL check
dakshin-k Apr 22, 2024
d01d312
Update check to cover URLs with same host but different endpoints
dakshin-k Apr 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 10 additions & 6 deletions packages/bruno-electron/src/ipc/network/authorize-user-in-window.js
View file
Open in desktop
@@ -1,6 +1,10 @@
const { BrowserWindow } = require('electron');
const { preferencesUtil } = require('../../store/preferences');

const matchesCallbackUrl = (url, callbackUrl) => {
return url ? new URL(url).host == new URL(callbackUrl).host : false;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope: my callback url is pointing to the same server that is auth url. It will pass this check and fail on missing 'code' even before the first redirect.
Screenshot from 2024-04-22 23-48-42

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh man, there are lots of edge cases here 😅

Comparing the href as you suggested earlier will not work as the href includes any query params in the URL, i.e. while it solves the trailing / problem it breaks for the below case:

callbackUrl: https://callback.url/endpoint
testUrl       : https://callback.url/endpoint?code=abcd

// testUrl.href = https://callback.url/endpoint?code=abcd

This is the reason I compared against hostname instead of href. I currently don't have a better idea to implement this, will think about it and come back later.

Ideas are welcome if I'm missing something 😇

Copy link
Contributor

@lohit-1 lohit-1 Apr 23, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function could be something similar to this @dakshin-k

const matchesCallbackUrl = (url, callbackUrl) => {
  let { origin: urlOrigin, pathname: urlPathname } = new URL(url);
  let { origin: callbackUrlOrigin, pathname: callbackUrlPathname } = new URL(callbackUrl);

  urlPathname = urlPathname?.at(-1) == '/' ? urlPathname?.slice(0, -1) : urlPathname;
  callbackUrlPathname = callbackUrlPathname?.at(-1) == '/' ? callbackUrlPathname?.slice(0, -1) : callbackUrlPathname;

  const urlWithoutQueryParams = `${urlOrigin}${urlPathname}`;
  const callbackUrlWithoutQueryParams = `${callbackUrlOrigin}${callbackUrlPathname}`;

  return url ? urlWithoutQueryParams == callbackUrlWithoutQueryParams : false;
};

Edit:
Not recommended, seems like too much of an overkill
The below solution should be enough

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this suffice?
testUrl.href.startswith(callbackUrl.href)

The spec mandates the code param is appended the the query component, so it should be at the end. Also luckily the fragment component (the url part after #) is not allowed, so it does not have to be handled.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that worked! I have pushed the change, would someone mind testing it out once?

I faced a weird problem testing locally, not sure if I introduced a bug or found an existing one 😅

I'm able to generate auth tokens successfully now, but I can't make any requests - When I click the send request button, it just shows me the response from my OAuth server with the access token, it doesn't actually make a request using that token or display that response:

Screenshot 2024-04-23 at 9 59 51 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't mind that. That's actually #1999

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, thanks!

So I guess that's it on this PR? Let me know if there's anything further to do!


const authorizeUserInWindow = ({ authorizeUrl, callbackUrl, session }) => {
return new Promise(async (resolve, reject) => {
let finalUrl = null;
Expand Down Expand Up @@ -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(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=).*/)) {
Expand Down Expand Up @@ -93,4 +97,4 @@ const authorizeUserInWindow = ({ authorizeUrl, callbackUrl, session }) => {
});
};

module.exports = { authorizeUserInWindow };
module.exports = { authorizeUserInWindow, matchesCallbackUrl };
20 changes: 20 additions & 0 deletions packages/bruno-electron/tests/network/authorize-user.spec.js
View file
Open in desktop
@@ -0,0 +1,20 @@
const { matchesCallbackUrl } = require('../../src/ipc/network/authorize-user-in-window');

describe('matchesCallbackUrl', () => {
const testCases = [
{ url: null, expected: false },
{ url: '', expected: false },
{ 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 }
];

it.each(testCases)('$url - should be $expected', ({ url, expected }) => {
let callBackUrl = 'https://callback.url/endpoint';

let actual = matchesCallbackUrl(url, callBackUrl);

expect(actual).toBe(expected);
});
});