feat: Add HTTP proxy support and shared http_utils module#32
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces HTTP proxy support for all outbound HTTP requests by creating a centralized http_utils module. The changes consolidate duplicated proxy code (~140 lines) from captcha_providers.lua and enable proxy support for webhooks and IP reputation checks. Additionally, it fixes admin_auth.lua to properly use the WAF_SESSION_TTL environment variable instead of a hardcoded value.
Key changes:
- New
http_utils.luamodule implementing HTTP/HTTPS proxy support with CONNECT tunneling and NO_PROXY bypass rules - Refactored
captcha_providers.lua,webhooks.lua, andip_reputation.luato use the sharedhttp_utilsmodule - Fixed
admin_auth.luato read session TTL from environment variable
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| openresty/lua/http_utils.lua | New shared module providing HTTP request functionality with comprehensive proxy support (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) |
| openresty/lua/webhooks.lua | Migrated to use http_utils.request() for webhook delivery with proxy support |
| openresty/lua/ip_reputation.lua | Updated AbuseIPDB and webhook reputation checks to use http_utils with proxy support |
| openresty/lua/captcha_providers.lua | Removed ~140 lines of duplicated proxy code, now uses http_utils.post_form() |
| openresty/lua/admin_auth.lua | Fixed to use WAF_SESSION_TTL environment variable instead of hardcoded 86400 seconds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
openresty/lua/http_utils.lua
Outdated
| ngx.log(ngx.ERR, "CONNECT tunnel failed: ", err or (res and res.status)) | ||
| return nil, "Proxy CONNECT failed" |
There was a problem hiding this comment.
Inconsistent error handling after CONNECT: If the CONNECT request returns a non-200 status but res is not nil, the error message construction "err or (res and res.status)" may not produce a meaningful error message. When err is nil and res.status exists, concatenating it in the error message would be more informative. Consider changing the log message to explicitly show the status code, e.g., "CONNECT tunnel failed with status: " .. tostring(res.status).
| ngx.log(ngx.ERR, "CONNECT tunnel failed: ", err or (res and res.status)) | |
| return nil, "Proxy CONNECT failed" | |
| local status_text = res and ("status: " .. tostring(res.status)) or "no response from proxy" | |
| if err then | |
| ngx.log(ngx.ERR, "CONNECT tunnel failed (", status_text, "): ", err) | |
| else | |
| ngx.log(ngx.ERR, "CONNECT tunnel failed with ", status_text) | |
| end | |
| return nil, err or ("Proxy CONNECT failed with " .. status_text) |
openresty/lua/http_utils.lua
Outdated
| local proxy_host, proxy_port = proxy_url:match("https?://([^/:]+):?(%d*)") | ||
| if not proxy_host then | ||
| ngx.log(ngx.WARN, "Invalid proxy URL: ", proxy_url, ", falling back to direct connection") | ||
| use_proxy = false | ||
| else | ||
| proxy_port = tonumber(proxy_port) or 80 |
There was a problem hiding this comment.
The proxy URL parsing regex may not correctly handle all valid HTTP proxy URL formats. The pattern "https?://([^/:]+):?(%d*)" will not capture ports that are part of the domain itself when no explicit port is specified. For example, if proxy_port is an empty string after the match, it defaults to 80, but HTTPS proxies typically use port 443. Consider using a default port of 443 for HTTPS proxy URLs or requiring explicit port specification in the proxy URL configuration.
| local proxy_host, proxy_port = proxy_url:match("https?://([^/:]+):?(%d*)") | |
| if not proxy_host then | |
| ngx.log(ngx.WARN, "Invalid proxy URL: ", proxy_url, ", falling back to direct connection") | |
| use_proxy = false | |
| else | |
| proxy_port = tonumber(proxy_port) or 80 | |
| local proxy_scheme, proxy_host, proxy_port = proxy_url:match("^(https?)://([^/:]+):?(%d*)") | |
| if not proxy_host then | |
| ngx.log(ngx.WARN, "Invalid proxy URL: ", proxy_url, ", falling back to direct connection") | |
| use_proxy = false | |
| else | |
| if proxy_port ~= nil and proxy_port ~= "" then | |
| proxy_port = tonumber(proxy_port) | |
| else | |
| if proxy_scheme == "https" then | |
| proxy_port = 443 | |
| else | |
| proxy_port = 80 | |
| end | |
| end |
| local ok, conn_err = httpc:connect(proxy_host, proxy_port) | ||
| if not ok then | ||
| ngx.log(ngx.ERR, "Failed to connect to proxy: ", conn_err) | ||
| return nil, "Proxy connection failed: " .. conn_err | ||
| end | ||
|
|
||
| -- For HTTPS, use CONNECT tunnel | ||
| if url:match("^https://") then | ||
| local target_host = url:match("https://([^/]+)") | ||
| local res, err = httpc:request({ | ||
| method = "CONNECT", | ||
| path = target_host, | ||
| headers = { | ||
| ["Host"] = target_host, | ||
| }, | ||
| }) | ||
| if not res or res.status ~= 200 then | ||
| ngx.log(ngx.ERR, "CONNECT tunnel failed: ", err or (res and res.status)) | ||
| return nil, "Proxy CONNECT failed" | ||
| end | ||
|
|
||
| -- Upgrade to TLS | ||
| local ssl_ok, ssl_err = httpc:ssl_handshake(nil, host, opts.ssl_verify ~= false) | ||
| if not ssl_ok then | ||
| ngx.log(ngx.ERR, "SSL handshake through proxy failed: ", ssl_err) | ||
| return nil, "SSL handshake failed: " .. ssl_err | ||
| end |
There was a problem hiding this comment.
Resource leak: When the proxy connection flow fails after successful connection (e.g., during CONNECT tunnel or SSL handshake), the connection is not properly closed. The httpc:set_keepalive() or httpc:close() should be called in error paths to prevent connection leaks. Consider adding proper cleanup in the error handling sections before returning.
openresty/lua/http_utils.lua
Outdated
| -- Proxy configuration from environment | ||
| local HTTP_PROXY = os.getenv("HTTP_PROXY") or os.getenv("http_proxy") | ||
| local HTTPS_PROXY = os.getenv("HTTPS_PROXY") or os.getenv("https_proxy") | ||
| local NO_PROXY = os.getenv("NO_PROXY") or os.getenv("no_proxy") or "" | ||
|
|
||
| -- Parse NO_PROXY into a lookup table | ||
| local no_proxy_hosts = {} | ||
| for host in NO_PROXY:gmatch("[^,]+") do | ||
| local h = host:match("^%s*(.-)%s*$") -- trim whitespace | ||
| if h and h ~= "" then | ||
| no_proxy_hosts[h:lower()] = true | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
The NO_PROXY environment variable is read at module load time and stored in module-level variables. In OpenResty/nginx, Lua modules are loaded once per worker process. If the environment variables change after the worker starts, these changes will not be reflected until the worker is restarted. Consider documenting this limitation or implementing a mechanism to refresh proxy configuration if needed.
| -- Proxy configuration from environment | |
| local HTTP_PROXY = os.getenv("HTTP_PROXY") or os.getenv("http_proxy") | |
| local HTTPS_PROXY = os.getenv("HTTPS_PROXY") or os.getenv("https_proxy") | |
| local NO_PROXY = os.getenv("NO_PROXY") or os.getenv("no_proxy") or "" | |
| -- Parse NO_PROXY into a lookup table | |
| local no_proxy_hosts = {} | |
| for host in NO_PROXY:gmatch("[^,]+") do | |
| local h = host:match("^%s*(.-)%s*$") -- trim whitespace | |
| if h and h ~= "" then | |
| no_proxy_hosts[h:lower()] = true | |
| end | |
| end | |
| -- Proxy configuration from environment (refreshable) | |
| local HTTP_PROXY | |
| local HTTPS_PROXY | |
| local NO_PROXY = "" | |
| local no_proxy_hosts = {} | |
| -- Refresh proxy configuration from environment variables. | |
| -- This is called once at module load, and can be called again if the | |
| -- environment variables change during the worker's lifetime. | |
| local function refresh_proxy_config() | |
| HTTP_PROXY = os.getenv("HTTP_PROXY") or os.getenv("http_proxy") | |
| HTTPS_PROXY = os.getenv("HTTPS_PROXY") or os.getenv("https_proxy") | |
| NO_PROXY = os.getenv("NO_PROXY") or os.getenv("no_proxy") or "" | |
| -- Parse NO_PROXY into a lookup table | |
| no_proxy_hosts = {} | |
| for host in NO_PROXY:gmatch("[^,]+") do | |
| local h = host:match("^%s*(.-)%s*$") -- trim whitespace | |
| if h and h ~= "" then | |
| no_proxy_hosts[h:lower()] = true | |
| end | |
| end | |
| end | |
| -- Initialize proxy configuration at module load time | |
| refresh_proxy_config() | |
| -- Expose refresh function so callers can update proxy settings if needed | |
| _M.refresh_proxy_config = refresh_proxy_config |
| function _M.post_json(url, data, opts) | ||
| opts = opts or {} | ||
| opts.method = "POST" | ||
| opts.body = cjson.encode(data) | ||
| opts.headers = opts.headers or {} | ||
| opts.headers["Content-Type"] = "application/json" |
There was a problem hiding this comment.
Potential header overwrite issue: The function modifies opts.headers["Content-Type"] directly. If the caller provides opts.headers with a pre-existing "Content-Type" that should be preserved alongside the JSON content type, it will be silently overwritten. While this is likely intentional for ensuring JSON content type, consider whether this behavior should be documented in the function's documentation comment.
- Create http_utils.lua with proxy support for all outbound HTTP requests - Supports HTTP_PROXY, HTTPS_PROXY, NO_PROXY environment variables - Implements CONNECT tunneling for HTTPS through HTTP proxy - Provides request(), post_json(), and post_form() convenience functions - Update webhooks.lua to use http_utils for proxy support - Update ip_reputation.lua to use http_utils for AbuseIPDB and webhook calls - Refactor captcha_providers.lua to use shared http_utils module - Removes ~140 lines of duplicated proxy code - Fix admin_auth.lua to use WAF_SESSION_TTL environment variable - Previously hardcoded to 86400 (24 hours) - Now consistent with SSO modules (sso_ldap.lua, sso_oidc.lua)
6569aae to
9f6fe62
Compare
Create http_utils.lua with proxy support for all outbound HTTP requests
Update webhooks.lua to use http_utils for proxy support
Update ip_reputation.lua to use http_utils for AbuseIPDB and webhook calls
Refactor captcha_providers.lua to use shared http_utils module
Fix admin_auth.lua to use WAF_SESSION_TTL environment variable