diff --git a/src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubRepositoryUrls.cs b/src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubRepositoryUrls.cs new file mode 100644 index 0000000..022e056 --- /dev/null +++ b/src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubRepositoryUrls.cs @@ -0,0 +1,84 @@ +// Copyright (C) Tenacom and Contributors. Licensed under the MIT license. +// See the LICENSE file in the project root for full license information. + +using System; +using System.IO; +using CommunityToolkit.Diagnostics; + +namespace Buildvana.Tool.Services.ServerAdapters.Internal.GitHub; + +/// +/// Builds the URLs of a GitHub repository and of the resources it contains. +/// +/// +/// This type owns both ends of the same contract: how the repository URL is built, and how the URLs of +/// resources are derived from it. Keeping them together is the point — deriving a resource URL elsewhere, +/// by appending to the repository URL, is what previously produced links with no separator between the +/// repository name and the first path segment. +/// The repository URL deliberately carries no trailing slash, so it can be displayed and compared as +/// the canonical URL of the repository. Callers must therefore not treat it as a base URL for +/// , which would resolve a relative URL against its parent and drop the +/// repository name. +/// +internal sealed class GitHubRepositoryUrls +{ + private readonly string _repository; + + /// + /// Initializes a new instance of the class. + /// + /// The host name of the GitHub instance, e.g. github.com. + /// The owner of the repository. + /// The name of the repository. + public GitHubRepositoryUrls(string hostName, string repositoryOwner, string repositoryName) + { + Guard.IsNotNullOrEmpty(hostName); + Guard.IsNotNullOrEmpty(repositoryOwner); + Guard.IsNotNullOrEmpty(repositoryName); + + _repository = $"https://{hostName}/{repositoryOwner}/{repositoryName}"; + Repository = new Uri(_repository); + } + + /// + /// Gets the URL of the repository, without a trailing slash. + /// + public Uri Repository { get; } + + /// + /// Builds the URL of the release identified by a tag. + /// + /// The version string, which is also the tag name. + /// The URL of the release. + public Uri ReleaseTag(string version) + { + Guard.IsNotNullOrEmpty(version); + return new Uri($"{_repository}/releases/tag/{version}"); + } + + /// + /// Builds the URL of a file in the repository, as of a given commit or reference. + /// + /// The path to the file, relative to the repository root. + /// The SHA or reference to which the file belongs. + /// The URL of the file. + public Uri File(string path, string commitish) + { + Guard.IsNotNullOrEmpty(path); + Guard.IsNotNullOrEmpty(commitish); + Guard.IsTrue(!Path.IsPathFullyQualified(path), nameof(path), "A path must be relative to be converted to a file URL."); + + // Normalize to forward slashes for the URL, then reject paths that escape the repo. + // Every ".." segment must go, not just a leading one: Uri collapses parent segments as it parses, + // so enough of them anywhere in the path walk out of the repository and even out of the owner + // (".../blob/main/docs/../../../../../../etc/passwd" parses to "https://github.com/etc/passwd"). + var remotePath = path.Replace('\\', '/'); + var hasParentSegment = remotePath == ".." + || remotePath.StartsWith("../", StringComparison.Ordinal) + || remotePath.EndsWith("/..", StringComparison.Ordinal) + || remotePath.Contains("/../", StringComparison.Ordinal); + Guard.IsTrue(!hasParentSegment, nameof(path), "Only a path to a file in the repository can be converted to a file URL."); + + return new Uri($"{_repository}/blob/{commitish}/{remotePath}"); + } +} diff --git a/src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubServerAdapter.cs b/src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubServerAdapter.cs index 9f4f61d..2ae7128 100644 --- a/src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubServerAdapter.cs +++ b/src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubServerAdapter.cs @@ -28,6 +28,7 @@ internal sealed class GitHubServerAdapter : ServerAdapter private readonly GitService _git; private readonly string _token; + private readonly GitHubRepositoryUrls _urls; private GitHubServerAdapter(IServiceProvider services) { @@ -40,7 +41,7 @@ private GitHubServerAdapter(IServiceProvider services) HostName = originInfo.Host; RepositoryOwner = originInfo.PathSegments[0]; RepositoryName = originInfo.PathSegments[1]; - RepositoryUrl = new Uri($"https://{HostName}/{RepositoryOwner}/{RepositoryName}"); + _urls = new GitHubRepositoryUrls(HostName, RepositoryOwner, RepositoryName); var tokenEnv = services.GetRequiredService().GitHub?.TokenEnv is { Length: > 0 } e ? e : "GITHUB_TOKEN"; @@ -60,7 +61,7 @@ private GitHubServerAdapter(IServiceProvider services) public override string RepositoryName { get; } /// - public override Uri RepositoryUrl { get; } + public override Uri RepositoryUrl => _urls.Repository; /// /// Always . @@ -111,25 +112,10 @@ public override async Task IsPrivateRepositoryAsync() } /// - public override Uri GetReleaseUrl(string version) - { - Guard.IsNotNullOrEmpty(version); - return new Uri($"{RepositoryUrl}/releases/tag/{version}"); - } + public override Uri GetReleaseUrl(string version) => _urls.ReleaseTag(version); /// - public override Uri GetFileUrl(string path, string commitish) - { - Guard.IsNotNullOrEmpty(path); - Guard.IsNotNullOrEmpty(commitish); - Guard.IsTrue(!Path.IsPathFullyQualified(path), "A path must be relative to be converted to a file URL."); - - // Normalize to forward slashes for the URL, then reject paths that escape the repo. - var remotePath = path.Replace('\\', '/'); - Guard.IsTrue(remotePath != ".." && !remotePath.StartsWith("../", StringComparison.Ordinal), "Only a path to a file in the repository can be converted to a file URL."); - - return new Uri($"{RepositoryUrl}/blob/{commitish}/{remotePath}"); - } + public override Uri GetFileUrl(string path, string commitish) => _urls.File(path, commitish); /// public override async Task CreateReleaseAsync() diff --git a/src/Buildvana.Tool/Services/ServerAdapters/ServerRelease.cs b/src/Buildvana.Tool/Services/ServerAdapters/ServerRelease.cs index f206c86..e6b82b2 100644 --- a/src/Buildvana.Tool/Services/ServerAdapters/ServerRelease.cs +++ b/src/Buildvana.Tool/Services/ServerAdapters/ServerRelease.cs @@ -60,8 +60,11 @@ private protected ServerRelease(IServiceProvider services) /// The first call creates an empty commit, refreshes version information from the new Git height, /// then amends the commit with the final version-bearing message and captures its SHA into /// . Subsequent calls are no-ops. - /// calls this implicitly; callers only need to invoke it directly - /// when they want to guarantee a release commit exists without staging any file. + /// calls this implicitly, because it amends the release commit and + /// builds its own message afterwards. does not: it requires a release + /// commit to exist already, so that the version cannot move under a message its caller has formatted. + /// Call this directly to settle the version before anything reads it - notably before building, so + /// that artifacts carry the same version that will be tagged and published. /// public void EnsureReleaseCommit() { @@ -139,8 +142,12 @@ public void UpdateRepository(params string[] files) /// been published to the feed yet). /// The paths of the files to stage into the new commit. /// - /// If no release commit has been created yet, this method calls - /// first, so the post-release commit always sits on top of a tagged release commit (possibly empty). + /// A release commit must already exist: call (directly or via + /// ) first. + /// This method deliberately does not create the release commit itself. Doing so would move the + /// Git height, and with it the version, in the middle of a call whose the + /// caller has already formatted - typically from that very version, which the message would then + /// misreport. Requiring the commit up front keeps the version settled before any caller reads it. /// public void AddPostReleaseCommit(string message, params string[] files) { @@ -153,7 +160,10 @@ public void AddPostReleaseCommit(string message, params string[] files) ThrowHelper.ThrowInvalidOperationException("Internal error: cannot add a post-release commit when updates have already been pushed."); } - EnsureReleaseCommit(); + if (!_repositoryUpdated) + { + ThrowHelper.ThrowInvalidOperationException("Internal error: cannot add a post-release commit before the release commit has been created."); + } _git.Stage(files); _reporter.Info("Committing post-release changed files..."); diff --git a/src/Buildvana.Tool/Subcommands/ReleaseCommand.cs b/src/Buildvana.Tool/Subcommands/ReleaseCommand.cs index 2d3b498..49c4fca 100644 --- a/src/Buildvana.Tool/Subcommands/ReleaseCommand.cs +++ b/src/Buildvana.Tool/Subcommands/ReleaseCommand.cs @@ -176,8 +176,9 @@ public async Task ExecuteAsync(CancellationToken cancellationToken) // Create the release commit now, if none of the steps above had anything to commit. // The Git height, hence the version, must be final before anything is built: a release commit - // created later (for example by AddPostReleaseCommit) would bump the height after the fact, - // tagging and publishing a version one patch above the one the artifacts were built with. + // created later would bump the height after the fact, tagging and publishing a version one + // patch above the one the artifacts were built with. This call is also what makes the later + // AddPostReleaseCommit legal, as that method requires the release commit to exist already. release.EnsureReleaseCommit(); // At this point we know what the actual published version will be. diff --git a/tests/Buildvana.Tool.Tests/GitHubRepositoryUrlsTests.cs b/tests/Buildvana.Tool.Tests/GitHubRepositoryUrlsTests.cs new file mode 100644 index 0000000..9bb74f9 --- /dev/null +++ b/tests/Buildvana.Tool.Tests/GitHubRepositoryUrlsTests.cs @@ -0,0 +1,114 @@ +// Copyright (C) Tenacom and Contributors. Licensed under the MIT license. +// See the LICENSE file in the project root for full license information. + +using Buildvana.Tool.Services.ServerAdapters.Internal.GitHub; + +internal sealed class GitHubRepositoryUrlsTests +{ + private static GitHubRepositoryUrls Urls => new("github.com", "Tenacom", "Buildvana"); + + [Test] + public async Task Repository_HasNoTrailingSlash() + { + await Assert.That(Urls.Repository.ToString()).IsEqualTo("https://github.com/Tenacom/Buildvana"); + } + + [Test] + public async Task ReleaseTag_SeparatesRepositoryNameFromFirstSegment() + { + // Regression: this used to come out as ".../Tenacom/Buildvanareleases/tag/2.1.2-preview". + var url = Urls.ReleaseTag("2.1.2-preview").ToString(); + await Assert.That(url).IsEqualTo("https://github.com/Tenacom/Buildvana/releases/tag/2.1.2-preview"); + } + + [Test] + public async Task File_SeparatesRepositoryNameFromFirstSegment() + { + // Regression: this used to come out as ".../Tenacom/Buildvanablob/main/CHANGELOG.md". + var url = Urls.File("CHANGELOG.md", "main").ToString(); + await Assert.That(url).IsEqualTo("https://github.com/Tenacom/Buildvana/blob/main/CHANGELOG.md"); + } + + [Test] + public async Task File_AcceptsCommitShaAsCommitish() + { + var url = Urls.File("CHANGELOG.md", "7f7aebf19f12da3c8e6335b16cc7d5482b7b70a6").ToString(); + await Assert.That(url).IsEqualTo("https://github.com/Tenacom/Buildvana/blob/7f7aebf19f12da3c8e6335b16cc7d5482b7b70a6/CHANGELOG.md"); + } + + [Test] + public async Task File_NormalizesBackslashesToForwardSlashes() + { + var url = Urls.File(@"docs\ConstantsSyntax.md", "main").ToString(); + await Assert.That(url).IsEqualTo("https://github.com/Tenacom/Buildvana/blob/main/docs/ConstantsSyntax.md"); + } + + [Test] + public async Task Urls_HonorNonDefaultHostName() + { + var urls = new GitHubRepositoryUrls("github.example.com", "Contoso", "Widgets"); + await Assert.That(urls.Repository.ToString()).IsEqualTo("https://github.example.com/Contoso/Widgets"); + await Assert.That(urls.ReleaseTag("1.0.0").ToString()).IsEqualTo("https://github.example.com/Contoso/Widgets/releases/tag/1.0.0"); + await Assert.That(urls.File("README.md", "main").ToString()).IsEqualTo("https://github.example.com/Contoso/Widgets/blob/main/README.md"); + } + + [Test] + [Arguments("..")] + [Arguments("../outside.md")] + [Arguments(@"..\outside.md")] + [Arguments("docs/..")] + [Arguments("docs/nested/../ConstantsSyntax.md")] + [Arguments("docs/../../outside.md")] + [Arguments("docs/../../../../../../etc/passwd")] + public async Task File_RejectsPathWithParentSegment(string path) + { + // A parent segment anywhere is rejected, not just a leading one: Uri collapses them as it parses, + // so the last case above would otherwise resolve to "https://github.com/etc/passwd". + await Assert.That(() => Urls.File(path, "main")).Throws(); + } + + [Test] + [Arguments("..gitignore.md")] + [Arguments("docs/..hidden/file.md")] + public async Task File_AcceptsNameStartingWithTwoDots(string path) + { + // Only a whole ".." segment escapes; two dots at the start of a name are just a name. + var url = Urls.File(path, "main").ToString(); + await Assert.That(url).IsEqualTo($"https://github.com/Tenacom/Buildvana/blob/main/{path}"); + } + + [Test] + public async Task File_RejectsFullyQualifiedPath() + { + // Built from a relative path so the test means the same thing on every platform. + var fullyQualified = Path.GetFullPath("CHANGELOG.md"); + await Assert.That(() => Urls.File(fullyQualified, "main")).Throws(); + } + + [Test] + public async Task ReleaseTag_RejectsEmptyVersion() + { + await Assert.That(() => Urls.ReleaseTag(string.Empty)).Throws(); + } + + [Test] + public async Task File_RejectsEmptyPath() + { + await Assert.That(() => Urls.File(string.Empty, "main")).Throws(); + } + + [Test] + public async Task File_RejectsEmptyCommitish() + { + await Assert.That(() => Urls.File("CHANGELOG.md", string.Empty)).Throws(); + } + + [Test] + [Arguments("", "Tenacom", "Buildvana")] + [Arguments("github.com", "", "Buildvana")] + [Arguments("github.com", "Tenacom", "")] + public async Task Constructor_RejectsEmptyArguments(string hostName, string owner, string name) + { + await Assert.That(() => new GitHubRepositoryUrls(hostName, owner, name)).Throws(); + } +}