Skip to content

Commit

Permalink
wincred: fix bug in credential matching on port nums
Browse files Browse the repository at this point in the history
Fix a bug whereby we would not correctly match existing credentials when
port numbers were used in the search. Additionally, actually now store
the non-default port number in the target URI if there is one given.

We also now always trim any trailing '/' from the end of the path of a
service/target URI. Previously we erronously only trimmed the '/' from a
'pathless' URI, i.e.:

  https://example.com/      => https://example.com
  https://example.com/path/ => https://example.com/path/

This is a safe change since the `Enumerate` method that is used by both
`Get` and `Remove` already matches _both_ trailing and non-trailing
slashes by virtue of creating and comparing components of a `System.Uri`
which does normalisation of the path.

Both `Get` and `Remove` would act (eventually) to remove any bad
credentials stored with a trailing slash, even if new credentials are
only written out without the slash.
  • Loading branch information
mjcheetham committed Jun 16, 2022
1 parent 3ff096f commit a0a7d94
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 6 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using System;
using Xunit;
using GitCredentialManager.Interop.Windows;
using GitCredentialManager.Interop.Windows.Native;

namespace GitCredentialManager.Tests.Interop.Windows
{
Expand Down Expand Up @@ -233,5 +234,75 @@ public void WindowsCredentialManager_RemoveUriUserInfo(string input, string expe
string actual = WindowsCredentialManager.RemoveUriUserInfo(input);
Assert.Equal(expected, actual);
}

[PlatformTheory(Platforms.Windows)]
[InlineData("https://example.com", null, "https://example.com", "alice", true)]
[InlineData("https://example.com", "alice", "https://example.com", "alice", true)]
[InlineData("https://example.com", null, "https://example.com:443", "alice", true)]
[InlineData("https://example.com", "alice", "https://example.com:443", "alice", true)]
[InlineData("https://example.com:1234", null, "https://example.com:1234", "alice", true)]
[InlineData("https://example.com:1234", "alice", "https://example.com:1234", "alice", true)]
[InlineData("https://example.com", null, "http://example.com", "alice", false)]
[InlineData("https://example.com", "alice", "http://example.com", "alice", false)]
[InlineData("https://example.com", "alice", "http://example.com:443", "alice", false)]
[InlineData("http://example.com:443", "alice", "https://example.com", "alice", false)]
[InlineData("https://example.com", "bob", "https://example.com", "alice", false)]
[InlineData("https://example.com", "bob", "https://bob@example.com", "bob", true)]
[InlineData("https://example.com", "bob", "https://example.com", "bob", true)]
[InlineData("https://example.com", "alice", "https://example.com", "ALICE", false)] // username case sensitive
[InlineData("https://example.com", "alice", "https://EXAMPLE.com", "alice", true)] // host NOT case sensitive
[InlineData("https://example.com/path", "alice", "https://example.com/path", "alice", true)]
[InlineData("https://example.com/path", "alice", "https://example.com/PATH", "alice", true)] // path NOT case sensitive
public void WindowsCredentialManager_IsMatch(
string service, string account, string targetName, string userName, bool expected)
{
string fullTargetName = $"{WindowsCredentialManager.TargetNameLegacyGenericPrefix}{TestNamespace}:{targetName}";
var win32Cred = new Win32Credential
{
UserName = userName,
TargetName = fullTargetName
};

var credManager = new WindowsCredentialManager(TestNamespace);

bool actual = credManager.IsMatch(service, account, win32Cred);

Assert.Equal(expected, actual);
}

[PlatformTheory(Platforms.Windows)]
[InlineData("https://example.com", null, "https://example.com")]
[InlineData("https://example.com", "bob", "https://bob@example.com")]
[InlineData("https://example.com", "bob@id.example.com", "https://bob_id.example.com@example.com")] // @ in user
[InlineData("https://example.com:443", null, "https://example.com")] // default port
[InlineData("https://example.com:1234", null, "https://example.com:1234")]
[InlineData("https://example.com/path", null, "https://example.com/path")]
[InlineData("https://example.com/path/with/more/parts", null, "https://example.com/path/with/more/parts")]
[InlineData("https://example.com/path/trim/", null, "https://example.com/path/trim")] // path trailing slash
[InlineData("https://example.com/", null, "https://example.com")] // no path trailing slash
public void WindowsCredentialManager_CreateTargetName(string service, string account, string expected)
{
string fullExpected = $"{TestNamespace}:{expected}";

var credManager = new WindowsCredentialManager(TestNamespace);

string actual = credManager.CreateTargetName(service, account);

Assert.Equal(fullExpected, actual);
}

[PlatformTheory(Platforms.Windows)]
[InlineData(TestNamespace, "https://example.com", null, $"{TestNamespace}:https://example.com")]
[InlineData(null, "https://example.com", null, "https://example.com")]
[InlineData("", "https://example.com", null, "https://example.com")]
[InlineData(" ", "https://example.com", null, "https://example.com")]
public void WindowsCredentialManager_CreateTargetName_Namespace(string @namespace, string service, string account, string expected)
{
var credManager = new WindowsCredentialManager(@namespace);

string actual = credManager.CreateTargetName(service, account);

Assert.Equal(expected, actual);
}
}
}
24 changes: 18 additions & 6 deletions src/shared/Core/Interop/Windows/WindowsCredentialManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace GitCredentialManager.Interop.Windows
{
public class WindowsCredentialManager : ICredentialStore
{
private const string TargetNameLegacyGenericPrefix = "LegacyGeneric:target=";
internal const string TargetNameLegacyGenericPrefix = "LegacyGeneric:target=";

private readonly string _namespace;

Expand Down Expand Up @@ -260,7 +260,7 @@ private WindowsCredential CreateCredentialFromStructure(Win32Credential credenti
return url;
}

private bool IsMatch(string service, string account, Win32Credential credential)
internal /* for testing */ bool IsMatch(string service, string account, Win32Credential credential)
{
// Match against the username first
if (!string.IsNullOrWhiteSpace(account) &&
Expand All @@ -286,14 +286,20 @@ private bool IsMatch(string service, string account, Win32Credential credential)
if (Uri.TryCreate(service, UriKind.Absolute, out Uri serviceUri) &&
Uri.TryCreate(targetName, UriKind.Absolute, out Uri targetUri))
{
// Match scheme/protocol
if (!StringComparer.OrdinalIgnoreCase.Equals(serviceUri.Scheme, targetUri.Scheme))
{
return false;
}

// Match host name
if (!StringComparer.OrdinalIgnoreCase.Equals(serviceUri.Host, targetUri.Host))
{
return false;
}

// Match port number
if (!serviceUri.IsDefaultPort && serviceUri.Port == targetUri.Port)
if (serviceUri.Port != targetUri.Port)
{
return false;
}
Expand All @@ -313,7 +319,7 @@ private bool IsMatch(string service, string account, Win32Credential credential)
return false;
}

private string CreateTargetName(string service, string account)
internal /* for testing */ string CreateTargetName(string service, string account)
{
var serviceUri = new Uri(service, UriKind.Absolute);
var sb = new StringBuilder();
Expand All @@ -339,9 +345,15 @@ private string CreateTargetName(string service, string account)
sb.Append(serviceUri.Host);
}

if (!string.IsNullOrWhiteSpace(serviceUri.AbsolutePath.TrimEnd('/')))
if (!serviceUri.IsDefaultPort)
{
sb.AppendFormat(":{0}", serviceUri.Port);
}

string trimmedPath = serviceUri.AbsolutePath.TrimEnd('/');
if (!string.IsNullOrWhiteSpace(trimmedPath))
{
sb.Append(serviceUri.AbsolutePath);
sb.Append(trimmedPath);
}

return sb.ToString();
Expand Down

0 comments on commit a0a7d94

Please sign in to comment.