fix: Refactor subject pattern validation in validatePrTitle.js #251
Conversation
EelcoLos
force-pushed
the
regex-injection-fix
branch
from
fbc342b
to
23d3c6f
Compare
There was a problem hiding this comment.
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.
| const sanitizedPattern = subjectPattern.replace( | ||
| /([.*+?^${}()|[\]\\])/g, | ||
| (match) => (allowedSpecialChars.includes(match) ? match : `\\${match}`) | ||
| ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Related to this, I found these npm libraries:
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.
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. |
|
I see, yes—thanks for starting the discussion! Based on the warning in the |
|
as discussed, we've decided to not go through with this PR |
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:
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