Close #11 #12 #13: loopback operator security, mode API cleanup, script walker consolidation#14
Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThis PR enforces loopback-only server binding checks (replacing header-based locality checks), consolidates file-walking into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
|
@cursor review |
Greptile SummaryThis PR consolidates three focused refactoring tasks with security hardening as the primary driver. #11 Loopback Security Hardening: Removed header-based access control for #12 Mode Resolver API Cleanup: Made #13 Script Consolidation: Created shared Confidence Score: 5/5
Important Files Changed
Flowchartflowchart TD
A[Server Startup] --> B{Dashboard Enabled?}
B -->|Yes| C[assertLoopbackHostname]
C --> D[Bind to 127.0.0.1 only]
D --> E[Dashboard Server Running]
F[Platform Worker Startup] --> G{HTTP Port Specified?}
G -->|Yes| H[assertLoopbackHostname]
H --> I[Bind to 127.0.0.1 only]
I --> J[Worker HTTP Running]
E --> K[Queue endpoints accessible]
J --> L[Events endpoint accessible]
K -.->|No header checks| M[Loopback binding is security boundary]
L -.->|No header checks| M
M --> N[Network isolation prevents remote access]
style C fill:#90EE90
style H fill:#90EE90
style M fill:#FFD700
style N fill:#87CEEB
Last reviewed commit: 40d1c61 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/platform-worker.ts (1)
276-282: Pre-existing:parseEnvelope(body)is called twice for the same request body.Line 277 calls
handleEnvelope(parseEnvelope(body))and line 278 callsparseEnvelope(body)again to check the command. Consider storing the envelope in a local variable to avoid the redundant parse.♻️ Suggested fix
+ const envelope = parseEnvelope(body); try { - const response = await handleEnvelope(parseEnvelope(body)); - if ((parseEnvelope(body).command ?? "event") === "shutdown") { + const response = await handleEnvelope(envelope); + if ((envelope.command ?? "event") === "shutdown") { setTimeout(() => { void shutdown(); }, 0);src/index.ts (1)
314-314: Nit: log message hardcodes127.0.0.1instead of usingdashboardHostname.For consistency with the new constant, consider using
dashboardHostnamein the log string.♻️ Suggested fix
- console.log(`[open-mem] Dashboard available at http://127.0.0.1:${port}`); + console.log(`[open-mem] Dashboard available at http://${dashboardHostname}:${port}`);scripts/utils/file-walk.ts (1)
13-14: Unnecessary Set→Array conversion; use the Set directly.
extensionSetis created for deduplication but then immediately spread back into an array. You can useextensionSetdirectly with.has()combined withpath.extname(), which is both clearer and avoids the intermediate allocation.♻️ Suggested simplification
+import { existsSync, readdirSync, statSync } from "node:fs"; +import { extname, join } from "node:path"; + export function walkFiles(rootDir: string, options: FileWalkOptions = {}): string[] { if (!existsSync(rootDir)) return []; const { extensions, ignoredDirNames = [] } = options; const extensionSet = extensions ? new Set(extensions) : null; - const extensionList = extensionSet ? [...extensionSet] : null; const ignoredDirs = new Set(ignoredDirNames); const files: string[] = []; const walk = (dir: string): void => { const entries = readdirSync(dir).sort((a, b) => a.localeCompare(b)); for (const entry of entries) { if (ignoredDirs.has(entry)) continue; const fullPath = join(dir, entry); const stat = statSync(fullPath); if (stat.isDirectory()) { walk(fullPath); continue; } - if (!extensionList || extensionList.some((ext) => fullPath.endsWith(ext))) { + if (!extensionSet || extensionSet.has(extname(fullPath))) { files.push(fullPath); } } };Note: this assumes extensions are passed in
.extformat (e.g.,".ts"), which is already the convention in all call sites.tests/scripts/file-walk.test.ts (1)
21-55: Consider adding a test forignoredDirNames.The test suite covers missing directories, extension filtering, and sort order, but the
ignoredDirNamesoption is untested. A quick test creating anode_modules-like directory and verifying it's skipped would round out coverage.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
1 participant
Summary
This PR closes follow-up issues #11, #12, and #13 with focused, production-quality changes that prioritize security boundaries and maintainability.
#11 Harden local-only operator route security model
/v1/queueand/v1/queue/processin favor of loopback listener boundary controls.#12 Reduce dead exports in mode resolver API surface
getDefaultModeConfig).#13 Consolidate duplicate file-walk utilities in architecture scripts
scripts/utils/file-walk.ts.Validation
bun run lintbun run typecheckbun run test:repobun run check:architecturebun run check:boundariesAll passed locally.
Commit structure
fix(http): enforce loopback boundary for operator routesrefactor(scripts): consolidate architecture file-walk utilityrefactor(modes): remove dead resolver exportsChecklist
Note
Medium Risk
Modifies the security boundary for operator queue endpoints by removing header-based gating and relying solely on loopback-only server binding; misconfiguration could unintentionally expose operator routes. Other changes are low-risk refactors with added test coverage.
Overview
Security model change: Removes header-based locality checks from operator queue endpoints (
/v1/queue,/v1/queue/process) and documents that these routes do not trustHost/X-Forwarded-For; enforcement is now via loopback-only listener binding.Adds a small loopback hostname guard (
assertLoopbackHostname/isLoopbackHostname) and applies it to the dashboard and platform-worker HTTP servers, with new tests validating loopback acceptance and that queue routes ignore header spoofing.Maintainability refactors: Consolidates architecture scripts’ recursive file walking into
scripts/utils/file-walk.ts(deterministic ordering + extension filtering) with tests, and reduces internal workflow mode resolver API surface by removing dead exports and tightening loader cache typing.Written by Cursor Bugbot for commit caae2fb. This will update automatically on new commits. Configure here.
Summary by CodeRabbit