feat: bind to 127.0.0.1 by default instead of 0.0.0.0#2812
Conversation
Port bindings now default to localhost-only, preventing prediction endpoints from being exposed to the entire network during development. - Add HostIP field to command.Port struct (defaults to 127.0.0.1) - Add --host flag to cog serve (default 127.0.0.1, use 0.0.0.0 to expose) - Support host:port syntax in cog run -p (e.g. -p 0.0.0.0:8888) - Bind cog predict/train to 127.0.0.1 - Update GetHostPortForContainer to match configured host IP
|
LGTM |
|
@anish-sahoo Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
Changes default port binding from Issues found:
|
|
I'm Bonk, and I've done a quick review of your PR. Summary: Changes default Docker port binding from Issues (highest severity first):
I posted the full review as a top-level PR comment because the batch review API returned a 422 (likely stale line numbers). You can view it here: #2812 (comment) |
michaeldwan
left a comment
There was a problem hiding this comment.
Review
Real security improvement — localhost-by-default is the right call, applied centrally at the bind site, with docs regenerated. One blocker and a few should-fix items below.
Blocker
cog exec -p lost its host:port override in the merge. Commit a4aec985 added host:port parsing (-p 0.0.0.0:8000) and a HostIP to pkg/cli/run.go. Main renamed cog run → cog exec (ba48ff8e); during the merge (88bde6bd) git couldn't carry those changes into the renamed exec.go, so the current loop is the old strconv.Atoi(portString) with no HostIP and no host:port syntax. Because docker.go now defaults empty HostIP to 127.0.0.1, cog exec -p 8888 jupyter notebook silently became localhost-only with no escape hatch — contradicting the commit message's stated intent ("Support host:port syntax in cog run -p"). exec.go isn't in this diff, so this can't be an inline comment — please restore the host:port parsing from the original run.go diff and update the --publish help text.
Should-fix (inline below)
docker.go— the core HostIP propagation change has no unit test.run.go— exact-stringHostIPmatch is brittle; fall back to the single binding when only one exists.command.go— extract a sharedDefaultHostIPconstant ("127.0.0.1" is duplicated in 5 sites).serve.go— remote-Docker (DOCKER_HOST=tcp://…) users get silent breakage; no warning.
Nits (inline below)
serve.go:20— package var namedhostis generic;serveHost/bindHostis clearer.serve.go:174— IPv6--hostvalues produce malformed display URLs (missing brackets);displayHostlogic is also untested.run_test.go— duplicated mock scaffolding, not table-driven; missing edge cases (custom IP, IPv6, inspect returning emptyHostIP).
cog predict/cog train hardcoding 127.0.0.1 is intended (local-only) and correct. pkg/registry_testhelpers using 0.0.0.0 directly via nat.PortBinding is correctly unaffected.
- Restore host:port parsing for `cog exec -p` lost in the cog run→exec rename; support plain ports, IPv4, bare and bracketed IPv6, with port range and empty-host validation - Extract shared command.DefaultHostIP constant (was duplicated) - Narrow GetHostPortForContainer fallback to single wildcard/empty bindings - Warn when serving via a remote Docker daemon; show a navigable localhost URL alongside 0.0.0.0; bracket IPv6 display hosts - Rename generic serve `host` var to `serveHost` - Add unit tests for port binding helpers, host parsing, remote-host detection, and serve URL formatting; make run_test table-driven - Regenerate docs/cli.md and docs/llms.txt
|
LGTM |
|
LGTM |
|
APIError: Bad Request: AiError: AiError: Invalid data for image - reason invalid base64 encoding (aad49186-90be-4eb7-8a90-c388fee5327e) |
|
APIError: Bad Request: AiError: AiError: Invalid data for image - reason invalid base64 encoding (67ef8603-4189-4822-9de4-14041877885e) |
Summary
0.0.0.0to127.0.0.1across all commands, preventing prediction endpoints from being accidentally exposed to the entire network during development.--hostflag tocog serveandhost:portsyntax tocog run -pso users can opt into binding to all interfaces when needed.Changes
pkg/docker/command/command.goHostIPfield toPortstructpkg/docker/docker.goport.HostIP(default127.0.0.1) instead of hardcoded""pkg/docker/run.goGetHostPortForContainernow accepts ahostIPparameterpkg/docker/run_test.gopkg/cli/serve.go--hostflag (default127.0.0.1)pkg/cli/run.go-pflag supportshost:portsyntax (e.g.-p 0.0.0.0:8888), defaults to127.0.0.1pkg/predict/predictor.gocog predict/cog trainbind to127.0.0.1User-facing behavior
cog serve0.0.0.0127.0.0.1. Use--host 0.0.0.0to expose.cog run -p 88880.0.0.0:8888127.0.0.1:8888. Use-p 0.0.0.0:8888to expose.cog predict/cog train0.0.0.0127.0.0.1Test plan