Replace prompt_path with prompt_text and require organization#10
Conversation
- Replace prompt_path (file path) with prompt_text (text column) to eliminate path traversal / LFI vulnerability. Prompt files serve as default templates, with content stored in the DB per-org. - Make organization_id non-nullable — every reviewer belongs to an org. - Auto-create default reviewers when an organization is created via after_create callback + AutomatedPlanReviewer.create_defaults_for. - Seeds simplified to call create_defaults_for (idempotent). - Removed prompt_file_exists validation and prompt_content method. - Updated fixtures, tests, and ActiveAdmin to match. Amp-Thread-ID: https://ampcode.com/threads/T-019c8bd2-604a-7649-83a8-f19c5d7065ed Co-authored-by: Amp <amp@ampcode.com>
424f3a7 to
d95f7b0
Compare
![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: 424f3a78f9
ℹ️ 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".
| remove_column :automated_plan_reviewers, :prompt_path | ||
|
|
||
| # Make organization_id required | ||
| change_column_null :automated_plan_reviewers, :organization_id, false |
There was a problem hiding this comment.
This migration can fail in environments that already have "global" reviewers with organization_id = NULL (the previous model explicitly allowed belongs_to :organization, optional: true). Calling change_column_null ... false without first updating or deleting those rows will raise at migration time and block deployment for affected databases.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This app is pre-production — there are no existing rows with NULL organization_id in any deployed environment. The "global reviewer" concept from PR #5 was never seeded (seeds always set an org). Safe to enforce NOT NULL without a backfill step here.
| if File.exist?(file_path) | ||
| reviewer.update_column(:prompt_text, File.read(file_path)) |
There was a problem hiding this comment.
Rows whose prompt_path no longer resolves to an existing file are silently skipped here, so their new prompt_text stays NULL; the subsequent change_column_null :prompt_text, false then fails and aborts the migration. This is a deploy-blocking path for any environment where a previously valid file path was later removed or moved.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The prompt files are checked into the repo at prompts/reviewers/*.md and ship with every deploy. They'll always exist at migration time. If someone deleted them, the app would be broken in other ways too (seeds, create_defaults_for). Not a concern for this codebase.
1 participant
What
Addresses two design issues from the PR #5 review:
1. Replace
prompt_pathwithprompt_text(Critical: LFI fix)prompt_pathwas an admin-editable file path used inFile.read(Rails.root.join(prompt_path)), creating a path traversal / local file inclusion vulnerability. An admin could set it to../../config/master.keyor/etc/passwd.Now the prompt content is stored directly in the database as a
textcolumn. The markdown files inprompts/reviewers/remain as default templates — they're read during seeding to populate initial reviewers, but at runtime the app only reads from the DB. This also makes prompts customizable per-org without deploying code changes.2. Make
organization_idnon-nullableThe nullable
organization_id(for "global reviewers") was half-baked — no code consumed it, and it caused a MySQL NULL uniqueness bug where the unique index on[:organization_id, :key]wouldn't prevent duplicate keys whenorganization_idis NULL.Removed the concept entirely. Every reviewer belongs to an organization.
Also fixed
find_or_create_by!instead of the brittlecount == 0guardprompt_file_existsvalidation andprompt_contentmethod (no longer needed)Tests
All 168 tests pass.