Skip to content

Draft: Extend `isSafeUrl` util to allow unknown protocols

Dheeraj Joshi requested to merge djadmin-safe-url-utils into master

Related issue: #409876

What does this MR do and why?

Background

Currently, the isSafeUrl utility restricts the use of unknown protocols in URLs, ensuring that only safe and whitelisted protocols (http:, https:) are allowed. However, there is a need to extend this utility to allow unknown protocols (like slack://) while still excluding potentially vulnerable protocols.

This requirement is actually relevant for GlLink and GlButton (perhaps the SafeLinkDirective) because most of the links are rendered with GitLab UI components. But to maintain parity, this proposal aims to introduce the necessary changes here and eventually extend them to gitlab-ui once approved from a security standpoint.

Proposal

We are adding a new argument called options to the isSafeUrl function. This argument allows for the inclusion of the allowUnknownProtocols option, and potentially other options in the future to make things safe-by-default.

Protocols Considered Potentially Vulnerable:

The list of potentially vulnerable protocols based on DOMPurify and sanitize-url are:

  1. javascript:
  2. data:
  3. vbscript:

Benefits

Improved security posture: At the moment, if we need to allow unknown protocols with GlLink, we need to add is-unsafe-href attribute. This is risky as it does allow everything, including the potentially vulnerable javascript: protocols. So by explicitly allowing unknown protocols while blocking potentially vulnerable ones, we can enhance the security posture of our components, reducing the risk of malicious attacks.

Note

We eventually want to deprecate is-unsafe-href prop in favour of providing explicit options as proposed in this MR. Here's the issue for investigation its usage in the codebase: #410457

Open-questions

  1. Absolute or Root-Relative URLs: The requirement for Safe URLs to be always absolute or root-relative has been temporarily removed. However, further investigation is needed to determine if there are any issues or concerns with modern browsers that we support by removing this requirement.

  2. Protection Against Path Traversal Attacks: The proposal currently does not include explicit protections against path traversal attacks, such as URLs containing ../../../../etc/host. It is suggested to accept such URLs as safe within the frontend JavaScript context. If additional protections are required, it should be discussed further.

Screenshots or screen recordings

N/A

How to set up and validate locally

N/A

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Dheeraj Joshi

Merge request reports