-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add HTTP errors to dotnet tool install scenarios (NU1302) with allowInsecureConnections support and E2E tests #50027
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: marcpopMSFT <[email protected]>
Co-authored-by: marcpopMSFT <[email protected]>
|
@copilot this warnings should be an error as it was made an error in nuget restore starting in .NET 9. We should also handle the workaround provided by nuget. Here is the full nuget error for reference. |
Co-authored-by: marcpopMSFT <[email protected]>
I've updated the implementation to use errors instead of warnings, matching the NU1302 error format from NuGet restore. The changes include:
However, the The core error functionality is now implemented per commit 3218d6b. |
Co-authored-by: marcpopMSFT <[email protected]>
|
@marcpopMSFT - Any chance this can be completed for RCx? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
@copilot can you resolve the conflicts |
Conflicts have been resolved successfully. The merge conflict in |
|
@copilot fix the WhenRunWithHttpSourceItShouldThrowError test. |
Co-authored-by: marcpopMSFT <[email protected]>
I've fixed the |
…ture Co-authored-by: marcpopMSFT <[email protected]>
Co-authored-by: nkolev92 <[email protected]>
|
@marcpopMSFT seems fine now. |
|
@copilot add a tests for the new warning messages that cover add the source at the CLI as well as including the source in the nuget.config and including allowinsecureconnections. It should be an E2E test that checks for the string resource in the output of the command. |
I've added comprehensive E2E tests that cover all HTTP source validation scenarios:
All tests use real Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…g scenarios Co-authored-by: marcpopMSFT <[email protected]>
| File.WriteAllText(nugetConfigPath, nugetConfigContent); | ||
|
|
||
| var toolInstallCommand = new DotnetCommand(Log, "tool", "install", "-g", "fake-tool") | ||
| .WithEnvironmentVariable("DOTNET_SKIP_WORKLOAD_INTEGRITY_CHECK", "true") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong since the test doesn't deal with workloads.
|
/backport to release/10.0.1xx |
|
Started backporting to release/10.0.1xx: https://github.com/dotnet/sdk/actions/runs/17922842153 |
|
@marcpopMSFT backporting to "release/10.0.1xx" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Creating an empty commit: Initial plan
Applying: Add HTTP warning functionality to dotnet tool install
.git/rebase-apply/patch:59: trailing whitespace.
.git/rebase-apply/patch:293: trailing whitespace.
warning: 2 lines add whitespace errors.
Using index info to reconstruct a base tree...
M src/Cli/dotnet/CliStrings.resx
M src/Cli/dotnet/xlf/CliStrings.cs.xlf
M src/Cli/dotnet/xlf/CliStrings.de.xlf
M src/Cli/dotnet/xlf/CliStrings.es.xlf
M src/Cli/dotnet/xlf/CliStrings.fr.xlf
M src/Cli/dotnet/xlf/CliStrings.it.xlf
M src/Cli/dotnet/xlf/CliStrings.ja.xlf
M src/Cli/dotnet/xlf/CliStrings.ko.xlf
M src/Cli/dotnet/xlf/CliStrings.pl.xlf
M src/Cli/dotnet/xlf/CliStrings.pt-BR.xlf
M src/Cli/dotnet/xlf/CliStrings.ru.xlf
M src/Cli/dotnet/xlf/CliStrings.tr.xlf
M src/Cli/dotnet/xlf/CliStrings.zh-Hans.xlf
M src/Cli/dotnet/xlf/CliStrings.zh-Hant.xlf
Falling back to patching base and 3-way merge...
Auto-merging src/Cli/dotnet/CliStrings.resx
CONFLICT (content): Merge conflict in src/Cli/dotnet/CliStrings.resx
Auto-merging src/Cli/dotnet/xlf/CliStrings.cs.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.de.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.es.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.fr.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.it.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.ja.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.ko.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.pl.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.pt-BR.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.ru.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.tr.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.zh-Hans.xlf
Auto-merging src/Cli/dotnet/xlf/CliStrings.zh-Hant.xlf
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0002 Add HTTP warning functionality to dotnet tool install
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
superseded by #50966. I copied the code from this PR as cherry-picking wasn't working. |
NuGetPackageDownloaderError_NU1302_HttpSourceUsedresource string with proper NU1302 error formatCheckHttpSourcesmethod to detect and reject HTTP sourcesLoadNuGetSourcesmethodChanges Made
HTTP Error Enforcement
NuGetPackageDownloader.cs: AddedCheckHttpSourcesmethod that examines package sources and throwsNuGetPackageInstallerExceptionfor HTTP sourcesError_NU1302_HttpSourceUsedinCliStrings.resxwith the exact NU1302 error format from NuGet restoreCheckHttpSourcesinLoadNuGetSourcesto ensure errors occur early in the processToolInstallGlobalOrToolPathCommandconstructor for early error detectionallowInsecureConnections Support (Simplified)
CheckHttpSourcesmethod: Uses directPackageSourceproperties instead of reflection or manual settings parsingpackageSource.IsHttp && !packageSource.IsHttps && !packageSource.AllowInsecureConnectionslogicError Output Format
The implementation now generates proper NU1302 errors (without redundant "error NU1302" prefix since NuGet handles that):
allowInsecureConnections Configuration Usage
Users can opt out of HTTP source errors by setting
allowInsecureConnections="true"in their nuget.config:Behavior
E2E Test Coverage
WhenRunWithHttpSourceViaAddSourceItShouldShowNU1302Error: Tests HTTP source validation when using--add-sourceCLI parameterWhenRunWithHttpSourceInNuGetConfigItShouldShowNU1302Error: Tests HTTP source validation when HTTP source is configured in nuget.configWhenRunWithHttpSourceAndAllowInsecureConnectionsItShouldSucceed: Tests thatallowInsecureConnections="true"bypasses HTTP source validationDotnetCommandto execute actualdotnet tool installcommands and verify error messages in outputError_NU1302_HttpSourceUsedresource stringTest Fixes
WhenRunWithPackageIdWithSourceItShouldCreateValidShimtest: Changed the test source fromhttp://mysource.comtohttps://mysource.comto avoid triggering the NU1302 error while still testing the functionality of using additional sources with tool installMerge Conflict Resolution
CliStrings.resxpreserving both my NU1302 error string and new entries from mainCode Simplification (Based on NuGet Team Feedback)
AllowInsecureConnectionspropertyIsHttp,IsHttps,AllowInsecureConnections)Test Approach Resolution
Fixes #36756.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.