-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Added functionality to allow for looking up IP addresses #23
base: master
Are you sure you want to change the base?
Conversation
…use NGINX maps. - Updated ipinfo.conf to remove old location blocks that have been moved to map.conf. Added functionality to allow for looking up IP addresses. - New map.conf that containing maps. - Updated nginx.conf to increase map_hash_bucket_size above default value. - Updated ipinfo.conf to include new location blocks to allow for looking up IP addresses. Updated README to include details on added functionality.
SG / Singapore | ||
AS14061 / DigitalOcean, LLC | ||
|
||
$ curl https://ipinfo.tw/lookup/18.179.200.1/ip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder what's the use case for this endpoint?
HK | ||
|
||
$ curl https://ipinfo.tw/lookup/18.179.200.1/country_name | ||
South Korea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use JP/Japan would be better for it?
AS16509 / Amazon.com, Inc. | ||
|
||
$ curl https://ipinfo.tw/lookup/18.179.200.1/as | ||
AS8075 / Microsoft Corporation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I doubt this will confuse the reader 😅 as the same IP returns totally different result.
add_header X-XSS-Protection "1; mode=block" always; | ||
add_header X-Content-Type-Options "nosniff" always; | ||
add_header Referrer-Policy "no-referrer-when-downgrade" always; | ||
add_header Content-Security-Policy "default-src 'none'; img-src 'self'" always; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to keep the same blank line style as the origin.
"~^\/lookup\/([^\/]+)\/json\/?$" application/json; | ||
|
||
default text/plain; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should keep the last newline char here.
@coderabbitai review |
WalkthroughThe update introduces new API endpoints for IP information retrieval in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ? TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- README.md (2 hunks)
- nginx/conf.d/ipinfo.conf (2 hunks)
- nginx/conf.d/map.conf (1 hunks)
- nginx/nginx.conf (1 hunks)
Additional comments: 5
nginx/conf.d/ipinfo.conf (2)
2-13: The addition of security-related headers such as
X-Frame-Options
,X-XSS-Protection
, andContent-Security-Policy
is a good practice to enhance the security of the server responses.50-56: The
default_type
directive is set correctly for JSON and plain text responses based on the context of the request. This ensures proper content negotiation.nginx/conf.d/map.conf (2)
- 1-9: The regex for IPv4 and IPv6 seems correct, but it's important to ensure that these patterns have been thoroughly tested, especially the complex IPv6 pattern, to avoid any false positives or negatives.
Verification successful
The executed script has successfully tested the IPv6 regex pattern against a sample IPv6 address (
fe80::1
), and the pattern has matched correctly. This indicates that the regex pattern for IPv6 in themap.conf
file is functioning as intended for the provided test case.* 12-36: The mapping of URIs to responses is well-defined. However, ensure that the `$remote_addr` and other variables used in the map are correctly populated and that the responses are as expected.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the correctness of IPv4 and IPv6 regex patterns. echo "Testing IPv4 regex..." echo "192.168.1.1" | grep -P "(([0-9]{1,3}\.){3}[0-9]{1,3})" echo "Testing IPv6 regex..." echo "fe80::1" | grep -P "(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|...)"Length of output: 260
Script:
#!/bin/bash # Verify the correctness of the full IPv6 regex pattern. echo "Testing full IPv6 regex..." echo "fe80::1" | grep -P "(([0-9a-fA-F]{1,4}:){7,7}[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,7}:|([0-9a-fA-F]{1,4}:){1,6}:[0-9a-fA-F]{1,4}|([0-9a-fA-F]{1,4}:){1,5}(:[0-9a-fA-F]{1,4}){1,2}|([0-9a-fA-F]{1,4}:){1,4}(:[0-9a-fA-F]{1,4}){1,3}|([0-9a-fA-F]{1,4}:){1,3}(:[0-9a-fA-F]{1,4}){1,4}|([0-9a-fA-F]{1,4}:){1,2}(:[0-9a-fA-F]{1,4}){1,5}|[0-9a-fA-F]{1,4}:((:[0-9a-fA-F]{1,4}){1,6})|:((:[0-9a-fA-F]{1,4}){1,7}|:)|fe80:(:[0-9a-fA-F]{0,4}){0,4}%[0-9a-zA-Z]{1,}|::(ffff(:0{1,4}){0,1}:){0,1}((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])|([0-9a-fA-F]{1,4}:){1,4}:((25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9])\.){3,3}(25[0-5]|(2[0-4]|1{0,1}[0-9]){0,1}[0-9]))"Length of output: 757
README.md (1)
- 123-150: The examples provided in the README are helpful for understanding how to use the new endpoints. Ensure that these examples are kept up-to-date with any changes to the API.
map_hash_max_size 2048; | ||
map_hash_bucket_size 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration for map_hash_max_size
and map_hash_bucket_size
seems appropriate. However, ensure that these values are optimized for the expected number of elements in the map and the server's memory capacity.
location ~* "/(.*)" { | ||
default_type ""; | ||
|
||
if ($request_uri ~* "/(.*)") { | ||
add_header Content-Type $ip_response_content_type; | ||
add_header X-Powered-By "ipinfo.tw/github" always; | ||
add_header Cache-Control "no-store" always; | ||
add_header X-Frame-Options "SAMEORIGIN" always; | ||
add_header X-XSS-Protection "1; mode=block" always; | ||
add_header X-Content-Type-Options "nosniff" always; | ||
add_header Referrer-Policy "no-referrer-when-downgrade" always; | ||
add_header Content-Security-Policy "default-src 'none'; img-src 'self'" always; | ||
} | ||
|
||
return 200 $ip_response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regex pattern in the location
block is overly broad and could potentially match unintended URIs. Consider refining the regex to match specific paths more accurately.
- location ~* "/(.*)" {
+ location ~* "^/(ip|country|country_code|country_name|as|asn|as_desc|user_agent|json)/?$" {
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
location ~* "/(.*)" { | |
default_type ""; | |
if ($request_uri ~* "/(.*)") { | |
add_header Content-Type $ip_response_content_type; | |
add_header X-Powered-By "ipinfo.tw/github" always; | |
add_header Cache-Control "no-store" always; | |
add_header X-Frame-Options "SAMEORIGIN" always; | |
add_header X-XSS-Protection "1; mode=block" always; | |
add_header X-Content-Type-Options "nosniff" always; | |
add_header Referrer-Policy "no-referrer-when-downgrade" always; | |
add_header Content-Security-Policy "default-src 'none'; img-src 'self'" always; | |
} | |
return 200 $ip_response; | |
location ~* "^/(ip|country|country_code|country_name|as|asn|as_desc|user_agent|json)/?$" { | |
default_type ""; | |
if ($request_uri ~* "/(.*)") { | |
add_header Content-Type $ip_response_content_type; | |
add_header X-Powered-By "ipinfo.tw/github" always; | |
add_header Cache-Control "no-store" always; | |
add_header X-Frame-Options "SAMEORIGIN" always; | |
add_header X-XSS-Protection "1; mode=block" always; | |
add_header X-Content-Type-Options "nosniff" always; | |
add_header Referrer-Policy "no-referrer-when-downgrade" always; | |
add_header Content-Security-Policy "default-src 'none'; img-src 'self'" always; | |
} | |
return 200 $ip_response; |
You can also specify the following URI to retrieve certain info about an IP. Replace "{ip_address}" with the IPv4 or IPv6 you want to lookup. | ||
- `lookup/{ip_address}/ip`: IP address | ||
- `lookup/{ip_address}/country`: Country code and name | ||
- `lookup/{ip_address}/country_code`: Country code | ||
- `lookup/{ip_address}/country_name`: Country name | ||
- `lookup/{ip_address}/as`: AS number and description | ||
- `lookup/{ip_address}/asn`: AS number | ||
- `lookup/{ip_address}/as_desc`: AS description | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 NOTE
Codebase verification is a beta feature.
The search for location
blocks within the ipinfo.conf
file has returned several entries, but none of them match the new URI endpoints described in the PR objectives and the AI-generated summaries, such as lookup/{ip_address}/ip
or lookup/{ip_address}/country
. This suggests that the expected new location blocks for IP address lookup may not have been added to the ipinfo.conf
file.
Based on the information gathered:
- The
nginx.conf
file has been correctly updated with an increasedmap_hash_bucket_size
. - The
map.conf
file contains mappings that are likely related to the new IP lookup functionality. - The
ipinfo.conf
file does not show evidence of the new location blocks for IP address lookup as described in the PR objectives.
Given these findings, it appears that there may be an issue with the implementation of the new URI endpoints in the ipinfo.conf
file.
- The
ipinfo.conf
file does not contain the new location blocks for IP address lookup as expected from the PR description.
Analysis chain
The documentation for the new URI endpoints is clear and provides a good understanding of the new features. It's important that the examples given are tested to ensure they work as documented.
The verification of the nginx.conf
file confirms that the map_hash_bucket_size
has been set to 1024, which aligns with the PR objective to increase this value above the default.
The map.conf
file contains mappings, as indicated by the presence of map
directives with variables such as $lookup_ip
, $ip_response
, and $ip_response_content_type
. This suggests that the file has been set up to handle IP address mappings, which is also in line with the PR objectives.
However, the attempt to verify the new location blocks in the ipinfo.conf
file did not yield any results. This could mean that the expected changes are not present, or there was an issue with the search command.
Given the lack of output for the ipinfo.conf
file, I will need to perform another search to confirm the presence of the new location blocks that should enable the lookup of IP addresses.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify that the examples in the README work as expected.
curl -s https://ipinfo.tw/lookup/8.8.8.8/ip
Length of output: 109
Script:
#!/bin/bash
# Search for the existence of map.conf, nginx.conf, and ipinfo.conf files in the repository
fd 'map\.conf'
fd 'nginx\.conf'
fd 'ipinfo\.conf'
Length of output: 119
Script:
#!/bin/bash
# Verify the specific changes in nginx.conf, map.conf, and ipinfo.conf
# Check for the increased map_hash_bucket_size in nginx.conf
rg 'map_hash_bucket_size' nginx/nginx.conf
# Look for mappings in map.conf
rg 'map\s+\$' nginx/conf.d/map.conf
# Confirm new location blocks in ipinfo.conf
rg 'location\s+/' nginx/conf.d/ipinfo.conf
Length of output: 254
Script:
#!/bin/bash
# Perform a more detailed search for location blocks in ipinfo.conf
rg 'location' nginx/conf.d/ipinfo.conf
Length of output: 233
I really need this. Was the request merged? I'm looking for a self hosted API that I can provide an IP address and it can provide back this kind of info. |
Hi @benisai I haven't had a chance to get back to reviewing the comments by @PeterDaveHello and making changes. So I don't believe it's been merged. You could always check out my fork if you need it. |
I will indeed. I see you have a docker container as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 1 out of 4 changed files in this pull request and generated no suggestions.
Files not reviewed (3)
- nginx/conf.d/ipinfo.conf: Language not supported
- nginx/conf.d/map.conf: Language not supported
- nginx/nginx.conf: Language not supported
Comments skipped due to low confidence (1)
README.md:134
- The example responses for the IP address 18.179.200.1 are inconsistent. The country code and country name should match.
$ curl https://ipinfo.tw/lookup/18.179.200.1/country_code
Moved existing functionality for looking up requesting IP address to use NGINX maps.
Added functionality to allow for looking up IP addresses.
Updated README to include details on added functionality.
Summary by CodeRabbit
New Features
Documentation
Refactor
Style
Chores
map_hash_max_size
andmap_hash_bucket_size
directives.