fix: path traversal in init agent + FK cascade for event pruning - #1070
Conversation
…t FK Two fixes from PR #1059 bot review findings: 1. [CRITICAL] src/term-commands/init.ts — genie init agent <name> passed CLI input directly to join(baseDir, name) without validation. Names like '../outside' or '/etc/passwd' would write to arbitrary paths. Added guard rejecting names with path separators or traversal sequences. 2. [HIGH] src/db/migrations/026_events_trace_id.sql — parent_event_id FK had no ON DELETE clause (defaults to NO ACTION). This would cause genie db prune-events to fail when deleting parent events still referenced by children. Changed to ON DELETE SET NULL so pruning works and child events retain their own data.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2b626a11f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS trace_id UUID; | ||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS parent_event_id BIGINT REFERENCES genie_runtime_events(id); | ||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS parent_event_id BIGINT REFERENCES genie_runtime_events(id) ON DELETE SET NULL; |
There was a problem hiding this comment.
Changing 026_events_trace_id.sql in place will not fix existing databases that already applied migration 026, because runMigrations skips applied migrations by filename (_genie_migrations name check in src/lib/db-migrations.ts). In those environments the FK remains NO ACTION, so genie db prune-events can still fail when deleting parent rows. This needs a follow-up migration (e.g., 027_...) that explicitly alters the existing parent_event_id constraint to ON DELETE SET NULL.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates a database migration to include ON DELETE SET NULL for parent event IDs and adds path traversal protection for agent names in the CLI. Feedback highlights the need for an index on the foreign key to prevent performance degradation during pruning and suggests hardening the agent name validation to prevent potential YAML injection via newlines.
|
|
||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS trace_id UUID; | ||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS parent_event_id BIGINT REFERENCES genie_runtime_events(id); | ||
| ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS parent_event_id BIGINT REFERENCES genie_runtime_events(id) ON DELETE SET NULL; |
There was a problem hiding this comment.
Adding a foreign key with ON DELETE SET NULL is correct for the stated goal, but deleting parent records during pruning will trigger a sequential scan of the genie_runtime_events table to find and update child records if an index on parent_event_id is missing. For a table that requires pruning, this will cause significant performance degradation as the table grows.
Additionally, note that ALTER TABLE ... ADD COLUMN IF NOT EXISTS will skip the entire clause if the column already exists. If this migration has already been applied to any environment (e.g., from a previous version of the PR), the ON DELETE SET NULL behavior will not be added. If you need to support existing databases where this column might already exist without the constraint, consider a separate ALTER TABLE ... ADD CONSTRAINT statement.
ALTER TABLE genie_runtime_events ADD COLUMN IF NOT EXISTS parent_event_id BIGINT REFERENCES genie_runtime_events(id) ON DELETE SET NULL;
CREATE INDEX IF NOT EXISTS idx_runtime_events_parent_event_id ON genie_runtime_events(parent_event_id) WHERE parent_event_id IS NOT NULL;| /** genie init agent <name> — scaffold agent directory */ | ||
| async function initAgent(name: string, options: { dir?: string }): Promise<void> { | ||
| // Guard against path traversal — name is CLI input and lands in join(baseDir, name) | ||
| if (!name || /[\/\\]/.test(name) || name === '.' || name === '..' || name.includes('..')) { |
There was a problem hiding this comment.
The path traversal check is effective, but the name is also used directly in the YAML frontmatter of the generated AGENTS.md file. If the name contains newlines, it could result in invalid YAML or broken file content. Consider adding a check for newlines or using a more restrictive identifier pattern.
Also, name === '..' is redundant as it is already covered by name.includes('..').
| if (!name || /[\/\\]/.test(name) || name === '.' || name === '..' || name.includes('..')) { | |
| if (!name || /[\\/\\\\\\r\\n]/.test(name) || name === '.' || name.includes('..')) { |
1 participant
Summary
Two fixes from bot review findings on PR #1059 (rolling promotion):
[CRITICAL] Path traversal in
genie init agentgenie init agent <name>passed CLI input directly tojoin(baseDir, name)without validation. Names like../outsideor absolute paths would write agent scaffolding to arbitrary filesystem locations.Fix: Reject names containing
/,\,.., or empty strings before they reach the path join.[HIGH] FK blocks event retention pruning
parent_event_idFK ongenie_runtime_eventsused defaultNO ACTIONdelete behavior. Whengenie db prune-eventsdeletes old parent events, child events referencing them would cause the delete to fail.Fix:
ON DELETE SET NULL— parent can be pruned while children retain their own data with a nulled-out parent reference.Test plan
bun run typecheck— cleanbun test— 2163 pass, 0 failinit-flow.test.ts,init-bootstrap.test.ts)