[MarkdownEditor] Allow document relative links in the link validator plugin#9554
Conversation
|
|
||
| export const DEFAULT_OPTIONS = { | ||
| allowRelative: true, | ||
| allowDocumentRelative: false, |
There was a problem hiding this comment.
We can also default this to true if we want to just enable this functionality for free, but I figured I'd start with false to avoid changing existing implementations
There was a problem hiding this comment.
i think your choice to make it false by default is good 👍
There was a problem hiding this comment.
Pull request overview
This PR updates EUI’s Markdown link validator plugin to optionally support document-relative links (e.g. [link](discover)), addressing a regression seen when moving from a legacy editor to EUI’s Markdown editor while preserving protocol allowlisting for security.
Changes:
- Add
allowDocumentRelative(andbaseUrl) options toeuiMarkdownLinkValidatorand implement document-relative URL resolution. - Add Jest coverage for document-relative URL detection and resolution behavior.
- Update EUI documentation and examples to demonstrate enabling document-relative links, plus add a changelog entry.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/website/docs/components/editors-and-syntax/markdown/plugins.mdx | Updates plugin docs/example to enable allowDocumentRelative and show document-relative links. |
| packages/website/docs/components/editors-and-syntax/markdown/format.mdx | Adds security/link-validation documentation clarifying document-relative behavior and configuration. |
| packages/eui/src/components/markdown_editor/plugins/markdown_link_validator.tsx | Implements document-relative detection + resolution and adds new options. |
| packages/eui/src/components/markdown_editor/plugins/markdown_link_validator.test.tsx | Adds tests for isDocumentRelativeUrl and allowDocumentRelative resolution. |
| packages/eui/src/components/markdown_editor/plugins/markdown_default_plugins/parsing_plugins.ts | Updates parsing plugin config docs to mention the new option. |
| packages/eui/src/components/markdown_editor/markdown_editor.stories.tsx | Adds a document-relative link example to the story content. |
| packages/eui/changelogs/upcoming/9554.md | Adds changelog entry for the new option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e-window target (#260782) ## Summary Closes #253431 Closes #257733 This fixes a couple of regressions in the new Markdown panel which resulted from us switching from our own bespoke Markdown editor to `EuiMarkdownEditor` ### Fix relative links I've added a new parsing plugin to our implementation of the markdown editor which handles space-relative links like `[discover](discover)` linking to `/app/discover`. The EUI link validator strips these as invalid, so the new parsing plugin will resolve them to full paths before the link validator runs. It also handles other document-relative links like `../../../upper-level-path` I have opened an [EUI PR](elastic/eui#9554) to commit these changes upstream to the link validator plugin. Once that merges, we can revert this custom plugin and just pass `allowDocumentRelative: true` to the default EUI Markdown plugins' `parsingConfig`. ### Add open-in-new-tab setting This restores the previous toggle setting for opening links in a new tab or not. The user can do this through a new settings popover. In legacy, it defaulted to `false`. I'm keeping it `true` by default in the new implementation, but allowing the user to disable it. <img width="1168" height="509" alt="Screenshot 2026-04-01 at 2 38 15 PM" src="https://github.com/user-attachments/assets/4570163e-ee76-491b-8ee6-caf0fda74887" /> #### Known design bug To prevent the settings button tooltip from covering the Editor/Preview toggle, I set the tooltip position to `bottom` <img width="996" height="196" alt="Screenshot 2026-03-31 at 3 10 26 PM" src="https://github.com/user-attachments/assets/c07fbe15-d7e1-431f-b89e-c556de68a806" /> But because of EUI limitations, I can't do the same for the help button: <img width="265" height="150" alt="Screenshot 2026-03-31 at 3 42 08 PM" src="https://github.com/user-attachments/assets/6274b0d0-3456-45a2-b295-14841f818ef1" /> I've submitted an upstream [EUI PR](elastic/eui#9546) to allow us to fix this. ### Checklist - [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
| }); | ||
| }); | ||
|
|
||
| describe('euiMarkdownLinkValidator with allowDocumentRelative', () => { |
There was a problem hiding this comment.
(non-blocking) would you consider adding a test case for allowDocumentRelative: true with allowRelative: false? to confirm it's intentional that allowRelative is required for allowDocumentRelative to be enabled…
| it('preserves hash fragments during resolution', () => { | ||
| const ast = createLinkAst('discover#/view/saved-search'); | ||
| euiMarkdownLinkValidator({ allowDocumentRelative: true, baseUrl })(ast); | ||
|
|
||
| expect(getLinkNode(ast).url).toBe( | ||
| '/s/my-space/app/discover#/view/saved-search' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
(non-blocking) would it be a good idea to add a similar test case for preserving query strings e.g. 'discover?index=logstash-*'
|
|
||
| export const DEFAULT_OPTIONS = { | ||
| allowRelative: true, | ||
| allowDocumentRelative: false, |
There was a problem hiding this comment.
i think your choice to make it false by default is good 👍
| /** | ||
| * Resolves a document relative URL against a base URL, replicating | ||
| * native browser resolution of e.g. `<a href="discover">`. | ||
| */ |
There was a problem hiding this comment.
(non-blocking) is it actually true this replicates native browser resolution if it's stripping the trailing slash? i understand resolution would be different whether there's a trailing slash or not… i would suggest updating the JSDoc if you agree
There was a problem hiding this comment.
I ended up catching the trailing slash bug after writing the initial comment, good call on updating it. Pushed a fix.
| * resolution, the same way an `<a href="discover">` would behave in | ||
| * plain HTML. | ||
| * @default false | ||
| */ |
There was a problem hiding this comment.
if it's intentional, i think it'd be good to mention here that allowRelative=true is required for allowDocumentRelative=true to work
also, similarly to my comment below, would you consider adjusting the "using the browser's native URL resolution" part to mention trailing slashes will be stripped?
Co-authored-by: Arturo Castillo Delgado <arturo@arturu.com>
💚 Build SucceededHistory
|
💚 Build Succeeded
History
|
|
@acstll Thanks for the approval, I don't have merge permissions in this repo, if you're able to hit merge for me? |
Summary
In elastic/kibana#257733, the Dashboard Markdown panel experienced a regression when switching from our legacy custom Markdown editor to the EUI Markdown editor: relative links without a leading
/started getting thrown out by the markdown link validator plugin. Links like<a href="discover">, which the browser would natively resolve to the/app/discoveron the current space, no longer worked.We were able to build a workaround parsing plugin to resolve these inks before the link validator ran, but this capability should probably be added to EUI directly.
This PR adds a document relative link resolver to the
euiMarkdownLinkValidatorplugin. It is enabled by theallowDocumentRelativeoption.Why not just let the consumer disable the plugin?
We can also get document-relative links to work if we disable the link validator altogether, but restricting protocols to the default allowlist of
http,https, andmailtois still big help for security. With this change, we can allow document-relative links to work while still blocking dangerous stuff likeftp://orjavascript:Also, browsers resolve document-relative hrefs inconsistently if a trailing
/gets added to a base URL. Given a link to the[same document root](../another-page), the browser would resolve:http://base.url/a/b/c->http://base.url/a/b/another-pagehttp://base.url/a/b/c/->http://base.url/a/b/c/another-pageThe changes in this PR will strip the trailing
/from the base URL before resolving the link to ensure consistent behavior.Kibana doesn't seem to add trailing slashes all that much, but the EUI docs website does.
Alternative: create a new plugin?
If we want to keep
euiMarkdownLinkValidatorsolely as a yes/no validator, we could take the document link resolver code and put it in a new plugin, which would get added to the default plugin chain beforeeuiMarkdownLinkValidatorruns. I think there's a simplicity advantage of keeping this code in the validator plugin itself, because it allows us to put theallowDocumentRelativeconfig option in the same object asallowRelative. Open to changing this if the EUI team disagrees.API Changes
allowDocumentRelativeoptionallowDocumentRelativeenables the document relative link resolver in the link validator plugin, defaults tofalseso as to not change existing implementationsImpact Assessment
Impact level: 🟢 Low
This PR sets
allowDocumentRelativetofalseby default. Consumers will have to manually enable it to change link validation behavior.Release Readiness
QA instructions for reviewer
Testing the feature
Note that the Markdown editor storybook seems to be broken, as I wasn't able to actually change the editor content in my testing. So here's how I was able to test locally:
packages/eui/src/components/markdown_editor/plugins/markdown_link_validator.tsxchange line 50 to readallowDocumentRelative: truepackages/eui/src/components/markdown_editor/markdown_editor.stories.tsxadd the following to the end ofinitialContent:[link](link)linkis a link and hasn't been converted to a text nodelocalhost:6006/link(You'll get a Not Found page because Storybook only uses ?-based pathing)Documentation
Review the two docs pages listed under Release Readiness
Checklist before marking Ready for Review
- [ ] QA: Tested light/dark modes, high contrast, mobile, Chrome/Safari/Edge/Firefox, keyboard-only, screen reader- [ ] Breaking changes: Addedbreaking changelabel (if applicable)Reviewer checklist