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
WalkthroughThis pull request introduces modifications to the configuration and authentication handling in the Actual Budget server application. The changes primarily focus on enhancing the proxy and login method configurations. Key modifications include adding a new Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (1)src/account-db.js (1)
The validation against 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 |
src/app-account.js
Outdated
| @@ -59,6 +60,10 @@ app.post('/login', async (req, res) => { | |||
| let loginMethod = getLoginMethod(req); | |||
| console.log('Logging in via ' + loginMethod); | |||
| let tokenRes = null; | |||
| if (!config.allowedLoginMethods.includes(loginMethod)) { | |||
There was a problem hiding this comment.
This is a security improvement. getLoginMethod prefers whatever's in the clientSide request. This allows us to reject that if the server admin has decided they don't want header auth enabled.
| export interface Config { | ||
| mode: 'test' | 'development'; | ||
| loginMethod: 'password' | 'header' | 'openid'; | ||
| loginMethod: LoginMethod; | ||
| allowedLoginMethods: LoginMethod[]; |
There was a problem hiding this comment.
We have method on the auth table now. But header auth is still using password, just accepting it a different way for now. Maybe we should filter the getLoginMethods using this when returning it in bootstrap as well. Haven't fully explored the recent openid changes, so I'm not quite sure.
twk3
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/load-config.js (1)
133-137: Add debug logging for trustedAuthProxiesFor consistency with other configuration values, consider adding debug logging for trustedAuthProxies.
Add after line 217:
debug(`using trusted proxies ${finalConfig.trustedProxies.join(', ')}`); +debug(`using trusted auth proxies ${finalConfig.trustedAuthProxies?.join(', ') ?? 'default'}`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/account-db.js(1 hunks)src/load-config.js(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/load-config.js (1)
Learnt from: twk3
PR: actualbudget/actual-server#499
File: src/load-config.js:57-58
Timestamp: 2024-12-09T15:23:05.910Z
Learning: In `src/load-config.js`, keep `allowedLoginMethods` default value as `['password', 'header']` to ensure backward compatibility and avoid breaking existing header authentication.
🔇 Additional comments (3)
src/load-config.js (3)
57-58: LGTM: Configuration changes align with objectives
The changes appropriately:
- Include all supported login methods while maintaining backward compatibility
- Add clear documentation about future proxy trust separation
Also applies to: 66-68
123-127: LGTM: Robust environment variable processing
The implementation properly handles the ACTUAL_ALLOWED_LOGIN_METHODS environment variable with appropriate string processing.
129-132: LGTM: Appropriate handling of proxy trust configuration
The implementation correctly processes IP/CIDR values without case conversion, which is appropriate for network addresses.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/load-config.js (2)
57-68: LGTM! Consider documenting the proxy distinction.The configuration changes look good and maintain backward compatibility. The comment about the future distinction between
trustedProxiesandtrustedAuthProxiesis important for maintainers.Consider adding a TODO comment with a link to an issue tracking this future change, to ensure the distinction between proxy types is properly documented and implemented.
227-231: Simplify the debug template literal.The debug logging is good, but the template literal could be simplified.
-debug( - `using trusted auth proxies ${ - finalConfig.trustedAuthProxies?.join(', ') ?? 'same as trusted proxies' - }`, -); +debug( + 'using trusted auth proxies %s', + finalConfig.trustedAuthProxies?.join(', ') ?? 'same as trusted proxies' +);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/load-config.js(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/load-config.js (1)
Learnt from: twk3
PR: actualbudget/actual-server#499
File: src/load-config.js:57-58
Timestamp: 2024-12-09T15:23:05.910Z
Learning: In `src/load-config.js`, keep `allowedLoginMethods` default value as `['password', 'header']` to ensure backward compatibility and avoid breaking existing header authentication.
🔇 Additional comments (1)
src/load-config.js (1)
57-57: Verify OpenID configuration compatibility.
The addition of 'openid' to allowedLoginMethods needs verification with the OpenID configuration logic.
✅ Verification successful
Based on the verification results, I can now provide a final response:
OpenID configuration is properly integrated and compatible
The addition of 'openid' to allowedLoginMethods is correctly implemented with proper:
- Type definitions in
config-types.ts - Full OpenID authentication flow implementation in
src/accounts/openid.js - Integration with the main application in
src/app-openid.js - Configuration validation and runtime support in
src/account-db.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify OpenID configuration usage and dependencies
# Check for OpenID-related configuration and validation
rg -A 10 "openid" --type js
# Check for files that might need updates for OpenID support
fd -e js -e ts -x grep -l "loginMethod\|allowedLoginMethods"
Length of output: 16458
Script:
#!/bin/bash
# Let's verify the OpenID configuration implementation and its dependencies
# Check OpenID configuration type definitions
ast-grep --pattern 'interface $_ {
$$$
allowedLoginMethods?: $_
$$$
}'
# Check how loginMethods are validated
rg -A 5 "allowedLoginMethods" --type ts --type js
# Check the actual usage of login methods in authentication flow
ast-grep --pattern 'function $_($$_) {
$$$
loginMethod
$$$
}'
Length of output: 1621
Doc update PR: actualbudget/docs#581
Fixes: #371
Fixes: #392