omgnetwork / ewallet

eWallet Backend for the OmiseGO SDKs.
https://omisego.network/
Apache License 2.0
324 stars 74 forks source link

isAbsoluteUrl allows any scheme #599

Closed jpopxfile closed 5 years ago

jpopxfile commented 5 years ago

Subject of the issue

A very general regex is used for checking if a string is an absolute url. isAbsoluteUrl allows any scheme (e.g. javascript://)

Your environment

https://github.com/omisego/ewallet/blob/e22934a98feb8e232a344c2f5bc90d0dea5a91af/apps/admin_panel/assets/src/utils/url.js#L28

Steps to reproduce

function isAbsoluteURL (url) {
  return /^([a-z][a-z\d+\-.]*:)?\/\//i.test(url)
};

isAbsoluteURL("javascript://%0d%0aalert(1)")
true

Expected behavior

I am not sure what expected behavior is supposed to be, but it might be a good idea to whitelist schemes to only allow ws, wss, http and https. If I am wrong about this, please close this issue.

Actual behavior

Currently, only https://github.com/omisego/ewallet/blob/master/apps/admin_panel/assets/src/socket/connector.js#L32 is using this function for the websocket connection.

However, as this is a utils function, there might be the slight chance that it could be used in the future in other portions of code.

An example of how it can be used dangerously:

var url = "javascript://%0d%0aalert(1)";
if (isAbsoluteURL(url)){
    window.location = url;
}
jarindr commented 5 years ago

Hello @jpopxfile thank you so much for your concern. Yes, that would definitely help if the function is used somewhere risky from user's input. We will keep this in mind for sure.

I will keep this issue open first, feel free to open a PR if you willing to :D

jpopxfile commented 5 years ago

Have done a PR, not sure if it's compliant to Omisego's coding standards, but please take a look and give me some feedback. Thanks!