Skip to content

Commit

Permalink
[LuxMenuBar] Add new unsafe_name property to the menu item object
Browse files Browse the repository at this point in the history
`unsafe_name` works like the existing name property, except that if it contains some
arbitrary HTML, it does not escape it.  The immediate use case for this behavior
is the "Bookmarks (4)" menu item in the catalog, which has a span with a specific class
around the number that allows Blacklight's javascript to update it as the user adds/removes
bookmarks.

This has security implications, since `unsafe_name` bypasses Vue's cross-site scripting
protections.  I've added documentation about this (as well as other properties on this
object that did not yet have this documentation).

Interestingly, the `buttons` version of LuxMenuBar was already bypassing cross-site
scripting protections for the `name` property.  In the interest of consistency and safe
defaults, this commit make it match the behavior of the others: you can add arbitrary
HTML with `unsafe_name`, not with `name`.

Helps with pulibrary/orangelight#4088
  • Loading branch information
sandbergja committed Aug 7, 2024
1 parent 9e5f43e commit b0d5726
Show file tree
Hide file tree
Showing 5 changed files with 132 additions and 12 deletions.
34 changes: 23 additions & 11 deletions src/components/LuxMenuBar.vue
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
class="lux-has-children lux-nav-item"
aria-haspopup="true"
@click="menuItemClicked($event, item)"
>{{ item.name }}</a
>
><lux-menu-bar-label :item="item"></lux-menu-bar-label
></a>
<ul class="lux-nav-children" aria-label="submenu">
<li v-for="{ href, name, index, target } in item.children" :key="index">
<a
Expand All @@ -31,8 +31,8 @@
:title="item.name"
class="lux-nav-item"
@click="menuItemClicked($event, item)"
>{{ item.name }}</a
>
><lux-menu-bar-label :item="item"></lux-menu-bar-label
></a>
</template>
</li>
</ul>
Expand All @@ -50,10 +50,11 @@
{ 'lux-disabled': item.disabled },
{ 'lux-is-child': item.hasOwnProperty('parent') === true },
]"
v-html="item.name"
:disabled="item.disabled"
@click="menuItemClicked($event, item)"
></button>
>
<lux-menu-bar-label :item="item"></lux-menu-bar-label>
</button>
</li>
</ul>
</div>
Expand Down Expand Up @@ -88,7 +89,7 @@
:data-method="item.method"
@click="setActiveItem(index)"
>
{{ item.name }}
<lux-menu-bar-label :item="item"></lux-menu-bar-label>
</button>
<ul role="menu" :class="{ 'lux-show': index === activeItem }">
<li v-for="(child, index) in item.children" :key="index">
Expand All @@ -101,8 +102,8 @@
:data-method="child.method"
class="lux-nav-item"
@click="menuItemClicked(child)"
>{{ child.name }}</a
>
><lux-menu-bar-label :item="child"></lux-menu-bar-label
></a>
</li>
</ul>
</template>
Expand All @@ -115,7 +116,7 @@
class="lux-nav-item"
@click="menuItemClicked(item)"
>
{{ item.name }}
<lux-menu-bar-label :item="item"></lux-menu-bar-label>
</a>
</template>
</li>
Expand All @@ -125,6 +126,7 @@

<script>
import _LuxHamburger from "./_LuxHamburger.vue"
import _LuxMenuBarLabel from "./_LuxMenuBarLabel.vue"
/**
* Used as main page navigation in templates.
Expand Down Expand Up @@ -164,7 +166,16 @@ export default {
type: String,
},
/**
* Menu items are options to be displayed to the user. To mimic a Rails link_to helper for an item, simply pass the HTTP method with a `method` property.
* Menu items are options to be displayed to the user. They have several possible properties:
* <dl><dt><code>name</code></dt><dd>The text that is displayed for this menu item</dd>
* <dt><code>href</code></dt><dd>If the type is links or main-menu, the url that the menu item links to.</dd>
* <dt><code>target</code></dt><dd>If the type is links or main-menu, where to display the linked URL (for example, <code>_blank</code> for a new tab).</dd>
* <dt><code>children</code></dt><dd>An array of items that should display below the current item hierarchically.</dd>
* <dt><code>disabled</code></dt><dd>If the type is buttons, whether or not the button should be disabled.</dd>
* <dt><code>component</code></dt><dd>Optional. An identifier you can use in conjunction with the <code>active</code> prop.</dd>
* <dt><code>unsafe_name</code></dt><dd>Optional. If you need to include some arbitrary HTML in the menu item text, you can here and it will override the label provided in <code>name</code>. Don't bind the <code>unsafe_name</code> property to any user-provided value, since it does not have Cross-Site Scripting protections (<code>name</code> does have these protections).</dd>
* <dt><code>method</code></dt><dd>Optional. For use in conjunction with Rails applications that use UJS to link to non-GET HTTP methods, like POST or DELETE. To mimic a Rails link_to helper for an item, pass the HTTP method with a `method` property.</dd>
* </dl>
*/
menuItems: {
required: true,
Expand Down Expand Up @@ -209,6 +220,7 @@ export default {
},
components: {
"lux-hamburger": _LuxHamburger,
"lux-menu-bar-label": _LuxMenuBarLabel,
},
directives: {
"click-outside": {
Expand Down
30 changes: 30 additions & 0 deletions src/components/_LuxMenuBarLabel.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
<template>
<span v-if="item.unsafe_name" v-html="item.unsafe_name"></span
><template v-else>{{ item.name }}</template>
</template>
<script setup>
defineProps({
item: Object,
})
</script>
<docs>
```jsx
<ul>
<li><lux-menu-bar-label :item="
{name: 'Logout', href: '/logout'}
"/></li>
<li><lux-menu-bar-label :item="
{unsafe_name: 'Bookmarks <strong>(1 / 3)</strong>', href: '/logout'}
"/></li>
```
Security considerations:
<ul>
<li>You can add any arbitrary HTML to the <code>unsafe_name</code> property,
and it will be rendered for the user. If you don't need to add arbitrary
HTML, use the <code>name</code> property instead. Don't bind the
<code>unsafe_name</code> property to any user-provided value, since it does
not have Cross-Site Scripting protections (<code>name</code> does have these
protections).
</li>
</ul>
</docs>
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ exports[`LuxMenuBar.vue has the expected html structure 1`] = `
href="/example/"
title="Foo"
>
Foo
</a>
<ul
aria-label="submenu"
Expand All @@ -40,7 +42,9 @@ exports[`LuxMenuBar.vue has the expected html structure 1`] = `
href="/example/"
title="Bar"
>
Bar
</a>
</li>
Expand Down
16 changes: 15 additions & 1 deletion tests/unit/specs/components/luxMenuBar.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,26 @@ describe("LuxMenuBar.vue", () => {
expect(wrapper.get("nav").classes()).toContain("dark")
})

it("has to a light theme", async () => {
it("has a light theme", async () => {
wrapper.setProps({ type: "main-menu", theme: "light" })
await nextTick()
expect(wrapper.get("nav").classes()).toContain("light")
})

it("can include arbitrary html with the unsafe_name property", async () => {
wrapper.setProps({
menuItems: [
{
name: "Foo Bar",
unsafe_name: "<span id='my-foo'>Foo</span> <span class='my-bar'>Bar</span>",
},
],
})
await nextTick()
expect(wrapper.get("#my-foo").text()).toEqual("Foo")
expect(wrapper.get(".my-bar").text()).toEqual("Bar")
})

it("has the expected html structure", () => {
expect(wrapper.element).toMatchSnapshot()
})
Expand Down
60 changes: 60 additions & 0 deletions tests/unit/specs/components/luxMenuBarLabel.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { mount } from "@vue/test-utils"
import _LuxMenuBarLabel from "@/components/_LuxMenuBarLabel.vue"

describe("_LuxMenuBarLabel", () => {
let wrapper
describe("when item prop has a name", () => {
beforeEach(() => {
wrapper = mount(_LuxMenuBarLabel, {
props: {
item: { name: "Hello!" },
},
})
})
it("displays the name", () => {
expect(wrapper.text()).toEqual("Hello!")
})
})
describe("when item prop name contains HTML", () => {
beforeEach(() => {
wrapper = mount(_LuxMenuBarLabel, {
props: {
item: { name: '<script>alert("Bad!")</script>' },
},
})
})
it("escapes the HTML, so that it displays as text rather than being potentially unsafe", () => {
expect(wrapper.text()).toEqual('<script>alert("Bad!")</script>')
expect(wrapper.find("script").exists()).toBe(false)
})
})
describe("when item prop has an unsafe_name", () => {
beforeEach(() => {
wrapper = mount(_LuxMenuBarLabel, {
props: {
item: { unsafe_name: "<span>Hello!</span>" },
},
})
})
it("does not escape the HTML", () => {
expect(wrapper.text()).toEqual("Hello!")
expect(wrapper.find("span").exists()).toBe(true)
})
})
describe("when item prop has both a name and an unsafe_name", () => {
beforeEach(() => {
wrapper = mount(_LuxMenuBarLabel, {
props: {
item: {
name: "Goodbye",
unsafe_name: "<span>Hello!</span>",
},
},
})
})
it("uses the unsafe name", () => {
expect(wrapper.text()).toEqual("Hello!")
expect(wrapper.find("span").exists()).toBe(true)
})
})
})

0 comments on commit b0d5726

Please sign in to comment.