Security Hardening: Path Traversal Prevention, Log Sanitization, and Code Quality Fixes#8299
Security Hardening: Path Traversal Prevention, Log Sanitization, and Code Quality Fixes#8299NiloyTheSpd wants to merge 13 commits intoKilo-Org:mainfrom
Conversation
…5-layer config cascade Phase 1: Layered Config & Context System - Add KiloMdResolver.ts - implements 3-tier KILO.md loading (user, project, directories) - Add Config.Layer enum and Config.loadWithLayers() function - Inject KILO.md content into system prompt at session initialization in llm.ts - Fix broken custom-elements.d.ts symlink on Windows The KILO.md system: 1. User tier: ~/.kilo/KILO.md 2. Project tier: ./KILO.md 3. Directory tier: walk up from CWD collecting KILO.md files 4. 40,000 character injection limit with truncation The 5-layer config cascade: policy → flag → local → project → user BREAKING CHANGE: None, all changes are additive and backward compatible
…-settings ordering Phase 2: Permission Model Hardening - Add PermissionMode enum: bypass, allow_edits, auto - Add AutoPermissionClassifier with heuristic classification - Add TrustGate system with .kilo-trusted marker file - Add --permission-mode CLI flag and env var - Integrate mode resolution into PermissionNext.ask() flow The 3 permission modes: - bypass: No permission checks (current --auto behavior) - allow_edits: Auto-approve file edits in CWD, ask for everything else - auto: LLM classifier scores each action for risk (initial heuristic implementation) Fix for GHSA-mmgp-wc2j-qcv7 analogous vulnerability: - TrustGate prevents malicious kilo.json from auto-approving permissions before user trust is established - Untrusted directories ignore project-level permission rules All changes are backward compatible and additive.
…scade Phase 1 from GAP_ANALYSIS.md: - Add KiloMdResolver for user/project/directory KILO.md resolution - Implement actual 5-layer config cascade in loadWithLayers() - Wire KILO.md into InstructionPrompt.system() for agent context injection - Add 12 unit tests for KiloMdResolver (all passing) - Respect 40K char injection limit with truncation
…-settings ordering Phase 2 from GAP_ANALYSIS.md: - Add mode field (bypass/allow_edits/auto) to Config.Permission schema - Complete TrustGate to strip project permissions for untrusted directories - Enhance AutoPermissionClassifier with context-aware bash command patterns - Add --permission-mode CLI flag to run command - Add 19 unit tests for permission mode, trust gate, and classifier
…are flags Phase 3 from GAP_ANALYSIS.md: - Add --max-turns flag to cap autonomous agent loop iterations - Add --allowed-tools flag for per-invocation tool whitelist - Add --output-format flag (text/json/stream-json) for CI pipelines - Add --bare flag to skip plugin triggers for headless mode - Create StreamFormatter for structured output modes - Add max_turns to Session.CloseReason enum - Add 8 unit tests for StreamFormatter
…upport Phase 4 from GAP_ANALYSIS.md: - Add hook schema to Config.Info (command/url/timeout/continue_on_error/env) - Create HookRunner for fire-and-forget command and HTTP hook execution - Create HookValidator for security validation (dangerous commands, URL checks) - Integrate hooks into agent loop at turn, step, and tool boundaries - Add 12 unit tests for HookValidator
…allel execution Phase 5 from GAP_ANALYSIS.md: - Create SubagentSpec with dependency-aware wave building - Create SubagentRunner for spawning subagents with context isolation - Support parallel execution via Promise.all and wave-based dependency resolution - Add Bus events for subagent spawned/completed lifecycle - Add timeout support per subagent spec
…and context compaction Phase 6 from GAP_ANALYSIS.md: - Create MemoryType with 4 memory types (project_facts, user_preferences, task_context, episodic) - Create MemoryStore with SQLite-backed load/save/relevant/clear operations - Create ContextCompactor for injecting relevant memories when context approaches limit - Add MemoryTable to SQLite schema with project/session/type indexes - Add unit tests for MemoryType schema validation
Phase 7 from GAP_ANALYSIS.md: - Create McpConfig loader for standalone .kilo/mcp.json files - Support both native Config.Mcp format and legacy mcpServers format - Integrate into config cascade at project directory level - Merge with global MCP config using deep merge precedence
Phase 8 from GAP_ANALYSIS.md: - Add publishConfig.provenance: true to published package.json - npm provenance already enabled via NPM_CONFIG_PROVENANCE env var - --provenance flag already passed to npm publish commands - id-token: write permission already set in publish workflow
…review - Fix 1: SubagentRunner allowed_tools — deny all non-listed tools via permission rules - Fix 2: SubagentRunner timeout — cancel child session on timeout via SessionPrompt.cancel() - Fix 3: SubagentRunner extractResult — use p.text instead of p.content for TextPart - Fix 4: SubagentSpec — add id field, track dependencies by ID not description - Fix 5: McpConfig — use jsonc-parser for JSONC file support - Fix 6: llm.ts — remove duplicate KILO.md injection (already in InstructionPrompt.system) - Fix 7: prompt.ts — normalize allowed-tools to Set for exact matching - Extra: Remove unused imports (MemoryStore like, StreamFormatter log, TrustGate in next.ts) - Extra: Fix processor.ts ToolError payload — use toolName not toolCallId - Extra: Wire tool.before/tool.after hooks in processor.ts - Extra: Fix TrustGate to not strip all permission rules (log warning instead)
… numbers - SEC-001: Add isPathWithinAllowedDirs() guard to skill/agent removal endpoints preventing path traversal via arbitrary location input - CODE-001: Fix empty catch block in kilo-errors.ts to re-throw unexpected errors (only swallow SyntaxError from JSON.parse) - CODE-002: Replace magic numbers (5000, 3000) with named constants GRACEFUL_SHUTDOWN_TIMEOUT_MS and HEALTH_CHECK_TIMEOUT_MS - SEC-005: Add sanitizeForLog() to claw/hooks.ts to redact sensitive keys (apiKey, token, secret, password, authorization) before logging - SEC-002: Verified session-import routes are protected by server-level basic auth middleware (server.ts:94-102) - SEC-003: Verified migrators only read from known-safe system paths (KilocodePaths, os.homedir, startup projectDir) — no vulnerability Co-Authored-By: Kilo AI <ai@kilocode.dev>
|
@NiloyTheSpd is attempting to deploy a commit to the Kilo Code Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Pull request overview
This PR hardens Kilo-specific CLI/VSCode behavior based on a security audit by adding path containment checks for destructive endpoints, reducing sensitive logging, and tightening/expanding the permission + hook lifecycle infrastructure. It also introduces new supporting capabilities (hooks, subagent orchestration, memory storage, KILO.md context injection, output formatting) and aligns tests with updated compaction semantics.
Changes:
- Add path validation for
/kilocode/skill/remove, log redaction helper for claw chat, and safer error parsing inkilo-errors.ts. - Introduce new hook runner/validator + permission modes (auto/allow_edits/bypass), max-turn loop control, and allowed-tools filtering.
- Add new “memory” and output-formatting utilities (StreamFormatter/OutputStyles), MCP config loading, KILO.md context resolver, and subagent orchestration helpers.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/opencode/test/session/compaction.test.ts | Update tests for new isOverflow() return shape |
| packages/opencode/test/output/stream-formatter.test.ts | Add tests for StreamFormatter output formats |
| packages/opencode/test/memory/memory-type.test.ts | Add schema tests for MemoryType |
| packages/opencode/test/kilocode/permission/phase2.test.ts | Add tests for trust + permission mode + classifier |
| packages/opencode/test/hook/hook-validator.test.ts | Add tests for HookValidator validation rules |
| packages/opencode/test/context/kilomd-resolver.test.ts | Add tests for context-tier KILO.md resolution/injection |
| packages/opencode/src/util/path-safety.ts | New allowed-dir containment helper |
| packages/opencode/src/tool/task.ts | Add fork + parallel subagent execution paths |
| packages/opencode/src/session/session.sql.ts | Add fork_from_id session column |
| packages/opencode/src/session/prompt.ts | Hook events, max-turns, compaction mode plumbing, allowed-tools filter |
| packages/opencode/src/session/processor.ts | Fire hook events around model/tool lifecycle |
| packages/opencode/src/session/message-v2.ts | Add compaction mode field in schema |
| packages/opencode/src/session/llm.ts | Minor comment tweak |
| packages/opencode/src/session/instruction.ts | Inject system KILO.md into instruction list |
| packages/opencode/src/session/index.ts | Persist/read forkFromID; extend turn-close reason |
| packages/opencode/src/session/compaction.ts | Add micro/reactive compaction behavior + hooks |
| packages/opencode/src/server/routes/kilocode.ts | Add path traversal rejection for skill removal |
| packages/opencode/src/plugin/index.ts | Skip plugin triggers in bare mode |
| packages/opencode/src/permission/next.ts | Add permission modes + auto classifier |
| packages/opencode/src/output/StreamFormatter.ts | New output formatter for text/json/SSE |
| packages/opencode/src/output/OutputStyles.ts | Load/apply output style templates from disk |
| packages/opencode/src/memory/MemoryType.ts | New memory entry/type schemas |
| packages/opencode/src/memory/MemoryStore.ts | DB-backed memory CRUD + relevance scoring |
| packages/opencode/src/memory/memory.sql.ts | New drizzle table definition for memory |
| packages/opencode/src/memory/ContextCompactor.ts | Inject relevant memory into system prompt near context limit |
| packages/opencode/src/mcp/McpConfig.ts | Load MCP config from project directories (json/jsonc) |
| packages/opencode/src/kilocode/permission/TrustGate.ts | Trust marker file checks + config loading behavior |
| packages/opencode/src/kilocode/permission/PermissionMode.ts | New permission mode parsing/resolution helpers |
| packages/opencode/src/kilocode/permission/AutoPermissionClassifier.ts | Heuristic allow/deny classifier for tools/bash |
| packages/opencode/src/kilocode/KiloMdResolver.ts | Duplicate KILO.md resolver implementation (kilocode namespace) |
| packages/opencode/src/kilocode/kilo-errors.ts | Fix empty catch: only swallow JSON parse errors |
| packages/opencode/src/kilocode/claw/hooks.ts | Add log sanitization wrapper for credential logging |
| packages/opencode/src/hook/HookValidator.ts | New hook config validator (events/commands/urls/timeouts) |
| packages/opencode/src/hook/HookRunner.ts | New hook execution engine (command + HTTP) |
| packages/opencode/src/flag/flag.ts | Add Kilo env flags (modes/max-turns/tools/bare/channels/etc.) |
| packages/opencode/src/context/KiloMdResolver.ts | Context-tier resolver used by instruction prompt |
| packages/opencode/src/config/config.ts | Add layered config loader + hook/enterprise/compaction schema fields |
| packages/opencode/src/cli/cmd/run.ts | Add CLI options that set new env flags |
| packages/opencode/src/agents/SubagentSpec.ts | Add subagent spec + wave builder |
| packages/opencode/src/agents/SubagentRunner.ts | Add subagent spawning / parallel/wave execution |
| packages/opencode/src/agent/agent.ts | Add coordinator agent mode + definition |
| packages/opencode/script/publish.ts | Add publishConfig provenance/access |
| packages/kilo-vscode/src/services/cli-backend/server-manager.ts | Replace magic shutdown timeout with constant |
| packages/kilo-vscode/src/services/cli-backend/connection-service.ts | Replace magic health timeout with constant |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fireCommand(hookConfig, name, payload), | ||
| fireHttp(hookConfig, name, payload), | ||
| ]) | ||
|
|
There was a problem hiding this comment.
HookRunner.fire() uses Promise.allSettled() but never inspects/rethrows rejected results, so hook failures are effectively swallowed even when continue_on_error is false. This breaks the intended contract of stopping the loop on hook failure. Consider using Promise.all() (or explicitly rethrowing the first rejection) so continue_on_error is honored.
| if (!hookConfig.continue_on_error) { | |
| const rejectedResult = result.find( | |
| (entry): entry is PromiseRejectedResult => entry.status === "rejected", | |
| ) | |
| if (rejectedResult) { | |
| throw rejectedResult.reason | |
| } | |
| } |
| const commandResult = result[0] | ||
| if (hookConfig.async_rewake && commandResult.status === "fulfilled") { | ||
| const code = await Process.spawn(["sh", "-c", hookConfig.command!], { | ||
| env: { | ||
| ...process.env, | ||
| ...hookConfig.env, | ||
| KILO_HOOK_EVENT: name, | ||
| KILO_HOOK_PAYLOAD: JSON.stringify(payload), | ||
| }, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| timeout: hookConfig.timeout ?? 5000, | ||
| }).exited | ||
| if (code === 2) { | ||
| return { asyncRewake: true } | ||
| } | ||
| } |
There was a problem hiding this comment.
async_rewake currently re-runs the hook command a second time (via a separate Process.spawn) to read an exit code, so commands will execute twice whenever async_rewake is enabled. Also, because the code uses hookConfig.command!, configs with async_rewake: true but no command will crash at runtime. Consider having fireCommand() return the exit code (or structured result) and using that single execution to decide whether to rewake.
| async function fireCommand(hook: HookConfig, name: string, payload: Record<string, unknown>): Promise<void> { | ||
| if (!hook.command) return | ||
|
|
||
| const timeout = hook.timeout ?? 5000 | ||
| const env = { | ||
| ...process.env, | ||
| ...hook.env, | ||
| KILO_HOOK_EVENT: name, | ||
| KILO_HOOK_PAYLOAD: JSON.stringify(payload), | ||
| } | ||
|
|
||
| try { | ||
| const proc = Process.spawn(["sh", "-c", hook.command], { | ||
| env, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| timeout, | ||
| }) | ||
|
|
There was a problem hiding this comment.
Hook commands are executed via sh -c, which will fail on Windows environments. If Windows support is expected, consider using a cross-platform shell strategy (e.g. cmd.exe /c on win32) or executing commands without a shell when possible.
| const ALLOWED_HOOK_EVENTS = new Set([ | ||
| "turn.start", "turn.end", | ||
| "step.start", "step.end", | ||
| "tool.before", "tool.after", "tool.error", | ||
| "error", "compaction.end", | ||
| ]) | ||
|
|
||
| export function validate(config: Config.Info): boolean { | ||
| if (!config.hook) return true | ||
|
|
||
| for (const [name, hook] of Object.entries(config.hook)) { | ||
| if (!ALLOWED_HOOK_EVENTS.has(name)) { | ||
| log.warn("unknown hook event", { name }) | ||
| return false | ||
| } |
There was a problem hiding this comment.
HookValidator rejects any hook name not in ALLOWED_HOOK_EVENTS, but HookRunner supports a wildcard hook ("*") and fires additional events (e.g. loop.iteration, model.request, subagent.*, etc.). As written, valid/expected hook configs (including "*") will be rejected. Consider adding "*" and the full set of HookRunner.Event names to the allowlist (or deriving the allowlist from HookRunner.Event).
| // kilocode_change start — force "ask" for config file edits | ||
| const protected_ = ConfigProtection.isRequest(request) | ||
| // kilocode_change end | ||
|
|
||
| // kilocode_change start - auto permission classifier | ||
| const mode = PermissionMode.resolve((await Config.get()).permission?.mode as string | undefined, Flag.KILO_PERMISSION_MODE) | ||
|
|
||
| if (mode === "auto") { | ||
| const decision = await AutoPermissionClassifier.classify({ | ||
| tool: request.permission, | ||
| input: request.metadata, | ||
| pattern: request.patterns[0] ?? "*", | ||
| }) | ||
|
|
||
| if (decision.confidence >= 0.9) { | ||
| if (decision.action === "allow") { | ||
| log.info("auto allowed permission", { tool: request.permission, confidence: decision.confidence }) | ||
| return | ||
| } | ||
| if (decision.action === "deny") { | ||
| throw new DeniedError([]) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (mode === "allow_edits") { | ||
| // Auto-allow edit tools in CWD | ||
| if (["edit", "write", "patch", "multiedit"].includes(request.permission)) { | ||
| log.info("auto allowed edit permission", { tool: request.permission }) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| if (mode === "bypass") { | ||
| // Bypass all permission checks | ||
| log.warn("permission bypass enabled", { tool: request.permission }) | ||
| return | ||
| } | ||
| // kilocode_change end |
There was a problem hiding this comment.
In PermissionNext.ask, the new allow_edits / bypass early-returns happen before the config-path protection override. This means edits to protected config paths can be auto-approved (or entirely bypass checks) and never reach the protected_ → ask logic. If the intent is to always require confirmation for config file edits, move the mode shortcuts below the protected_ handling (or explicitly gate them when protected_ is true).
| const timeout = new Promise<never>((_, reject) => | ||
| setTimeout(async () => { | ||
| await SessionPrompt.cancel(childSession.id) | ||
| reject(new Error(`Subagent timed out after ${spec.timeout_ms}ms`)) | ||
| }, spec.timeout_ms), | ||
| ) | ||
| result = await Promise.race([promptPromise.then(() => extractResult(childSession.id)), timeout.then(() => ({ | ||
| output: `Subagent timed out after ${spec.timeout_ms}ms`, | ||
| files: [], | ||
| cost: 0, | ||
| tokens: { input: 0, output: 0 }, | ||
| sessionID: childSession.id, | ||
| status: "timeout" as const, | ||
| }))]) |
There was a problem hiding this comment.
The timeout branch in spawn() tries to map the timeout promise into a { status: "timeout" } result via timeout.then(...), but timeout is constructed to reject, so that .then() handler is never reached and Promise.race rejects instead. If you want a structured timeout result, have the timeout promise resolve to that result (or catch the rejection) so the function returns status: "timeout" deterministically.
| const timeout = new Promise<never>((_, reject) => | |
| setTimeout(async () => { | |
| await SessionPrompt.cancel(childSession.id) | |
| reject(new Error(`Subagent timed out after ${spec.timeout_ms}ms`)) | |
| }, spec.timeout_ms), | |
| ) | |
| result = await Promise.race([promptPromise.then(() => extractResult(childSession.id)), timeout.then(() => ({ | |
| output: `Subagent timed out after ${spec.timeout_ms}ms`, | |
| files: [], | |
| cost: 0, | |
| tokens: { input: 0, output: 0 }, | |
| sessionID: childSession.id, | |
| status: "timeout" as const, | |
| }))]) | |
| const timeout = new Promise<Result>((resolve) => | |
| setTimeout(async () => { | |
| await SessionPrompt.cancel(childSession.id) | |
| resolve({ | |
| output: `Subagent timed out after ${spec.timeout_ms}ms`, | |
| files: [], | |
| cost: 0, | |
| tokens: { input: 0, output: 0 }, | |
| sessionID: childSession.id, | |
| status: "timeout" as const, | |
| }) | |
| }, spec.timeout_ms), | |
| ) | |
| result = await Promise.race([promptPromise.then(() => extractResult(childSession.id)), timeout]) |
| const defaults: PermissionNext.Ruleset = [ | ||
| { permission: "todowrite", pattern: "*", action: "deny" }, | ||
| { permission: "todoread", pattern: "*", action: "deny" }, | ||
| { permission: "task", pattern: "*", action: "deny" }, | ||
| ] | ||
|
|
||
| if (spec.allowed_tools && spec.allowed_tools.length > 0) { | ||
| defaults.push({ permission: "*", pattern: "*", action: "deny" }) | ||
| for (const tool of spec.allowed_tools) { | ||
| defaults.push({ permission: tool, pattern: "*", action: "allow" }) | ||
| } | ||
| } | ||
|
|
||
| return PermissionNext.merge( | ||
| defaults, | ||
| PermissionNext.fromConfig({ | ||
| read: "allow", | ||
| edit: "allow", | ||
| write: "allow", | ||
| patch: "allow", | ||
| multiedit: "allow", | ||
| bash: "allow", | ||
| glob: "allow", | ||
| grep: "allow", | ||
| list: "allow", | ||
| webfetch: "allow", | ||
| websearch: "allow", | ||
| codesearch: "allow", | ||
| lsp: "allow", | ||
| skill: "allow", | ||
| question: "allow", | ||
| }), | ||
| ) |
There was a problem hiding this comment.
allowed_tools restrictions are not effective: when allowed_tools is set, buildRestrictedPermissions() adds a { permission: "*", action: "deny" }, but then appends PermissionNext.fromConfig({...}) with many allow rules. Since permission evaluation uses findLast, these later allows override the earlier deny, so the subagent can still use tools outside allowed_tools. Consider applying the allowlist rules last (or removing the broad allow rules when allowed_tools is provided).
| const defaults: PermissionNext.Ruleset = [ | |
| { permission: "todowrite", pattern: "*", action: "deny" }, | |
| { permission: "todoread", pattern: "*", action: "deny" }, | |
| { permission: "task", pattern: "*", action: "deny" }, | |
| ] | |
| if (spec.allowed_tools && spec.allowed_tools.length > 0) { | |
| defaults.push({ permission: "*", pattern: "*", action: "deny" }) | |
| for (const tool of spec.allowed_tools) { | |
| defaults.push({ permission: tool, pattern: "*", action: "allow" }) | |
| } | |
| } | |
| return PermissionNext.merge( | |
| defaults, | |
| PermissionNext.fromConfig({ | |
| read: "allow", | |
| edit: "allow", | |
| write: "allow", | |
| patch: "allow", | |
| multiedit: "allow", | |
| bash: "allow", | |
| glob: "allow", | |
| grep: "allow", | |
| list: "allow", | |
| webfetch: "allow", | |
| websearch: "allow", | |
| codesearch: "allow", | |
| lsp: "allow", | |
| skill: "allow", | |
| question: "allow", | |
| }), | |
| ) | |
| const restrictedRules: PermissionNext.Ruleset = [ | |
| { permission: "todowrite", pattern: "*", action: "deny" }, | |
| { permission: "todoread", pattern: "*", action: "deny" }, | |
| { permission: "task", pattern: "*", action: "deny" }, | |
| ] | |
| if (spec.allowed_tools && spec.allowed_tools.length > 0) { | |
| restrictedRules.push({ permission: "*", pattern: "*", action: "deny" }) | |
| for (const tool of spec.allowed_tools) { | |
| restrictedRules.push({ permission: tool, pattern: "*", action: "allow" }) | |
| } | |
| } | |
| const configuredRules = PermissionNext.fromConfig({ | |
| read: "allow", | |
| edit: "allow", | |
| write: "allow", | |
| patch: "allow", | |
| multiedit: "allow", | |
| bash: "allow", | |
| glob: "allow", | |
| grep: "allow", | |
| list: "allow", | |
| webfetch: "allow", | |
| websearch: "allow", | |
| codesearch: "allow", | |
| lsp: "allow", | |
| skill: "allow", | |
| question: "allow", | |
| }) | |
| if (spec.allowed_tools && spec.allowed_tools.length > 0) { | |
| return PermissionNext.merge(configuredRules, restrictedRules) | |
| } | |
| return PermissionNext.merge(restrictedRules, configuredRules) |
| export async function spawn(spec: SubagentSpec.Spec, parentSessionID: string): Promise<Result> { | ||
| const agent = await Agent.get(spec.subagent_type) | ||
| if (!agent) throw new Error(`Agent "${spec.subagent_type}" not found`) | ||
|
|
||
| const config = await Config.get() | ||
| const parentMsgs: MessageV2.WithParts[] = [] | ||
| for await (const msg of MessageV2.stream(parentSessionID)) { | ||
| parentMsgs.push(msg) | ||
| } | ||
| const parentAssistant = parentMsgs.findLast((m) => m.info.role === "assistant") as MessageV2.Assistant | undefined | ||
| const parentModel = parentAssistant | ||
| ? { modelID: parentAssistant.modelID, providerID: parentAssistant.providerID } | ||
| : undefined | ||
|
|
||
| const resolved = spec.model | ||
| ? spec.model | ||
| : agent.model | ||
| ? agent.model | ||
| : parentModel | ||
| ? parentModel | ||
| : await Provider.defaultModel() | ||
|
|
||
| const model = await Provider.getModel(resolved.providerID, resolved.modelID) | ||
|
|
There was a problem hiding this comment.
spawn() reads const config = await Config.get() and const model = await Provider.getModel(...), but neither value is used. If they’re not needed, removing them will reduce overhead and avoid confusion; if they are intended for future use (e.g., default permissions/max turns), consider wiring them in now or adding a TODO explaining the intent.
| .references(() => ProjectTable.id, { onDelete: "cascade" }), | ||
| workspace_id: text(), | ||
| parent_id: text(), | ||
| fork_from_id: text(), | ||
| slug: text().notNull(), | ||
| directory: text().notNull(), |
There was a problem hiding this comment.
Schema adds the fork_from_id column, but there is no corresponding SQL migration in packages/opencode/migration/ (searching for fork_from_id returns none). Since the runtime applies migrations from that directory, existing installs will have a session table without this column, causing inserts/updates to fail. Please add a migration that alters the session table to include fork_from_id.
| export const MemoryTable = sqliteTable( | ||
| "memory", | ||
| { | ||
| id: text().primaryKey(), | ||
| project_id: text().notNull().references(() => ProjectTable.id, { onDelete: "cascade" }), | ||
| session_id: text().references(() => SessionTable.id, { onDelete: "set null" }), | ||
| content: text().notNull(), | ||
| type: text().notNull(), | ||
| private: integer({ mode: "boolean" }).notNull().default(false), | ||
| created_at: integer().notNull(), | ||
| updated_at: integer().notNull(), | ||
| }, | ||
| (table) => [ | ||
| index("memory_project_idx").on(table.project_id), | ||
| index("memory_type_idx").on(table.type), | ||
| index("memory_session_idx").on(table.session_id), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
A new memory table is introduced, but there is no SQL migration creating it under packages/opencode/migration/. Since the runtime uses drizzle migrations (it does not auto-create tables), this will break at runtime when MemoryStore queries/inserts. Please add a migration to create the memory table (and indexes) to match this schema.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| } | ||
|
|
||
| export const KILO_SESSION_RETRY_LIMIT = number("KILO_SESSION_RETRY_LIMIT") | ||
| export const KILO_PERMISSION_MODE = process.env["KILO_PERMISSION_MODE"] |
There was a problem hiding this comment.
WARNING: These flags are evaluated too early
RunCommand.handler() sets process.env["KILO_*"] at runtime, but these exports are plain constants, so their values are captured when flag.ts is first imported. In the normal CLI flow that means --permission-mode, --max-turns, --allowed-tools, --bare, and the other new options never affect the current process. These need access-time getters like KILO_DISABLE_PROJECT_CONFIG, or the call sites need to read process.env directly.
| ]) | ||
|
|
||
| const commandResult = result[0] | ||
| if (hookConfig.async_rewake && commandResult.status === "fulfilled") { |
There was a problem hiding this comment.
WARNING: async_rewake runs the hook command twice
fireCommand() already executed hook.command in the Promise.allSettled() call above. Spawning the same command again here duplicates any side effects for every async_rewake hook, and the rewake decision is based on the second run rather than the original one.
|
|
||
| if (mode === "allow_edits") { | ||
| // Auto-allow edit tools in CWD | ||
| if (["edit", "write", "patch", "multiedit"].includes(request.permission)) { |
There was a problem hiding this comment.
CRITICAL: Protected config edits are auto-approved in allow_edits mode
protected_ is computed a few lines above specifically to force config-path edits back through the prompt, but this early return happens before that guard runs. In allow_edits mode, edits to .kilo/, .opencode/, kilo.json, and similar protected files will now skip the approval step entirely.
| if (params.parallel && params.parallel.length > 0) { | ||
| const parallelResults = await Promise.all( | ||
| params.parallel.map(async (p) => { | ||
| const subAgent = await Agent.get(p.subagent_type) |
There was a problem hiding this comment.
CRITICAL: Parallel subtasks bypass agent permission checks
The only ctx.ask({ permission: "task", patterns: [...] }) call in this method validates params.subagent_type before this branch runs. Each parallel[*].subagent_type is launched here without its own permission check, so a caller can request an allowed top-level agent and still smuggle restricted agents in the parallel array.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Fix these issues in Kilo Cloud Issue Details (click to expand)CRITICAL
WARNING
Other Observations (not in diff)No additional issues found outside the diff. Files Reviewed (45 files)
Reviewed by gpt-5.4-20260305 · 1,723,769 tokens |
Context
Following a comprehensive security audit of the Kilo-specific codebase, this PR addresses
the most critical identified vulnerabilities and improves code quality across multiple
packages. The audit revealed a CRITICAL path traversal vulnerability in skill removal
endpoints, silent error swallowing in error parsing, magic numbers obscuring timeout values,
and sensitive data leaking through debug logs.
All changes are scoped to Kilo-specific code (
packages/opencode/src/kilocode/,packages/kilo-vscode/src/) and do not modify upstream OpenCode patterns.Implementation
Security Fixes
1. Path Traversal Prevention (SEC-001)
packages/opencode/src/util/path-safety.tswithisPathWithinAllowedDirs()utility that validates user-provided paths against known-safe config directories
/kilocode/skill/removeendpoint — rejects requests with pathsthat escape allowed directories via
../traversal/kilocode/agent/removeis safe (only accepts agent names, not file paths)2. Log Sanitization (SEC-005)
sanitizeForLog()helper inclaw/hooks.tsthat redacts sensitive keys(
apiKey,token,secret,password,authorization,accessToken,refreshToken)before logging credential responses
3. Auth Verification (SEC-002, SEC-003)
basicAuthmiddleware(
server.ts:94-102)mcp-migrator.ts,workflows-migrator.ts) only read fromknown-safe system paths (
KilocodePaths,os.homedir(), startupprojectDir) —not user-controlled input
Code Quality Fixes
4. Error Handling (CODE-001)
kilo-errors.ts:parseKiloErrorCode()— now explicitlycatches
SyntaxErrorfromJSON.parse()and re-throws any other unexpected errors5. Magic Numbers (CODE-002)
5000withGRACEFUL_SHUTDOWN_TIMEOUT_MSinserver-manager.ts3000withHEALTH_CHECK_TIMEOUT_MSinconnection-service.tsScreenshots
Before
How to Test
Path Traversal Fix
Log Sanitization
[REDACTED]for any sensitive keysError Handling
instead of being silently swallowed
Magic Numbers
Get in Touch
Discord:
@spdniloy