Skip to content

Commit ff22ec3

Browse files
authored
Python: fix(python): check the full download path in SessionsPythonTool (#14791)
### Motivation and Context <!-- Thank you for your contribution to the semantic-kernel repo! Please help reviewers and future users, providing the following information: 1. Why is this change required? 2. What problem does it solve? 3. What scenario does it contribute to? 4. If it fixes an open issue, please link to the issue here. --> When `allowed_download_directories` is set, `SessionsPythonTool.download_file()` checks that the local path is inside one of the allowed folders. The check only resolved the folder part of the path and then added the file name back without resolving it. When the file name is a symbolic link, the path that was checked and the place the file was actually written could be different, so a path could pass the check and still be written outside the allowed folders. This also brings the Python check in line with how the .NET `SessionsPythonPlugin` resolves download paths. ### Description <!-- Describe your changes, the overall approach, the underlying design. These notes will help understanding how your code works. Thanks! --> - `_validate_local_path_for_download` now resolves the whole path, including the file name, with `os.path.realpath`. It compares that result with the allowed folders and returns the same value, so the path that is checked is the path that is written. - A path that resolves to an allowed folder itself is still rejected. - Paths that resolve to a folder (for example `allowed/.`) are now rejected with the normal "not within allowed download directories" error. Before, they passed the check and failed later when opening the folder for writing. - No change when `allowed_download_directories` is not set, and no change to uploads. - The "denied" log line now also shows the resolved path. - New tests: - a link to a file outside the allowed folder is rejected, both when the target file exists and when it does not; - a link to a file inside the allowed folder still works. Validation: - `uv run --no-sync pytest tests/unit/core_plugins/test_sessions_python_plugin.py`: 89 passed - pre-commit hooks (ruff, ruff-format, pyupgrade, bandit and the rest) on both changed files: all passed ### Contribution Checklist <!-- Before submitting this PR, please make sure: --> - [x] The code builds clean without any errors or warnings - [x] The PR follows the [SK Contribution Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md) and the [pre-submission formatting script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts) raises no violations - [x] All unit tests pass, and I have added new tests where possible - [x] I didn't break anyone 😄
1 parent 3bbde09 commit ff22ec3

3 files changed

Lines changed: 75 additions & 11 deletions

File tree

‎python/.pre-commit-config.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ repos:
2121
name: Check Valid Python Samples
2222
types: ["python"]
2323
- repo: https://github.com/nbQA-dev/nbQA
24-
rev: 1.8.5
24+
rev: 1.10.0
2525
hooks:
2626
- id: nbqa-check-ast
2727
name: Check Valid Python Notebooks
2828
types: ["jupyter"]
2929
- repo: https://github.com/asottile/pyupgrade
30-
rev: v3.17.0
30+
rev: v3.21.2
3131
hooks:
3232
- id: pyupgrade
3333
args: [--py311-plus]

‎python/semantic_kernel/core_plugins/sessions_python_tool/sessions_python_plugin.py‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,9 @@ def _validate_local_path_for_upload(self, local_file_path: str) -> str:
230230
def _validate_local_path_for_download(self, local_file_path: str) -> str:
231231
"""Validate local path is within allowed download directories (optional protection).
232232
233+
The whole path is resolved, including the file name, so a symbolic link is judged by
234+
the location it points to and not by where the link itself lives.
235+
233236
Args:
234237
local_file_path: The path to validate.
235238
@@ -243,21 +246,18 @@ def _validate_local_path_for_download(self, local_file_path: str) -> str:
243246
if self.allowed_download_directories is None:
244247
return os.path.realpath(local_file_path)
245248

246-
parent_dir = os.path.dirname(local_file_path) or "."
247-
canonical_parent = os.path.realpath(parent_dir)
248-
filename = os.path.basename(local_file_path)
249-
canonical_path = os.path.join(canonical_parent, filename)
249+
canonical_path = os.path.realpath(local_file_path)
250250

251251
for allowed_dir in self.allowed_download_directories:
252252
allowed_canonical = os.path.realpath(allowed_dir)
253253
try:
254-
common = os.path.commonpath([allowed_canonical, canonical_parent])
255-
if common == allowed_canonical:
254+
common = os.path.commonpath([allowed_canonical, canonical_path])
255+
if common == allowed_canonical and canonical_path != allowed_canonical:
256256
return canonical_path
257257
except ValueError:
258-
continue
258+
continue # Different drives on Windows
259259

260-
logger.warning(f"Download denied for path: {local_file_path}")
260+
logger.warning(f"Download denied for path: {local_file_path} (resolved: {canonical_path})")
261261
raise FunctionExecutionException(
262262
f"Access denied: '{local_file_path}' is not within allowed download directories."
263263
)

‎python/tests/unit/core_plugins/test_sessions_python_plugin.py‎

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# Copyright (c) Microsoft. All rights reserved.
22

3-
from unittest.mock import Mock, mock_open, patch
3+
from unittest.mock import AsyncMock, Mock, mock_open, patch
44

55
import httpx
66
import pytest
@@ -1032,6 +1032,70 @@ async def async_return(result):
10321032
mock_get.assert_awaited_once()
10331033

10341034

1035+
def _symlink_or_skip(link, target):
1036+
try:
1037+
link.symlink_to(target)
1038+
except (OSError, NotImplementedError):
1039+
pytest.skip("Symbolic links cannot be created in this environment.")
1040+
1041+
1042+
async def _download_to(local_file_path, allowed_dir):
1043+
plugin = SessionsPythonTool(
1044+
auth_callback=lambda: "sample_token",
1045+
allowed_download_directories={str(allowed_dir)},
1046+
)
1047+
mock_request = httpx.Request(method="GET", url="https://example.com/files/content/remote.txt")
1048+
mock_response = httpx.Response(status_code=200, content=b"file data", request=mock_request)
1049+
with (
1050+
patch(
1051+
"semantic_kernel.core_plugins.sessions_python_tool.sessions_python_plugin.SessionsPythonTool._ensure_auth_token",
1052+
return_value="test_token",
1053+
),
1054+
patch("httpx.AsyncClient.get", new=AsyncMock(return_value=mock_response)),
1055+
):
1056+
await plugin.download_file(remote_file_name="remote.txt", local_file_path=str(local_file_path))
1057+
1058+
1059+
@pytest.mark.parametrize("target_exists", [True, False])
1060+
async def test_download_file_denied_through_symlink_to_outside(
1061+
aca_python_sessions_unit_test_env, tmp_path, target_exists
1062+
):
1063+
"""Test that a symlink inside an allowed directory cannot redirect the write outside it."""
1064+
allowed_dir = tmp_path / "allowed"
1065+
allowed_dir.mkdir()
1066+
outside_dir = tmp_path / "outside"
1067+
outside_dir.mkdir()
1068+
outside_file = outside_dir / "outside.txt"
1069+
if target_exists:
1070+
outside_file.write_bytes(b"original")
1071+
link = allowed_dir / "link.txt"
1072+
_symlink_or_skip(link, outside_file)
1073+
1074+
with pytest.raises(FunctionExecutionException, match="Access denied"):
1075+
await _download_to(link, allowed_dir)
1076+
1077+
if target_exists:
1078+
assert outside_file.read_bytes() == b"original"
1079+
else:
1080+
assert not outside_file.exists()
1081+
1082+
1083+
async def test_download_file_allowed_through_symlink_within_allowed_directory(
1084+
aca_python_sessions_unit_test_env, tmp_path
1085+
):
1086+
"""Test that a symlink whose target stays inside the allowed directory is still usable."""
1087+
allowed_dir = tmp_path / "allowed"
1088+
allowed_dir.mkdir()
1089+
target = allowed_dir / "target.txt"
1090+
link = allowed_dir / "link.txt"
1091+
_symlink_or_skip(link, target)
1092+
1093+
await _download_to(link, allowed_dir)
1094+
1095+
assert target.read_bytes() == b"file data"
1096+
assert link.is_symlink()
1097+
1098+
10351099
def test_allowed_directories_accepts_list(aca_python_sessions_unit_test_env):
10361100
"""Test that allowed directories can be passed as a list and are converted to a set."""
10371101
plugin = SessionsPythonTool(

0 commit comments

Comments
 (0)