(#3566) Avoid credential bleed from saved sources with the same hostname #3572
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
corbob
force-pushed
the
source-auth-1.x
branch
2 times, most recently
from
658b4e7 to
54b7bee
Compare
The ExplicitSources property is used when looking up credentials. This backports this property from 2.x to 1.x.
corbob
force-pushed
the
source-auth-1.x
branch
from
6ea7d22 to
9d6c439
Compare
corbob
changed the title
corbob
force-pushed
the
source-auth-1.x
branch
from
9d6c439 to
ab46595
Compare
vexx32
approved these changes
Nov 25, 2024
src/chocolatey/infrastructure.app/nuget/ChocolateyNugetCredentialProvider.cs
Outdated
Show resolved
Hide resolved
Previously we looked up any available sources in the config by the hostname, before falling back to trying an exact match if we had collisions. This still allowed credentials to be reused in situations where we don't actually know if they're applicable; many repository servers will support different credentials for individual repositories, so we cannot and should not assume that credentials for one repository will actually match another repository, nor that users want the credentials to be shared for both. It also led to the possibility of users storing one repository first, and then later specifying a different repository on the same server, and choco would try to use the stored credentials for the first repository for the explicitly-entered URL which is nowhere in config. Instead, we should only match the whole URL (which can be done with Uri. Equals to ensure that we match hostnames case-insensitively, but routes case-sensitively), and expect users to provide credentials if they provide a URL that is not explicitly in the sources. Additionally, we try to ensure that if a user has named a specific source, rather than themselves providing a URL at the command line, we prioritise finding that in the already-configured sources and use that source if the URL matches the current URL that NuGet requires a credential for.
corbob
force-pushed
the
source-auth-1.x
branch
from
ab46595 to
d22a10d
Compare
vexx32
approved these changes
Nov 25, 2024
gep13
reviewed
Nov 26, 2024
Comment on lines
7
to
8
| [Parameter()] | ||
| [switch]$All |
There was a problem hiding this comment.
This was removed in the other PR, so should also be removed here, right?
gep13
reviewed
Nov 26, 2024
| @@ -0,0 +1,66 @@ | |||
| Describe 'Ensuring credentials do not bleed from configured sources' -Tag CredentialProvider -ForEach @( | |||
There was a problem hiding this comment.
Same comment here, as per the other PR. I think we need more words here as to what this test is doing, and why the asserts are set the way that they are.
gep13
requested changes
Nov 26, 2024
Add Pester tests to ensure we don't inadvertently bleed configured credentials into scenarios where they should not be used.
corbob
force-pushed
the
source-auth-1.x
branch
from
d22a10d to
3d599b1
Compare
Closed
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description Of Changes
Backport #3568 to Chocolatey CLI 1.x
Motivation and Context
See #3566 - the credentials are being reused in places that they shouldn't.
Testing
Ran through Test Kitchen locally and in Team City.
To test with CLE locally I editted the
nupkgproduced by Team City and changed the version reported to be1.5.0-pullrequesand then tested with CLE5.0.6.Screenshot from local test kitchen run:
Operating Systems Testing
Change Types Made
Change Checklist
Related Issue