Conversation
Drivers, probes and an optional LAN listener now go through one engine keyed by host:port, so two Lua drivers can poll the same inverter (or different unit IDs behind one gateway) without each taking a socket the device will not accept. modbus_proxy is off by default. When on, other integrations talk to FTW instead of the device. Writes stay blocked unless allow_write is set — they would bypass the control loop. Enabling it needs a restart. Signed-off-by: Cursor Agent <cursoragent@cursor.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3890d96698
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| first = err | ||
| } | ||
| } | ||
| p.wg.Wait() |
There was a problem hiding this comment.
When a proxy client keeps a persistent connection idle, as Home Assistant commonly does, Close closes only the listeners and then waits while the accepted connection remains blocked in readMBAP until its 90-second read deadline. Because main defers this call on SIGTERM and API-triggered restarts, shutdown can stall for up to 90 seconds; track and close accepted connections or otherwise cancel their reads before waiting.
Useful? React with 👍 / 👎.
| render: function (ctx) { | ||
| var field = ctx.field, config = ctx.config; | ||
| if (!config.homeassistant) config.homeassistant = {}; | ||
| if (!config.modbus_proxy) config.modbus_proxy = {}; |
There was a problem hiding this comment.
On an upgraded installation with no modbus_proxy section, merely opening this tab and saving any Home Assistant edit creates a disabled proxy object because the render initializes it and captureCurrentTab serializes all of the new controls. modbusProxyRestartReasons then compares the old nil pointer with this object and reports that the TCP listener requires a restart, so an otherwise hot-reloadable HA edit produces a spurious restart prompt and persists unrelated configuration; preserve the absent disabled state until the operator actually changes a proxy setting.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3890d96. Configure here.
| return | ||
| } | ||
| slog.Warn("modbus proxy accept", "listen", ln.Addr().String(), "err", err) | ||
| return |
There was a problem hiding this comment.
Accept errors stop the proxy
Medium Severity
A non-shutdown Accept error returns from serve and never loops again. The listen socket stays bound, so LAN clients cannot reconnect until the process restarts, even though startup logged the proxy as listening.
Reviewed by Cursor Bugbot for commit 3890d96. Configure here.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Reviewed the Modbus proxy + shared-session engine. Two lenses:
Correctness — session pooling in engine.go is ref-counted and mutex-guarded correctly; Capability.mu serializes every Read/Write*/executePDU call including applyUnit, so concurrent drivers/proxy clients sharing one socket can't interleave requests or race on the unit-ID switch. readMBAP/writeMBAP bound the ADU length (≤254 bytes) before allocating, so a malicious length field can't trigger a large alloc. Connection count is capped (proxyMaxClients=16) and idle-timed out (90s), so a LAN client can't hold the proxy open indefinitely or exhaust listener slots.
Security — the write gate (forward(), proxy.go) runs before cap.executePDU, so a denied write never reaches the driver/hardware; unknown function codes default-deny to illegal-function. allow_write is one flag applied to all binds from a single proxy config, matching the docs. Each listener is pinned to exactly one backend host:port at creation, no way for a LAN client to redirect to an arbitrary backend. Off by default.
No blocking findings. Good test coverage on the shared-socket + unit-id-multiplex paths (engine_test.go, proxy_test.go, slave_test.go). Safe to merge from my read — the PR body already flags overlap with #999/#957/#826 on config.go/main.go, worth a rebase-check before merging whichever lands second.
Keep master's shared Modbus session (#987) and graft the LAN proxy onto it instead of a second pool. Resolve config/UI overlap with OCPP; drop the retired CalDAV comments. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_e01b3346-132c-4ce9-9192-60f84a493c3b) |
There was a problem hiding this comment.
Agentic security review of the Modbus TCP proxy found two medium issues on the unauthenticated listener path: missing unit-ID allowlisting on forwarded PDUs, and proxy traffic monopolizing the shared backend session used by drivers.
Sent by Cursor Security Agent: Security Reviewer
| func (p *Proxy) forward(backend Bind, unitID uint8, pdu []byte) []byte { | ||
| fc := pdu[0] | ||
| if isModbusWrite(fc) && !p.allowWrite { | ||
| return exceptionPDU(fc, modbusExcIllegalFn) | ||
| } | ||
| if !isModbusRead(fc) && !isModbusWrite(fc) { | ||
| return exceptionPDU(fc, modbusExcIllegalFn) | ||
| } | ||
| p.mu.Lock() | ||
| cap := p.byAddr[sessionKey(backend.Host, backend.Port)] | ||
| p.mu.Unlock() | ||
| if cap == nil { | ||
| return exceptionPDU(fc, modbusExcGWPath) | ||
| } | ||
| res, err := cap.executePDU(unitID, pdu) |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
forward sends the client-supplied Modbus unit ID on the shared backend TCP session. Binds only map listen address to host:port; configured driver unit IDs are never recorded or checked, so any host that can reach the unauthenticated listener can address every slave the gateway will answer, not only the inverter or meter FTW is configured for.
Impact: On a TCP-to-RS485 gateway this exposes other batteries, meters, and chargers. Reads leak register maps; if allow_write is enabled, writes and unit 0 can change hardware FTW does not manage.
Reviewed by Cursor Security Reviewer for commit 272a083. Configure here.
| func (c *Capability) executePDU(unitID uint8, pdu []byte) ([]byte, error) { | ||
| conn := c.conn | ||
| conn.mu.Lock() | ||
| defer conn.mu.Unlock() | ||
| if err := conn.ensureClient(); err != nil { | ||
| return nil, err | ||
| } | ||
| conn.applyUnit(int(unitID)) | ||
| res, err := conn.client.roundTrip(unitID, pdu) | ||
| if err == nil { | ||
| conn.noteLiveResponse() | ||
| return res, nil | ||
| } | ||
| if !isTransportError(err) { | ||
| conn.noteLiveResponse() | ||
| return res, err | ||
| } | ||
| if rerr := conn.prepareTransportRetry(); rerr != nil { | ||
| return nil, fmt.Errorf("pdu after reconnect: %w (original: %v)", rerr, err) | ||
| } | ||
| conn.applyUnit(int(unitID)) | ||
| res, err = conn.client.roundTrip(unitID, pdu) | ||
| conn.finishRequest(err) | ||
| return res, markTransport(err) |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: MEDIUM
Unauthenticated proxy clients share the same per-endpoint mutex and TCP socket as driver polls. executePDU holds conn.mu for the whole round-trip (up to the 5s request timeout), including reconnect-once. proxyMaxClients only caps accept goroutines; a single client can serialize the session indefinitely, and a transport error from proxy traffic trips shared reconnect backoff for drivers.
Impact: A LAN host can starve FTW's Modbus drivers on that endpoint: delayed or missing inverter/meter reads and, if the site meter uses that session, stale meter data that stops dispatch.
Reviewed by Cursor Security Reviewer for commit 272a083. Configure here.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
What it does: adds an opt-in Modbus TCP proxy so other LAN tools (HA's native Modbus, Node-RED, …) can share the single TCP session FTW already holds to an inverter/meter, instead of opening a second competing connection.
Correctness
Engine.Opendelegates to the existing pooledDialWithOptions(session sharing from #987) —engine.gois wiring, not a second pool, which matches the comment and the tests (sessionCount/refCountassert one shared socket for two opens).executePDUholdsconn.mufor the full round-trip, so concurrent proxy clients can't interleave responses on the shared session — correct for a synchronous protocol like Modbus TCP.- Read/write function-code classification is whitelist-based: anything not explicitly a known read or write op (e.g. diagnostics, exception status) falls through to "illegal function" rather than being allowed by default — safer than a blacklist.
readMBAPbounds-checkslengthbefore allocating the PDU buffer (< 2 || > proxyMaxADULength), so no over-read/panic on a malformed frame.- Multi-endpoint config validation (
ModbusProxyBinds) requires an explicitproxy_listenper driver when there's more than one Modbus host:port, and rejects two endpoints colliding on the same listen address — good, catches a misconfig at parse time instead of at runtime. - Good test coverage: engine sharing, proxy multiplexing, write-gating, unknown-function-code handling, config parsing/validation, restart-required signaling.
Security
- Off by default; writes blocked unless
allow_write: trueis set explicitly — sane default for a LAN listener with no Modbus-level auth (documented plainly in the config comments and the new UI copy). proxyMaxClientssemaphore caps concurrent proxy connections — bounds the new listener's exposure to a basic connection-flood.- No secrets, no new outbound network destinations — this only opens a new inbound LAN listener, which is the feature itself and is clearly flagged as such.
Nothing blocking from my read — safe to merge.
3 participants
Accepted text proposal
Fredrik asked for a Modbus proxy in FTW so other integrations can talk to devices through the box, and for every driver to go through the same engine so several drivers can poll one device while it still holds a single socket.
What changed
host:portinDial(fix(modbus): share one TCP session per endpoint and name eviction wars #987). This PR no longer keeps a second pool.Engine.Openis the named factory; it Dials into that session.modbus_proxylistens on the LAN (default:1502when the site has one Modbus endpoint) and multiplexes client PDUs onto that session. Writes are denied unlessallow_writeis set. Unknown function codes are illegal-function, not forwarded.host:portbackends each needcapabilities.modbus.proxy_listen. Two drivers on the same endpoint share one listen address.config.example.yamlis the operator surface.Why
Many inverters accept one Modbus TCP client. FTW holding that socket is why Home Assistant’s native Modbus / Sungrow / SolarEdge integrations cannot share the device. A proxy on the box is the usual fix (port 1502 is already treated as a proxy port in discovery).
Boundaries and safety
slog.Errorand continue).0x0B, dispatch still uses the driver path and its own backoff.modbus_proxy.*and, while the proxy is on, driver Modbus host/port/proxy_listenrequire a restart. Driver add/remove still hot-reloads; they join or leave the shared session.Out of scope: exposing FTW telemetry as a virtual Modbus map, TLS, unit-id remapping, hot-reloading the listen port.
Verification
go test ./internal/modbus/and./internal/config/after mergingorigin/master.go test ./cmd/ftw/node --test web/settings/tabs/ha.test.mjsMerged
origin/master(includes #987 shared session and OCPP). Conflicts in config/UI were both-sides-keep. The client.go overlap was the same “one socket” intent already on master; the proxy now sits on that pool.Checklist