Skip to content

Add --socket option to listen on a UNIX socket#1862

Open
vexofp wants to merge 3 commits into
esphome:mainfrom
vexofp:unix-socket
Open

Add --socket option to listen on a UNIX socket#1862
vexofp wants to merge 3 commits into
esphome:mainfrom
vexofp:unix-socket

Conversation

@vexofp

@vexofp vexofp commented Jul 5, 2026

Copy link
Copy Markdown

What does this implement/fix?

Add a command line option (--socket) that makes the primary web server listen on a specified UNIX socket path (instead of TCP). This restores a feature that was previously available in the legacy dashboard.

Related issue or feature (if applicable):

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Frontend coordination

  • No frontend change needed
  • Companion frontend PR: esphome/device-builder-frontend#

Checklist

  • The code change is tested and works locally.
  • Pre-commit hooks pass (ruff, codespell, yaml/json/python checks).
  • Tests have been added or updated under tests/ where applicable.
  • components.index.json / definitions/components/*.json have not been hand-edited (regenerate via script/sync_components.py if a sync is needed).
  • Architecture-level changes are reflected in docs/ARCHITECTURE.md and/or docs/API.md.

@vexofp

vexofp commented Jul 5, 2026

Copy link
Copy Markdown
Author

The option name (--socket) and behavior (overriding --host/--port) were chosen to mirror that of the legacy dashboard. The host/port options do still affect the mDNS advertisement, which seems useful if you point them at where a reverse proxy ultimately listens.

The current changes do not affect the HA ingress or remote build sites, since UNIX socket listening is mainly used with reverse proxies and seems less useful/applicable for those. If requested in the future, --ingress-socket/--remote-build-socket options could extend the pattern to those sites.

I did not find any existing tests that seemed related to the socket binding options, so I held off adding anything for now. Happy to add some if I can get a bit of direction as to the preferred location/approach.

@bdraco

bdraco commented Jul 5, 2026

Copy link
Copy Markdown
Member

Definitely needs some tests for this

@bdraco

bdraco commented Jul 5, 2026

Copy link
Copy Markdown
Member

@esphbot review

@esphbot

esphbot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

@vexofp: The option name (--socket) and behavior (overriding --host/--port) were chosen to mirror that ...

Direction on tests: use tests/test_ha_addon_failsafe.py. Mirror test_non_ha_addon_binds_public_site_normally. Patch device_builder.web.run_app with a fake_run_app. Assert path == <socket>, host == [], and resolve_bind_host not called. Cover both branches — standard and the add-on serve_public_unauthenticated path. Also document --socket in README/ARCHITECTURE; the ticked docs box has no diff behind it.

@esphbot

esphbot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

@bdraco: Definitely needs some tests for this

Agreed. Location: tests/test_ha_addon_failsafe.py. Reuse the fake_run_app capture pattern. Assert path forwarded, host empty, resolve_bind_host skipped. One case per run() branch.

@esphbot

esphbot commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

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.

Blocking issues found.

  • No test coverage for the --socket path (maintainer-requested)
  • New --socket flag is undocumented, but the docs checkbox is ticked

@codspeed-hq

codspeed-hq Bot commented Jul 5, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 27 untouched benchmarks


Comparing vexofp:unix-socket (06862af) with main (9bedc3d)

Open in CodSpeed

@codecov

codecov Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.58%. Comparing base (d1fd04c) to head (06862af).
⚠️ Report is 60 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1862      +/-   ##
==========================================
+ Coverage   99.56%   99.58%   +0.01%     
==========================================
  Files         234      239       +5     
  Lines       18982    19399     +417     
==========================================
+ Hits        18899    19318     +419     
+ Misses         83       81       -2     
Flag Coverage Δ
py3.12 99.55% <100.00%> (+0.02%) ⬆️
py3.14 99.58% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
esphome_device_builder/__main__.py 95.38% <ø> (+1.11%) ⬆️
...home_device_builder/controllers/config/settings.py 100.00% <100.00%> (ø)
esphome_device_builder/device_builder.py 98.03% <100.00%> (+0.01%) ⬆️

... and 53 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot added the new-feature New feature label Jul 5, 2026
@bdraco bdraco marked this pull request as draft July 6, 2026 14:20
@vexofp

vexofp commented Jul 9, 2026

Copy link
Copy Markdown
Author

Added additional documentation and UNIX socket case coverage to the relevant test_*_binds_public_* tests.

Did not try to tackle messing with the socket file permissions. (Which is a divergence from the legacy dashboard.) For one, it is poorly-supported / rather cumbersome under aiohttp (see aio-libs/aiohttp#4155). Also, the legacy behavior of making the socket world-writable was problematic, since that undermines one of the key security advantages of UNIX sockets and any work-around that later restricts the socket file suffers from a vulnerable race window. There are other ways of safely adding access for socket files (e.g. default ACLs). If there are requests for something like a --socket-mode option in the future, that seems like it could fit its own PR.

@vexofp vexofp marked this pull request as ready for review July 9, 2026 19:46
@vexofp vexofp requested a review from esphbot July 9, 2026 19:47
@bdraco

bdraco commented Jul 9, 2026

Copy link
Copy Markdown
Member

If you want to do an aiohttp PR, I can take a look

@esphbot

esphbot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@vexofp: Added additional documentation and UNIX socket case coverage to the relevant test_*_binds_public_*...

Agreed on skipping socket perms. World-writable defeats the socket boundary; post-bind chmod has a race window. Default ACLs or same-uid proxy are the safe paths. Fits a future --socket-mode PR.

One nit on the added tests: assert captured["host"] == [] if unix_socket else ["0.0.0.0"] binds == tighter than the ternary. The None branch asserts a truthy literal, so the TCP host check never runs. Parenthesize: == ([] if unix_socket else ["0.0.0.0"]).

@esphbot

esphbot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot left a comment

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.

Blocking issues found.

  • Operator precedence makes the TCP-path host assertion vacuous

@vexofp vexofp requested a review from esphbot July 9, 2026 20:09
@esphbot

esphbot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@bdraco: If you want to do an aiohttp PR, I can take a look

One aiohttp-adjacent gap here. UnixSite.start() calls create_unix_server without passing cleanup_socket, relying on the default. On Python 3.13+ asyncio removes a stale socket before bind and unlinks on clean close, so modern runtimes are covered. On the 3.12 floor neither exists — stale file, then EADDRINUSE on restart. An aiohttp-level knob for socket mode (or explicit stale-unlink) would help pre-3.13, else docs can note the floor.

@esphbot

esphbot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

PR Review — Add --socket option to listen on a UNIX socket

Clean, correctly-scoped feature; the prior operator-precedence blocker is now fixed and nothing else blocks merge.

Specific strengths:

  • The subtle bind logic is right and verified: host=[] (not None) suppresses TCP binding in aiohttp's _run_app while path= adds the UnixSite, so no unintended TCP port is opened, and --host/--port stay live for mDNS advertisement.

  • The front-door banner %s%s%s formatting renders the socket path alone vs host:port correctly for both branches.

  • The prior blocker is resolved — all three parametrized asserts now use == ([] if unix_socket else ["0.0.0.0"]) with the parentheses, restoring the real host check on the TCP path.

  • Docs section added covering auth/Origin gates staying active and the mDNS/reverse-proxy precedence.

  • Non-blocking: stale-socket cleanup is handled by asyncio on Python ≥3.13 but likely not on the declared 3.12 floor (unverified) — worth confirming or documenting.



Checklist

  • Bind logic correct (no unintended TCP port; host=[] suppresses TCP, path= adds UnixSite)
  • Prior operator-precedence blocker resolved (parenthesized ternary)
  • Front-door banner reports socket vs host:port correctly
  • Auth / Origin gates unchanged; no new security hole (default umask restricts socket to owner)
  • Stale-socket / cleanup handled across supported Python floor
  • Docs updated for CLI flag and trust boundary

Automated review by Kōan (Claude) HEAD=06862af 5 min 27s

@esphbot esphbot left a comment

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.

No blocking issues found.

@bdraco bdraco left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please update THREAT_MODEL.md to explain the default permissions are not a bug and not something we defend

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new-feature New feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants