fix: Refactor subject pattern validation in validatePrTitle.js

[EelcoLos]

This pull request refactors the subject pattern validation in the validatePrTitle.js file. It introduces a whitelist of allowed special characters and escapes all special characters that are not in the whitelist. This ensures that the subject pattern is properly validated and improves the overall code quality.

This part of the code is being noticed by Github security checks as "Regular expression injection".
Github states on that:

Constructing a regular expression with unsanitized user input is dangerous as a malicious user may be able to modify the meaning of the expression. In particular, such a user may be able to provide a regular expression fragment that takes exponential time in the worst case, and use that to perform a Denial of Service attack.

I tried the normal sanitation rules, which broke some tests. this solution doesn't break any of the current tests.

Demo PR

Brink-Software#25

[amannn]

Please see the inline comment. Apart from this, restricting which regex values are allowed would be a breaking change and would also introduce maintenance effort. I'm therefore a bit hesitant to move forward with this, but if you can provide some examples of regex values that you find concerning might help to discuss this.

Generally, in regard to this analysis:

Constructing a regular expression with unsanitized user input is dangerous as a malicious user may be able to modify the meaning of the expression. In particular, such a user may be able to provide a regular expression fragment that takes exponential time in the worst case, and use that to perform a Denial of Service attack.

We're using a regex string that the user provides via configuration to the GitHub action, so if this would cause a DoS, it's caused by the developers themselves. Same as if you'd add an infinite loop to code. Therefore I'm not sure if this is something that needs to be handled by this action.

[amannn]

Can you provide an example of a regex that would be changed with this logic? From what I can tell the regex matches all chars that are allowed, therefore I'm wondering what would change based on this?

[amannn]

Related to this, I found these npm libraries:

• https://www.npmjs.com/package/safe-regex
• https://www.npmjs.com/package/safe-regex2

Note that both claim that:

WARNING: This module has both false positives and false negatives. It is not meant as a full checker, but it detect basic cases.

[EelcoLos]

Please see the inline comment. Apart from this, restricting which regex values are allowed would be a breaking change and would also introduce maintenance effort. I'm therefore a bit hesitant to move forward with this, but if you can provide some examples of regex values that you find concerning might help to discuss this.

Generally, in regard to this analysis:

Constructing a regular expression with unsanitized user input is dangerous as a malicious user may be able to modify the meaning of the expression. In particular, such a user may be able to provide a regular expression fragment that takes exponential time in the worst case, and use that to perform a Denial of Service attack.

We're using a regex string that the user provides via configuration to the GitHub action, so if this would cause a DoS, it's caused by the developers themselves. Same as if you'd add an infinite loop to code. Therefore I'm not sure if this is something that needs to be handled by this action.

I see and acknowledge that this would be more maintenance. The first change I made locally broke 6 of the tests. Therefore, at first, I was wondering whether I should make this PR in the first place.
But then again, I felt like: I'd rather hear your opinion then just not making the PR in the first place.
If the maintenance would be too high, I'd rather close this PR and consider the matter resolved.

[amannn]

I see, yes—thanks for starting the discussion! Based on the warning in the safe-regex packages, it seems like it's hard to get rid of all false positives. As we're not creating regexes from arbitrary user input but from a configuration that was made by developers, I currently don't see this as an issue.

[EelcoLos]

as discussed, we've decided to not go through with this PR