From b0d57265a0c6e14b963afdb5743202957b3305a6 Mon Sep 17 00:00:00 2001
From: Jane Sandberg <js7389@princeton.edu>
Date: Wed, 7 Aug 2024 10:38:24 -0700
Subject: [PATCH] [LuxMenuBar] Add new unsafe_name property to the menu item
 object

`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
---
 src/components/LuxMenuBar.vue                 | 34 +++++++----
 src/components/_LuxMenuBarLabel.vue           | 30 ++++++++++
 .../__snapshots__/luxMenuBar.spec.js.snap     |  4 ++
 .../unit/specs/components/luxMenuBar.spec.js  | 16 ++++-
 .../specs/components/luxMenuBarLabel.spec.js  | 60 +++++++++++++++++++
 5 files changed, 132 insertions(+), 12 deletions(-)
 create mode 100644 src/components/_LuxMenuBarLabel.vue
 create mode 100644 tests/unit/specs/components/luxMenuBarLabel.spec.js

diff --git a/src/components/LuxMenuBar.vue b/src/components/LuxMenuBar.vue
index 4cccbfb..fbc4fe2 100644
--- a/src/components/LuxMenuBar.vue
+++ b/src/components/LuxMenuBar.vue
@@ -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
@@ -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>
@@ -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>
@@ -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">
@@ -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>
@@ -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>
@@ -125,6 +126,7 @@
 
 <script>
 import _LuxHamburger from "./_LuxHamburger.vue"
+import _LuxMenuBarLabel from "./_LuxMenuBarLabel.vue"
 
 /**
  * Used as main page navigation in templates.
@@ -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,
@@ -209,6 +220,7 @@ export default {
   },
   components: {
     "lux-hamburger": _LuxHamburger,
+    "lux-menu-bar-label": _LuxMenuBarLabel,
   },
   directives: {
     "click-outside": {
diff --git a/src/components/_LuxMenuBarLabel.vue b/src/components/_LuxMenuBarLabel.vue
new file mode 100644
index 0000000..0e6fbd5
--- /dev/null
+++ b/src/components/_LuxMenuBarLabel.vue
@@ -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>
diff --git a/tests/unit/specs/components/__snapshots__/luxMenuBar.spec.js.snap b/tests/unit/specs/components/__snapshots__/luxMenuBar.spec.js.snap
index d1a03dd..d131c30 100644
--- a/tests/unit/specs/components/__snapshots__/luxMenuBar.spec.js.snap
+++ b/tests/unit/specs/components/__snapshots__/luxMenuBar.spec.js.snap
@@ -14,7 +14,9 @@ exports[`LuxMenuBar.vue has the expected html structure 1`] = `
         href="/example/"
         title="Foo"
       >
+        
         Foo
+        
       </a>
       <ul
         aria-label="submenu"
@@ -40,7 +42,9 @@ exports[`LuxMenuBar.vue has the expected html structure 1`] = `
         href="/example/"
         title="Bar"
       >
+        
         Bar
+        
       </a>
     </li>
     
diff --git a/tests/unit/specs/components/luxMenuBar.spec.js b/tests/unit/specs/components/luxMenuBar.spec.js
index 7f27088..218d7d4 100644
--- a/tests/unit/specs/components/luxMenuBar.spec.js
+++ b/tests/unit/specs/components/luxMenuBar.spec.js
@@ -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()
   })
diff --git a/tests/unit/specs/components/luxMenuBarLabel.spec.js b/tests/unit/specs/components/luxMenuBarLabel.spec.js
new file mode 100644
index 0000000..8f3ab10
--- /dev/null
+++ b/tests/unit/specs/components/luxMenuBarLabel.spec.js
@@ -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)
+    })
+  })
+})