Skip to content
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

lib: avoid unsafe String methods that depend on RegExp prototype methods #36593

Closed

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Dec 21, 2020

The string prototype methods that take RegExp arguments depend on the following properties not being mutated by user code:

The String.prototype method depends on the RegExp.prototype method
String.prototype.match RegExp.prototype[Symbol.match], which depends
on RegExp.prototype.exec
String.prototype.matchAll RegExp.prototype[Symbol.matchAll], which depends
on %RegExpStringIteratorPrototype%
String.prototype.replace RegExp.prototype[Symbol.replace]
String.prototype.replaceAll RegExp.prototype[Symbol.replace]
String.prototype.search RegExp.prototype[Symbol.search]
String.prototype.split RegExp.prototype[Symbol.split]

Note that the string‑taking overloads are safe‑ish.


Alternatively, it might be a good idea to create SafeRegExp.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 21, 2020
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Can you split this into several PRs? It would make reviewing and benchmarking easier.

const re = new RegExp('^' + StringPrototypeReplace(
StringPrototypeReplace(servername, /([.^$+?\-\\[\]{}])/g, '\\$1'),
/\*/g, '[^.]*'
const re = new RegExp('^' + StringPrototypeReplaceAll(
Copy link
Contributor

Choose a reason for hiding this comment

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

String.prototype.replaceAll is not available to primordials because it can be disabled by --no-harmony-string-replaceall

@ExE-Boss ExE-Boss marked this pull request as draft November 27, 2021 11:36
@ExE-Boss ExE-Boss force-pushed the lib/avoid-unsafe-string-regexp-methods branch from 4ca6ac7 to eeb3a9b Compare November 27, 2021 13:32
@ExE-Boss ExE-Boss marked this pull request as ready for review November 27, 2021 13:33
@ExE-Boss ExE-Boss marked this pull request as draft November 27, 2021 13:33
@aduh95
Copy link
Contributor

aduh95 commented Nov 27, 2021

Note that StringPrototypeReplaceAll is still relying on %StringPrototype%[@@replace] being undefined.

// User-land
RegExp.prototype[Symbol.replace] = () => 'foo';
String.prototype[Symbol.replace] = () => 'baz';
// Core
console.log(StringPrototypeReplace('ber', /e/, 'a')); // 'foo'
console.log(StringPrototypeReplace('ber', 'e', 'a')); // 'baz'
console.log(StringPrototypeReplaceAll('beer', 'e', 'a')); // 'baz'
console.log(RegExpPrototypeSymbolReplace(/e/, 'ber', 'a')); // 'bar'
console.log(RegExpPrototypeSymbolReplace(/e/g, 'beer', 'a')); // 'baar'

It's hard to predict if having a RegEx is worth it performance wise, but it's something we should have in mind.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jul 7, 2022

Superseded by #43393 and #43475.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants