Repository navigation
Make SABnzbd's bind address survive a restart, and assert the control that matters - #69
Merged
ivan-pinatti merged 6 commits intoAug 18, 2026
Conversation
The 5.0.4 bump on #61 turned the suite red on test_sabnzbd_config_disables_ipv6: the running config read `host = ::` where the seeded ini says `0.0.0.0`. SABnzbd is not the culprit. Its DEF_HOST is 127.0.0.1 and validate_host accepts `0.0.0.0` unchanged. linuxserver's start script is: FAMILY=:: if ! test -f /proc/net/if_inet6; then FAMILY=0.0.0.0; fi exec ... SABnzbd.py --config-file /config --server "$FAMILY" `--server` overrides the config file and SABnzbd persists it back, so our value cannot survive a start. `/proc/net/if_inet6` exists here because gluetun keeps IPv6 sysctls enabled for its own DNS listener, which docs/HARDENING.md already records as deliberate. Reported upstream twice, linuxserver/docker-sabnzbd#116 and #240, and closed NOT_PLANNED as intended: forcing it is what stops someone binding 127.0.0.1 and locking themselves out of the web UI. So the test now accepts either value for `host` and keeps `ipv6_hosting` and `ipv6_servers` asserted strictly. Those two are untouched by `--server` and they are what actually governs IPv6: server selection and the [::1] listener. The bind address was never the control here anyway, since the listener sits in gluetun's namespace, which is assigned no IPv6 address and is firewalled. Rejected: disabling IPv6 in that namespace so the container picks 0.0.0.0, which would break gluetun's DNS listener, and vendoring a custom start script, which is a permanent fork of upstream's launcher to control a value that protects nothing in this topology.
docker-compose-vpn.yml says IPv6 is left enabled in gluetun's namespace because its DNS server binds [::]:53, and that leaks are prevented by no interface holding an IPv6 address plus gluetun's ip6tables rules. Nothing asserted the first half of that. It is also the property the SABnzbd bind address was standing in for, and the better one to test. The bind address belongs to linuxserver's start script, so asserting it proved only what upstream felt like doing, and a listener on `::` reaches nothing while no IPv6 address exists in the namespace. If a provider ever hands one out inside the tunnel, this fails and the bind address becomes worth arguing about again, which the old test would not have caught. Read from /proc/net/if_inet6 rather than `ip -6 addr`, since the image may not carry iproute2, and only scope 00 counts: link-local and host addresses are not routable.
Defense in depth for the value the image overwrites. The shipped service script passes `--server ::` on every start, `--server` beats the config file, and SABnzbd persists the override back into sabnzbd.ini, so `host = 0.0.0.0` cannot survive a container start. That is what turned the 5.0.4 bump red. patches/sabnzbd/svc-sabnzbd/run replaces the script and narrows the condition: the family address is still imposed when the configured host is missing or loopback, so nobody can bind 127.0.0.1 and lock themselves out of the web UI, which is the case upstream said it was protecting when it closed #240. A reachable value is left alone. Verified against the decision table: empty, localhost, 127.* and ::1 still get --server, while 0.0.0.0, :: and a LAN address are respected. This is not the control that matters here, and the patch is not load bearing. test_vpn_namespace_has_no_global_ipv6_address asserts the property docker-compose-vpn.yml actually relies on, no interface in gluetun's namespace holding a routable IPv6 address, so the bind address is belt rather than braces and the test still accepts either value. docs/TODO.md carries the condition to check before deleting the patch. The file is a complete GPL-3.0 file from linuxserver/docker-sabnzbd. patches/ README.md records it with its license, which is the per-directory breakdown NOTICE.md already points at.
The first version grepped the file for `^host` and took the first match. The NNTP server entries under [servers] carry their own unindented `host = ...` line, so a reordered ini would have handed a news server's hostname to --server. Demonstrated: on a file with [servers] before [misc], the old expression returns news.example.net. Section tracking fixes it, verified against four shapes: [misc] first, [servers] first, a loopback value, and no host key at all. The patch now also matches the change being proposed upstream line for line, passing a configured host back through --server rather than dropping the flag, so this file becomes a no-op if linuxserver takes it.
Filed linuxserver/docker-sabnzbd#274 as a feature request rather than a bug, because the bug framing is what #240 was closed under, with the fix as #275. The vendored patch is identical to that pull request, so it becomes a no-op if the change ships. The TODO now also records how often this has been raised, #2 in 2016, #35 in 2017, #42 in 2018, #116 in 2021 and #240 in 2024, all closed. Whoever reads this next should know the odds before spending time chasing it.
|
Warning Review limit reached
Next review available in: 14 minutes Limit details: You’ve used all 3 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
Comment |
…ess-is-the-containers-to-choose
ivan-pinatti
marked this pull request as ready for review
August 18, 2026 16:14
Collaborator
Author
|
/run-tests |
ivan-pinatti
deleted the
fix/sabnzbd-bind-address-is-the-containers-to-choose
branch
August 18, 2026 18:08
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The 5.0.4 bump on #61 went red on
test_sabnzbd_config_disables_ipv6: the running config readhost = ::where the seeded ini says0.0.0.0. This is the whole investigation and the fix.SABnzbd is not at fault
DEF_HOSTis127.0.0.1insabnzbd/constants.pyandvalidate_hostinsabnzbd/cfg.pyaccepts0.0.0.0unchanged. The container is doing it, inroot/etc/s6-overlay/s6-rc.d/svc-sabnzbd/run:--serveroverrides the config file and SABnzbd persists it back, so our value cannot survive a start./proc/net/if_inet6exists here because gluetun keeps IPv6 sysctls enabled for its own DNS listener, whichdocker-compose-vpn.yml:70already records as deliberate.Three changes
1. Assert the control that actually protects us.
docker-compose-vpn.yml:70says leaks are prevented by "no IPv6 addresses assigned to any interface" plus gluetun's ip6tables rules, and nothing asserted the first half.test_vpn_namespace_has_no_global_ipv6_addressnow reads/proc/net/if_inet6in that namespace and fails on any scope00address. This is strictly more coverage than before: the old test would have passed happily while a routable IPv6 address sat on the tunnel.2. Accept either bind address in the test. It belongs to the container, not to us, and a listener on
::reaches nothing while change 1 holds.ipv6_hostingandipv6_serversstay asserted strictly, since--serverdoes not touch them and they are what governs server selection and the[::1]listener.3. Vendor the run script anyway, as belt.
patches/sabnzbd/svc-sabnzbd/runnarrows the condition: the family address is still imposed when the configured host is missing or loopback, so nobody can bind127.0.0.1and lock themselves out, which is the case upstream said it was protecting. A reachable value is respected.Upstream
Raised as linuxserver/docker-sabnzbd#274 with the fix as #275, framed as a feature request because the bug framing is what #240 was closed under. The patch is identical to that PR, so it becomes a no-op if the change ships.
docs/TODO.mdcarries the removal condition.Expectations should be low. The same behavior was reported in #2 (2016), #35 (2017), #42 (2018), #116 (2021) and #240 (2024), and all five were closed, #240 as
NOT_PLANNEDwith "That's the intended behavior".One defect worth calling out
My first version of the patch read the host with
sed -n 's/^host.*//p' | head -1. The NNTP server entries under[servers]carry their own unindentedhost = ...line, so on a reordered ini that returnsnews.example.netand hands it to--server. Fixed with section-aware extraction, verified against four ini shapes.