Skip to content

feat(core): add directive for escaping html contents#2951

Open
jw-foss wants to merge 3 commits intomainfrom
feat/add-directive-for-escaping-html-contents
Open

feat(core): add directive for escaping html contents#2951
jw-foss wants to merge 3 commits intomainfrom
feat/add-directive-for-escaping-html-contents

Conversation

@jw-foss
Copy link
Copy Markdown
Contributor

@jw-foss jw-foss commented Mar 2, 2026

Summary

  • Add a new directive to escape raw HMTL bindings.
  • Add test cases against this directive.
  • Add DOMPurify as the escaping method.

Reason

A Vue directive for safely rendering HTML content with XSS protection via DOMPurify. Use this instead of v-html to prevent Cross-Site Scripting (XSS) vulnerabilities.

Why?: v-html renders raw HTML without sanitization, which can execute malicious scripts. v-safe-html automatically removes dangerous content (scripts, event handlers, javascript: URLs) while preserving safe HTML formatting.

Comparison

HTML Content v-html (Unsafe) v-safe-html (Safe)
<strong>Bold text</strong> ✅ Renders: Bold text ✅ Renders: Bold text
<a href="https://example.com">Link</a> ✅ Renders clickable link ✅ Renders clickable link
<script>alert('XSS')</script> Executes script! ✅ Script tag removed
<img src=x onerror="alert('XSS')"> Executes on error! onerror removed
<a href="javascript:alert('XSS')">Click</a> Executes on click! href sanitized
<div onclick="alert('XSS')">Text</div> Executes on click! onclick removed
<iframe src="evil.com"></iframe> Loads external content! iframe removed
<em>Italic</em> with <br> breaks ✅ Renders with formatting ✅ Renders with formatting

Package size impacts (summarized by Copilot)

Current size

@kong-ui-public/core Bundle Impact
Current Bundle Sizes (built Mar 2, before DOMPurify):

ES module: 68 KB (18.8 KB gzipped)
UMD: 48 KB (16.1 KB gzipped)

After the adoption

Expected Impact:

The core package will add 23 KB minified (8.5 KB gzipped) when DOMPurify is included
Well within the 246 KB configured size limit
In production with compression, the impact is only 8.5 KB

Size Analysis
The security benefit of preventing XSS attacks far outweighs the small bundle size impact:

8.5 KB gzipped is negligible for modern web apps
DOMPurify is battle-tested and widely used (millions of downloads/week)
Tree-shaking ensures only used parts of DOMPurify are included
The alternative (custom sanitization) would likely be similar or larger in size and less secure
Verdict: The bundle impact is minimal and acceptable for the critical XSS protection provided.

Expected size:

ES module: 91 KB (~27.3 KB gzipped)
UMD: 71 KB (~24.6 KB gzipped)

And because this will add approximately 8.5KB to the consuming apps that uses @kong-ui-public/core, if that is not acceptable we could also publish a dedicated package for this work.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new v-safe-html directive to @kong-ui-public/core to sanitize raw HTML bindings using DOMPurify, along with tests and documentation.

Changes:

  • Introduce vSafeHtml directive + SafeHtmlPlugin for global registration.
  • Add unit tests for sanitization behavior and DOMPurify configuration passthrough.
  • Add DOMPurify and Vue Test Utils dependencies, plus directive documentation.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pnpm-lock.yaml Locks new deps for packages/core/core (dompurify, @vue/test-utils).
packages/core/core/src/index.ts Exposes SafeHtmlPlugin and vSafeHtml from the package entrypoint.
packages/core/core/src/directives/vSafeHtml.ts Implements directive that sanitizes and sets innerHTML.
packages/core/core/src/directives/vSafeHtml.spec.ts Adds Vitest coverage for sanitization + config behavior + updates.
packages/core/core/src/directives/README.md Documents directive registration and usage.
packages/core/core/package.json Adds dompurify dependency and @vue/test-utils devDependency.
packages/core/core/README.md Adds a top-level section documenting v-safe-html.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +39 to +65
HTML sanitization directive backed by DOMPurify.

Global registration:

```ts
import { createApp } from 'vue'
import { SafeHtmlPlugin } from '@kong-ui-public/core'

createApp(App).use(SafeHtmlPlugin)
```

Local registration:

```ts
import { vSafeHtml } from '@kong-ui-public/core'

export default {
directives: { safeHtml: vSafeHtml },
}
```

Template usage:

```vue
<div v-safe-html="htmlString" />
<div v-safe-html="{ html: htmlString, config: { ALLOWED_TAGS: ['a', 'strong'] } }" />
```
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new section duplicates the directive docs that also live under ./src/directives/README.md, while the rest of this README links out to per-feature docs (e.g., useAxios, useWindow). To keep documentation consistent and avoid drift, consider replacing this block with a link to the directives README (or ensure only one source of truth).

Suggested change
HTML sanitization directive backed by DOMPurify.
Global registration:
```ts
import { createApp } from 'vue'
import { SafeHtmlPlugin } from '@kong-ui-public/core'
createApp(App).use(SafeHtmlPlugin)
```
Local registration:
```ts
import { vSafeHtml } from '@kong-ui-public/core'
export default {
directives: { safeHtml: vSafeHtml },
}
```
Template usage:
```vue
<div v-safe-html="htmlString" />
<div v-safe-html="{ html: htmlString, config: { ALLOWED_TAGS: ['a', 'strong'] } }" />
```
[v-safe-html directive docs](./src/directives/README.md)

Copilot uses AI. Check for mistakes.
import { vSafeHtml } from '@kong-ui-public/core'

export default {
directives: { safeHtml: vSafeHtml },
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code block uses a tab indentation before directives:; elsewhere the repo generally uses spaces. Please convert this to spaces to avoid inconsistent formatting and potential lint/formatting churn.

Suggested change
directives: { safeHtml: vSafeHtml },
directives: { safeHtml: vSafeHtml },

Copilot uses AI. Check for mistakes.

expect(wrapper.html()).toContain('<strong>two</strong>')
expect(wrapper.html()).not.toContain('one')
})
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current tests cover updates when the directive value is a new string, but they don't cover the object form ({ html, config }) updating—especially the common case where html is mutated while the object reference stays the same. Adding a test for object-value updates would guard against stale DOM issues in the directive updated hook.

Suggested change
})
})
it('updates when object content html changes', async () => {
const content = {
html: '<strong>one</strong>',
config: { ALLOWED_TAGS: ['strong'] },
}
const wrapper = mount(TestComponent, {
props: {
content,
},
})
expect(wrapper.html()).toContain('<strong>one</strong>')
content.html = '<strong>two</strong>'
await wrapper.setProps({ content })
expect(wrapper.html()).toContain('<strong>two</strong>')
expect(wrapper.html()).not.toContain('one')
})

Copilot uses AI. Check for mistakes.
Comment on lines +31 to +33
if (binding.value !== binding.oldValue) {
applySafeHtml(el, binding.value)
}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated hook skips re-sanitizing when binding.value === binding.oldValue, which breaks updates for reactive/object values that are mutated in place (same reference, different html/config). Consider always calling applySafeHtml on updated, or compare the resolved html/config instead of reference equality so DOM stays in sync.

Suggested change
if (binding.value !== binding.oldValue) {
applySafeHtml(el, binding.value)
}
applySafeHtml(el, binding.value)

Copilot uses AI. Check for mistakes.
@adamdehaven
Copy link
Copy Markdown
Member

I appreciate you updating the description, but I'm still not sure I understand why we need this directive, or where it will be used (meaning I don't see an existing feature or JIRA that needs to take advantage of this functionality).

Also, exposing the config to allow-list HTML elements, such as script tags, seems to defeat the purpose of the directive?

@jw-foss
Copy link
Copy Markdown
Contributor Author

jw-foss commented Mar 3, 2026

I appreciate you updating the description, but I'm still not sure I understand why we need this directive, or where it will be used (meaning I don't see an existing feature or JIRA that needs to take advantage of this functionality).

We had some occurrences of XSS exploits in the past such as:

The directive should be the very first step that we always want to stay away from using v-html directive, I will addd follow up steps of adding an ESLint plugin to warn/raise error when using v-html. I think this should be a precaution for someone accidentally renders user inputs directly into the UI.

Also, exposing the config to allow-list HTML elements, such as script tags, seems to defeat the purpose of the directive?

Agreed with the script tags, some of the tags might need to be used during rendering, developers should use as they know the risk

@adamdehaven
Copy link
Copy Markdown
Member

We had some occurrences of XSS exploits in the past

The two instances you linked above wouldn't have been solved by this directive 🤔

I'm still having a hard time understanding why this is needed, as it seems just not utilizing v-html altogether would be the better route. We could add a rule in our shared ESLint config to disallow the attribute (this could be overridden if absolutely needed).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants