Securing websockets by frieck · Pull Request #1802 · EvolutionAPI/evolution-api · GitHub
Skip to content

Securing websockets#1802

Merged
DavidsonGomes merged 3 commits intoEvolutionAPI:developfrom
frieck:main
Aug 4, 2025
Merged

Securing websockets#1802
DavidsonGomes merged 3 commits intoEvolutionAPI:developfrom
frieck:main

Conversation

@frieck
Copy link
Copy Markdown
Contributor

@frieck frieck commented Aug 4, 2025

Fixing publicly exposed WebSocket endpoints.

Summary by Sourcery

Restrict WebSocket Engine.IO handshake to only allow connections from localhost

Bug Fixes:

  • Prevent public exposure of WebSocket endpoints by enforcing a localhost address check on Engine.IO handshake

Enhancements:

  • Add remoteAddress inspection and isLocalhost helper for IPv4 and IPv6 addresses

@sourcery-ai
Copy link
Copy Markdown
Contributor

sourcery-ai Bot commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @frieck - 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:31` </location>
<code_context>
       allowRequest: async (req, callback) => {
         try {
           const url = new URL(req.url || '', 'http://localhost');
+          const isInternalConnection = req.socket.remoteAddress === '127.0.0.1' || req.socket.remoteAddress === '::1';
           const params = new URLSearchParams(url.search);

</code_context>

<issue_to_address>
Consider handling IPv6-mapped IPv4 addresses for localhost.

Also include '::ffff:127.0.0.1' in the internal connection check to cover IPv6-mapped IPv4 localhost addresses.
</issue_to_address>

<suggested_fix>
<<<<<<< SEARCH
          const isInternalConnection = req.socket.remoteAddress === '127.0.0.1' || req.socket.remoteAddress === '::1';
=======
          const isInternalConnection =
            req.socket.remoteAddress === '127.0.0.1' ||
            req.socket.remoteAddress === '::1' ||
            req.socket.remoteAddress === '::ffff:127.0.0.1';
>>>>>>> REPLACE

</suggested_fix>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

allowRequest: async (req, callback) => {
try {
const url = new URL(req.url || '', 'http://localhost');
const isInternalConnection = req.socket.remoteAddress === '127.0.0.1' || req.socket.remoteAddress === '::1';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider handling IPv6-mapped IPv4 addresses for localhost.

Also include '::ffff:127.0.0.1' in the internal connection check to cover IPv6-mapped IPv4 localhost addresses.

Suggested change
const isInternalConnection = req.socket.remoteAddress === '127.0.0.1' || req.socket.remoteAddress === '::1';
const isInternalConnection =
req.socket.remoteAddress === '127.0.0.1' ||
req.socket.remoteAddress === '::1' ||
req.socket.remoteAddress === '::ffff:127.0.0.1';

@frieck
Copy link
Copy Markdown
Contributor Author

frieck commented Aug 4, 2025

@sourcery-ai review

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @frieck - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

const url = new URL(req.url || '', 'http://localhost');
const params = new URLSearchParams(url.search);

const remoteAddress = req.socket.remoteAddress;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)

Suggested change
const remoteAddress = req.socket.remoteAddress;
const {remoteAddress} = req.socket;


ExplanationObject destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.

From the Airbnb Javascript Style Guide

@frieck
Copy link
Copy Markdown
Contributor Author

frieck commented Aug 4, 2025

@sourcery-ai review

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @frieck - I've reviewed your changes - here's some feedback:

  • Consider using a CIDR-based or net.isLoopback utility instead of hardcoding specific loopback address strings to cover all IPv4/IPv6 variants.
  • If the app sits behind a reverse proxy, req.socket.remoteAddress might not reflect the original client; ensure you handle X-Forwarded-For headers or configure trusted proxies.
  • You could extract the localhost check into a reusable helper to keep the controller code concise and improve testability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using a CIDR-based or net.isLoopback utility instead of hardcoding specific loopback address strings to cover all IPv4/IPv6 variants.
- If the app sits behind a reverse proxy, req.socket.remoteAddress might not reflect the original client; ensure you handle X-Forwarded-For headers or configure trusted proxies.
- You could extract the localhost check into a reusable helper to keep the controller code concise and improve testability.

## Individual Comments

### Comment 1
<location> `src/api/integrations/event/websocket/websocket.controller.ts:34` </location>
<code_context>
           const params = new URLSearchParams(url.search);

+          const { remoteAddress } = req.socket;
+          const isLocalhost =
+            remoteAddress === '127.0.0.1' || remoteAddress === '::1' || remoteAddress === '::ffff:127.0.0.1';
+
           // Permite conexões internas do Socket.IO (EIO=4 é o Engine.IO v4)
</code_context>

<issue_to_address>
Consider IPv6-mapped IPv4 and proxy scenarios for localhost detection.

This check may not handle all localhost scenarios, such as alternative IPv6-mapped formats or proxy headers like X-Forwarded-For. Consider updating the logic or documenting these limitations if relevant to your deployment.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +34 to +35
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Consider IPv6-mapped IPv4 and proxy scenarios for localhost detection.

This check may not handle all localhost scenarios, such as alternative IPv6-mapped formats or proxy headers like X-Forwarded-For. Consider updating the logic or documenting these limitations if relevant to your deployment.

@DavidsonGomes DavidsonGomes changed the base branch from main to develop August 4, 2025 21:53
@DavidsonGomes DavidsonGomes merged commit a8343a8 into EvolutionAPI:develop Aug 4, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants