## Code Review: FAIL AUDIT ### CRITICAL BLOCKER: PR Not Mergeable **PR #79 is CLOSED with merge conflicts (CONFLICTING)** - The PR was closed without being merged and cannot be merged in its current state due to conflicts. ``` state: CLOSED mergeStateStatus: DIRTY mergedAt: null ``` The branch `feature/ticket-134-researcher-agent-infrastructure` needs to be rebased onto main and the conflicts resolved before this can be reviewed properly. --- ### Security Issues Found #### 1. Path Traversal Vulnerability in `read_code` (mcp_controller.rb) **Location:** `app/controllers/api/v1/mcp_controller.rb:947` **Issue:** The security check happens BEFORE `File.expand_path`, allowing bypass with `../` sequences. ```ruby # VULNERABLE CODE: rails_root = Rails.root.to_s unless file_path.start_with?(rails_root) # Check happens first return { error: "Access denied..." } end expanded_path = File.expand_path(file_path) # Expand happens AFTER ``` **Attack Example:** `/rails/app/../../etc/passwd` would pass the check but expand to `/etc/passwd`. **Fix:** Expand the path FIRST, then check: ```ruby expanded_path = File.expand_path(file_path) rails_root = Rails.root.to_s unless expanded_path.start_with?(rails_root) return { error: "Access denied..." } end ``` #### 2. SQL Injection Risk in Migration (insert_researcher_agent.rb) **Location:** `db/migrate/20251229035535_insert_researcher_agent.rb:18` **Issue:** Raw SQL with unsanitized `project.name.parameterize`: ```ruby VALUES ( '#{project.name.parameterize}-researcher', # Unsafe interpolation ``` While `parameterize` provides some sanitization, it's safer to use parameterized queries or sanitize explicitly. --- ### Missing Test Coverage The following new code has NO test coverage: 1. **`handle_create_proposal` handler** - No specs in `spec/requests/api/v1/mcp_spec.rb` 2. **`handle_read_code` handler** - No specs testing: - Normal file reading - Path traversal security - File size limits - Error handling 3. **Migration tests** - No specs for `InsertResearcherAgent` **Required tests:** ```ruby # spec/requests/api/v1/mcp_spec.rb describe "create_proposal" do it "creates a proposal for researcher agents" it "blocks non-researcher agents from creating proposals" it "validates required parameters" end describe "read_code" do it "reads files within Rails directory" it "blocks path traversal attacks" it "enforces file size limits" it "returns error for non-existent files" end ``` --- ### Positive Findings 1. **MCP Permissions (mcp_permissions.yml):** Properly configured - researcher has read-only access with appropriate blocks 2. **Agent Model:** Enum and capabilities correctly added 3. **TypeScript definitions:** Properly match Ruby MCP tools 4. **Guardrails:** Permission-level restrictions prevent unauthorized modifications --- ### Action Required 1. **Rebase the PR** onto main to resolve merge conflicts 2. **Fix path traversal vulnerability** in `read_code` 3. **Fix SQL injection risk** in migration 4. **Add comprehensive test coverage** for new MCP handlers 5. **Re-open PR** and request re-review