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

[JENKINS-42148] Fix catastrophic backtracking in admin list form validation #485

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@ydubreuil
Copy link

commented Feb 17, 2017

In order to prevent backtracking, this patch splits the admin list and
validate that each admin matches the constraint. The regular expression
is simplified so it can't backtrack anymore.

@reviewbybees

[JENKINS-42148] Fix catastrophic backtracking in admin list form vali…
…dation

In order to prevent backtracking, this patch splits the admin list and
validate that each admin matches the constraint. The regular expression
is simplified so it can't backtrack anymore.
@reviewbybees

This comment has been minimized.

Copy link

commented Feb 25, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@benpatterson

This comment has been minimized.

Copy link
Member

commented Mar 6, 2017

Hi @ydubreuil would you mind putting in a condition for \n so we can avoid regressions later on? Thanks for finding + fixing this!

@ydubreuil

This comment has been minimized.

Copy link
Author

commented Mar 8, 2017

Sorry, I don't understand what condition you want me to add.

It can't backtrack anymore because the regexp starts with ^ to start the matching at the beginning of the string

if (!adminlistPattern.matcher(value).matches()) {
return FormValidation.error("GitHub username may only contain alphanumeric characters or dashes "
+ "and cannot begin with a dash. Separate them with whitespaces.");
for (String admin : value.split("\\s")) {

This comment has been minimized.

Copy link
@benpatterson

benpatterson Mar 8, 2017

Member

This line is ultimately my concern. I believe some users will want to enter with newline characters rather than whitespace. We'll need to catch those in the form validation. So I'd want a test for that. Does that make sense?

This comment has been minimized.

Copy link
@ydubreuil

ydubreuil May 23, 2017

Author

Right, I updated the PR to split on any space character. That should do the trick.

Apologies for the very long delay here.

@ydubreuil

This comment has been minimized.

Copy link
Author

commented Jun 19, 2017

@benpatterson I think my last commit fixed the concern you have for multi-lines input

Any chance to get this merged?

@benpatterson

This comment has been minimized.

Copy link
Member

commented Jun 20, 2017

@ydubreuil I'm on board, but there are merge conflicts now. Could you fix it up?

@ydubreuil ydubreuil force-pushed the ydubreuil:fix-JENKINS-42148 branch from 5822c57 to b6a1296 Aug 4, 2017

@ydubreuil

This comment has been minimized.

Copy link
Author

commented Aug 4, 2017

@benpatterson I fixed conflicts after re-merging from master. It should be good now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.