From f1d16fa1678f396d6f55f9d2b34ac2057074aa92 Mon Sep 17 00:00:00 2001 From: Andy Date: Fri, 21 Apr 2017 17:08:53 -0700 Subject: [PATCH] warn on inline script tags in HTML --- docs/rules.md | 3 +- src/const.js | 4 --- src/messages/html.js | 21 ++++-------- src/rules/html/ensureRequiredAttributes.js | 40 ---------------------- src/rules/html/index.js | 2 +- src/rules/html/warn-on-inline.js | 22 ++++++++++++ src/scanners/html.js | 1 - tests/scanners/test.html.js | 37 ++++++++------------ 8 files changed, 45 insertions(+), 85 deletions(-) delete mode 100644 src/rules/html/ensureRequiredAttributes.js create mode 100644 src/rules/html/warn-on-inline.js diff --git a/docs/rules.md b/docs/rules.md index 07824f83..86c55b90 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -146,7 +146,6 @@ A :white_check_mark: next to a section of rules means they have all been filed i | :negative_squared_cross_mark: | warning | theme_xbl_property | theme | Themes are not allowed to use XBL properties | | | | | | :negative_squared_cross_mark: | warning | unsafe_langpack_theme | theme / langpack | Unsafe tag for add-on type | | | | | | :x: | warning | remote_src_href | theme / langpack | `src`/`href` attributes must be local | | | | | -| :white_check_mark: | warning | prefwindow_id | | `` elements must have IDs | | | | PREFWINDOW_REQUIRES_ID | | :x: | warning | iframe_type_unsafe | | iframe/browser missing 'type' attribute | | | | | | :x: | warning | iframe_type_unsafe | | Typeless iframes/browsers must be local | | | | | | :x: | warning | banned_remote_scripts | | Scripts must not be remote | | | | | @@ -158,7 +157,7 @@ A :white_check_mark: next to a section of rules means they have all been filed i | :x: | warning | extra_closing_tags | | Markup parsing error | | | | | | :x: | warning | extra_closing_tags | | Parse error: tag closed before opened | | | | | | :x: | warning | invalid_nesting | | Markup invalidly nested | | | | | - +| :white_check_mark: | warning | inline script | | Inline script is disallowed by CSP | | | | INLINE_SCRIPT | ## chrome.manifest diff --git a/src/const.js b/src/const.js index 401db7ac..1a35fa12 100644 --- a/src/const.js +++ b/src/const.js @@ -67,10 +67,6 @@ export const RDF_UNALLOWED_TAGS = ['hidden']; export const RDF_UNALLOWED_IF_LISTED_TAGS = ['updateKey', 'updateURL']; export const RDF_OBSOLETE_TAGS = ['file', 'requires', 'skin']; -export const HTML_TAGS_WITH_REQUIRED_ATTRIBUTES = { - prefwindow: ['id'], -}; - // Package type constants. export const PACKAGE_ANY = 0; export const PACKAGE_EXTENSION = 1; diff --git a/src/messages/html.js b/src/messages/html.js index 02890ea5..f047958f 100644 --- a/src/messages/html.js +++ b/src/messages/html.js @@ -1,18 +1,9 @@ import { gettext as _, singleLineString } from 'utils'; - -export var _tagRequiresAttribute = (tagName, attribute) => { - return { - code: `${tagName}_REQUIRES_${attribute}`.toUpperCase(), - legacyCode: [ - 'markup', - 'starttag', - `${tagName}_${attribute}`, - ], - message: _(`<${tagName}> missing "${attribute}"`), - description: _(singleLineString`The <${tagName}> tag requires the - ${attribute}, but it's missing.`), - }; +export const INLINE_SCRIPT = { + code: 'INLINE_SCRIPT', + legacyCode: null, + message: _('Inline scripts blocked by default'), + description: _(singleLineString`Default CSP rules prevent inline JavaScript + from running (https://mzl.la/2pn32nd).`), }; - -export const PREFWINDOW_REQUIRES_ID = _tagRequiresAttribute('prefwindow', 'id'); diff --git a/src/rules/html/ensureRequiredAttributes.js b/src/rules/html/ensureRequiredAttributes.js deleted file mode 100644 index 07a9be0d..00000000 --- a/src/rules/html/ensureRequiredAttributes.js +++ /dev/null @@ -1,40 +0,0 @@ -import { HTML_TAGS_WITH_REQUIRED_ATTRIBUTES, VALIDATION_ERROR } from 'const'; -import * as messages from 'messages'; - - -export function ensureRequiredAttributes($, filename) { - return new Promise((resolve) => { - var linterMessages = []; - - for (let tag in HTML_TAGS_WITH_REQUIRED_ATTRIBUTES) { - linterMessages = linterMessages.concat( - _ensureAttributesInTag($, tag, HTML_TAGS_WITH_REQUIRED_ATTRIBUTES[tag], - filename)); - } - - resolve(linterMessages); - }); -} - -export function _ensureAttributesInTag($, tag, attributes, filename) { - var linterMessages = []; - - $(tag).each((i, element) => { - for (let attributeName of attributes) { - var errorCode = `${tag}_REQUIRES_${attributeName}`.toUpperCase(); - - if ($(element).attr(attributeName) === undefined) { - linterMessages.push({ - code: errorCode, - message: messages[errorCode].message, - description: messages[errorCode].description, - sourceCode: `<${tag}>`, - file: filename, - type: VALIDATION_ERROR, - }); - } - } - }); - - return linterMessages; -} diff --git a/src/rules/html/index.js b/src/rules/html/index.js index 257d428d..bdff8c2e 100644 --- a/src/rules/html/index.js +++ b/src/rules/html/index.js @@ -1 +1 @@ -export * from './ensureRequiredAttributes'; +export * from './warn-on-inline'; diff --git a/src/rules/html/warn-on-inline.js b/src/rules/html/warn-on-inline.js new file mode 100644 index 00000000..1412723d --- /dev/null +++ b/src/rules/html/warn-on-inline.js @@ -0,0 +1,22 @@ +import { VALIDATION_WARNING } from 'const'; +import * as messages from 'messages'; + + +export function warnOnInline($, filename) { + return new Promise((resolve) => { + var linterMessages = []; + $('script').each((i, element) => { + if ($(element).attr('src') === undefined) { + linterMessages.push( + Object.assign({}, messages.INLINE_SCRIPT, { + /* This could occur in any HTML file, so let's make it + * a warning in case they've included any other file. + */ + type: VALIDATION_WARNING, + file: filename, + })); + } + }); + resolve(linterMessages); + }); +} diff --git a/src/scanners/html.js b/src/scanners/html.js index 38b2b9c9..6f9eb982 100644 --- a/src/scanners/html.js +++ b/src/scanners/html.js @@ -11,7 +11,6 @@ export default class HTMLScanner extends BaseScanner { _getContents() { return new Promise((resolve) => { var htmlDoc = cheerio.load(this.contents); - resolve(htmlDoc); }); } diff --git a/tests/scanners/test.html.js b/tests/scanners/test.html.js index c9fdd9f4..f7f69e11 100644 --- a/tests/scanners/test.html.js +++ b/tests/scanners/test.html.js @@ -1,7 +1,7 @@ import cheerio from 'cheerio'; import sinon from 'sinon'; -import { VALIDATION_ERROR } from 'const'; +import { VALIDATION_WARNING } from 'const'; import { getRuleFiles, validHTML } from '../helpers'; import HTMLScanner from 'scanners/html'; import * as rules from 'rules/html'; @@ -31,37 +31,30 @@ describe('HTML', function() { }); }); - it('should require tag to have an id attribute', () => { - var badHTML = validHTML(singleLineString` - `); - var goodHTML = validHTML(singleLineString` - `); + it('should require '); var htmlScanner = new HTMLScanner(badHTML, 'index.html'); return htmlScanner.getContents() .then(($) => { - return rules.ensureRequiredAttributes($, htmlScanner.filename); + return rules.warnOnInline($, htmlScanner.filename); }) .then((linterMessages) => { assert.equal(linterMessages.length, 1); assert.equal(linterMessages[0].code, - messages.PREFWINDOW_REQUIRES_ID.code); - assert.equal(linterMessages[0].sourceCode, ''); - assert.equal(linterMessages[0].type, VALIDATION_ERROR); + messages.INLINE_SCRIPT.code); + assert.equal(linterMessages[0].type, VALIDATION_WARNING); + }); + }); - // Make sure there are no errors when an ID is provided. - htmlScanner = new HTMLScanner(goodHTML, 'index.html'); - return htmlScanner.getContents(); - }) + it('should accept a `); + var htmlScanner = new HTMLScanner(goodHTML, 'index.html'); + + return htmlScanner.getContents() .then(($) => { - return rules.ensureRequiredAttributes($, htmlScanner.filename); + return rules.warnOnInline($, htmlScanner.filename); }) .then((linterMessages) => { assert.equal(linterMessages.length, 0);