Skip to content

fix: path traversal in init agent + FK cascade for event pruning #1070

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
namastex888 merged 1 commit into from
Apr 5, 2026
Merged

fix: path traversal in init agent + FK cascade for event pruning #1070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/db/migrations/026_events_trace_id.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
-- 026_events_trace_id.sql — Add trace_id and parent_event_id for distributed tracing (#859)

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;

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Add a new migration instead of editing 026 in place

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 gemini-code-assist Bot Apr 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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;

CREATE INDEX IF NOT EXISTS idx_runtime_events_trace_id ON genie_runtime_events(trace_id) WHERE trace_id IS NOT NULL;
6 changes: 6 additions & 0 deletions src/term-commands/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,12 @@ function resolveAgentsDir(wsRoot: string, dirOption?: string): string {

/** 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('..')) {

@gemini-code-assist gemini-code-assist Bot Apr 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

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('..').

Suggested change
if (!name || /[\/\\]/.test(name) || name === '.' || name === '..' || name.includes('..')) {
if (!name || /[\\/\\\\\\r\\n]/.test(name) || name === '.' || name.includes('..')) {

console.error('Error: Agent name must not contain path separators or traversal sequences.');
process.exit(1);
}

const cwd = process.cwd();
const ws = findWorkspace(cwd);
if (!ws) {
Expand Down
Loading