feat: Add 8G firewall support #12
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure WalkthroughAdds the 8G firewall: a new nginx maps file that flags suspicious request parts with numeric codes, an enforcement snippet that appends reason codes and returns 403/405, a deployment script to fetch/update these files (with backups), and a .gitignore entry to ignore backup files. (39 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Nginx
participant Maps as "8G Firewall Maps"
participant Enforcer as "8G Enforcement"
Client->>Nginx: HTTP request
Nginx->>Maps: Evaluate uri, query, ua, referer, cookie, method
Maps-->>Nginx: Set numeric flags (e.g. $bad_querystring_ng)
Nginx->>Enforcer: Inspect flags, append to $ng_reason
alt any flag != 0
Enforcer-->>Client: Return 403 or 405 (with $ng_reason)
else
Nginx-->>Client: Continue normal processing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
nginx/common/8g-firewall.conf (1)
57-119: Consider adding configuration options for rule customization.The URI validation rules are comprehensive but may cause false positives for legitimate use cases. Consider implementing a mechanism to selectively enable/disable specific rule categories or adjust sensitivity levels.
For example, you could:
- Split rules into severity levels (critical, high, medium, low)
- Add nginx variables to control which rule sets are active
- Implement allowlisting for specific legitimate patterns
This would provide more flexibility for different deployment scenarios while maintaining security.
nginx/common/8g.conf (1)
5-38: Consider implementing additional security features.To enhance the 8G Firewall implementation, consider adding:
- IP allowlisting: Skip checks for trusted sources
- Rate limiting: Prevent brute force attempts
- Geo-blocking: Optional country-based restrictions
- Custom error pages: Avoid information disclosure
Example IP allowlist implementation:
geo $whitelist { default 0; 10.0.0.0/8 1; # Internal network 192.168.0.0/16 1; # Local network } # Skip firewall checks for whitelisted IPs if ($whitelist) { set $skip_firewall 1; }Would you like me to create an issue for implementing these enhancements?
| map default $ng_reason { | ||
| default ''; | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the map directive syntax for $ng_reason initialization.
The map directive appears to have incorrect syntax. The first parameter should be a variable to match against, not the literal string "default". This configuration might not work as intended.
🏁 Script executed:
#!/bin/bash
# Description: Check if this map syntax is valid and search for similar patterns in nginx configs
# Search for similar map directive patterns
rg "map\s+default\s+" -A 3
# Search for proper map directive examples
rg "map\s+\$\w+\s+\$ng_reason" -A 3
# Look for nginx configuration examples with reason variables
fd -e conf | xargs rg -l "ng_reason" | head -10 | xargs catLength of output: 12309
🌐 Web query:
nginx map directive syntax “map default” first argument literal string
💡 Result:
The map directive in NGINX is used to create variables whose values depend on the value of another variable or string. The syntax generally looks like this:
map $variable $result {
default value;
key1 value1;
key2 value2;
...
}
- The first argument is the source variable or string to be evaluated (e.g.,
$http_host,$request_uri). - The second argument is the name of the new variable to be assigned the mapped value.
When you say "map default" first argument literal string, I believe you want to know how to use a literal string as the first argument in the map directive or how to use default in the map.
Example of map usage:
map $http_user_agent $mobile {
default 0;
"~*iphone" 1;
"~*android" 1;
}
Here:
$http_user_agentis the first argument (a variable),$mobileis the second argument (the new variable),defaultis a special key inside the map block meaning "any other unmatched value".
If you want to use a literal string (like "map default") as the first argument, it's usually not done directly because the first argument is supposed to be a variable or something to evaluate.
Could you clarify your use case or give me an example configuration snippet? That way, I can provide a more precise explanation or correction.
Invalid map directive—first argument must be a variable
The snippet in nginx/common/8g-firewall.conf (lines 5–8):
map default $ng_reason {
default '';
}uses the literal “default” as the source, which isn’t valid. The first argument to map must be an existing variable. To initialize $ng_reason to an empty string, please pick one of these approaches:
-
Replace the block with a simple
setdirective:- map default $ng_reason { - default ''; - } + set $ng_reason '';
-
Or change to a proper
mapusing a real variable (for example,$request_uri):- map default $ng_reason { + map $request_uri $ng_reason { default ''; }
Either will correctly initialize $ng_reason.
🤖 Prompt for AI Agents
In nginx/common/8g-firewall.conf around lines 5 to 8, the map directive
incorrectly uses the literal "default" as the source variable, which is invalid.
To fix this, either replace the entire map block with a set directive to
initialize $ng_reason to an empty string, or modify the map to use a valid
existing variable like $request_uri as the source. Choose one approach to
properly initialize $ng_reason without syntax errors.
| map $http_user_agent $bad_bot_ng { | ||
|
|
||
| default 0; | ||
|
|
||
| "~*^[a-z0-9]$" 1; | ||
| "~*<|%0a|%0d|%27|%3c|%3e|%00|0x00|\\\x22" 2; | ||
| "~*ahrefs|archiver|curl|libwww-perl|pycurl|scan" 3; | ||
| "~*oppo\sa33|(?:c99|php|web)shell|site.{0,2}copier" 4; | ||
| "~*base64_decode|bin/bash|disconnect|eval|unserializ" 5; | ||
| "~*acapbot|acoonbot|alexibot|asterias|attackbot|awario|backdor|becomebot|binlar|blackwidow|blekkobot|blex|blowfish|bullseye|bunnys|butterfly|careerbot|casper|censysinspect" 6; | ||
| "~*checkpriv|cheesebot|cherrypick|chinaclaw|choppy|claudebot|clshttp|cmsworld|copernic|copyrightcheck|cosmos|crawlergo|crescent|datacha|\bdemon\b|diavol|discobot|dittospyder" 7; | ||
| "~*dotbot|dotnetdotcom|dumbot|econtext|emailcollector|emailsiphon|emailwolf|eolasbot|eventures|extract|eyenetie|feedfinder|flaming|flashget|flicky|foobot|fuck" 8; | ||
| "~*g00g1e|getright|gigabot|go-ahead-got|gozilla|grabnet|grafula|harvest|heritrix|httracks?|icarus6j|imagesiftbot|jetbot|jetcar|jikespider|kmccrew|leechftp|libweb|liebaofast" 9; | ||
| "~*linkscan|linkwalker|lwp-download|majestic|masscan|mauibot|miner|mechanize|mj12bot|morfeus|moveoverbot|mozlila|nbot|netmechanic|netspider|nicerspro|nikto|ninja|nominet|nutch" 10; | ||
| "~*octopus|pagegrabber|planetwork|postrank|proximic|purebot|queryn|queryseeker|radian6|radiation|realdownload|remoteview|rogerbot|scan|scooter|seekerspid" 11; | ||
| "~*semalt|siclab|sindice|sistrix|sitebot|siteexplorer|sitesnagger|skygrid|smartdownload|snoopy|sosospider|spankbot|spbot|sqlmap|stackrambler|stripper|sucker|surftbot" 12; | ||
| "~*sux0r|suzukacz|suzuran|takeout|teleport|telesoft|true_robots|turingos|turnit|vampire|vikspider|voideye|webleacher|webreaper|webstripper|webvac|webviewer|webwhacker" 13; | ||
| "~*winhttp|wwwoffle|woxbot|xaldon|xxxyy|yamanalab|yioopbot|youda|zeus|zmeu|zune|zyborg" 14; | ||
|
|
||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Review the blocking of legitimate tools in user agent patterns.
Line 127 blocks common tools like curl, libwww-perl, and pycurl which are often used for legitimate purposes such as monitoring, API integrations, and automated testing.
Consider:
- Moving these to a separate category that can be optionally enabled
- Implementing an allowlist for known good sources
- Using rate limiting instead of outright blocking for these tools
- "~*ahrefs|archiver|curl|libwww-perl|pycurl|scan" 3;
+ "~*ahrefs|archiver|scan" 3;
+ # Consider separate handling for development tools
+ # "~*curl|libwww-perl|pycurl" 100;🏁 Script executed:
#!/bin/bash
# Locate the firewall config file and print lines 115–150 for context
file=$(fd 8g-firewall.conf 2>/dev/null | head -n1)
if [[ -n "$file" ]]; then
sed -n '115,150p' "$file"
else
echo "File 8g-firewall.conf not found"
fiLength of output: 2932
Reevaluate blocking of common HTTP clients in the bot map
The bad_bot_ng map in nginx/common/8g-firewall.conf (around line 127) currently blacklists widely-used tools (curl, libwww-perl, pycurl), which many CI checks, monitoring systems or API integrations depend on. Outright rejecting these UAs risks interfering with legitimate traffic.
Consider:
- Removing
curl|libwww-perl|pycurlfrom this pattern and placing them in a separate, opt-in rule - Implementing a host-based allowlist for known good origins
- Using rate limiting (
limit_req) instead of returning 444 for these clients
Suggested diff:
- "~*ahrefs|archiver|curl|libwww-perl|pycurl|scan" 3;
+ "~*ahrefs|archiver|scan" 3;
+# Optional: separate rule for CLI clients with lower priority
+# "~*curl|libwww-perl|pycurl" 100;📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| map $http_user_agent $bad_bot_ng { | |
| default 0; | |
| "~*^[a-z0-9]$" 1; | |
| "~*<|%0a|%0d|%27|%3c|%3e|%00|0x00|\\\x22" 2; | |
| "~*ahrefs|archiver|curl|libwww-perl|pycurl|scan" 3; | |
| "~*oppo\sa33|(?:c99|php|web)shell|site.{0,2}copier" 4; | |
| "~*base64_decode|bin/bash|disconnect|eval|unserializ" 5; | |
| "~*acapbot|acoonbot|alexibot|asterias|attackbot|awario|backdor|becomebot|binlar|blackwidow|blekkobot|blex|blowfish|bullseye|bunnys|butterfly|careerbot|casper|censysinspect" 6; | |
| "~*checkpriv|cheesebot|cherrypick|chinaclaw|choppy|claudebot|clshttp|cmsworld|copernic|copyrightcheck|cosmos|crawlergo|crescent|datacha|\bdemon\b|diavol|discobot|dittospyder" 7; | |
| "~*dotbot|dotnetdotcom|dumbot|econtext|emailcollector|emailsiphon|emailwolf|eolasbot|eventures|extract|eyenetie|feedfinder|flaming|flashget|flicky|foobot|fuck" 8; | |
| "~*g00g1e|getright|gigabot|go-ahead-got|gozilla|grabnet|grafula|harvest|heritrix|httracks?|icarus6j|imagesiftbot|jetbot|jetcar|jikespider|kmccrew|leechftp|libweb|liebaofast" 9; | |
| "~*linkscan|linkwalker|lwp-download|majestic|masscan|mauibot|miner|mechanize|mj12bot|morfeus|moveoverbot|mozlila|nbot|netmechanic|netspider|nicerspro|nikto|ninja|nominet|nutch" 10; | |
| "~*octopus|pagegrabber|planetwork|postrank|proximic|purebot|queryn|queryseeker|radian6|radiation|realdownload|remoteview|rogerbot|scan|scooter|seekerspid" 11; | |
| "~*semalt|siclab|sindice|sistrix|sitebot|siteexplorer|sitesnagger|skygrid|smartdownload|snoopy|sosospider|spankbot|spbot|sqlmap|stackrambler|stripper|sucker|surftbot" 12; | |
| "~*sux0r|suzukacz|suzuran|takeout|teleport|telesoft|true_robots|turingos|turnit|vampire|vikspider|voideye|webleacher|webreaper|webstripper|webvac|webviewer|webwhacker" 13; | |
| "~*winhttp|wwwoffle|woxbot|xaldon|xxxyy|yamanalab|yioopbot|youda|zeus|zmeu|zune|zyborg" 14; | |
| } | |
| map $http_user_agent $bad_bot_ng { | |
| default 0; | |
| "~*^[a-z0-9]$" 1; | |
| "~*<|%0a|%0d|%27|%3c|%3e|%00|0x00|\\\x22" 2; | |
| - "~*ahrefs|archiver|curl|libwww-perl|pycurl|scan" 3; | |
| + "~*ahrefs|archiver|scan" 3; | |
| + # Optional: separate rule for CLI clients with lower priority | |
| + # "~*curl|libwww-perl|pycurl" 100; | |
| "~*oppo\sa33|(?:c99|php|web)shell|site.{0,2}copier" 4; | |
| "~*base64_decode|bin/bash|disconnect|eval|unserializ" 5; | |
| "~*acapbot|acoonbot|alexibot|asterias|attackbot|awario|backdor|becomebot|binlar|blackwidow|blekkobot|blex|blowfish|bullseye|bunnys|butterfly|careerbot|casper|censysinspect" 6; | |
| "~*checkpriv|cheesebot|cherrypick|chinaclaw|choppy|claudebot|clshttp|cmsworld|copernic|copyrightcheck|cosmos|crawlergo|crescent|datacha|\bdemon\b|diavol|discobot|dittospyder" 7; | |
| "~*dotbot|dotnetdotcom|dumbot|econtext|emailcollector|emailsiphon|emailwolf|eolasbot|eventures|extract|eyenetie|feedfinder|flaming|flashget|flicky|foobot|fuck" 8; | |
| "~*g00g1e|getright|gigabot|go-ahead-got|gozilla|grabnet|grafula|harvest|heritrix|httracks?|icarus6j|imagesiftbot|jetbot|jetcar|jikespider|kmccrew|leechftp|libweb|liebaofast" 9; | |
| "~*linkscan|linkwalker|lwp-download|majestic|masscan|mauibot|miner|mechanize|mj12bot|morfeus|moveoverbot|mozlila|nbot|netmechanic|netspider|nicerspro|nikto|ninja|nominet|nutch" 10; | |
| "~*octopus|pagegrabber|planetwork|postrank|proximic|purebot|queryn|queryseeker|radian6|radiation|realdownload|remoteview|rogerbot|scan|scooter|seekerspid" 11; | |
| "~*semalt|siclab|sindice|sistrix|sitebot|siteexplorer|sitesnagger|skygrid|smartdownload|snoopy|sosospider|spankbot|spbot|sqlmap|stackrambler|stripper|sucker|surftbot" 12; | |
| "~*sux0r|suzukacz|suzuran|takeout|teleport|telesoft|true_robots|turingos|turnit|vampire|vikspider|voideye|webleacher|webreaper|webstripper|webvac|webviewer|webwhacker" 13; | |
| "~*winhttp|wwwoffle|woxbot|xaldon|xxxyy|yamanalab|yioopbot|youda|zeus|zmeu|zune|zyborg" 14; | |
| } |
🤖 Prompt for AI Agents
In nginx/common/8g-firewall.conf around lines 121 to 140, the bad_bot_ng map
currently blocks common HTTP clients like curl, libwww-perl, and pycurl, which
can disrupt legitimate CI, monitoring, or API traffic. To fix this, remove these
user agents from the existing blacklist pattern and create a separate, opt-in
rule for them. Additionally, implement a host-based allowlist for trusted
origins and replace the outright blocking with rate limiting using limit_req for
these clients to reduce false positives and improve traffic handling.
nginx/common/8g.conf
Outdated
| if ($not_allowed_host_ng) { | ||
| set $ng_reason "${ng_reason}:not_allowed_host_${not_allowed_host_ng}:"; | ||
| return 403; | ||
| } | ||
|
|
||
| if ($bad_cookie_ng) { | ||
| set $ng_reason "${ng_reason}:bad_cookie_${bad_cookie_ng}:"; | ||
| return 403; | ||
| } | ||
|
|
||
| if ($not_allowed_method_ng) { | ||
| set $ng_reason "${ng_reason}:not_allowed_method_${not_allowed_method_ng}:"; | ||
| return 405; | ||
| } | ||
|
|
||
| if ($bad_referer_ng) { | ||
| set $ng_reason "${ng_reason}:bad_referer_${bad_referer_ng}:"; | ||
| return 403; | ||
| } | ||
|
|
||
| if ($bad_bot_ng) { | ||
| set $ng_reason "${ng_reason}:bad_bot_${bad_bot_ng}:"; | ||
| return 403; | ||
| } | ||
|
|
||
| if ($bad_querystring_ng) { | ||
| set $ng_reason "${ng_reason}:bad_querystring_${bad_querystring_ng}:"; | ||
| return 403; | ||
| } | ||
|
|
||
| if ($bad_request_ng) { | ||
| set $ng_reason "${ng_reason}:bad_request_${bad_request_ng}:"; | ||
| return 403; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement logging and optimize the enforcement logic.
The current implementation has several issues:
- The
$ng_reasonvariable is constructed but never logged, providing no visibility into blocked requests - Multiple
ifstatements in nginx can impact performance (see If Is Evil) - No access logging for security events
Add logging configuration:
+# Log blocked requests with reasons
+location @blocked {
+ access_log /var/log/nginx/8g_blocked.log combined_plus_reason;
+ return 403;
+}
+
if ($not_allowed_host_ng) {
set $ng_reason "${ng_reason}:not_allowed_host_${not_allowed_host_ng}:";
- return 403;
+ error_page 403 = @blocked;
+ return 403;
}Also define a custom log format in the http context:
log_format combined_plus_reason '$remote_addr - $remote_user [$time_local] '
'"$request" $status $body_bytes_sent '
'"$http_referer" "$http_user_agent" '
'reason="$ng_reason"';This provides audit trails for security analysis and debugging false positives.
🤖 Prompt for AI Agents
In nginx/common/8g.conf around lines 5 to 38, the current code sets the
$ng_reason variable on blocked requests but does not log it, and uses multiple
if statements which can degrade performance. To fix this, define a custom
log_format in the http context that includes the $ng_reason variable for
detailed logging of blocked requests. Then, replace the multiple if statements
with a more efficient approach such as using a single if block or map directive
to set $ng_reason and return the appropriate status, ensuring that all blocked
requests are logged with the custom format for better auditability and
debugging.
|
Config from here https://github.com/t18d/nG-SetEnvIf This repo has been referred by perishablepress. |
|
In In current setup, add to |
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
nginx/common/8g-firewall.conf (2)
8-10: Past review comment still applies: Invalid map directive syntax.The issue flagged in the previous review remains unresolved. The
map default $ng_reasondirective uses an invalid literal string as the first argument instead of a variable.
130-130: Past review comment still applies: Blocking legitimate HTTP clients.The previous concern about blocking
curl,libwww-perl, andpycurlremains unaddressed, which may disrupt CI/CD pipelines, monitoring systems, and API integrations.
🧹 Nitpick comments (1)
scripts/update-8g-firewall.sh (1)
113-113: Useprintforecho -efor portable newline handling.Plain
echodoes not interpret\nin all shells, so the summary line will display a literal\ninstead of a line break.Apply this diff:
-echo "----\nSummary: added=$summary_added updated=$summary_updated skipped=$summary_skipped failed=$summary_failed" +printf -- "----\nSummary: added=%d updated=%d skipped=%d failed=%d\n" \ + "$summary_added" "$summary_updated" "$summary_skipped" "$summary_failed"Or simply use two separate echo calls:
-echo "----\nSummary: added=$summary_added updated=$summary_updated skipped=$summary_skipped failed=$summary_failed" +echo "----" +echo "Summary: added=$summary_added updated=$summary_updated skipped=$summary_skipped failed=$summary_failed"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.gitignore(1 hunks)nginx/common/8g-firewall.conf(1 hunks)nginx/common/8g.conf(1 hunks)scripts/update-8g-firewall.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- nginx/common/8g.conf
2 participants
Summary
New Features
Chores
Fixes: https://github.com/flywp/flywp-app/issues/636
Summary by CodeRabbit
New Features
Configuration