Skip to content

Commit 57e7f3b

Browse files
authored
Merge pull request DSpace#6183 from tdonohue/sanitize_metadata
Minor updates to `dsMetadata` directive to sanitize metadata
2 parents 5709da0 + f9cec07 commit 57e7f3b

4 files changed

Lines changed: 131 additions & 6 deletions

File tree

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,92 @@
1+
/**
2+
* Regression test for a Cross-Site Scripting (XSS) vulnerability that was introduced by
3+
* https://github.com/DSpace/dspace-angular/pull/4776.
4+
*
5+
* This test creates a new item submission whose abstract contains such a payload and verifies that:
6+
* - the payload is NOT executed (no JavaScript side effect happens), and
7+
* - the dangerous `onerror` attribute is stripped from the rendered markup (while safe, surrounding
8+
* markup/tags are preserved),
9+
* when the abstract is displayed via the `[dsMetadata]` directive.
10+
*
11+
*/
12+
describe('Metadata XSS sanitization', () => {
13+
// A classic XSS payload: an image with a broken `src` so that its `onerror` handler fires as soon as
14+
// the browser tries (and fails) to load it. If the payload is not sanitized, `onerror` will run and set
15+
// `window.dsXssExecuted = true`. (NOTE: This uses "role=presentation" to avoid failing accessibility checks)
16+
const XSS_PAYLOAD = 'XSS Test <img src="x" onerror="window.dsXssExecuted = true;" role="presentation"/>';
17+
const SAFE_TEXT = 'XSS Test';
18+
const UNIQUE_TITLE = `XSS sanitization test item ${Date.now()}`;
19+
20+
/**
21+
* Asserts that the XSS payload has NOT executed on the current page.
22+
*/
23+
function assertXssDidNotExecute(): void {
24+
cy.window().then((win: any) => {
25+
expect(win.dsXssExecuted).to.not.equal(true);
26+
});
27+
}
28+
29+
it('should sanitize a malicious item abstract and not execute injected script when rendered via [dsMetadata]', () => {
30+
cy.visit('/mydspace');
31+
32+
// This page is restricted, so we will be shown the login form. Fill it out & submit.
33+
cy.env(['DSPACE_TEST_SUBMIT_USER', 'DSPACE_TEST_SUBMIT_USER_PASSWORD']).then(({ DSPACE_TEST_SUBMIT_USER, DSPACE_TEST_SUBMIT_USER_PASSWORD }) => {
34+
cy.loginViaForm(DSPACE_TEST_SUBMIT_USER, DSPACE_TEST_SUBMIT_USER_PASSWORD);
35+
});
36+
37+
// Start a submission
38+
cy.get('button[data-test="submission-dropdown"]').click();
39+
cy.get('#entityControlsDropdownMenu button[title="none"]').click();
40+
cy.get('ds-authorized-collection-selector input[type="search"]').type(Cypress.expose('DSPACE_TEST_SUBMIT_COLLECTION_NAME'));
41+
cy.get('ds-authorized-collection-selector button[title="'.concat(Cypress.expose('DSPACE_TEST_SUBMIT_COLLECTION_NAME')).concat('"]')).click();
42+
43+
// Give the item a unique (safe) title so we can reliably find it again afterward
44+
cy.get('#dc_title', { timeout: 10000 }).type(UNIQUE_TITLE);
45+
46+
// Enter our malicious abstract into the dc.description.abstract field
47+
cy.get('#dc_description_abstract').type(XSS_PAYLOAD);
48+
49+
// Save for Later to persist the (unsanitized, as stored) abstract on the workspace item
50+
cy.get('ds-submission-form-footer [data-test="save-for-later"]').click();
51+
52+
// "Save for Later" should send us to MyDSpace
53+
cy.url().should('include', '/mydspace');
54+
// The malicious payload should NOT have executed while the submission form/footer rendered the abstract
55+
assertXssDidNotExecute();
56+
57+
// Close any open notifications, to make sure they don't get in the way of next steps
58+
cy.get('[data-bs-dismiss="alert"]').click({ multiple: true });
59+
60+
// Search for the item we just created via its unique title
61+
cy.intercept('GET', '/server/api/discover/search/objects*').as('search-results');
62+
cy.get('[data-test="search-box"]').type(UNIQUE_TITLE);
63+
cy.get('[data-test="search-button"]').click();
64+
cy.wait('@search-results');
65+
66+
// Find the specific result matching our unique title, and scope all further assertions to it.
67+
cy.contains('[data-test="list-object"]', UNIQUE_TITLE, { timeout: 10000 })
68+
.should('exist')
69+
.as('result');
70+
71+
// The XSS payload must NOT have executed while the [dsMetadata] directive rendered the abstract
72+
assertXssDidNotExecute();
73+
74+
// The abstract should be rendered (via the [dsMetadata] directive) inside a truncatable part
75+
cy.get('@result').find('.item-list-abstract span').first().then(($abstract) => {
76+
// Sanitization removes *dangerous attributes* (like `onerror`), but it does NOT necessarily
77+
// remove the surrounding element itself (e.g. `<img>` is a permitted tag). So we assert that:
78+
// - the safe text content is still present,
79+
// - the `onerror` attribute is gone from the markup entirely,
80+
// - if the `<img>` tag survived sanitization, it has no `onerror` attribute on it.
81+
expect($abstract.text()).to.include(SAFE_TEXT);
82+
expect($abstract.html()).to.not.include('onerror');
83+
84+
const img = $abstract.find('img');
85+
if (img.length > 0) {
86+
// eslint-disable-next-line no-unused-expressions,@typescript-eslint/no-unused-expressions
87+
expect(img.attr('onerror')).to.be.undefined;
88+
}
89+
});
90+
});
91+
});
92+

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,14 @@ export class Metadata {
4848
* @param escapeHTML Whether the HTML is used inside a `[innerHTML]` attribute
4949
* @returns {MetadataValue[]} the matching values or an empty array.
5050
*/
51-
public static all(metadata: MetadataMapInterface, keyOrKeys: string | string[], hitHighlights?: MetadataMapInterface, filter?: MetadataValueFilter, escapeHTML?: boolean, limit?: number): MetadataValue[] {
51+
public static all(metadata: MetadataMapInterface = {}, keyOrKeys: string | string[], hitHighlights?: MetadataMapInterface, filter?: MetadataValueFilter, escapeHTML?: boolean, limit?: number): MetadataValue[] {
5252
const matches: MetadataValue[] = [];
5353
if (isNotEmpty(hitHighlights)) {
5454
for (const mdKey of Metadata.resolveKeys(hitHighlights, keyOrKeys)) {
5555
if (hitHighlights[mdKey]) {
5656
for (const candidate of hitHighlights[mdKey]) {
5757
if (Metadata.valueMatches(candidate as MetadataValue, filter) && (isEmpty(limit) || (hasValue(limit) && matches.length < limit))) {
58-
const nonHighlightValues = metadata[mdKey] as MetadataValue[];
58+
const nonHighlightValues = metadata?.[mdKey] as MetadataValue[];
5959
const nonHighlightValue = nonHighlightValues?.find((value: MetadataValue) => Metadata.valueMatches(value, filter));
6060
const language = nonHighlightValue?.language ?? candidate.language ?? null;
6161
matches.push(Object.assign(new MetadataValue(), candidate, { language }));
@@ -109,14 +109,14 @@ export class Metadata {
109109
* @param escapeHTML Whether the HTML is used inside a `[innerHTML]` attribute
110110
* @returns {MetadataValue} the first matching value, or `undefined`.
111111
*/
112-
public static first(metadata: MetadataMapInterface, keyOrKeys: string | string[], hitHighlights?: MetadataMapInterface, filter?: MetadataValueFilter, escapeHTML?: boolean): MetadataValue {
112+
public static first(metadata: MetadataMapInterface = {}, keyOrKeys: string | string[], hitHighlights?: MetadataMapInterface, filter?: MetadataValueFilter, escapeHTML?: boolean): MetadataValue {
113113
if (isNotEmpty(hitHighlights)) {
114114
for (const key of Metadata.resolveKeys(hitHighlights, keyOrKeys)) {
115115
const values: MetadataValue[] = hitHighlights[key] as MetadataValue[];
116116
if (values) {
117117
const metadataValue = values.find((value: MetadataValue) => Metadata.valueMatches(value, filter));
118118
if (metadataValue) {
119-
const nonHighlightValues = metadata[key] as MetadataValue[];
119+
const nonHighlightValues = metadata?.[key] as MetadataValue[];
120120
const nonHighlightValue = nonHighlightValues?.find((value: MetadataValue) => Metadata.valueMatches(value, filter));
121121
const language = nonHighlightValue?.language ?? metadataValue.language ?? null;
122122
return Object.assign(new MetadataValue(), metadataValue, { language });

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {
33
ComponentFixture,
44
TestBed,
55
} from '@angular/core/testing';
6+
import { DomSanitizer } from '@angular/platform-browser';
67

78
import { MetadataValue } from '../core/shared/metadata.models';
89
import { MetadataDirective } from './metadata.directive';
@@ -23,6 +24,7 @@ describe('MetadataDirective', () => {
2324
let fixture: ComponentFixture<HostComponent>;
2425
let host: HostComponent;
2526
let span: HTMLSpanElement;
27+
let sanitizer: DomSanitizer;
2628

2729
function createMetadata(value?: string, language?: string): MetadataValue {
2830
return {
@@ -42,6 +44,7 @@ describe('MetadataDirective', () => {
4244

4345
fixture = TestBed.createComponent(HostComponent);
4446
host = fixture.componentInstance;
47+
sanitizer = TestBed.inject(DomSanitizer);
4548
fixture.detectChanges();
4649
span = fixture.nativeElement.querySelector('span');
4750
});
@@ -95,4 +98,25 @@ describe('MetadataDirective', () => {
9598
fixture.detectChanges();
9699
expect(span.innerHTML.toLowerCase()).toBe('<em>italic</em>');
97100
});
101+
102+
it('sanitizes the value before setting innerHTML', () => {
103+
const sanitizeSpy = spyOn(sanitizer, 'sanitize').and.callThrough();
104+
host.mv = createMetadata('<em>Italic</em>', 'en');
105+
fixture.detectChanges();
106+
expect(sanitizeSpy).toHaveBeenCalledWith(jasmine.any(Number), '<em>Italic</em>');
107+
});
108+
109+
it('strips out script tags from the value (XSS protection)', () => {
110+
host.mv = createMetadata('<script>alert("XSS")</script>Safe text', 'en');
111+
fixture.detectChanges();
112+
expect(span.innerHTML).not.toContain('<script');
113+
expect(span.innerHTML).toContain('Safe text');
114+
});
115+
116+
it('strips out inline event handlers from the value (XSS protection)', () => {
117+
host.mv = createMetadata('<img src="x" onerror="document.body.insertAdjacentHTML(\'afterbegin\',\'<h1>XSS!</h1>\')">', 'en');
118+
fixture.detectChanges();
119+
expect(span.innerHTML).not.toContain('onerror');
120+
expect(document.body.querySelector('h1')).toBeNull();
121+
});
98122
});

src/app/shared/metadata.directive.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import {
44
inject,
55
Input,
66
Renderer2,
7+
SecurityContext,
78
} from '@angular/core';
9+
import { DomSanitizer } from '@angular/platform-browser';
810

911
import { MetadataValue } from '../core/shared/metadata.models';
1012
import { normalizeLanguageCode } from './utils/normalize-language-code-utils';
@@ -37,6 +39,12 @@ export class MetadataDirective {
3739
*/
3840
private renderer = inject(Renderer2);
3941

42+
/**
43+
* Angular DomSanitizer instance used to sanitize the metadata value before
44+
* inserting it into the DOM as innerHTML, preventing XSS attacks.
45+
*/
46+
private sanitizer = inject(DomSanitizer);
47+
4048
/**
4149
* Input property for the directive. Accepts a `MetadataValue` object.
4250
* When set, it updates the host element's `innerHTML` and `lang` attribute.
@@ -51,7 +59,7 @@ export class MetadataDirective {
5159
/**
5260
* Updates the host element's `innerHTML` and `lang` attribute based on the current `MetadataValue`.
5361
* - If `MetadataValue` is provided:
54-
* - Sets `innerHTML` to `MetadataValue.value` (or an empty string if `value` is null/undefined).
62+
* - Sets `innerHTML` to the sanitized `MetadataValue.value` (or an empty string if `value` is null/undefined).
5563
* - Sets the `lang` attribute to `MetadataValue.language` (or removes it if `language` is null/undefined).
5664
* - If `MetadataValue` is null/undefined:
5765
* - Clears the `innerHTML`.
@@ -60,7 +68,8 @@ export class MetadataDirective {
6068
private updateHost(): void {
6169
if (this._metadataValue) {
6270
const val = this._metadataValue.value ?? '';
63-
this.renderer.setProperty(this.el.nativeElement, 'innerHTML', val);
71+
const sanitizedVal = this.sanitizer.sanitize(SecurityContext.HTML, val) ?? '';
72+
this.renderer.setProperty(this.el.nativeElement, 'innerHTML', sanitizedVal);
6473
if (this._metadataValue.language) {
6574
const normalizedLang = normalizeLanguageCode(this._metadataValue.language);
6675
this.renderer.setAttribute(this.el.nativeElement, 'lang', normalizedLang);

0 commit comments

Comments
 (0)