Seed trust_proxy=true so per-IP limit works behind StartOS proxy#6
Seed trust_proxy=true so per-IP limit works behind StartOS proxy#6wksantiago wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe Changestrust_proxy init config
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
startos/init/seedFiles.ts (1)
12-14: ⚡ Quick winConsider setting
trusted_proxiesexplicitly for defense in depth.While leaving
trusted_proxiesempty and relying on network topology (relay port only accessible via proxy) is acceptable given the current deployment model, explicitly settingtrusted_proxiesto the proxy's IP address or range would provide defense in depth. If the network configuration changes or the relay becomes directly accessible, clients could spoofX-Forwarded-Forheaders.The comment's final sentence "(Setting a wrong proxy address would silently disable the fix.)" is confusing since no proxy address is being set here. If you meant to warn about the risk of misconfiguring
trusted_proxies, consider clarifying or documenting the proxy's IP/range for future reference.🛡️ Example: Setting trusted_proxies explicitly
export const seedFiles = sdk.setupOnInit(async (effects) => { await wispToml.merge(effects, { - security: { trust_proxy: true }, + security: { + trust_proxy: true, + trusted_proxies: ["127.0.0.1", "::1"], // Adjust to actual proxy IP/range + }, }) })Note: You would need to determine the actual IP address or range used by the StartOS proxy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@startos/init/seedFiles.ts` around lines 12 - 14, Set the `trusted_proxies` configuration explicitly to the proxy's IP address or range instead of leaving it empty, providing defense in depth against potential X-Forwarded-For header spoofing if the network configuration changes. Additionally, update or replace the existing comment that references setting a proxy address to clarify the actual security approach being used and document the proxy's IP range for future maintainability, removing the confusing statement about what would happen if a wrong proxy address were set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@startos/init/seedFiles.ts`:
- Around line 4-5: The comment on lines 4-5 in startos/init/seedFiles.ts
incorrectly describes what the code sets. It claims the merge operation writes
"new schema defaults (fixed host/port/storage)" but the actual merge call only
explicitly sets "security.trust_proxy". Update the comment to accurately state
that the code seeds wisp.toml on every init to ensure trust_proxy is configured,
and clarify that schema defaults for host, port, and storage are applied during
file read and validation, not by the merge operation itself. Replace the comment
with the provided suggestion to reflect what the code actually does.
---
Nitpick comments:
In `@startos/init/seedFiles.ts`:
- Around line 12-14: Set the `trusted_proxies` configuration explicitly to the
proxy's IP address or range instead of leaving it empty, providing defense in
depth against potential X-Forwarded-For header spoofing if the network
configuration changes. Additionally, update or replace the existing comment that
references setting a proxy address to clarify the actual security approach being
used and document the proxy's IP range for future maintainability, removing the
confusing statement about what would happen if a wrong proxy address were set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d29e954-cb95-4d3d-9e03-228332dc2478
📒 Files selected for processing (1)
startos/init/seedFiles.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Superseded by #7, which lands the per-IP fix off current master. Behind the StartOS proxy all clients share the proxy IP, so rather than trusting X-Forwarded-For (which depends on the proxy injecting it, and trusting it from any peer is a spoofing surface if it does not), #7 stops limiting per IP entirely and relies on global max_connections plus the event/query rate limits, matching how nostr-rs-relay and strfry are packaged. |
Closes #5
Summary by CodeRabbit