fix(deploy): terminate TLS in front of services via Caddy reverse proxy (#747)

[frankbria]

Closes #747

Problem

Frontend (next start -H 0.0.0.0) and backend (HOST=0.0.0.0) bound all
interfaces over plaintext http:///ws://, and remote-setup.sh installed no
reverse proxy. On the shared VPS, JWT bearer tokens and API keys travelled in
cleartext on every interface.

Change

App processes now bind loopback only; a TLS-terminating Caddy reverse
proxy is the sole public listener. Because the frontend already supports
same-origin operation (NEXT_PUBLIC_API_URL || ''), Caddy path-routes a single
https:// origin — no CORS, no plaintext leaving the host.

Process	Binds	Public
Backend	127.0.0.1:14200	no
Frontend	127.0.0.1:14100	no
Caddy	:80, :443	yes

• ecosystem.{staging,production}.config.js — frontend binds 127.0.0.1
• .env.{staging,production}.example — HOST=127.0.0.1; public origins are
  https:///wss:// (new .env.production.example)
• deploy/Caddyfile.example — automatic Let's Encrypt TLS; routes
  /api,/auth,/ws,/health,/docs → backend, everything else → Next.js;
  WebSocket upgrades proxied transparently
• scripts/remote-setup.sh — installs Caddy; firewall exposes only 80/443 and
  denies the loopback-only app ports
• deploy/README.md — setup + routing

The existing CSP (security-headers.js) already derives connect-src from the
NEXT_PUBLIC_* env at build time, so it picks up the https/wss origins with
no change.

Acceptance criteria

• A TLS-terminating reverse proxy fronts the services; app processes bind 127.0.0.1.
• Provisioning sets up the proxy; documented origins are https:///wss://.

Testing

• tests/test_deploy_config.py extended: loopback binding (both ecosystems +
  env HOST), https/wss origins, Caddyfile routing to both loopback ports,
  and proxy provisioning in remote-setup.sh. caddy validate runs when caddy
  is installed (skipped otherwise).
• Verified deploy/Caddyfile.example with caddy validate (Caddy 2 container):
  Valid configuration, auto HTTP→HTTPS redirect + TLS policy enabled.

Known limitations

• server.py's CLI --host default stays 0.0.0.0 (local dev / codeframe serve
  convenience); the deploy env pins HOST=127.0.0.1. Production binding is
  enforced by config, not the code default.
• Caddyfile.example uses the staging ports (14100/14200); production defaults
  (3000/8000) are noted in a comment to adjust.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 11 minutes

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4deaf0eb-9c28-490c-b7ac-935497cdaa9a

📥 Commits

Reviewing files that changed from the base of the PR and between 0273994 and 53b3529.

📒 Files selected for processing (8)

• .env.production.example
• .env.staging.example
• deploy/Caddyfile.example
• deploy/README.md
• ecosystem.production.config.js
• ecosystem.staging.config.js
• scripts/remote-setup.sh
• tests/test_deploy_config.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/747-tls-reverse-proxy

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Review: fix(deploy): terminate TLS via Caddy reverse proxy (#747)

Solid piece of work overall — the loopback-binding + Caddy design is the right shape (same-origin, no CORS, auto-TLS renewal), the docs are clear, and the new tests actually assert the security-relevant properties (loopback binding, https/wss origins, Caddyfile routing, firewall denial of the app ports) rather than just checking file existence. No prior reviews exist on this PR yet (only a rate-limited CodeRabbit placeholder with no content), so this is the first substantive pass.

Critical: the automated deploy pipeline still writes 0.0.0.0 config, undoing this fix

.github/workflows/deploy.yml (deploy-staging and deploy-production jobs, "Create environment file" steps) generates the actual remote .env.staging / .env.production file from GitHub Secrets on every push to main / every release — independent of the .env.staging.example / .env.production.example templates this PR edits. That generated file writes:

API_HOST=${ENV_API_HOST}
API_PORT=${ENV_API_PORT}

but the backend's actual bind-address logic (codeframe/ui/server.py:800, default=os.environ.get("HOST", "0.0.0.0")) reads HOST, not API_HOST. API_HOST only maps to Settings.api_host (codeframe/core/config.py:575), a field that's never read anywhere (grep -rn "\.api_host\b" returns nothing) — it's dead config. So:

• This PR's fix only takes effect for the manual remote-setup.sh → hand-edited .env.staging/.env.production path.
• The CI/CD auto-deploy path, which is presumably the live mechanism deploying dev.codeframeapp.com and production on every push/release, never sets HOST at all. pm2 start ecosystem.staging.config.js loads that env file via dotenv, finds no HOST key, and the backend falls back to its 0.0.0.0 default — silently re-exposing the exact plaintext-on-all-interfaces issue [P1.20] Terminate TLS in front of the services (they bind 0.0.0.0 over plaintext HTTP/ws) #747 describes, on the very next automated deploy after this merges.

Suggest adding a HOST=127.0.0.1 line (or renaming API_HOST → HOST throughout, being careful not to collide with the already-used secrets.HOST which is the SSH target, not the app bind address) to both "Create environment file" steps in deploy.yml. Given the PR's own test suite (tests/test_deploy_config.py) already asserts loopback binding for the ecosystem configs and .env.*.example templates, it'd be worth extending it (or adding a companion assertion) to catch deploy.yml regenerating a non-loopback HOST, since that's the actual deployment path in production use.

Should-fix: reverse proxy breaks per-IP rate limiting (including auth brute-force protection)

codeframe/lib/rate_limiter.py:get_client_ip() only trusts X-Forwarded-For/X-Real-IP when the direct connection comes from an IP listed in RATE_LIMIT_TRUSTED_PROXIES (codeframe/core/config.py:604, default "" — empty). After this change, every request's direct connection is Caddy at 127.0.0.1. Neither .env.staging.example nor .env.production.example (nor the pre-existing .env.example) sets RATE_LIMIT_TRUSTED_PROXIES, so post-deploy, get_client_ip() will treat 127.0.0.1 as untrusted, ignore the forwarded headers (logging a warning per request), and fall back to the direct IP — meaning every client collapses into a single 127.0.0.1 rate-limit bucket. Concretely: RATE_LIMIT_AUTH=10/minute (brute-force protection on login) becomes a shared 10/minute budget across all users behind the proxy — one user's failed logins (or an attacker's) can lock out everyone else, and the per-client isolation this limiter is designed for is gone.

Recommend adding RATE_LIMIT_TRUSTED_PROXIES=127.0.0.1 to both new/edited env templates and a line in deploy/README.md, since Caddy's reverse_proxy sets X-Forwarded-For automatically and the app already has the trusted-proxy mechanism built for exactly this case — it just needs to be turned on for the new topology this PR introduces.

Minor

• scripts/remote-setup.sh: the loop sudo ufw delete allow ${port}/tcp 2>/dev/null || true — ufw delete normally prompts for interactive y/n confirmation, and the || true means a declined/failed delete (leaving 14100/14200 exposed) is swallowed silently with no message to the operator. Worth at least echoing a warning when the delete doesn't succeed, since the whole point of this loop is closing an exposed port.
• scripts/remote-setup.sh end-of-script pointer See docs/REMOTE_STAGING_DEPLOYMENT.md for full guide (line ~272, not modified by this diff) is already stale — that file lives under legacydocs/ now. Since this PR adds deploy/README.md as the real source of truth and touches the surrounding "Next Steps" text anyway, consider repointing this line while you're in the file.

Nice

• The .env.production.example addition and staging template edits are consistent with each other and with deploy/Caddyfile.example / deploy/README.md.
• Good call documenting the known limitation that server.py's CLI --host default stays 0.0.0.0 for local-dev convenience — that's an honest, scoped disclosure rather than silently leaving it inconsistent.
• Test additions are meaningfully tied to the security property (loopback bind, https/wss origins, Caddyfile routing, firewall denial), not just presence checks, and gracefully skip caddy validate when Caddy isn't installed.

[frankbria]

Third-party review (opencode / GLM) + triage

Ran an independent review against the actual codebase. It verified routing is correct (frontend calls /api/v2/*, /auth/*, and wss://…/ws/sessions/{id}/chat, all covered by the Caddy path matcher; WS upgrades proxy transparently) and surfaced four findings — all addressed in 5d8ee70:

Sev	Finding	Resolution
Major	Backend started without --host → loopback binding depended on a mutable HOST env var; a stale HOST=0.0.0.0 would keep the backend exposed.	Pin --host explicitly in both ecosystems (staging 127.0.0.1; production HOST const defaulting to 127.0.0.1). Added test_backend_binds_loopback.
Major	Behind TLS termination the app sees client 127.0.0.1, collapsing per-client/auth-brute-force rate limits.	The app already has its own trusted-proxy mechanism, so documented RATE_LIMIT_TRUSTED_PROXIES=127.0.0.1 in the env examples. Deliberately did NOT enable uvicorn proxy_headers — that would rewrite request.client and bypass the app's own get_client_ip spoof protection (codeframe/lib/rate_limiter.py). Verified before fixing.
Minor	tls internal trips an untrusted-CA error.	Noted caddy trust / accept-warning in Caddyfile + README.
Minor	README "no CORS needed" could mislead an operator into blanking CORS_ALLOWED_ORIGINS.	Clarified: same-origin, but keep the var set.

Demo — TLS proxy end-to-end (caddy validate + live)

caddy validate on deploy/Caddyfile.example: Valid configuration (auto HTTP→HTTPS redirect + TLS policy enabled). Live run (Caddy tls internal fronting loopback stub services):

Request	Result
https://localhost/ (TLS on :443)	200, scheme HTTPS
/ → frontend 127.0.0.1:14100	✓ routed
/api/health, /auth/x → backend 127.0.0.1:14200	✓ routed
http://localhost/	308 → https (auto-redirect)

Both acceptance criteria demonstrated with outcome evidence.

[claude]

Follow-up review (after 5d8ee70)

Checked this against my prior review and the opencode/GLM triage in the thread above, so I won't re-litigate what's already resolved:

• RATE_LIMIT_TRUSTED_PROXIES — confirmed wired correctly. Settings.rate_limit_trusted_proxies (codeframe/core/config.py:604) feeds get_client_ip() in codeframe/lib/rate_limiter.py, and both .env.staging.example/.env.production.example now set RATE_LIMIT_TRUSTED_PROXIES=127.0.0.1. Good — this closes the rate-limit-collapse gap I flagged.
• My earlier "deploy.yml still writes API_HOST not HOST" critical — deploy.yml itself is unchanged (still emits API_HOST=${ENV_API_HOST}, never HOST=), but it's now moot in practice: both ecosystem.production.config.js (const HOST = process.env.HOST || envConfig.HOST || '127.0.0.1') and ecosystem.staging.config.js (hardcoded --host 127.0.0.1) default to loopback regardless of what's in the generated env file. That's a reasonable defense-in-depth fix, arguably better than patching deploy.yml's naming mismatch directly.

New finding — the CI/CD redeploy path won't actually pick up the new --host/-H args on an already-running service

.github/workflows/deploy.yml branches on is_process_online:

• staging (line 218-219): pm2 restart "${BACKEND_NAME}" --update-env / same for frontend
• production (line 545, 554): pm2 restart "${BACKEND_NAME}" / same for frontend
• only the not-online branch does pm2 start ecosystem.*.config.js (which reads the updated args field)

pm2 restart <name> restarts the process in place using the args it was originally launched with — it does not re-read args from the ecosystem file. --update-env only refreshes the env object from the current shell/config, not the args string. Since staging/production almost always have the backend and frontend already online at deploy time (that's the normal continuous-deploy case), the next merge-triggered deploy after this PR lands will hit pm2 restart <name>, not pm2 start ecosystem.*.config.js — so the new --host 127.0.0.1 / -H 127.0.0.1 args this PR adds will silently not apply to the live processes. They'd keep running with whatever host arg they were last started with (which, pre-PR, was 0.0.0.0 for the frontend and no explicit --host for the backend).

The fix only takes effect for a genuinely fresh pm2 start, i.e. first-time provisioning or a manual pm2 delete <name> && pm2 start ecosystem.*.config.js on the existing host. Given the whole point of this PR is closing a live-cleartext-exposure issue on the running VPS, this is worth calling out explicitly (either as a one-time manual remediation step in deploy/README.md/the PR description, or by changing deploy.yml to pm2 restart ecosystem.*.config.js / pm2 reload ecosystem.*.config.js so it re-reads args on every deploy — the latter is more robust against this recurring for any future ecosystem-file arg change).

Still open from my earlier pass (minor, unaddressed by 5d8ee70)

• scripts/remote-setup.sh:242 — sudo ufw delete allow ${port}/tcp 2>/dev/null || true: ufw delete normally requires an interactive y/n confirmation; in a non-interactive SSH heredoc this can silently no-op, leaving the port exposed with no operator-visible warning.
• scripts/remote-setup.sh:300 — still points to docs/REMOTE_STAGING_DEPLOYMENT.md, which lives under legacydocs/ now; worth repointing to the new deploy/README.md while touching this section.

Neither is a regression from this PR, but both are in code this PR is actively editing.

Everything else

No new concerns on the Caddyfile routing, TLS/tls internal guidance, or test additions — tests/test_deploy_config.py still correctly asserts the loopback-binding/https-origin/firewall properties, and the demo evidence in the thread (caddy validate + live 200/308 routing) is solid corroboration for the reverse-proxy behavior itself. The one gap is the deploy-pipeline's ability to actually roll the new bind args out to an already-running host, as above.

[claude]

Follow-up review (after 53b3529)

Only change since my last pass is 53b35292 (docs pointer fix). Confirmed it does what it says — scripts/remote-setup.sh:284,300 now point at deploy/README.md instead of the stale docs/REMOTE_STAGING_DEPLOYMENT.md. No new issues introduced by that commit.

Spot-checked one thing I hadn't verified yet: .env.staging.example/.env.production.example dropped the /ws suffix from NEXT_PUBLIC_WS_URL (now wss://your-domain instead of .../ws). That's correct, not a regression — AgentTerminal.tsx:75 and useAgentChat.ts:144 both treat the env var as a bare origin and append /ws/sessions/${id}/... themselves, and the Caddyfile's @backend matcher covers /ws/*. Matches what the opencode/GLM triage upthread already verified for routing.

Still open from my prior review (unaddressed, not a regression from this PR but in code it actively edits)

• deploy.yml won't roll the new bind args onto an already-running service. Unchanged in this PR — pm2 restart <name> (staging .github/workflows/deploy.yml:218-219, production :545,554) restarts in place with whatever args the process was originally launched with; only the not-yet-online branch does pm2 start ecosystem.*.config.js, which is the one that re-reads args. Staging/production are normally already online at deploy time, so the very next merge-triggered deploy after this lands will restart the existing process instead of relaunching it — the new --host 127.0.0.1/-H 127.0.0.1 won't apply to the live host until someone does a one-time pm2 delete <name> && pm2 start ecosystem.*.config.js (or deploy.yml moves to pm2 restart ecosystem.*.config.js/pm2 reload, which re-reads args every time). Worth either a one-time manual remediation note in deploy/README.md or the deploy.yml change, since the whole point of this PR is closing a live cleartext-exposure issue on a host that's presumably already running.
• scripts/remote-setup.sh:242 — sudo ufw delete allow ${port}/tcp 2>/dev/null || true: ufw delete <rule> normally requires an interactive y/n confirmation on stdin; in a non-interactive run (piped/SSH heredoc) that prompt has nothing to answer, ufw errors out, and || true swallows it silently — the port can stay exposed with no operator-visible warning that the "cleanup" didn't happen.

Everything else already covered across the thread stands: loopback binding, the RATE_LIMIT_TRUSTED_PROXIES wiring, Caddyfile routing/tls internal guidance, and the test additions in tests/test_deploy_config.py are all solid and unchanged since the last pass.