-
Notifications
You must be signed in to change notification settings - Fork 144
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check for mozIndexedDB; close #15 #44
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import path from 'path'; | ||
|
||
import ESLint from 'eslint'; | ||
|
||
import { ESLINT_TYPES } from 'const'; | ||
import * as messages from 'messages'; | ||
import ESLintRules from 'rules'; | ||
|
||
|
||
export default class JavaScriptScanner { | ||
|
||
constructor(code, filename) { | ||
this.code = code; | ||
this.filename = filename; | ||
} | ||
|
||
scan() { | ||
return new Promise((resolve) => { | ||
// ESLint is synchronous and doesn't accept streams, so we need to | ||
// pass it the entire source file as a string. | ||
let eslint = new ESLint.CLIEngine({ | ||
ignore: false, | ||
rulePaths: [path.join('src', 'rules')], | ||
rules: ESLintRules, | ||
useEslintrc: false, | ||
}); | ||
|
||
var validatorMessages = []; | ||
var report = eslint.executeOnText(this.code, this.filename); | ||
|
||
for (let message of report.results[0].messages) { | ||
validatorMessages.push({ | ||
id: message.ruleId.toUpperCase(), | ||
message: messages[message.ruleId.toUpperCase()].message, | ||
description: messages[message.ruleId.toUpperCase()].description, | ||
code: message.source, | ||
file: this.filename, | ||
line: message.line, | ||
column: message.column, | ||
severity: ESLINT_TYPES[message.severity], | ||
}); | ||
} | ||
|
||
resolve(validatorMessages); | ||
}); | ||
} | ||
|
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
// Here we can re-export all our message so one can just do | ||
// import * as messages from 'messages'; | ||
|
||
export * from './javascript'; | ||
export * from './layout'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { gettext as _ } from 'utils'; | ||
|
||
|
||
export const MOZINDEXEDDB = { | ||
code: 'MOZINDEXEDDB', | ||
message: _('mozIndexedDB has been removed; use indexedDB instead.'), | ||
description: _('mozIndexedDB has been removed; use indexedDB instead.'), | ||
}; | ||
|
||
export const MOZINDEXEDDB_PROPERTY = { | ||
code: 'MOZINDEXEDDB_PROPERTY', | ||
message: _('mozIndexedDB used as an object key/property.'), | ||
description: _('mozIndexedDB has been removed; use indexedDB instead.'), | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import { ESLINT_ERROR, ESLINT_WARNING } from 'const'; | ||
|
||
export default { | ||
mozIndexedDB: ESLINT_ERROR, | ||
mozIndexedDB_property: ESLINT_WARNING, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
module.exports = function(context) { | ||
return { | ||
Identifier: function(node) { | ||
// Catches `var foo = mozIndexedDB;`. | ||
if (node.name === 'mozIndexedDB' && | ||
node.parent.type !== 'MemberExpression') { | ||
return context.report(node, 'MOZINDEXEDDB'); | ||
} | ||
}, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
module.exports = function(context) { | ||
return { | ||
Identifier: function(node) { | ||
// Catches `var foo = 'mozIndexedDB'; var myDatabase = window[foo];`. | ||
if (node.parent.init && node.parent.init.value === 'mozIndexedDB') { | ||
return context.report(node, 'MOZINDEXEDDB_PROPERTY'); | ||
} | ||
}, | ||
MemberExpression: function(node) { | ||
// Catches `var foo = window.mozIndexedDB;` and | ||
// `var foo = window['mozIndexedDB'];`. | ||
if (node.property.name === 'mozIndexedDB' || | ||
node.property.value === 'mozIndexedDB') { | ||
context.report(node, 'MOZINDEXEDDB_PROPERTY'); | ||
} | ||
}, | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import yauzl from 'yauzl'; | ||
|
||
import { DuplicateZipEntryError } from 'exceptions'; | ||
|
||
import { endsWith } from 'utils'; | ||
|
||
/* | ||
* Simple Promise wrapper for the Yauzl unzipping lib to unpack add-on .xpis. | ||
|
@@ -96,7 +96,45 @@ export default class Xpi { | |
resolve(readStream); | ||
}); | ||
}) | ||
.catch((err) => reject(err)); | ||
.catch(reject); | ||
}); | ||
} | ||
|
||
getFileAsString(path) { | ||
return new Promise((resolve, reject) => { | ||
return this.getFileAsStream(path) | ||
.then((fileStream) => { | ||
var fileString = ''; | ||
fileStream.on('data', (chunk) => { | ||
fileString += chunk; | ||
}); | ||
|
||
// Once the file is assembled, resolve the promise. | ||
fileStream.on('end', () => { | ||
resolve(fileString); | ||
}); | ||
}) | ||
.catch(reject); | ||
}); | ||
} | ||
|
||
getJSFiles() { | ||
return new Promise((resolve, reject) => { | ||
return this.getMetaData() | ||
.then((metadata) => { | ||
let jsFiles = []; | ||
|
||
for (let filename in metadata) { | ||
// TODO: Check for JS files better than this (follow require path) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question - I wonder how the current validator deals with the possibility of imports including requires/imports of files that don't end in .js? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, it's likely something for a separate issue and could be its own can of worms. A brute-force way would be to get ESLint to try and parse every file, and anything that throws a parsing error is assumed to not be JS. There are edge cases for that implementation as well, but it would prevent people sneaking code past the validator by naming every file |
||
// of add-on? | ||
if (endsWith(filename, '.js')) { | ||
jsFiles.push(filename); | ||
} | ||
} | ||
|
||
resolve(jsFiles); | ||
}) | ||
.catch(reject); | ||
}); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's a test for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, good catch. I actually was thinking "I think I left out a test for something" but couldn't remember what.