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

WW-4834 fixed faulty regex #157

Merged
merged 3 commits into from
Aug 7, 2017
Merged

WW-4834 fixed faulty regex #157

merged 3 commits into from
Aug 7, 2017

Conversation

sdutry
Copy link
Member

@sdutry sdutry commented Aug 2, 2017

No description provided.

@lukaszlenart
Copy link
Member

Could you wait a bit with merging this PR? I have asked somebody to take a look on this.

@sdutry
Copy link
Member Author

sdutry commented Aug 2, 2017

Ok,

I will wait.

@atcazzual
Copy link

atcazzual commented Aug 3, 2017

The expression did not seem to work at all until I escaped the slashes, changing / to \/

Once I got it working, there then seems to be a bug in the new expression when matching on URLs that use IP addresses. The grouping has changed causing two problems with matching IP addresses.

  1. The dot . character that delimits the octets in an IP address only applies to the last condition, 25[0-5]\. on line 57, instead of all conditions for an IP octet. This makes matching most IP addresses fail. The only IPs that will match would need to have 3-digit octets for the first three where the first two digits are 25. NOTE: This seems to have been resolved by the commit above.
  2. The conditions for the last octet are no longer grouped (line 58) making the OR | operator act on what was a higher level group. Because of this, the fourth octet would have to be only one or two digits.

For example, only IPs like the following will pass validation:
http://253.254.255.1 (mostly resolved by the commit above)
http://253.254.255.12 (mostly resolved by the commit above)

After the commit above, any IP with 3 digits in the last octet will not pass validation:
http://1.2.3.100
http://1.2.3.255

@sdutry
Copy link
Member Author

sdutry commented Aug 3, 2017

After the commit above, any IP with 3 digits in the last octet will not pass validation

You are right, i forgot the grouping (meaning the or statements mean something completely different), i'll fix this ASAP

The expression did not seem to work at all until I escaped the slashes, changing / to \/

I don't see why this would be. I am not aware that a "/" is a special sign inside a java string or java RegEx. Or is it because this same regex is used in javascript regex validation (where the '/' sign does mean something). I'll change it back ASAP too

@lukaszlenart
Copy link
Member

I think we are good here, thanks a lot for yours work :)

@sdutry
Copy link
Member Author

sdutry commented Aug 4, 2017

@lukaszlenart
Sorry for breaking it in the first place. That wasn't my intention.

Do you want me to merge this now, or am i still overlooking stuff?

@lukaszlenart
Copy link
Member

I would wait a bit and give Adam a chance to post a comment (if he wants to). No rush :)

@atcazzual
Copy link

Looks good to me. All matching capabilities from prior to the optimizations seem to be there.

@asfgit asfgit merged commit 1382805 into apache:master Aug 7, 2017
@sdutry sdutry deleted the WW-4834 branch August 7, 2017 09:14
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