Allow safeProtocol to be passed as a parameter for defaultUrlTransform #941

Activity

1. github-actions

   added

   👋 phase/newPost is being triaged automaticallyPost is being triaged automatically

   🤞 phase/openPost is being triaged manuallyPost is being triaged manually

   and removed

   👋 phase/newPost is being triaged automaticallyPost is being triaged automatically

   on Apr 24, 2026
2. wooorm commented on Jun 3, 2026

   

   wooorm

   on Jun 3, 2026

   Member

   More actions

   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?
3. janpe commented on Jun 4, 2026

   

   janpe

   on Jun 4, 2026

   Last edited by janpe

   Author

   More actions

   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?
4. remcohaszing commented on Jun 4, 2026

   

   remcohaszing

   on Jun 4, 2026

   Member

   More actions

   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 ''
   }
5. janpe commented on Jun 4, 2026

   

   janpe

   on Jun 4, 2026

   Author

   More actions

   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?
6. Huaian666 commented on Jun 6, 2026

   

   Huaian666

   on Jun 6, 2026

   More actions

   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'
7. remcohaszing commented on Jun 7, 2026

   

   remcohaszing

   on Jun 7, 2026

   Member

   More actions

   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.