[WIP] fix: Fix the auth proxy trust by ensuring the proxy is in the trust #499
Conversation
- Validate that the closest peer to the express server is trusted proxy - Add a new trustedAuthProxies config to eventually be used for this - Add a allowedLoginMethod config to enable fully disabling header auth
actual-github-bot
bot
changed the title
|
@tuetenk0pp let me know if this solves the issue you were running into. By default your local proxy should be trusted, but you can refine it. We are only checking the closest peer, so if your auth proxy is further out than that, it's up to you to configure your closest peer proxy to only accept the header from a proxy you trust. This matches the behaviour of the project I had initially suggested as a reference during the original feature which is https://github.com/BeryJu/hass-auth-header |
|
@twk3 I am very busy at the moment and not able to test the PR. I will test it once I find some time to spend. |
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
|
Can confirm this PR works and solves the issue of the Trusted Proxies. |
|
👋 Hi! It looks like this PR has not had any changes for a week now. Would you like someone to review this PR? If so - please remove the "[WIP]" prefix from the PR title. That will let the community know that this PR is open for a review. |
twk3
changed the title
WalkthroughThe pull request introduces several changes across multiple files to enhance the login functionality and configuration management. In Assessment against linked issues
Possibly related PRs
Suggested labels
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 your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/util/validate-user.js (2)
50-51: Consider using let/const consistently.The code uses
letfor some variables but switches tovarlater. Consider usingconsthere as these values aren't reassigned.-let peer = req.socket.remoteAddress; -let peerIp = ipaddr.process(peer); +const peer = req.socket.remoteAddress; +const peerIp = ipaddr.process(peer);
52-58: Improve type safety and variable declaration.The code has a few issues:
- Uses
varinstead ofconst- Has a TypeScript ignore comment that should be addressed
-const rangeList = { - allowed_ips: trustedAuthProxies.map((q) => ipaddr.parseCIDR(q)), -}; -/* eslint-disable @typescript-eslint/ban-ts-comment */ -// @ts-ignore : there is an error in the ts definition for the function, but this is valid -var matched = ipaddr.subnetMatch(peerIp, rangeList, 'fail'); -/* eslint-enable @typescript-eslint/ban-ts-comment */ +const rangeList = { + allowed_ips: trustedAuthProxies.map((cidr) => ipaddr.parseCIDR(cidr)), +}; +const matched = ipaddr.subnetMatch(peerIp, rangeList, 'fail');Consider opening an issue or PR to fix the TypeScript definitions in the
ipaddr.jslibrary instead of using@ts-ignore.src/app-account.js (1)
Line range hint
68-82: Consider improving sensitive data handling in debug logsWhile the password obfuscation is good, consider moving the debug statement to use the
debugSensitivelogger (as seen in load-config.js) to ensure sensitive data is only logged when explicitly enabled.- console.debug('HEADER VALUE: ' + obfuscated); + const debugSensitive = createDebug('actual-sensitive:auth'); + debugSensitive('HEADER VALUE: ' + obfuscated);src/load-config.js (1)
57-68: Good security practice: Explicit separation of proxy trust concernsThe separation between
trustedProxies(for Express) andtrustedAuthProxies(for header auth) is a good security practice that follows the principle of least privilege. The default trusted networks are appropriately set to common private network ranges.Consider documenting these security-critical settings in the README to help users understand the implications of modifying them.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
upcoming-release-notes/499.mdis excluded by!**/*.md
📒 Files selected for processing (5)
src/app-account.js(2 hunks)src/app.js(1 hunks)src/config-types.ts(1 hunks)src/load-config.js(2 hunks)src/util/validate-user.js(1 hunks)
🔇 Additional comments (7)
src/config-types.ts (2)
3-4: LGTM! Well-structured type definition.
The LoginMethod type alias effectively encapsulates all supported authentication methods, improving type safety and maintainability.
7-10: Verify the relationship between loginMethod and allowedLoginMethods.
The configuration now has both a single loginMethod and an array of allowedLoginMethods. This might lead to confusion or inconsistency.
✅ Verification successful
Based on the search results, I can now provide a final response:
The relationship between loginMethod and allowedLoginMethods is valid and serves different purposes
The codebase shows these properties work together in a clear way:
allowedLoginMethodsdefines the list of permitted authentication methodsloginMethodspecifies the currently active/default method- There's a validation check in
app-account.jsthat ensures the chosenloginMethodexists inallowedLoginMethods - The default configuration in
load-config.jsshows a sensible setup with password as default method and both password/header as allowed methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of both properties to understand their relationship
rg -A 5 "loginMethod|allowedLoginMethods" --type ts --type js
Length of output: 4023
src/util/validate-user.js (2)
47-48: LGTM! Sensible fallback mechanism.
The fallback to trustedProxies when trustedAuthProxies is not set provides good backward compatibility.
59-64: LGTM! Good security logging.
The code properly logs both successful and failed authentication attempts with the peer IP address.
src/app.js (1)
25-25: Verify the impact on rate limiting and security middleware.
Setting trust proxy affects how Express handles X-Forwarded-* headers, which impacts:
- Rate limiting (configured below this line)
- IP-based security features
- Request IP determination
Ensure that:
- Rate limiting works correctly with trusted proxies
- Security features properly identify client IPs
✅ Verification successful
Let me gather more information about the trust proxy configuration and its relationship with rate limiting.
Let me search for the configuration file and rate limiter implementation details.
Trust proxy configuration is properly implemented and secure
The configuration is well-implemented with:
- Default trusted proxy ranges limited to private networks (
10.0.0.0/8,172.16.0.0/12,192.168.0.0/16,fc00::/7,::1/128) - Rate limiting properly configured with:
- 500 requests per minute window
- Standard rate limit headers enabled
- Legacy headers disabled
The trust proxy setting is appropriately placed before the rate limiter middleware and the configuration aligns with security best practices for handling X-Forwarded-* headers in a proxied environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of req.ip and X-Forwarded-* headers
rg -A 5 "req\.ip|X-Forwarded-" --type js
# Search for rate limiting configurations
rg -A 5 "rateLimit|limiter" --type js
Length of output: 1405
Script:
#!/bin/bash
# Search for trust proxy and config related code
ast-grep --pattern 'app.set("trust proxy", $_)'
ast-grep --pattern 'app.set('\''trust proxy'\'', $_)'
# Search for config definition related to trusted proxies
rg -A 3 "trustedProxies.*=" --type js
rg -A 3 "export.*trustedProxies" --type js
# Search for IP-based security features
rg -A 5 "req\.(ip|ips)" --type js
Length of output: 287
Script:
#!/bin/bash
# Find and show configuration files
fd -e js config
fd -e json config
# Show content of potential config files
fd -e js config -x cat {}
fd -e json config -x cat {}
# Look for rate limiter configuration and implementation
ast-grep --pattern 'rateLimit({
$$$
})'
Length of output: 9887
src/app-account.js (1)
17-17: LGTM: Good security enhancement for login method validation
The early validation of login methods against an allowed list is a good security practice that prevents unauthorized authentication attempts.
Also applies to: 63-66
src/load-config.js (1)
123-137: Verify environment variable parsing for security settings
The parsing logic for trusted proxies looks correct, but let's verify there are no edge cases that could lead to security issues.
✅ Verification successful
Let's gather more information about how these proxy settings are used in the identified files.
Based on the findings, I can now provide a final response:
Proxy trust settings are properly handled with appropriate security measures
The verification shows that:
- The proxy settings are correctly used in Express's trust proxy mechanism
- The validation logic in
validate-user.jsproperly processes IP addresses usingipaddr.jslibrary - Empty/malformed values are filtered out by the
.filter(Boolean)call in the parsing logic - There's a secure fallback mechanism where
trustedAuthProxiesfalls back totrustedProxieswhen not set - The IP validation uses CIDR parsing for proper network range checking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential edge cases in proxy trust parsing
# Test 1: Check if empty/malformed values are properly handled
echo "Test 1: Empty/malformed values"
ACTUAL_TRUSTED_PROXIES=",, , invalid" \
ACTUAL_TRUSTED_AUTH_PROXIES=",, , invalid" \
node -e "
const config = require('./src/load-config.js').default;
console.log('Trusted Proxies:', config.trustedProxies);
console.log('Trusted Auth Proxies:', config.trustedAuthProxies);
"
# Test 2: Search for any direct proxy trust validations in the codebase
echo -e "\nTest 2: Proxy trust validation usage"
rg -l "trustedAuthProxies|trustedProxies" --type js
Length of output: 1033
Script:
#!/bin/bash
# Check how proxy settings are used in the identified files
echo "Checking app.js usage:"
rg "trustedAuthProxies|trustedProxies" src/app.js -B 2 -A 2
echo -e "\nChecking validate-user.js usage:"
rg "trustedAuthProxies|trustedProxies" src/util/validate-user.js -B 2 -A 2
# Let's also check the actual config structure
echo -e "\nChecking default config structure:"
ast-grep --pattern 'export const defaultConfig = {
$$$
}'
Length of output: 1007
twk3
changed the title
|
Just one more thing here to get it ready for review. The login method changed a bit with the intro of openid, specifically the |
TODO: update docs
Fixes: #371
Fixes: #392