diff --git a/core/src/components/loading/test/loading.spec.ts b/core/src/components/loading/test/loading.spec.ts index 11fa9658d75..8cc81e61512 100644 --- a/core/src/components/loading/test/loading.spec.ts +++ b/core/src/components/loading/test/loading.spec.ts @@ -4,6 +4,18 @@ import { config } from '../../../global/config'; import { Loading } from '../loading'; describe('loading: custom html', () => { + let consoleWarnSpy: jest.SpyInstance; + + beforeEach(() => { + consoleWarnSpy = jest.spyOn(console, 'warn'); + // Suppress console.warn output from polluting the test output + consoleWarnSpy.mockImplementation(() => {}); + }); + + afterEach(() => { + consoleWarnSpy.mockRestore(); + }); + it('should not allow for custom html by default', async () => { const page = await newSpecPage({ components: [Loading], @@ -15,6 +27,17 @@ describe('loading: custom html', () => { expect(content.querySelector('button.custom-html')).toBe(null); }); + it('should discard a message carrying an onload handler', async () => { + config.reset({ innerHTMLTemplatesEnabled: true }); + const page = await newSpecPage({ + components: [Loading], + html: ``, + }); + + const content = page.body.querySelector('.loading-content')!; + expect(content.innerHTML).toEqual(''); + }); + it('should allow for custom html', async () => { config.reset({ innerHTMLTemplatesEnabled: true }); const page = await newSpecPage({ diff --git a/core/src/utils/sanitization/index.ts b/core/src/utils/sanitization/index.ts index bb851fea7e0..ea590fdcdd9 100644 --- a/core/src/utils/sanitization/index.ts +++ b/core/src/utils/sanitization/index.ts @@ -1,4 +1,4 @@ -import { printIonError } from '@utils/logging'; +import { printIonError, printIonWarning } from '@utils/logging'; /** * Sanitize an untrusted HTML string. @@ -19,8 +19,8 @@ import { printIonError } from '@utils/logging'; * @param untrustedString - The HTML string to sanitize. Pass an * `IonicSafeString` to bypass sanitization, or `undefined` to short-circuit. * @returns The sanitized HTML string, or `undefined` if the input was - * `undefined`. Returns `''` if sanitization fails or the input contains - * an inline `onload=` handler. + * `undefined`. Returns `''` if sanitization fails or the input appears to + * carry an inline `onload` handler. */ export const sanitizeDOMString = (untrustedString: IonicSafeString | string | undefined): string | undefined => { try { @@ -32,12 +32,18 @@ export const sanitizeDOMString = (untrustedString: IonicSafeString | string | un } /** - * onload is fired when appending to a document - * fragment in Chrome. If a string - * contains onload then we should not - * attempt to add this to the fragment. + * In Blink a non-outermost `` (e.g. ``) fires + * its load handler while `innerHTML` below parses the string, so it runs + * before the attribute-stripping pass can remove it. The match has to + * stay this loose because attribute names are case-insensitive and + * whitespace is allowed before the `=`. */ - if (untrustedString.includes('onload=')) { + if (/onload\s*=/i.test(untrustedString)) { + printIonWarning( + 'sanitizeDOMString - Content was discarded because it appears to contain an onload handler:', + untrustedString.substring(0, 100) + ); + return ''; } diff --git a/core/src/utils/sanitization/test/sanitization.spec.ts b/core/src/utils/sanitization/test/sanitization.spec.ts index 2ca069e387f..fa854277cc1 100644 --- a/core/src/utils/sanitization/test/sanitization.spec.ts +++ b/core/src/utils/sanitization/test/sanitization.spec.ts @@ -1,6 +1,18 @@ import { blockedTags, IonicSafeString, reflectPropertiesToAttributes, sanitizeDOMString, sanitizeDOMTree } from '..'; describe('sanitizeDOMString', () => { + let consoleWarnSpy: jest.SpyInstance; + + beforeEach(() => { + consoleWarnSpy = jest.spyOn(console, 'warn'); + // Suppress console.warn output from polluting the test output + consoleWarnSpy.mockImplementation(() => {}); + }); + + afterEach(() => { + consoleWarnSpy.mockRestore(); + }); + it('disable sanitizer', () => { enableSanitizer(false); expect(sanitizeDOMString('')).toEqual( @@ -26,6 +38,33 @@ describe('sanitizeDOMString', () => { ).toEqual(''); }); + it('filter onload', () => { + /** + * Only a non-outermost `` fires its load handler mid-parse, so the + * nesting is what makes these payloads real. + */ + expect(sanitizeDOMString('')).toEqual(''); + expect(sanitizeDOMString('')).toEqual(''); + expect(sanitizeDOMString('')).toEqual(''); + expect(sanitizeDOMString('')).toEqual(''); + }); + + it('filter onload discards benign content too (known false positive)', () => { + /** + * The check runs before parsing, so it can't tell an attribute from a + * URL or from prose. Both of these lose their whole string. + */ + expect(sanitizeDOMString('docs')).toEqual(''); + expect(sanitizeDOMString('docs')).toEqual(''); + }); + + it('warn when content is discarded by the onload check', () => { + sanitizeDOMString(''); + + expect(consoleWarnSpy).toHaveBeenCalledTimes(1); + expect(consoleWarnSpy.mock.calls[0][0]).toContain('onload handler'); + }); + it('filter href JS', () => { expect(sanitizeDOMString('harmless link')).toEqual( 'harmless link'