Skip to content

Potential fix for code scanning alert no. 83: Client-side URL redirect #1133

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Potential fix for code scanning alert no. 83: Client-side URL redirect #1133

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 24 additions & 27 deletions src/javascript/app/pages/callback/callback.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import Client from '../../base/client';
import BinarySocket from '../../base/socket';
import GTM from '../../../_common/base/gtm';
import { get as getLanguage, urlLang } from '../../../_common/language';

Check failure on line 11 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

'urlLang' is defined but never used

Check failure on line 11 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

'urlLang' is defined but never used
import { isStorageSupported, removeCookies } from '../../../_common/storage';
import { urlFor } from '../../../_common/url';
import { getPropertyValue } from '../../../_common/utility';
Expand Down Expand Up @@ -84,36 +84,33 @@

// redirect back
let set_default = true;
if (redirect_url) {
const do_not_redirect = [
'reset_passwordws',
'lost_passwordws',
'change_passwordws',
'home',
'404',
];
const reg = new RegExp(do_not_redirect.join('|'), 'i');
if (!reg.test(redirect_url) && urlFor('') !== redirect_url) {
set_default = false;
}
const trusted_urls = [
urlFor('user/metatrader'),
Client.defaultRedirectUrl(),
urlFor('home'),
];

if (redirect_url && trusted_urls.includes(redirect_url)) {
set_default = false;
}

if (set_default) {
const lang_cookie = urlLang(redirect_url) || Cookies.get('language');
const lang_cookie = Cookies.get('language') || getLanguage();
const language = getLanguage();
redirect_url =
Client.isAccountOfType('financial') || Client.isOptionsBlocked()
? urlFor('user/metatrader')
: Client.defaultRedirectUrl();
if (lang_cookie && lang_cookie !== language) {
redirect_url = redirect_url.replace(
new RegExp(`/${language}/`, 'i'),
`/${lang_cookie.toLowerCase()}/`
);
}
}
getElementById('loading_link').setAttribute('href', redirect_url);
window.location.replace(redirect_url); // need to redirect not using pjax
redirect_url =

Check failure on line 100 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 16 spaces but found 15

Check failure on line 100 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 16 spaces but found 15
Client.isAccountOfType('financial') || Client.isOptionsBlocked()
? urlFor('user/metatrader')
: Client.defaultRedirectUrl();
if (lang_cookie && lang_cookie !== language) {

Check failure on line 104 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 16 spaces but found 15

Check failure on line 104 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 16 spaces but found 15
redirect_url = redirect_url.replace(

Check failure on line 105 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 20 spaces but found 19

Check failure on line 105 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 20 spaces but found 19
new RegExp(`/${language}/`, 'i'),

Check failure on line 106 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 24 spaces but found 23

Check failure on line 106 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 24 spaces but found 23
`/${lang_cookie.toLowerCase()}/`

Check failure on line 107 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 24 spaces but found 23

Check failure on line 107 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 24 spaces but found 23
);

Check failure on line 108 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 20 spaces but found 19

Check failure on line 108 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 20 spaces but found 19
}

Check failure on line 109 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 16 spaces but found 15

Check failure on line 109 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 16 spaces but found 15
}

Check failure on line 110 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 12 spaces but found 11

Check failure on line 110 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 12 spaces but found 11
getElementById('loading_link').setAttribute('href', redirect_url);

Check failure on line 111 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / Build and Test 

Expected indentation of 12 spaces but found 11

Check failure on line 111 in src/javascript/app/pages/callback/callback.jsx

View workflow job for this annotation

GitHub Actions / build_and_deploy_preview_link 

Expected indentation of 12 spaces but found 11

Check warning

Code scanning / CodeQL

Client-side URL redirect Medium

Untrusted URL redirection depends on a
user-provided value
Loading
.
Untrusted URL redirection depends on a
user-provided value
Loading
.
Untrusted URL redirection depends on a
user-provided value
Loading
.

Copilot Autofix

AI 17 days ago

To fix the issue, we need to ensure that redirect_url is always validated against a list of trusted URLs or domains before being used in window.location.replace. This involves:

  1. Maintaining a strict list of allowed URLs or domains.
  2. Validating redirect_url against this list, even when it is modified based on language settings or other parameters.
  3. Rejecting or defaulting to a safe URL if redirect_url does not pass validation.

The changes will be made in src/javascript/app/pages/callback/callback.jsx to ensure that redirect_url is validated before redirection.

Suggested changeset 1
src/javascript/app/pages/callback/callback.jsx

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/javascript/app/pages/callback/callback.jsx b/src/javascript/app/pages/callback/callback.jsx
--- a/src/javascript/app/pages/callback/callback.jsx
+++ b/src/javascript/app/pages/callback/callback.jsx
@@ -85,10 +85,13 @@
             // redirect back
-            let set_default = true;
-            const trusted_urls = [
-                urlFor('user/metatrader'),
-                Client.defaultRedirectUrl(),
-                urlFor('home'),
-            ];
+            const isTrustedUrl = (url) => {
+                const trusted_urls = [
+                    urlFor('user/metatrader'),
+                    Client.defaultRedirectUrl(),
+                    urlFor('home'),
+                ];
+                return trusted_urls.includes(url);
+            };
 
-            if (redirect_url && trusted_urls.includes(redirect_url)) {
+            let set_default = true;
+            if (redirect_url && isTrustedUrl(redirect_url)) {
                 set_default = false;
@@ -96,5 +99,5 @@
 
-            if (set_default) {
-                const lang_cookie = Cookies.get('language') || getLanguage();
-                const language = getLanguage();
+           if (set_default) {
+               const lang_cookie = Cookies.get('language') || getLanguage();
+               const language = getLanguage();
                redirect_url =
@@ -110,2 +113,7 @@
            }
+
+           if (!isTrustedUrl(redirect_url)) {
+               redirect_url = Client.defaultRedirectUrl();
+           }
+
            getElementById('loading_link').setAttribute('href', redirect_url);
EOF
@@ -85,10 +85,13 @@
// redirect back
let set_default = true;
const trusted_urls = [
urlFor('user/metatrader'),
Client.defaultRedirectUrl(),
urlFor('home'),
];
const isTrustedUrl = (url) => {
const trusted_urls = [
urlFor('user/metatrader'),
Client.defaultRedirectUrl(),
urlFor('home'),
];
return trusted_urls.includes(url);
};

if (redirect_url && trusted_urls.includes(redirect_url)) {
let set_default = true;
if (redirect_url && isTrustedUrl(redirect_url)) {
set_default = false;
@@ -96,5 +99,5 @@

if (set_default) {
const lang_cookie = Cookies.get('language') || getLanguage();
const language = getLanguage();
if (set_default) {
const lang_cookie = Cookies.get('language') || getLanguage();
const language = getLanguage();
redirect_url =
@@ -110,2 +113,7 @@
}

if (!isTrustedUrl(redirect_url)) {
redirect_url = Client.defaultRedirectUrl();
}

getElementById('loading_link').setAttribute('href', redirect_url);
Copilot is powered by AI and may make mistakes. Always verify output.

window.location.replace(redirect_url); // need to redirect not using pjax

Check warning

Code scanning / CodeQL

Client-side URL redirect Medium

Untrusted URL redirection depends on a
user-provided value
Loading
.
Untrusted URL redirection depends on a
user-provided value
Loading
.
Untrusted URL redirection depends on a
user-provided value
Loading
.

Copilot Autofix

AI 17 days ago

To fix the issue, we need to ensure that the redirect_url is validated against a strict list of trusted URLs and does not rely on user-controlled input. This can be achieved by maintaining a whitelist of authorized redirect paths and validating redirect_url against this whitelist. If the URL is not in the whitelist, the code should default to a safe URL.

Steps to implement the fix:

  1. Replace the trusted_urls array with a whitelist of authorized paths (e.g., /user/metatrader, /home).
  2. Extract the path from redirect_url and validate it against the whitelist.
  3. If the path is not in the whitelist, default to a safe URL.
  4. Ensure that the validation logic is robust and accounts for edge cases.
Suggested changeset 1
src/javascript/app/pages/callback/callback.jsx

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/src/javascript/app/pages/callback/callback.jsx b/src/javascript/app/pages/callback/callback.jsx
--- a/src/javascript/app/pages/callback/callback.jsx
+++ b/src/javascript/app/pages/callback/callback.jsx
@@ -86,9 +86,6 @@
             let set_default = true;
-            const trusted_urls = [
-                urlFor('user/metatrader'),
-                Client.defaultRedirectUrl(),
-                urlFor('home'),
-            ];
+            const trusted_paths = ['/user/metatrader', '/home'];
+            const url_path = new URL(redirect_url, window.location.origin).pathname;
 
-            if (redirect_url && trusted_urls.includes(redirect_url)) {
+            if (redirect_url && trusted_paths.includes(url_path)) {
                 set_default = false;
@@ -99,5 +96,5 @@
                 const language = getLanguage();
-               redirect_url =
-                   Client.isAccountOfType('financial') || Client.isOptionsBlocked()
-                       ? urlFor('user/metatrader')
+                redirect_url =
+                    Client.isAccountOfType('financial') || Client.isOptionsBlocked()
+                        ? urlFor('user/metatrader')
                        : Client.defaultRedirectUrl();
EOF
@@ -86,9 +86,6 @@
let set_default = true;
const trusted_urls = [
urlFor('user/metatrader'),
Client.defaultRedirectUrl(),
urlFor('home'),
];
const trusted_paths = ['/user/metatrader', '/home'];
const url_path = new URL(redirect_url, window.location.origin).pathname;

if (redirect_url && trusted_urls.includes(redirect_url)) {
if (redirect_url && trusted_paths.includes(url_path)) {
set_default = false;
@@ -99,5 +96,5 @@
const language = getLanguage();
redirect_url =
Client.isAccountOfType('financial') || Client.isOptionsBlocked()
? urlFor('user/metatrader')
redirect_url =
Client.isAccountOfType('financial') || Client.isOptionsBlocked()
? urlFor('user/metatrader')
: Client.defaultRedirectUrl();
Copilot is powered by AI and may make mistakes. Always verify output.
});
};

Expand Down