From 322d5a136aac9854e46b3dc1c098ec48f8bc8013 Mon Sep 17 00:00:00 2001 From: Riccardo De Agostini Date: Sat, 1 Aug 2026 01:48:34 +0200 Subject: [PATCH 1/2] Give GitHub URL composition an owner, and tests The missing separator fixed in 6bba582 was possible because no single place owned the contract: the repository URL was built in the constructor without a trailing slash, and two unrelated methods appended path segments to it. The two ends could drift, and did, for three releases. GitHubRepositoryUrls now owns both ends. It takes the host, owner and name, builds the repository URL, and derives the release and file URLs from it; GitHubServerAdapter holds one and delegates. It needs no service provider, no Git origin and no token, so it is directly testable - which is why the methods it replaces never were. Two fixes fell out of writing the tests: Guard.IsTrue takes the parameter name as its second argument, not a message, so both guards were reporting 'Parameter "A path must be relative to be converted to a file URL." must be true, was false'. They now pass nameof(path) and the message to the three-argument overload. The escape check only rejected a leading "..", but Uri collapses parent segments as it parses, so enough of them anywhere in the path walked out of the repository and even out of the owner: "docs/../../../../../../etc/passwd" resolved to "https://github.com/etc/passwd". Any ".." segment is now rejected, while a name merely starting with two dots is still accepted. No changelog entry: every type involved is internal, the only caller passes a literal "CHANGELOG.md", and the URLs these methods produce were already covered by the entry for 6bba582. Co-Authored-By: Claude Opus 5 (1M context) --- .../Internal/GitHub/GitHubRepositoryUrls.cs | 84 +++++++++++++ .../Internal/GitHub/GitHubServerAdapter.cs | 24 +--- .../GitHubRepositoryUrlsTests.cs | 114 ++++++++++++++++++ 3 files changed, 203 insertions(+), 19 deletions(-) create mode 100644 src/Buildvana.Tool/Services/ServerAdapters/Internal/GitHub/GitHubRepositoryUrls.cs create mode 100644 tests/Buildvana.Tool.Tests/GitHubRepositoryUrlsTests.cs 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/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(); + } +} From 954644088fbd74d334b739cf3331738eba4ef485 Mon Sep 17 00:00:00 2001 From: Riccardo De Agostini Date: Sat, 1 Aug 2026 01:51:33 +0200 Subject: [PATCH 2/2] Require the release commit before a post-release commit AddPostReleaseCommit called EnsureReleaseCommit for its caller, which meant it could move the Git height - and with it the version - in the middle of a call whose message the caller had already formatted from that same version. That is exactly how the self-reference commit came to announce 2.1.2-preview while the release it sat on top of was tagged 2.1.3-preview. _version.Update() is called in exactly one place in the codebase, inside EnsureReleaseCommit, so the version can only move at that single moment. Requiring the release commit up front therefore removes the hazard outright, rather than working around it with a deferred message factory: once the commit exists, nothing a caller invokes can change the version underneath it. UpdateRepository keeps creating the commit implicitly, and the asymmetry is deliberate: it amends the release commit, so it owns it, and it builds its message internally once the version has settled. A post-release commit merely sits on top of that commit, so it depends on it. No functional change today - ReleaseCommand already settles the release commit before packing, which is what made that call legal in the first place. Co-Authored-By: Claude Opus 5 (1M context) --- .../Services/ServerAdapters/ServerRelease.cs | 20 ++++++++++++++----- .../Subcommands/ReleaseCommand.cs | 5 +++-- 2 files changed, 18 insertions(+), 7 deletions(-) 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.