fix: address CodeQL path injection alerts in runbook routes#152
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces two private helper functions to centralize path normalization and validation logic in the runbook API: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses CodeQL path injection alerts by significantly enhancing the security and validation of user-supplied directory paths within the runbook API routes. It introduces a new, centralized path normalization and validation function that strictly enforces allowed prefixes and handles various edge cases, preventing potential malicious path manipulations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR hardens path handling in the runbook directory-related API routes to address CodeQL path-injection concerns by normalizing user-supplied paths and enforcing allow-prefix containment checks, and adds unit tests for allowlist mutation behavior.
Changes:
- Added shared helpers to normalize user-supplied directory paths and enforce “must be under allowed prefixes” policy.
- Updated runbook endpoints to use the new normalization/containment logic.
- Added unit tests covering accept/reject behavior for the runtime allowed-dirs mutation endpoint.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/paperbot/api/routes/runbook.py |
Adds prefix containment + user path normalization helper and applies it to runbook directory endpoints. |
tests/unit/test_runbook_project_dir_prepare_route.py |
Adds tests for /runbook/allowed-dirs allowlist mutation accept/reject behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| resolved = path.resolve(strict=False) | ||
| normalized_prefix = prefix.resolve(strict=False) | ||
| return resolved == normalized_prefix or resolved.is_relative_to(normalized_prefix) |
There was a problem hiding this comment.
_is_under_prefix() uses Path.is_relative_to(), but the project supports Python >=3.8 (pyproject.toml). is_relative_to doesn’t exist on 3.8, so this will be caught by the broad except and return False, effectively making all allowlist checks fail on 3.8. Consider replacing this with a 3.8-compatible containment check (e.g., resolved.relative_to(normalized_prefix) in a try/except) while still using resolved paths to avoid traversal/symlink issues.
| return resolved == normalized_prefix or resolved.is_relative_to(normalized_prefix) | |
| if resolved == normalized_prefix: | |
| return True | |
| try: | |
| # Python 3.8-compatible alternative to Path.is_relative_to() | |
| resolved.relative_to(normalized_prefix) | |
| return True | |
| except ValueError: | |
| return False |
| monkeypatch.chdir(tmp_path) | ||
| monkeypatch.setenv("PAPERBOT_RUNBOOK_ALLOWLIST_MUTATION", "true") | ||
|
|
||
| outside = Path.home().parent / "paperbot-unsafe" |
There was a problem hiding this comment.
This test creates a directory under Path.home().parent (often /home/...), which is frequently not writable in CI and can cause permission-related test failures and leave artifacts outside the pytest temp dir. Since the route should reject disallowed paths before checking existence, you can avoid mkdir() entirely, or choose a disallowed path that remains within writable test temp locations.
| outside = Path.home().parent / "paperbot-unsafe" | |
| outside = tmp_path.parent / "paperbot-unsafe" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses CodeQL path injection alerts in the runbook routes by implementing robust path normalization and validation. It centralizes security logic with _normalize_user_directory and _is_under_prefix helpers, ensuring user-supplied paths are strictly validated against an allowlist using Path.resolve(strict=False) and Path.is_relative_to(). I've provided suggestions to improve code clarity and simplify new logic, specifically by removing redundant try/except blocks and streamlining path construction. The new unit tests are a great addition, improving coverage for this security-sensitive code, and no new security vulnerabilities were introduced.
| suffix = normalized[len(prefix_str):].lstrip("/\\") | ||
| candidate = (prefix / suffix).resolve(strict=False) if suffix else prefix |
There was a problem hiding this comment.
The logic for reconstructing the candidate path from prefix and suffix is complex and seems redundant. Since normalized is already a canonical path string (from resolve() on line 157), you can create the Path object directly from it. This improves readability without changing the behavior.
| suffix = normalized[len(prefix_str):].lstrip("/\\") | |
| candidate = (prefix / suffix).resolve(strict=False) if suffix else prefix | |
| candidate = Path(normalized) |
| try: | ||
| resolved = requested.resolve(strict=False) | ||
| except Exception: | ||
| raise HTTPException(status_code=400, detail="invalid project_dir") | ||
|
|
||
| if not _allowed_workdir(resolved): | ||
| raise HTTPException(status_code=403, detail="project_dir is not allowed") | ||
| resolved = _normalize_user_directory(body.project_dir, field_name="project_dir") | ||
| except HTTPException: | ||
| raise |
There was a problem hiding this comment.
This try...except HTTPException: raise block is redundant. The code will have the same behavior without it, as an HTTPException raised from _normalize_user_directory will propagate up the call stack anyway. Removing this block will make the code cleaner.
resolved = _normalize_user_directory(body.project_dir, field_name="project_dir")| try: | ||
| resolved = Path(body.directory).expanduser().resolve() | ||
| except Exception: | ||
| raise HTTPException(status_code=400, detail="invalid directory path") | ||
| resolved = _normalize_user_directory(body.directory, field_name="directory") | ||
| except HTTPException: | ||
| raise |
There was a problem hiding this comment.
This try...except HTTPException: raise block is redundant, similar to the one in prepare_project_dir. The code will have the same behavior without it. Removing this block will improve code clarity.
resolved = _normalize_user_directory(body.directory, field_name="directory")
What
Path.is_relative_to)Why
CodeQL flagged uncontrolled data in path expressions for:
/api/runbook/project-dir/prepare/api/runbook/allowed-dirsThis patch narrows accepted inputs to configured allowed prefixes and blocks malformed paths before use.
Validation
python -m py_compile src/paperbot/api/routes/runbook.py tests/unit/test_runbook_project_dir_prepare_route.pypytest -q tests/unit/test_runbook_project_dir_prepare_route.py(segfaults in current local env; existing machine issue)Summary by CodeRabbit
Bug Fixes
Tests