Improving the urls to not break protocols and adding tests

[iifawzi]

Hi, This's my first contribution to this great project, and hopefully not the last one.
This PR should fix #3983
I've also added a bunch of tests, testing the urls utils.

Signed-off-by: iifawzi iifawzie@gmail.com

[CLAassistant]

All committers have signed the CLA.

[iifawzi]

This's my first contribution, I might not be aware of the folder structure decisions that was taken while ago, but I think it would make the code more organized if all the tests are under a separate directory, test for example. Even if not all the tests under a single main test directory, a sub-folder test in each folder would work too, like having all the urls tests in test folder under the utils directory.

[apoorv-mishra]

Just a few code style nitpicks!

[vn-ncvinh]

@tommoor , I'm aware of a few possible XSS vulnerabilities here.

In the file urls.ts contains 2 code snippets with vulnerabilities.

export function isUrl(text: string) {
  if (text.match(/\n/)) {
    return false;
  }

  try {
    const url = new URL(text);
    return url.hostname !== "";
  } catch (err) {
    return false;
  }
}

export function sanitizeUrl(url: string | null | undefined) {
  if (!url) {
    return undefined;
  }

  if (
    (!isUrl(url) &&
      !url.startsWith("/") &&
      !url.startsWith("#") &&
      !url.startsWith("mailto:")) ||
    url.startsWith("javascript:") ||
    url.startsWith("file:") ||
    url.startsWith("vbscript:") ||
    url.startsWith("data:")
  ) {
    return `https://${url}`;
  }
  return url;
}

An attacker can easily bypass the filter to generate a url containing the XSS attack payload.

jAvascript://%0Aalert(origin)

jAvascript://: will fool and pass 2 test functions.

%0A%0Dalert(origin): Execute command alert origin.

Solution

Use filter by the URL protocol

var blacklist_protocol = ['javascript:', 'file:', 'vbscript:', 'data:']
export function isUrl(text: string) {
  if (text.match(/\n/)) {
    return false;
  }
  
  try {
    const url = new URL(text);
    return url.hostname !== "" && !blacklist_protocol.includes(url.protocol);
  } catch (err) {
    return false;
  }
}

export function sanitizeUrl(url: string | null | undefined) {
  if (!url) {
    return undefined;
  }

  if (
    (!isUrl(url) &&
      !url.startsWith("/") &&
      !url.startsWith("#") &&
      !url.startsWith("mailto:"))
  ) {
    return `https://${url}`;
  }
  return url;
}

[iifawzi]

Nice catch @vn-ncvinh, it's way better this way. Anyway, if we want to stick with the startsWith way, we can lowercase the urls before checking them.

[vn-ncvinh]

Nice catch @vn-ncvinh, it's way better this way. Anyway, if we want to stick with the startsWith way, we can lowercase the urls before checking them.

If using lowercase, there is still a way to bypass it by using special characters.

[apoorv-mishra]

Interesting. Thanks for pointing this out 🙏

Maybe we should replace these with the existing validator module that we already use on server-side? But I wonder why isn't it used already 🤔 To keep the bundle size low @tommoor ?

[iifawzi]

Ahh I see, thank you @vn-ncvinh for clarifying this, my bad.

[tommoor]

I don't think the validator module is doing anything too special, in fact it may also be vulnerable to the use of special characters as a bypass based on this…

https://github.com/validatorjs/validator.js/blob/master/src/lib/isURL.js#L84

[apoorv-mishra]

Plug: Seems like mermaid uses this.

[vn-ncvinh]

I think the URL of javascript works quite well, we just need to add a blacklist of protocols.
For urls containing special characters, it will be automatically removed.
But maybe in the future there will be protocol spoofing vulnerability, but that's javascript problem, and we don't need to care about it right now.

[tommoor]

Isn't this fun 😆

[apoorv-mishra]

I'm actually surprised why none of the third party validators leverage new URL() too as part of the validation flow 🤔

[vn-ncvinh]

I don't know about that either.
But maybe when they build a new validator function, they will avoid using the built-in validator functions.

[vn-ncvinh]

I think should add tel:, fax:, sms:... or some other alternative link

[tommoor]

I think should add tel:, fax:, sms:... or some other alternative link

Not sure what you mean here?

[vn-ncvinh]

I think should add tel:, fax:, sms:... or some other alternative link

Not sure what you mean here?

These will make it easier to the contact methods. It is similar to the mailto: that was added earlier.

• tel:0123456789 - Make a call to the phone number 0123456789
• fax:0123456789 - Make a fax to 0123456789
• sms:0123456789 - Make new SMS to phone number 0123456789

[apoorv-mishra]

I'm thinking, maybe instead of having a blacklist, we should have allowed_protocols? Because, the blacklist above might still be bypassed by something like new URL("somerandomprotocol://malicious.something"). We're more confident about what we want to allow than what we don't.

[iifawzi]

Although this will be more restrictive it would make more sense to be implemented this way, and legitimate protocols can be added by the time, when people notice it, through the issues/PRs.

instead of only sanitizing the forbidden protocols and forgetting some.

[vn-ncvinh]

whether to use blacklist or whitelist depends on each specific case. We will prioritize using what is supposed to be the minority, and here is the blacklist. Of the multitude of protocols available, only a few are considered dangerous, and it is easier to list them than to list the rest.

[iifawzi]

I think the subsequences of forgetting any dangerous protocol are more dangerous than forgetting any allowed protocol, in the later case, people will notice them easily and will be added accordingly

[vn-ncvinh]

So far, only 4 protocols have been deemed dangerous, which can lead to xss.

[tommoor]

@iifawzi in the aim of maintainability let's keep going with the blocklist approach, I don't want to be fielding PR's for eternity for every desktop app possible.