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

TypeError occurred in matchAll #2

Closed
isnot opened this issue Feb 11, 2021 · 3 comments
Closed

TypeError occurred in matchAll #2

isnot opened this issue Feb 11, 2021 · 3 comments
Labels
enhancement New feature or request inProgress Task/issue is in progress

Comments

@isnot
Copy link

isnot commented Feb 11, 2021

Originally posted by @RomanBurunkov in #1 (comment)

Could you please elaborate the issue with TypeError occurred > in matchAll.
In which conditions that happens?

[isnot] For more details, I wrote test case for this problem.
please refer my branch 'test-isnot'. isnot#1
In my environment, TypeError came from each time at 'mpd-on-update'.

const matches = [...message.match(/(?:changed:\s*)(.*)/g)];
  • Match doesn't catch the groups with a /g flag so do we really need them in regex?

[isnot] If messages from MPD server constains multiple 'changed:', maybe need '/g flag'.
But I haven't actually confirmed this situation yet.

  • If there are no match it returns null so with spread operator we got an array with a one element like [null] with all consequences
  • Since no regex groups you have to .substring(9) but in case you have null in result array you will get an error.

[isnot] I will confirm more on this point

Thank you.

@RomanBurunkov
Copy link
Owner

RomanBurunkov commented Feb 11, 2021

Looks you are using an old node version, where matchAll hasn't been released.

You can use a polyfill for it or update node to 12 or higher. I believe it should be present in node 12.

See more more details about the polyfill at MatchAll

E.g. you can use some of the match-all packages from the npm and add matchAll method to the string proto.

if(!String.prototype.matchAll) {
	(function() {
		String.prototype.matchAll = function(reg) {
			return matchAll(this, reg);
		};
	})();
}

@isnot
Copy link
Author

isnot commented Feb 13, 2021

@RomanBurunkov

I understand. I'm using Node.js v10.
Also, the polyfill you suggested will should work fine.

By the way, I will try another way.
using a simple String method instead of RegExp.

cf. my updated fix for src/mpd.js and test case.

  _matchAllChanged(message = '') {
    const data = [];
    if (typeof message !== 'string' || message === '') {
      return data;
    }
    for (const line of message.split('\n')) {
      if (line.indexOf('changed:') === 0) {
        data.push(line.substring(8).trim());
      }
    }
    return data;
  }

On the other hand, this product currently has only one "matchAll".
It should be easy to rewrite.
And it can be adapted to a wider range of use cases.

Normaly, reducing RegExp is expected to improve performance.
In some cases, it may prevent potential ReDoS vulnerabilities in the future.

I know it's an hyperbole to say about this one fix.
But it shouldn't be a bad way.

Thank you and regards.

@RomanBurunkov RomanBurunkov added the enhancement New feature or request label Feb 15, 2021
@RomanBurunkov RomanBurunkov added the inProgress Task/issue is in progress label Mar 7, 2021
@RomanBurunkov
Copy link
Owner

Fixed in 0.3.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request inProgress Task/issue is in progress
Projects
None yet
Development

No branches or pull requests

2 participants