mirror of https://github.com/alexbelgium/hassio-addons.git synced 2026-10-04 07:21:36 +02:00

Files

Alexandre 556eca95d3 fix(sabnzbd): stop forwarding X-Forwarded-For, which ingress 403s on (#3023 )

Reported from a remote session: ingress answered
`403 External internet access denied - https://sabnzbd.org/access-denied`.

Root cause is `check_access()` in `sabnzbd/interface.py`:

    # Never check the XFF header unless access would have been granted
    # based on the remote IP alone!
    if is_allowed and cfg.verify_xff_header() and (xff_ips := ...):
        is_allowed = all(is_local_addr(ip) or is_loopback_addr(ip)
                         for ip in xff_ips)

nginx's own address is loopback, so the first test passes, and then every
address in X-Forwarded-For has to be local too. Supervisor puts the browser's
address in that header, so anyone reaching Home Assistant from outside the LAN
is refused. `verify_xff_header` defaults to on (`cfg.py:531`), so this is not a
configuration a user opted into.

Reproduced against the running add-on, `GET /config/general/`:

    no X-Forwarded-For                    200
    X-Forwarded-For: 81.164.12.7          403  External internet access denied
    X-Forwarded-For: 81.164.12.7, 172.30.32.2  403
    X-Forwarded-For: 192.168.1.44         200

which is why it worked on the LAN and not from outside. Verified the fix the
same way, running the shipped nginx.conf and ingress.conf in front of the live
add-on with both variants side by side: the current config 403s on a public
address, the fixed one answers 200 for all three chains, and redirects, static
roots and the API are unaffected. That instance has an empty `url_base`, so the
pass-through routing is now confirmed for both `url_base` values.

The header was forwarded because SABnzbd reads it — the wrong test, since what
it does with it is reject. Clearing it leaves SABnzbd looking at nginx's
loopback address, which is what it saw before the header was added; ingress is
gated by Home Assistant authentication before reaching this proxy either way.

The evidence.md entry records the methodology error, per the skill's own
feed-the-skill rule.

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

2026-08-25 16:15:13 +02:00

3.7 KiB

Raw Permalink Blame History

Evidence — measurement methodology and case studies

Why summed RSS and reserved-vs-resident both matter

Summed RSS double-counts shared pages. Removing a duplicate process frees its private memory, not its RSS. scripts/measure.sh reports PSS and private alongside RSS — quote private when arguing "removing this saves N MB".
A big mapping is not necessarily resident. Large SysV/tmpfs segments are lazily populated; reserved size is reported separately from resident for this reason.
/proc/meminfo and free show host figures (no memory cgroup namespace here) — never attribute those to the add-on.
Sample duration matters: a 3 s CPU sample measured 2.3% where a 20 s sample measured 21.6% for the same process. Use ≥20 s for anything you report.

Before asserting anything, ask what would show it false

"This process is duplicated" → is it? ps -ef --forest, compare parents and start times.
"This costs 500 MB" → is it resident? grep Rss /proc/<pid>/smaps_rollup — plain smaps prints one Rss: line per mapping (dozens of them), not a process total.
"This block never runs" → is its payload in the image? command -v, apt history.
"The flag isn't set" → tr '\0' '\n' < /proc/<pid>/cmdline.

When you correct yourself mid-analysis, keep the correction visible in your notes and in what you report — a retracted claim that stays retracted is worth more than one quietly dropped.

The failure mode this loop keeps producing

Every bug shipped from the source session came from one move: measuring this host correctly, then generalising it to all hosts.

/dev/shm was 7.7 GB here, so a flag looked useless — but Home Assistant ignores shm_size, so elsewhere it is Docker's 64 MB default and removing the flag reintroduces a crash loop.
An MCP entry was identified by its URL — but that URL is the documented default, so the rule would have deleted a user's hand-written configuration.
A GPU probe created a hardware context — but that proved the driver worked, not that Chromium's GPU path did.
SABnzbd's source was grepped to see which proxy headers it reads, and X-Forwarded-For was forwarded because it reads that one — but verify_xff_header is on by default and makes it reject every address in the chain that is not local, so ingress answered 403 for anyone reaching Home Assistant from outside the LAN (#3019, fixed in #3023). Every check ran from inside the container, where no such header exists. That an app reads a header is not a reason to send it — find out what it does with it, and exercise the path a remote user takes.

The pattern is always inference standing in for detection. Before changing a default, ask what this is like on a host unlike yours. Prefer detecting the condition at runtime over asserting it. When ownership matters, record it rather than infer it.

"Merged and inert" — CI passing proves the build works, not that the change does anything

Both changes in the session that produced this skill passed CI, merged, and were inert:

The Xvfb resolution cap wrote its env file correctly and Xvfb still started at the base-image default — wrong env mechanism for that service.
The GPU flags reached Chromium's command line exactly as intended, and the GPU process still reported --use-gl=disabled, having overridden them after its own init failed.

Once the rebuilt add-on is running, re-run the measurement that motivated the work. Some fixes cannot be self-verified — a service that reads its environment only at start makes an env-var fix unproven until the add-on restarts, which needs the user or ha-cli with their agreement. If you cannot restart, the change is Assumed, not Verified, and must be reported that way.

3.7 KiB Raw Permalink Blame History