Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ A :white_check_mark: next to a section of rules means they have all been filed i
| :white_check_mark: | warning | evil | | Use of `document.write` strongly discouraged. | | | [testcases/javascript/entity_values.py](https://github.com/mozilla/amo-validator/blob/master/validator/testcases/javascript/entity_values.py#L64) | NO_DOCUMENT_WRITE |
| :white_check_mark: | warning | nsIDNSServiceResolve | | `nsIDNSService.resolve()` should not be used. | | | [testcases/javascript/entity_values.py](https://github.com/mozilla/amo-validator/blob/master/validator/testcases/javascript/entity_values.py#L87) | NSI_DNS_SERVICE_RESOLVE |
| :white_check_mark: | warning | nsISound_play | | `nsISound.play` should not be used | | | [testcases/javascript/entity_values.py](https://github.com/mozilla/amo-validator/blob/master/validator/testcases/javascript/entity_values.py#L103) | NSI_SOUND_PLAY |
| :x: | warning | init | | `init` should not be called with a null first argument | | | | |
| :white_check_mark: | warning | init | | `init` should not be called with a null first argument | | | [testcases/javascript/entity_values.py](https://github.com/mozilla/amo-validator/blob/master/validator/testcases/javascript/entity_values.py#L129) | INIT_NULL_ARG |
| :white_check_mark: | warning | override | | Extensions must not alter user preferences such as the new tab URL without explicit user consent. | | | [testcases/javascript/entity_values.py](https://github.com/mozilla/amo-validator/blob/master/validator/testcases/javascript/entity_values.py#L153) | TAB_URL_OVERRIDE |

### instanceactions
Expand Down
1 change: 1 addition & 0 deletions src/const.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export const ESLINT_RULE_MAPPING = {
deprecated_entities: ESLINT_WARNING,
eval_string_arg: ESLINT_ERROR,
global_require_arg: ESLINT_WARNING,
init_null_arg: ESLINT_WARNING,
low_level_module: ESLINT_WARNING,
mozindexeddb: ESLINT_ERROR,
mozindexeddb_property: ESLINT_WARNING,
Expand Down
10 changes: 10 additions & 0 deletions src/messages/javascript.js
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,16 @@ export const NSI_DNS_SERVICE_RESOLVE = {
],
};

export const INIT_NULL_ARG = {
code: 'INIT_NULL_ARG',
message: _('`init` should not be called with a null first argument'),
description: _(singleLineString`Calling 'nsITransferable.init()' with a null
first argument has the potential to leak data across private browsing mode
sessions. 'null' is appropriate only when reading data or writing data
which is not associated with a particular window.`),
legacyCode: ['js_entity_values', 'nsITransferable', 'init'],
};

export const NSI_SOUND_PLAY = {
code: 'NSI_SOUND_PLAY',
message: _('nsISound.play should not be used.'),
Expand Down
6 changes: 3 additions & 3 deletions src/rules/javascript/deprecated_entities.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { NO_DOCUMENT_WRITE, NSI_DNS_SERVICE_RESOLVE,
NSI_SOUND_PLAY, TAB_URL_OVERRIDE } from 'messages';
import { getNodeReferenceName } from 'utils';
import { getNodeReference } from 'utils';

export const DEPRECATED_ENTITIES = [
{
Expand Down Expand Up @@ -31,11 +31,11 @@ export function deprecated_entities(context) {
node.callee.property.type === 'Identifier' &&
node.callee.object.type === 'Identifier') {

let objectName = getNodeReferenceName(context, node.callee.object);
let nodeReference = getNodeReference(context, node.callee.object);

for (let entity of DEPRECATED_ENTITIES) {
// Check to see if the node matches a deprecated entity.
if (objectName === entity.object &&
if (nodeReference.name === entity.object &&
node.callee.property.name === entity.property) {
return context.report(node, entity.error.code);
}
Expand Down
1 change: 1 addition & 0 deletions src/rules/javascript/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export * from './banned_identifiers';
export * from './deprecated_entities';
export * from './eval_string_arg';
export * from './global_require_arg';
export * from './init_null_arg';
export * from './low_level_module';
export * from './mozindexeddb';
export * from './mozindexeddb_property';
Expand Down
29 changes: 29 additions & 0 deletions src/rules/javascript/init_null_arg.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import { INIT_NULL_ARG } from 'messages/javascript';
import { getNodeReference } from 'utils';

export function init_null_arg(context) {
return {
CallExpression: function(node) {
// Check if what's being called is nsiTransferable.init()
if (typeof node.callee !== 'undefined') {
let nodeReference = getNodeReference(context, node.callee);
let nodeObject = getNodeReference(context, nodeReference.object);

if (nodeObject === undefined) {
return;
}
if (nodeObject.name === 'nsITransferable' &&
nodeReference.property.name === 'init') {

if (node.arguments.length > 0) {
// Get the reference to the first arg and check if it's null.
let arg = getNodeReference(context, node.arguments[0]);
if (arg.value === null) {
return context.report({node: node, message: INIT_NULL_ARG.code});
}
}
}
}
},
};
}
14 changes: 9 additions & 5 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,15 @@ export function getRootExpression(node) {
* example: var foo = document;
* The node for foo will return 'document'
*/
export function getNodeReferenceName(context, node) {
export function getNodeReference(context, node) {
var variables = context.getScope().variables;
var scopeVar;

// Just return the value if the node passed in is a reference to a literal.
if (typeof node === 'undefined' || node.type === 'Literal') {
return node;
}

// Finds variable reference in current scope.
for (let variable of variables) {
if (variable.name === node.name) {
Expand All @@ -87,12 +92,11 @@ export function getNodeReferenceName(context, node) {
occurance.declarations[0].init !== null) {
// Get what the name of what it was assigned to or the raw
// value depending on the initalization
lastAssignment = occurance.declarations[0].init.name ||
occurance.declarations[0].init.raw;
lastAssignment = occurance.declarations[0].init;
} else if (occurance.type === 'ExpressionStatement' &&
occurance.expression.type === 'AssignmentExpression') {
// Get the right hand side of the assignment
lastAssignment = occurance.expression.right.name;
lastAssignment = occurance.expression.right;
}
}

Expand All @@ -105,7 +109,7 @@ export function getNodeReferenceName(context, node) {

// If that variable doesn't exist in scope, then just return the node's
// name.
return node.name;
return node;
}

/*
Expand Down
83 changes: 83 additions & 0 deletions tests/rules/javascript/test.init_null_arg.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
import JavaScriptScanner from 'scanners/javascript';
import { VALIDATION_WARNING } from 'const';
import * as messages from 'messages';

describe('init_null_arg', () => {
it('should not allow init called with null', () => {
var code = 'nsITransferable.init(null);';
var jsScanner = new JavaScriptScanner(code, 'badcode.js');

return jsScanner.scan()
.then((validationMessages) => {
assert.equal(validationMessages.length, 1);
assert.equal(validationMessages[0].code, messages.INIT_NULL_ARG.code);
assert.equal(validationMessages[0].type, VALIDATION_WARNING);
});
});

it('should not allow init called with a null ref', () => {
var code = 'var foo = null; nsITransferable.init(foo);';
var jsScanner = new JavaScriptScanner(code, 'badcode.js');

return jsScanner.scan()
.then((validationMessages) => {
assert.equal(validationMessages.length, 1);
assert.equal(validationMessages[0].code, messages.INIT_NULL_ARG.code);
assert.equal(validationMessages[0].type, VALIDATION_WARNING);
});
});

it('should allow init called with a non-null arg', () => {
var code = 'nsITransferable.init();';
var jsScanner = new JavaScriptScanner(code, 'badcode.js');

return jsScanner.scan()
.then((validationMessages) => {
assert.equal(validationMessages.length, 0);
});
});

it('should not allow init called with an aliased varaible and null', () => {
var code = 'var foo = nsITransferable; foo.init(null);';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, can you add one more test to show it catches this?

var foo = nsITransferable.init;
foo(null);

var jsScanner = new JavaScriptScanner(code, 'badcode.js');

return jsScanner.scan()
.then((validationMessages) => {
assert.equal(validationMessages.length, 1);
assert.equal(validationMessages[0].code, messages.INIT_NULL_ARG.code);
assert.equal(validationMessages[0].type, VALIDATION_WARNING);
});
});

it('should not allow nsITransferable.init called aliased with null', () => {
var code = 'var foo = nsITransferable.init; foo(null);';
var jsScanner = new JavaScriptScanner(code, 'badcode.js');

return jsScanner.scan()
.then((validationMessages) => {
assert.equal(validationMessages.length, 1);
assert.equal(validationMessages[0].code, messages.INIT_NULL_ARG.code);
assert.equal(validationMessages[0].type, VALIDATION_WARNING);
});
});

it('should allow nsITransferable.init called aliased without null', () => {
var code = 'var foo = nsITransferable.init; foo();';
var jsScanner = new JavaScriptScanner(code, 'badcode.js');

return jsScanner.scan()
.then((validationMessages) => {
assert.equal(validationMessages.length, 0);
});
});

it('should allow init called with an aliased varaible but not null', () => {
var code = 'var foo = nsITransferable; foo.init();';
var jsScanner = new JavaScriptScanner(code, 'badcode.js');

return jsScanner.scan()
.then((validationMessages) => {
assert.equal(validationMessages.length, 0);
});
});
});
10 changes: 5 additions & 5 deletions tests/test.utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe('utils.getRootExpression()', function() {
});
});

describe('utils.getNodeReferenceName()', () => {
describe('utils.getNodeReference()', () => {
// Represents scope for following code:
// var foo = window; foo = bar;
var context = {
Expand Down Expand Up @@ -114,16 +114,16 @@ describe('utils.getNodeReferenceName()', () => {

it('should return the name of the referenced variable', () => {
var ref = { name: 'foo' };
var val = utils.getNodeReferenceName(context, ref);
var val = utils.getNodeReference(context, ref);

assert.equal(val, 'bar');
assert.equal(val.name, 'bar');
});

it('should return the name of the reference if not in scope', () => {
var ref = { name: 'doesNotExist' };
var val = utils.getNodeReferenceName(context, ref);
var val = utils.getNodeReference(context, ref);

assert.equal(val, ref.name);
assert.equal(val.name, ref.name);
});
});

Expand Down