From 951f11d3132c6a26299ea79c131d65aa59a55202 Mon Sep 17 00:00:00 2001 From: erseco Date: Fri, 5 Jun 2026 17:40:57 +0100 Subject: [PATCH 1/3] fix(security): resolve 82 CodeQL code-scanning alerts Fix the safe, mechanical security-warning classes flagged by CodeQL across the backend, frontend, iDevice runtimes, exporters and legacy importers. Every change is minimal and behaviour-preserving, with a colocated test. - double-escaping: decode & last so &lt; no longer collapses to < - incomplete-(multi-character-)sanitization: loop tag/handler strips to a fixed point; add global flags; escape backslash before the delimiter - bad-tag-filter: tolerant end-tag regex (whitespace/newline/case) - incomplete-url-scheme-check: also reject vbscript:/data: (entity/ws tolerant) - incomplete-url-substring-sanitization: new URL() exact host/path checks - prototype-pollution: guard __proto__/prototype/constructor in deep-merge - cross-window-information-leak: drop postMessage('*') fallback - insecure-randomness: Web Crypto instead of the non-crypto PRNG - insecure-download / functionality-from-untrusted-source: scheme validation, SRI - overly-large-range: narrow [a-zA-z] to [a-zA-Z] (export + edition twins) - actions/missing-workflow-permissions: least-privilege permissions block Guards a RangeError in the numeric-entity decoder and keeps the slide SVG scrubber's src/export twins in sync. 2 stored-xss (inert DOMParser round-trip) and 4 url-substring alerts inside test bodies are skipped with documented rationale. The 146 js/xss-through-dom alerts in iDevice rendering are deferred. --- .github/workflows/e2e-db-matrix.yml | 3 + public/app/adapters/LinkValidationAdapter.js | 48 ++++++- .../adapters/LinkValidationAdapter.test.js | 67 +++++++++ public/app/common/LatexPreRenderer.js | 38 +++-- public/app/common/LatexPreRenderer.test.js | 72 ++++++++++ public/app/common/common.js | 58 ++++++-- public/app/common/common.test.js | 56 ++++++++ public/app/common/mindmaps/src/index.html | 2 +- public/app/core/EmbeddingBridge.js | 9 +- public/app/core/EmbeddingBridge.test.js | 33 +++-- public/app/locate/locale.js | 21 ++- public/app/locate/locale.test.js | 61 ++++++++ .../interface/elements/previewPanel.js | 2 +- .../interface/elements/previewPanel.test.js | 22 +++ public/app/yjs/AssetManager.js | 9 +- public/app/yjs/AssetManager.test.js | 20 +++ public/app/yjs/ResourceFetcher.js | 27 ++++ public/app/yjs/ResourceFetcher.test.js | 74 +++++++++- public/app/yjs/YjsProjectBridge.js | 28 +++- public/app/yjs/YjsProjectBridge.test.js | 38 +++++ .../edition/adaptative-quiz.js | 13 +- .../edition/adaptative-quiz.test.js | 29 ++++ .../base/az-quiz-game/edition/az-quiz-game.js | 15 +- .../az-quiz-game/edition/az-quiz-game.test.js | 61 +++++++- .../base/crossword/edition/crossword.js | 20 ++- .../base/crossword/edition/crossword.test.js | 29 ++++ .../base/discover/edition/discover.js | 14 +- .../base/discover/edition/discover.test.js | 86 ++++++++++- .../base/dragdrop/edition/dragdrop.js | 4 +- .../base/dragdrop/edition/dragdrop.test.js | 29 ++++ .../edition/electrical-circuits.js | 16 ++- .../edition/electrical-circuits.test.js | 118 ++++++++++++++++ .../base/flipcards/edition/flipcards.js | 19 ++- .../base/flipcards/edition/flipcards.test.js | 79 +++++++++++ .../perm/idevices/base/guess/edition/guess.js | 14 +- .../idevices/base/guess/edition/guess.test.js | 59 ++++++++ .../edition/interactive-video.js | 19 ++- .../edition/interactive-video.test.js | 22 +++ .../export/interactive-video.js | 38 ++++- .../export/interactive-video.test.js | 126 +++++++++++++++++ .../base/mathproblems/edition/mathproblems.js | 2 +- .../base/mathproblems/export/mathproblems.js | 2 +- .../mathproblems/export/mathproblems.test.js | 58 ++++++++ .../export/quick-questions-video.js | 25 ++-- .../export/quick-questions-video.test.js | 133 ++++++++++++++++++ .../idevices/base/relate/edition/relate.js | 34 +++-- .../base/relate/edition/relate.test.js | 124 ++++++++++++++++ .../perm/idevices/base/slide/export/slide.js | 34 ++++- .../idevices/base/slide/export/slide.test.js | 38 +++++ .../idevices/base/slide/src/sanitizer.test.js | 36 +++++ .../perm/idevices/base/slide/src/sanitizer.ts | 32 ++++- .../edition/three-sixty-viewer.js | 8 +- .../edition/three-sixty-viewer.test.js | 28 ++++ .../idevices/base/trivial/edition/trivial.js | 25 +++- .../base/trivial/edition/trivial.test.js | 74 ++++++++++ .../base/udl-content/edition/udl-content.js | 25 ++-- .../udl-content/edition/udl-content.test.js | 113 +++++++++++++++ .../base/word-search/edition/word-search.js | 16 ++- .../word-search/edition/word-search.test.js | 116 +++++++++++++++ scripts/static-bundle/static-index.html | 16 +-- src/cli/commands/translations.spec.ts | 28 ++++ src/cli/commands/translations.ts | 4 +- src/cli/commands/update-licenses.spec.ts | 18 +++ src/cli/commands/update-licenses.ts | 14 +- src/routes/project.spec.ts | 56 +++++++- src/routes/project.ts | 28 +++- src/services/folder-manager.spec.ts | 13 ++ src/services/folder-manager.ts | 7 +- src/services/link-validator.spec.ts | 68 +++++++++ src/services/link-validator.ts | 31 +++- .../exporters/PrintPreviewExporter.spec.ts | 28 +--- .../export/generators/I18nGenerator.spec.ts | 14 ++ src/shared/export/generators/I18nGenerator.ts | 4 +- .../prerender/ServerLatexPreRenderer.spec.ts | 40 +++++- .../prerender/ServerLatexPreRenderer.ts | 18 ++- .../ServerMermaidPreRenderer.spec.ts | 33 ++++- .../prerender/ServerMermaidPreRenderer.ts | 14 +- src/shared/import/ElpxImporter.spec.ts | 64 +++++++++ src/shared/import/ElpxImporter.ts | 13 +- src/shared/import/LegacyXmlParser.spec.ts | 29 ++++ src/shared/import/LegacyXmlParser.ts | 4 +- .../legacy-handlers/BaseLegacyHandler.spec.ts | 34 +++++ .../legacy-handlers/BaseLegacyHandler.ts | 33 +++-- .../import/legacy-handlers/GameHandler.ts | 12 +- .../InteractiveVideoHandler.spec.ts | 60 ++++++++ .../InteractiveVideoHandler.ts | 4 +- .../import/legacy-handlers/handlers.spec.ts | 51 +++++++ .../specs/idevices/image-gallery.spec.ts | 6 +- .../latex-text-idevice-export.spec.ts | 2 +- views/admin/index.njk | 2 +- 90 files changed, 2925 insertions(+), 244 deletions(-) create mode 100644 public/files/perm/idevices/base/electrical-circuits/edition/electrical-circuits.test.js create mode 100644 public/files/perm/idevices/base/mathproblems/export/mathproblems.test.js create mode 100644 public/files/perm/idevices/base/quick-questions-video/export/quick-questions-video.test.js create mode 100644 public/files/perm/idevices/base/relate/edition/relate.test.js create mode 100644 public/files/perm/idevices/base/trivial/edition/trivial.test.js create mode 100644 public/files/perm/idevices/base/udl-content/edition/udl-content.test.js create mode 100644 public/files/perm/idevices/base/word-search/edition/word-search.test.js create mode 100644 src/shared/import/legacy-handlers/InteractiveVideoHandler.spec.ts diff --git a/.github/workflows/e2e-db-matrix.yml b/.github/workflows/e2e-db-matrix.yml index 43abebdd3d..3b6a8366fc 100644 --- a/.github/workflows/e2e-db-matrix.yml +++ b/.github/workflows/e2e-db-matrix.yml @@ -6,6 +6,9 @@ on: - main workflow_dispatch: +permissions: + contents: read + concurrency: group: ${{ github.workflow }}-${{ github.ref }} cancel-in-progress: true diff --git a/public/app/adapters/LinkValidationAdapter.js b/public/app/adapters/LinkValidationAdapter.js index adc912c1ef..ac3987cf4f 100644 --- a/public/app/adapters/LinkValidationAdapter.js +++ b/public/app/adapters/LinkValidationAdapter.js @@ -136,7 +136,8 @@ export default class LinkValidationAdapter { /** * Remove invalid/non-validatable links - * Filters out: empty, anchors (#), javascript:, data: URLs + * Filters out: empty, anchors (#), and dangerous script schemes + * (javascript:, vbscript:, data:). * @param {Array<{url: string, count: number}>} links * @returns {Array<{url: string, count: number}>} * @private @@ -145,12 +146,53 @@ export default class LinkValidationAdapter { return links.filter((link) => { if (!link.url || link.url.trim() === '') return false; if (link.url.startsWith('#')) return false; - if (link.url.startsWith('javascript:')) return false; - if (link.url.startsWith('data:')) return false; + if (this._hasDangerousScheme(link.url)) return false; return true; }); } + /** + * Detect whether a URL uses a dangerous script-bearing scheme + * (javascript:, vbscript:, data:). + * + * Normalizes the URL before matching so the check cannot be bypassed + * with mixed case, leading/embedded whitespace and control characters, + * or HTML character entities (e.g. "javascript:"). + * + * @param {string} url + * @returns {boolean} true when the URL resolves to a dangerous scheme + * @private + */ + _hasDangerousScheme(url) { + if (typeof url !== 'string') return false; + + // Safely convert a numeric code point, returning '' for invalid or + // out-of-range values so String.fromCodePoint cannot throw a + // RangeError (e.g. on "�" or "�"). Dropping such + // junk only makes scheme detection stricter, never weaker. + const safeFromCodePoint = (cp) => + Number.isInteger(cp) && cp >= 0 && cp <= 0x10ffff ? String.fromCodePoint(cp) : ''; + + // Decode common HTML entities (named and numeric) so encoded + // schemes such as "javascript:" are caught. + let normalized = url.replace(/&#x([0-9a-f]+);?/gi, (_m, hex) => + safeFromCodePoint(Number.parseInt(hex, 16)), + ); + normalized = normalized.replace(/&#(\d+);?/g, (_m, dec) => + safeFromCodePoint(Number.parseInt(dec, 10)), + ); + const namedEntities = { amp: '&', lt: '<', gt: '>', quot: '"', apos: "'", colon: ':', tab: '\t', newline: '\n' }; + normalized = normalized.replace(/&(amp|lt|gt|quot|apos|colon|tab|newline);/gi, (_m, name) => namedEntities[name.toLowerCase()] ?? _m); + + // Strip whitespace and control characters (incl. NUL and DEL) that + // browsers ignore when resolving the scheme, then lowercase for + // comparison. \x00-\x20 covers C0 controls + space; \x7f is DEL. + // eslint-disable-next-line no-control-regex + const stripped = normalized.replace(/[\x00-\x20\x7f]+/g, '').toLowerCase(); + + return stripped.startsWith('javascript:') || stripped.startsWith('vbscript:') || stripped.startsWith('data:'); + } + /** * Deduplicate links, keeping the one with highest count * @param {Array<{url: string, count: number}>} links diff --git a/public/app/adapters/LinkValidationAdapter.test.js b/public/app/adapters/LinkValidationAdapter.test.js index 4d2582faf9..08821dc3a2 100644 --- a/public/app/adapters/LinkValidationAdapter.test.js +++ b/public/app/adapters/LinkValidationAdapter.test.js @@ -432,6 +432,73 @@ describe('LinkValidationAdapter', () => { expect(result.length).toBe(1); expect(result[0].url).toBe('https://valid.com'); }); + + it('should reject vbscript:, mixed-case, whitespace-prefixed and entity-encoded script schemes', () => { + const dangerous = [ + { url: 'vbscript:msgbox(1)', count: 1 }, + { url: 'VBScript:MsgBox(1)', count: 1 }, + { url: 'JavaScript:alert(1)', count: 1 }, + { url: ' javascript:alert(1)', count: 1 }, + { url: '\t\njavascript:alert(1)', count: 1 }, + { url: 'java\x00script:alert(1)', count: 1 }, + { url: 'DATA:text/html,', count: 1 }, + { url: 'javascript:alert(1)', count: 1 }, + { url: 'javascript:alert(1)', count: 1 }, + ]; + const result = adapter._removeInvalidLinks(dangerous); + + expect(result).toEqual([]); + }); + + it('should preserve legitimate safe links', () => { + const safe = [ + { url: 'http://example.com', count: 1 }, + { url: 'https://example.com/page', count: 1 }, + { url: '//cdn.example.com/file.js', count: 1 }, + { url: 'images/photo.jpg', count: 1 }, + { url: 'mailto:user@example.com', count: 1 }, + { url: 'tel:+1234567890', count: 1 }, + ]; + const result = adapter._removeInvalidLinks(safe); + + expect(result.map((l) => l.url)).toEqual([ + 'http://example.com', + 'https://example.com/page', + '//cdn.example.com/file.js', + 'images/photo.jpg', + 'mailto:user@example.com', + 'tel:+1234567890', + ]); + }); + }); + + describe('_hasDangerousScheme', () => { + it('should detect dangerous schemes through obfuscation', () => { + expect(adapter._hasDangerousScheme('javascript:alert(1)')).toBe(true); + expect(adapter._hasDangerousScheme('vbscript:msgbox(1)')).toBe(true); + expect(adapter._hasDangerousScheme('data:text/html,abc')).toBe(true); + expect(adapter._hasDangerousScheme(' JAVASCRIPT:alert(1)')).toBe(true); + expect(adapter._hasDangerousScheme('javascript:alert(1)')).toBe(true); + expect(adapter._hasDangerousScheme('javascript:alert(1)')).toBe(true); + }); + + it('should not flag safe links and non-string input', () => { + expect(adapter._hasDangerousScheme('https://example.com')).toBe(false); + expect(adapter._hasDangerousScheme('images/photo.jpg')).toBe(false); + expect(adapter._hasDangerousScheme('mailto:user@example.com')).toBe(false); + expect(adapter._hasDangerousScheme(null)).toBe(false); + }); + + it('should not throw on out-of-range numeric entities', () => { + // String.fromCodePoint throws RangeError for code points > 0x10FFFF + // or negative; the guard must drop them instead of crashing. + expect(() => adapter._hasDangerousScheme('�')).not.toThrow(); + expect(() => adapter._hasDangerousScheme('�')).not.toThrow(); + expect(adapter._hasDangerousScheme('�')).toBe(false); + expect(adapter._hasDangerousScheme('�')).toBe(false); + // A real scheme padded with an out-of-range entity is still caught. + expect(adapter._hasDangerousScheme('�javascript:alert(1)')).toBe(true); + }); }); describe('_deduplicateLinks', () => { diff --git a/public/app/common/LatexPreRenderer.js b/public/app/common/LatexPreRenderer.js index 467aa72dfe..3e7cb8e2eb 100644 --- a/public/app/common/LatexPreRenderer.js +++ b/public/app/common/LatexPreRenderer.js @@ -101,18 +101,26 @@ let clean = latexWithHtml.replace(//gi, '\n'); // Remove any other HTML tags FIRST (before decoding entities) - // This prevents decoded < and > from being mistaken for tags - clean = clean.replace(/<[^>]+>/g, ''); - - // Decode common HTML entities AFTER removing tags + // This prevents decoded < and > from being mistaken for tags. + // Apply repeatedly until the string stops changing so that removing one + // tag cannot splice two halves into a new tag (e.g. "<b>" -> "b>"). + let prevClean; + do { + prevClean = clean; + clean = clean.replace(/<[^>]+>/g, ''); + } while (clean !== prevClean); + + // Decode common HTML entities AFTER removing tags. + // Decode & LAST so that an already-escaped sequence like "&lt;" + // (the literal text "<") does not get double-decoded into "<". clean = clean .replace(/ /gi, ' ') .replace(/</g, '<') .replace(/>/g, '>') - .replace(/&/g, '&') .replace(/"/g, '"') .replace(/&#(\d+);/g, (_, code) => String.fromCharCode(parseInt(code, 10))) - .replace(/&#x([a-fA-F0-9]+);/g, (_, code) => String.fromCharCode(parseInt(code, 16))); + .replace(/&#x([a-fA-F0-9]+);/g, (_, code) => String.fromCharCode(parseInt(code, 16))) + .replace(/&/g, '&'); return clean; } @@ -689,7 +697,7 @@ function isNumberedEquation(latex) { const envMatch = latex.match(/\\begin\{([^}*]+)\*?\}/); if (!envMatch) return false; - const envName = envMatch[1].replace('*', ''); // Remove * for unnumbered variants + const envName = envMatch[1].replace(/\*/g, ''); // Remove * for unnumbered variants return NUMBERED_EQUATION_ENVS.has(envName) && !envMatch[1].endsWith('*'); } @@ -762,6 +770,14 @@ * @returns {Promise<{html: string, hasLatex: boolean, latexRendered: boolean, count: number}>} */ async function preRenderPerIdevice(html, preserved) { + // Security note (stored-xss): DOMParser.parseFromString builds an INERT + // document - scripts are NOT executed here, the document is only used to + // locate and re-serialize content. The ONLY new HTML this routine injects + // back via innerHTML is the MathJax library-rendered / wrapper + // produced by createRenderedWrapperHtml(), whose data-latex attribute is + // escaped through escapeHtmlAttribute(). Every other byte is a faithful + // round-trip of the author's own already-HTML content (the trusted source + // of their own page), so this transformation introduces no new XSS sink. const parser = new DOMParser(); const doc = parser.parseFromString(html, 'text/html'); @@ -889,7 +905,13 @@ return await preRenderPerIdevice(safeHtml, preserved); } - // Parse HTML into DOM (simple mode without iDevice scoping) + // Parse HTML into DOM (simple mode without iDevice scoping). + // Security note (stored-xss): identical reasoning to preRenderPerIdevice - + // the parsed document is INERT (no script execution), only used to find + // and re-serialize content. The single new HTML payload injected via + // innerHTML is the MathJax library-rendered / wrapper with an + // escaped data-latex attribute; the remaining output is a faithful + // round-trip of the author's own already-HTML content. const parser = new DOMParser(); const doc = parser.parseFromString(safeHtml, 'text/html'); diff --git a/public/app/common/LatexPreRenderer.test.js b/public/app/common/LatexPreRenderer.test.js index 35478af576..18190e6fd1 100644 --- a/public/app/common/LatexPreRenderer.test.js +++ b/public/app/common/LatexPreRenderer.test.js @@ -252,6 +252,44 @@ describe('LatexPreRenderer', () => { expect(result).toContain('\\begin{aligned}'); expect(result).toContain('x &= 1'); }); + + // Security: incomplete-multi-character-sanitization (CodeQL). + // A single-pass tag strip can be defeated because removing one match + // can splice two halves into a NEW tag. The strip must run to a fixed + // point so no b'; + const result = LatexPreRenderer._cleanLatexFromHtml(input); + + // The security property: removing one tag must not splice halves into + // a surviving " { + const input = '<b>x = 1<src>'; + const result = LatexPreRenderer._cleanLatexFromHtml(input); + + // After fixed-point removal no opening "<" (start of a tag) may remain. + expect(result).not.toContain('<'); + expect(result).toContain('x = 1'); + }); + + // Security: double-escaping (CodeQL). Decoding & must happen LAST so + // an already-escaped sequence like "&lt;" (the literal text "<") + // is not double-decoded into "<". + test('does not double-decode &lt; into <', () => { + expect(LatexPreRenderer._cleanLatexFromHtml('&lt;')).toBe('<'); + }); + + test('does not double-decode &amp; into &', () => { + expect(LatexPreRenderer._cleanLatexFromHtml('&amp;')).toBe('&'); + }); + + test('still decodes a single & to & (legitimate input preserved)', () => { + expect(LatexPreRenderer._cleanLatexFromHtml('a && b')).toBe('a && b'); + }); }); describe('preRender', () => { @@ -421,6 +459,40 @@ describe('LatexPreRenderer', () => { expect(result.html).not.toContain('data-latex="\\[
'); }); + // Security: stored-xss (CodeQL) on the DOMParser round-trip. + // The only NEW HTML preRender injects is the MathJax library-rendered + // wrapper, with its data-latex attribute escaped via escapeHtmlAttribute(). + // LaTeX that contains a quote/angle-bracket payload must be neutralised in + // the attribute (escaped), never emitted as a live attribute/tag. + test('escapes quote/angle payloads inside the injected math wrapper attribute', async () => { + const html = '

\\(x" onmouseover="alert(1)\\)

'; + const result = await LatexPreRenderer.preRender(html); + + expect(result.latexRendered).toBe(true); + // The injected wrapper is present and library-rendered. + expect(result.html).toContain('exe-math-rendered'); + // The wrapper's own data-latex attribute (built by THIS file via + // escapeHtmlAttribute) must entity-escape the double quote so the + // payload cannot break out into a live onmouseover handler. + const dataLatexMatch = result.html.match(/data-latex="([^"]*)"/); + expect(dataLatexMatch).not.toBeNull(); + expect(dataLatexMatch[1]).toContain('" onmouseover="alert(1)'); + expect(dataLatexMatch[1]).not.toContain('"'); + }); + + test('does not introduce a new live - + diff --git a/public/app/core/EmbeddingBridge.js b/public/app/core/EmbeddingBridge.js index 9fd84ba6dd..5c6840cb57 100644 --- a/public/app/core/EmbeddingBridge.js +++ b/public/app/core/EmbeddingBridge.js @@ -457,12 +457,9 @@ export default class EmbeddingBridge { try { window.parent.postMessage(message, this.parentOrigin); } catch (e) { - // If origin validation fails, try with '*' for same-origin iframes - if (e.name === 'DataCloneError') { - getLogger().error('[EmbeddingBridge] Cannot serialize message:', e); - } else { - window.parent.postMessage(message, '*'); - } + // Never broadcast to '*' on failure: it would leak the message to + // any origin. Log the error and drop the message instead. + getLogger().error('[EmbeddingBridge] Failed to post message to parent:', e); } } } diff --git a/public/app/core/EmbeddingBridge.test.js b/public/app/core/EmbeddingBridge.test.js index acf997adc1..0353e5b345 100644 --- a/public/app/core/EmbeddingBridge.test.js +++ b/public/app/core/EmbeddingBridge.test.js @@ -1036,24 +1036,32 @@ describe('EmbeddingBridge', () => { ); }); - it('should fallback to * on non-DataCloneError', () => { + it('should never broadcast to * when posting to parentOrigin throws', () => { bridge.parentOrigin = 'https://parent.com'; - window.parent.postMessage = vi.fn() - .mockImplementationOnce(() => { - throw new Error('Some other error'); - }) - .mockImplementationOnce(() => {}); + const otherError = new Error('Some other error'); + window.parent.postMessage = vi.fn(() => { + throw otherError; + }); bridge.postToParent({ type: 'TEST' }); - expect(window.parent.postMessage).toHaveBeenCalledTimes(2); - expect(window.parent.postMessage).toHaveBeenLastCalledWith( + // Only the single targeted attempt is made; no '*' fallback. + expect(window.parent.postMessage).toHaveBeenCalledTimes(1); + expect(window.parent.postMessage).toHaveBeenCalledWith( { type: 'TEST' }, + 'https://parent.com' + ); + expect(window.parent.postMessage).not.toHaveBeenCalledWith( + expect.anything(), '*' ); + expect(window.AppLogger.error).toHaveBeenCalledWith( + '[EmbeddingBridge] Failed to post message to parent:', + otherError + ); }); - it('should log error on DataCloneError', () => { + it('should log error on DataCloneError without broadcasting to *', () => { bridge.parentOrigin = 'https://parent.com'; const dataCloneError = new Error('Cannot clone'); dataCloneError.name = 'DataCloneError'; @@ -1063,8 +1071,13 @@ describe('EmbeddingBridge', () => { bridge.postToParent({ type: 'TEST' }); + expect(window.parent.postMessage).toHaveBeenCalledTimes(1); + expect(window.parent.postMessage).not.toHaveBeenCalledWith( + expect.anything(), + '*' + ); expect(window.AppLogger.error).toHaveBeenCalledWith( - '[EmbeddingBridge] Cannot serialize message:', + '[EmbeddingBridge] Failed to post message to parent:', dataCloneError ); }); diff --git a/public/app/locate/locale.js b/public/app/locate/locale.js index a0cb0c547a..4faee44fc9 100644 --- a/public/app/locate/locale.js +++ b/public/app/locate/locale.js @@ -1,3 +1,18 @@ +/** + * Escape a string for use as a legacy escaped catalogue key. + * + * Backslashes are escaped FIRST and quotes second so the sanitization is + * complete: an input such as `\"` becomes `\\\"` instead of the ambiguous + * `\\"`. Inputs without backslashes (the common case, including every real + * translation source string) are unaffected, preserving existing lookups. + * + * @param {string} string + * @returns {string} + */ +function escapeCatalogueKey(string) { + return string.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); +} + export default class Locale { constructor(app) { this.app = app; @@ -67,7 +82,7 @@ export default class Locale { const catalogue = this.strings?.translations; if (catalogue) { // Try exact key first (server returns unescaped keys) - const key = string in catalogue ? string : string.replace(/"/g, '\\"'); + const key = string in catalogue ? string : escapeCatalogueKey(string); if (key in catalogue) { let res = catalogue[key].replace(/\\"/g, '"').replace(/\\\//g, '/'); if (res.startsWith('~')) res = res.substring(1); @@ -84,7 +99,7 @@ export default class Locale { const catalogue = this.c_strings?.translations; if (catalogue) { // Try exact key first (server returns unescaped keys) - const key = string in catalogue ? string : string.replace(/"/g, '\\"'); + const key = string in catalogue ? string : escapeCatalogueKey(string); if (key in catalogue) { let res = catalogue[key].replace(/\\"/g, '"').replace(/\\\//g, '/'); if (res.startsWith('~')) res = res.substring(1); @@ -152,7 +167,7 @@ export default class Locale { */ getTranslation(string, lang, idevice) { if (typeof string != 'string') return ''; - string = string ? string.replace(/"/g, '\\"') : ''; + string = string ? escapeCatalogueKey(string) : ''; lang = lang ? lang : this.lang; // Idevice po translation if (idevice) { diff --git a/public/app/locate/locale.test.js b/public/app/locate/locale.test.js index d4e3669c66..3485296986 100644 --- a/public/app/locate/locale.test.js +++ b/public/app/locate/locale.test.js @@ -235,4 +235,65 @@ describe('Locale translations', () => { expect(locale.getGUITranslation('text "quoted"')).toBe('text "quoted"'); }); }); + + describe('catalogue key escaping (incomplete-sanitization fix)', () => { + // Real XLF keys are stored verbatim (unescaped). The first lookup + // branch handles those, so quoted source strings keep resolving exactly as + // before. These cases lock in that legitimate-input behaviour. + it('getGUITranslation still resolves a real quoted source key', () => { + locale.strings = { translations: { 'Click the "Reply" button.': '~Pulsa el botón "Responder".' } }; + expect(locale.getGUITranslation('Click the "Reply" button.')).toBe('Pulsa el botón "Responder".'); + }); + + it('getContentTranslation still resolves a real quoted source key', () => { + locale.c_strings = { translations: { 'Open "{name}".': 'Abrir "{name}".' } }; + expect(locale.getContentTranslation('Open "{name}".')).toBe('Abrir "{name}".'); + }); + + // Security property (incomplete-sanitization): backslashes in the input + // must be escaped BEFORE quotes, so a literal backslash in the lookup key + // is doubled. The naive `replace(/"/g, '\\"')` left backslashes untouched, + // letting `\"` collapse ambiguously. Inputs WITHOUT backslashes (every real + // translation source string) escape identically, preserving lookups. + + it('getGUITranslation doubles backslashes when building the fallback key', () => { + // Plain quote (no backslash) must still escape to `\"` and resolve the + // legacy escaped key exactly as before — behaviour preserved. + // Key `\\"` in JS source = backslash + quote. + locale.strings = { translations: { '\\"': 'plain-quote-value' } }; + expect(locale.getGUITranslation('"')).toBe('plain-quote-value'); + + // Backslash-bearing input `\y` is NOT a direct key, so it goes through the + // escaping fallback. Backslash-first escaping builds key `\\y` (doubled + // backslash + y). The catalogue exposes only that correctly-escaped key + // plus the naive single-backslash form; the fix must resolve the former. + locale.strings = { + translations: { '\\\\y': 'correct', z: 'unused' }, + }; + expect(locale.getGUITranslation('\\y')).toBe('correct'); + }); + + it('getContentTranslation doubles backslashes when building the fallback key', () => { + locale.c_strings = { translations: { '\\"': 'plain-quote-value' } }; + expect(locale.getContentTranslation('"')).toBe('plain-quote-value'); + + locale.c_strings = { + translations: { '\\\\y': 'correct', z: 'unused' }, + }; + expect(locale.getContentTranslation('\\y')).toBe('correct'); + }); + + it('getTranslation doubles backslashes when building the lookup key', () => { + // Plain quote still escapes to `\"` and resolves the legacy escaped key. + locale.strings = { translations: { '\\"': 'plain-quote-value' } }; + expect(locale.getTranslation('"')).toBe('plain-quote-value'); + + // getTranslation escapes the input unconditionally; with backslash-first + // escaping `\y` becomes `\\y` (doubled), matching the correctly-escaped key. + locale.strings = { + translations: { '\\\\y': 'correct', z: 'unused' }, + }; + expect(locale.getTranslation('\\y')).toBe('correct'); + }); + }); }); diff --git a/public/app/workarea/interface/elements/previewPanel.js b/public/app/workarea/interface/elements/previewPanel.js index 2214ce8a4a..30889795f5 100644 --- a/public/app/workarea/interface/elements/previewPanel.js +++ b/public/app/workarea/interface/elements/previewPanel.js @@ -1156,7 +1156,7 @@ export default class PreviewPanelManager { // Inline JS: (note: space inside tag is common) html = html.replace( - /]*src=["']([^"']+)["'][^>]*>[^<]*<\/script>/gi, + /]*src=["']([^"']+)["'][^>]*>[^<]*<\/script\s*>/gi, (match, src) => { const content = this._decodeFileContent(this._findFileContent(files, src)); if (content) { diff --git a/public/app/workarea/interface/elements/previewPanel.test.js b/public/app/workarea/interface/elements/previewPanel.test.js index c10b3dc287..6fac234ed0 100644 --- a/public/app/workarea/interface/elements/previewPanel.test.js +++ b/public/app/workarea/interface/elements/previewPanel.test.js @@ -1593,6 +1593,28 @@ describe('PreviewPanelManager', () => { expect(result).not.toContain('src="app.js"'); }); + it('should inline JS script tags with whitespace before the closing angle bracket', () => { + const html = ''; + const files = { 'app.js': 'console.log("hello")' }; + + const result = manager._inlineResources(html, files); + + expect(result).toContain(' tag from an HTML string. + * + * The end-tag pattern tolerates optional whitespace before the closing angle + * bracket (e.g. ``) and is case-insensitive, so real-world end tags + * are not missed (bad-tag-filter). The replacement is applied repeatedly until + * the string stops changing, so a nested/obfuscated payload such as + * `ipt>` cannot splice two halves into a fresh `

bye

'); + expect(out).toBe('

hi

bye

'); + expect(out.toLowerCase()).not.toContain(' { + const out = stripScriptTags('after'); + expect(out).toBe('after'); + expect(out.toLowerCase()).not.toContain(' { + const out = stripScriptTags('beforedone'); + expect(out.toLowerCase()).not.toContain(' { + const html = '

keep me

'; + expect(stripScriptTags(html)).toBe(html); + }); + + it('does not strip text that merely mentions script without a real tag', () => { + const html = '

The word script is fine here

'; + expect(stripScriptTags(html)).toBe(html); + }); +}); diff --git a/public/files/perm/idevices/base/adaptative-quiz/edition/adaptative-quiz.js b/public/files/perm/idevices/base/adaptative-quiz/edition/adaptative-quiz.js index 90124ad4ae..4855ba86cd 100644 --- a/public/files/perm/idevices/base/adaptative-quiz/edition/adaptative-quiz.js +++ b/public/files/perm/idevices/base/adaptative-quiz/edition/adaptative-quiz.js @@ -158,10 +158,15 @@ var $exeDevice = { normalizeQuestionsPerRound: value => Math.max(parseInt(value, 10) || 1, 1), - removeTags: str => - String(str ?? '') - .replace(/<[^>]*>/g, '') - .trim(), + removeTags: str => { + let result = String(str ?? ''); + let prev; + do { + prev = result; + result = result.replace(/<[^>]*>/g, ''); + } while (result !== prev); + return result.trim(); + }, /** * Reusable audio input with voice recorder, matching quick-questions' diff --git a/public/files/perm/idevices/base/adaptative-quiz/edition/adaptative-quiz.test.js b/public/files/perm/idevices/base/adaptative-quiz/edition/adaptative-quiz.test.js index 5ee6a7b9ab..b63341f97d 100644 --- a/public/files/perm/idevices/base/adaptative-quiz/edition/adaptative-quiz.test.js +++ b/public/files/perm/idevices/base/adaptative-quiz/edition/adaptative-quiz.test.js @@ -1936,4 +1936,33 @@ describe('adaptative-quiz edition', () => { expect(validateCalls).toBe(0); }); }); + + describe('removeTags', () => { + it('strips a simple tag and trims the remaining text', () => { + expect(idevice.removeTags('

Hello

')).toBe('Hello'); + }); + + it('returns an empty string for nullish input', () => { + expect(idevice.removeTags(null)).toBe(''); + expect(idevice.removeTags(undefined)).toBe(''); + }); + + it('leaves no live ', + '</script>', + 'ipt>alert(1)', + ]; + for (const payload of payloads) { + expect(idevice.removeTags(payload).toLowerCase()).not.toContain(' { + expect(idevice.removeTags(' Plain question text ')).toBe('Plain question text'); + expect(idevice.removeTags('2 plus 3 equals 5')).toBe('2 plus 3 equals 5'); + }); + }); }); diff --git a/public/files/perm/idevices/base/az-quiz-game/edition/az-quiz-game.js b/public/files/perm/idevices/base/az-quiz-game/edition/az-quiz-game.js index e926b0e4ed..be5feeaacf 100644 --- a/public/files/perm/idevices/base/az-quiz-game/edition/az-quiz-game.js +++ b/public/files/perm/idevices/base/az-quiz-game/edition/az-quiz-game.js @@ -1212,11 +1212,16 @@ var $exeDevice = { const words = []; $entries.find('ENTRY').each(function () { - const concept = $(this).find('CONCEPT').text(), - definition = $(this) - .find('DEFINITION') - .text() - .replace(/<[^>]*>/g, ''); + const concept = $(this).find('CONCEPT').text(); + let definition = $(this).find('DEFINITION').text(); + // Strip HTML tags repeatedly: removing one tag can splice + // surrounding characters into a new tag (e.g. "<
script>"), + // so loop until the string stops changing. + let prevDefinition; + do { + prevDefinition = definition; + definition = definition.replace(/<[^>]*>/g, ''); + } while (definition !== prevDefinition); if (concept && definition) { words.push(`${concept}#${definition}`); } diff --git a/public/files/perm/idevices/base/az-quiz-game/edition/az-quiz-game.test.js b/public/files/perm/idevices/base/az-quiz-game/edition/az-quiz-game.test.js index f0294d99ba..f1549c393e 100644 --- a/public/files/perm/idevices/base/az-quiz-game/edition/az-quiz-game.test.js +++ b/public/files/perm/idevices/base/az-quiz-game/edition/az-quiz-game.test.js @@ -32,7 +32,13 @@ function loadIdevice(code) { // Helper to strip HTML tags const stripTags = (html) => { if (!html) return ''; - return String(html).replace(/<[^>]*>/g, ''); + let result = String(html); + let prev; + do { + prev = result; + result = result.replace(/<[^>]*>/g, ''); + } while (result !== prev); + return result; }; // Mock jQuery with chaining support @@ -525,4 +531,57 @@ describe('az-quiz-game iDevice', () => { expect(typeof $exeDevice.validateData).toBe('function'); }); }); + + describe('mock $ text() stripTags helper', () => { + it('strips a simple HTML tag from selector text', () => { + expect(global.$('

Hello

').text()).toBe('Hello'); + }); + + it('strips nested/obfuscated tags, leaving no complete tag behind', () => { + // The fixed-point loop keeps stripping until the string stops + // changing, so a nested/obfuscated payload cannot leave a complete + // `<...>` tag (in particular, no `'; + const result = global.$(payload).text(); + // No surviving complete HTML tag, and no `'); + expect(result).not.toMatch(/<[^>]*>/); + }); + + it('leaves plain text without tags unchanged', () => { + expect(global.$('Plain text').text()).toBe('Plain text'); + }); + }); + + describe('importGlosary definition sanitization', () => { + // importGlosary strips HTML tags from each DEFINITION using a + // fixed-point loop so that removing one tag cannot splice surrounding + // characters into a new tag. This mirrors the source logic and asserts + // the security property on an obfuscated payload while preserving + // legitimate text. + const stripDefinition = (text) => { + let definition = text; + let prevDefinition; + do { + prevDefinition = definition; + definition = definition.replace(/<[^>]*>/g, ''); + } while (definition !== prevDefinition); + return definition; + }; + + it('leaves no complete tag behind for an obfuscated payload', () => { + const payload = 'ipt>The Sun'; + const result = stripDefinition(payload); + expect(result.toLowerCase()).not.toContain(''; + const cleaned = $exeDevice.stripTags(payload); + // The dangerous ") is left behind. + expect(cleaned).not.toMatch(/<[^>]*>/); + }); + + it('reaches a fixed point: re-stripping the output is a no-op', () => { + const payload = '<
script'; + const once = $exeDevice.stripTags(payload); + // A single-pass strip can splice new tags; the loop must converge. + expect(once).not.toMatch(/<[^>]*>/); + expect(once.replace(/<[^>]*>/g, '')).toBe(once); + }); + }); + describe('validTime', () => { it('returns true for valid time format hh:mm:ss', () => { expect($exeDevice.validTime('00:00:00')).toBe(true); diff --git a/public/files/perm/idevices/base/discover/edition/discover.js b/public/files/perm/idevices/base/discover/edition/discover.js index 6b9fc173fa..fc6fcc7caf 100644 --- a/public/files/perm/idevices/base/discover/edition/discover.js +++ b/public/files/perm/idevices/base/discover/edition/discover.js @@ -1723,11 +1723,15 @@ var $exeDevice = { const cardsJson = $entries .find('ENTRY') .map((_, entry) => { - const concept = $(entry).find('CONCEPT').text(), - definition = $(entry) - .find('DEFINITION') - .text() - .replace(/<[^>]*>/g, ''); + const concept = $(entry).find('CONCEPT').text(); + let definition = $(entry).find('DEFINITION').text(), + prevDefinition; + // Re-apply the tag strip until the result stabilises so that + // overlapping/nested markup cannot splice into a new tag. + do { + prevDefinition = definition; + definition = definition.replace(/<[^>]*>/g, ''); + } while (definition !== prevDefinition); return concept && definition ? { eText: concept, eTextBk: definition } : null; diff --git a/public/files/perm/idevices/base/discover/edition/discover.test.js b/public/files/perm/idevices/base/discover/edition/discover.test.js index f790642bf6..28d5de4b35 100644 --- a/public/files/perm/idevices/base/discover/edition/discover.test.js +++ b/public/files/perm/idevices/base/discover/edition/discover.test.js @@ -82,9 +82,15 @@ const $exeDevice = { removeTags: function (str) { const wrapper = { html: function(content) { this.content = content; }, - text: function() { + text: function() { if (!this.content) return ''; - return this.content.replace(/<[^>]*>/g, ''); + let result = this.content; + let prev; + do { + prev = result; + result = result.replace(/<[^>]*>/g, ''); + } while (result !== prev); + return result; } }; wrapper.html(str); @@ -294,6 +300,21 @@ describe('Discover Edition Functions', () => { expect($exeDevice.removeTags('
Content
')).toBe('Content'); expect($exeDevice.removeTags('
Link')).toBe('Link'); }); + + it('should leave no '); + expect(result.toLowerCase()).not.toContain(' { + // Re-sanitizing the output must not change it further, and no + // residual tag-opening "<" may survive a spliced payload. + const once = $exeDevice.removeTags('>ce'); + const twice = $exeDevice.removeTags(once); + expect(twice).toBe(once); + expect(once).not.toContain('<'); + }); }); describe('escapeQuotes', () => { @@ -494,6 +515,67 @@ describe('Discover Edition Functions', () => { }); }); + describe('importGlosary sanitization', () => { + // importGlosary parses real XML and uses jQuery DOM traversal, so it + // needs the genuine jQuery/DOMParser. The file-level mock above replaces + // global.$ (and, since window === globalThis here, window.$) with a + // vi.fn stub, but the real jQuery is still reachable via global.jQuery + // and the real DOMParser via global.DOMParser (both set by the setup). + const xmlEscape = (s) => + s + .replace(/&/g, '&') + .replace(//g, '>'); + // The payload is XML-escaped so that .text() yields it back as literal + // characters that reach the regex strip (otherwise the XML parser would + // treat it as markup and discard the tags itself). + const buildGlosaryXml = (definitionInner) => + `Term` + + `${xmlEscape(definitionInner)}`; + + const importWithRealJQuery = (xml) => { + const savedDollar = global.$; + global.$ = global.jQuery; + try { + const realExeDevice = global.loadIdevice( + join(__dirname, 'discover.js') + ); + let captured = null; + realExeDevice.insertCards = (cards) => { + captured = cards; + }; + realExeDevice.importGlosary(xml); + return captured; + } finally { + global.$ = savedDollar; + } + }; + + it('strips legitimate inline markup from definitions', () => { + const cards = importWithRealJQuery( + buildGlosaryXml('A bold definition') + ); + expect(cards).toEqual([ + { eText: 'Term', eTextBk: 'A bold definition' }, + ]); + }); + + it('fully removes nested/obfuscated tag payloads (no safe') + ); + expect(cards).toHaveLength(1); + const definition = cards[0].eTextBk; + expect(definition.toLowerCase()).not.toContain(' { it('downloads question text with the discover filename and container', () => { const realExeDevice = global.loadIdevice(join(__dirname, 'discover.js')); diff --git a/public/files/perm/idevices/base/dragdrop/edition/dragdrop.js b/public/files/perm/idevices/base/dragdrop/edition/dragdrop.js index 623867b3df..5b187f16d6 100644 --- a/public/files/perm/idevices/base/dragdrop/edition/dragdrop.js +++ b/public/files/perm/idevices/base/dragdrop/edition/dragdrop.js @@ -460,11 +460,11 @@ var $exeDevice = { }, decodeURIComponentSafe: function (s) { - return s ? decodeURIComponent(s).replace('%', '%') : s; + return s ? decodeURIComponent(s).replace(/%/g, '%') : s; }, encodeURIComponentSafe: function (s) { - return s ? encodeURIComponent(s.replace('%', '%')) : s; + return s ? encodeURIComponent(s.replace(/%/g, '%')) : s; }, validateCard: function () { diff --git a/public/files/perm/idevices/base/dragdrop/edition/dragdrop.test.js b/public/files/perm/idevices/base/dragdrop/edition/dragdrop.test.js index e44adfd5d4..c97bdd156b 100644 --- a/public/files/perm/idevices/base/dragdrop/edition/dragdrop.test.js +++ b/public/files/perm/idevices/base/dragdrop/edition/dragdrop.test.js @@ -41,4 +41,33 @@ describe('dragdrop iDevice export helpers', () => { expect(downloadBlob.mock.calls[0][1]).toBe('Activity-DragDrop.json'); expect(downloadBlob.mock.calls[0][2]).toBe('dragdropQIdeviceForm'); }); + + it('escapes every percent sign when encoding, not just the first', () => { + // Multiple '%' must all be escaped (global flag), otherwise the + // unescaped ones become decoder control characters. + const encoded = $exeDevice.encodeURIComponentSafe('100% off 50% sale'); + expect(encoded).not.toContain('%25'); + expect(encoded).toBe(encodeURIComponent('100% off 50% sale')); + }); + + it('round-trips strings containing multiple percent signs', () => { + const original = 'a%b%c%d'; + const restored = $exeDevice.decodeURIComponentSafe( + $exeDevice.encodeURIComponentSafe(original) + ); + expect(restored).toBe(original); + }); + + it('preserves legitimate input without percent signs', () => { + const original = 'https://example.com/image (1).png'; + const restored = $exeDevice.decodeURIComponentSafe( + $exeDevice.encodeURIComponentSafe(original) + ); + expect(restored).toBe(original); + }); + + it('returns falsy input unchanged for both safe helpers', () => { + expect($exeDevice.encodeURIComponentSafe('')).toBe(''); + expect($exeDevice.decodeURIComponentSafe('')).toBe(''); + }); }); diff --git a/public/files/perm/idevices/base/electrical-circuits/edition/electrical-circuits.js b/public/files/perm/idevices/base/electrical-circuits/edition/electrical-circuits.js index da405c0680..ae9fabc0fc 100644 --- a/public/files/perm/idevices/base/electrical-circuits/edition/electrical-circuits.js +++ b/public/files/perm/idevices/base/electrical-circuits/edition/electrical-circuits.js @@ -1783,11 +1783,17 @@ var $exeDevice = { const questionsJson = []; $entries.find('ENTRY').each(function () { const $this = $(this), - concept = $this.find('CONCEPT').text(), - definition = $this - .find('DEFINITION') - .text() - .replace(/<[^>]*>/g, ''); + concept = $this.find('CONCEPT').text(); + // Strip HTML tags. Apply the replacement repeatedly until the + // string stops changing, because removing one match can splice the + // remaining characters into a new tag (e.g. "<script>" -> + // "safe". The strip + // (looped to a fixed point) must remove every "<...>" tag so no + // " { + const xml = buildGlosaryXml('Term', 'A plain definition'); + $exeDevice.importGlosary(xml); + expect(captured.length).toBe(1); + expect(captured[0].eText).toBe('Term'); + expect(captured[0].eTextBk).toBe('A plain definition'); + }); + + it('returns false for malformed XML', () => { + expect($exeDevice.importGlosary('not xml at all <<')).toBe(false); + }); + }); + describe('addEvents', () => { let originalExeDevicesEdition; diff --git a/public/files/perm/idevices/base/guess/edition/guess.js b/public/files/perm/idevices/base/guess/edition/guess.js index bbefafc1f8..8ca4b9aba3 100644 --- a/public/files/perm/idevices/base/guess/edition/guess.js +++ b/public/files/perm/idevices/base/guess/edition/guess.js @@ -2547,11 +2547,15 @@ var $exeDevice = { const words = []; $entries.find('ENTRY').each(function () { - const concept = $(this).find('CONCEPT').text(), - definition = $(this) - .find('DEFINITION') - .text() - .replace(/<[^>]*>/g, ''); + const concept = $(this).find('CONCEPT').text(); + // Strip HTML tags repeatedly until stable: a single pass can splice + // two remaining halves into a new tag (e.g. <script> -> safe + const xml = buildGlosaryXml([ + { + concept: 'attack', + definition: + '<scr<script>ipt>alert(1)</script>safe', + }, + ]); + $exeDevice.importGlosary(xml); + expect(captured).toHaveLength(1); + // No residual opening-tag markup must remain after the fixed-point loop. + expect(captured[0].definition.indexOf('<')).toBe(-1); + expect(/`) + // and is case-insensitive. + var prev; + var out = svg; + do { + prev = out; + out = out + .replace(//gi, '') + .replace(/<\/?\s*foreignObject[^>]*>/gi, '') + .replace(/\son[a-z]+\s*=\s*"[^"]*"/gi, '') + .replace(/\son[a-z]+\s*=\s*'[^']*'/gi, '') + // Unquoted handler values: stop at whitespace or '>' (also '/>'). + .replace(/\son[a-z]+\s*=\s*[^\s>]+/gi, '') + // Dangerous URL schemes. `javascript:` and `vbscript:` are + // always unsafe and are removed outright. For `data:` we only + // strip the script-executing `data:text/html` form so + // legitimate embedded images (`data:image/...`) keep working. + .replace(/javascript:/gi, '') + .replace(/vbscript:/gi, '') + .replace(/data:\s*text\/html/gi, ''); + } while (out !== prev); + return out; } /** diff --git a/public/files/perm/idevices/base/slide/export/slide.test.js b/public/files/perm/idevices/base/slide/export/slide.test.js index db096a03ba..d7f0ead652 100644 --- a/public/files/perm/idevices/base/slide/export/slide.test.js +++ b/public/files/perm/idevices/base/slide/export/slide.test.js @@ -120,6 +120,44 @@ describe('$slide._scrubSvg', () => { expect(_scrubSvg(42)).toBe(''); expect(_scrubSvg('')).toBe(''); }); + + it('defeats nested/spliced ipt>xyipt>'); + expect(out.toLowerCase()).not.toContain(' end tags with whitespace before the angle bracket', () => { + const out = _scrubSvg('\n'); + expect(out.toLowerCase()).not.toContain(' { + expect(_scrubSvg('')).not.toContain('vbscript:'); + expect(_scrubSvg('').toLowerCase()).not.toContain('vbscript:'); + }); + + it('defeats spliced javascript: schemes (loops to a fixed point)', () => { + const out = _scrubSvg(''); + expect(out.toLowerCase()).not.toContain('javascript:'); + }); + + it('strips script-executing data:text/html URLs', () => { + const out = _scrubSvg(''); + expect(out.toLowerCase()).not.toContain('data:text/html'); + }); + + it('keeps legitimate embedded data:image URLs intact', () => { + const svg = ''; + expect(_scrubSvg(svg)).toContain('data:image/png;base64,AAAA'); + }); + + it('strips (which can embed arbitrary HTML), matching the src twin', () => { + const out = _scrubSvg('
hi
'); + expect(out.toLowerCase()).not.toContain('foreignobject'); + }); }); // ───────────────────────────────────────────────────────────────────────────── diff --git a/public/files/perm/idevices/base/slide/src/sanitizer.test.js b/public/files/perm/idevices/base/slide/src/sanitizer.test.js index f83db5d8d7..b80ae6b654 100644 --- a/public/files/perm/idevices/base/slide/src/sanitizer.test.js +++ b/public/files/perm/idevices/base/slide/src/sanitizer.test.js @@ -40,6 +40,42 @@ describe('scrubSvg', () => { const out = scrubSvg('
'); expect(out).not.toContain('foreignObject'); }); + + it('defeats nested/spliced would splice the outer halves + // into a fresh ipt>xyipt>'); + expect(out.toLowerCase()).not.toContain(' end tags with whitespace before the angle bracket', () => { + const out = scrubSvg('\n'); + expect(out.toLowerCase()).not.toContain(' { + expect(scrubSvg('')).not.toContain('vbscript:'); + expect(scrubSvg('').toLowerCase()).not.toContain('vbscript:'); + }); + + it('defeats spliced javascript: schemes (loops to a fixed point)', () => { + // `javasjavascript:cript:` collapses to `javascript:` after one removal. + const out = scrubSvg(''); + expect(out.toLowerCase()).not.toContain('javascript:'); + }); + + it('strips script-executing data:text/html URLs', () => { + const out = scrubSvg(''); + expect(out.toLowerCase()).not.toContain('data:text/html'); + }); + + it('keeps legitimate embedded data:image URLs intact', () => { + const svg = ''; + expect(scrubSvg(svg)).toContain('data:image/png;base64,AAAA'); + }); }); describe('sanitizeSvg', () => { diff --git a/public/files/perm/idevices/base/slide/src/sanitizer.ts b/public/files/perm/idevices/base/slide/src/sanitizer.ts index 6079102c37..87edcdae9d 100644 --- a/public/files/perm/idevices/base/slide/src/sanitizer.ts +++ b/public/files/perm/idevices/base/slide/src/sanitizer.ts @@ -25,13 +25,31 @@ type DomPurifyLike = { */ export function scrubSvg(svg: unknown): string { if (!svg || typeof svg !== 'string') return ''; - return svg - .replace(//gi, '') - .replace(/<\/?\s*foreignObject[^>]*>/gi, '') - .replace(/\son[a-z]+\s*=\s*"[^"]*"/gi, '') - .replace(/\son[a-z]+\s*=\s*'[^']*'/gi, '') - .replace(/\son[a-z]+\s*=\s*[^\s>]+/gi, '') - .replace(/javascript:/gi, ''); + // Apply every removal repeatedly until the string stops changing. + // A single pass can be defeated because deleting one match can splice + // two halves into a brand-new match (e.g. `ipt>` -> ``) and is case-insensitive. + let prev: string; + let out = svg; + do { + prev = out; + out = out + .replace(//gi, '') + .replace(/<\/?\s*foreignObject[^>]*>/gi, '') + .replace(/\son[a-z]+\s*=\s*"[^"]*"/gi, '') + .replace(/\son[a-z]+\s*=\s*'[^']*'/gi, '') + .replace(/\son[a-z]+\s*=\s*[^\s>]+/gi, '') + // Dangerous URL schemes. `javascript:` and `vbscript:` are always + // unsafe and are removed outright. For `data:` we only strip the + // script-executing `data:text/html` form so legitimate embedded + // images (`data:image/...`) keep working. + .replace(/javascript:/gi, '') + .replace(/vbscript:/gi, '') + .replace(/data:\s*text\/html/gi, ''); + } while (out !== prev); + return out; } /** diff --git a/public/files/perm/idevices/base/three-sixty-viewer/edition/three-sixty-viewer.js b/public/files/perm/idevices/base/three-sixty-viewer/edition/three-sixty-viewer.js index b3d49a6d6b..5c899cfe0f 100644 --- a/public/files/perm/idevices/base/three-sixty-viewer/edition/three-sixty-viewer.js +++ b/public/files/perm/idevices/base/three-sixty-viewer/edition/three-sixty-viewer.js @@ -1042,9 +1042,13 @@ var $exeDevice = { var parts = path.split('.'); var t = target; for (var i = 0; i < parts.length - 1; i++) { - t = t[parts[i]]; + var key = parts[i]; + if (key === '__proto__' || key === 'prototype' || key === 'constructor') return; + t = t[key]; } - t[parts[parts.length - 1]] = value; + var lastKey = parts[parts.length - 1]; + if (lastKey === '__proto__' || lastKey === 'prototype' || lastKey === 'constructor') return; + t[lastKey] = value; }, escapeHtml: str => diff --git a/public/files/perm/idevices/base/three-sixty-viewer/edition/three-sixty-viewer.test.js b/public/files/perm/idevices/base/three-sixty-viewer/edition/three-sixty-viewer.test.js index 7761148599..981f9add96 100644 --- a/public/files/perm/idevices/base/three-sixty-viewer/edition/three-sixty-viewer.test.js +++ b/public/files/perm/idevices/base/three-sixty-viewer/edition/three-sixty-viewer.test.js @@ -1063,4 +1063,32 @@ describe('three-sixty-viewer iDevice (edition)', () => { }); }); }); + + describe('setNestedPath (prototype pollution safety)', () => { + it('assigns a nested value for a legitimate dotted path', () => { + const target = { initialView: { yaw: 0, pitch: 0, fov: 75 } }; + $exeDevice.setNestedPath(target, 'initialView.yaw', 90); + expect(target.initialView.yaw).toBe(90); + }); + + it('assigns a top-level key for a single-segment path', () => { + const target = { src: '' }; + $exeDevice.setNestedPath(target, 'src', 'asset://pano.jpg'); + expect(target.src).toBe('asset://pano.jpg'); + }); + + it('does not pollute Object.prototype via a __proto__ payload', () => { + const payload = JSON.parse('{"__proto__":{"polluted":1}}'); + $exeDevice.setNestedPath({}, '__proto__.polluted', payload.__proto__.polluted); + expect({}.polluted).toBeUndefined(); + expect(Object.prototype.polluted).toBeUndefined(); + }); + + it('ignores assignments through constructor / prototype keys', () => { + $exeDevice.setNestedPath({}, 'constructor.prototype.polluted', 1); + $exeDevice.setNestedPath({}, 'prototype.polluted', 1); + expect({}.polluted).toBeUndefined(); + expect(Object.prototype.polluted).toBeUndefined(); + }); + }); }); diff --git a/public/files/perm/idevices/base/trivial/edition/trivial.js b/public/files/perm/idevices/base/trivial/edition/trivial.js index 8b74314a5e..6640da5b42 100644 --- a/public/files/perm/idevices/base/trivial/edition/trivial.js +++ b/public/files/perm/idevices/base/trivial/edition/trivial.js @@ -3113,6 +3113,24 @@ var $exeDevice = { return valids > 0 ? $exeDevice.selectsGame : false; }, + /** + * Strip HTML tags from a string. + * Applies the tag-removal regex repeatedly until the string stops + * changing, so that obfuscated/nested payloads (e.g. "ipt>") + * cannot reconstitute a tag after a single pass. + * @param {string} text + * @returns {string} + */ + stripHtmlTags: function (text) { + let result = String(text == null ? '' : text); + let previous; + do { + previous = result; + result = result.replace(/<[^>]*>/g, ''); + } while (result !== previous); + return result; + }, + importGlosary: function (xmlText) { const parser = new DOMParser(), xmlDoc = parser.parseFromString(xmlText, 'text/xml'), @@ -3128,10 +3146,9 @@ var $exeDevice = { $entries.find('ENTRY').each(function () { var $this = $(this), concept = $this.find('CONCEPT').text(), - definition = $this - .find('DEFINITION') - .text() - .replace(/<[^>]*>/g, ''); // Elimina HTML + definition = $exeDevice.stripHtmlTags( + $this.find('DEFINITION').text() + ); // Elimina HTML if (concept && definition) { questionsJson.push({ solution: concept, diff --git a/public/files/perm/idevices/base/trivial/edition/trivial.test.js b/public/files/perm/idevices/base/trivial/edition/trivial.test.js new file mode 100644 index 0000000000..f0e6d95683 --- /dev/null +++ b/public/files/perm/idevices/base/trivial/edition/trivial.test.js @@ -0,0 +1,74 @@ +/** + * Unit tests for trivial iDevice (edition code) + * + * Focused on stripHtmlTags, the HTML-tag sanitizer used when importing + * glossary entries. It must remove tags even from obfuscated/nested + * payloads that would survive a single-pass strip. + */ + +/* eslint-disable no-undef */ +import { readFileSync } from 'fs'; +import { fileURLToPath } from 'url'; +import { dirname, join } from 'path'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); + +/** + * Helper to load iDevice file and expose $exeDevice globally. + * Replaces 'var $exeDevice' with 'global.$exeDevice' to make it accessible. + */ +function loadIdevice(code) { + const modifiedCode = code.replace(/var\s+\$exeDevice\s*=/, 'global.$exeDevice ='); + // eslint-disable-next-line no-eval + (0, eval)(modifiedCode); + return global.$exeDevice; +} + +describe('trivial iDevice (edition)', () => { + let $exeDevice; + + beforeEach(() => { + global.$exeDevice = undefined; + const filePath = join(__dirname, 'trivial.js'); + const code = readFileSync(filePath, 'utf-8'); + $exeDevice = loadIdevice(code); + }); + + describe('stripHtmlTags', () => { + it('exists as a function', () => { + expect(typeof $exeDevice.stripHtmlTags).toBe('function'); + }); + + it('removes simple HTML tags but keeps text content', () => { + expect($exeDevice.stripHtmlTags('hello')).toBe('hello'); + expect($exeDevice.stripHtmlTags('a b c')).toBe('a b c'); + }); + + it('preserves plain text without tags unchanged', () => { + expect($exeDevice.stripHtmlTags('plain definition')).toBe('plain definition'); + }); + + it('strips a nested/obfuscated script payload leaving no script tag', () => { + const payload = 'ipt>alert(1)'; + const out = $exeDevice.stripHtmlTags(payload); + // No reconstituted ". + // The fixed-point loop must keep stripping until nothing remains. + const payload = 'ipt>alert(1)safe'; + $exeDevice.importGlosary(glossaryXml('term', payload)); + + expect(captured).toHaveLength(1); + const definition = captured[0].definition; + // The security property: no opening tag delimiter survives, so the + // reassembled "" left as text + // content is inert and not a tag.) + expect(definition.toLowerCase()).not.toContain(' { + let captured = null; + $exeDevice.addWords = (words) => { + captured = words; + }; + + // A naive single pass on "< Inc'; + const result = extractCopyrightFromLicense(content); + expect(result).not.toContain('<'); + expect(result?.toLowerCase()).not.toContain(' { + const content = 'Copyright (c) 2023 Acme Inc'; + expect(extractCopyrightFromLicense(content)).toBe('Acme Inc'); + const parens = 'Copyright (c) 2023 Acme (legacy) (note) Inc'; + expect(extractCopyrightFromLicense(parens)).toBe('Acme Inc'); + }); }); describe('getPackageInfo', () => { diff --git a/src/cli/commands/update-licenses.ts b/src/cli/commands/update-licenses.ts index 59e2c57169..433ba221b4 100644 --- a/src/cli/commands/update-licenses.ts +++ b/src/cli/commands/update-licenses.ts @@ -121,10 +121,18 @@ export function extractCopyrightFromLicense(content: string): string | null { // Get first line only let author = match[1].split('\n')[0]; // Clean up the result - remove "All rights reserved", email, etc. + // Apply the strip passes repeatedly to a fixed point: removing one + // <...> or (...) match can splice two halves into a brand-new match + // (e.g. "c>"), so a single pass is insufficient. + let prev: string; + do { + prev = author; + author = author + .replace(/all rights reserved\.?/gi, '') + .replace(/<[^>]+>/g, '') // Remove emails in + .replace(/\s*\([^)]*\)/g, ''); // Remove parenthetical notes + } while (author !== prev); author = author - .replace(/all rights reserved\.?/gi, '') - .replace(/<[^>]+>/g, '') // Remove emails in - .replace(/\s*\([^)]*\)/g, '') // Remove parenthetical notes .replace(/\s+/g, ' ') // Normalize whitespace .trim(); // Remove trailing punctuation diff --git a/src/routes/project.spec.ts b/src/routes/project.spec.ts index 7e313363d9..64d1bdd3a4 100644 --- a/src/routes/project.spec.ts +++ b/src/routes/project.spec.ts @@ -11,6 +11,7 @@ import { createProjectRoutes, createSymfonyCompatProjectRoutes, buildDefaultThemePayload, + isUnsafeLinkScheme, type ProjectDependencies, type SessionManagerDeps, type FileHelperDeps, @@ -1856,7 +1857,7 @@ describe('Project Routes', () => { expect(body.brokenLinks[0].brokenLinks).toBe('No broken links found'); }); - it('should skip javascript and data URLs', async () => { + it('should skip javascript, vbscript and data URLs (case-insensitive, padded)', async () => { const res = await app.handle( new Request('http://localhost/api/ode-management/odes/session/brokenlinks', { method: 'POST', @@ -1864,7 +1865,14 @@ describe('Project Routes', () => { body: JSON.stringify({ idevices: [ { - html: 'JS Link', + html: + 'JS Link' + + 'Mixed JS' + + 'Padded JS' + + 'VBScript' + + 'Mixed VBScript' + + '' + + '', }, ], }), @@ -1873,10 +1881,54 @@ describe('Project Routes', () => { expect(res.status).toBe(200); const body = await res.json(); + // Every dangerous-scheme link is filtered out, so nothing is reported. expect(body.brokenLinks[0].brokenLinks).toBe('No broken links found'); }); }); + describe('isUnsafeLinkScheme', () => { + it('treats script-capable schemes as unsafe', () => { + expect(isUnsafeLinkScheme('javascript:void(0)')).toBe(true); + expect(isUnsafeLinkScheme('vbscript:msgbox(1)')).toBe(true); + expect(isUnsafeLinkScheme('data:text/html,')).toBe(true); + }); + + it('is case-insensitive', () => { + expect(isUnsafeLinkScheme('JavaScript:void(0)')).toBe(true); + expect(isUnsafeLinkScheme('VBScript:msgbox(1)')).toBe(true); + expect(isUnsafeLinkScheme('DATA:text/html,x')).toBe(true); + }); + + it('tolerates leading whitespace and control characters', () => { + expect(isUnsafeLinkScheme(' javascript:alert(1)')).toBe(true); + expect(isUnsafeLinkScheme('\t\njavascript:alert(1)')).toBe(true); + expect(isUnsafeLinkScheme(' vbscript:msgbox(1)')).toBe(true); + expect(isUnsafeLinkScheme(' \tdata:text/html,x')).toBe(true); + // Leading NUL (U+0000) and US (U+001F) control-char padding that + // URL parsers strip before resolving the scheme. + const nul = String.fromCharCode(0); + const us = String.fromCharCode(0x1f); + expect(isUnsafeLinkScheme(`${nul}javascript:alert(1)`)).toBe(true); + expect(isUnsafeLinkScheme(`${us}vbscript:msgbox(1)`)).toBe(true); + expect(isUnsafeLinkScheme(`${nul}${us} data:text/html,x`)).toBe(true); + }); + + it('keeps legitimate links safe', () => { + expect(isUnsafeLinkScheme('http://example.com')).toBe(false); + expect(isUnsafeLinkScheme('https://example.com/page')).toBe(false); + expect(isUnsafeLinkScheme('//cdn.example.com/a.js')).toBe(false); + expect(isUnsafeLinkScheme('mailto:user@example.com')).toBe(false); + expect(isUnsafeLinkScheme('tel:+34123456789')).toBe(false); + expect(isUnsafeLinkScheme('files/img/photo.png')).toBe(false); + expect(isUnsafeLinkScheme('exe-node:abc')).toBe(false); + expect(isUnsafeLinkScheme('#anchor')).toBe(false); + expect(isUnsafeLinkScheme('relative/path.html')).toBe(false); + expect(isUnsafeLinkScheme('')).toBe(false); + // Substrings of dangerous schemes must not trip the prefix check. + expect(isUnsafeLinkScheme('https://x/javascript:notscheme')).toBe(false); + }); + }); + describe('Link Extraction (brokenlinks/extract)', () => { it('should extract links without validating', async () => { const res = await app.handle( diff --git a/src/routes/project.ts b/src/routes/project.ts index 9de7825253..a3b0ec8164 100644 --- a/src/routes/project.ts +++ b/src/routes/project.ts @@ -92,6 +92,29 @@ export async function resolveDefaultThemeForNewProject( } return themeDirName; } + +/** + * Returns true when a link URL uses a scheme that can execute script when + * navigated/rendered (e.g. `javascript:`, `vbscript:`, `data:`) and must + * therefore be discarded by the broken-link checker. + * + * The check is deliberately defensive: schemes are matched case-insensitively + * and after stripping leading whitespace and C0 control characters, which + * browsers ignore when resolving a URL. This prevents bypasses such as + * `VBScript:...`, a leading-space `" javascript:..."`, or control-character + * padding from slipping a dangerous scheme past the filter. + * + * Exported so the security property can be unit-tested directly. + */ +export function isUnsafeLinkScheme(url: string): boolean { + if (!url) return false; + // Strip leading whitespace and C0 control chars that URL parsers ignore, + // so padded schemes (e.g. a leading-space javascript:) cannot bypass the check. + // biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally matching the C0 control chars browsers ignore in URLs + const normalized = url.replace(/^[\s\u0000-\u001f]+/, '').toLowerCase(); + return normalized.startsWith('javascript:') || normalized.startsWith('vbscript:') || normalized.startsWith('data:'); +} + import { getAppVersion } from '../utils/version'; import { buildSiteThemeUrl } from '../utils/site-theme-url'; import { @@ -1833,8 +1856,9 @@ export function createSymfonyCompatProjectRoutes(deps: ProjectDependencies = def return links.filter(link => { if (!link.url || link.url.trim() === '') return false; if (link.url.startsWith('#')) return false; - if (link.url.startsWith('javascript:')) return false; - if (link.url.startsWith('data:')) return false; + // Drop URLs whose scheme can execute script (javascript:, + // vbscript:, data:), tolerating case and leading whitespace/controls. + if (isUnsafeLinkScheme(link.url)) return false; return true; }); }; diff --git a/src/services/folder-manager.spec.ts b/src/services/folder-manager.spec.ts index 175107ae67..c840d614a2 100644 --- a/src/services/folder-manager.spec.ts +++ b/src/services/folder-manager.spec.ts @@ -141,6 +141,19 @@ describe('Folder Manager Service', () => { expect(service.sanitizeFolderName('folder*name')).toBe('folder_name'); }); + it('should replace ALL invalid characters, not just the first', () => { + // Incomplete-sanitization regression guard: without the global flag + // only the first invalid character would be replaced, leaving the + // rest (e.g. directory separators) intact. + expect(service.sanitizeFolderName('ac')).toBe('a_b_c'); + expect(service.sanitizeFolderName('a/b/c')).toBe('a_b_c'); + expect(service.sanitizeFolderName('a\\b\\c')).toBe('a_b_c'); + expect(service.sanitizeFolderName('<>:"|?*')).toBe('_______'); + // No invalid character should survive sanitization. + expect(service.sanitizeFolderName('x:y/z')).not.toContain('/'); + expect(service.sanitizeFolderName('x:y/z')).not.toContain(':'); + }); + it('should remove leading dots and spaces', () => { expect(service.sanitizeFolderName('.hidden')).toBe('hidden'); expect(service.sanitizeFolderName(' folder')).toBe('folder'); diff --git a/src/services/folder-manager.ts b/src/services/folder-manager.ts index d7bb7c6b1c..3ea7c49247 100644 --- a/src/services/folder-manager.ts +++ b/src/services/folder-manager.ts @@ -113,6 +113,9 @@ export interface FolderManagerService { // Invalid characters for folder names (cross-platform safe) // Note: we check for control chars (0-31) separately to avoid regex escape issues const INVALID_CHARS = /[<>:"|?*\\/]/; +// Global variant used for sanitization so that ALL invalid characters are +// replaced, not just the first occurrence (incomplete sanitization fix). +const INVALID_CHARS_GLOBAL = /[<>:"|?*\\/]/g; const RESERVED_NAMES = ['CON', 'PRN', 'AUX', 'NUL', 'COM1', 'COM2', 'COM3', 'COM4', 'LPT1', 'LPT2', 'LPT3', 'LPT4']; /** @@ -182,8 +185,8 @@ export function createFolderManagerService(deps: FolderManagerDeps = {}): Folder const sanitizeFolderName = (name: string): string => { // Remove control characters first let sanitized = removeControlChars(name); - // Remove invalid characters - sanitized = sanitized.replace(INVALID_CHARS, '_'); + // Remove invalid characters (global: replace every occurrence) + sanitized = sanitized.replace(INVALID_CHARS_GLOBAL, '_'); // Remove leading/trailing dots and spaces sanitized = sanitized.replace(/^[.\s]+|[.\s]+$/g, ''); // Truncate to max length diff --git a/src/services/link-validator.spec.ts b/src/services/link-validator.spec.ts index 6322b83140..435e1bfaa4 100644 --- a/src/services/link-validator.spec.ts +++ b/src/services/link-validator.spec.ts @@ -115,6 +115,74 @@ describe('Link Validator Service', () => { const result = removeInvalidLinks(links); expect(result).toHaveLength(0); }); + + it('should remove vbscript: links', () => { + const links: RawExtractedLink[] = [ + { url: 'vbscript:msgbox("hi")', count: 1 }, + { url: 'VBScript:Execute("x")', count: 1 }, + ]; + const result = removeInvalidLinks(links); + expect(result).toHaveLength(0); + }); + + it('should remove dangerous schemes regardless of case', () => { + const links: RawExtractedLink[] = [ + { url: 'JaVaScRiPt:alert(1)', count: 1 }, + { url: 'JAVASCRIPT:void(0)', count: 1 }, + { url: 'DATA:text/html,

Hi

', count: 1 }, + ]; + const result = removeInvalidLinks(links); + expect(result).toHaveLength(0); + }); + + it('should remove dangerous schemes with leading whitespace/control chars', () => { + const links: RawExtractedLink[] = [ + { url: ' javascript:alert(1)', count: 1 }, + { url: '\tjavascript:alert(2)', count: 1 }, + { url: '\n\rvbscript:msgbox(1)', count: 1 }, + { url: ' data:text/html,

Hi

', count: 1 }, + ]; + const result = removeInvalidLinks(links); + expect(result).toHaveLength(0); + }); + + it('should remove dangerous schemes hidden by embedded control chars', () => { + const links: RawExtractedLink[] = [ + { url: 'java\tscript:alert(1)', count: 1 }, + { url: 'java\nscript:alert(2)', count: 1 }, + ]; + const result = removeInvalidLinks(links); + expect(result).toHaveLength(0); + }); + + it('should remove dangerous schemes obfuscated with HTML entities', () => { + const links: RawExtractedLink[] = [ + { url: 'java script:alert(1)', count: 1 }, + { url: 'java script:alert(2)', count: 1 }, + { url: 'java script:alert(3)', count: 1 }, + ]; + const result = removeInvalidLinks(links); + expect(result).toHaveLength(0); + }); + + it('should keep legitimate http/https/relative links', () => { + const links: RawExtractedLink[] = [ + { url: 'http://example.com', count: 1 }, + { url: 'https://example.com/page', count: 1 }, + { url: 'files/image.png', count: 1 }, + { url: 'mailto:user@example.com', count: 1 }, + { url: '//cdn.example.com/lib.js', count: 1 }, + ]; + const result = removeInvalidLinks(links); + expect(result).toHaveLength(5); + expect(result.map(l => l.url)).toEqual([ + 'http://example.com', + 'https://example.com/page', + 'files/image.png', + 'mailto:user@example.com', + '//cdn.example.com/lib.js', + ]); + }); }); describe('deduplicateLinks', () => { diff --git a/src/services/link-validator.ts b/src/services/link-validator.ts index 414bb94ab9..ab798bd1c4 100644 --- a/src/services/link-validator.ts +++ b/src/services/link-validator.ts @@ -83,16 +83,41 @@ export function cleanAndCountLinks(links: RawExtractedLink[]): RawExtractedLink[ return Array.from(urlCounts.entries()).map(([url, count]) => ({ url, count })); } +/** + * Schemes that can execute script or smuggle markup. These must never be + * treated as validatable links, regardless of casing, leading whitespace, + * control characters, or HTML-entity obfuscation. + */ +const DANGEROUS_URL_SCHEMES = ['javascript:', 'vbscript:', 'data:']; + +/** + * Normalize a URL for scheme inspection: decode the HTML entities and strip + * the leading whitespace / control characters that browsers ignore when + * resolving a URL scheme, then lowercase the result. This prevents bypasses + * such as " javascript:", "JaVaScRiPt:", or "java script:". + */ +function normalizeUrlForSchemeCheck(url: string): string { + return ( + url + // Decode HTML entities for whitespace/control chars (tab, LF, CR). + .replace(/&(?:#x0*9|#0*9|#x0*a|#0*10|#x0*d|#0*13|tab|newline);/gi, '') + // Strip leading/embedded whitespace and C0 control characters (U+0000-U+0020). + // biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally matching the C0 control chars browsers ignore when resolving a URL scheme, to block obfuscated-scheme bypasses. + .replace(/[\u0000-\u0020]+/g, '') + .toLowerCase() + ); +} + /** * Remove invalid/non-validatable links - * Filters out: empty, anchors (#), javascript:, data: URLs + * Filters out: empty, anchors (#), and dangerous schemes (javascript:, vbscript:, data:) */ export function removeInvalidLinks(links: RawExtractedLink[]): RawExtractedLink[] { return links.filter(link => { if (!link.url || link.url.trim() === '') return false; if (link.url.startsWith('#')) return false; - if (link.url.startsWith('javascript:')) return false; - if (link.url.startsWith('data:')) return false; + const normalized = normalizeUrlForSchemeCheck(link.url); + if (DANGEROUS_URL_SCHEMES.some(scheme => normalized.startsWith(scheme))) return false; return true; }); } diff --git a/src/shared/export/exporters/PrintPreviewExporter.spec.ts b/src/shared/export/exporters/PrintPreviewExporter.spec.ts index d19c890596..c85c7811e6 100644 --- a/src/shared/export/exporters/PrintPreviewExporter.spec.ts +++ b/src/shared/export/exporters/PrintPreviewExporter.spec.ts @@ -704,33 +704,7 @@ describe('PrintPreviewExporter', () => { const exp = new PrintPreviewExporter(doc, mockResourceProvider); const result = await exp.generatePreview(); - // Helper to check for display: none !important - const checkHidden = (snippet: string) => { - // Determine the tag type to construct the expectation - const isImg = snippet.startsWith('', '.*style=.*display:\\s*none.*!important.*>')), - ); - }; - - // Can't easily use regex match on exact input string because attributes might move if we parsed them? - // If we use string replace, attributes stay mostly put. - // Let's check that the specific class combinations now have style="display: none !important" attached. + // Check that the specific class combinations now have style="display: none !important" attached. // 1. Version divs expect(result.html).toContain('class="sopa-version js-hidden" style="display: none !important"'); diff --git a/src/shared/export/generators/I18nGenerator.spec.ts b/src/shared/export/generators/I18nGenerator.spec.ts index 00b2d2ad27..32d2ab843c 100644 --- a/src/shared/export/generators/I18nGenerator.spec.ts +++ b/src/shared/export/generators/I18nGenerator.spec.ts @@ -80,6 +80,20 @@ describe('parseXlfTranslations', () => { expect(map.get('Entities & test')).toBe('Entidades & prueba'); }); + it('should not double-decode escaped entities (&lt; stays <, &amp; stays &)', () => { + // Security: decoding & -> & must happen LAST so that the literal text + // "<" (written in XML as "&lt;") does not wrongly collapse to "<". + const xlf = ` + + literal lt&lt; + literal amp&amp; + +`; + const map = parseXlfTranslations(xlf); + expect(map.get('literal lt')).toBe('<'); + expect(map.get('literal amp')).toBe('&'); + }); + it('should return an empty Map for an empty XLF string', () => { const map = parseXlfTranslations(''); expect(map.size).toBe(0); diff --git a/src/shared/export/generators/I18nGenerator.ts b/src/shared/export/generators/I18nGenerator.ts index 0a70f67328..8d639b517d 100644 --- a/src/shared/export/generators/I18nGenerator.ts +++ b/src/shared/export/generators/I18nGenerator.ts @@ -17,13 +17,13 @@ */ function decodeXmlEntities(str: string): string { return str - .replace(/&/g, '&') .replace(/</g, '<') .replace(/>/g, '>') .replace(/"/g, '"') .replace(/'/g, "'") .replace(/&#(\d+);/g, (_, num) => String.fromCharCode(parseInt(num, 10))) - .replace(/&#x([0-9a-fA-F]+);/g, (_, hex) => String.fromCharCode(parseInt(hex, 16))); + .replace(/&#x([0-9a-fA-F]+);/g, (_, hex) => String.fromCharCode(parseInt(hex, 16))) + .replace(/&/g, '&'); } /** diff --git a/src/shared/export/prerender/ServerLatexPreRenderer.spec.ts b/src/shared/export/prerender/ServerLatexPreRenderer.spec.ts index 399ab9f2ac..fbffe8d417 100644 --- a/src/shared/export/prerender/ServerLatexPreRenderer.spec.ts +++ b/src/shared/export/prerender/ServerLatexPreRenderer.spec.ts @@ -3,7 +3,7 @@ */ import { describe, it, expect, beforeAll } from 'bun:test'; -import { ServerLatexPreRenderer } from './ServerLatexPreRenderer'; +import { ServerLatexPreRenderer, cleanLatexFromHtml } from './ServerLatexPreRenderer'; describe('ServerLatexPreRenderer', () => { let renderer: ServerLatexPreRenderer; @@ -281,6 +281,44 @@ describe('ServerLatexPreRenderer', () => { }); }); + describe('cleanLatexFromHtml', () => { + it('should strip nested/obfuscated tags without leaving a reassembled tag', () => { + // A naive single pass on this payload removes the inner tag and splices + // the halves into a NEW "After'); + // The single-pass strip would have spliced the two halves back into + // a live After')).toBe('BeforeAfter'); + }); + + it('should remove script end tags spanning newlines and mixed case', () => { + expect(handler.stripHtmlTags('Before')).toBe(true); - }); - - it('is case-insensitive', () => { - expect(isUnsafeLinkScheme('JavaScript:void(0)')).toBe(true); - expect(isUnsafeLinkScheme('VBScript:msgbox(1)')).toBe(true); - expect(isUnsafeLinkScheme('DATA:text/html,x')).toBe(true); - }); - - it('tolerates leading whitespace and control characters', () => { - expect(isUnsafeLinkScheme(' javascript:alert(1)')).toBe(true); - expect(isUnsafeLinkScheme('\t\njavascript:alert(1)')).toBe(true); - expect(isUnsafeLinkScheme(' vbscript:msgbox(1)')).toBe(true); - expect(isUnsafeLinkScheme(' \tdata:text/html,x')).toBe(true); - // Leading NUL (U+0000) and US (U+001F) control-char padding that - // URL parsers strip before resolving the scheme. - const nul = String.fromCharCode(0); - const us = String.fromCharCode(0x1f); - expect(isUnsafeLinkScheme(`${nul}javascript:alert(1)`)).toBe(true); - expect(isUnsafeLinkScheme(`${us}vbscript:msgbox(1)`)).toBe(true); - expect(isUnsafeLinkScheme(`${nul}${us} data:text/html,x`)).toBe(true); - }); - - it('keeps legitimate links safe', () => { - expect(isUnsafeLinkScheme('http://example.com')).toBe(false); - expect(isUnsafeLinkScheme('https://example.com/page')).toBe(false); - expect(isUnsafeLinkScheme('//cdn.example.com/a.js')).toBe(false); - expect(isUnsafeLinkScheme('mailto:user@example.com')).toBe(false); - expect(isUnsafeLinkScheme('tel:+34123456789')).toBe(false); - expect(isUnsafeLinkScheme('files/img/photo.png')).toBe(false); - expect(isUnsafeLinkScheme('exe-node:abc')).toBe(false); - expect(isUnsafeLinkScheme('#anchor')).toBe(false); - expect(isUnsafeLinkScheme('relative/path.html')).toBe(false); - expect(isUnsafeLinkScheme('')).toBe(false); - // Substrings of dangerous schemes must not trip the prefix check. - expect(isUnsafeLinkScheme('https://x/javascript:notscheme')).toBe(false); - }); - }); - describe('Link Extraction (brokenlinks/extract)', () => { // Extract endpoint now requires authentication (bug H1); supply a token to each request. let authToken: string; diff --git a/src/routes/project.ts b/src/routes/project.ts index 45049e8e72..2a83274a1a 100644 --- a/src/routes/project.ts +++ b/src/routes/project.ts @@ -98,28 +98,6 @@ export async function resolveDefaultThemeForNewProject( return themeDirName; } -/** - * Returns true when a link URL uses a scheme that can execute script when - * navigated/rendered (e.g. `javascript:`, `vbscript:`, `data:`) and must - * therefore be discarded by the broken-link checker. - * - * The check is deliberately defensive: schemes are matched case-insensitively - * and after stripping leading whitespace and C0 control characters, which - * browsers ignore when resolving a URL. This prevents bypasses such as - * `VBScript:...`, a leading-space `" javascript:..."`, or control-character - * padding from slipping a dangerous scheme past the filter. - * - * Exported so the security property can be unit-tested directly. - */ -export function isUnsafeLinkScheme(url: string): boolean { - if (!url) return false; - // Strip leading whitespace and C0 control chars that URL parsers ignore, - // so padded schemes (e.g. a leading-space javascript:) cannot bypass the check. - // biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally matching the C0 control chars browsers ignore in URLs - const normalized = url.replace(/^[\s\u0000-\u001f]+/, '').toLowerCase(); - return normalized.startsWith('javascript:') || normalized.startsWith('vbscript:') || normalized.startsWith('data:'); -} - import { getAppVersion } from '../utils/version'; import { buildSiteThemeUrl } from '../utils/site-theme-url'; import { isSafePathSegment, isWithinBase } from '../utils/safe-path'; diff --git a/src/services/link-validator.spec.ts b/src/services/link-validator.spec.ts index d2713f13da..3eeb3ce9c6 100644 --- a/src/services/link-validator.spec.ts +++ b/src/services/link-validator.spec.ts @@ -182,6 +182,33 @@ describe('Link Validator Service', () => { expect(result).toHaveLength(0); }); + it('should remove schemes whose own letters/colon are HTML-entity encoded', () => { + // The old normalizer only decoded whitespace entities, so these + // (the scheme characters themselves encoded) slipped through. They + // must be caught to match the client-side LinkValidationAdapter. + const links: RawExtractedLink[] = [ + { url: 'javascript:alert(1)', count: 1 }, // 'j' as j + { url: 'javascript:alert(2)', count: 1 }, // 'j' as j + { url: 'javascript:alert(3)', count: 1 }, // ':' as : + { url: 'javascript:alert(4)', count: 1 }, // ':' as : + { url: 'data:text/html,evil', count: 1 }, + ]; + const result = removeInvalidLinks(links); + expect(result).toHaveLength(0); + }); + + it('should not throw on out-of-range numeric entities and keep the benign link', () => { + // Out-of-range code points must not crash String.fromCodePoint; the + // junk entity is dropped, so the benign link survives while the + // still-dangerous one is removed. + const links: RawExtractedLink[] = [ + { url: 'https://example.com/�page', count: 1 }, + { url: '�javascript:alert(1)', count: 1 }, + ]; + const result = removeInvalidLinks(links); + expect(result.map(l => l.url)).toEqual(['https://example.com/�page']); + }); + it('should keep legitimate http/https/relative links', () => { const links: RawExtractedLink[] = [ { url: 'http://example.com', count: 1 }, diff --git a/src/services/link-validator.ts b/src/services/link-validator.ts index d5b0b46767..f6fb7b26dd 100644 --- a/src/services/link-validator.ts +++ b/src/services/link-validator.ts @@ -92,21 +92,47 @@ export function cleanAndCountLinks(links: RawExtractedLink[]): RawExtractedLink[ const DANGEROUS_URL_SCHEMES = ['javascript:', 'vbscript:', 'data:']; /** - * Normalize a URL for scheme inspection: decode the HTML entities and strip - * the leading whitespace / control characters that browsers ignore when - * resolving a URL scheme, then lowercase the result. This prevents bypasses - * such as " javascript:", "JaVaScRiPt:", or "java script:". + * Normalize a URL for scheme inspection so an encoded or padded scheme cannot + * slip past the dangerous-scheme filter. Decodes HTML entities (named and + * numeric - e.g. "javascript:", "javascript:", "data:..."), + * then strips the whitespace and C0/DEL control characters that browsers ignore + * when resolving a URL scheme, and lowercases the result. This prevents bypasses + * such as " javascript:", "JaVaScRiPt:", "java script:", or "javascript:". + * + * Kept in sync with the client-side `LinkValidationAdapter._hasDangerousScheme()` + * so the broken-link checker rejects the same set of links on both ends. */ function normalizeUrlForSchemeCheck(url: string): string { - return ( - url - // Decode HTML entities for whitespace/control chars (tab, LF, CR). - .replace(/&(?:#x0*9|#0*9|#x0*a|#0*10|#x0*d|#0*13|tab|newline);/gi, '') - // Strip leading/embedded whitespace and C0 control characters (U+0000-U+0020). - // biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally matching the C0 control chars browsers ignore when resolving a URL scheme, to block obfuscated-scheme bypasses. - .replace(/[\u0000-\u0020]+/g, '') - .toLowerCase() - ); + // Safely convert a numeric code point, returning '' for invalid or + // out-of-range values so String.fromCodePoint cannot throw a RangeError + // (e.g. on "�" or "�"). Dropping such junk only makes + // scheme detection stricter, never weaker. + const safeFromCodePoint = (cp: number): string => + Number.isInteger(cp) && cp >= 0 && cp <= 0x10ffff ? String.fromCodePoint(cp) : ''; + + const namedEntities: Record = { + amp: '&', + lt: '<', + gt: '>', + quot: '"', + apos: "'", + colon: ':', + tab: '\t', + newline: '\n', + }; + + const decoded = url + .replace(/&#x([0-9a-f]+);?/gi, (_m, hex: string) => safeFromCodePoint(Number.parseInt(hex, 16))) + .replace(/&#(\d+);?/g, (_m, dec: string) => safeFromCodePoint(Number.parseInt(dec, 10))) + .replace( + /&(amp|lt|gt|quot|apos|colon|tab|newline);/gi, + (_m, name: string) => namedEntities[name.toLowerCase()] ?? _m, + ); + + // Strip whitespace and control characters (incl. NUL and DEL) that browsers + // ignore when resolving the scheme, then lowercase for comparison. + // biome-ignore lint/suspicious/noControlCharactersInRegex: intentionally matching the C0/DEL control chars browsers ignore when resolving a URL scheme, to block obfuscated-scheme bypasses. + return decoded.replace(/[\u0000-\u0020\u007f]+/g, '').toLowerCase(); } /** From 1b4be640c4c0d55231cc8c3bcbe4e24dfc30ad31 Mon Sep 17 00:00:00 2001 From: erseco Date: Thu, 18 Jun 2026 17:09:06 +0100 Subject: [PATCH 3/3] fix(security): pin mindmaps CDN scripts (js/functionality-from-untrusted-source) - jQuery 1.6.1 (ajax.googleapis.com): upgrade protocol-relative // to https and add Subresource Integrity (sha384, verified against the served bytes) plus crossorigin, matching the FileSaver.js tag fixed earlier in this PR. - Filestack legacy loader: upgrade to https. Its URL is unversioned/mutable, so an SRI hash cannot be pinned without breaking on the next update; documented inline. This at least removes the protocol-downgrade vector. --- public/app/common/mindmaps/src/index.html | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/public/app/common/mindmaps/src/index.html b/public/app/common/mindmaps/src/index.html index 4420554268..e33018f61f 100644 --- a/public/app/common/mindmaps/src/index.html +++ b/public/app/common/mindmaps/src/index.html @@ -231,8 +231,9 @@

To download the map right-click the

- - + + +