Skip to content

Fixes #1430 Fix: windows security vulnerability and test failures#1424

Closed
Tahir-yamin wants to merge 1 commit intoaden-hive:mainfrom
Tahir-yamin:fix/tests-and-windows-security
Closed

Fixes #1430 Fix: windows security vulnerability and test failures#1424
Tahir-yamin wants to merge 1 commit intoaden-hive:mainfrom
Tahir-yamin:fix/tests-and-windows-security

Conversation

@Tahir-yamin
Copy link
Contributor

Description

This PR addresses critical issues affecting Windows users and the reliability of the test suite. It fixes a potential sandbox escape vulnerability, resolves CI failures due to incorrect test assumptions, and enables cross-platform testing.

1. 🚨 Security Fix: Windows Path Traversal

  • Issue: get_secure_path in security.py failed to strip the drive/root from absolute paths on Windows (e.g., /etc/passwd or C:\...) because lstrip(os.sep) only checked for \ on Windows, missing /.
  • Fix: Updated logic to strip both os.sep and os.altsep to prevent absolute paths from being joined incorrectly, avoiding sandbox escape.

2. 🐛 Fix: Broken Credential Tests

  • Issue: test_credentials.py asserted that anthropic and other credential specs had required=True. However, llm.py correctly defines them as required=False (optional). This caused pytest to fail for any fresh clone without keys.
  • Fix: Updated tests to match the actual flexible design. Validation logic is now tested using custom mock specs instead of relying on mutable production specs.

3. 🛠️ Fix: Cross-Platform Test Compatibility

  • Issue: test_file_system_toolkits.py used subprocess.run with Unix-only commands (e.g., ls, tr), causing 10+ failures on Windows.
  • Fix: Refactored tests to use unittest.mock for subprocess.run. This simulates command output, making the tests faster, safer, and truly platform-agnostic.

4. ✅ Enhancement: Mocked Tool Testing

  • Added a "happy path" mock test for web_search_tool to allow verifying tool logic without requiring real API keys in the CI environment.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Security patch
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Verified?

Ran full test suite on Windows 11 with Python 3.13.

pytest tools/tests/ -v

Result: 205 passed, 0 failed. (previously 7 failed, 198 passed)

  • tools/tests/test_credentials.py - PASSED
  • tools/tests/tools/test_file_system_toolkits.py - PASSED
  • tools/tests/tools/test_web_search_tool.py - PASSED
  • tools/tests/tools/test_security.py - PASSED

- Security: Fixed path traversal vulnerability on Windows in security.py
- Tests: Fixed credential tests to match optional Anthropic key design
- Tests: Mocked subprocess.run in file system tests for cross-platform compatibility
- Tests: Added mocked happy path for web search tool
@github-actions
Copy link

PR Closed - Requirements Not Met

This PR has been automatically closed because it doesn't meet the requirements.

PR Author: @Tahir-yamin
Found issues: #6 (assignees: none)
Problem: The PR author must be assigned to the linked issue.

To fix:

  1. Assign yourself (@Tahir-yamin) to one of the linked issues
  2. Re-open this PR

Why is this required? See #472 for details.

@github-actions github-actions bot closed this Jan 27, 2026
@Tahir-yamin Tahir-yamin changed the title fix: windows security vulnerability and test failures Fixes #1430 Jan 27, 2026
@Tahir-yamin Tahir-yamin changed the title Fixes #1430 Fixes #1430 Fix: windows security vulnerability and test failures Jan 27, 2026
@Tahir-yamin
Copy link
Contributor Author

Fixes #1430 Fix: windows security vulnerability and test failures

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant