From 6df5e5212c7c7e0b52ea9125d6f2f30d708501bc Mon Sep 17 00:00:00 2001 From: Raymond Hill Date: Tue, 2 Jan 2018 08:16:25 -0500 Subject: [PATCH] code review following feedback: https://github.com/gorhill/uMatrix/commit/821e45751a2dff18fbe131637d90380e72b688bd#commitcomment-26587989 --- src/css/popup.css | 2 +- src/js/background.js | 2 +- src/js/contentscript-start.js | 56 ++++++++++++----------------------- src/js/contentscript.js | 6 ++-- src/js/traffic.js | 2 -- 5 files changed, 24 insertions(+), 44 deletions(-) diff --git a/src/css/popup.css b/src/css/popup.css index bcd7542..b5a468b 100644 --- a/src/css/popup.css +++ b/src/css/popup.css @@ -129,7 +129,7 @@ body .toolbar button.fa { stroke: none; } #mtxSwitches > li.relevant > svg .dot { - fill: #888; + fill: #aaa; } #mtxSwitches > li.switchTrue.relevant > svg .dot { fill: #eee; diff --git a/src/js/background.js b/src/js/background.js index 50d18f8..5ac3651 100644 --- a/src/js/background.js +++ b/src/js/background.js @@ -110,7 +110,7 @@ return { }, clearBrowserCacheCycle: 0, - cspNoInlineScript: "script-src 'unsafe-eval' blob: *; report-uri about:blank", + cspNoInlineScript: "script-src 'unsafe-eval' blob: *", cspNoWorker: undefined, updateAssetsEvery: 11 * oneDay + 1 * oneHour + 1 * oneMinute + 1 * oneSecond, firstUpdateAfter: 11 * oneMinute, diff --git a/src/js/contentscript-start.js b/src/js/contentscript-start.js index 373791a..fccfe81 100644 --- a/src/js/contentscript-start.js +++ b/src/js/contentscript-start.js @@ -30,11 +30,9 @@ if ( typeof vAPI !== 'object' ) { return; } - vAPI.selfScriptSrcReported = vAPI.selfScriptSrcReported || false; vAPI.selfWorkerSrcReported = vAPI.selfWorkerSrcReported || false; - var reBadScriptSrc = /script-src[^;,]+?'(?:unsafe-inline|nonce-[^']+)'/, - reGoodWorkerSrc = /(?:child|worker)-src[^;,]+?'none'/; + var reGoodWorkerSrc = /(?:child|worker)-src[^;,]+?'none'/; var handler = function(ev) { if ( @@ -44,54 +42,38 @@ return false; } - // We do not want to report internal resources more than once. - // However, we do want to report external resources each time. - // TODO: this could eventually lead to duplicated reports for external - // resources if another extension uses the same approach as - // uMatrix. Think about what could be done to avoid duplicate - // reports. - var internal = ev.blockedURI.includes('://') === false; - // Firefox and Chromium differs in how they fill the - // 'effectiveDirective' property. Need to normalize here. - var directive = ev.effectiveDirective; - if ( directive.startsWith('script-src') ) { - if ( internal && vAPI.selfScriptSrcReported ) { return true; } - directive = 'script-src'; - } else if ( - directive.startsWith('worker-src') || - directive.startsWith('child-src') + // 'effectiveDirective' property. + if ( + ev.effectiveDirective.startsWith('worker-src') === false && + ev.effectiveDirective.startsWith('child-src') === false ) { - if ( internal && vAPI.selfWorkerSrcReported ) { return true; } - directive = 'worker-src'; - } else { return false; } // Further validate that the policy violation is relevant to uMatrix: // the event still could have been fired as a result of a CSP header // not injected by uMatrix. - if ( directive === 'script-src' ) { - if ( reBadScriptSrc.test(ev.originalPolicy) === true ) { - return false; - } - if ( internal ) { - vAPI.selfScriptSrcReported = true; - } - } else /* if ( directive === 'worker-src' ) */ { - if ( reGoodWorkerSrc.test(ev.originalPolicy) === false ) { - return false; - } - if ( internal ) { - vAPI.selfWorkerSrcReported = true; - } + if ( reGoodWorkerSrc.test(ev.originalPolicy) === false ) { + return false; + } + + // We do not want to report internal resources more than once. + // However, we do want to report external resources each time. + // TODO: this could eventually lead to duplicated reports for external + // resources if another extension uses the same approach as + // uMatrix. Think about what could be done to avoid duplicate + // reports. + if ( ev.blockedURI.includes('://') === false ) { + if ( vAPI.selfWorkerSrcReported ) { return true; } + vAPI.selfWorkerSrcReported = true; } vAPI.messaging.send( 'contentscript.js', { what: 'securityPolicyViolation', - directive: directive, + directive: 'worker-src', blockedURI: ev.blockedURI, documentURI: ev.documentURI, blocked: ev.disposition === 'enforce' diff --git a/src/js/contentscript.js b/src/js/contentscript.js index 54e72ff..7d13dbd 100644 --- a/src/js/contentscript.js +++ b/src/js/contentscript.js @@ -408,15 +408,15 @@ var collapser = (function() { (function() { if ( - vAPI.selfScriptSrcReported !== true && - document.querySelector('script:not([src])') !== null + document.querySelector('script:not([src])') !== null || + document.querySelector('a[href^="javascript:"]') !== null || + document.querySelector('[onabort],[onblur],[oncancel],[oncanplay],[oncanplaythrough],[onchange],[onclick],[onclose],[oncontextmenu],[oncuechange],[ondblclick],[ondrag],[ondragend],[ondragenter],[ondragexit],[ondragleave],[ondragover],[ondragstart],[ondrop],[ondurationchange],[onemptied],[onended],[onerror],[onfocus],[oninput],[oninvalid],[onkeydown],[onkeypress],[onkeyup],[onload],[onloadeddata],[onloadedmetadata],[onloadstart],[onmousedown],[onmouseenter],[onmouseleave],[onmousemove],[onmouseout],[onmouseover],[onmouseup],[onwheel],[onpause],[onplay],[onplaying],[onprogress],[onratechange],[onreset],[onresize],[onscroll],[onseeked],[onseeking],[onselect],[onshow],[onstalled],[onsubmit],[onsuspend],[ontimeupdate],[ontoggle],[onvolumechange],[onwaiting],[onafterprint],[onbeforeprint],[onbeforeunload],[onhashchange],[onlanguagechange],[onmessage],[onoffline],[ononline],[onpagehide],[onpageshow],[onrejectionhandled],[onpopstate],[onstorage],[onunhandledrejection],[onunload],[oncopy],[oncut],[onpaste]') !== null ) { vAPI.messaging.send('contentscript.js', { what: 'securityPolicyViolation', directive: 'script-src', documentURI: window.location.href }); - vAPI.selfScriptSrcReported = true; } collapser.addMany(document.querySelectorAll('img')); diff --git a/src/js/traffic.js b/src/js/traffic.js index cbe4f46..36d732a 100644 --- a/src/js/traffic.js +++ b/src/js/traffic.js @@ -310,8 +310,6 @@ var onHeadersReceived = function(details) { // blocked by our request handler. if ( µm.mustAllow(rootHostname, requestHostname, 'script' ) !== true ) { csp.push(µm.cspNoInlineScript); - } else { - cspReport.push(µm.cspNoInlineScript); } // TODO: Firefox will eventually support `worker-src`: