Minor updates to dsMetadata directive to sanitize metadata

cypress/e2e/metadata-xss-sanitization.cy.ts

@@ -0,0 +1,92 @@
+/**
+ * Regression test for a Cross-Site Scripting (XSS) vulnerability that was introduced by
+ * https://github.com/DSpace/dspace-angular/pull/4776.
+ *
+ * This test creates a new item submission whose abstract contains such a payload and verifies that:
+ *  - the payload is NOT executed (no JavaScript side effect happens), and
+ *  - the dangerous `onerror` attribute is stripped from the rendered markup (while safe, surrounding
+ *    markup/tags are preserved),
+ * when the abstract is displayed via the `[dsMetadata]` directive.
+ *
+ */
+describe('Metadata XSS sanitization', () => {
+  // A classic XSS payload: an image with a broken `src` so that its `onerror` handler fires as soon as
+  // the browser tries (and fails) to load it. If the payload is not sanitized, `onerror` will run and set
+  // `window.dsXssExecuted = true`. (NOTE: This uses "role=presentation" to avoid failing accessibility checks)
+  const XSS_PAYLOAD = 'XSS Test <img src="x" onerror="window.dsXssExecuted = true;" role="presentation"/>';
+  const SAFE_TEXT = 'XSS Test';
+  const UNIQUE_TITLE = `XSS sanitization test item ${Date.now()}`;
+
+  /**
+   * Asserts that the XSS payload has NOT executed on the current page.
+   */
+  function assertXssDidNotExecute(): void {
+    cy.window().then((win: any) => {
+      expect(win.dsXssExecuted).to.not.equal(true);
+    });
+  }
+
+  it('should sanitize a malicious item abstract and not execute injected script when rendered via [dsMetadata]', () => {
+    cy.visit('/mydspace');
+
+    // This page is restricted, so we will be shown the login form. Fill it out & submit.
+    cy.env(['DSPACE_TEST_SUBMIT_USER', 'DSPACE_TEST_SUBMIT_USER_PASSWORD']).then(({ DSPACE_TEST_SUBMIT_USER, DSPACE_TEST_SUBMIT_USER_PASSWORD }) => {
+      cy.loginViaForm(DSPACE_TEST_SUBMIT_USER, DSPACE_TEST_SUBMIT_USER_PASSWORD);
+    });
+
+    // Start a submission
+    cy.get('button[data-test="submission-dropdown"]').click();
+    cy.get('#entityControlsDropdownMenu button[title="none"]').click();
+    cy.get('ds-authorized-collection-selector input[type="search"]').type(Cypress.expose('DSPACE_TEST_SUBMIT_COLLECTION_NAME'));
+    cy.get('ds-authorized-collection-selector button[title="'.concat(Cypress.expose('DSPACE_TEST_SUBMIT_COLLECTION_NAME')).concat('"]')).click();
+
+    // Give the item a unique (safe) title so we can reliably find it again afterward
+    cy.get('#dc_title', { timeout: 10000 }).type(UNIQUE_TITLE);
+
+    // Enter our malicious abstract into the dc.description.abstract field
+    cy.get('#dc_description_abstract').type(XSS_PAYLOAD);
+
+    // Save for Later to persist the (unsanitized, as stored) abstract on the workspace item
+    cy.get('ds-submission-form-footer [data-test="save-for-later"]').click();
+
+    // "Save for Later" should send us to MyDSpace
+    cy.url().should('include', '/mydspace');
+    // The malicious payload should NOT have executed while the submission form/footer rendered the abstract
+    assertXssDidNotExecute();
+
+    // Close any open notifications, to make sure they don't get in the way of next steps
+    cy.get('[data-bs-dismiss="alert"]').click({ multiple: true });
+
+    // Search for the item we just created via its unique title
+    cy.intercept('GET', '/server/api/discover/search/objects*').as('search-results');
+    cy.get('[data-test="search-box"]').type(UNIQUE_TITLE);
+    cy.get('[data-test="search-button"]').click();
+    cy.wait('@search-results');
+
+    // Find the specific result matching our unique title, and scope all further assertions to it.
+    cy.contains('[data-test="list-object"]', UNIQUE_TITLE, { timeout: 10000 })
+      .should('exist')
+      .as('result');
+
+    // The XSS payload must NOT have executed while the [dsMetadata] directive rendered the abstract
+    assertXssDidNotExecute();
+
+    // The abstract should be rendered (via the [dsMetadata] directive) inside a truncatable part
+    cy.get('@result').find('.item-list-abstract span').first().then(($abstract) => {
+      // Sanitization removes *dangerous attributes* (like `onerror`), but it does NOT necessarily
+      // remove the surrounding element itself (e.g. `<img>` is a permitted tag). So we assert that:
+      // - the safe text content is still present,
+      // - the `onerror` attribute is gone from the markup entirely,
+      // - if the `<img>` tag survived sanitization, it has no `onerror` attribute on it.
+      expect($abstract.text()).to.include(SAFE_TEXT);
+      expect($abstract.html()).to.not.include('onerror');
+
+      const img = $abstract.find('img');
+      if (img.length > 0) {
+        // eslint-disable-next-line no-unused-expressions,@typescript-eslint/no-unused-expressions
+        expect(img.attr('onerror')).to.be.undefined;
+      }
+    });
+  });
+});
+

src/app/core/shared/metadata.utils.ts

@@ -48,14 +48,14 @@ export class Metadata {
    * @param escapeHTML Whether the HTML is used inside a `[innerHTML]` attribute
    * @returns {MetadataValue[]} the matching values or an empty array.
    */
-  public static all(metadata: MetadataMapInterface, keyOrKeys: string | string[], hitHighlights?: MetadataMapInterface, filter?: MetadataValueFilter, escapeHTML?: boolean, limit?: number): MetadataValue[] {
+  public static all(metadata: MetadataMapInterface = {}, keyOrKeys: string | string[], hitHighlights?: MetadataMapInterface, filter?: MetadataValueFilter, escapeHTML?: boolean, limit?: number): MetadataValue[] {
     const matches: MetadataValue[] = [];
     if (isNotEmpty(hitHighlights)) {
       for (const mdKey of Metadata.resolveKeys(hitHighlights, keyOrKeys)) {
         if (hitHighlights[mdKey]) {
           for (const candidate of hitHighlights[mdKey]) {
             if (Metadata.valueMatches(candidate as MetadataValue, filter) && (isEmpty(limit) || (hasValue(limit) && matches.length < limit))) {
-              const nonHighlightValues = metadata[mdKey] as MetadataValue[];
+              const nonHighlightValues = metadata?.[mdKey] as MetadataValue[];
               const nonHighlightValue = nonHighlightValues?.find((value: MetadataValue) => Metadata.valueMatches(value, filter));
               const language = nonHighlightValue?.language ?? candidate.language ?? null;
               matches.push(Object.assign(new MetadataValue(), candidate, { language }));
@@ -109,14 +109,14 @@ export class Metadata {
    * @param escapeHTML Whether the HTML is used inside a `[innerHTML]` attribute
    * @returns {MetadataValue} the first matching value, or `undefined`.
    */
-  public static first(metadata: MetadataMapInterface, keyOrKeys: string | string[], hitHighlights?: MetadataMapInterface, filter?: MetadataValueFilter, escapeHTML?: boolean): MetadataValue {
+  public static first(metadata: MetadataMapInterface = {}, keyOrKeys: string | string[], hitHighlights?: MetadataMapInterface, filter?: MetadataValueFilter, escapeHTML?: boolean): MetadataValue {
     if (isNotEmpty(hitHighlights)) {
       for (const key of Metadata.resolveKeys(hitHighlights, keyOrKeys)) {
         const values: MetadataValue[] = hitHighlights[key] as MetadataValue[];
         if (values) {
           const metadataValue = values.find((value: MetadataValue) => Metadata.valueMatches(value, filter));
           if (metadataValue) {
-            const nonHighlightValues = metadata[key] as MetadataValue[];
+            const nonHighlightValues = metadata?.[key] as MetadataValue[];
             const nonHighlightValue = nonHighlightValues?.find((value: MetadataValue) => Metadata.valueMatches(value, filter));
             const language = nonHighlightValue?.language ?? metadataValue.language ?? null;
             return Object.assign(new MetadataValue(), metadataValue, { language });

src/app/shared/metadata.directive.spec.ts

@@ -3,6 +3,7 @@ import {
   ComponentFixture,
   TestBed,
 } from '@angular/core/testing';
+import { DomSanitizer } from '@angular/platform-browser';
 
 import { MetadataValue } from '../core/shared/metadata.models';
 import { MetadataDirective } from './metadata.directive';
@@ -23,6 +24,7 @@ describe('MetadataDirective', () => {
   let fixture: ComponentFixture<HostComponent>;
   let host: HostComponent;
   let span: HTMLSpanElement;
+  let sanitizer: DomSanitizer;
 
   function createMetadata(value?: string, language?: string): MetadataValue {
     return {
@@ -42,6 +44,7 @@ describe('MetadataDirective', () => {
 
     fixture = TestBed.createComponent(HostComponent);
     host = fixture.componentInstance;
+    sanitizer = TestBed.inject(DomSanitizer);
     fixture.detectChanges();
     span = fixture.nativeElement.querySelector('span');
   });
@@ -95,4 +98,25 @@ describe('MetadataDirective', () => {
     fixture.detectChanges();
     expect(span.innerHTML.toLowerCase()).toBe('<em>italic</em>');
   });
+
+  it('sanitizes the value before setting innerHTML', () => {
+    const sanitizeSpy = spyOn(sanitizer, 'sanitize').and.callThrough();
+    host.mv = createMetadata('<em>Italic</em>', 'en');
+    fixture.detectChanges();
+    expect(sanitizeSpy).toHaveBeenCalledWith(jasmine.any(Number), '<em>Italic</em>');
+  });
+
+  it('strips out script tags from the value (XSS protection)', () => {
+    host.mv = createMetadata('<script>alert("XSS")</script>Safe text', 'en');
+    fixture.detectChanges();
+    expect(span.innerHTML).not.toContain('<script');
+    expect(span.innerHTML).toContain('Safe text');
+  });
+
+  it('strips out inline event handlers from the value (XSS protection)', () => {
+    host.mv = createMetadata('<img src="x" onerror="document.body.insertAdjacentHTML(\'afterbegin\',\'<h1>XSS!</h1>\')">', 'en');
+    fixture.detectChanges();
+    expect(span.innerHTML).not.toContain('onerror');
+    expect(document.body.querySelector('h1')).toBeNull();
+  });
 });

src/app/shared/metadata.directive.ts

@@ -4,7 +4,9 @@ import {
   inject,
   Input,
   Renderer2,
+  SecurityContext,
 } from '@angular/core';
+import { DomSanitizer } from '@angular/platform-browser';
 
 import { MetadataValue } from '../core/shared/metadata.models';
 import { normalizeLanguageCode } from './utils/normalize-language-code-utils';
@@ -37,6 +39,12 @@ export class MetadataDirective {
    */
   private renderer = inject(Renderer2);
 
+  /**
+   * Angular DomSanitizer instance used to sanitize the metadata value before
+   * inserting it into the DOM as innerHTML, preventing XSS attacks.
+   */
+  private sanitizer = inject(DomSanitizer);
+
   /**
    * Input property for the directive. Accepts a `MetadataValue` object.
    * When set, it updates the host element's `innerHTML` and `lang` attribute.
@@ -51,7 +59,7 @@ export class MetadataDirective {
   /**
    * Updates the host element's `innerHTML` and `lang` attribute based on the current `MetadataValue`.
    * - If `MetadataValue` is provided:
-   *   - Sets `innerHTML` to `MetadataValue.value` (or an empty string if `value` is null/undefined).
+   *   - Sets `innerHTML` to the sanitized `MetadataValue.value` (or an empty string if `value` is null/undefined).
    *   - Sets the `lang` attribute to `MetadataValue.language` (or removes it if `language` is null/undefined).
    * - If `MetadataValue` is null/undefined:
    *   - Clears the `innerHTML`.
@@ -60,7 +68,8 @@ export class MetadataDirective {
   private updateHost(): void {
     if (this._metadataValue) {
       const val = this._metadataValue.value ?? '';
-      this.renderer.setProperty(this.el.nativeElement, 'innerHTML', val);
+      const sanitizedVal = this.sanitizer.sanitize(SecurityContext.HTML, val) ?? '';
+      this.renderer.setProperty(this.el.nativeElement, 'innerHTML', sanitizedVal);
       if (this._metadataValue.language) {
         const normalizedLang = normalizeLanguageCode(this._metadataValue.language);
         this.renderer.setAttribute(this.el.nativeElement, 'lang', normalizedLang);