diff --git a/@kindspells/astro-shield/e2e/e2e.test.mts b/@kindspells/astro-shield/e2e/e2e.test.mts index 8c9e138..82084fe 100644 --- a/@kindspells/astro-shield/e2e/e2e.test.mts +++ b/@kindspells/astro-shield/e2e/e2e.test.mts @@ -551,4 +551,42 @@ describe('middleware (hybrid 3)', () => { ), ) }) + + it('does not "validate" sri signatures for cross-origin scripts that are not in the allow list', async () => { + const response = await fetch(`${baseUrl}/injected/`) + const cspHeader = response.headers.get('content-security-policy') + + assert(cspHeader !== null) + assert(cspHeader) + + const scriptDirective = cspHeader + .split(/;\s*/) + .filter(directive => directive.startsWith('script-src'))[0] + assert(scriptDirective) + + // This hash belongs to an allowed script that included its integrity + // attribute as well (https://code.jquery.com/jquery-3.7.1.slim.min.js). + assert( + scriptDirective.includes( + 'sha256-kmHvs0B+OpCW5GVHUNjv9rOmY0IvSIRcf7zGUDTDQM8=', + ), + ) + + // This hash belongs to an allowed script that did not include its + // integrity attribute (https://code.jquery.com/ui/1.13.2/jquery-ui.min.js). + assert( + scriptDirective.includes( + 'sha256-lSjKY0/srUM9BE3dPm+c4fBo1dky2v27Gdjm2uoZaL0=', + ), + ) + + // The MOST IMPORTANT assertionf of this test: + // This hash belongs to the script that is "injected" in the page + // (more precisely, that is not in the allow list) + assert( + !scriptDirective.includes( + 'sha256-BbhdlvQf/xTY9gja0Dq3HiwQF8LaCRTXxZKRutelT44=', + ), + ) + }) }) diff --git a/@kindspells/astro-shield/src/core.mjs b/@kindspells/astro-shield/src/core.mjs index e9615c0..7490c9e 100644 --- a/@kindspells/astro-shield/src/core.mjs +++ b/@kindspells/astro-shield/src/core.mjs @@ -48,7 +48,7 @@ export const generateSRIHash = data => { /** * @typedef {( - * hash: string | null, + * hash: string, * attrs: string, * setCrossorigin: boolean, * content?: string | undefined, @@ -57,23 +57,22 @@ export const generateSRIHash = data => { /** @type {ElemReplacer} */ const scriptReplacer = (hash, attrs, setCrossorigin, content) => - `${content ?? ''}` /** @type {ElemReplacer} */ const styleReplacer = (hash, attrs, setCrossorigin, content) => - `${content ?? ''}` /** @type {ElemReplacer} */ const linkStyleReplacer = (hash, attrs, setCrossorigin) => - `` -const srcRegex = /\s+(src|href)\s*=\s*("(?.*?)"|'(?.*?)')/i const integrityRegex = /\s+integrity\s*=\s*("(?.*?)"|'(?.*?)')/i const relStylesheetRegex = /\s+rel\s*=\s*('stylesheet'|"stylesheet")/i @@ -85,6 +84,7 @@ const getRegexProcessors = () => { t2: 'scripts', regex: /(\s+[a-z][a-z0-9\-_]*(=('[^']*?'|"[^"]*?"))?)*?)\s*>(?[\s\S]*?)<\/\s*script\s*>/gi, + srcRegex: /\s+src\s*=\s*("(?.*?)"|'(?.*?)')/i, replacer: scriptReplacer, hasContent: true, attrsRegex: undefined, @@ -94,6 +94,7 @@ const getRegexProcessors = () => { t2: 'styles', regex: /(\s+[a-z][a-z0-9\-_]*(=('[^']*?'|"[^"]*?"))?)*?)\s*>(?[\s\S]*?)<\/\s*style\s*>/gi, + srcRegex: /\s+(href|src)\s*=\s*("(?.*?)"|'(?.*?)')/i, // not really used replacer: styleReplacer, hasContent: true, attrsRegex: undefined, @@ -103,6 +104,7 @@ const getRegexProcessors = () => { t2: 'styles', regex: /(\s+[a-z][a-z0-9\-_]*(=('[^']*?'|"[^"]*?"))?)*?)\s*\/?>/gi, + srcRegex: /\s+href\s*=\s*("(?.*?)"|'(?.*?)')/i, replacer: linkStyleReplacer, hasContent: false, attrsRegex: relStylesheetRegex, @@ -148,11 +150,19 @@ export const updateStaticPageSriHashes = async ( let updatedContent = content let match - for (const { attrsRegex, hasContent, regex, replacer, t, t2 } of processors) { + for (const { + attrsRegex, + hasContent, + regex, + srcRegex, + replacer, + t, + t2, + } of processors) { // biome-ignore lint/suspicious/noAssignInExpressions: safe while ((match = regex.exec(content)) !== null) { const attrs = match.groups?.attrs ?? '' - const content = match.groups?.content ?? '' + const elemContent = match.groups?.content ?? '' /** @type {string | undefined} */ let sriHash = undefined @@ -167,6 +177,14 @@ export const updateStaticPageSriHashes = async ( const integrityMatch = integrityRegex.exec(attrs) const src = srcMatch?.groups?.src1 ?? srcMatch?.groups?.src2 ?? '' + if (elemContent && src) { + logger.warn( + `${t} "${src}" must have either a src/href attribute or content, but not both. Removing it.`, + ) + updatedContent = updatedContent.replace(match[0], '') + continue + } + if (integrityMatch) { sriHash = integrityMatch.groups?.integrity1 ?? @@ -217,20 +235,22 @@ export const updateStaticPageSriHashes = async ( !(allowInlineScripts === false && t === 'Script') && !(allowInlineStyles === false && t === 'Style') ) { - sriHash = generateSRIHash(content) + sriHash = generateSRIHash(elemContent) h[`inline${t}Hashes`].add(sriHash) pageHashes[t2].add(sriHash) } else { logger.warn( - `Skipping SRI hash generation for inline ${t.toLowerCase()} "${relativeFilepath}" (inline ${t2} are disabled)`, + `Removing inline ${t.toLowerCase()} block (inline ${t2} are disabled).`, ) + updatedContent = updatedContent.replace(match[0], '') + continue } } if (sriHash) { updatedContent = updatedContent.replace( match[0], - replacer(sriHash, attrs, setCrossorigin, content), + replacer(sriHash, attrs, setCrossorigin, elemContent), ) } } @@ -261,17 +281,26 @@ export const updateDynamicPageSriHashes = async ( styles: new Set(), }) - for (const { attrsRegex, hasContent, regex, replacer, t, t2 } of processors) { + for (const { + attrsRegex, + hasContent, + regex, + srcRegex, + replacer, + t, + t2, + } of processors) { // biome-ignore lint/suspicious/noAssignInExpressions: safe while ((match = regex.exec(content)) !== null) { const attrs = match.groups?.attrs ?? '' - const content = match.groups?.content ?? '' + const elemContent = match.groups?.content ?? '' /** @type {string | undefined} */ let sriHash = undefined let setCrossorigin = false if (attrs) { + // This is to skip elements that are not stylesheets if (attrsRegex && !attrsRegex.test(attrs)) { continue } @@ -280,33 +309,57 @@ export const updateDynamicPageSriHashes = async ( const integrityMatch = integrityRegex.exec(attrs) const src = srcMatch?.groups?.src1 ?? srcMatch?.groups?.src2 - if (content && src) { + if (elemContent && src) { logger.warn( - `scripts must have either a src attribute or content, but not both "${src}"`, + `${t} "${src}" must have either a src/href attribute or content, but not both. Removing it.`, ) + updatedContent = updatedContent.replace(match[0], '') continue } if (integrityMatch) { - sriHash = + const givenSriHash = integrityMatch.groups?.integrity1 ?? integrityMatch.groups?.integrity2 - if (sriHash) { + if (givenSriHash) { if (src) { const globalHash = globalHashes[t2].get(src) if (globalHash) { - if (globalHash !== sriHash) { - throw new Error( - `SRI hash mismatch for "${src}", expected "${globalHash}" but got "${sriHash}"`, + if (globalHash !== givenSriHash) { + logger.warn( + `Detected integrity hash mismatch for resource "${src}". Removing it.`, ) + updatedContent = updatedContent.replace(match[0], '') + } else { + sriHash = givenSriHash + pageHashes[t2].add(sriHash) } } else { - globalHashes[t2].set(src, sriHash) + logger.warn( + `Detected reference to not explicitly allowed external resource "${src}". Removing it.`, + ) + updatedContent = updatedContent.replace(match[0], '') + } + } else if (elemContent) { + if ( + (t2 === 'scripts' && + (sri?.allowInlineScripts ?? 'all') === 'all') || + (t2 === 'styles' && (sri?.allowInlineStyles ?? 'all') === 'all') + ) { + sriHash = givenSriHash + pageHashes[t2].add(sriHash) + } else { + logger.warn( + `Removing inline ${t.toLowerCase()} block (inline ${t2} are disabled).`, + ) + updatedContent = updatedContent.replace(match[0], '') } } - pageHashes[t2].add(sriHash) } else { - logger.warn('Found empty integrity attribute, skipping...') + logger.warn( + `Found empty integrity attribute, removing inline ${t.toLowerCase()} block.`, + ) + updatedContent = updatedContent.replace(match[0], '') } continue } @@ -325,6 +378,7 @@ export const updateDynamicPageSriHashes = async ( src.indexOf('?astro&type=') >= 0 ) ) { + // TODO: Perform fetch operation when running in dev mode logger.warn( `Unable to obtain SRI hash for local resource: "${src}"`, ) @@ -339,49 +393,39 @@ export const updateDynamicPageSriHashes = async ( pageHashes[t2].add(sriHash) } else { logger.warn( - `Detected reference to not-allow-listed external resource "${src}"`, + `Detected reference to not explicitly allowed external resource "${src}". Removing it.`, ) - if (setCrossorigin) { - updatedContent = updatedContent.replace( - match[0], - replacer(null, attrs, true, ''), - ) - } + updatedContent = updatedContent.replace(match[0], '') continue - - // TODO: add scape hatch to allow fetching arbitrary external resources - // const resourceResponse = await fetch(src, { method: 'GET' }) - // const resourceContent = await resourceResponse.arrayBuffer() - // sriHash = generateSRIHash(resourceContent) - // globalHashes[t2].set(src, sriHash) - // pageHashes[t2].add(sriHash) } } else { - logger.warn(`Unable to process external resource: "${src}"`) + // TODO: Introduce flag to decide if external resources using unknown protocols should be removed + logger.warn(`Unable to process external resource: "${src}".`) continue } } } if (hasContent && !sriHash) { - // TODO: port logic from `updateStaticPageSriHashes` to handle inline resources if ( ((sri?.allowInlineScripts ?? 'all') === 'all' && t === 'Script') || ((sri?.allowInlineStyles ?? 'all') === 'all' && t === 'Style') ) { - sriHash = generateSRIHash(content) + sriHash = generateSRIHash(elemContent) pageHashes[t2].add(sriHash) } else { logger.warn( - `Skipping SRI hash generation for inline ${t.toLowerCase()} (inline ${t2} are disabled)`, + `Removing inline ${t.toLowerCase()} block (inline ${t2} are disabled)`, ) + updatedContent = updatedContent.replace(match[0], '') + continue } } if (sriHash) { updatedContent = updatedContent.replace( match[0], - replacer(sriHash, attrs, setCrossorigin, content), + replacer(sriHash, attrs, setCrossorigin, elemContent), ) } } @@ -697,7 +741,6 @@ export const processStaticFiles = async (logger, { distDir, sri }) => { sri, ) - if (!sri.hashesModule) { return } diff --git a/@kindspells/astro-shield/tests/core.test.mts b/@kindspells/astro-shield/tests/core.test.mts index 38caa2d..28e0e09 100644 --- a/@kindspells/astro-shield/tests/core.test.mts +++ b/@kindspells/astro-shield/tests/core.test.mts @@ -365,7 +365,7 @@ describe('updateStaticPageSriHashes', () => { My Test Page - + ` @@ -382,7 +382,7 @@ describe('updateStaticPageSriHashes', () => { expect(h.extScriptHashes.size).toBe(1) expect( h.extScriptHashes.has( - 'sha256-zOEqmAz4SCAi+TcSQgdhUuurJfrfnwWqtmdTOP+bBkc=', + 'sha256-XJRisMK9wQvjjOmHgwTyaPbBdQ7sIaEh6BiqErhW4f8=', ), ).toBe(true) expect(h.inlineScriptHashes.size).toBe(0) @@ -477,8 +477,6 @@ describe('updateStaticPageSriHashes', () => { expect(h.extScriptHashes.size).toBe(0) expect(h.inlineStyleHashes.size).toBe(0) }) - - // TODO: Add tests for external styles }) describe('updateDynamicPageSriHashes', () => { @@ -659,7 +657,7 @@ describe('updateDynamicPageSriHashes', () => { expect(pageHashes.styles.size).toBe(0) }) - it('avoids adding sri hash to external script when not allow-listed (cross origin)', async () => { + it('removes external script when not explicitly allowed (cross origin)', async () => { const remoteScript = 'https://raw.githubusercontent.com/KindSpells/astro-shield/ae9521048f2129f633c075b7f7ef24e11bbd1884/main.mjs' const content = ` @@ -667,7 +665,7 @@ describe('updateDynamicPageSriHashes', () => { My Test Page - + ` @@ -676,7 +674,7 @@ describe('updateDynamicPageSriHashes', () => { My Test Page - + ` @@ -895,6 +893,136 @@ describe('updateDynamicPageSriHashes', () => { 'Unable to obtain SRI hash for local resource: "/problematic/local/script.js"', ) }) + + it('removes scripts with both src and content', async () => { + const content = ` + + My Test Page + + + + + ` + + const expected = ` + + My Test Page + + + + + ` + + // "pre-loaded" (its value does not matter in this case) + const h = getMiddlewareHashes() + h.scripts.set( + '/core.mjs', + 'sha256-6vcZ3jYR5LROXY5VlgX+tgNuIUVynHfMRQFXUnXSf64=', + ) + + const { pageHashes, updatedContent } = await updateDynamicPageSriHashes( + console, + content, + h, + ) + + expect(updatedContent).toEqual(expected) + + // "no changes" + expect(h.scripts.size).toBe(1) + expect(h.scripts.get('/core.mjs')).toEqual( + 'sha256-6vcZ3jYR5LROXY5VlgX+tgNuIUVynHfMRQFXUnXSf64=', + ) + + expect(h.styles.size).toBe(0) + expect(pageHashes.scripts.size).toBe(0) + expect(pageHashes.styles.size).toBe(0) + }) + + it('removes external scripts with integrity mismatch', async () => { + // "pre-loaded" (its value will differ from the one in the content) + const h = getMiddlewareHashes() + h.scripts.set( + '/core.mjs', + 'sha256-1111111111111111111111111111111111111111111=', + ) + + const content = ` + + My Test Page + + + + + ` + + const expected = ` + + My Test Page + + + + + ` + + const { pageHashes, updatedContent } = await updateDynamicPageSriHashes( + console, + content, + h, + ) + + expect(updatedContent).toEqual(expected) + + // "no changes" + expect(h.scripts.size).toBe(1) + expect(h.scripts.get('/core.mjs')).toEqual( + 'sha256-1111111111111111111111111111111111111111111=', + ) + + expect(h.styles.size).toBe(0) + expect(pageHashes.scripts.size).toBe(0) + expect(pageHashes.styles.size).toBe(0) + }) + + it('removes external (cross-origin) scripts with integrity attribute but not explicitly allowed', async () => { + // No "pre-loaded" hashes + const h = getMiddlewareHashes() + + const content = ` + + My Test Page + + + + + ` + + const expected = ` + + My Test Page + + + + + ` + + const { pageHashes, updatedContent } = await updateDynamicPageSriHashes( + console, + content, + h, + // We do not pass an allow-list + ) + + expect(updatedContent).toEqual(expected) + + // "no changes" + expect(h.scripts.size).toBe(0) + expect(h.styles.size).toBe(0) + expect(pageHashes.scripts.size).toBe(0) + expect(pageHashes.styles.size).toBe(0) + }) }) describe('scanAllowLists', () => { @@ -1045,7 +1173,7 @@ describe('getMiddlewareHandler', () => { `) }) - it('protects from validating disallowed inline scripts', async () => { + it('removes inline scripts when they are not allowed', async () => { const hashes = { scripts: new Map(), styles: new Map(), @@ -1083,7 +1211,7 @@ describe('getMiddlewareHandler', () => { My Test Page - + `, status: 200, @@ -1103,7 +1231,7 @@ describe('getMiddlewareHandler', () => { My Test Page - + `) }) @@ -1178,7 +1306,7 @@ describe('getCSPMiddlewareHandler', () => { `) }) - it('protects from validating disallowed inline scripts', async () => { + it('removes inline scripts when they are not allowed', async () => { const hashes = { scripts: new Map(), styles: new Map(), @@ -1217,7 +1345,7 @@ describe('getCSPMiddlewareHandler', () => { My Test Page - + `, status: 200, @@ -1237,7 +1365,7 @@ describe('getCSPMiddlewareHandler', () => { My Test Page - + `) }) diff --git a/@kindspells/astro-shield/vitest.config.unit.mts b/@kindspells/astro-shield/vitest.config.unit.mts index 3cdbb6d..b362970 100644 --- a/@kindspells/astro-shield/vitest.config.unit.mts +++ b/@kindspells/astro-shield/vitest.config.unit.mts @@ -20,7 +20,7 @@ export default defineConfig({ ], thresholds: { statements: 77.0, - branches: 77.0, + branches: 78.0, functions: 87.0, lines: 77.0, },