Skip to content

possible polynomial runtime due to backtracking #1440

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

elaneri
Copy link

@elaneri elaneri commented Apr 2, 2023

Hi, I found this could be a issue

Make sure the regex used here, which is vulnerable to polynomial runtime due to backtracking, cannot lead to denial of service.

@CLAassistant
Copy link

CLAassistant commented Apr 2, 2023

CLA assistant check
All committers have signed the CLA.

@ElijahPepe
Copy link

The general idea here is lost, because the time complexity is equivalent for both implementations. However, I will add that Java uses a lightweight implementation of .split() for single-character strings (that are not meta characters) that doesn't use RegEx, to my knowledge. In this case, trimming the string afterwards would be faster. LGTM.

@uwussimo
Copy link

uwussimo commented Apr 3, 2023

Based on the potential issue described, the update to the code is a good improvement and should be approved.

Adding the trim() method call on the token ensures that any leading or trailing whitespaces in the input string are removed before converting the string to an enum value. This helps avoid any possible IllegalArgumentException that could have occurred if there were any leading or trailing whitespaces in the input string.

The update also doesn't change the overall behavior of the method and doesn't introduce any new issues or side effects. Therefore, it is recommended to approve the update to ensure the code is more robust and less prone to errors.

@ElijahPepe
Copy link

@uwussimo Not sure where you're getting some of that from, this is a function taking a string and splitting it by whitespace padded commas.

@elaneri
Copy link
Author

elaneri commented Apr 3, 2023

Hi , @ElijahPepe, here you can find more information about this https://rules.sonarsource.com/java/RSPEC-5852
regards

@ElijahPepe
Copy link

This code is used in a specific, uncontrollable string that wouldn't be susceptible to a ReDoS. Not saying that the pull request isn't valid, but there's no security risk here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants