Fix code review findings: 5 HIGH, 12 MEDIUM severity issues

[JeremyDev87]

Summary

Address all findings from the 2026-02-12 comprehensive code review (score: 8.5/10).

Issues Found

HIGH Severity (5)

• H1: Untrusted plan comment fallback - findPlanComment() falls back to any comment author
• H2: Shell injection via labels/author in execute prompt templates
• H3: NONE accepted as valid authorized_approvers association
• H4: Unsafe as LeonidasMode type cast without validation
• H5: Unsafe as Command type cast without validation

MEDIUM Severity (12)

• M1: API key and GitHub token not registered with core.setSecret()
• M2: escapeForShellArg() doesn't handle newlines/carriage returns
• M3: allowed_tools default includes dangerous Bash(npx:*) and Bash(node:*) (documented)
• M4: Plan comment not wrapped in prompt injection delimiters
• M5: Octokit instances re-created on every call (no caching)
• M6: String interpolation used for file paths instead of path.join()
• M7: Test suite only covers 5 of 8 supported languages
• M10: run() function already refactored (no action needed)
• M12: ESLint ignores missing coverage/** directory

LOW Severity (5)

• L1: Magic number 5 for reserved turns
• L3: Silent catch in linkSubIssues (already fixed)
• L4: parseRepo() doesn't validate empty input
• L5: Temp prompt file created with default permissions

[github-actions]

🏛️ Leonidas Implementation Plan

Summary

Address all 22 findings (5 HIGH, 12 MEDIUM, 5 LOW) from the 2026-02-12 comprehensive code review by fixing security vulnerabilities, improving code quality, and enhancing test coverage.

Implementation Steps

• Step 1: Fix HIGH severity auth/injection vulnerabilities (H1-H5)

  • H1 (src/github.ts:42-49): Remove untrusted fallback in findPlanComment() — keep only trusted bot filtering, remove lines 42-49
  • H2 (src/prompts/execute.ts:38-45): Extend escapeForShellArg() in sanitize.ts to handle labels/author arrays, apply to issueLabels/issueAuthor before template interpolation
  • H3 (src/config.ts:162): Add validation to reject NONE in authorized_approvers — throw error if found
  • H4 (src/main.ts:23): Replace unsafe as LeonidasMode cast with type guard validation function
  • H5 (src/post_process.ts:241): Replace unsafe as Command cast with type guard validation function
  • Verify: npm run typecheck passes, security tests confirm fixes

• Step 2: Fix MEDIUM severity security issues (M1-M4)

  • M1 (src/main.ts:18): Call core.setSecret(inputs.anthropic_api_key) and core.setSecret(inputs.github_token) after reading inputs
  • M2 (src/utils/sanitize.ts:57-62): Add newline (\n, \r) escaping to escapeForShellArg()
  • M4 (src/prompts/execute.ts:88): Wrap planComment in prompt injection delimiters using wrapUserContent() (already done for title/body, extend to plan)
  • Verify: npm run lint passes, security review checklist complete

• Step 3: Fix MEDIUM code quality issues (M5-M7)

  • M5 (src/github.ts:6-8): Cache Octokit instances — convert createOctokit() to singleton pattern with token-keyed Map cache
  • M6 (src/main.ts:271): Replace string interpolation with path.join(tmpDir, filename)
  • M7 (src/i18n.test.ts): Add test coverage for missing language it (Italian) — update test suite to cover all 8 supported languages, not just 5
  • Verify: npm test passes, test coverage improves

• Step 4: Fix ESLint config and LOW severity issues (M12, L1-L5)

  • M12 (eslint.config.mjs:9): Add coverage/** to ignores array (currently missing, causing false positives)
  • L1 (src/main.ts:108-109, 36): Extract magic number 5 to named constant RESERVED_TURNS = 5 at top of main.ts
  • L4 (src/post_process.ts:57-59): Add validation to parseRepo() — throw error if repo is empty or does not contain /
  • L5 (src/main.ts:272): Add file mode 0o600 (owner read/write only) to fs.writeFileSync() call for temp prompt file
  • Verify: npm run lint passes, all validations enforce constraints

• Step 5: Run full verification suite

  • Execute npm run lint — must pass with no errors
  • Execute npm run typecheck — must pass with no type errors
  • Execute npm run build — must succeed
  • Execute npm test — must pass with all tests green
  • Verify: All 4 checks pass, ready for commit

• Step 6: Commit changes with descriptive message

  • Stage all modified files
  • Create atomic commit: fix: address 22 code review findings (5 HIGH, 12 MEDIUM, 5 LOW)
  • Push to branch
  • Verify: Changes committed and pushed successfully

• Step 7: Create pull request

  • Use gh pr create with title: #234: Fix code review findings: 5 HIGH, 12 MEDIUM severity issues
  • PR body: Closes #234 + summary of security fixes and improvements
  • Verify: PR created and linked to issue

Considerations

Security Priority:

• HIGH severity fixes (H1-H5) are critical and handled first
• M1-M4 address secrets management and injection defense
• All fixes follow defense-in-depth principle

Type Safety:

• H4 and H5 eliminate unsafe type casts with proper validation
• TypeScript strict mode enforces correctness

Testing:

• M7 improves test coverage for i18n (8 languages, currently only 5 tested)
• Full test suite verification ensures no regressions

Backward Compatibility:

• H1 change may affect repos with custom GitHub App installations (rare edge case)
• H3 explicitly rejects insecure NONE approver config (security hardening)
• All other changes are internal improvements with no API surface changes

Skipped Issues:

• M3: Documented as design decision (not fixing)
• M10: Already refactored (no action needed)
• L3: Already fixed in previous commit (no action needed)

Verification

1. Security Validation:

   • H1: Verify findPlanComment() only trusts bot authors
   • H2: Verify escapeForShellArg() handles all shell metacharacters + newlines
   • H3: Verify NONE is rejected in authorized_approvers
   • H4/H5: Verify type guards prevent invalid mode/command values

2. Build Checks:

   • All 4 CI checks (lint, typecheck, build, test) must pass
   • No new warnings or errors introduced

3. Test Coverage:

   • i18n tests cover all 8 supported languages
   • Security fixes have corresponding test validation

---

To approve this plan and start implementation, comment /approve on this issue.

[JeremyDev87]

/approve

[github-actions]

⚡ Leonidas is starting implementation for issue #234...

[github-actions]

⚠️ Leonidas Partial Progress

Implementation was interrupted, but a draft PR was created to preserve progress.

Draft PR: #239
Workflow run: View logs

To continue: Comment /approve again to retry, or manually complete the draft PR.