-
Notifications
You must be signed in to change notification settings - Fork 5k
feat: add wildcard "*" to allow all hosts to connect via websocket #2280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add wildcard "*" to allow all hosts to connect via websocket #2280
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds support for a wildcard "*" value in the WEBSOCKET_ALLOWED_HOSTS configuration to bypass WebSocket IP restriction checks while preserving existing explicit host validation behavior. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/api/integrations/event/websocket/websocket.controller.ts:36-37` </location>
<code_context>
const websocketConfig = configService.get<Websocket>('WEBSOCKET');
const allowedHosts = websocketConfig.ALLOWED_HOSTS || '127.0.0.1,::1,::ffff:127.0.0.1';
- const isAllowedHost = allowedHosts
+ const allowAllHosts = allowedHosts.trim() === '*';
+ const isAllowedHost = allowAllHosts || allowedHosts
.split(',')
.map((h) => h.trim())
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider supporting '*' alongside other entries in ALLOWED_HOSTS, not only when it's the sole value.
Currently `allowAllHosts` is only true when `allowedHosts.trim() === '*'`. With a value like `"*,10.0.0.1"`, `*` will never match `remoteAddress` and the code will not allow all hosts as many would expect. To treat `*` as a wildcard even when combined with other entries, derive `allowAllHosts` from the split tokens, e.g.:
```ts
const tokens = allowedHosts.split(',').map(h => h.trim());
const allowAllHosts = tokens.includes('*');
const isAllowedHost = allowAllHosts || tokens.includes(remoteAddress);
```
(or equivalent), reusing `tokens` for the includes check.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Please, fix the lint with |
📋 Description
Adds support for using
*inWEBSOCKET_ALLOWED_HOSTSenv variable to allow any host to bypass the API’s WebSocket IP-restriction checks. Useful for local/Docker setups where container IPs change. Normal host whitelisting behavior remains unchanged when*is not used🧪 Type of Change
🧪 Testing
WEBSOCKET_ALLOWED_HOSTS=*bypasses IP validation✅ Checklist
📝 Additional Notes
Intended only for development scenarios. Production should continue using explicit whitelists as introduced in the original security enhancement PR #1929
Summary by Sourcery
New Features: