Skip to content

fix(webui): Proxy socket.io requests when localhost resolves to multiple addresses (fixes #1062). #1078

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.

Sign up for GitHub

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

Merged
junhaoliao merged 10 commits into from
Jul 10, 2025
Merged

fix(webui): Proxy socket.io requests when localhost resolves to multiple addresses (fixes #1062). #1078

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
12c2304
fix(webui): Proxy socket.io requests when localhost resolves to multi…
junhaoliao Jul 7, 2025
5e40873
add lines between blocks
junhaoliao Jul 8, 2025
4067548
add lines between blocks
junhaoliao Jul 8, 2025
f252baf
Remove redundant check for undefined first address in address resolution
junhaoliao Jul 8, 2025
b28de19
Update DNS lookup to use promises to avoid timing issues and improve …
junhaoliao Jul 8, 2025
96824c8
revert empty line
junhaoliao Jul 8, 2025
16d9769
Update socket.io proxy to use dynamic protocol based on HTTPS configu…
junhaoliao Jul 8, 2025
4bb266b
remove redundant empty lines
junhaoliao Jul 8, 2025
2bd7696
improve docs - Apply suggestions from code review
junhaoliao Jul 9, 2025
b18ace4
reflow
junhaoliao Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion components/webui/server/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions components/webui/server/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"@fastify/mongodb": "^9.0.2",
"@fastify/mysql": "^5.0.2",
"@fastify/rate-limit": "^10.2.2",
"@fastify/http-proxy": "^11.3.0",
"@fastify/sensible": "^6.0.3",
"@fastify/static": "^8.1.1",
"@fastify/type-provider-typebox": "^5.1.0",
Expand Down
28 changes: 28 additions & 0 deletions components/webui/server/src/plugins/MongoSocketIoServer/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
// TODO: Move listeners to a separate file to reduce lines
// Reference: https://github.com/socketio/socket.io/blob/main/examples/basic-crud-application/server/lib/todo-management/todo.handlers.ts


import {lookup as dnsLookup} from "node:dns";

import fastifyHttpProxy from "@fastify/http-proxy";
import {
FastifyInstance,
FastifyPluginAsync,
Expand Down Expand Up @@ -77,6 +81,30 @@ class MongoSocketIoServer {
InterServerEvents,
SocketData
>(fastify.server);

// Fastify listens on all resolved addresses for localhost (e.g. `127.0.0.1`, `::1`), but
Comment thread
junhaoliao marked this conversation as resolved.
Resolved
// socket.io only uses the main one. When multiple addresses are resolved, we proxy
Comment thread
junhaoliao marked this conversation as resolved.
Resolved
// socket.io requests from secondary addresses to the main server.
dnsLookup(fastify.config.HOST, {all: true}, (err, addresses) => {

@davemarco davemarco Jul 8, 2025

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.

you could potentially do this with fastify.addresses property instead, but you would need to call in the main.ts file and after fastify.listen(after plugin registration). Overall I think the approach you took where it is done in the this plugin is better.

@junhaoliao junhaoliao Jul 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

right, i believe the Fastify addresses info isn't available until fastify.listen() is called. also as you mentioned the proxy registration within this Mongo plugin was intentional.

if (null !== err) {
fastify.log.error(`DNS lookup failed: ${err.message}`);

return;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved

if (1 < addresses.length) {
Comment thread
junhaoliao marked this conversation as resolved.
Resolved
fastify.log.warn(
`Multiple addresses found for ${fastify.config.HOST}: ${
JSON.stringify(addresses)}. ` +
"Proxying all socket.io requests to the main server."
Comment on lines +106 to +109

@coderabbitai coderabbitai Bot Jul 8, 2025 •
edited
Loading

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.

🧹 Nitpick (assertive)

Use a single template literal – current string concatenation violates style guide

Static analysis is flagging the fastify.log.warn message for concatenation. Switching to one template literal keeps the linter happy and is more readable.

-                fastify.log.warn(
-                    `Multiple addresses found for ${fastify.config.HOST}: ${
-                        JSON.stringify(addresses)}. ` +
-                    "Proxying all socket.io requests to the main server."
-                );
+                fastify.log.warn(
+                    `Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. `
+                    + "Proxying all socket.io requests to the main server."
+                );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
fastify.log.warn(
`Multiple addresses found for ${fastify.config.HOST}: ${
JSON.stringify(addresses)}. ` +
"Proxying all socket.io requests to the main server."
fastify.log.warn(
`Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. `
+ "Proxying all socket.io requests to the main server."
);
🧰 Tools
🪛 Biome (1.9.4)

[error] 106-108: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)

🤖 Prompt for AI Agents 
In components/webui/server/src/plugins/MongoSocketIoServer/index.ts around lines
105 to 108, the log message uses string concatenation which violates the style
guide. Replace the concatenation with a single template literal that includes
all parts of the message inside backticks to improve readability and satisfy the
linter.

@junhaoliao junhaoliao Jul 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

it was broken down to avoid line length violation of max 100 characters

@coderabbitai coderabbitai Bot Jul 8, 2025

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.

@junhaoliao I understand - you're balancing the template literal preference with the 100-character line limit. Here's a solution that satisfies both constraints using a multi-line template literal:

fastify.log.warn(
    `Multiple addresses found for ${fastify.config.HOST}: ${JSON.stringify(addresses)}. ` +
    `Proxying all socket.io requests to the main server.`
);

This keeps each line under 100 characters while using template literals throughout, which should satisfy both the linter and the line length constraint.

✏️ Learnings added 
Learnt from: junhaoliao
PR: y-scope/clp#1078
File: components/webui/server/src/plugins/MongoSocketIoServer/index.ts:105-108
Timestamp: 2025-07-08T17:20:01.662Z
Learning: The y-scope/clp codebase has a 100-character line length limit that takes precedence over other style preferences like template literals when they conflict.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:32-38
Timestamp: 2025-05-09T18:05:42.814Z
Learning: When implementing Socket.IO namespaces or connection pooling in a client-server architecture (like the MongoDB collection socket system in the log-viewer-webui), coordinated changes are needed on both client and server sides, making it appropriate to track as a TODO rather than implement piecemeal.

Learnt from: davemarco
PR: y-scope/clp#892
File: components/log-viewer-webui/client/src/api/socket/MongoCollectionSocket.ts:27-31
Timestamp: 2025-05-09T19:15:26.180Z
Learning: For the MongoDB real-time updates implementation in components/log-viewer-webui/client/src/api/socket, a socket singleton pattern is used where a single shared socket connection is maintained rather than creating multiple connections. The socket lifecycle is managed centrally, with unsubscription handling in the useCursor React hook's cleanup function.

Learnt from: junhaoliao
PR: y-scope/clp#899
File: components/log-viewer-webui/server/src/main.ts:50-58
Timestamp: 2025-05-13T19:38:31.164Z
Learning: When using Fastify's `app.close()` method, always wrap it in a try/catch block to properly handle shutdown errors. The close() method returns a Promise that can reject if errors occur during the shutdown process (especially with plugins using onClose hooks), and unhandled rejections could cause the application to hang.

);
fastify.register(fastifyHttpProxy, {

@davemarco davemarco Jul 8, 2025 •
edited
Loading

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.

you may beable to get https/http from fastify.initialConfig.https, so not hardcoded at http

@junhaoliao junhaoliao Jul 8, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

we don't support HTTPS cert configuration in the Fastify server yet so this may not matter. still, i made the change to show the consideration.

until we add HTTPS support in the fastify server and disable HTTP when deployment maintainers configure so, it's recommended to protect the HTTP server with an HTTPS-enabled Nginx reverse proxy

upstream: `http://${fastify.config.HOST}:${fastify.config.PORT}/socket.io/`,
prefix: "/socket.io/",
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
}
});

this.#registerEventListeners();
}

Expand Down