Skip to content

feat(forward): apply DDL to a database — SECURITY REVIEW REQUIRED#464

Open
seonghobae wants to merge 1 commit into
mainfrom
feat/forward-apply-on-main
Open

feat(forward): apply DDL to a database — SECURITY REVIEW REQUIRED#464
seonghobae wants to merge 1 commit into
mainfrom
feat/forward-apply-on-main

Conversation

@seonghobae

Copy link
Copy Markdown
Collaborator

Independent PR against main (replaces #458, which was needlessly stacked on adhesive-piper).

⚠️ SECURITY REVIEW REQUIRED — writes DDL to a live database

  • POST /api/connections/{uuid}/apply-sql, body { sql, dry_run=true }.
  • dry-run by default — runs the batch in one transaction and rolls back (pre-flight validation); caller must set dry_run=false to persist.
  • editor role; IDOR-safe (non-member → 404, member-without-editor → 403).
  • SSRF-guarded: apply_postgres_sql reuses validate_postgres_dsn_target + pinned IP.
  • DSN decrypted only in memory; errors DSN-redacted; failed apply is ok=false.
  • PostgreSQL only for now.

Open questions: arbitrary-SQL vs server-generated-migration-only; statement allow/deny-list; rate-limit + audit. backend pytest +7 (IDOR/editor gates, dry-run paths, dialect dispatch, DSN redaction); mypy clean.

🤖 Generated with Claude Code

…ult)

Standalone against main. Forward engineering (model -> DB). POST /api/connections/{uuid}/apply-sql runs DDL in ONE transaction; dry_run=True (default) rolls back as a pre-flight, dry_run=False commits. Editor role, IDOR-safe (non-member 404, member-without-editor 403), SSRF-guarded via validate_postgres_dsn_target + pinned IP, DSN-redacted errors. PostgreSQL only. +7 tests. SECURITY REVIEW REQUIRED before merge (writes to a live DB).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AxU2xaupAjp912oDNFuWyd
tx = conn.transaction()
await tx.start()
try:
await conn.execute(sql)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Intentional and documented, not an injection defect: this endpoint's purpose is executing caller-supplied DDL (forward engineering), which is why the PR is titled SECURITY REVIEW REQUIRED and excluded from auto-merge. Mitigations in place: editor-role + IDOR gates, SSRF-pinned connection, single transaction with dry-run-by-default rollback, DSN-redacted errors. Open questions for the human reviewer are listed in the PR body (restrict to server-generated migrations only / statement allow-list / audit+rate limits).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants