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

Hlepers::ipMatch() fixed warning when passed invalid IP address #122

Merged
merged 1 commit into from Mar 29, 2017

Conversation

@hrach
Copy link
Contributor

hrach commented Mar 27, 2017

  • bug fix
  • no BC break

This fixes a warning which happened on our production servers. Generally, ipMatch() function should be able to handle invalid input without a warning.

The cause was an invalid header
HTTP_X_FORWARDED_FOR: "192.168.5.237,mcd14281.lax,mcd14281.lax, 64.134.222.215, 185.152.66.10, 100.97.10.64"
which was parsed by RequestFactory and validated against known/allowed proxies.

@hrach hrach force-pushed the hrach:ip_match_warning_fix branch from e5b8f4b to 5e57fdd Mar 27, 2017
@dg

This comment has been minimized.

Copy link
Member

dg commented Mar 27, 2017

Related to #67

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Mar 27, 2017

Since ipMatch is a validation function, it should handle invalid input. Personally, I don't really mind if it is by @ or if, but I really think, the fix should be here, not in the request factory.

Also, I've implemented the fix for the variable which is expected to be validated, not the validating "pattern".

@dg

This comment has been minimized.

Copy link
Member

dg commented Mar 27, 2017

It is not validation function, it is just detecting function. Why do you think it is validation function?

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Mar 27, 2017

It validates the first argument against the second. Why do you think it is a detection function? What does is detect?

@dg

This comment has been minimized.

Copy link
Member

dg commented Mar 27, 2017

It detects (or determines) whether the IP address matches the mask. It expects IP address and mask.

@hrach hrach force-pushed the hrach:ip_match_warning_fix branch from 5e57fdd to f1dab72 Mar 27, 2017
@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Mar 27, 2017

I have repushed the implementation.

Though, it is unpleasant to contribute. Well, we (I) can feel that you probably don't agree with the way of fixing this, but please, write it down. What do think? What are you going to do? E.g "note to myself, #67, will review later" - nothing hard, but enlights the situation a lot.

@dg

This comment has been minimized.

Copy link
Member

dg commented Mar 27, 2017

Simply I think that it is correct that function trigger warning on invalid input and I am talking with you to find out why you have a different opinion. It is rather up to you to explain that this is a bug when it's not obvious.

This new solution seems OK to me. Just I do not know if it's a BC break or not, due to dependency on extension filter.

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Mar 27, 2017

It is rather up to you to explain that this is a bug when it's not obvious.

That's something that didn't cross my mind, I had no idea that this bug report is not obvious.

Just I do not know if it's a BC break or not, due to dependency on extension filter.

filter_var seems to be core function, isn't it?

@dg dg merged commit 9c58019 into nette:master Mar 29, 2017
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 81.194%
Details
@hrach hrach deleted the hrach:ip_match_warning_fix branch Mar 29, 2017
dg added a commit that referenced this pull request Mar 29, 2017
@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Mar 29, 2017

Thanks!

1 similar comment
@dg

This comment has been minimized.

Copy link
Member

dg commented Mar 29, 2017

Thanks!

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