fix(security): address PR #404 review feedback

[0xSolace]

Summary

Addresses security review comments from PR #404 (dev→main merge).

HIGH SEVERITY

• SSRF bypass: Expanded private bridge IP validation to include all RFC1918 ranges (10.x, 172.16-31.x, 192.168.x) in addition to existing CGNAT (100.64.x)
• Dev CORS in prod: Gated localhost CORS origins behind NODE_ENV !== 'production'
• Rate limiting: Added withRateLimit(handler, RateLimitPresets.STRICT) to /api/auth/pair
• Cross-org sandbox: Scoped sandbox lookup in pairing endpoint to org via findByIdAndOrg
• Timing side-channel: Replaced plain !== with timingSafeEqual for internal token comparison

MEDIUM SEVERITY

• Backup ownership: Added backup.sandbox_record_id === rec.id check before restore
• Token in URL: Added security comment documenting the Redis fallback risk
• Stale balance: Added getOrgBalance() refresh after warning/shutdown billing actions

LOW SEVERITY

• Typo imports: Fixed milaidy → milady imports in 7 source files (shim kept for test compat)

Deferred

• Add Claude Code GitHub Workflow #9 (env-var node capacity check) — existing warning is adequate, needs design discussion
• feat: Multi-Image Generation with Style Presets and Aspect Ratio Controls #10 (billing_status CHECK constraint) — would require new migration, low risk

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
eliza-cloud-v2	Ready	Preview, Comment	Mar 23, 2026 4:36pm

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3cc3ff18-d64a-4427-b190-d2c334504d59

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/pr404-review-feedback

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[claude]

PR #405 Review

Overall this is a solid security hardening PR with clear fixes for real vulnerabilities. A few issues worth addressing before merge:

---

Bug: "Backup does not belong to this agent" returns 500

In app/api/v1/milady/agents/[agentId]/restore/route.ts, the new error message added in milady-sandbox.ts is not mapped to an appropriate HTTP status:

const status =
  result.error === "Agent not found"       ? 404
  : result.error === "No backup found"     ? 404
  : result.error === "Stopped agents..."   ? 409
  : 500;  // ← "Backup does not belong to this agent" falls here

"Backup does not belong to this agent" indicates a cross-agent access attempt and should return 403, not 500. A 500 also risks leaking stack traces or confusing clients into thinking it's a server fault.

---

Security: Length pre-check leaks token length before timingSafeEqual

In app/api/agents/[id]/headscale-ip/route.ts:

const tokensMatch =
  providedToken.length === expectedToken.length &&   // ← leaks length via short-circuit
  timingSafeEqual(Buffer.from(providedToken), Buffer.from(expectedToken));

The length check short-circuits before the constant-time comparison, leaking whether the attacker has the correct token length via timing. The standard fix is to hash both values first so lengths are always equal:

const { createHmac } = await import("node:crypto");
const hmacKey = Buffer.from(expectedToken);
const a = createHmac("sha256", hmacKey).update(providedToken).digest();
const b = createHmac("sha256", hmacKey).update(expectedToken).digest();
const tokensMatch = timingSafeEqual(a, b);

This is a low-severity finding in practice (token length leakage provides minimal oracle value), but worth fixing given the intent is constant-time comparison.

---

Migration concern: 0056_add_billing_status_check.sql locks table under load

ALTER TABLE "milady_sandboxes" ADD CONSTRAINT "billing_status_check"
  CHECK (billing_status IN ('active', 'warning', 'shutdown_pending', 'suspended'));

This acquires ACCESS EXCLUSIVE and performs a full table scan to validate existing rows before the constraint is applied. On a live table this will block all reads/writes for the duration. The safe pattern is:

-- Step 1: add without validating existing rows (no table scan, shorter lock)
ALTER TABLE "milady_sandboxes" ADD CONSTRAINT "billing_status_check"
  CHECK (billing_status IN ('active', 'warning', 'shutdown_pending', 'suspended')) NOT VALID;

-- Step 2: validate in a separate migration (ShareUpdateExclusiveLock, non-blocking)
ALTER TABLE "milady_sandboxes" VALIDATE CONSTRAINT "billing_status_check";

---

Nit: Test file name still has the milaidy typo

packages/tests/unit/milaidy-sandbox-bridge-security.test.ts — the imports inside were fixed but the filename itself still has the old typo. Worth renaming for consistency.

---

Nit: _journal.json missing newline at EOF

Both 0048_02_elite_rumiko_fujikawa_alters.sql and meta/_journal.json are missing a trailing newline. Minor but causes noisy diffs in some tools.

---

What looks good

• SSRF fix: RFC1918 expansion (10.x, 172.16-31.x, 192.168.x, 100.64-127.x) is correct and complete.
• Dev CORS gating: NODE_ENV !== 'production' guard is the right approach.
• Cross-org sandbox scoping in /api/auth/pair: findByIdAndOrg is the correct fix.
• Rate limiting on /api/auth/pair via RateLimitPresets.STRICT is appropriate.
• Backup ownership check: The backup.sandbox_record_id !== rec.id guard in milady-sandbox.ts is correct — just needs the HTTP status fix above in the route layer.
• Removing hasLegacyDockerSandboxId path closes the SSRF bypass cleanly, and the removed test case validates this.
• SSH pool cap (50 + LRU eviction): Well-implemented, prevents unbounded resource growth.
• Cache-unavailable → hard 503: Converting the URL token leak fallback to a thrown error is the right security call.
• Migration split: Breaking 0048_elite_rumiko_fujikawa into 4 targeted files follows the project's migration guidelines correctly.

🤖 Generated with Claude Code

[0xSolace]

Consolidated into PR #403 (cherry-picked commits onto fix/steward-security-migrations)