Allow safeProtocol to be passed as a parameter for defaultUrlTransform

[janpe]

Initial checklist

• I read the support docs
• I read the contributing guide
• I agree to follow the code of conduct
• I searched issues and discussions and couldn’t find anything (or linked relevant results below)

Problem

Hello! I'd like to add tel to the allowed protocols but not deviate so much from defaultUrlTransform to write my own

Current solutions

Writing your own urlTransform. I don't prefer this since I'd like to follow the default even if changes in future versions.

Proposed solutions

I'd like to propose that the safeProtocol value used in defaultUrlTransform could be passed as a parameter

[wooorm]

I don't prefer this since I'd like to follow the default even if changes in future versions.

You can write something like urlTransform(value) { return /^tel:/i.test(value) ? undefined : defaultUrlTransform(value) }?

I don’t understand why this is needed? What does this give?

[janpe]

I don't prefer this since I'd like to follow the default even if changes in future versions.

You can write something like urlTransform(value) { return /^tel:/i.test(value) ? undefined : defaultUrlTransform(value) }?

I don’t understand why this is needed? What does this give?

The reason I don't want to wrap defaultUrlTransform is that it does more than match a protocol — a /^tel:/.test(value) wrapper skips the rest of its sanitization logic, so it's less safe and I'd have to keep it in sync by hand if that logic ever changes. I just want to change the allowed-protocol list while keeping everything else (and future fixes) as-is.

I'm doing that by adding an optional safeProtocol arg to defaultUrlTransform, but that's a bit awkward since urlTransform is called as (url, key, node) and the extra positional arg collides with that signature (hence the wrapper at the call site).

A createUrlTransform(safeProtocol?) factory might be cleaner — same idea as #943. It returns a plain (url) => string and keeps defaultUrlTransform single-arg:

import Markdown, {createUrlTransform} from 'react-markdown'

const urlTransform = createUrlTransform(/^(https?|ircs?|mailto|xmpp|tel)$/i)

Happy to reshape it to whichever you prefer. Allowing protocols like tel: is a real need for a lot of apps, and right now the only path is to reimplement the sanitizer — which is exactly the footgun this avoids. Could we land one of these shapes?

[remcohaszing]

I’m inclined to say urlTransform is sufficient. You can write something like this:

const regex = /^(https?|ircs?|mailto|xmpp|tel):/i
function urlTransform(value) {
  if (!URL.canParse(value)) {
    return value
  }

  if (regex.test(value)) {
    return value
  }

  return ''
}

[janpe]

Thanks @remcohaszing. I'd still like to make the case for a small built-in option, if you're open to it.

If I'm reading the history right, tel: was actually a default-allowed protocol until v9.0.0 and was dropped to align with GitHub's allow-list — is that the main reason, or was there a safety concern too?

My hesitation with urlTransform is the cost of extending it for a single additive protocol: I'd have to either reimplement the sanitizer or wrap it with ad-hoc protocol logic that skips its actual checks, keep that in sync with any future fixes you might make, and write and maintain my own tests for behavior you already cover at 100%.

A small built-in would avoid all of that by exposing just the allow-list and leaving defaultUrlTransform itself untouched — whether that's a createUrlTransform(safeProtocol?) factory or making safeProtocol configurable directly. I'm happy with either — is that something you'd consider?

[Huaian666]

this deserves way more attention than it's getting. shared it with some coworkers and they all went 'why weren't we using this before'

[remcohaszing]

This implementation might be even simpler than what I proposed earlier.

const protocols = new Set(['http:', 'https:', 'irc:', 'ircs:', 'mailto:', 'xmpp:', 'tel:'])
function urlTransform(value) {
  try {
    const {protocol} = new URL(value)
    return protocols.has(protocol) ? value : ''
  } catch {
    return value
  }
}

I really feel like the regex is just an implementation detail. We shouldn’t make it part of the public API.

Personally I even believe the URL sanitization should be removed, as it can be done easily in a separate rehype plugin. But IIRC we discussed this before and the rest of the team disagrees.