[Winoren (Winoren)]

Motivation and Context

download_file rejects . and .. path segments in the remote file name (#14520). The upload direction of the same plugin never got the equivalent guard: _construct_remote_file_path only prefixed /mnt/data/ and passed the rest through, so a model-supplied remote_file_path of ../../escape.txt was transmitted to the sessions service as /mnt/data/../../escape.txt.

The two directions of one plugin therefore disagreed on the same input, and the permissive one is reachable straight from a tool call:

LLM tool call: upload_file(remote_file_path="../../escape.txt", local_file_path=<allowed dir>)
  upload_file                  -> _validate_local_path_for_upload   (local side only, passes)
  upload_file                  -> _construct_remote_file_path        ("/mnt/data/../../escape.txt")
  http_client.post(files={"file": (remote_file_path, ...)})         -> leaves the process

remote_file_path is a string supplied by the model, not a path object, so nothing between the tool argument and the request body rejects the segments. Local validation cannot help here, which is why #13478's allowed_upload_directories check does not cover this case.

Description

Applies the guard in the same helper download_file uses, with the same exception type and the same split on /, so the check cannot drift from the download side again:

if any(segment in (".", "..") for segment in remote_file_path.split("/")):
    raise FunctionExecutionException("The remote file path must not contain '.' or '..' path segments.")

Placing it in _construct_remote_file_path rather than inline in upload_file means any future caller is covered by default.

Testing

• New cases extend the existing test_sessions_python_plugin.py; no new file.
• The 9 malicious inputs (".", "..", "../x", "../../x", "./x", "nested/./x", "nested/../x", "nested/..", "/mnt/data/../../x") fail with DID NOT RAISE when the guard is reverted, and pass with it in place.
• 3 further cases assert ordinary paths still upload unchanged, including a nested path, an already-prefixed path, and they pass both with and without the fix, so they guard against over-rejection rather than coupling to the change.
• Over a 12-case differential sweep, 10 inputs are byte-identical before and after. The only behavior change is rejecting the dot segments. Legitimate names are untouched, including .hidden.txt and a..b/c.txt, since neither is a standalone . or .. segment.
• ruff check, ruff format --check and mypy are clean on both files.

Note on the local test run: the errors seen in this file's fixtures are reproducible on unmodified main in my environment (a local dependency gap), so they are not caused by this change.

One thing I did not change

SessionsPythonPlugin.UploadFileAsync on the .NET side has the same gap - it only does Verify.NotNullOrWhiteSpace(remoteFileName). I kept this PR to the Python side to stay reviewable and single-purpose, since your recent work on this plugin has paired .NET and Python (e.g. #14085, #14140). Happy to extend it if you would rather land both together.

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟢 Approval recommended

The focused validation preserves ordinary path handling, has regression coverage, and has no identified blocking issues.

Review effort: Balanced
Findings: None

What changed in this PR

Adds remote-path validation to Python session uploads, matching the existing download restriction on literal . and .. path segments.

Changes:

• Reject dot segments in the remote-path helper and document the constraint.
• Add regression tests for rejected paths and preserved ordinary paths.

File	Description
python/​tests/​unit/​core_plugins/​test_sessions_python_plugin.py	Tests rejected dot segments and accepted ordinary paths.
python/​semantic_kernel/​core_plugins/​sessions_python_tool/​sessions_python_plugin.py	Adds and documents remote-path validation.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Winoren (Winoren)]

@microsoft-github-policy-service agree

[PRABHU KIRAN VANDRANKI (VANDRANKI)]

Community review. I am not a maintainer, so this does not clear the merge gate.

I read the full diff and the surrounding sessions_python_plugin.py (upload_file, _construct_remote_file_path, and the existing check in download_file), then ran the tests and a probe on the PR head (python/ in a venv, Python 3.13, no network).

What it does: _construct_remote_file_path now rejects any /-separated segment equal to . or .. with a FunctionExecutionException, which is the same check download_file already does on remote_file_name (line 448). _construct_remote_file_path is only called from upload_file (line 366), so the change affects only uploads. The check runs on the raw argument before the /mnt/data/ prefix is added, and before any auth or network call.

What I ran:

• tests/unit/core_plugins/test_sessions_python_plugin.py on the PR head: 86 passed. With semantic_kernel/ taken from origin/main, the 9 new test_upload_file_rejects_dot_segments[...] cases fail and the rest pass. The three "ordinary path" cases in test_upload_file_allows_ordinary_remote_paths pass on both, so they act as a guard and not as a bug repro.
• A direct probe of _construct_remote_file_path: a/../b, /mnt/data/../x, a/./b, ./ and x/.. are rejected. .hidden, ..., a/.../b, dir/ and a//b are accepted and come out under /mnt/data/ unchanged.

Observations, none of them blocking:

• The check is on literal segments only. %2e%2e/b and a/%2e%2e/b pass through unchanged (/mnt/data/%2e%2e/b). In download_file the name is URL-quoted, so that string would be double-encoded there. For upload the value is sent as the multipart filename (files = {"file": (remote_file_path, ...)}), and I did not check how the sessions service treats an encoded name, so I do not know whether it matters. A leading space ( ../b) also passes. I only mention these so you know where the line is drawn.
• The new test docstring says upload must reject the same segments as download. The two error messages differ slightly ("remote file path" versus "remote file name"), which is fine, and the tests match on "path segments" only.

I did not run the integration tests or CI, and I did not try the real Azure sessions service.

I have no blocking concerns: the change does what it says, it matches the existing download check, and the new tests fail without it.

[github-actions]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
core_plugins/sessions_python_tool
sessions_python_plugin.py	183	11	93%	101, 125, 127–128, 130, 132, 134, 222–223, 257–258
TOTAL	29126	5480	81%

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
4488	22 💤	0 ❌	0 🔥	2m 24s ⏱️