FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tools: report unsafe string and regex primordials as lint errors · nodejs/node@ac9599a · GitHub

/ node Public

Commit ac9599a

Browse files
aduh95
authored and
danielleadams
committed
tools: report unsafe string and regex primordials as lint errors
| The string method | looks up the property | | ----------------------------- | --------------------- | | `String.prototype.match` | `Symbol.match` | | `String.prototype.matchAll` | `Symbol.matchAll` | | `String.prototype.replace` | `Symbol.replace` | | `String.prototype.replaceAll` | `Symbol.replace` | | `String.prototype.search` | `Symbol.search` | | `String.prototype.split` | `Symbol.split` | Functions that lookup the `exec` property on the prototype chain: * `RegExp.prototype[Symbol.match]` * `RegExp.prototype[Symbol.matchAll]` * `RegExp.prototype[Symbol.replace]` * `RegExp.prototype[Symbol.search]` * `RegExp.prototype[Symbol.split]` * `RegExp.prototype.test` `RegExp.prototype[Symbol.replace]` and `RegExp.prototype[Symbol.split]` are still allowed for a lack of a better solution. PR-URL: #43393 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 0bb84b0 commit ac9599a

4 files changed

Lines changed: 98 additions & 17 deletions

File tree

‎lib/_tls_common.js‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ const {
2727
ArrayPrototypePush,
2828
JSONParse,
2929
ObjectCreate,
30-
StringPrototypeReplace,
30+
RegExpPrototypeSymbolReplace,
3131
} = primordials;
3232

3333
const {
@@ -134,21 +134,21 @@ function translatePeerCertificate(c) {
134134
c.infoAccess = ObjectCreate(null);
135135

136136
// XXX: More key validation?
137-
StringPrototypeReplace(info, /([^\n:]*):([^\n]*)(?:\n|$)/g,
138-
(all, key, val) => {
139-
if (val.charCodeAt(0) === 0x22) {
140-
// The translatePeerCertificate function is only
141-
// used on internally created legacy certificate
142-
// objects, and any value that contains a quote
143-
// will always be a valid JSON string literal,
144-
// so this should never throw.
145-
val = JSONParse(val);
146-
}
147-
if (key in c.infoAccess)
148-
ArrayPrototypePush(c.infoAccess[key], val);
149-
else
150-
c.infoAccess[key] = [val];
151-
});
137+
RegExpPrototypeSymbolReplace(/([^\n:]*):([^\n]*)(?:\n|$)/g, info,
138+
(all, key, val) => {
139+
if (val.charCodeAt(0) === 0x22) {
140+
// The translatePeerCertificate function is only
141+
// used on internally created legacy certificate
142+
// objects, and any value that contains a quote
143+
// will always be a valid JSON string literal,
144+
// so this should never throw.
145+
val = JSONParse(val);
146+
}
147+
if (key in c.infoAccess)
148+
ArrayPrototypePush(c.infoAccess[key], val);
149+
else
150+
c.infoAccess[key] = [val];
151+
});
152152
}
153153
return c;
154154
}

‎lib/repl.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -535,7 +535,7 @@ function REPLServer(prompt,
535535

536536
// This will set the values from `savedRegExMatches` to corresponding
537537
// predefined RegExp properties `RegExp.$1`, `RegExp.$2` ... `RegExp.$9`
538-
RegExpPrototypeTest(regExMatcher,
538+
RegExpPrototypeExec(regExMatcher,
539539
ArrayPrototypeJoin(savedRegExMatches, sep));
540540

541541
let finished = false;

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

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,5 +143,45 @@ new RuleTester({
143143
code: 'ReflectDefineProperty({}, "key", { enumerable: true })',
144144
errors: [{ message: /null-prototype/ }],
145145
},
146+
{
147+
code: 'RegExpPrototypeTest(/some regex/, "some string")',
148+
errors: [{ message: /looks up the "exec" property/ }],
149+
},
150+
{
151+
code: 'RegExpPrototypeSymbolMatch(/some regex/, "some string")',
152+
errors: [{ message: /looks up the "exec" property/ }],
153+
},
154+
{
155+
code: 'RegExpPrototypeSymbolMatchAll(/some regex/, "some string")',
156+
errors: [{ message: /looks up the "exec" property/ }],
157+
},
158+
{
159+
code: 'RegExpPrototypeSymbolSearch(/some regex/, "some string")',
160+
errors: [{ message: /looks up the "exec" property/ }],
161+
},
162+
{
163+
code: 'StringPrototypeMatch("some string", /some regex/)',
164+
errors: [{ message: /looks up the Symbol\.match property/ }],
165+
},
166+
{
167+
code: 'StringPrototypeMatchAll("some string", /some regex/)',
168+
errors: [{ message: /looks up the Symbol\.matchAll property/ }],
169+
},
170+
{
171+
code: 'StringPrototypeReplace("some string", /some regex/, "some replacement")',
172+
errors: [{ message: /looks up the Symbol\.replace property/ }],
173+
},
174+
{
175+
code: 'StringPrototypeReplaceAll("some string", /some regex/, "some replacement")',
176+
errors: [{ message: /looks up the Symbol\.replace property/ }],
177+
},
178+
{
179+
code: 'StringPrototypeSearch("some string", /some regex/)',
180+
errors: [{ message: /looks up the Symbol\.search property/ }],
181+
},
182+
{
183+
code: 'StringPrototypeSplit("some string", /some regex/)',
184+
errors: [{ message: /looks up the Symbol\.split property/ }],
185+
},
146186
]
147187
});

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,17 @@ function checkPropertyDescriptor(context, node) {
6363
});
6464
}
6565

66+
function createUnsafeStringMethodReport(context, name, lookedUpProperty) {
67+
return {
68+
[`${CallExpression}[expression.callee.name=${JSON.stringify(name)}]`](node) {
69+
context.report({
70+
node,
71+
message: `${name} looks up the ${lookedUpProperty} property on the first argument`,
72+
});
73+
}
74+
};
75+
}
76+
6677
const CallExpression = 'ExpressionStatement[expression.type="CallExpression"]';
6778
module.exports = {
6879
meta: { hasSuggestions: true },
@@ -87,6 +98,36 @@ module.exports = {
8798
[`${CallExpression}[expression.callee.name="ObjectCreate"][expression.arguments.length=2]`](node) {
8899
checkProperties(context, node.expression.arguments[1]);
89100
},
101+
[`${CallExpression}[expression.callee.name="RegExpPrototypeTest"]`](node) {
102+
context.report({
103+
node,
104+
message: '%RegExp.prototype.test% looks up the "exec" property of `this` value',
105+
suggest: [{
106+
desc: 'Use RegexpPrototypeExec instead',
107+
fix(fixer) {
108+
const testRange = { ...node.range };
109+
testRange.start = testRange.start + 'RegexpPrototype'.length;
110+
testRange.end = testRange.start + 'Test'.length;
111+
return [
112+
fixer.replaceTextRange(node.range, 'Exec'),
113+
fixer.insertTextAfter(node, ' !== null'),
114+
];
115+
}
116+
}]
117+
});
118+
},
119+
[`${CallExpression}[expression.callee.name=${/^RegExpPrototypeSymbol(Match|MatchAll|Search)$/}]`](node) {
120+
context.report({
121+
node,
122+
message: node.expression.callee.name + ' looks up the "exec" property of `this` value',
123+
});
124+
},
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'),
90131
};
91132
},
92133
};

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL