pal-mcp-server icon indicating copy to clipboard operation
pal-mcp-server copied to clipboard

fix: path traversal vulnerability in is_dangerous_path()

Open DragonFSKY opened this issue 2 weeks ago • 7 comments

Summary

  • Fixes path traversal vulnerability (CWE-22) where subdirectories of dangerous paths were not blocked
  • is_dangerous_path() now uses prefix matching instead of exact string matching
  • For example, /etc/passwd is now correctly blocked when /etc is in DANGEROUS_PATHS

Fixes

  • Fixes #312
  • Fixes #293

Changes

  • Modified utils/security_config.py: Changed is_dangerous_path() to use prefix matching
  • Added tests/test_path_traversal_security.py: 8 test cases covering the vulnerability fix

Test plan

  • [x] All existing tests pass (18 file protection tests)
  • [x] All new security tests pass (8 tests)
  • [x] Verified /etc/passwd, /etc/shadow, /var/log/auth.log are now blocked
  • [x] Verified safe paths like /tmp/test are still allowed
  • [x] Verified similar names like /tmp/etcbackup are not incorrectly blocked

DragonFSKY avatar Dec 03 '25 07:12 DragonFSKY

Summary of Changes

Hello @DragonFSKY, I'm Gemini Code Assist[^1]! 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 significantly enhances the application's security by fixing a critical path traversal vulnerability. The is_dangerous_path() utility function has been refactored to perform robust prefix matching, ensuring that not only explicitly listed dangerous directories but also all their subdirectories are correctly identified and blocked. This prevents unauthorized access to sensitive system files and improves the overall security posture against common attack vectors.

Highlights

  • Security Fix: Addressed a path traversal vulnerability (CWE-22) in the is_dangerous_path() function.
  • Improved Path Validation: The is_dangerous_path() function now uses prefix matching instead of exact string matching, ensuring that subdirectories of dangerous paths are correctly blocked (e.g., /etc/passwd is blocked if /etc is a dangerous path).
  • New Test Coverage: Introduced a new test file, tests/test_path_traversal_security.py, with 8 new test cases to validate the fix and prevent regressions.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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 :thumbsup: and :thumbsdown: 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.

[^1]: Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

gemini-code-assist[bot] avatar Dec 03 '25 07:12 gemini-code-assist[bot]

@P0LESTAR Thank you for your detailed review and catching this important issue!

You're right - the Windows path separator handling has a flaw. When C:\ is in DANGEROUS_PATHS, appending \ creates C:\\ (double backslash), which doesn't match paths like C:\Users.

I'll fix this by using Path.is_relative_to() (available in Python 3.9+, which is our minimum requirement) instead of string prefix matching. This provides correct cross-platform path comparison.

Could you please help verify the fix once I push the update? 🙏


@gemini-code-assist @chatgpt-codex-connector Regarding the /home concern - I've verified that this is a false positive. The code flow in resolve_and_validate_path() is:

  1. First checks is_dangerous_path() - blocks /home itself
  2. Then checks is_home_directory_root() - blocks /home/username
  3. Subdirectories like /home/username/project pass both checks and are allowed

The existing tests in TestHomeDirectoryProtection confirm this behavior works correctly.

DragonFSKY avatar Dec 05 '25 05:12 DragonFSKY

Hello @DragonFSKY,

Thank you for the detailed update and clarification!

Regarding the Windows path separator issue, using Path.is_relative_to() is an excellent approach to ensure robust and cross-platform path comparison. This will definitely improve the reliability of the is_dangerous_path() function. Please do tag me again once you've pushed the update, and I'll be happy to review the changes.

I also appreciate the clarification on the /home concern. Your explanation of the resolve_and_validate_path() flow, specifically how is_dangerous_path() blocks /home itself and is_home_directory_root() handles /home/username, makes perfect sense. It's good to know that the existing tests confirm this correct behavior.

Looking forward to seeing the updated changes!

gemini-code-assist[bot] avatar Dec 05 '25 05:12 gemini-code-assist[bot]

LGTM. It works fine on both Linux and Windows.

Where is "EXCLUDED_DIRS" used?

P0LESTAR avatar Dec 05 '25 08:12 P0LESTAR

is pre-existing code, not added by this PR. It's used to skip directories like node_modules, .git, __pycache__ during recursive file search.

DragonFSKY avatar Dec 06 '25 15:12 DragonFSKY

EXCLUDED_DIRS is pre-existing code, not added by this PR. It is used to skip directories like node_modules, .git, pycache during recursive file search.

DragonFSKY avatar Dec 06 '25 15:12 DragonFSKY

Oh, that's right. I was mistaken

P0LESTAR avatar Dec 07 '25 14:12 P0LESTAR

@codex review

guidedways avatar Dec 15 '25 10:12 guidedways

@codex review

Please re-analyze this PR after the latest fix commit (e5548ac).

The fix addresses your previous feedback about blocking all home directory subpaths by:

  1. Splitting DANGEROUS_PATHS into DANGEROUS_SYSTEM_PATHS and DANGEROUS_HOME_CONTAINERS
  2. System paths (e.g., /etc) block the path AND all subdirectories
  3. Home containers (e.g., /home) block ONLY the exact path, allowing /home/user/project to pass through

This ensures users can still access their project directories under /home/* while maintaining security.

DragonFSKY avatar Dec 15 '25 12:12 DragonFSKY

@guidedways Thanks for the reminder! Fixed in e5548ac.

DragonFSKY avatar Dec 15 '25 12:12 DragonFSKY

Codex Review: Didn't find any major issues. Nice work!

ℹ️ 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".

@DragonFSKY

adding "C:\" to the “block subdirs” set effectively blocks almost all real-world absolute Windows workspaces, same issue on windows

guidedways avatar Dec 15 '25 16:12 guidedways

Pushed follow-up fix commit ba08308 on top of this PR (maintainer push).\n\n- Fixes macOS symlinked system dirs (/etc -> /private/etc, /var -> /private/var) so prefix-blocking works as intended\n- Avoids overblocking Windows workspaces by not treating C:\ as a prefix-blocked system path (drive root still blocked via root check)\n- Adjusts macOS test TMPDIR to /tmp so tests don't create workspaces under /private/var when /var is blocked\n\nRan: pytest tests/test_path_traversal_security.py tests/test_file_protection.py

guidedways avatar Dec 15 '25 17:12 guidedways