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.

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

feat: add check for at signs in redirect url #3510

Merged
merged 2 commits into from
May 28, 2023

Conversation

davidsoderberg
Copy link
Contributor

Fixing the redirect vulnerability in github oauth endpoint.

@linear
Copy link

linear bot commented May 26, 2023

NV-2228 Open redirect vulnerability

Description

An open redirect vulnerability exists in `/v1/auth/github/callback`. 
An attacker can exploit the vulnerability by tricking a victim to click the malicious URL.
A successful exploit allows the attacker to log into novu as the victim.

The vulnerable code: `https://github.com/novuhq/novu/blob/5f94d65b1ab8d618aed9768a6e2877a1614e10d8/apps/api/src/app/auth/auth.controller.ts#L94-L96`.

Steps to reproduce

Prerequisite: Enable Sign In with GitHub functionality.

0. [Attacker] Send the following URL to the victim: `https://<novu_api>/v1/auth/github?source=web&redirectUrl=http://localhost:pass<attacker_server>/`
1. [Victim] Open the URL in your web browser.
2. [Victim] Log into GitHub if not.
3. [Victim] You will be redirected to `https://<novu_api>/v1/auth/github/callback?code=&state=%7B%22source%22%3A%22web%22%2C%22redirectUrl%22%3A%22http%3A%2F%2Flocalhost%3Apass%40<attacker_server>%2F%22%7D` in turn `http://localhost:pass<attacker_server>/?token=<JWT_token>`.
4. [Attacker] Get the `<JWT_token>` value sent by the victim on step 3 by inspecting the server's log.
5. [Attacker] Open the following URL in your web browser: `https://<novu_web>/auth/login?token=<JWT_token>`
6. [Attacker] Confirm you logged in as the victim.

@@ -91,7 +91,7 @@ export class AuthController {
* Make sure we only allow localhost redirects for CLI use and our own success route
* https://github.com/novuhq/novu/security/code-scanning/3
*/
if (redirectUrl && redirectUrl.startsWith('http://localhost:')) {
if (redirectUrl && redirectUrl.startsWith('http://localhost:') && !redirectUrl.includes('@')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we missing the check for the question mark? Based on the PR title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Me and Dima discussed and said it was not needed after I had created the pr :)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it was not needed in the end :) I see David updated the title

@davidsoderberg davidsoderberg changed the title feat: add check for at signs and question marks in redirect url feat: add check for at signs in redirect url May 28, 2023
@scopsy scopsy added this pull request to the merge queue May 28, 2023
Merged via the queue into next with commit 7168ed7 May 28, 2023
20 checks passed
@scopsy scopsy deleted the nv-2228-open-redirect-vulnerability branch May 28, 2023 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants