Skip to content

Commit

Permalink
Do not warn http sources in push/delete scenarios when allowInsecureC…
Browse files Browse the repository at this point in the history 
…onnections is set to true. (#5381)
  • Loading branch information
@heng-liu
heng-liu committed Sep 8, 2023
1 parent c871727 commit 9014d80
Show file tree
Hide file tree
Showing 9 changed files with 179 additions and 38 deletions.
3 changes: 2 additions & 1 deletion src/NuGet.Core/NuGet.Commands/CommandRunners/DeleteRunner.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public static async Task Run(
source = CommandRunnerUtility.ResolveSource(sourceProvider, source);
PackageSource packageSource = CommandRunnerUtility.GetOrCreatePackageSource(sourceProvider, source);
// Only warn for V3 style sources because they have a service index which is different from the final push url.
if (packageSource.IsHttp && !packageSource.IsHttps &&
if (packageSource.IsHttp && !packageSource.IsHttps && !packageSource.AllowInsecureConnections &&
(packageSource.ProtocolVersion == 3 || packageSource.Source.EndsWith("json", StringComparison.OrdinalIgnoreCase)))
{
logger.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.Warning_HttpServerUsage, "delete", packageSource.Source));
Expand All @@ -42,6 +42,7 @@ await packageUpdateResource.Delete(
endpoint => apiKey ?? CommandRunnerUtility.GetApiKey(settings, endpoint, source),
desc => nonInteractive || confirmFunc(desc),
noServiceEndpoint,
packageSource.AllowInsecureConnections,
logger);
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/NuGet.Core/NuGet.Commands/CommandRunners/PushRunner.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public static async Task Run(
var packageUpdateResource = await CommandRunnerUtility.GetPackageUpdateResource(sourceProvider, packageSource);

// Only warn for V3 style sources because they have a service index which is different from the final push url.
if (packageSource.IsHttp && !packageSource.IsHttps &&
if (packageSource.IsHttp && !packageSource.IsHttps && !packageSource.AllowInsecureConnections &&
(packageSource.ProtocolVersion == 3 || packageSource.Source.EndsWith("json", StringComparison.OrdinalIgnoreCase)))
{
logger.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.Warning_HttpServerUsage, "push", packageSource.Source));
Expand Down Expand Up @@ -90,6 +90,7 @@ await packageUpdateResource.Push(
noServiceEndpoint,
skipDuplicate,
symbolPackageUpdateResource,
packageSource.AllowInsecureConnections,
logger);
}

Expand Down
2 changes: 2 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/PublicAPI/net472/PublicAPI.Unshipped.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ NuGet.Protocol.PackageVulnerabilitySeverity.Low = 0 -> NuGet.Protocol.PackageVul
NuGet.Protocol.PackageVulnerabilitySeverity.Moderate = 1 -> NuGet.Protocol.PackageVulnerabilitySeverity
NuGet.Protocol.PackageVulnerabilitySeverity.Unknown = -1 -> NuGet.Protocol.PackageVulnerabilitySeverity
~const NuGet.Protocol.JsonProperties.Url = "url" -> string
~NuGet.Protocol.Core.Types.PackageUpdateResource.Delete(string packageId, string packageVersion, System.Func<string, string> getApiKey, System.Func<string, bool> confirm, bool noServiceEndpoint, bool allowInsecureConnections, NuGet.Common.ILogger log) -> System.Threading.Tasks.Task
~NuGet.Protocol.Core.Types.PackageUpdateResource.Push(System.Collections.Generic.IList<string> packagePaths, string symbolSource, int timeoutInSecond, bool disableBuffering, System.Func<string, string> getApiKey, System.Func<string, string> getSymbolApiKey, bool noServiceEndpoint, bool skipDuplicate, NuGet.Protocol.Core.Types.SymbolPackageUpdateResourceV3 symbolPackageUpdateResource, bool allowInsecureConnections, NuGet.Common.ILogger log) -> System.Threading.Tasks.Task
2 changes: 2 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/PublicAPI/netcoreapp5.0/PublicAPI.Unshipped.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ NuGet.Protocol.PackageVulnerabilitySeverity.Low = 0 -> NuGet.Protocol.PackageVul
NuGet.Protocol.PackageVulnerabilitySeverity.Moderate = 1 -> NuGet.Protocol.PackageVulnerabilitySeverity
NuGet.Protocol.PackageVulnerabilitySeverity.Unknown = -1 -> NuGet.Protocol.PackageVulnerabilitySeverity
~const NuGet.Protocol.JsonProperties.Url = "url" -> string
~NuGet.Protocol.Core.Types.PackageUpdateResource.Delete(string packageId, string packageVersion, System.Func<string, string> getApiKey, System.Func<string, bool> confirm, bool noServiceEndpoint, bool allowInsecureConnections, NuGet.Common.ILogger log) -> System.Threading.Tasks.Task
~NuGet.Protocol.Core.Types.PackageUpdateResource.Push(System.Collections.Generic.IList<string> packagePaths, string symbolSource, int timeoutInSecond, bool disableBuffering, System.Func<string, string> getApiKey, System.Func<string, string> getSymbolApiKey, bool noServiceEndpoint, bool skipDuplicate, NuGet.Protocol.Core.Types.SymbolPackageUpdateResourceV3 symbolPackageUpdateResource, bool allowInsecureConnections, NuGet.Common.ILogger log) -> System.Threading.Tasks.Task
2 changes: 2 additions & 0 deletions src/NuGet.Core/NuGet.Protocol/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ NuGet.Protocol.PackageVulnerabilitySeverity.Low = 0 -> NuGet.Protocol.PackageVul
NuGet.Protocol.PackageVulnerabilitySeverity.Moderate = 1 -> NuGet.Protocol.PackageVulnerabilitySeverity
NuGet.Protocol.PackageVulnerabilitySeverity.Unknown = -1 -> NuGet.Protocol.PackageVulnerabilitySeverity
~const NuGet.Protocol.JsonProperties.Url = "url" -> string
~NuGet.Protocol.Core.Types.PackageUpdateResource.Delete(string packageId, string packageVersion, System.Func<string, string> getApiKey, System.Func<string, bool> confirm, bool noServiceEndpoint, bool allowInsecureConnections, NuGet.Common.ILogger log) -> System.Threading.Tasks.Task
~NuGet.Protocol.Core.Types.PackageUpdateResource.Push(System.Collections.Generic.IList<string> packagePaths, string symbolSource, int timeoutInSecond, bool disableBuffering, System.Func<string, string> getApiKey, System.Func<string, string> getSymbolApiKey, bool noServiceEndpoint, bool skipDuplicate, NuGet.Protocol.Core.Types.SymbolPackageUpdateResourceV3 symbolPackageUpdateResource, bool allowInsecureConnections, NuGet.Common.ILogger log) -> System.Threading.Tasks.Task
50 changes: 41 additions & 9 deletions src/NuGet.Core/NuGet.Protocol/Resources/PackageUpdateResource.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,21 @@ public Uri SourceUri
get { return UriUtility.CreateSourceUri(_source); }
}

public async Task Push(
IList<string> packagePaths,
string symbolSource, // empty to not push symbols
int timeoutInSecond,
bool disableBuffering,
Func<string, string> getApiKey,
Func<string, string> getSymbolApiKey,
bool noServiceEndpoint,
bool skipDuplicate,
SymbolPackageUpdateResourceV3 symbolPackageUpdateResource,
ILogger log)
{
await Push(packagePaths, symbolSource, timeoutInSecond, disableBuffering, getApiKey, getSymbolApiKey, noServiceEndpoint, skipDuplicate, symbolPackageUpdateResource, allowInsecureConnections: false, log);
}

public async Task Push(
IList<string> packagePaths,
string symbolSource, // empty to not push symbols
Expand All @@ -62,6 +77,7 @@ public async Task Push(
bool noServiceEndpoint,
bool skipDuplicate,
SymbolPackageUpdateResourceV3 symbolPackageUpdateResource,
bool allowInsecureConnections,
ILogger log)
{
// TODO: Figure out how to hook this up with the HTTP request
Expand All @@ -78,7 +94,7 @@ public async Task Push(
{
// Push nupkgs and possibly the corresponding snupkgs.
await PushPackagePath(packagePath, _source, symbolSource, apiKey, getSymbolApiKey, noServiceEndpoint, skipDuplicate,
symbolPackageUpdateResource, requestTimeout, log, tokenSource.Token);
symbolPackageUpdateResource, allowInsecureConnections, requestTimeout, log, tokenSource.Token);
}
else // Explicit snupkg push
{
Expand All @@ -90,7 +106,7 @@ await PushPackagePath(packagePath, _source, symbolSource, apiKey, getSymbolApiKe
string symbolApiKey = getSymbolApiKey(symbolSource);

await PushSymbolsPath(packagePath, symbolSource, symbolApiKey,
noServiceEndpoint, skipDuplicate, symbolPackageUpdateResource,
noServiceEndpoint, skipDuplicate, symbolPackageUpdateResource, allowInsecureConnections,
requestTimeout, log, tokenSource.Token);
}
}
Expand Down Expand Up @@ -144,6 +160,17 @@ public async Task Delete(string packageId,
Func<string, bool> confirm,
bool noServiceEndpoint,
ILogger log)
{
await Delete(packageId, packageVersion, getApiKey, confirm, noServiceEndpoint, allowInsecureConnections: false, log);
}

public async Task Delete(string packageId,
string packageVersion,
Func<string, string> getApiKey,
Func<string, bool> confirm,
bool noServiceEndpoint,
bool allowInsecureConnections,
ILogger log)
{
var sourceDisplayName = GetSourceDisplayName(_source);
var apiKey = getApiKey(_source);
Expand All @@ -163,7 +190,7 @@ public async Task Delete(string packageId,
packageVersion,
sourceDisplayName
));
await DeletePackage(_source, apiKey, packageId, packageVersion, noServiceEndpoint, log, CancellationToken.None);
await DeletePackage(_source, apiKey, packageId, packageVersion, noServiceEndpoint, allowInsecureConnections, log, CancellationToken.None);
log.LogInformation(string.Format(CultureInfo.CurrentCulture,
Strings.DeleteCommandDeletedPackage,
packageId,
Expand All @@ -181,6 +208,7 @@ private async Task PushSymbolsPath(string packagePath,
bool noServiceEndpoint,
bool skipDuplicate,
SymbolPackageUpdateResourceV3 symbolPackageUpdateResource,
bool allowInsecureConnections,
TimeSpan requestTimeout,
ILogger log,
CancellationToken token)
Expand Down Expand Up @@ -214,7 +242,7 @@ private async Task PushSymbolsPath(string packagePath,
bool warnForHttpSources = true;
foreach (string packageToPush in symbolsToPush)
{
await PushPackageCore(symbolSource, apiKey, packageToPush, noServiceEndpoint, skipDuplicate, requestTimeout, warnForHttpSources, log, token);
await PushPackageCore(symbolSource, apiKey, packageToPush, noServiceEndpoint, skipDuplicate, requestTimeout, warnForHttpSources, allowInsecureConnections, log, token);
warnForHttpSources = false;
}
}
Expand All @@ -232,6 +260,7 @@ private async Task PushPackagePath(string packagePath,
bool noServiceEndpoint,
bool skipDuplicate,
SymbolPackageUpdateResourceV3 symbolPackageUpdateResource,
bool allowInsecureConnections,
TimeSpan requestTimeout,
ILogger log,
CancellationToken token)
Expand All @@ -257,7 +286,7 @@ private async Task PushPackagePath(string packagePath,
bool warnForHttpSources = true;
foreach (string nupkgToPush in nupkgsToPush)
{
bool packageWasPushed = await PushPackageCore(source, apiKey, nupkgToPush, noServiceEndpoint, skipDuplicate, requestTimeout, warnForHttpSources, log, token);
bool packageWasPushed = await PushPackageCore(source, apiKey, nupkgToPush, noServiceEndpoint, skipDuplicate, requestTimeout, warnForHttpSources, allowInsecureConnections, log, token);
// Push corresponding symbols, if successful.
if (packageWasPushed && !string.IsNullOrEmpty(symbolSource))
{
Expand Down Expand Up @@ -287,7 +316,7 @@ private async Task PushPackagePath(string packagePath,
}

string symbolApiKey = getSymbolApiKey(symbolSource);
await PushPackageCore(symbolSource, symbolApiKey, symbolPackagePath, noServiceEndpoint, skipDuplicate, requestTimeout, warnForHttpSources, log, token);
await PushPackageCore(symbolSource, symbolApiKey, symbolPackagePath, noServiceEndpoint, skipDuplicate, requestTimeout, warnForHttpSources, allowInsecureConnections, log, token);
}
warnForHttpSources = false;
}
Expand All @@ -300,6 +329,7 @@ private async Task<bool> PushPackageCore(string source,
bool skipDuplicate,
TimeSpan requestTimeout,
bool warnForHttpSources,
bool allowInsecureConnections,
ILogger log,
CancellationToken token)
{
Expand All @@ -320,7 +350,7 @@ private async Task<bool> PushPackageCore(string source,
else
{
wasPackagePushed = await PushPackageToServer(source, apiKey, packageToPush, noServiceEndpoint, skipDuplicate,
requestTimeout, warnForHttpSources, log, token);
requestTimeout, warnForHttpSources, allowInsecureConnections, log, token);
}

if (wasPackagePushed)
Expand Down Expand Up @@ -370,14 +400,15 @@ private async Task<bool> PushPackageToServer(string source,
bool skipDuplicate,
TimeSpan requestTimeout,
bool warnForHttpSources,
bool allowInsecureConnections,
ILogger logger,
CancellationToken token)
{
Uri serviceEndpointUrl = GetServiceEndpointUrl(source, string.Empty, noServiceEndpoint);
bool useTempApiKey = IsSourceNuGetSymbolServer(source);
HttpStatusCode? codeNotToThrow = ConvertSkipDuplicateParamToHttpStatusCode(skipDuplicate);
bool showPushCommandPackagePushed = true;
if (warnForHttpSources && serviceEndpointUrl.Scheme == Uri.UriSchemeHttp)
if (warnForHttpSources && serviceEndpointUrl.Scheme == Uri.UriSchemeHttp && !allowInsecureConnections)
{
logger.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.Warning_HttpServerUsage, "push", serviceEndpointUrl));
}
Expand Down Expand Up @@ -668,6 +699,7 @@ private async Task DeletePackage(string source,
string packageId,
string packageVersion,
bool noServiceEndpoint,
bool allowInsecureConnections,
ILogger logger,
CancellationToken token)
{
Expand All @@ -678,7 +710,7 @@ private async Task DeletePackage(string source,
}
else
{
if (sourceUri.Scheme == Uri.UriSchemeHttp)
if (sourceUri.Scheme == Uri.UriSchemeHttp && !allowInsecureConnections)
{
logger.LogWarning(string.Format(CultureInfo.CurrentCulture, Strings.Warning_HttpServerUsage, "delete", sourceUri));
}
Expand Down
32 changes: 28 additions & 4 deletions test/NuGet.Clients.Tests/NuGet.CommandLine.Test/NuGetDeleteCommandTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -414,8 +414,10 @@ public void DeleteCommand_Failure_InvalidArguments(string args)
Util.TestCommandInvalidArguments(args);
}

[Fact]
public void DeleteCommand_WhenDeleteWithHttpSource_Warns()
[Theory]
[InlineData("true", false)]
[InlineData("false", true)]
public void DeleteCommand_WhenDeleteWithHttpSourceAndAllowInsecureConnections_WarnsCorrectly(string allowInsecureConnections, bool isHttpWarningExpected)
{
var nugetexe = Util.GetNuGetExePath();

Expand All @@ -431,10 +433,23 @@ public void DeleteCommand_WhenDeleteWithHttpSource_Warns()
return HttpStatusCode.OK;
});

using SimpleTestPathContext config = new SimpleTestPathContext();

// Arrange the NuGet.Config file
string nugetConfigContent =
$@"<configuration>
<packageSources>
<clear />
<add key='http-feed' value='{server.Uri}nuget' protocalVersion=""3"" allowInsecureConnections=""{allowInsecureConnections}"" />
</packageSources>
</configuration>";
File.WriteAllText(config.NuGetConfig, nugetConfigContent);

// Act
string[] args = new string[] {
"delete", "testPackage1", "1.1.0",
"-Source", server.Uri + "nuget", "-NonInteractive" };
"-Source", server.Uri + "nuget",
"-ConfigFile", config.NuGetConfig, "-NonInteractive" };

var r = CommandRunner.Run(
nugetexe,
Expand All @@ -444,7 +459,16 @@ public void DeleteCommand_WhenDeleteWithHttpSource_Warns()
// Assert
Assert.Equal(0, r.ExitCode);
Assert.True(deleteRequestIsCalled);
Assert.Contains("WARNING: You are running the 'delete' operation with an 'HTTP' source", r.AllOutput);

string expectedWarning = "WARNING: You are running the 'delete' operation with an 'HTTP' source";
if (isHttpWarningExpected)
{
Assert.Contains(expectedWarning, r.AllOutput);
}
else
{
Assert.DoesNotContain(expectedWarning, r.AllOutput);
}
}
}

Expand Down
Loading

0 comments on commit 9014d80

Please sign in to comment.