Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 254358c

Browse filesBrowse files
aduh95richardlau
aduh95
authored and
richardlau
committed
tools: refactor avoid-prototype-pollution lint rule
The lint rule was not catching all occurences of unsafe primordials use, and was too strict on some methods. PR-URL: #43476 Backport-PR-URL: #44926 Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
1 parent db31de6 commit 254358c
Copy full SHA for 254358c

File tree

Expand file treeCollapse file tree

3 files changed

+71
-23
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+71
-23
lines changed
Open diff view settings
Collapse file

‎lib/internal/net.js‎

Copy file name to clipboardExpand all lines: lib/internal/net.js
+6Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,16 @@ const IPv6Reg = new RegExp('^(' +
2929
')(%[0-9a-zA-Z-.:]{1,})?$');
3030

3131
function isIPv4(s) {
32+
// TODO(aduh95): Replace RegExpPrototypeTest with RegExpPrototypeExec when it
33+
// no longer creates a perf regression in the dns benchmark.
34+
// eslint-disable-next-line node-core/avoid-prototype-pollution
3235
return RegExpPrototypeTest(IPv4Reg, s);
3336
}
3437

3538
function isIPv6(s) {
39+
// TODO(aduh95): Replace RegExpPrototypeTest with RegExpPrototypeExec when it
40+
// no longer creates a perf regression in the dns benchmark.
41+
// eslint-disable-next-line node-core/avoid-prototype-pollution
3642
return RegExpPrototypeTest(IPv6Reg, s);
3743
}
3844

Collapse file

‎test/parallel/test-eslint-avoid-prototype-pollution.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-eslint-avoid-prototype-pollution.js
+23Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ new RuleTester({
4545
'ReflectDefineProperty({}, "key", { "__proto__": null })',
4646
'ObjectDefineProperty({}, "key", { \'__proto__\': null })',
4747
'ReflectDefineProperty({}, "key", { \'__proto__\': null })',
48+
'StringPrototypeReplace("some string", "some string", "some replacement")',
49+
'StringPrototypeReplaceAll("some string", "some string", "some replacement")',
50+
'StringPrototypeSplit("some string", "some string")',
4851
'new Proxy({}, otherObject)',
4952
'new Proxy({}, someFactory())',
5053
'new Proxy({}, { __proto__: null })',
@@ -167,18 +170,38 @@ new RuleTester({
167170
code: 'StringPrototypeMatch("some string", /some regex/)',
168171
errors: [{ message: /looks up the Symbol\.match property/ }],
169172
},
173+
{
174+
code: 'let v = StringPrototypeMatch("some string", /some regex/)',
175+
errors: [{ message: /looks up the Symbol\.match property/ }],
176+
},
177+
{
178+
code: 'let v = StringPrototypeMatch("some string", new RegExp("some regex"))',
179+
errors: [{ message: /looks up the Symbol\.match property/ }],
180+
},
170181
{
171182
code: 'StringPrototypeMatchAll("some string", /some regex/)',
172183
errors: [{ message: /looks up the Symbol\.matchAll property/ }],
173184
},
185+
{
186+
code: 'let v = StringPrototypeMatchAll("some string", new RegExp("some regex"))',
187+
errors: [{ message: /looks up the Symbol\.matchAll property/ }],
188+
},
174189
{
175190
code: 'StringPrototypeReplace("some string", /some regex/, "some replacement")',
176191
errors: [{ message: /looks up the Symbol\.replace property/ }],
177192
},
193+
{
194+
code: 'StringPrototypeReplace("some string", new RegExp("some regex"), "some replacement")',
195+
errors: [{ message: /looks up the Symbol\.replace property/ }],
196+
},
178197
{
179198
code: 'StringPrototypeReplaceAll("some string", /some regex/, "some replacement")',
180199
errors: [{ message: /looks up the Symbol\.replace property/ }],
181200
},
201+
{
202+
code: 'StringPrototypeReplaceAll("some string", new RegExp("some regex"), "some replacement")',
203+
errors: [{ message: /looks up the Symbol\.replace property/ }],
204+
},
182205
{
183206
code: 'StringPrototypeSearch("some string", /some regex/)',
184207
errors: [{ message: /looks up the Symbol\.search property/ }],
Collapse file

‎tools/eslint-rules/avoid-prototype-pollution.js‎

Copy file name to clipboardExpand all lines: tools/eslint-rules/avoid-prototype-pollution.js
+42-23Lines changed: 42 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
'use strict';
22

3+
const CallExpression = (fnName) => `CallExpression[callee.name=${fnName}]`;
4+
35
function checkProperties(context, node) {
46
if (
57
node.type === 'CallExpression' &&
@@ -64,8 +66,10 @@ function checkPropertyDescriptor(context, node) {
6466
}
6567

6668
function createUnsafeStringMethodReport(context, name, lookedUpProperty) {
69+
const lastDotPosition = '$String.prototype.'.length;
70+
const unsafePrimordialName = `StringPrototype${name.charAt(lastDotPosition).toUpperCase()}${name.slice(lastDotPosition + 1, -1)}`;
6771
return {
68-
[`${CallExpression}[expression.callee.name=${JSON.stringify(name)}]`](node) {
72+
[CallExpression(unsafePrimordialName)](node) {
6973
context.report({
7074
node,
7175
message: `${name} looks up the ${lookedUpProperty} property on the first argument`,
@@ -74,31 +78,46 @@ function createUnsafeStringMethodReport(context, name, lookedUpProperty) {
7478
};
7579
}
7680

77-
const CallExpression = 'ExpressionStatement[expression.type="CallExpression"]';
81+
function createUnsafeStringMethodOnRegexReport(context, name, lookedUpProperty) {
82+
const dotPosition = 'Symbol.'.length;
83+
const safePrimordialName = `RegExpPrototypeSymbol${lookedUpProperty.charAt(dotPosition).toUpperCase()}${lookedUpProperty.slice(dotPosition + 1)}`;
84+
const lastDotPosition = '$String.prototype.'.length;
85+
const unsafePrimordialName = `StringPrototype${name.charAt(lastDotPosition).toUpperCase()}${name.slice(lastDotPosition + 1, -1)}`;
86+
return {
87+
[[
88+
`${CallExpression(unsafePrimordialName)}[arguments.1.type=Literal][arguments.1.regex]`,
89+
`${CallExpression(unsafePrimordialName)}[arguments.1.type=NewExpression][arguments.1.callee.name=RegExp]`,
90+
].join(',')](node) {
91+
context.report({
92+
node,
93+
message: `${name} looks up the ${lookedUpProperty} property of the passed regex, use ${safePrimordialName} directly`,
94+
});
95+
}
96+
};
97+
}
98+
7899
module.exports = {
79100
meta: { hasSuggestions: true },
80101
create(context) {
81102
return {
82-
[`${CallExpression}[expression.callee.name=${/^(Object|Reflect)DefinePropert(ies|y)$/}]`](
83-
node
84-
) {
85-
switch (node.expression.callee.name) {
103+
[CallExpression(/^(Object|Reflect)DefinePropert(ies|y)$/)](node) {
104+
switch (node.callee.name) {
86105
case 'ObjectDefineProperties':
87-
checkProperties(context, node.expression.arguments[1]);
106+
checkProperties(context, node.arguments[1]);
88107
break;
89108
case 'ReflectDefineProperty':
90109
case 'ObjectDefineProperty':
91-
checkPropertyDescriptor(context, node.expression.arguments[2]);
110+
checkPropertyDescriptor(context, node.arguments[2]);
92111
break;
93112
default:
94113
throw new Error('Unreachable');
95114
}
96115
},
97116

98-
[`${CallExpression}[expression.callee.name="ObjectCreate"][expression.arguments.length=2]`](node) {
99-
checkProperties(context, node.expression.arguments[1]);
117+
[`${CallExpression('ObjectCreate')}[arguments.length=2]`](node) {
118+
checkProperties(context, node.arguments[1]);
100119
},
101-
[`${CallExpression}[expression.callee.name="RegExpPrototypeTest"]`](node) {
120+
[CallExpression('RegExpPrototypeTest')](node) {
102121
context.report({
103122
node,
104123
message: '%RegExp.prototype.test% looks up the "exec" property of `this` value',
@@ -116,18 +135,18 @@ module.exports = {
116135
}],
117136
});
118137
},
119-
[`${CallExpression}[expression.callee.name=${/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/}]`](node) {
138+
[CallExpression(/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/)](node) {
120139
context.report({
121140
node,
122-
message: node.expression.callee.name + ' looks up the "exec" property of `this` value',
141+
message: node.callee.name + ' looks up the "exec" property of `this` value',
123142
});
124143
},
125-
...createUnsafeStringMethodReport(context, 'StringPrototypeMatch', 'Symbol.match'),
126-
...createUnsafeStringMethodReport(context, 'StringPrototypeMatchAll', 'Symbol.matchAll'),
127-
...createUnsafeStringMethodReport(context, 'StringPrototypeReplace', 'Symbol.replace'),
128-
...createUnsafeStringMethodReport(context, 'StringPrototypeReplaceAll', 'Symbol.replace'),
129-
...createUnsafeStringMethodReport(context, 'StringPrototypeSearch', 'Symbol.search'),
130-
...createUnsafeStringMethodReport(context, 'StringPrototypeSplit', 'Symbol.split'),
144+
...createUnsafeStringMethodReport(context, '%String.prototype.match%', 'Symbol.match'),
145+
...createUnsafeStringMethodReport(context, '%String.prototype.matchAll%', 'Symbol.matchAll'),
146+
...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.replace%', 'Symbol.replace'),
147+
...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.replaceAll%', 'Symbol.replace'),
148+
...createUnsafeStringMethodReport(context, '%String.prototype.search%', 'Symbol.search'),
149+
...createUnsafeStringMethodOnRegexReport(context, '%String.prototype.split%', 'Symbol.split'),
131150

132151
'NewExpression[callee.name="Proxy"][arguments.1.type="ObjectExpression"]'(node) {
133152
for (const { key, value } of node.arguments[1].properties) {
@@ -146,15 +165,15 @@ module.exports = {
146165
});
147166
},
148167

149-
[`${CallExpression}[expression.callee.name=PromisePrototypeCatch]`](node) {
168+
[CallExpression('PromisePrototypeCatch')](node) {
150169
context.report({
151170
node,
152171
message: '%Promise.prototype.catch% look up the `then` property of ' +
153172
'the `this` argument, use PromisePrototypeThen instead',
154173
});
155174
},
156175

157-
[`${CallExpression}[expression.callee.name=PromisePrototypeFinally]`](node) {
176+
[CallExpression('PromisePrototypeFinally')](node) {
158177
context.report({
159178
node,
160179
message: '%Promise.prototype.finally% look up the `then` property of ' +
@@ -163,10 +182,10 @@ module.exports = {
163182
});
164183
},
165184

166-
[`${CallExpression}[expression.callee.name=${/^Promise(All(Settled)?|Any|Race)/}]`](node) {
185+
[CallExpression(/^Promise(All(Settled)?|Any|Race)/)](node) {
167186
context.report({
168187
node,
169-
message: `Use Safe${node.expression.callee.name} instead of ${node.expression.callee.name}`,
188+
message: `Use Safe${node.callee.name} instead of ${node.callee.name}`,
170189
});
171190
},
172191
};

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.