Skip to content

Commit 4721002

Browse files
committed
Address PR comments
1 parent 93a974d commit 4721002

2 files changed

Lines changed: 15 additions & 34 deletions

File tree

‎dotnet/src/Plugins/Plugins.Core/CodeInterpreter/SessionsPythonPlugin.cs‎

Lines changed: 9 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -128,16 +128,16 @@ public async Task<SessionsRemoteFileMetadata> UploadFileAsync(
128128
Verify.NotNullOrWhiteSpace(localFilePath, nameof(localFilePath));
129129

130130
ValidateRemoteFileName(remoteFileName);
131-
var validatedLocalPath = this.ValidateLocalPathForUpload(localFilePath);
132-
133-
this._logger.LogInformation("Uploading file: {LocalFilePath} to {RemoteFileName}", validatedLocalPath, remoteFileName);
134-
135-
using var httpClient = this._httpClientFactory.CreateClient();
131+
this.ValidateFileUploadConfiguration();
136132

137133
using var localFile = SecureFileSystem.OpenRead(
138134
localFilePath,
139135
this._settings.AllowedUploadDirectories!,
140-
out _);
136+
out var resolvedLocalPath);
137+
138+
this._logger.LogInformation("Uploading file: {LocalFilePath} to {RemoteFileName}", resolvedLocalPath, remoteFileName);
139+
140+
using var httpClient = this._httpClientFactory.CreateClient();
141141
using var fileContent = new StreamContent(localFile);
142142

143143
using var multipartFormDataContent = new MultipartFormDataContent()
@@ -317,12 +317,10 @@ private static void ValidateRemoteFileName(string remoteFileName)
317317
}
318318

319319
/// <summary>
320-
/// Validates that the local file path is within allowed upload directories.
320+
/// Validates that local file uploads are enabled and configured.
321321
/// </summary>
322-
/// <param name="localFilePath">The local file path to validate.</param>
323-
/// <returns>The canonicalized path if valid.</returns>
324-
/// <exception cref="InvalidOperationException">Thrown when file operations are disabled or the path is not allowed.</exception>
325-
private string ValidateLocalPathForUpload(string localFilePath)
322+
/// <exception cref="InvalidOperationException">Thrown when file uploads are disabled or allowed directories are not configured.</exception>
323+
private void ValidateFileUploadConfiguration()
326324
{
327325
if (!this._settings.EnableDangerousFileUploads)
328326
{
@@ -335,26 +333,6 @@ private string ValidateLocalPathForUpload(string localFilePath)
335333
throw new InvalidOperationException(
336334
"File upload requires 'AllowedUploadDirectories' to be configured.");
337335
}
338-
339-
var canonicalPath = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(localFilePath));
340-
341-
foreach (var allowedDir in this._settings.AllowedUploadDirectories)
342-
{
343-
var canonicalAllowedDir = PathUtilities.GetSafeFullPath(SecureFileSystem.NormalizePath(allowedDir));
344-
// Ensure we match the directory correctly by appending separator
345-
var separator = Path.DirectorySeparatorChar.ToString();
346-
var allowedDirWithSeparator = canonicalAllowedDir.EndsWith(separator, PathUtilities.PathComparison)
347-
? canonicalAllowedDir
348-
: canonicalAllowedDir + separator;
349-
350-
if (canonicalPath.StartsWith(allowedDirWithSeparator, PathUtilities.PathComparison))
351-
{
352-
return canonicalPath;
353-
}
354-
}
355-
356-
throw new InvalidOperationException(
357-
$"Access denied: '{localFilePath}' is not within allowed upload directories.");
358336
}
359337

360338
/// <summary>

‎dotnet/src/Plugins/Plugins.UnitTests/Core/SessionsPythonPluginTests.cs‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -562,7 +562,8 @@ public async Task ItShouldDenyUploadOutsideAllowedDirectoriesAsync()
562562
var exception = await Assert.ThrowsAsync<InvalidOperationException>(
563563
() => plugin.UploadFileAsync("test.txt", FileTestDataFilePath));
564564

565-
Assert.Contains("not within allowed upload directories", exception.Message);
565+
Assert.Contains("not within allowed directories", exception.Message);
566+
Assert.Null(this._messageHandlerStub.RequestUri);
566567
}
567568

568569
[Fact]
@@ -771,7 +772,8 @@ public async Task ItShouldDenyUploadWithDifferentPathCasingOnLinuxAsync()
771772
var exception = await Assert.ThrowsAsync<InvalidOperationException>(
772773
() => plugin.UploadFileAsync("test.txt", disallowedFile));
773774

774-
Assert.Contains("not within allowed upload directories", exception.Message);
775+
Assert.Contains("not within allowed directories", exception.Message);
776+
Assert.Null(this._messageHandlerStub.RequestUri);
775777
}
776778
finally
777779
{
@@ -805,7 +807,8 @@ public async Task ItShouldDenyUploadWithPathTraversalAsync()
805807
var exception = await Assert.ThrowsAsync<InvalidOperationException>(
806808
() => plugin.UploadFileAsync("test.txt", traversalPath));
807809

808-
Assert.Contains("not within allowed upload directories", exception.Message);
810+
Assert.Contains("not within allowed directories", exception.Message);
811+
Assert.Null(this._messageHandlerStub.RequestUri);
809812
}
810813

811814
public void Dispose()

0 commit comments

Comments
 (0)