fix(integrations): warn when bearer auth crosses plaintext HTTP to non-loopback#315
Conversation
|
@mvanhorn is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds plaintext-HTTP Bearer token safety detection and enforcement across Pi (TypeScript), OpenClaw (JavaScript), and Hermes (Python). When AGENTMEMORY_SECRET would be sent over plaintext HTTP to non-loopback hosts, the code warns once or raises an error if AGENTMEMORY_REQUIRE_HTTPS=1. Tests and docs are updated accordingly. ChangesPlaintext Bearer Auth Safety Guards
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/integration-plaintext-http.test.ts`:
- Line 7: Update the test import to match the ES module extension convention:
replace the import of createPlaintextBearerAuthGuard from
"../integrations/pi/security.ts" with "../integrations/pi/security.js" so the
test uses the same `.js` import convention as the source (ensure the imported
symbol createPlaintextBearerAuthGuard remains unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e0da6aa-1900-41d9-aebe-17a4567293fb
📒 Files selected for processing (5)
integrations/hermes/__init__.pyintegrations/openclaw/plugin.mjsintegrations/pi/index.tsintegrations/pi/security.tstest/integration-plaintext-http.test.ts
…n-loopback (rohitg00#275) Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
…alike hostnames + docs @mvanhorn's PR rohitg00#315 lands the right plaintext-bearer-auth warn-once pattern across three runtimes (hermes/python, openclaw/mjs, pi/typescript). Reviewed cleanly. Adding test cases + docs before merge so the contract is locked: Test additions (test/integration-plaintext-http.test.ts pi suite, +4 cases, 13 total in suite): - IPv6 loopback (http://[::1]:3111) — URL parser strips brackets, hostname comes back as `::1`, the LOOPBACK_HOSTS set already covers this. Test pins the parsing semantic so a future refactor can't silently break it. - Private LAN IPs (RFC1918 ranges 192.168.x.y, 10.x.x.x) — NOT loopback, guard MUST warn. RFC1918 was a deliberate design choice (loopback set stays minimal: localhost / 127.0.0.1 / ::1). LAN deployment behind a tunnel still leaks the bearer over the wire. - No-secret short-circuit (guard with secret="" or undefined) — guard never fires when a bearer wouldn't actually be sent. Mirrors the usesPlaintextBearerAuth() early-return. - Lookalike hostname (http://localhost.evil.com:3111) — `parsed.hostname` returns the full `localhost.evil.com` string, which is NOT in LOOPBACK_HOSTS. Guard warns. This pins the exact-match (vs prefix) loopback semantic. Docs additions: - integrations/pi/README.md and integrations/hermes/README.md now document AGENTMEMORY_REQUIRE_HTTPS in the env-vars table — same row shape as AGENTMEMORY_SECRET, names the env value (=1), describes the warn-vs-throw semantic, and lists the loopback hostnames the guard recognises. No code changes to the guard itself — it's already correct. Pre-merge hardening only. 899 / 899 tests pass on rebased branch. Build clean.
80f5580 to
9875c3d
Compare
|
Shipped in v0.9.12 (release notes). Thanks @mvanhorn — symmetric three-runtime guard (Python / mjs / TypeScript), warn-once semantic, and the AGENTMEMORY_REQUIRE_HTTPS=1 escalation knob is exactly the right shape for hardened deployments. Hardening commits we layered before merge (IPv6 / LAN-IP / lookalike-hostname edge-case tests + env-var docs in pi/hermes README) preserved every line of your design — the guard logic itself didn't need a change. |
|
Thanks @rohitg00 - really appreciate the merge and the extra hardening pass. The IPv6 / LAN-IP / lookalike-hostname coverage tightens the policy in exactly the gaps a single-loopback-string check would miss; good additions. Glad the env-var escape hatch landed cleanly for the hardened-deployment path. |
|
Thanks for your contribution and whatever you do for community, I use /last30days daily. |
|
Thanks @rohitg00, really glad /last30days is in your rotation. The bearer-over-HTTP warn was a fun one to track down; reach out anytime if there's something else useful to add on the integrations side. |
|
@rohitg00 thanks for the merge. Warning when a bearer token would hit non-loopback plaintext HTTP is exactly the kind of polite hardening that prevents accidental exposure without breaking the dev-loop on localhost. |
Summary
Each of the three plugin clients now warns once when a bearer token would be sent over plaintext HTTP to a non-loopback host. Loopback HTTP stays silent (it's not actually wire-exposed). HTTPS stays silent. An opt-in
AGENTMEMORY_REQUIRE_HTTPS=1env var turns the warning into a startup error for users who want a guarantee.What changed
integrations/openclaw/plugin.mjs: HTTP + non-loopback + secret -> one-timeconsole.warn. Hard-fail whenAGENTMEMORY_REQUIRE_HTTPS=1.integrations/pi/index.ts+ newintegrations/pi/security.ts: same guard, factored into a small module so the test can target it directly.integrations/hermes/__init__.py: same guard usingwarnings.warn+RuntimeErrorfor the hard-fail path.test/integration-plaintext-http.test.ts: covers loopback silence, remote HTTP warning fires once, HTTPS silence, hard-fail when env is set.Default behavior is unchanged. The warning fires only when the configuration is actually dangerous (HTTP + non-loopback + secret).
Why not raise by default
http://localhost:3111is the documented default and is loopback-only — the kernel routes those packets without touching any NIC. Hard-failing every plaintext-localhost user would break the install flow for no security gain.Testing
node --checkon the modified.mjsfilepython3 -m py_compileon the modified Python pluginnpm testcouldn't run locally becausevitestisn't in the sandbox; the new test follows the existingtest/*.test.tsshape, CI/CodeRabbit will exercise itFixes #275
Signed-off-by: Matt Van Horn mvanhorn@gmail.com
Summary by CodeRabbit
New Features
Tests
Documentation
Chore