fix: reduce some false positive matches

[antongolub]

• fix: handle some false positive matches
  * feat: introduce SEMVER_REGEX const
• test: check what exactly is captured by regex
• test: verify test cases via semver
• chore: up deps, apply xo

closes #21

[antongolub]

@sindresorhus, rfr.

[sindresorhus]

I think it's important to ensure we don't introduce another ReDoS vulnerability with these changes. I have already had a few: https://github.com/sindresorhus/semver-regex/commits/main

https://www.google.com/search?client=safari&rls=en&q=redos+check&ie=UTF-8&oe=UTF-8

[antongolub]

Reasonable. https://devina.io/redos-checker, the results are almost identical:

Before

After

[antongolub]

@sindresorhus, can you trigger tests again?

[antongolub]

Mb add named semverRegex export instead? We'd like to avoid default exports in our codebase.

export function semverRegex() {
	return /(?<=^v?|\sv?)(?:(?:0|[1-9]\d{0,9}?)\.){2}(?:0|[1-9]\d{0,9})(?:-(?:0|[1-9]\d*|[\da-z-]*?[a-z-][\da-z-]*?)){0,100}?(?=$| |\+|\.)(?:(?<=-\S+)(?:\.(?:[\da-z-]*?[a-z-][\da-z-]*|0|[1-9]\d*)){1,100}?)?(?!\.)(?:\+(?:[\da-z-]+(?:\.[\da-z-]+)*){1,100}?(?!\w))?(?!\+)/gi;
}

export default semverRegex;

[antongolub]

I have to admit, that I have no idea how to implement trully redos-safe checker: we have a fairly wide range of chunk combinations and the semver spec does not impose their length restrictions.

[antongolub]

@sindresorhus,

Anyway, is any chance to land this?

[sindresorhus]

we have a fairly wide range of chunk combinations and the semver spec does not impose their length restrictions.

But we can restrict them to some realistic values.

[sindresorhus]

Anyway, is any chance to land this?

To be honest, I don't know where to start reviewing. The regex is just way too complicated now. Maybe if you could split up the regex into manageable parts. Like the main version component, prerelease, and build.

If you need it right away, I would suggest just doing:

npm install 'antongolub/semver-regex#fix-false-positive-matches'

[antongolub]

We have dozens packages depending on semver-regex, so I'd like to provide the fix here. The value of OSS is not in forks, but in improving existing tools (IMO)

The regex is just way too complicated now.

Like email regex, url regex, strict iso8601, etc. They are all complicated because the formats they describe are complicated too. I believe someday robots will write regexes (https://github.com/MaLeLabTs/RegexGenerator)

In general, the patch looks optimistic: the old tests work and the new ones also pass. Maybe we just need to extnd redos tests.

If the new regex completely covers the spec, perhaps this will be its last change)

[antongolub]

Leaving things as they are seems wrong:

This misbehavior has been here for so long time and probably has a bunch of side effects, that the fix will have to be published as 5.0.0.

[antongolub]

@sindresorhus,

https://devina.io/redos-checker

/(?<=^v?|\sv?)(?:(?:0|[1-9]\d{0,9}?)\.){2}(?:0|[1-9]\d{0,9})(?:-(?:--?|0|[1-9]\d*|\d*[a-z]+\d*)){0,100}(?=$| |\+|\.)(?:(?<=-\S+)(?:\.(?:--?|[\da-z-]*[a-z-]\d*|0|[1-9]\d*)){1,100}?)?(?!\.)(?:\+(?:[\da-z]\.?-?){1,100}?(?!\w))?(?!\+)/gi

[antongolub]

npm install 'antongolub/semver-regex#fix-false-positive-matches'

@sindresorhus, let's publish this as beta, so we could override the current version via yarn resolutions.

[antongolub]

The execution time is strongly related to the hardware. I could just up the threshold, but I have no idea where the optimum lies.

[sindresorhus]

CI is failing

[antongolub]

So close)

Execution time: 20
t.true(difference < 20, `Execution time: ${difference}`);

[antongolub]

✖ invalid version does not cause catatrophic backtracking Execution time: 26
t.true(difference < 25, Execution time: ${difference});

[antongolub]

I'll set the timeout to 50.

[antongolub]

@sindresorhus, your turn

[sindresorhus]

Mb add named semverRegex export instead? We'd like to avoid default exports in our codebase.

I prefer using default export when it makes sense and it makes no sense to export both a default and named export with the same thing.

[sindresorhus]

Thanks :)

[sindresorhus]

https://github.com/sindresorhus/semver-regex/releases/tag/v4.0.4