diff --git a/cypress/e2e/metadata-xss-sanitization.cy.ts b/cypress/e2e/metadata-xss-sanitization.cy.ts new file mode 100644 index 00000000000..421a1dff97a --- /dev/null +++ b/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 '; + 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. `` 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 `` 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; + } + }); + }); +}); + diff --git a/src/app/core/shared/metadata.utils.ts b/src/app/core/shared/metadata.utils.ts index 2f24e0963a7..28485d057af 100644 --- a/src/app/core/shared/metadata.utils.ts +++ b/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 }); diff --git a/src/app/shared/metadata.directive.spec.ts b/src/app/shared/metadata.directive.spec.ts index 2c34df05e11..a388510aa12 100644 --- a/src/app/shared/metadata.directive.spec.ts +++ b/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; 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('italic'); }); + + it('sanitizes the value before setting innerHTML', () => { + const sanitizeSpy = spyOn(sanitizer, 'sanitize').and.callThrough(); + host.mv = createMetadata('Italic', 'en'); + fixture.detectChanges(); + expect(sanitizeSpy).toHaveBeenCalledWith(jasmine.any(Number), 'Italic'); + }); + + it('strips out script tags from the value (XSS protection)', () => { + host.mv = createMetadata('Safe text', 'en'); + fixture.detectChanges(); + expect(span.innerHTML).not.toContain(' { + host.mv = createMetadata('', 'en'); + fixture.detectChanges(); + expect(span.innerHTML).not.toContain('onerror'); + expect(document.body.querySelector('h1')).toBeNull(); + }); }); diff --git a/src/app/shared/metadata.directive.ts b/src/app/shared/metadata.directive.ts index 468033a4543..011c5e2ca11 100644 --- a/src/app/shared/metadata.directive.ts +++ b/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);