Skip to content

fix(ecs-mcp-server): add path validation for user-supplied paths - #4480

Open
matthewgoodman13 wants to merge 1 commit into
awslabs:mainfrom
matthewgoodman13:fix/ecs-mcp-path-validation
Open

fix(ecs-mcp-server): add path validation for user-supplied paths#4480
matthewgoodman13 wants to merge 1 commit into
awslabs:mainfrom
matthewgoodman13:fix/ecs-mcp-path-validation

Conversation

@matthewgoodman13

Copy link
Copy Markdown
Member

Fixes N/A

Summary

Changes

Add a shared path_validation module to ecs-mcp-server, mirroring the one in
eks-mcp-server.

validate_file_path() in utils/security.py only pattern-matched the raw string for
../ sequences, so an absolute path, a ~ prefix, or a symlink pointing at a user
configuration directory was accepted. prepare_template_files() also swallowed validation
errors whose message contained does not exist, so a rejected path could still be created.

The new validate_path():

  • Expands ~ and resolves paths with os.path.realpath, collapsing .. and following
    symlinks before any check
  • Rejects paths resolving inside a sensitive directory (~/.aws, ~/.ssh, ~/.kube,
    ~/.gnupg, ~/.docker, /etc, /root, /var/lib), and paths containing one, since a
    parent such as $HOME would pull it into a Docker build context
  • Derives the home directory entries from both $HOME and the password database
  • Accepts relative paths and returns the resolved absolute path

Applied in prepare_template_files (before os.makedirs and the template writes),
build_and_push_image (before the directory becomes the docker buildx build context), and
validate_cloudformation_template. validate_file_path is removed; every caller now uses the
new module.

Two differences from the EKS module: relative paths are accepted rather than rejected, and one
validate_path(path, must_exist=False) replaces two functions with identical bodies.

User experience

Before, app_path was used as written, so ~/.aws — or a symlink resolving to it — became
the template output directory and the Docker build context. Now such a path is rejected
before anything is written or built:

Path '~/.aws' resolves to '/home/user/.aws', which is not usable because it would
expose the sensitive directory '/home/user/.aws'

Project directories, absolute or relative, are unaffected.

Testing

787 tests pass, up from 722 on main. path_validation.py is at 100% line and branch
coverage, covering symlink, ~, relative, traversal, ancestor, prefix-collision and
case-insensitive-filesystem inputs. ruff check, ruff format, pyright, bandit and
uv build are clean.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

  • I have reviewed the contributing guidelines
  • I have performed a self-review of this change
  • Changes have been tested
  • Changes are documented

Is this a breaking change? (N)

Paths resolving inside or containing one of the listed directories are now rejected where they
were previously accepted. No other input is affected.

RFC issue number: N/A

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

Path validation only pattern-matched the raw string for "../" sequences,
so an absolute path, a "~" prefix, or a symlink pointing at a user
configuration directory was accepted. prepare_template_files also
swallowed validation errors containing "does not exist", so a rejected
path could still be created.

Add a path_validation module, mirroring the one in eks-mcp-server, that
expands "~", resolves paths with os.path.realpath, and rejects any path
resolving inside or containing a sensitive directory (~/.aws, ~/.ssh,
~/.kube, ~/.gnupg, ~/.docker, /etc, /root, /var/lib). Relative paths stay
supported, and the resolved absolute path is returned.

Applied in prepare_template_files, build_and_push_image, and
validate_cloudformation_template, replacing validate_file_path in
utils/security.py.
@codecov

codecov Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.15%. Comparing base (ebcafae) to head (c38994d).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4480   +/-   ##
=======================================
  Coverage   93.14%   93.15%           
=======================================
  Files        1038     1039    +1     
  Lines       87530    87568   +38     
  Branches    14121    14124    +3     
=======================================
+ Hits        81529    81570   +41     
+ Misses       3638     3637    -1     
+ Partials     2363     2361    -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@matthewgoodman13 matthewgoodman13 added the 👮admin👮 Looking for admin help to unblock label Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

👮admin👮 Looking for admin help to unblock

Projects

Status: To triage

Development

Successfully merging this pull request may close these issues.

2 participants