Warn when http sources are used in packages.config restore operations #4583

erdembayar · 2022-04-19T01:21:17Z

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1519

Regression? Last working version:

Description

Pretty straightforward. Contains the same warning as #4552, but obviously with a code since it's restore.

PR Checklist

jeffkl · 2022-04-19T17:38:48Z

Team Review: Add unit tests for the two other code paths where the warning is logged

erdembayar · 2022-04-21T00:27:46Z

Team Review: Add unit tests for the two other code paths where the warning is logged

@NuGet/nuget-client
I added tests for 2 other code paths.

nkolev92

You're missing packages.config restore in Visual Studio.

src/NuGet.Clients/NuGet.CommandLine/Commands/InstallCommand.cs

erdembayar · 2022-04-23T01:06:42Z

You're missing packages.config restore in Visual Studio.

I just tested PR restore in Visual Studio and I don't see any warning in "Package Manager console".
It seems restore logging in Visual Studio has a bug.
When I'm debugging packages.config restore in VS, I can see it's logging something but nothing in "Package Manager console".

I can file separate bug for this. Does it sound fair?

nkolev92 · 2022-04-25T16:43:21Z

I just tested PR restore in Visual Studio and I don't see any warning in "Package Manager console".

I assume you mean PC restore right?

When I'm debugging packages.config restore in VS, I can see it's logging something but nothing in "Package Manager console".

Are you looking in PMC?
We have an output window titled Package Manager, check there.

erdembayar · 2022-04-25T17:19:28Z

I just tested PR restore in Visual Studio and I don't see any warning in "Package Manager console".

I assume you mean PC restore right?

When I'm debugging packages.config restore in VS, I can see it's logging something but nothing in "Package Manager console".

Are you looking in PMC? We have an output window titled Package Manager, check there.

Sorry for naming confusion let me clear it for you.

1. PMUI scenario
I tested both PC and PR with PMUI install scenario, both cases I don't see any warning in Output >> PackageManager, I suspect there is a logging bug here. Would you like me file bug for it?
PR:

PC:

2. PMC scenario
Then I tested both PC and PR in PMC window

PR: I can see asset file include text2xml entry, that means restore did happen.

PC:

By the way I did clean up cache between repro cycles and I used below 2 sources for tests:

nkolev92 · 2022-04-25T17:26:04Z

Installation in packages.config is different from installation in PackageReference.
We discussed shortly in one of the teams channels, it's all restore in PackageReference. In packages.config, they are different gestures.

We'll have a different issue for installation scenarios.

nkolev92 · 2022-04-25T17:27:24Z

I originally thought your branch didn't have my changes, but it seems like now does. How are you testing that the PR scenarios?

I'll do some tests on my end, as that's something I expect to work.

erdembayar · 2022-04-25T18:31:55Z

I originally thought your branch didn't have my changes, but it seems like now does. How are you testing that the PR scenarios?

I'll do some tests on my end, as that's something I expect to work.

I just tested in VS PMUI and VS PMC with below 2 feeds.

    <add key="nuget" value="http://api.nuget.org/v3/index.json" />
    <add key="myinsecure" value="http://nuget.org/api/v2" />

nkolev92 · 2022-04-25T18:42:39Z

My question is more about what version of VS you are testing. Are you using experimental instance etc.

erdembayar · 2022-04-25T19:36:03Z

My question is more about what version of VS you are testing. Are you using experimental instance etc.

Yes, it's experimental instance for VS 17.2 Preview 4.

nkolev92 · 2022-04-25T21:30:16Z

@erdembayar

Try using a legacy package reference project.

For SDK based projects, the warnings are added in by the SDK and given that the SDK doesn't know about NU1803 it can't parse it.

Root cause explained: NuGet/Home#7126

erdembayar · 2022-04-26T00:24:54Z

@erdembayar

Try using a legacy package reference project.

For SDK based projects, the warnings are added in by the SDK and given that the SDK doesn't know about NU1803 it can't parse it.

Root cause explained: NuGet/Home#7126

You're right it works for legacy PR warnings are added.

Does above apply for package PMUI install of packages.config projects too?

nkolev92 · 2022-04-26T00:26:59Z

Does above apply for package PMUI install of packages.config projects too

What does above mean?

erdembayar · 2022-04-26T00:32:20Z

Does above apply for package PMUI install of packages.config projects too

What does above mean?

Install package to PC project via PMUI, I don't see any warning, but it's not sdk based so just wandering why:

erdembayar · 2022-04-26T00:55:32Z

Does above apply for package PMUI install of packages.config projects too

What does above mean?

Install package to PC project via PMUI, I don't see any warning, but it's not sdk based so just wandering why: ![image]

Never mind, I was using wrong logger, I changed it. Now both PMC and PMUI warnings are working.

It's ready to review.

erdembayar · 2022-04-26T19:56:14Z

@NuGet/nuget-client
It's ready to review.

nkolev92 · 2022-04-26T22:58:32Z

When you say you PMC and PMUI warnings work, can you clarify what commands you are running there?

nkolev92

Looks pretty good overall.

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/InstallCommandTests.cs

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/RestoreCommandTests.cs

test/NuGet.Core.FuncTests/Msbuild.Integration.Test/MsbuildRestoreTaskTests.cs

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/RestoreCommandTests.cs

test/NuGet.Clients.FuncTests/NuGet.CommandLine.FuncTest/Commands/InstallCommandTests.cs

nkolev92 · 2022-04-26T22:10:03Z

src/NuGet.Core/NuGet.PackageManagement/IDE/PackageRestoreManager.cs

@@ -377,6 +378,22 @@ private NuGetPackageManager GetNuGetPackageManager(string solutionDirectory)

            packageRestoreContext.Token.ThrowIfCancellationRequested();

+            IEnumerable<SourceRepository> enabledSources =
+                packageRestoreContext


packageRestoreContext has the SourceRepositories. Can we use those?

I think we can use packageRestoreContext.SourceRepositories because the collection is populated with sources that are enabled.

NuGet.Client/src/NuGet.Core/NuGet.Protocol/CachingSourceProvider.cs

Lines 33 to 36 in aca9465

_repositories = _packageSourceProvider.LoadPackageSources()

.Where(s => s.IsEnabled)

.Select(CreateRepository)

.ToList();

I had to add back this change, because some code paths sourceRepositories is set explicitly null so we can't use packageRestoreContext.SourceRepositories that case.

NuGet.Client/src/NuGet.Core/NuGet.PackageManagement/IDE/PackageRestoreManager.cs

Line 312 in f52a839

sourceRepositories: null,

I'd really love it if we can avoid adding any public methods in NuGetPackageManager.

I tried to follow RestorePackageAsync into NuGetPackageManager and it uses the ISourceRepositoryProvider to get the providers if they are not set on the context.

Maybe we can just set them?

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs

erdembayar · 2022-05-02T23:49:47Z

@nkolev92 @kartheekp-ms
I addressed PR comments.

kartheekp-ms · 2022-05-06T19:02:50Z

src/NuGet.Core/NuGet.PackageManagement/IDE/PackageRestoreManager.cs

+                PackageSource source = enabledSource.PackageSource;
+                if (source.IsHttp && !source.IsHttps)
+                {
+                    packageRestoreContext.Logger.Log(LogLevel.Warning, string.Format(CultureInfo.CurrentCulture, Strings.Warning_HttpServerUsage, "restore", source.Source));


I think we need to add log code to the warning. Similar to PR restore logic.

NuGet.Client/src/NuGet.Core/NuGet.Commands/RestoreCommand/RestoreCommand.cs

Lines 223 to 224 in f52a839

await _logger.LogAsync(RestoreLogMessage.CreateWarning(NuGetLogCode.NU1803,

string.Format(CultureInfo.CurrentCulture, Strings.Warning_HttpServerUsage, "restore", source.Source)));

log codes only work in PackageReference.
For consistency we try not to add the log codes in packages.config as none of the resolver warnings/errors have any codes.

For consistency we try not to add the log codes in packages.config as none of the resolver warnings/errors have any codes.

I don't understand this. With a log code, the customer has something to search for (or in VS, something to click in the Error List window), and then there can be a docs page where we can provide more detailed instructions on how to resolve the error.

Why is consistency with not using log codes matter more than this?

You can't suppress warnings or elevate warnings in packages.config. Log codes usually suggest that you can do that.
Also warnings/errors in packages.config and PackageReference are different because pc restore rarely puts any log messages there.

I think the message itself should be actionable enough.

erdembayar requested a review from a team as a code owner April 19, 2022 01:21

erdembayar force-pushed the dev-eryondon-warn-httpconnection-packagesconfigprojects branch from f72d714 to 4d9725d Compare April 21, 2022 00:25

erdembayar requested review from jeffkl, nkolev92 and kartheekp-ms April 21, 2022 00:29

nkolev92 reviewed Apr 21, 2022

View reviewed changes

nkolev92 reviewed Apr 26, 2022

View reviewed changes

kartheekp-ms reviewed Apr 27, 2022

View reviewed changes

src/NuGet.Core/NuGet.PackageManagement/NuGetPackageManager.cs Outdated Show resolved Hide resolved

nkolev92 previously approved these changes May 2, 2022

View reviewed changes

erdembayar dismissed nkolev92’s stale review via cd707e8 May 3, 2022 01:13

erdembayar force-pushed the dev-eryondon-warn-httpconnection-packagesconfigprojects branch 2 times, most recently from cd707e8 to 25a1ec4 Compare May 4, 2022 21:08

jeffkl previously approved these changes May 5, 2022

View reviewed changes

erdembayar dismissed jeffkl’s stale review via e86a84c May 6, 2022 00:42

kartheekp-ms reviewed May 6, 2022

View reviewed changes

nkolev92 previously approved these changes May 10, 2022

View reviewed changes

erdembayar added 7 commits May 10, 2022 18:33

Warn on http source with packages.config restore

e152146

Add more test

b453d20

Address PR comments

5293cdb

Fix logger

62d59ce

Address PR review comments

13dfb9e

Fix test errors

62bda32

Inject sourceprovider

fe739a9

erdembayar dismissed nkolev92’s stale review via fe739a9 May 11, 2022 01:34

erdembayar force-pushed the dev-eryondon-warn-httpconnection-packagesconfigprojects branch from e86a84c to fe739a9 Compare May 11, 2022 01:34

nkolev92 merged commit b647254 into dev May 11, 2022

nkolev92 deleted the dev-eryondon-warn-httpconnection-packagesconfigprojects branch May 11, 2022 23:30

heng-liu mentioned this pull request Aug 1, 2023

Opt-out of "HTTPS everywhere" : do not warn for HTTP sources in packages.config restore scenarios in all NuGet tools when allowInsecureConnections is set to true NuGet/Home#12788

Closed

heng-liu mentioned this pull request Aug 11, 2023

Opt out Https everywhere in packages.config restore scenarios in all NuGet tools #5356

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn when http sources are used in packages.config restore operations #4583

Warn when http sources are used in packages.config restore operations #4583

erdembayar commented Apr 19, 2022

jeffkl commented Apr 19, 2022

erdembayar commented Apr 21, 2022

nkolev92 left a comment

erdembayar commented Apr 23, 2022 •

edited

Loading

nkolev92 commented Apr 25, 2022

erdembayar commented Apr 25, 2022 •

edited

Loading

nkolev92 commented Apr 25, 2022 •

edited

Loading

nkolev92 commented Apr 25, 2022

erdembayar commented Apr 25, 2022

nkolev92 commented Apr 25, 2022

erdembayar commented Apr 25, 2022

nkolev92 commented Apr 25, 2022

erdembayar commented Apr 26, 2022 •

edited

Loading

nkolev92 commented Apr 26, 2022

erdembayar commented Apr 26, 2022

erdembayar commented Apr 26, 2022 •

edited

Loading

erdembayar commented Apr 26, 2022

nkolev92 commented Apr 26, 2022

nkolev92 left a comment

nkolev92 Apr 26, 2022

kartheekp-ms Apr 27, 2022

erdembayar May 5, 2022

nkolev92 May 5, 2022

erdembayar commented May 2, 2022

kartheekp-ms May 6, 2022 •

edited

Loading

nkolev92 May 6, 2022 •

edited

Loading

zivkan May 6, 2022

nkolev92 May 6, 2022

	_repositories = _packageSourceProvider.LoadPackageSources()
	.Where(s => s.IsEnabled)
	.Select(CreateRepository)
	.ToList();

	await _logger.LogAsync(RestoreLogMessage.CreateWarning(NuGetLogCode.NU1803,
	string.Format(CultureInfo.CurrentCulture, Strings.Warning_HttpServerUsage, "restore", source.Source)));

Warn when http sources are used in packages.config restore operations #4583

Warn when http sources are used in packages.config restore operations #4583

Conversation

erdembayar commented Apr 19, 2022

Bug

Description

PR Checklist

jeffkl commented Apr 19, 2022

erdembayar commented Apr 21, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

erdembayar commented Apr 23, 2022 • edited Loading

nkolev92 commented Apr 25, 2022

erdembayar commented Apr 25, 2022 • edited Loading

nkolev92 commented Apr 25, 2022 • edited Loading

nkolev92 commented Apr 25, 2022

erdembayar commented Apr 25, 2022

nkolev92 commented Apr 25, 2022

erdembayar commented Apr 25, 2022

nkolev92 commented Apr 25, 2022

erdembayar commented Apr 26, 2022 • edited Loading

nkolev92 commented Apr 26, 2022

erdembayar commented Apr 26, 2022

erdembayar commented Apr 26, 2022 • edited Loading

erdembayar commented Apr 26, 2022

nkolev92 commented Apr 26, 2022

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 Apr 26, 2022

Choose a reason for hiding this comment

kartheekp-ms Apr 27, 2022

Choose a reason for hiding this comment

erdembayar May 5, 2022

Choose a reason for hiding this comment

nkolev92 May 5, 2022

Choose a reason for hiding this comment

erdembayar commented May 2, 2022

kartheekp-ms May 6, 2022 • edited Loading

Choose a reason for hiding this comment

nkolev92 May 6, 2022 • edited Loading

Choose a reason for hiding this comment

zivkan May 6, 2022

Choose a reason for hiding this comment

nkolev92 May 6, 2022

Choose a reason for hiding this comment

erdembayar commented Apr 23, 2022 •

edited

Loading

erdembayar commented Apr 25, 2022 •

edited

Loading

nkolev92 commented Apr 25, 2022 •

edited

Loading

erdembayar commented Apr 26, 2022 •

edited

Loading

erdembayar commented Apr 26, 2022 •

edited

Loading

kartheekp-ms May 6, 2022 •

edited

Loading

nkolev92 May 6, 2022 •

edited

Loading