fix: support custom links in markdown

[villebro]

SUMMARY

Currently it's not possible to use links to custom protocols, despite adding them to HTML_SANITIZATION_SCHEMA_EXTENSIONS["protocols"] in superset_config.py. This is due to the fact that ReactMarkdown has built-in link sanitization functionality that's overlapping with the rehypePlugins that we're using.

As we're already using rehype to sanitize the markdown, it makes sense to disable the built-in sanitization. This can be done by setting transformLinkUri={null}. Here's what ChatGPT said about the change:

Me:

So if I'm already sanitizing links using rehype-sanitize, I can safely set transformLinkUri to null?

ChatGPT:

Yes, if you're already using rehype-sanitize effectively to sanitize links, you can set transformLinkUri to null in react-markdown. Here's why:

Role of transformLinkUri: In react-markdown, transformLinkUri is a function used for transforming or validating URLs in links. By default, it applies some basic security checks to URLs. Setting it to null disables these default transformations.

Role of rehype-sanitize: If you're using rehype-sanitize, it sanitizes the HTML output of the Markdown conversion, including URLs in links. With a proper configuration, rehype-sanitize can ensure that only safe URLs are allowed, effectively preventing XSS attacks and other security vulnerabilities associated with links.

Ensuring Adequate Sanitization: Before setting transformLinkUri to null, make sure that your rehype-sanitize configuration adequately covers your security needs. This includes specifying allowed protocols, attributes, and other elements. Your configuration should be strict enough to prevent unsafe URLs but flexible enough to allow your required content.

Testing and Validation: After making this change, thoroughly test your application to ensure that no unsafe links are allowed. This should include testing with various types of links, especially edge cases, to confirm that the sanitizer works as expected.

Security Best Practices: Always stay informed about security best practices and be prepared to update your sanitization strategy as new threats emerge. Regularly review and update your rehype-sanitize configuration to maintain robust security.

In conclusion, setting transformLinkUri to null is safe as long as you're confident in your rehype-sanitize configuration and its ability to effectively sanitize links. Always ensure through testing and validation that your implementation maintains the security integrity of your application.

Originally, I wanted to add a RTL test that ensures the links render correctly with this change. However, we've had to disable react-markdown and the rehype libraries in our test suites due to ESM incompatibility with Jest:

superset/superset-frontend/spec/helpers/shim.tsx

Lines 87 to 90 in b4a35e6

	// Check /~https://github.com/remarkjs/react-markdown/issues/635
	jest.mock('react-markdown', () => (props: any) => <>{props.children}</>);
	jest.mock('rehype-sanitize', () => () => jest.fn());
	jest.mock('rehype-raw', () => () => jest.fn());

For this reason we're unable to add proper tests for react-markdown before it either adds CJS, or Jest fully supports ESM. I'm bumping Jest in the following PR to a more recent version: #26182 However, unfortunately this version doesn't yet work correctly with ESM, so it seems we may need to wait a while longer before this test can be added.

TESTING INSTRUCTIONS

1. Checkout master branch
2. Add the following to your superset_config.py: HTML_SANITIZATION_SCHEMA_EXTENSIONS = {"protocols": {"href": ["foo"] } }
3. Create a markdown component on a dashboard with a link to foo://bar and another one with xyz://qwerty
4. Notice that both links point to javascript:void()
5. Checkout this PR
6. Notice that the first link now points to foo://bar as expected, and the other one is completely missing a href thanks to our rehype sanitization

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[michael-s-molina]

LGTM

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b4a35e6) 69.19% compared to head (78f3f30) 69.19%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #26211   +/-   ##
=======================================
  Coverage   69.19%   69.19%           
=======================================
  Files        1945     1945           
  Lines       75928    75928           
  Branches     8453     8453           
=======================================
  Hits        52537    52537           
  Misses      21207    21207           
  Partials     2184     2184           

Flag	Coverage Δ
javascript	56.51% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.