Harden cookie service bind defaults#26
Conversation
protostatis
left a comment
There was a problem hiding this comment.
Sky's Code Review
This PR hardens the cookie service bind defaults by rejecting non-loopback binds unless --allow-remote-bind is explicitly passed. The change adds a Config.allow_remote_bind field (default False), a _is_loopback_bind_host() helper that handles IPv4, IPv6, bracketed IPv6, and localhost, and a preflight check in main() that exits with code 2 for non-loopback binds. Documentation (README.md, python/README.md, SKILL.md) and smoke tests are updated accordingly. Clean, focused security fix — the default-secure stance prevents accidental exposure of an unauthenticated endpoint that returns browser cookies.
Verdict: Approve
Comments
- The
_is_loopback_bind_hostpreflight is a configuration-level check, not a network-level guarantee — a bind to 0.0.0.0 with--allow-remote-bindis still accepted. This is fine since the opt-in is explicit, but worth noting in a post-merge comment. - The capabilities endpoint now uses
_is_loopback_bind_hostfor thelocal_onlyflag. This is informational for consumers and could be useful for automated routing decisions. - Smoke test coverage is adequate: tests both the helper function and the subprocess-level rejection. If CI doesn't always run
cargo build, the subprocess test may occasionally skip — consider whether that's acceptable. - Overall: focused, well-tested, well-documented. No blocking issues.
Reviewed by Sky — Unchained Sky engineering agent
Inline Comments (could not attach to lines)
scripts/cookie_service.py:73 — Good: default False is the secure choice. Consider whether an UNBROWSER_ALLOW_REMOTE_BIND env-var override would be useful alongside the existing --allow-private-network pattern, or intentionally omit it to force explicit opt-in.
scripts/cookie_service.py:96 — _is_loopback_bind_host is more robust than the previous in ("127.0.0.1", "localhost", "::1") — handles bracketed IPv6 and any loopback address. Minor note: the docstring/capability comment could call out that this is a bind-host check, not a DNS-resolved check.
scripts/cookie_service.py:547 — The --allow-remote-bind check fires before the binary existence check, which is good — fail fast on the security gate. Consider whether the error message should mention cfg.host so operators see exactly which bind host was rejected.
scripts/cookie_service_smoke.py:149 — Good subprocess test: validates both exit code 2 and the stderr message. If you also want to test the success path, add a case with --host 127.0.0.1 --quiet that exits before the binary check (exit 2 for missing binary is expected in CI without cargo build).
skills/unbrowser/SKILL.md:44 — Documentation is clear and consistent with the code change. The --allow-remote-bind is now surfaced as a required opt-in, which matches the new behavior.
README.md:245 — Wording is clearer than before. The sentence explaining the risk (/solve is unauthenticated and can return browser cookies) is direct and actionable.
Summary
Tests