Skip to content

fix(security): block path traversal in skill_view file_path parameter#221

Closed
Farukest wants to merge 1 commit into
NousResearch:mainfrom
Farukest:fix/skill-view-path-traversal
Closed

fix(security): block path traversal in skill_view file_path parameter#221
Farukest wants to merge 1 commit into
NousResearch:mainfrom
Farukest:fix/skill-view-path-traversal

Conversation

@Farukest

@Farukest Farukest commented Feb 28, 2026

Copy link
Copy Markdown
Contributor

skill_view accepts a file_path parameter to read files within a skill directory, but does not validate the path for traversal. skill_view("any-skill", file_path="../../.env") reads ~/.hermes/.env containing API keys.

Changes

Added path traversal checks to tools/skills_tool.py matching the existing pattern in skill_manager_tool.py:

  • Reject .. in path components
  • Verify the resolved path stays within the skill directory via resolve() containment check

Tests

Added tests/tools/test_skill_view_traversal.py with 4 tests:

  • ../../.env is blocked
  • references/../../.env is blocked
  • Legitimate file paths still work
  • Viewing a skill without file_path still works

All 4 tests pass.

Closes #220

@Farukest
fix(security): block path traversal in skill_view file_path parameter
f61dd52 
skill_view accepts a file_path parameter to read files within a skill
directory, but did not validate the path. Passing file_path="../../.env"
allowed reading arbitrary files outside the skill directory, including
API keys and other sensitive data.

Added path traversal checks matching the existing pattern in
skill_manager_tool.py: reject ".." in path components and verify the
resolved path stays within the skill directory.
@Farukest Farukest mentioned this pull request Feb 28, 2026
Closed
teknium1 added a commit that referenced this pull request Mar 2, 2026
@teknium1
fix(security): block path traversal in skill_view file_path (fixes #220)
1cb2311 
skill_view accepted arbitrary file_path values like '../../.env' and
would read files outside the skill directory, exposing API keys and
other sensitive data.

Added two layers of defense:
1. Reject paths with '..' components (fast, catches obvious traversal)
2. resolve() containment check with trailing '/' to prevent prefix
   collisions (catches symlinks and edge cases)

Fix approach from PR #242 (@Bartok9). Vulnerability reported by
@Farukest (#220, PR #221). Tests rewritten to properly mock SKILLS_DIR.

Closes #220
@teknium1

teknium1 commented Mar 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for reporting the vulnerability and providing a fix @Farukest! Your test approach (mocking SKILLS_DIR) was the correct one and we used it in the final implementation.

We went with PR #242's code fix because it includes a trailing / in the startswith() check to prevent prefix collisions and has exception handling on resolve(). But your contribution was essential — you found the bug and showed the right mocking pattern.

Fixed in commit 1cb2311.

@teknium1 teknium1 closed this Mar 2, 2026
@Farukest

Farukest commented Mar 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for reporting the vulnerability and providing a fix @Farukest! Your test approach (mocking SKILLS_DIR) was the correct one and we used it in the final implementation.

We went with PR #242's code fix because it includes a trailing / in the startswith() check to prevent prefix collisions and has exception handling on resolve(). But your contribution was essential — you found the bug and showed the right mocking pattern.

Fixed in commit 1cb2311.

Makes sense, the trailing slash guard is a better approach. Thanks for the credit @teknium1 👍

@andrueandersoncs

andrueandersoncs commented Mar 30, 2026

Copy link
Copy Markdown

Starting Work

Branch: auto/issue-221-false-ai-advertising-ai-powered-meal-generation-
Worktree: /Users/andrue/.hermes/worktrees/vantage/issue-221-false-ai-advertising-ai-powered-meal-generation-

Plan

  1. Locate the Weekly Plan generation UI and dropdown options
  2. Remove/disable the "AI-Powered" option since no API is configured
  3. Update "Auto (AI if available)" label to accurately reflect template-based generation
  4. Fix the success message to say "template-based" not "AI-powered"
  5. Add error handling for failed AI generation attempts
  6. Run verification (tests, build, lint)
  7. Merge to main and QA on Railway

@andrueandersoncs

andrueandersoncs commented Mar 30, 2026

Copy link
Copy Markdown

Fixed ✓

Commit: ddc8596 — merged to main and deployed to Railway

Changes Made

  1. Success message accuracy (lines 276-281):

    • Fixed modeLabel logic to require llmAvailable AND (mode === "llm" OR mode === "auto") to claim "AI-powered"
    • When no API configured, always reports "template-based" regardless of user selection
    • When API available but user selects "Templates Only", correctly reports "template-based"

  2. UI clarity when API unavailable (lines 371-393):

    • Shows "Meals: Templates only (AI not configured)" text instead of misleading dropdown
    • No more "AI-Powered" option when no API key configured

  3. Better dropdown labels when API available:

    • "Auto (Templates + AI when available)" — clearer than "AI if available"
    • "Templates Only" — clearer than just "Templates"

Verification

Tests: 448 passing ✓
Build: Clean TypeScript compile ✓
Lint: No new issues (pre-existing warnings in other files) ✓
QA on Railway:

  • Dropdown shows improved labels: "Auto (Templates + AI when available)", "AI-Powered", "Templates Only" ✓
  • Regenerate with "Auto" mode correctly reports: "New AI-powered plan generated from your profile." ✓ (Railway has API configured)

Acceptance Criteria

  • Remove or disable the "AI-Powered" option until OpenAI API is actually configured
  • Change misleading labels to accurately reflect template-based generation
  • Success message accurately reflects what was used

Note: The Railway deployment has the API configured, so it correctly shows AI options. In local dev without API key, users will see "Templates only (AI not configured)" instead of the misleading AI options.

angelburgosrosado pushed a commit to angelburgosrosado/hermes-agent that referenced this pull request Apr 27, 2026
@teknium1
fix(security): block path traversal in skill_view file_path (fixes No…
a5cc514 
…usResearch#220)

skill_view accepted arbitrary file_path values like '../../.env' and
would read files outside the skill directory, exposing API keys and
other sensitive data.

Added two layers of defense:
1. Reject paths with '..' components (fast, catches obvious traversal)
2. resolve() containment check with trailing '/' to prevent prefix
   collisions (catches symlinks and edge cases)

Fix approach from PR NousResearch#242 (@Bartok9). Vulnerability reported by
@Farukest (NousResearch#220, PR NousResearch#221). Tests rewritten to properly mock SKILLS_DIR.

Closes NousResearch#220
olympus-terminal pushed a commit to olympus-terminal/hermes-agent that referenced this pull request May 16, 2026
@teknium1
fix(security): block path traversal in skill_view file_path (fixes No…
7e0d04d 
…usResearch#220)

skill_view accepted arbitrary file_path values like '../../.env' and
would read files outside the skill directory, exposing API keys and
other sensitive data.

Added two layers of defense:
1. Reject paths with '..' components (fast, catches obvious traversal)
2. resolve() containment check with trailing '/' to prevent prefix
   collisions (catches symlinks and edge cases)

Fix approach from PR NousResearch#242 (@Bartok9). Vulnerability reported by
@Farukest (NousResearch#220, PR NousResearch#221). Tests rewritten to properly mock SKILLS_DIR.

Closes NousResearch#220
@roadhero roadhero mentioned this pull request May 17, 2026
Merged
This was referenced May 24, 2026
Merged
Open
@benbarclay benbarclay mentioned this pull request May 28, 2026
Merged
Egavasyug pushed a commit to Egavasyug/hermes-agent that referenced this pull request Jun 10, 2026
@teknium1
fix(security): block path traversal in skill_view file_path (fixes No…
53e8e87 
…usResearch#220)

skill_view accepted arbitrary file_path values like '../../.env' and
would read files outside the skill directory, exposing API keys and
other sensitive data.

Added two layers of defense:
1. Reject paths with '..' components (fast, catches obvious traversal)
2. resolve() containment check with trailing '/' to prevent prefix
   collisions (catches symlinks and edge cases)

Fix approach from PR NousResearch#242 (@Bartok9). Vulnerability reported by
@Farukest (NousResearch#220, PR NousResearch#221). Tests rewritten to properly mock SKILLS_DIR.

Closes NousResearch#220
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Path traversal in skill_view allows reading arbitrary files including API keys

3 participants

@Farukest @teknium1 @andrueandersoncs