add java doc warning comment into ipAddressMatcher for potential DNS resolution #13621
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
spring-projects-issues
added
the
status: waiting-for-triage
jzheaux
added
in: web
jzheaux
requested changes
Oct 12, 2023
There was a problem hiding this comment.
I don't think the new JavaDoc adds anything that wasn't already said earlier. The existing JavaDoc says:
* Takes a specific IP address or a range specified using the IP/Netmask (e.g.
* 192.168.1.0/24 or 202.24.0.0/14).so the correct format is already indicated.
That said, I like your second idea to validate the parameters. Will you please change the code to validate that the first character is [, :, or a digit? Something like this:
Assert.isTrue(ipAddress.charAt(0) == `[` ||
ipAddress.charAt(0) == ':' ||
Character.digit(ipAddress.charAt(0), 16), "ipAddress must start with a [, :, or a hexadecimal digit");This is needed in the constructor as well as the matches(String) method when preparing the address for comparison.
|
Hi, @maoling. Are you able to make the requested changes? |
jzheaux
added
the
status: ideal-for-contribution
FdHerrera
pushed a commit
to FdHerrera/spring-security
that referenced
this pull request
Jan 25, 2024
|
Hi guys, hope you're doing well Since no updates has been made in this pull request, I went ahead and created this PR to address the changes suggested. Let me know if its ok for you |
jzheaux
removed
the
status: ideal-for-contribution
|
Closing in favor of #14491 |
FdHerrera
pushed a commit
to FdHerrera/spring-security
that referenced
this pull request
Jan 30, 2024
jzheaux
pushed a commit
that referenced
this pull request
Jan 31, 2024
sara9674
referenced
this pull request
Aug 18, 2024
I went through everything to get it to fit with Spring's docuemntation standard. Lots of small changes for punctuation, grammar, usage, voice, and so on. Also added some links, mostly to the API Javadoc.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In our use case, our api gateway used
ipAddressMatcherutil to check whether a request's ip is in the ip-black list, but sometimes, theremoteAddressorX-Forwarded-Foris in a bad format(maybe malicious requests) as following. They're not Ip, but hostname. ipAddressMatcher calls theInetAddress#getByName which causes a DNS resolution, then the cloud vm alarms with this behavior.This PR just adds java doc warning comment, If your guys prefer the implementation by removing using the
InetAddress#getByNameand check ipv4/ipv6 inside, I will create another PR to rewrite it.