fix(provider): bind dev oauth and custom-server to localhost by default#287
Conversation
0f43d5e to
52f6f3d
Compare
Coverage Report for CI Build 25577691033Coverage increased (+0.6%) to 84.827%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
ba9ee13 to
0455866
Compare
provider/dev_provider.go and provider/custom_server.go used Addr ":port",
which listens on every interface. The dev OAuth UI and the embedded
go-oauth2/oauth2 helper are intended for local development and embedded
flows, not for LAN exposure — but the bare-port shorthand silently
exposed them to anyone who could route to the host's other addresses
(other LAN clients, Docker network siblings, kube pods, etc).
Add localBindAddr(host, port) in provider/service.go that defaults the
listen host to 127.0.0.1 when the caller passes an empty host. Callers
that explicitly want non-loopback exposure can pass a hostname:
* dev_provider — uses Provider.Host (existing field) as the host;
empty -> 127.0.0.1, explicit "0.0.0.0"/"192.168.x.y" honored.
* custom_server — uses the host from c.URL ("http://localhost:8080");
"localhost" or empty -> 127.0.0.1; explicit non-loopback honored.
Same change in v1 and v2 in single PR.
Tests in both modules:
* TestLocalBindAddr — table for empty/explicit-loopback/explicit-LAN/
explicit-0.0.0.0/IPv6 cases.
* TestLocalBindAddr_DefaultIsNotAllInterfaces — regression test for
the property "default bind never produces ':port' shorthand".
Confirmed locally that reverting localBindAddr to "return ':' + port"
makes this test fail with a clear message.
0455866 to
c6c83bb
Compare
umputun
left a comment
There was a problem hiding this comment.
lgtm. Nice catch on the bundled deadlock fix in CustomServer.Run: pre-existing, c.lock was being held through the URL-parse and SplitHostPort error returns, which would block Shutdown(). The regression test TestCustomServer_Run_BadURLDoesNotDeadlockShutdown is well-shaped.
one tiny note: the host == "localhost" normalization in custom_server.go:97 only normalizes the literal string. 127.0.0.1 and [::1] baked into c.URL would skip that branch, but they then go through localBindAddr → net.JoinHostPort, which produces a valid loopback bind. So no functional issue; just observing that the explicit "localhost" branch is needed only because localBindAddr treats empty as the default-loopback signal.
Summary
provider/dev_provider.goandprovider/custom_server.golistened withAddr: ":port", which binds to every interface. The dev OAuth UI and the embedded go-oauth2/oauth2 helper are intended for local development and embedded flows, not for LAN exposure -- but the bare-port shorthand silently exposed them to anyone who could route to the host's other addresses (other LAN clients, Docker network siblings, kubernetes pods on the same node, etc.).Change
Add
localBindAddr(host, port)inprovider/service.gothat defaults the listen host to127.0.0.1when the caller passes an empty host. Callers that explicitly want non-loopback exposure can pass a hostname:Provider.Host(existing field) as the host; empty ->127.0.0.1, explicit"0.0.0.0"/"192.168.x.y"honored.c.URL(http://localhost:8080);"localhost"or empty ->127.0.0.1; explicit non-loopback honored.Same change in v1 and v2 in single PR.
README updated with a "Listen address" section documenting:
AddDevProvider("0.0.0.0", port)for dev;URL: "http://0.0.0.0:port"for custom server);127.0.0.1inside a container is not reachable from the host, so0.0.0.0is the right value when the auth process runs in a container.Also fixed an outdated
AddDevProvider(port)reference in the README -- the function takes(host, port).Test
TestLocalBindAddr-- table covering empty / explicit-loopback / explicit-LAN / explicit-0.0.0.0/ IPv6.TestLocalBindAddr_DefaultIsNotAllInterfaces-- asserts the default bind never produces the":port"shorthand. Reverting the helper toreturn ":" + portmakes this test fail with:Should not be: ":12345" -- default bind must not be all-interfaces.Both modules;
go test -race ./...green;golangci-lint run --new-from-rev=master0 issues.Migration note
Existing deployments that intentionally exposed the dev OAuth helper or custom server beyond loopback need to set
Provider.Host = "0.0.0.0"(dev) or change theirOpts.URLhost to0.0.0.0/specific IP (custom-server). Default behaviour change is the security-relevant point of this PR.