From cd25e9dc3e3ecceb8f383315a7bdd3fffccaf02b Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Sun, 18 Oct 2026 14:15:56 +0800 Subject: [PATCH] Send GitLab the right auth on the right host A GitLab credential stopped working, without the token being wrong, in two ways. The typed client (NGitLab, on HttpWebRequest) sent only Authorization: Bearer and dropped it on a 301; the raw HTTP calls sent Bearer and PRIVATE-TOKEN together and kept PRIVATE-TOKEN across a redirect to any host. Behind a redirecting base URL a PAT worked, an OAuth token read 401, and the PAT went wherever the redirect pointed. GitLab 13.0 to 17.7 also reads a Bearer of 41 or more characters as an OAuth token alone, so a long access token was refused as a Bearer. - Every call now sends the one header its credential type names: PRIVATE-TOKEN for personal, project and group access tokens, Bearer for OAuth. NGitLab's two construction sites share a RequestOptions that rewrites the header and follows a GET or HEAD redirect inside one origin itself; the raw calls share one client behind OriginScopedRedirectHandler, which gains opt-in FollowsCrossOrigin = false and RepeatsWritesOnly = true. - A redirect to another scheme, host or port is not followed and fails with the typed provider_base_url_redirects, naming the URL GitLab pointed at. So does the next page of a list when its Link header names another origin: NGitLab builds that request itself, and the options now refuse it before the credential is attached. NGitLab never re-sends a write, and a raw write is repeated only by a 307 or 308; a 301, 302 or 303 that would turn it into a GET is refused rather than read as the write's answer. - A tokened git command that its remote sends to another origin fails naming the redirect (the same code and the origin git was sent to) instead of git's own prompt for a username. It stays a WorkspaceException, so the push retry, the per-repo push isolation and the workflow nodes that catch one keep treating it as a git failure. - Adding a provider, or moving its API address, asks GET /api/v4/version once, with no credential, and refuses an address that redirects to another origin. An edit that leaves the address as stored asks nothing, so an existing connection is never rewritten or blocked. - An edit that only upgrades http to https on the same host (80 to 443, or one explicit port) is no longer a move past connected tokens, so the self-service fix for a redirecting base URL does not require disconnecting every member; the edit form mirrors the rule. While tokens are connected the new address must also answer HTTP, any status: a mistaken upgrade cannot be edited back (that is a move) and would break every token, so it is refused with provider_endpoint_unreachable and the stored address stays. With no token connected nothing is asked. - Resolving a credentialed GitLab clone logs a Warning when the clone URL's origin is not the base URL's. Nothing is rewritten. --- .../Agents/Commands/RunCommandService.cs | 7 +- .../Agents/Workspace/CloneUrlAuthority.cs | 38 +++ .../Agents/Workspace/GitRemoteRedirect.cs | 45 +++ .../Integrators/LocalGitBranchIntegrator.cs | 4 +- .../Providers/LocalGitWorkspaceProvider.cs | 6 +- .../Agents/Workspace/RemoteTipResolver.cs | 2 +- .../Workspace/RepositoryWorkspaceResolver.cs | 7 +- .../Agents/Workspace/WorkspaceException.cs | 11 +- .../OriginScopedRedirectHandler.cs | 35 ++- .../IProviderEndpointProbe.cs | 29 ++ .../ProviderEndpointProbe.cs | 123 ++++++++ .../ProviderInstanceService.cs | 65 +++- .../Providers/GitLab/GitLabCredential.cs | 58 ++++ .../Services/Providers/GitLab/GitLabHttp.cs | 57 ++++ ...abRepositoryProvider.ConnectionWebhooks.cs | 43 ++- ...GitLabRepositoryProvider.MemberStanding.cs | 7 +- .../GitLab/GitLabRepositoryProvider.cs | 154 ++++----- .../Providers/GitLab/GitLabRequestOptions.cs | 147 +++++++++ .../Services/Redaction/CredentialUrl.cs | 13 + .../Supervisor/SupervisorAcceptanceGrader.cs | 2 +- .../ProviderBaseUrlRedirectsException.cs | 87 ++++++ .../ProviderEndpointUnreachableException.cs | 42 +++ .../Failures/FailureCodes.cs | 6 + .../ListPagingPastTheCeilingE2ETests.cs | 5 +- .../ProviderBaseUrlRedirectE2ETests.cs | 145 +++++++++ .../Infrastructure/PostgresFixture.cs | 7 + .../TestProviderEndpointProbe.cs | 51 +++ ...roviderInstanceEndpointBindingFlowTests.cs | 99 ++++++ .../ProviderInstanceRedirectProbeFlowTests.cs | 225 +++++++++++++ .../Workflows/CloneUrlAuthorityFlowTests.cs | 166 ++++++++++ .../Workflows/GitPublishRemoteFixture.cs | 27 ++ .../TokenedGitCredentialHelperFlowTests.cs | 46 ++- .../Agents/CloneUrlAuthorityTests.cs | 74 +++++ .../Agents/DefaultRunnerKindTests.cs | 3 +- .../Agents/RunCommandCallerPostureTests.cs | 5 +- .../Agents/WorkspaceSpecTests.cs | 5 +- .../Architecture/FailureTaxonomyTests.cs | 2 + .../OriginScopedRedirectHandlerTests.cs | 104 +++++- .../GitLab/GitLabCredentialHeaderTests.cs | 83 +++++ .../Providers/GitLab/GitLabCredentialTests.cs | 84 +++++ .../Providers/GitLab/GitLabLoopback.cs | 104 ++++++ .../Providers/GitLab/GitLabRedirectTests.cs | 295 ++++++++++++++++++ .../ProviderBaseUrlRedirectsExceptionTests.cs | 92 ++++++ .../Providers/ProviderEndpointProbeTests.cs | 255 +++++++++++++++ ...oviderEndpointUnreachableExceptionTests.cs | 30 ++ .../ProviderInstanceEndpointBindingTests.cs | 30 ++ .../Redaction/CredentialUrlTests.cs | 23 ++ .../Workflows/GitRemoteRedirectTests.cs | 143 +++++++++ .../Workflows/TokenedGitCommandTests.cs | 16 + .../ai-code-space/connect-remote-modal.tsx | 4 +- frontend/src/lib/credentialEndpoint.test.ts | 41 ++- frontend/src/lib/credentialEndpoint.ts | 35 ++- 52 files changed, 3034 insertions(+), 153 deletions(-) create mode 100644 backend/src/CodeSpace.Core/Services/Agents/Workspace/CloneUrlAuthority.cs create mode 100644 backend/src/CodeSpace.Core/Services/Agents/Workspace/GitRemoteRedirect.cs create mode 100644 backend/src/CodeSpace.Core/Services/ProviderInstances/IProviderEndpointProbe.cs create mode 100644 backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderEndpointProbe.cs create mode 100644 backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabCredential.cs create mode 100644 backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabHttp.cs create mode 100644 backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRequestOptions.cs create mode 100644 backend/src/CodeSpace.Messages/Exceptions/ProviderBaseUrlRedirectsException.cs create mode 100644 backend/src/CodeSpace.Messages/Exceptions/ProviderEndpointUnreachableException.cs create mode 100644 backend/tests/CodeSpace.E2ETests/Repositories/ProviderBaseUrlRedirectE2ETests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Infrastructure/TestProviderEndpointProbe.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/ProviderInstances/ProviderInstanceRedirectProbeFlowTests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Workflows/CloneUrlAuthorityFlowTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Agents/CloneUrlAuthorityTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabCredentialHeaderTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabCredentialTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabLoopback.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabRedirectTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Providers/ProviderBaseUrlRedirectsExceptionTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Providers/ProviderEndpointProbeTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Providers/ProviderEndpointUnreachableExceptionTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Workflows/GitRemoteRedirectTests.cs diff --git a/backend/src/CodeSpace.Core/Services/Agents/Commands/RunCommandService.cs b/backend/src/CodeSpace.Core/Services/Agents/Commands/RunCommandService.cs index 009bd3be3..753ed92cf 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Commands/RunCommandService.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Commands/RunCommandService.cs @@ -9,6 +9,7 @@ using CodeSpace.Core.Settings; using CodeSpace.Messages.Agents; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; namespace CodeSpace.Core.Services.Agents.Commands; @@ -20,8 +21,9 @@ public sealed class RunCommandService : IRunCommandService, IScopedDependency private readonly IWorkspaceProviderRegistry _workspaces; private readonly AgentDefaultRunnerSetting _defaultRunner; private readonly CallerCommandLanes _lanes; + private readonly ILogger _logger; - public RunCommandService(CodeSpaceDbContext db, IProviderAuthResolver auth, ISandboxRunnerRegistry runners, IWorkspaceProviderRegistry workspaces, AgentDefaultRunnerSetting defaultRunner, CallerCommandLanes lanes) + public RunCommandService(CodeSpaceDbContext db, IProviderAuthResolver auth, ISandboxRunnerRegistry runners, IWorkspaceProviderRegistry workspaces, AgentDefaultRunnerSetting defaultRunner, CallerCommandLanes lanes, ILogger logger) { _db = db; _auth = auth; @@ -29,6 +31,7 @@ public RunCommandService(CodeSpaceDbContext db, IProviderAuthResolver auth, ISan _workspaces = workspaces; _defaultRunner = defaultRunner; _lanes = lanes; + _logger = logger; } public async Task RunAsync(RunCommandRequest request, CancellationToken cancellationToken) @@ -184,6 +187,8 @@ private async Task BuildWorkspaceRequestAsync(Guid repositoryI var token = await ResolveTokenAsync(repo, cancellationToken).ConfigureAwait(false); + CloneUrlAuthority.WarnWhenForeign(_logger, repo, credentialed: token is not null); + return new WorkspaceRequest { RepositoryUrl = repo.CloneUrlHttps, diff --git a/backend/src/CodeSpace.Core/Services/Agents/Workspace/CloneUrlAuthority.cs b/backend/src/CodeSpace.Core/Services/Agents/Workspace/CloneUrlAuthority.cs new file mode 100644 index 000000000..6d7a33fdf --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Agents/Workspace/CloneUrlAuthority.cs @@ -0,0 +1,38 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Redaction; +using CodeSpace.Messages.Enums; +using Microsoft.Extensions.Logging; + +namespace CodeSpace.Core.Services.Agents.Workspace; + +/// +/// Whether a GitLab repository is cloned from the origin its provider connection was set up for. A clone's credential is +/// answered for the clone URL's scheme and authority alone (), so a URL the host redirects to +/// another origin — http to https, a host alias, another port — finds no credential and fails with a prompt for one, which +/// names as the redirect it is once the command has failed (provider_base_url_redirects, +/// as the provider's REST calls name it). This check is the other half: it says so BEFORE any clone, from the two stored +/// URLs, when they disagree — but not when they agree and a proxy in front redirects anyway, which only the failed command +/// can show. The two origins differ when GitLab's +/// own external_url disagrees with the base URL the owner typed, which is exactly the case that redirects. Nothing is +/// rewritten — the clone URL is what GitLab reported, the base URL what the owner stored, and each is theirs to change — but +/// the first thing anyone debugging that run needs is the pair, so the resolve says so, with both origins, and only when a +/// credential is about to be presented (an anonymous clone follows a redirect without harm). +/// +internal static class CloneUrlAuthority +{ + /// + /// True when and the provider's are both http(s) URLs of a GitLab + /// connection and name different origins — scheme, host or port; path and userinfo are not part of one. + /// + internal static bool IsForeign(ProviderKind provider, string? baseUrl, string? cloneUrl) => provider == ProviderKind.GitLab && IsHttp(baseUrl) && IsHttp(cloneUrl) && !CredentialUrl.SameEndpoint(baseUrl, cloneUrl); + + /// Logs a Warning naming both origins when is about to be cloned with a credential from an origin other than its provider's base URL. Never throws, never changes the request. + internal static void WarnWhenForeign(ILogger logger, Repository repository, bool credentialed) + { + if (!credentialed || !IsForeign(repository.ProviderInstance.Provider, repository.ProviderInstance.BaseUrl, repository.CloneUrlHttps)) return; + + logger.LogWarning("Repository {RepositoryId} is cloned from {CloneOrigin}, which is not the origin of its {Provider} provider's base URL {BaseOrigin}. git presents the credential to the clone URL's origin only, so if either one redirects to the other the clone fails asking for credentials. Edit the provider's base URL to the origin GitLab serves from, or correct GitLab's external_url", repository.Id, CredentialUrl.Endpoint(repository.CloneUrlHttps), repository.ProviderInstance.Provider, CredentialUrl.Endpoint(repository.ProviderInstance.BaseUrl)); + } + + private static bool IsHttp(string? url) => Uri.TryCreate(url?.Trim(), UriKind.Absolute, out var parsed) && (parsed.Scheme == Uri.UriSchemeHttp || parsed.Scheme == Uri.UriSchemeHttps); +} diff --git a/backend/src/CodeSpace.Core/Services/Agents/Workspace/GitRemoteRedirect.cs b/backend/src/CodeSpace.Core/Services/Agents/Workspace/GitRemoteRedirect.cs new file mode 100644 index 000000000..508728d5c --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Agents/Workspace/GitRemoteRedirect.cs @@ -0,0 +1,45 @@ +using System.Text.RegularExpressions; +using CodeSpace.Core.Services.OutboundHttp; +using CodeSpace.Messages.Exceptions; + +namespace CodeSpace.Core.Services.Agents.Workspace; + +/// +/// Names the redirect a tokened git command met. git follows a redirect of the ref advertisement itself and makes the +/// origin it was sent to the remote's new base; the credential helper () answers for the +/// remote's own scheme and authority only, so at the new origin no credential is found and git fails with +/// could not read Username for '<that origin>': terminal prompts disabled — its own text, which reads like a +/// credential that was never configured. When that origin is not the remote's, the failure says what happened instead: the +/// remote redirected to <origin>, as a provider's REST call to the same address says +/// (, code provider_base_url_redirects). +/// +/// Still a : its catch sites — the push retry, a per-repo push that never fails a run, +/// the workflow nodes, the supervisor's grader — treat any git failure as one, and a failure that is a different type would +/// walk past all of them. A prompt at the remote's own origin is not a redirect (a remote that refused the credential once is +/// not asked again) and keeps its plain failure. Nothing is rewritten and nothing is sent anywhere new: this only reads what +/// git already reported. +/// +internal static partial class GitRemoteRedirect +{ + /// + /// as a , naming the redirect when — + /// the failed command's output against — shows git was sent to another origin. + /// + internal static WorkspaceException Failure(string remoteUrl, string stderr, string message) => + Named(remoteUrl, stderr) is { } redirect ? new WorkspaceException(message, redirect) : new WorkspaceException(message); + + /// The redirect shows, or null: git asked for a credential at an http(s) origin other than 's own. + internal static ProviderBaseUrlRedirectsException? Named(string remoteUrl, string stderr) + { + if (HttpUri(remoteUrl) is not { } remote || CredentialPrompt().Match(stderr) is not { Success: true } prompt) return null; + + if (HttpUri(prompt.Groups["origin"].Value) is not { } sentTo || OriginScopedRedirectHandler.IsSameOrigin(remote, sentTo)) return null; + + return new ProviderBaseUrlRedirectsException(remote, sentTo); + } + + private static Uri? HttpUri(string url) => Uri.TryCreate(url.Trim(), UriKind.Absolute, out var parsed) && (parsed.Scheme == Uri.UriSchemeHttp || parsed.Scheme == Uri.UriSchemeHttps) ? parsed : null; + + [GeneratedRegex(@"could not read (?:Username|Password) for '(?[^']+)'", RegexOptions.CultureInvariant)] + private static partial Regex CredentialPrompt(); +} diff --git a/backend/src/CodeSpace.Core/Services/Agents/Workspace/Integrators/LocalGitBranchIntegrator.cs b/backend/src/CodeSpace.Core/Services/Agents/Workspace/Integrators/LocalGitBranchIntegrator.cs index fb207df01..bca7832a6 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Workspace/Integrators/LocalGitBranchIntegrator.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Workspace/Integrators/LocalGitBranchIntegrator.cs @@ -216,7 +216,7 @@ private async Task CloneAsync(IntegrationRequest request, string directory, Canc var result = await RunTokenedGitAsync(request, new[] { "clone", url, directory }, directory, cancellationToken).ConfigureAwait(false); if (result.Status != SandboxStatus.Success) - throw TokenedGitCommand.OldGitFailure(Remote(request).IsTokened, result.Stderr) ?? new WorkspaceException($"git clone failed (exit {result.ExitCode}): {LocalGitWorkspaceProvider.Redact(Summarize(result.Stderr), request.Token)}"); + throw TokenedGitCommand.OldGitFailure(Remote(request).IsTokened, result.Stderr) ?? GitRemoteRedirect.Failure(url, result.Stderr, $"git clone failed (exit {result.ExitCode}): {LocalGitWorkspaceProvider.Redact(Summarize(result.Stderr), request.Token)}"); } /// Check out the EXACT shared base (detached) so every apply --3way resolves the pre-image against the commit the agents saw. False when the SHA is not in history (a bad base). @@ -457,7 +457,7 @@ private async Task CommitAsync(string directory, int count, CancellationToken ca return "remote integration branch advanced — refusing to clobber it"; // A genuine infrastructure failure (auth / network / repo gone) — surface it (token redacted). - throw TokenedGitCommand.OldGitFailure(Remote(request).IsTokened, result.Stderr) ?? new WorkspaceException($"git push failed (exit {result.ExitCode}): {LocalGitWorkspaceProvider.Redact(Summarize(result.Stderr), request.Token)}"); + throw TokenedGitCommand.OldGitFailure(Remote(request).IsTokened, result.Stderr) ?? GitRemoteRedirect.Failure(Remote(request).Url, result.Stderr, $"git push failed (exit {result.ExitCode}): {LocalGitWorkspaceProvider.Redact(Summarize(result.Stderr), request.Token)}"); } private async Task ResetToBaseAsync(string directory, IntegrationRequest request, CancellationToken cancellationToken) diff --git a/backend/src/CodeSpace.Core/Services/Agents/Workspace/Providers/LocalGitWorkspaceProvider.cs b/backend/src/CodeSpace.Core/Services/Agents/Workspace/Providers/LocalGitWorkspaceProvider.cs index 48db283ad..35a71987e 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Workspace/Providers/LocalGitWorkspaceProvider.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Workspace/Providers/LocalGitWorkspaceProvider.cs @@ -368,7 +368,7 @@ private async Task CloneAsync(WorkspaceRequest request, RepositoryCommandContext var result = await RunGitAsync(args, context, cancellationToken).ConfigureAwait(false); if (result.Status != SandboxStatus.Success) - throw TokenedGitCommand.OldGitFailure(context.Remote.IsTokened, result.Stderr) ?? new WorkspaceException($"git clone failed (exit {result.ExitCode}): {Redact(Summarize(result.Stderr), request.Token)}"); + throw TokenedGitCommand.OldGitFailure(context.Remote.IsTokened, result.Stderr) ?? GitRemoteRedirect.Failure(url, result.Stderr, $"git clone failed (exit {result.ExitCode}): {Redact(Summarize(result.Stderr), request.Token)}"); if (!string.IsNullOrWhiteSpace(request.PinnedSha)) await MaterializePinAsync(request, context, cancellationToken).ConfigureAwait(false); @@ -1007,7 +1007,7 @@ private async Task PushToRemoteOrThrowAsync(MaterializedRepo repo, string publis if (first.Status != SandboxStatus.Success && TokenedGitCommand.OldGitFailure(remote.IsTokened, first.Stderr) is { } oldGit) throw oldGit; - if (first.Status == SandboxStatus.Success || !ResolvesItsCredential(repo) || !IsCredentialRefusal(first.Stderr)) + if (first.Status == SandboxStatus.Success || !ResolvesItsCredential(repo) || !IsCredentialRefusal(first.Stderr) || GitRemoteRedirect.Named(remote.Url, first.Stderr) is not null) { EnsureSuccess(repo, args(remote.Url), first, remote.IsTokened); return; @@ -1198,7 +1198,7 @@ private static async Task EnsureSuccessAsync(MaterializedRepo repo, IRea private static string EnsureSuccess(MaterializedRepo repo, IReadOnlyList args, SandboxResult result, bool tokened = false) { if (result.Status != SandboxStatus.Success) - throw TokenedGitCommand.OldGitFailure(tokened, result.Stderr) ?? new WorkspaceException($"git {string.Join(' ', RedactArgs(args, repo.Token))} failed (exit {result.ExitCode}): {Redact(result.Stderr.Trim(), repo.Token)}"); + throw TokenedGitCommand.OldGitFailure(tokened, result.Stderr) ?? GitRemoteRedirect.Failure(repo.RepositoryUrl, result.Stderr, $"git {string.Join(' ', RedactArgs(args, repo.Token))} failed (exit {result.ExitCode}): {Redact(result.Stderr.Trim(), repo.Token)}"); SandboxOutputCompleteness.RequireStdout(result); return result.Stdout; diff --git a/backend/src/CodeSpace.Core/Services/Agents/Workspace/RemoteTipResolver.cs b/backend/src/CodeSpace.Core/Services/Agents/Workspace/RemoteTipResolver.cs index a5ae3f0d4..f581111b2 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Workspace/RemoteTipResolver.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Workspace/RemoteTipResolver.cs @@ -93,7 +93,7 @@ public sealed class RemoteTipResolver : IRemoteTipResolver, ISingletonDependency } if (result.Status != SandboxStatus.Success) - throw TokenedGitCommand.OldGitFailure(remote.IsTokened, result.Stderr) ?? new WorkspaceException($"git ls-remote failed (exit {result.ExitCode}) resolving the launch base for {SanitizeUrl(request.RepositoryUrl)}: {LocalGitWorkspaceProvider.Redact(Truncate(result.Stderr), request.Token)} — the launch base pin requires a reachable remote; the clone would fail the same way later"); + throw TokenedGitCommand.OldGitFailure(remote.IsTokened, result.Stderr) ?? GitRemoteRedirect.Failure(remote.Url, result.Stderr, $"git ls-remote failed (exit {result.ExitCode}) resolving the launch base for {SanitizeUrl(request.RepositoryUrl)}: {LocalGitWorkspaceProvider.Redact(Truncate(result.Stderr), request.Token)} — the launch base pin requires a reachable remote; the clone would fail the same way later"); // sha1 (40) and sha256 (64) object formats both count — a sha256 remote must not read as "ref missing". return result.Stdout diff --git a/backend/src/CodeSpace.Core/Services/Agents/Workspace/RepositoryWorkspaceResolver.cs b/backend/src/CodeSpace.Core/Services/Agents/Workspace/RepositoryWorkspaceResolver.cs index 9c1a13f26..128bf6f89 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Workspace/RepositoryWorkspaceResolver.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Workspace/RepositoryWorkspaceResolver.cs @@ -6,6 +6,7 @@ using CodeSpace.Messages.Agents; using CodeSpace.Messages.Enums; using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; namespace CodeSpace.Core.Services.Agents.Workspace; @@ -28,11 +29,13 @@ public sealed class RepositoryWorkspaceResolver : IAgentWorkspaceResolver, IScop { private readonly CodeSpaceDbContext _db; private readonly IProviderAuthResolver _auth; + private readonly ILogger _logger; - public RepositoryWorkspaceResolver(CodeSpaceDbContext db, IProviderAuthResolver auth) + public RepositoryWorkspaceResolver(CodeSpaceDbContext db, IProviderAuthResolver auth, ILogger logger) { _db = db; _auth = auth; + _logger = logger; } public async Task ResolveAsync(AgentTask task, Guid teamId, CancellationToken cancellationToken) @@ -77,6 +80,8 @@ public RepositoryWorkspaceResolver(CodeSpaceDbContext db, IProviderAuthResolver var token = await ResolveTokenAsync(repo, cancellationToken).ConfigureAwait(false); + CloneUrlAuthority.WarnWhenForeign(_logger, repo, credentialed: token is not null); + var requestedRef = string.IsNullOrWhiteSpace(@ref) ? null : @ref; return new WorkspaceRequest diff --git a/backend/src/CodeSpace.Core/Services/Agents/Workspace/WorkspaceException.cs b/backend/src/CodeSpace.Core/Services/Agents/Workspace/WorkspaceException.cs index 0333dcb5c..54de26029 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Workspace/WorkspaceException.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Workspace/WorkspaceException.cs @@ -1,3 +1,4 @@ +using CodeSpace.Messages.Exceptions; using CodeSpace.Messages.Failures; namespace CodeSpace.Core.Services.Agents.Workspace; @@ -13,10 +14,18 @@ public class WorkspaceException : Exception, IFailure { public FailureKind Kind => FailureKind.Unprocessable; - public string Code => FailureCodes.WorkspaceUnresolvable; + /// provider_base_url_redirects when git was redirected to another origin (), else workspace_unresolvable. + public string Code => Redirect?.Code ?? FailureCodes.WorkspaceUnresolvable; + /// The redirect's own details (the origin to edit the base URL to) when git was redirected; none otherwise. + public IReadOnlyDictionary? Details => Redirect?.Details; + + private ProviderBaseUrlRedirectsException? Redirect => InnerException as ProviderBaseUrlRedirectsException; public WorkspaceException(string message) : base(message) { } public WorkspaceException(string message, Exception inner) : base(message, inner) { } + + /// , followed by what the redirect says to do about it. + public WorkspaceException(string message, ProviderBaseUrlRedirectsException redirect) : base($"{message} — {redirect.Message}", redirect) { } } diff --git a/backend/src/CodeSpace.Core/Services/OutboundHttp/OriginScopedRedirectHandler.cs b/backend/src/CodeSpace.Core/Services/OutboundHttp/OriginScopedRedirectHandler.cs index ae1fd0d3e..76ad1734d 100644 --- a/backend/src/CodeSpace.Core/Services/OutboundHttp/OriginScopedRedirectHandler.cs +++ b/backend/src/CodeSpace.Core/Services/OutboundHttp/OriginScopedRedirectHandler.cs @@ -20,11 +20,33 @@ namespace CodeSpace.Core.Services.OutboundHttp; /// 300/301/302 after a POST, continue as a body-less GET; an https→http downgrade or a non-http scheme is not followed, /// and the 3xx is returned. Unlike it, at most hops (the built-in allows 50) are followed, /// after which the last 3xx is returned as is. +/// +/// A caller whose credential is its own and belongs to ONE origin — a git provider's client, its token attached to +/// every request — sets to false: a hop that would change origin is then not followed at +/// all and the 3xx is returned for the caller to name, rather than followed anonymously into a 401 that reads as a bad token. +/// A hop inside the origin is followed as above, with every header kept. +/// +/// A caller whose answer to a write is the write's own answer sets : a request that +/// changes something (anything but GET or HEAD) is then followed only by a redirect that repeats it — 307 or 308 — and a +/// 300, 301, 302 or 303 that would turn it into a GET, or leave its method and body to a client's guess, is returned as +/// the 3xx. Followed, a POST's answer would be the answer to a GET it never sent. /// public sealed class OriginScopedRedirectHandler : DelegatingHandler { public const int MaxRedirects = 10; + /// + /// Whether a hop that changes scheme, host or port is followed (without the request's headers) or returned as the 3xx + /// it is. True — the default, what an http.request workflow node has always had — follows it. + /// + public bool FollowsCrossOrigin { get; init; } = true; + + /// + /// Whether a request that changes something is followed only by a 307 or 308, which repeat it. False — the default, as + /// every HTTP client does — also follows a 300, 301, 302 or 303 that turns a POST into a GET (and a 303 of any write). + /// + public bool RepeatsWritesOnly { get; init; } + protected override async Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) { var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false); @@ -40,16 +62,20 @@ protected override async Task SendAsync(HttpRequestMessage return response; } - private static Uri? ReadFollowableTarget(HttpRequestMessage request, HttpResponseMessage response) + private Uri? ReadFollowableTarget(HttpRequestMessage request, HttpResponseMessage response) { if (!IsRedirect(response.StatusCode) || response.Headers.Location is not { } location) return null; var target = location.IsAbsoluteUri ? location : new Uri(request.RequestUri!, location); - return IsFollowable(request.RequestUri!, target) && !CarriesBodyAcrossOrigin(request, target, response.StatusCode) ? target : null; + return IsFollowable(request.RequestUri!, target) && (FollowsCrossOrigin || IsSameOrigin(request.RequestUri!, target)) && !CarriesBodyAcrossOrigin(request, target, response.StatusCode) && !(RepeatsWritesOnly && RewritesAWrite(request.Method, response.StatusCode)) ? target : null; } - private static bool IsRedirect(HttpStatusCode status) => (int)status is 300 or 301 or 302 or 303 or 307 or 308; + /// A redirect that is not a repeat of a request that changes something: a 300, 301, 302 or 303 answered to anything but a GET or a HEAD. + private static bool RewritesAWrite(HttpMethod method, HttpStatusCode status) => method != HttpMethod.Get && method != HttpMethod.Head && (int)status is 300 or 301 or 302 or 303; + + /// The statuses that carry a Location to follow: 300, 301, 302, 303, 307 and 308. + internal static bool IsRedirect(HttpStatusCode status) => (int)status is 300 or 301 or 302 or 303 or 307 or 308; /// https anywhere, or http from http — never a downgrade, never another scheme. private static bool IsFollowable(Uri current, Uri target) => target.Scheme == Uri.UriSchemeHttps || (target.Scheme == Uri.UriSchemeHttp && current.Scheme == Uri.UriSchemeHttp); @@ -65,7 +91,8 @@ private static void Retarget(HttpRequestMessage request, Uri target, HttpStatusC request.RequestUri = target; } - private static bool IsSameOrigin(Uri current, Uri target) => current.Scheme == target.Scheme && current.Port == target.Port && string.Equals(current.IdnHost, target.IdnHost, StringComparison.OrdinalIgnoreCase); + /// The same scheme, port and (case-insensitive, IDN-normalised) host — the origin a request's headers were written for. + internal static bool IsSameOrigin(Uri current, Uri target) => current.Scheme == target.Scheme && current.Port == target.Port && string.Equals(current.IdnHost, target.IdnHost, StringComparison.OrdinalIgnoreCase); private static bool ContinuesAsGet(HttpStatusCode status, HttpMethod method) => status == HttpStatusCode.SeeOther || ((int)status is 300 or 301 or 302 && method == HttpMethod.Post); diff --git a/backend/src/CodeSpace.Core/Services/ProviderInstances/IProviderEndpointProbe.cs b/backend/src/CodeSpace.Core/Services/ProviderInstances/IProviderEndpointProbe.cs new file mode 100644 index 000000000..0ef51b9ab --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/ProviderInstances/IProviderEndpointProbe.cs @@ -0,0 +1,29 @@ +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; + +namespace CodeSpace.Core.Services.ProviderInstances; + +/// +/// Asks a git provider's address, once, before it is stored. Whether it redirects to another origin — so the redirect is +/// named when the owner types it rather than as a 401 on every call afterwards — is best effort by contract: a host that is +/// down, slow, unresolvable or answers anything but a redirect leaves the URL as the owner wrote it. Whether it answers at +/// all is asked only of an http to https upgrade that connected tokens will follow, and there silence is the verdict. +/// +public interface IProviderEndpointProbe +{ + /// + /// Returns when (the address provider calls go to) answers no redirect to another origin — or + /// could not be asked, which is not a verdict on it. Throws naming the + /// URL it pointed at when it does redirect there. Never sends a credential. + /// + Task EnsureNoRedirectToAnotherOriginAsync(ProviderKind provider, string url, CancellationToken cancellationToken); + + /// + /// Returns when answers HTTP at all — any status, a redirect included. Throws + /// when it does not within the probe's deadline (a TLS handshake that + /// cannot complete, a host that is down or silent). Asked only of an http to https upgrade that connected tokens will + /// follow, where the one thing worse than a refused edit is an address nothing answers at that cannot be edited back; + /// a URL that is not an absolute http(s) one has nothing to answer and returns. Never sends a credential. + /// + Task EnsureAnswersAsync(ProviderKind provider, string url, CancellationToken cancellationToken); +} diff --git a/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderEndpointProbe.cs b/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderEndpointProbe.cs new file mode 100644 index 000000000..19db1d332 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderEndpointProbe.cs @@ -0,0 +1,123 @@ +using CodeSpace.Core.DependencyInjection; +using CodeSpace.Core.Services.OutboundHttp; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; +using Microsoft.Extensions.Logging; + +namespace CodeSpace.Core.Services.ProviderInstances; + +/// +/// GET {url}/api/v4/version against a GitLab, redirects off for any hop that leaves the origin (a hop inside it is +/// followed, so a path that normalises on the way is not read as a verdict), no credential and no cookie — the answer's +/// status is not read at all: GitLab wants a token for that route and says 401, which is as good as 200 here. Only a 3xx +/// to another scheme, host or port counts, and it is named, not followed. +/// +/// Everything else — a host that is down, unresolvable, slow (the call gives up after ), +/// not an http(s) URL, a provider with no known API root — is no verdict: the owner saved what they typed before this +/// existed, and a probe that blocked saves for hosts it cannot reach would be a regression for every air-gapped +/// deployment. The address is whatever the owner typed, as every provider call to it already is: no destination guard here +/// (an on-prem GitLab is on a private network), and the redirect it names is shown as scheme, host, port and path only. +/// +/// asks the same host the other question, for the one edit that is allowed past +/// connected tokens — an http to https upgrade of the same host: does anything answer HTTP at the new address? The route +/// does not matter (the address as typed, no credential, no cookie) and neither does the status, a redirect included; a TLS +/// handshake that cannot complete, a host that is down or one that never answers does. There silence is the verdict, because +/// an address nothing answers at cannot be edited back while the tokens are connected. +/// +public sealed class ProviderEndpointProbe : IProviderEndpointProbe, ISingletonDependency +{ + public static readonly TimeSpan DefaultTimeout = TimeSpan.FromSeconds(5); + + private const string GitLabVersionPath = "/api/v4/version"; + + private static readonly HttpClient Client = BuildClient(); + + private readonly TimeSpan _timeout; + private readonly ILogger _logger; + + public ProviderEndpointProbe(ILogger logger) : this(logger, DefaultTimeout) + { + } + + internal ProviderEndpointProbe(ILogger logger, TimeSpan timeout) + { + _logger = logger; + _timeout = timeout; + } + + public async Task EnsureNoRedirectToAnotherOriginAsync(ProviderKind provider, string url, CancellationToken cancellationToken) + { + if (ProbeUri(provider, url) is not { } probe) return; + + var answer = await AskAsync(probe, cancellationToken).ConfigureAwait(false); + + if (!answer.Received) _logger.LogInformation("Provider URL {Endpoint} could not be asked whether it redirects ({Reason}); it is saved as written", ProviderBaseUrlRedirectsException.OriginOf(probe), answer.Failure); + + if (answer.RedirectsTo is not { } target || OriginScopedRedirectHandler.IsSameOrigin(probe, target)) return; + + _logger.LogWarning("Provider URL {Endpoint} answers a redirect to {Target}, which is another origin", ProviderBaseUrlRedirectsException.OriginOf(probe), ProviderBaseUrlRedirectsException.Sanitised(target)); + + throw new ProviderBaseUrlRedirectsException(provider, probe, target); + } + + public async Task EnsureAnswersAsync(ProviderKind provider, string url, CancellationToken cancellationToken) + { + if (AnswerUri(url) is not { } address) return; + + var answer = await AskAsync(address, cancellationToken).ConfigureAwait(false); + + if (answer.Received) return; + + _logger.LogWarning("Provider URL {Endpoint} did not answer HTTP ({Reason}); an upgrade to it is refused while credentials are connected", ProviderBaseUrlRedirectsException.OriginOf(address), answer.Failure); + + throw new ProviderEndpointUnreachableException(provider, address, answer.Failure); + } + + /// The API root probed for at , userinfo dropped so none is sent; null when the provider has none or the URL is not an absolute http(s) one. + private static Uri? ProbeUri(ProviderKind provider, string url) + { + if (provider != ProviderKind.GitLab || !Uri.TryCreate(url?.Trim(), UriKind.Absolute, out var parsed) || (parsed.Scheme != Uri.UriSchemeHttp && parsed.Scheme != Uri.UriSchemeHttps)) return null; + + return new UriBuilder(parsed) { UserName = "", Password = "", Path = parsed.AbsolutePath.TrimEnd('/') + GitLabVersionPath, Query = "", Fragment = "" }.Uri; + } + + /// The address as typed for — userinfo, query and fragment dropped so none is sent; null when it is not an absolute http(s) URL. + private static Uri? AnswerUri(string url) + { + if (!Uri.TryCreate(url?.Trim(), UriKind.Absolute, out var parsed) || (parsed.Scheme != Uri.UriSchemeHttp && parsed.Scheme != Uri.UriSchemeHttps)) return null; + + return new UriBuilder(parsed) { UserName = "", Password = "", Path = parsed.AbsolutePath.TrimEnd('/'), Query = "", Fragment = "" }.Uri; + } + + /// What answered: whether anything did, where it redirected to (null when it did not), and, when nothing answered, why. + private readonly record struct Answer(bool Received, Uri? RedirectsTo, string Failure); + + /// One GET of . A host that cannot be reached, a handshake that cannot complete and the deadline are an that was not received; the caller's own cancellation is not swallowed. + private async Task AskAsync(Uri probe, CancellationToken cancellationToken) + { + using var deadline = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + deadline.CancelAfter(_timeout); + + try + { + using var request = new HttpRequestMessage(HttpMethod.Get, probe); + using var response = await Client.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, deadline.Token).ConfigureAwait(false); + + return new Answer(true, RedirectOf(probe, response), string.Empty); + } + catch (Exception ex) when (ex is not OperationCanceledException || !cancellationToken.IsCancellationRequested) + { + return new Answer(false, null, ex.GetType().Name); + } + } + + /// Where redirects to, or null when it does not. + private static Uri? RedirectOf(Uri probe, HttpResponseMessage response) + { + if (!OriginScopedRedirectHandler.IsRedirect(response.StatusCode) || response.Headers.Location is not { } location) return null; + + return location.IsAbsoluteUri ? location : new Uri(probe, location); + } + + private static HttpClient BuildClient() => new(new OriginScopedRedirectHandler { FollowsCrossOrigin = false, InnerHandler = new SocketsHttpHandler { AllowAutoRedirect = false, UseCookies = false, ConnectTimeout = DefaultTimeout } }) { Timeout = Timeout.InfiniteTimeSpan }; +} diff --git a/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs b/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs index 0cad3147e..398aa3bd3 100644 --- a/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs +++ b/backend/src/CodeSpace.Core/Services/ProviderInstances/ProviderInstanceService.cs @@ -31,10 +31,11 @@ public sealed partial class ProviderInstanceService : IProviderInstanceService, private readonly ICredentialRevocationService _revocation; private readonly IOAuthGrantRevoker _grants; private readonly IPostCommitActions _postCommit; + private readonly IProviderEndpointProbe _endpointProbe; private readonly TimeProvider _clock; private readonly ILogger _logger; - public ProviderInstanceService(CodeSpaceDbContext db, ICurrentTeam currentTeam, ICurrentUser currentUser, IPayloadEncryptor encryptor, IRepositoryBindingService binding, IWebhookScopeTransitionService webhookScope, IConnectionWebhookProvisioner connectionHooks, ICredentialRevocationService revocation, IOAuthGrantRevoker grants, IPostCommitActions postCommit, TimeProvider clock, ILogger logger) + public ProviderInstanceService(CodeSpaceDbContext db, ICurrentTeam currentTeam, ICurrentUser currentUser, IPayloadEncryptor encryptor, IRepositoryBindingService binding, IWebhookScopeTransitionService webhookScope, IConnectionWebhookProvisioner connectionHooks, ICredentialRevocationService revocation, IOAuthGrantRevoker grants, IPostCommitActions postCommit, IProviderEndpointProbe endpointProbe, TimeProvider clock, ILogger logger) { _db = db; _currentTeam = currentTeam; @@ -46,6 +47,7 @@ public ProviderInstanceService(CodeSpaceDbContext db, ICurrentTeam currentTeam, _revocation = revocation; _grants = grants; _postCommit = postCommit; + _endpointProbe = endpointProbe; _clock = clock; _logger = logger; } @@ -104,6 +106,7 @@ public async Task AddAsync(ProviderKind provider, string displayName, stri EnsureOAuthPairConsistent(normalisedClientId, oauthClientSecret); await EnsureNotDuplicateAsync(teamId, provider, normalisedBaseUrl, normalisedClientId, cancellationToken).ConfigureAwait(false); + await _endpointProbe.EnsureNoRedirectToAnotherOriginAsync(provider, ApiOf(normalisedBaseUrl, apiUrl), cancellationToken).ConfigureAwait(false); var instance = new ProviderInstance { @@ -142,6 +145,8 @@ public async Task UpdateAsync(UpdateProviderInstanceCommand request, Cancellatio if (baseUrlChanged || clientIdChanged) await EnsureUniquenessKeyAvailableAsync(instance, newBaseUrl, newClientId, cancellationToken).ConfigureAwait(false); await EnsureSecretsStayWithTheirEndpointAsync(instance, edits, cancellationToken).ConfigureAwait(false); + await EnsureAnUpgradeAnswersWhileSecretsAreBoundAsync(instance, edits, cancellationToken).ConfigureAwait(false); + await EnsureAnEditedApiAddressDoesNotRedirectAsync(instance, edits, cancellationToken).ConfigureAwait(false); LogEndpointMove(instance, edits); var previousScope = instance.WebhookScope; @@ -168,9 +173,18 @@ private static UpdateProviderInstanceCommand WithStoredUrlsKept(ProviderInstance /// Whether an edit moves where this connection presents its secrets: the API every provider call goes to /// (ApiUrl, or the base URL when unset) or the base URL the OAuth token, authorize and revoke endpoints are /// derived from. A move is a change of scheme, host or port (); a path, a - /// trailing slash or a credential slot is not. + /// trailing slash or a credential slot is not — and neither is an upgrade of http to https on the very same host + /// (), which a base URL that redirects to https asks for and which sends a + /// token nowhere new, only over TLS: refusing it while tokens are connected left the owner no way to fix the URL + /// short of disconnecting every member's token. /// internal static bool EndpointMoves(string storedBaseUrl, string? storedApiUrl, string nextBaseUrl, string? nextApiUrl) => + UrlMoves(storedBaseUrl, nextBaseUrl) || UrlMoves(ApiOf(storedBaseUrl, storedApiUrl), ApiOf(nextBaseUrl, nextApiUrl)); + + private static bool UrlMoves(string stored, string next) => !CredentialUrl.SameEndpoint(stored, next) && !CredentialUrl.IsHttpsUpgrade(stored, next); + + /// Whether an edit changes the scheme, host or port of either URL at all — the moves, and the http to https upgrades that are allowed past bound credentials. + internal static bool EndpointChanges(string storedBaseUrl, string? storedApiUrl, string nextBaseUrl, string? nextApiUrl) => !CredentialUrl.SameEndpoint(storedBaseUrl, nextBaseUrl) || !CredentialUrl.SameEndpoint(ApiOf(storedBaseUrl, storedApiUrl), ApiOf(nextBaseUrl, nextApiUrl)); /// @@ -193,19 +207,60 @@ private async Task EnsureSecretsStayWithTheirEndpointAsync(ProviderInstance inst throw new ProviderEndpointSecretRequiredException(instance.Id); } + /// + /// An http to https upgrade is the one change of endpoint a connected token may follow, because it sends the token nowhere + /// new; it can still land every connected token on an address nothing serves over TLS (a host with no https, a certificate + /// this server does not trust), and putting the old address back is a move, refused while the tokens are connected — the + /// owner would be stranded where the carve-out was meant to free them. So while any token is connected the new address + /// must answer HTTP before the edit is kept, and the stored one stays otherwise. With nothing connected a mistaken address + /// is one edit away from undone, so nothing is asked; an edit that relies on no upgrade asks nothing either. + /// + private async Task EnsureAnUpgradeAnswersWhileSecretsAreBoundAsync(ProviderInstance instance, UpdateProviderInstanceCommand edits, CancellationToken cancellationToken) + { + var upgraded = UpgradedAddresses(instance, edits); + + if (upgraded.Count == 0 || await CountPresentableCredentialsAsync(instance.Id, cancellationToken).ConfigureAwait(false) == 0) return; + + foreach (var address in upgraded) await _endpointProbe.EnsureAnswersAsync(instance.Provider, address, cancellationToken).ConfigureAwait(false); + } + + /// The edit's next base URL and API address, each when it differs from the stored one by an http to https upgrade of the same host and nothing else (). + private static IReadOnlyList UpgradedAddresses(ProviderInstance instance, UpdateProviderInstanceCommand edits) + { + var nextBaseUrl = NextBaseUrl(instance, edits); + var addresses = new[] { (Stored: instance.BaseUrl, Next: nextBaseUrl), (Stored: ApiOf(instance.BaseUrl, instance.ApiUrl), Next: ApiOf(nextBaseUrl, NextApiUrl(instance, edits))) }; + + return addresses.Where(address => CredentialUrl.IsHttpsUpgrade(address.Stored, address.Next)).Select(address => address.Next).Distinct(StringComparer.Ordinal).ToList(); + } + + /// + /// A new API address is asked once whether it redirects to another origin, so the redirect is named now, to the owner + /// who typed it, instead of reading as a 401 on every call afterwards. After the refusals above, so a repoint that is + /// refused never contacts the host it named. An edit that leaves the address as stored asks nothing — a connection + /// whose stored URL already redirects keeps saving its other fields; its calls name the redirect when they meet it. + /// + private async Task EnsureAnEditedApiAddressDoesNotRedirectAsync(ProviderInstance instance, UpdateProviderInstanceCommand edits, CancellationToken cancellationToken) + { + var next = ApiOf(NextBaseUrl(instance, edits), NextApiUrl(instance, edits)); + + if (string.Equals(next, ApiOf(instance.BaseUrl, instance.ApiUrl), StringComparison.Ordinal)) return; + + await _endpointProbe.EnsureNoRedirectToAnotherOriginAsync(instance.Provider, next, cancellationToken).ConfigureAwait(false); + } + /// Credentials that would still be presented: connected through this instance and not disconnected (a disconnect revokes the row and blanks its token). private Task CountPresentableCredentialsAsync(Guid providerInstanceId, CancellationToken cancellationToken) => _db.Credential.AsNoTracking().CountAsync(c => c.ProviderInstanceId == providerInstanceId && c.DeletedDate == null && c.Status != CredentialStatus.Revoked, cancellationToken); - /// One audit line per endpoint move, naming only scheme://host:port on each side () — never embedded credentials, path or query. + /// One audit line per endpoint change — a move, or an http to https upgrade — naming only scheme://host:port on each side () — never embedded credentials, path or query. private void LogEndpointMove(ProviderInstance instance, UpdateProviderInstanceCommand edits) { var nextBaseUrl = NextBaseUrl(instance, edits); var nextApiUrl = NextApiUrl(instance, edits); - if (!EndpointMoves(instance.BaseUrl, instance.ApiUrl, nextBaseUrl, nextApiUrl)) return; + if (!EndpointChanges(instance.BaseUrl, instance.ApiUrl, nextBaseUrl, nextApiUrl)) return; - _logger.LogInformation("Provider instance {ProviderInstanceId} of team {TeamId} moved its endpoint from {PreviousEndpoint} (API {PreviousApiEndpoint}) to {Endpoint} (API {ApiEndpoint}), by user {UserId}", instance.Id, instance.TeamId, CredentialUrl.Endpoint(instance.BaseUrl), CredentialUrl.Endpoint(ApiOf(instance.BaseUrl, instance.ApiUrl)), CredentialUrl.Endpoint(nextBaseUrl), CredentialUrl.Endpoint(ApiOf(nextBaseUrl, nextApiUrl)), _currentUser.Id); + _logger.LogInformation("Provider instance {ProviderInstanceId} of team {TeamId} moved its endpoint from {PreviousEndpoint} (API {PreviousApiEndpoint}) to {Endpoint} (API {ApiEndpoint}), by user {UserId}; an http to https upgrade of the same host: {HttpsUpgrade}", instance.Id, instance.TeamId, CredentialUrl.Endpoint(instance.BaseUrl), CredentialUrl.Endpoint(ApiOf(instance.BaseUrl, instance.ApiUrl)), CredentialUrl.Endpoint(nextBaseUrl), CredentialUrl.Endpoint(ApiOf(nextBaseUrl, nextApiUrl)), _currentUser.Id, !EndpointMoves(instance.BaseUrl, instance.ApiUrl, nextBaseUrl, nextApiUrl)); } private static string NextBaseUrl(ProviderInstance instance, UpdateProviderInstanceCommand edits) => edits.BaseUrl != null ? NormaliseBaseUrl(edits.BaseUrl) : instance.BaseUrl; diff --git a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabCredential.cs b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabCredential.cs new file mode 100644 index 000000000..9a59efed3 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabCredential.cs @@ -0,0 +1,58 @@ +using System.Net; +using CodeSpace.Messages.Enums; + +namespace CodeSpace.Core.Services.Providers.GitLab; + +/// +/// The token a GitLab call presents and the ONE header it travels in. A personal, project or group access token goes in +/// PRIVATE-TOKEN, which every GitLab version reads as exactly that; an OAuth token goes in +/// Authorization: Bearer. Never both, and never an access token as a Bearer: GitLab 13.0 to 17.7 reads a Bearer +/// value of 41 or more characters as an OAuth token alone, so a long access token (a custom prefix, a 66-character token) is +/// refused there however valid it is — a 401 that looks like a revoked token. Sending both made the raw calls work with a +/// PAT and fail with an OAuth token as soon as a redirect dropped the Bearer, and the typed client (Bearer only) fail on +/// every long access token. +/// +/// A struct that overrides : a record's generated one would print the token into whatever +/// message or log line a value of this type is formatted into. +/// +internal readonly struct GitLabCredential +{ + internal const string PrivateTokenHeader = "PRIVATE-TOKEN"; + + internal const string AuthorizationHeader = "Authorization"; + + public GitLabCredential(string token, AuthType type) + { + Token = token; + Type = type; + } + + public string Token { get; } + + public AuthType Type { get; } + + /// The header this credential's type names: PRIVATE-TOKEN for an access token, Authorization for anything else (OAuth). + public string HeaderName => IsAccessToken ? PrivateTokenHeader : AuthorizationHeader; + + public string HeaderValue => IsAccessToken ? Token : $"Bearer {Token}"; + + private bool IsAccessToken => Type is AuthType.Pat or AuthType.ProjectAccessToken or AuthType.GroupAccessToken; + + /// This credential as the request's only credential header — a stale one of either name is removed first. + public void Attach(HttpRequestMessage request) + { + request.Headers.Remove(PrivateTokenHeader); + request.Headers.Remove(AuthorizationHeader); + request.Headers.TryAddWithoutValidation(HeaderName, HeaderValue); + } + + /// The same, for NGitLab's request, whose headers it has already written an Authorization: Bearer into. + public void Attach(WebHeaderCollection headers) + { + headers.Remove(PrivateTokenHeader); + headers.Remove(AuthorizationHeader); + headers[HeaderName] = HeaderValue; + } + + public override string ToString() => $"{HeaderName} credential ({Type})"; +} diff --git a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabHttp.cs b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabHttp.cs new file mode 100644 index 000000000..66fdfef22 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabHttp.cs @@ -0,0 +1,57 @@ +using CodeSpace.Core.Services.OutboundHttp; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; +using NGitLab; +using NGitLab.Impl; + +namespace CodeSpace.Core.Services.Providers.GitLab; + +/// +/// The one way the GitLab provider talks to GitLab: both of NGitLab's request paths and every raw HTTP call send the +/// credential in the header its type names () and follow a redirect only inside one origin +/// with that credential attached again. A redirect to another origin — http to https, a host alias, another port — is not +/// followed, so nothing is sent there, and fails with naming where GitLab +/// pointed. The raw calls' client had followed every redirect itself, keeping a PRIVATE-TOKEN across it. +/// +internal static class GitLabHttp +{ + /// + /// One client for the process, as the raw calls' always was: connections are pooled, and each request carries its own + /// credential, so one client serves every connection's token. Redirects are followed by the handler, not the primary + /// handler, which is where a cross-origin hop is refused rather than followed. A write is followed only by a redirect that + /// repeats it (307, 308): a POST turned into a GET by a 301 or 302 would hand back that GET's answer as the write's, so + /// it is refused instead, as refuses any write NGitLab sent. + /// + private static readonly HttpClient Client = new(new OriginScopedRedirectHandler { FollowsCrossOrigin = false, RepeatsWritesOnly = true, InnerHandler = new SocketsHttpHandler { AllowAutoRedirect = false } }); + + /// NGitLab's typed client, its requests sent as sends them. + public static GitLabClient NewClient(string host, GitLabCredential credential) => new(host, credential.Token, new GitLabRequestOptions(credential, host)); + + /// NGitLab's raw request builder — what a paged read is made from — sent the same way. + public static API NewApi(string host, GitLabCredential credential) => new(new GitLabCredentials(host, credential.Token), new GitLabRequestOptions(credential, host)); + + /// + /// sent with . A redirect that was not followed throws + /// instead of coming back as a 3xx the caller would read as some other + /// failure, so no call site has to know about redirects. + /// + public static async Task SendAsync(HttpRequestMessage request, GitLabCredential credential, HttpCompletionOption completion, CancellationToken cancellationToken) + { + var requested = request.RequestUri!; + + credential.Attach(request); + + var response = await Client.SendAsync(request, completion, cancellationToken).ConfigureAwait(false); + + if (!OriginScopedRedirectHandler.IsRedirect(response.StatusCode) || response.Headers.Location is not { } location) return response; + + var target = location.IsAbsoluteUri ? location : new Uri(response.RequestMessage!.RequestUri!, location); + + response.Dispose(); + + throw new ProviderBaseUrlRedirectsException(ProviderKind.GitLab, requested, target); + } + + /// reading the whole answer. + public static Task SendAsync(HttpRequestMessage request, GitLabCredential credential, CancellationToken cancellationToken) => SendAsync(request, credential, HttpCompletionOption.ResponseContentRead, cancellationToken); +} diff --git a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.ConnectionWebhooks.cs b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.ConnectionWebhooks.cs index 808fcb232..c4cb34ca8 100644 --- a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.ConnectionWebhooks.cs +++ b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.ConnectionWebhooks.cs @@ -35,17 +35,17 @@ public sealed partial class GitLabRepositoryProvider : IConnectionWebhookRegistr public async Task FindConnectionWebhookByCallbackUrlAsync(ProviderContext context, string ownerPath, string callbackUrl, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var url = BuildGroupHooksUrl(host, ownerPath, null); - var hooks = await _resilience.ExecuteAsync(context.Instance, nameof(FindConnectionWebhookByCallbackUrlAsync), _ => ListGroupHooksAsync(url, token, cancellationToken), cancellationToken).ConfigureAwait(false); + var hooks = await _resilience.ExecuteAsync(context.Instance, nameof(FindConnectionWebhookByCallbackUrlAsync), _ => ListGroupHooksAsync(url, credential, cancellationToken), cancellationToken).ConfigureAwait(false); return MatchGroupHookByCallbackUrl(hooks, callbackUrl); } public async Task RegisterConnectionWebhookAsync(ProviderContext context, string ownerPath, WebhookRegistration request, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var url = BuildGroupHooksUrl(host, ownerPath, null); var payload = JsonSerializer.Serialize(BuildGroupHookUpsert(request)); @@ -54,11 +54,11 @@ public async Task RegisterConnectionWebhookAsync(ProviderContext // group's hooks for the one at this registration's callback URL, which carries the row's own id. The create's // answer is judged outside the wrapper: a 4xx/5xx is the registration's failure, not retried. var answer = await _resilience.ExecuteNonIdempotentAsync(context.Instance, nameof(RegisterConnectionWebhookAsync), - _ => SendGroupHookRequestAsync(HttpMethod.Post, url, token, payload, cancellationToken), - _ => FindLandedGroupHookAsync(url, token, request.CallbackUrl, cancellationToken), + _ => SendGroupHookRequestAsync(HttpMethod.Post, url, credential, payload, cancellationToken), + _ => FindLandedGroupHookAsync(url, credential, request.CallbackUrl, cancellationToken), cancellationToken).ConfigureAwait(false); - var created = JsonSerializer.Deserialize(EnsureAccepted(answer, CaptureGroupHookRequest("POST", url, token, payload)).Body, _snakeCaseJson); + var created = JsonSerializer.Deserialize(EnsureAccepted(answer, CaptureGroupHookRequest("POST", url, credential, payload)).Body, _snakeCaseJson); if (created == null) throw new InvalidOperationException($"GitLab accepted the group hook on {ownerPath} but answered a body we could not read"); @@ -68,12 +68,12 @@ public async Task RegisterConnectionWebhookAsync(ProviderContext public async Task DeleteConnectionWebhookAsync(ProviderContext context, string ownerPath, string externalWebhookId, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var url = BuildGroupHooksUrl(host, ownerPath, externalWebhookId); - var answer = await _resilience.ExecuteAsync(context.Instance, nameof(DeleteConnectionWebhookAsync), _ => SendGroupHookRequestAsync(HttpMethod.Delete, url, token, null, cancellationToken), cancellationToken).ConfigureAwait(false); + var answer = await _resilience.ExecuteAsync(context.Instance, nameof(DeleteConnectionWebhookAsync), _ => SendGroupHookRequestAsync(HttpMethod.Delete, url, credential, null, cancellationToken), cancellationToken).ConfigureAwait(false); - EnsureAccepted(answer, CaptureGroupHookRequest("DELETE", url, token, null)); + EnsureAccepted(answer, CaptureGroupHookRequest("DELETE", url, credential, null)); } /// @@ -94,9 +94,9 @@ private static GroupHookAnswer EnsureAccepted(GroupHookAnswer answer, CapturedPr /// refuses fails the attempt with the list's own answer rather than reading as "nothing /// landed", so a probe that cannot tell never leads to a second create. /// - private static async Task FindLandedGroupHookAsync(string url, string token, string callbackUrl, CancellationToken cancellationToken) + private static async Task FindLandedGroupHookAsync(string url, GitLabCredential credential, string callbackUrl, CancellationToken cancellationToken) { - var landed = FindGroupHook(await ListGroupHooksAsync(url, token, cancellationToken).ConfigureAwait(false), callbackUrl); + var landed = FindGroupHook(await ListGroupHooksAsync(url, credential, cancellationToken).ConfigureAwait(false), callbackUrl); return landed == null ? null : new GroupHookAnswer(HttpStatusCode.OK, JsonSerializer.Serialize(landed, _snakeCaseJson)); } @@ -108,7 +108,7 @@ private static GroupHookAnswer EnsureAccepted(GroupHookAnswer answer, CapturedPr /// for GitLab's largest page and follows X-Next-Page forward until GitLab names no later page. Every page is /// judged like any other answer: a refused page leaves the list unknown, never shorter. /// - private static async Task> ListGroupHooksAsync(string url, string token, CancellationToken cancellationToken) + private static async Task> ListGroupHooksAsync(string url, GitLabCredential credential, CancellationToken cancellationToken) { var hooks = new List(); int? page = 1; @@ -116,7 +116,7 @@ private static async Task> ListGroupHooksAsync(string url, while (page is { } current) { var pageUrl = $"{url}?per_page={GroupHooksPageSize}&page={current}"; - var answer = EnsureAccepted(await SendGroupHookRequestAsync(HttpMethod.Get, pageUrl, token, null, cancellationToken).ConfigureAwait(false), CaptureGroupHookRequest("GET", pageUrl, token, null)); + var answer = EnsureAccepted(await SendGroupHookRequestAsync(HttpMethod.Get, pageUrl, credential, null, cancellationToken).ConfigureAwait(false), CaptureGroupHookRequest("GET", pageUrl, credential, null)); hooks.AddRange(JsonSerializer.Deserialize>(answer.Body, _snakeCaseJson) ?? new List()); page = NextPageAfter(current, answer.NextPage); @@ -135,22 +135,19 @@ private static async Task> ListGroupHooksAsync(string url, /// /// One raw call to the group-hook endpoint. A refusal's body is the evidence - /// persists to the attempt row any member can read, so the this request presented is struck + /// persists to the attempt row any member can read, so the token this presented is struck /// from it before anything reads it; an accepted answer is read as sent. /// - private static async Task SendGroupHookRequestAsync(HttpMethod method, string url, string token, string? payload, CancellationToken cancellationToken) + private static async Task SendGroupHookRequestAsync(HttpMethod method, string url, GitLabCredential credential, string? payload, CancellationToken cancellationToken) { using var request = new HttpRequestMessage(method, url); if (payload != null) request.Content = new StringContent(payload, Encoding.UTF8, "application/json"); - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", token); - - using var response = await _countsHttpClient.SendAsync(request, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, cancellationToken).ConfigureAwait(false); var body = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); - return new GroupHookAnswer(response.StatusCode, response.IsSuccessStatusCode ? body : CredentialNeedles.Strike(body, [token]), ReadNextPage(response)); + return new GroupHookAnswer(response.StatusCode, response.IsSuccessStatusCode ? body : CredentialNeedles.Strike(body, [credential.Token]), ReadNextPage(response)); } /// GitLab's X-Next-Page: the next page's number on a list, empty on its last page, absent on anything else. @@ -193,11 +190,11 @@ private static string BuildGroupHooksUrl(string host, string ownerPath, string? return hookId == null ? root : $"{root}/{hookId}"; } - private static CapturedProviderRequest CaptureGroupHookRequest(string method, string url, string token, string? body) + private static CapturedProviderRequest CaptureGroupHookRequest(string method, string url, GitLabCredential credential, string? body) { - var headers = new Dictionary { ["Authorization"] = $"Bearer {token}" }; + var headers = new Dictionary { [credential.HeaderName] = credential.HeaderValue }; - return ProviderCallCapture.CaptureRedacted(method, url, headers, body, new[] { token }); + return ProviderCallCapture.CaptureRedacted(method, url, headers, body, new[] { credential.Token }); } private static RemoteWebhook? MatchGroupHookByCallbackUrl(IEnumerable hooks, string callbackUrl) diff --git a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.MemberStanding.cs b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.MemberStanding.cs index 00a8c0870..cdebfb519 100644 --- a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.MemberStanding.cs +++ b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.MemberStanding.cs @@ -17,15 +17,12 @@ public sealed partial class GitLabRepositoryProvider : IRepositoryMemberStanding public async Task GetMemberStandingAsync(ProviderContext context, RemoteRepository repository, string userExternalId, CancellationToken cancellationToken) { - var auth = await _authResolver.ResolveAsync(context, cancellationToken).ConfigureAwait(false); - var host = string.IsNullOrWhiteSpace(context.Instance.ApiUrl) ? context.Instance.BaseUrl : context.Instance.ApiUrl; + var (host, credential) = await ResolveCredentialAsync(context, cancellationToken).ConfigureAwait(false); var url = $"{host.TrimEnd('/')}/api/v4/projects/{Uri.EscapeDataString(repository.ExternalId)}/members/all/{Uri.EscapeDataString(userExternalId)}"; using var request = new HttpRequestMessage(HttpMethod.Get, url); - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {auth.Token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", auth.Token); - using var response = await _countsHttpClient.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, cancellationToken).ConfigureAwait(false); if ((int)response.StatusCode == 404) return PullRequestAuthorAssociation.None; if (!response.IsSuccessStatusCode) return PullRequestAuthorAssociation.Unknown; diff --git a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.cs b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.cs index 6651f3455..ccdc2b1b6 100644 --- a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.cs +++ b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRepositoryProvider.cs @@ -166,8 +166,8 @@ private sealed record GitLabProjectsCountConnection(int Count); /// public async Task> ListPullRequestsAsync(ProviderContext context, RemoteRepository repository, PullRequestState? stateFilter, int page, int perPage, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); - var reader = new ProjectListReader(context.Instance, new NGitLab.Impl.API(new NGitLab.Impl.GitLabCredentials(host, token)).Get(), int.Parse(repository.ExternalId)); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var reader = new ProjectListReader(context.Instance, GitLabHttp.NewApi(host, credential).Get(), int.Parse(repository.ExternalId)); // GitLab's `state=closed` is NOT the same as GitHub's `Closed` — it excludes // merged MRs, which on GitLab are a distinct state (`merged`), not closed-with-flag. @@ -413,8 +413,8 @@ private static string SourceBranchRequestedDetail(MergeRequest accepted) => /// public async Task> ListIssuesAsync(ProviderContext context, RemoteRepository repository, IssueState? stateFilter, int page, int perPage, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); - var reader = new ProjectListReader(context.Instance, new NGitLab.Impl.API(new NGitLab.Impl.GitLabCredentials(host, token)).Get(), int.Parse(repository.ExternalId)); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var reader = new ProjectListReader(context.Instance, GitLabHttp.NewApi(host, credential).Get(), int.Parse(repository.ExternalId)); var issues = await ReadPageAsync(reader, nameof(ListIssuesAsync), IssuePageUrl(reader.ProjectId, MapIssueStateFilterToGitLab(stateFilter), page, perPage), cancellationToken).ConfigureAwait(false); @@ -472,11 +472,10 @@ private sealed record GitLabIssueCountsProject(GitLabCountsBucket? Opened, GitLa /// Issue counts via the REST X-Total header — the read_api-less fallback, mirroring for MRs. private async Task CountIssuesViaXTotalAsync(ProviderContext context, int projectId, CancellationToken cancellationToken) { - var auth = await _authResolver.ResolveAsync(context, cancellationToken).ConfigureAwait(false); - var host = string.IsNullOrWhiteSpace(context.Instance.ApiUrl) ? context.Instance.BaseUrl : context.Instance.ApiUrl; + var (host, credential) = await ResolveCredentialAsync(context, cancellationToken).ConfigureAwait(false); - var openTask = FetchStateCountAsync(host, projectId, "issues", "opened", auth, cancellationToken); - var closedTask = FetchStateCountAsync(host, projectId, "issues", "closed", auth, cancellationToken); + var openTask = FetchStateCountAsync(host, projectId, "issues", "opened", credential, cancellationToken); + var closedTask = FetchStateCountAsync(host, projectId, "issues", "closed", credential, cancellationToken); await Task.WhenAll(openTask, closedTask).ConfigureAwait(false); @@ -730,14 +729,6 @@ public async Task> ListPullRequestFilesAsyn }, cancellationToken).ConfigureAwait(false); } - /// - /// Singleton HttpClient for the X-Total-header fallback path. .NET best practice — one - /// HttpClient per process (or per host) to avoid socket exhaustion. Per-call auth headers - /// are attached to each HttpRequestMessage individually so the same client can serve every - /// credential safely. - /// - private static readonly HttpClient _countsHttpClient = new(); - public async Task CountPullRequestsAsync(ProviderContext context, RemoteRepository repository, CancellationToken cancellationToken) { var client = await BuildClientAsync(context, cancellationToken).ConfigureAwait(false); @@ -814,12 +805,11 @@ private sealed record GitLabCountsBucket(int Count); /// private async Task CountViaXTotalAsync(ProviderContext context, int projectId, CancellationToken cancellationToken) { - var auth = await _authResolver.ResolveAsync(context, cancellationToken).ConfigureAwait(false); - var host = string.IsNullOrWhiteSpace(context.Instance.ApiUrl) ? context.Instance.BaseUrl : context.Instance.ApiUrl; + var (host, credential) = await ResolveCredentialAsync(context, cancellationToken).ConfigureAwait(false); - var openTask = FetchStateCountAsync(host, projectId, "merge_requests", "opened", auth, cancellationToken); - var closedTask = FetchStateCountAsync(host, projectId, "merge_requests", "closed", auth, cancellationToken); - var mergedTask = FetchStateCountAsync(host, projectId, "merge_requests", "merged", auth, cancellationToken); + var openTask = FetchStateCountAsync(host, projectId, "merge_requests", "opened", credential, cancellationToken); + var closedTask = FetchStateCountAsync(host, projectId, "merge_requests", "closed", credential, cancellationToken); + var mergedTask = FetchStateCountAsync(host, projectId, "merge_requests", "merged", credential, cancellationToken); await Task.WhenAll(openTask, closedTask, mergedTask).ConfigureAwait(false); @@ -839,20 +829,14 @@ private async Task CountViaXTotalAsync(ProviderContext /// with PerPage=1, parses the X-Total header. Returns 0 when the header is missing (GitLab's /// >10k pagination cutoff) or the response is non-success — counts should never error out the whole call. /// - private static async Task FetchStateCountAsync(string host, int projectId, string resource, string state, ResolvedAuth auth, CancellationToken cancellationToken) + private static async Task FetchStateCountAsync(string host, int projectId, string resource, string state, GitLabCredential credential, CancellationToken cancellationToken) { var url = $"{host.TrimEnd('/')}/api/v4/projects/{projectId}/{resource}?state={state}&per_page=1"; using var request = new HttpRequestMessage(HttpMethod.Get, url); - // GitLab accepts the OAuth Bearer token via the standard Authorization header - // and PATs via PRIVATE-TOKEN — both work for /merge_requests. Bearer is the - // common case (OAuth credentials are most users), PAT is the legacy fallback. - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {auth.Token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", auth.Token); - try { - using var response = await _countsHttpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); if (!response.IsSuccessStatusCode) return 0; if (response.Headers.TryGetValues("X-Total", out var values) && int.TryParse(values.FirstOrDefault(), out var total)) @@ -961,7 +945,7 @@ public async Task PostCommentAsync(ProviderContext con public async Task SubmitReviewAsync(ProviderContext context, RemoteRepository repository, int number, SubmitPullRequestReviewInput input, CancellationToken cancellationToken) { - var (client, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (client, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var projectId = int.Parse(repository.ExternalId); var verdict = input.Verdict; var body = input.Body; @@ -971,7 +955,7 @@ public async Task SubmitReviewAsync(ProviderContext con // made OUTSIDE the resilience wrapper: it throws ALREADY-classified provider exceptions, which // the wrapper's duck-typed status-code mapper would otherwise double-wrap (and a 5xx re-retry). if (action == GitLabApprovalAction.Unapprove) - await UnapproveAsync(host, token, projectId, number, cancellationToken).ConfigureAwait(false); + await UnapproveAsync(host, credential, projectId, number, cancellationToken).ConfigureAwait(false); // approve → read the MR's current approval state first (GitLab's approve isn't idempotent). // Already-approved becomes a no-op; ineligible (author / low role) fails clearly outside the @@ -1038,21 +1022,19 @@ private async Task ReadApproveDecisionAsync(ProviderInsta /// NGitLab 11.7 wraps approve but NOT unapprove (only ResetApprovals, which nukes EVERYONE's /// approvals — wrong semantic), so this mirrors the raw-HTTP pattern used by the scope probe. /// - private async Task UnapproveAsync(string host, string token, int projectId, int iid, CancellationToken cancellationToken) + private async Task UnapproveAsync(string host, GitLabCredential credential, int projectId, int iid, CancellationToken cancellationToken) { var url = $"{host.TrimEnd('/')}/api/v4/projects/{projectId}/merge_requests/{iid}/unapprove"; using var request = new HttpRequestMessage(HttpMethod.Post, url); - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", token); - using var response = await _countsHttpClient.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, cancellationToken).ConfigureAwait(false); var responseBody = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); // The body becomes the failure's detail and scope hint, which reach external_call.failed and the node's error — // this raw call never passes the resilience translation that strikes an echo, so the token it presented is // struck here, before classification (which reads only the status and the scope tag). - var failure = ClassifyUnapproveResponse((int)response.StatusCode, CredentialNeedles.Strike(responseBody, [token])); + var failure = ClassifyUnapproveResponse((int)response.StatusCode, CredentialNeedles.Strike(responseBody, [credential.Token])); if (failure != null) throw failure; } @@ -1186,17 +1168,14 @@ public async Task ProbeCredentialAsync(ProviderContext co /// private async Task?> TryFetchTokenScopesAsync(ProviderContext context, CancellationToken cancellationToken) { - var auth = await _authResolver.ResolveAsync(context, cancellationToken).ConfigureAwait(false); - var host = string.IsNullOrWhiteSpace(context.Instance.ApiUrl) ? context.Instance.BaseUrl : context.Instance.ApiUrl; + var (host, credential) = await ResolveCredentialAsync(context, cancellationToken).ConfigureAwait(false); var url = $"{host.TrimEnd('/')}/api/v4/personal_access_tokens/self"; using var request = new HttpRequestMessage(HttpMethod.Get, url); - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {auth.Token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", auth.Token); try { - using var response = await _countsHttpClient.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, cancellationToken).ConfigureAwait(false); if (!response.IsSuccessStatusCode) return null; var json = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); @@ -1241,17 +1220,14 @@ public async Task GetActorAccessAsync(ProviderContext con // decided by the gate against the node's declared capability — not here. The token's SCOPE // (api/read_api) is a separate axis the gate checks too, so this stays "what role does this person // hold", independent of which write they do. - var auth = await _authResolver.ResolveAsync(context, cancellationToken).ConfigureAwait(false); - var host = string.IsNullOrWhiteSpace(context.Instance.ApiUrl) ? context.Instance.BaseUrl : context.Instance.ApiUrl; + var (host, credential) = await ResolveCredentialAsync(context, cancellationToken).ConfigureAwait(false); var url = $"{host.TrimEnd('/')}/api/v4/projects/{Uri.EscapeDataString(repository.ExternalId)}"; using var request = new HttpRequestMessage(HttpMethod.Get, url); - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {auth.Token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", auth.Token); try { - using var response = await _countsHttpClient.SendAsync(request, HttpCompletionOption.ResponseContentRead, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, cancellationToken).ConfigureAwait(false); Serilog.Log.Information("[preflight-gitlab] GET projects/{Id} → HTTP {Status}", repository.ExternalId, (int)response.StatusCode); // Can't even see the project → not a member / no access at all. @@ -1335,7 +1311,7 @@ public async Task GetActorAccessAsync(ProviderContext con public async Task FindWebhookByCallbackUrlAsync(ProviderContext context, RemoteRepository repository, string callbackUrl, CancellationToken cancellationToken) { - var (client, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (client, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var projectId = int.Parse(repository.ExternalId); try @@ -1352,7 +1328,7 @@ public async Task GetActorAccessAsync(ProviderContext con // Failing to LIST hooks fails the registration exactly as surely as failing to create // one, and for the same reasons (revoked token, no permission, instance down) — so the // operator gets the same evidence from either step. - throw new ProviderWebhookRegistrationException(DescribeHookFailure(ex, CaptureHookRequest("GET", host, projectId, token, null), token), ex); + throw new ProviderWebhookRegistrationException(DescribeHookFailure(ex, CaptureHookRequest("GET", host, projectId, credential, null), credential.Token), ex); } } @@ -1382,7 +1358,7 @@ public async Task GetActorAccessAsync(ProviderContext con public async Task RegisterWebhookAsync(ProviderContext context, RemoteRepository repository, WebhookRegistration request, CancellationToken cancellationToken) { - var (client, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (client, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var projectId = int.Parse(repository.ExternalId); var upsert = BuildHookUpsert(request); var payload = JsonSerializer.Serialize(upsert); @@ -1397,19 +1373,17 @@ public async Task RegisterWebhookAsync(ProviderContext context, R return await _resilience.ExecuteNonIdempotentAsync(context.Instance, nameof(RegisterWebhookAsync), async _ => { using var message = new HttpRequestMessage(HttpMethod.Post, url) { Content = new StringContent(payload, System.Text.Encoding.UTF8, "application/json") }; - message.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); - message.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", token); - using var response = await _countsHttpClient.SendAsync(message, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(message, credential, cancellationToken).ConfigureAwait(false); var body = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); if (!response.IsSuccessStatusCode) { // The refusal's body becomes last_error and the attempt's error, which any member can read: strike the token it may echo. - var refusal = CredentialNeedles.Strike(body, [token]); + var refusal = CredentialNeedles.Strike(body, [credential.Token]); throw new ProviderWebhookRegistrationException( - DescribeHookFailure(new HttpRequestException($"GitLab refused the hook with {(int)response.StatusCode}: {refusal}"), CaptureHookRequest("POST", host, projectId, token, payload), token), + DescribeHookFailure(new HttpRequestException($"GitLab refused the hook with {(int)response.StatusCode}: {refusal}"), CaptureHookRequest("POST", host, projectId, credential, payload), credential.Token), new HttpRequestException($"{(int)response.StatusCode}: {refusal}")); } @@ -1424,8 +1398,8 @@ public async Task RegisterWebhookAsync(ProviderContext context, R } catch (Exception ex) { - var sent = CaptureHookRequest("POST", host, projectId, token, payload); - throw new ProviderWebhookRegistrationException(DescribeHookFailure(ex, sent, token), ex); + var sent = CaptureHookRequest("POST", host, projectId, credential, payload); + throw new ProviderWebhookRegistrationException(DescribeHookFailure(ex, sent, credential.Token), ex); } } @@ -1456,18 +1430,18 @@ private static Dictionary BuildHookUpsert(WebhookRegistration re /// /// What we sent, for an operator to read months later. NGitLab hands back neither the URL it /// composed nor the payload it serialised, so both are reproduced here — verified against a - /// live instance: {host}/api/v4/projects/{id}/hooks, authenticated with a bearer token, - /// body serialised snake_case (which is why reproduces it exactly). + /// live instance: {host}/api/v4/projects/{id}/hooks, authenticated in the header the credential's type names + /// (), body serialised snake_case (which is why reproduces it exactly). /// - /// The credential is masked because Authorization is a credential header; the + /// The credential is masked because PRIVATE-TOKEN and Authorization are credential headers; the /// webhook secret is masked because it rides in a field named token. Two independent /// rules in , so neither secret depends on the other's rule. /// - private static CapturedProviderRequest CaptureHookRequest(string method, string host, int projectId, string token, string? body) + private static CapturedProviderRequest CaptureHookRequest(string method, string host, int projectId, GitLabCredential credential, string? body) { - var headers = new Dictionary { ["Authorization"] = $"Bearer {token}" }; + var headers = new Dictionary { [credential.HeaderName] = credential.HeaderValue }; - return ProviderCallCapture.CaptureRedacted(method, $"{host.TrimEnd('/')}/api/v4/projects/{projectId}/hooks", headers, body, new[] { token }); + return ProviderCallCapture.CaptureRedacted(method, $"{host.TrimEnd('/')}/api/v4/projects/{projectId}/hooks", headers, body, new[] { credential.Token }); } /// @@ -1607,37 +1581,35 @@ private static string ExtractFileName(string path) public async Task GetLatestCommitAsync(ProviderContext context, RemoteRepository repository, string? path, string? reference, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var gitRef = string.IsNullOrWhiteSpace(reference) ? repository.DefaultBranch : reference; var projectId = int.Parse(repository.ExternalId); return await _resilience.ExecuteAsync(context.Instance, nameof(GetLatestCommitAsync), async _ => - await FetchLatestCommitAsync(host, token, projectId, gitRef, (path ?? string.Empty).Trim('/'), cancellationToken).ConfigureAwait(false), + await FetchLatestCommitAsync(host, credential, projectId, gitRef, (path ?? string.Empty).Trim('/'), cancellationToken).ConfigureAwait(false), cancellationToken).ConfigureAwait(false); } public async Task> ListPathCommitsAsync(ProviderContext context, RemoteRepository repository, IReadOnlyList paths, string? reference, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var gitRef = string.IsNullOrWhiteSpace(reference) ? repository.DefaultBranch : reference; var projectId = int.Parse(repository.ExternalId); // One commit lookup per entry, bounded (cap 8) + best-effort (a path that fails is omitted). return await BoundedParallelMap.RunAsync(paths, 8, - (entryPath, ct) => FetchLatestCommitAsync(host, token, projectId, gitRef, entryPath.Trim('/'), ct), + (entryPath, ct) => FetchLatestCommitAsync(host, credential, projectId, gitRef, entryPath.Trim('/'), ct), cancellationToken).ConfigureAwait(false); } - private static async Task FetchLatestCommitAsync(string host, string token, int projectId, string gitRef, string path, CancellationToken cancellationToken) + private static async Task FetchLatestCommitAsync(string host, GitLabCredential credential, int projectId, string gitRef, string path, CancellationToken cancellationToken) { var url = $"{host.TrimEnd('/')}/api/v4/projects/{projectId}/repository/commits?per_page=1&ref_name={Uri.EscapeDataString(gitRef)}"; if (!string.IsNullOrEmpty(path)) url += $"&path={Uri.EscapeDataString(path)}"; using var request = new HttpRequestMessage(HttpMethod.Get, url); - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", token); - using var response = await _countsHttpClient.SendAsync(request, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, cancellationToken).ConfigureAwait(false); if (!response.IsSuccessStatusCode) return null; var json = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); @@ -1662,7 +1634,7 @@ private sealed record GitLabCommitJson(string? Id, string? ShortId, string? Titl public async Task GetStatsAsync(ProviderContext context, RemoteRepository repository, CancellationToken cancellationToken) { - var (client, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (client, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var projectId = int.Parse(repository.ExternalId); return await _resilience.ExecuteAsync(context.Instance, nameof(GetStatsAsync), async _ => @@ -1671,9 +1643,9 @@ public async Task GetStatsAsync(ProviderContext context, // stars/forks too. Branch/tag/release counts come from the X-Total header, best-effort. var project = await client.Projects.GetByIdAsync(projectId, new SingleProjectQuery { Statistics = true }).ConfigureAwait(false); - var branches = await CountResourceViaXTotalAsync(host, token, $"projects/{projectId}/repository/branches", cancellationToken).ConfigureAwait(false); - var tags = await CountResourceViaXTotalAsync(host, token, $"projects/{projectId}/repository/tags", cancellationToken).ConfigureAwait(false); - var releases = await CountResourceViaXTotalAsync(host, token, $"projects/{projectId}/releases", cancellationToken).ConfigureAwait(false); + var branches = await CountResourceViaXTotalAsync(host, credential, $"projects/{projectId}/repository/branches", cancellationToken).ConfigureAwait(false); + var tags = await CountResourceViaXTotalAsync(host, credential, $"projects/{projectId}/repository/tags", cancellationToken).ConfigureAwait(false); + var releases = await CountResourceViaXTotalAsync(host, credential, $"projects/{projectId}/releases", cancellationToken).ConfigureAwait(false); return new RemoteRepositoryStats { @@ -1795,7 +1767,7 @@ private static RemoteRelease ToRemoteReleaseDetail(ReleaseInfo r, RemoteReposito public async Task> GetLanguagesAsync(ProviderContext context, RemoteRepository repository, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); var projectId = int.Parse(repository.ExternalId); return await _resilience.ExecuteAsync(context.Instance, nameof(GetLanguagesAsync), async _ => @@ -1803,10 +1775,8 @@ public async Task> GetLanguagesAsync(ProviderConte // GitLab's /languages returns { "C#": 97.4, ... } — already percentages. Best-effort. var url = $"{host.TrimEnd('/')}/api/v4/projects/{projectId}/languages"; using var request = new HttpRequestMessage(HttpMethod.Get, url); - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", token); - using var response = await _countsHttpClient.SendAsync(request, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, cancellationToken).ConfigureAwait(false); if (!response.IsSuccessStatusCode) return (IReadOnlyList)Array.Empty(); var json = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); @@ -1816,16 +1786,14 @@ public async Task> GetLanguagesAsync(ProviderConte } /// Item count for a paginated GitLab list via the X-Total header (per_page=1). Best-effort → null on any failure. - private static async Task CountResourceViaXTotalAsync(string host, string token, string path, CancellationToken cancellationToken) + private static async Task CountResourceViaXTotalAsync(string host, GitLabCredential credential, string path, CancellationToken cancellationToken) { try { var url = $"{host.TrimEnd('/')}/api/v4/{path}?per_page=1"; using var request = new HttpRequestMessage(HttpMethod.Get, url); - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", token); - using var response = await _countsHttpClient.SendAsync(request, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, HttpCompletionOption.ResponseHeadersRead, cancellationToken).ConfigureAwait(false); if (!response.IsSuccessStatusCode) return null; if (response.Headers.TryGetValues("X-Total", out var values) && int.TryParse(values.FirstOrDefault(), out var total)) @@ -1843,7 +1811,7 @@ public async Task> GetLanguagesAsync(ProviderConte public async Task RenderMarkdownAsync(ProviderContext context, RemoteRepository repository, string markdown, CancellationToken cancellationToken) { - var (_, host, token) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); + var (_, host, credential) = await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false); return await _resilience.ExecuteAsync(context.Instance, nameof(RenderMarkdownAsync), async _ => { @@ -1852,10 +1820,8 @@ public async Task RenderMarkdownAsync(ProviderContext co var body = JsonSerializer.Serialize(new { text = markdown, gfm = true, project = repository.FullPath }); using var request = new HttpRequestMessage(HttpMethod.Post, url) { Content = new StringContent(body, System.Text.Encoding.UTF8, "application/json") }; - request.Headers.TryAddWithoutValidation("Authorization", $"Bearer {token}"); - request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", token); - using var response = await _countsHttpClient.SendAsync(request, cancellationToken).ConfigureAwait(false); + using var response = await GitLabHttp.SendAsync(request, credential, cancellationToken).ConfigureAwait(false); response.EnsureSuccessStatusCode(); var json = await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false); @@ -1869,14 +1835,22 @@ private sealed record GitLabMarkdownResponse(string? Html); private async Task BuildClientAsync(ProviderContext context, CancellationToken cancellationToken) => (await BuildAuthedAsync(context, cancellationToken).ConfigureAwait(false)).Client; - // Like BuildClientAsync but also surfaces the host + token for the raw /unapprove call NGitLab - // doesn't wrap. One auth resolve serves both (no double round-trip). - private async Task<(GitLabClient Client, string Host, string Token)> BuildAuthedAsync(ProviderContext context, CancellationToken cancellationToken) + // Like BuildClientAsync but also surfaces the host + credential for the raw calls NGitLab doesn't wrap. + // One auth resolve serves both (no double round-trip). + private async Task<(GitLabClient Client, string Host, GitLabCredential Credential)> BuildAuthedAsync(ProviderContext context, CancellationToken cancellationToken) + { + var (host, credential) = await ResolveCredentialAsync(context, cancellationToken).ConfigureAwait(false); + + return (GitLabHttp.NewClient(host, credential), host, credential); + } + + /// The host every call goes to (the API URL, else the base URL) and the credential to present there, in the header its type names. + private async Task<(string Host, GitLabCredential Credential)> ResolveCredentialAsync(ProviderContext context, CancellationToken cancellationToken) { var auth = await _authResolver.ResolveAsync(context, cancellationToken).ConfigureAwait(false); var host = string.IsNullOrWhiteSpace(context.Instance.ApiUrl) ? context.Instance.BaseUrl : context.Instance.ApiUrl; - return (new GitLabClient(host, auth.Token), host, auth.Token); + return (host, new GitLabCredential(auth.Token, context.Credential.AuthType)); } private static RemoteRepository ToRemoteRepository(Project project) => new() diff --git a/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRequestOptions.cs b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRequestOptions.cs new file mode 100644 index 000000000..e135d1973 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Providers/GitLab/GitLabRequestOptions.cs @@ -0,0 +1,147 @@ +using System.Net; +using CodeSpace.Core.Services.OutboundHttp; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; +using NGitLab; + +namespace CodeSpace.Core.Services.Providers.GitLab; + +/// +/// How NGitLab 11.7.0 sends a request, which it does on and exposes only through +/// . Two things are wrong with its own sending, and both are set right here, on the request it +/// hands over just before it is sent. +/// +/// The header. NGitLab writes Authorization: Bearer <token> whatever the token is; replaces it with the one header the credential's type names. +/// +/// The redirect. Left to a 301 is followed with the Authorization header dropped on +/// every hop, so the call arrives anonymous and fails with a bare 401. Auto-redirect is turned off and the hop is followed +/// here instead: a GET or HEAD whose redirect stays inside one origin () +/// is re-sent with the same headers, at most times. Anything else — a +/// redirect to another origin, to a request that changes something (NGitLab has written its body to a stream it does not hand +/// back, so it cannot be re-sent, and a create the first answer may already have landed must not be repeated), or one that does +/// not end — is not followed and fails with naming where GitLab pointed. +/// +/// The origin. NGitLab builds a request for whatever URL a paged answer names (Link: <url>; rel="next"), and +/// hands it over here like any other, so the credential would follow a page link to any origin the answer chose. A request +/// to another origin than the connection's is refused before the credential is attached, with the same +/// : nothing is sent there. +/// +/// Construct with (0, TimeSpan.Zero), what is: no retries of its own — the +/// provider's resilience wrapper retries — and its 5-minute timeout. +/// +internal sealed class GitLabRequestOptions : RequestOptions +{ + private readonly GitLabCredential _credential; + private readonly Uri? _connection; + + /// What every request presents, in the one header its type names. + /// The address the connection's calls go to; its origin is the only one a request may name. Not an absolute URL: NGitLab could not have built a request from it either, so none is checked. + public GitLabRequestOptions(GitLabCredential credential, string host) : base(0, TimeSpan.Zero) + { + _credential = credential; + _connection = Uri.TryCreate(host.Trim(), UriKind.Absolute, out var parsed) ? parsed : null; + } + + public override WebResponse GetResponse(HttpWebRequest request) + { + var current = Prepare(request); + + for (var hop = 0; ; hop++) + { + var response = (HttpWebResponse)current.GetResponse(); + + if (FollowTarget(current, response, hop) is not { } next) return response; + + response.Dispose(); + current = next; + } + } + + public override async Task GetResponseAsync(HttpWebRequest request, CancellationToken cancellationToken) + { + var current = Prepare(request); + + for (var hop = 0; ; hop++) + { + var response = await SendAsync(current, cancellationToken).ConfigureAwait(false); + + if (FollowTarget(current, response, hop) is not { } next) return response; + + response.Dispose(); + current = next; + } + } + + /// One request, aborted with its caller's token — as the base class sends it. + private static async Task SendAsync(HttpWebRequest request, CancellationToken cancellationToken) + { + using (cancellationToken.CanBeCanceled ? cancellationToken.Register(request.Abort) : default) + { + return (HttpWebResponse)await request.GetResponseAsync().ConfigureAwait(false); + } + } + + /// The request as it is sent: on the connection's origin, its redirects left to us, and the credential in the one header its type names. + private HttpWebRequest Prepare(HttpWebRequest request) + { + EnsureOnTheConnectionsOrigin(request.RequestUri); + + request.AllowAutoRedirect = false; + _credential.Attach(request.Headers); + + return request; + } + + private void EnsureOnTheConnectionsOrigin(Uri target) + { + if (_connection is null || OriginScopedRedirectHandler.IsSameOrigin(_connection, target)) return; + + throw new ProviderBaseUrlRedirectsException(ProviderKind.GitLab, _connection, target); + } + + /// + /// The request that follows , or null when it is not a redirect (it is the answer). A redirect + /// that is not followed throws rather than returning, so the 3xx never reaches NGitLab to be read as a body. + /// + private static HttpWebRequest? FollowTarget(HttpWebRequest sent, HttpWebResponse response, int hop) + { + if (!OriginScopedRedirectHandler.IsRedirect(response.StatusCode) || !Uri.TryCreate(sent.RequestUri, response.Headers["Location"], out var target)) return null; + + var followable = IsReadOnly(sent.Method) && OriginScopedRedirectHandler.IsSameOrigin(sent.RequestUri, target) && hop < OriginScopedRedirectHandler.MaxRedirects; + + if (!followable) + { + response.Dispose(); + + throw new ProviderBaseUrlRedirectsException(ProviderKind.GitLab, sent.RequestUri, target); + } + + return Follow(sent, target); + } + + private static bool IsReadOnly(string method) => method is "GET" or "HEAD"; + + /// + /// The request at carrying what NGitLab set on . + /// is obsolete, and it is the one transport NGitLab 11.7.0 speaks and hands to : the next hop + /// has to be the same kind of request, so the warning is silenced for this one line rather than the file. + /// + private static HttpWebRequest Follow(HttpWebRequest sent, Uri target) + { +#pragma warning disable SYSLIB0014 + var next = WebRequest.CreateHttp(target); +#pragma warning restore SYSLIB0014 + + next.Method = sent.Method; + next.Accept = sent.Accept; + next.Headers = sent.Headers; + next.AutomaticDecompression = sent.AutomaticDecompression; + next.Timeout = sent.Timeout; + next.ReadWriteTimeout = sent.ReadWriteTimeout; + next.Proxy = sent.Proxy; + next.AllowAutoRedirect = false; + + return next; + } +} diff --git a/backend/src/CodeSpace.Core/Services/Redaction/CredentialUrl.cs b/backend/src/CodeSpace.Core/Services/Redaction/CredentialUrl.cs index 9f4708486..92f936ba4 100644 --- a/backend/src/CodeSpace.Core/Services/Redaction/CredentialUrl.cs +++ b/backend/src/CodeSpace.Core/Services/Redaction/CredentialUrl.cs @@ -62,6 +62,19 @@ public static bool SameEndpoint(string? stored, string? next) return Uri.Compare(a, b, UriComponents.Scheme | UriComponents.Host | UriComponents.StrongPort, UriFormat.SafeUnescaped, StringComparison.OrdinalIgnoreCase) == 0; } + /// + /// Whether is the same endpoint as with its scheme moved from http to + /// https and nothing else: the same host (case aside) and the same port — each scheme's own default (80 then 443), or one + /// explicit port both spell. The one change of endpoint that sends a key nowhere new: to the same host, over TLS. Another + /// host (an alias included), another port, a downgrade and a non-http(s) value are all something else. + /// + public static bool IsHttpsUpgrade(string? stored, string? next) + { + if (HttpUri(stored) is not { Scheme: "http" } a || HttpUri(next) is not { Scheme: "https" } b) return false; + + return string.Equals(a.IdnHost, b.IdnHost, StringComparison.OrdinalIgnoreCase) && (a.IsDefaultPort ? b.IsDefaultPort : !b.IsDefaultPort && a.Port == b.Port); + } + /// The endpoint alone — scheme://host:port, never userinfo, path or query — for a log line about where a key is sent. public static string Endpoint(string? url) { diff --git a/backend/src/CodeSpace.Core/Services/Supervisor/SupervisorAcceptanceGrader.cs b/backend/src/CodeSpace.Core/Services/Supervisor/SupervisorAcceptanceGrader.cs index 1b7a0b527..e85e9b4de 100644 --- a/backend/src/CodeSpace.Core/Services/Supervisor/SupervisorAcceptanceGrader.cs +++ b/backend/src/CodeSpace.Core/Services/Supervisor/SupervisorAcceptanceGrader.cs @@ -363,7 +363,7 @@ private async Task CloneAtBaseAsync(WorkspaceRequest clone, string baseSha, stri TokenedGitCommand.Spec(remote, new SandboxSpec { Command = "git", Args = new[] { "clone", remote.Url, directory }, WorkingDirectory = directory, TimeoutSeconds = CloneTimeoutSeconds, AllowNetwork = true }), cancellationToken).ConfigureAwait(false); if (cloneResult.Status != SandboxStatus.Success) - throw TokenedGitCommand.OldGitFailure(remote.IsTokened, cloneResult.Stderr) ?? new WorkspaceException($"git clone failed (exit {cloneResult.ExitCode}): {LocalGitWorkspaceProvider.Redact(Summarize(cloneResult.Stderr), clone.Token)}"); + throw TokenedGitCommand.OldGitFailure(remote.IsTokened, cloneResult.Stderr) ?? GitRemoteRedirect.Failure(remote.Url, cloneResult.Stderr, $"git clone failed (exit {cloneResult.ExitCode}): {LocalGitWorkspaceProvider.Redact(Summarize(cloneResult.Stderr), clone.Token)}"); // Model-authored setup/acceptance commands run INSIDE this clone next. The clone named the remote without its // credential, so origin holds none; the strip stays as a belt, via the SAME shared helper LocalGitWorkspaceProvider's diff --git a/backend/src/CodeSpace.Messages/Exceptions/ProviderBaseUrlRedirectsException.cs b/backend/src/CodeSpace.Messages/Exceptions/ProviderBaseUrlRedirectsException.cs new file mode 100644 index 000000000..37ef47944 --- /dev/null +++ b/backend/src/CodeSpace.Messages/Exceptions/ProviderBaseUrlRedirectsException.cs @@ -0,0 +1,87 @@ +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Failures; + +namespace CodeSpace.Messages.Exceptions; + +/// +/// Thrown when a git provider's base URL answers a redirect that was not followed — above all one to another ORIGIN: a +/// different scheme, host or port (http to https, a host alias, a reverse proxy on another port, an external_url that +/// disagrees with the base URL). A redirect inside one origin is followed with the credential attached again; one that +/// changes origin is not, because a connected token is only ever sent to the origin it was connected for and the new origin +/// was named by whoever answered, not by the member who connected it. Left to a client's own redirect follower the credential +/// went missing (a typed client dropped its Authorization and arrived anonymous) or went along (a PRIVATE-TOKEN +/// kept on a hop to another host), and the failure read as a bare 401 that sent the owner to re-issue a token that was never +/// wrong. It names the URL the provider pointed at instead: the owner edits the base URL to it — an edit that only upgrades +/// http to https on the same host is allowed while tokens are connected — or removes the redirect. +/// +/// The same failure covers the two redirects inside one origin that are not followed either: one answered to a +/// request that changes something (a request is never sent again blindly, so a create the first answer may already have +/// landed is not repeated) and one that does not end. And it covers the next page of a list when the answer names another +/// origin for it (Link: rel="next"), which is as much the answering server's to write as a Location. +/// +/// git's own transport follows a redirect itself and then asks for a credential at the origin it was sent to, finding +/// none; the command's failure is named with this type too, built from the two origins alone +/// (), because git does not know which provider a remote belongs to. +/// +/// Both URLs are scheme, host, port and path alone: the redirect's Location is the redirecting server's to write, +/// and it may carry userinfo or a query string with a credential in it, which this message, its details and the logs never +/// repeat. +/// +public sealed class ProviderBaseUrlRedirectsException : Exception, IFailure +{ + public FailureKind Kind => FailureKind.Unprocessable; + + public string Code => FailureCodes.ProviderBaseUrlRedirects; + + public IReadOnlyDictionary? Details => ProviderKind is { } provider + ? new Dictionary { ["provider"] = provider.ToString(), ["requestedOrigin"] = RequestedOrigin, ["finalUrl"] = FinalUrl, ["finalOrigin"] = FinalOrigin } + : new Dictionary { ["requestedOrigin"] = RequestedOrigin, ["finalUrl"] = FinalUrl, ["finalOrigin"] = FinalOrigin }; + + /// The provider whose address redirected; null when the redirect was met by git's own transport, which does not know it. + public ProviderKind? ProviderKind { get; } + + /// Scheme, host and port (when not the scheme's own) of the URL the call was made to — the origin the credential was connected for. + public string RequestedOrigin { get; } + + /// Where the provider pointed: scheme, host, port and path, never userinfo, query or fragment. + public string FinalUrl { get; } + + /// Scheme, host and port of — what the provider's base URL would be edited to. + public string FinalOrigin { get; } + + public ProviderBaseUrlRedirectsException(ProviderKind providerKind, Uri requested, Uri redirectsTo) : this((ProviderKind?)providerKind, requested, redirectsTo) + { + } + + /// A redirect git met, for a remote whose provider is not known here: is the remote, the origin git was sent to and found no credential for. + public ProviderBaseUrlRedirectsException(Uri requested, Uri redirectsTo) : this(null, requested, redirectsTo) + { + } + + private ProviderBaseUrlRedirectsException(ProviderKind? providerKind, Uri requested, Uri redirectsTo) : base(BuildMessage(providerKind, requested, redirectsTo)) + { + ProviderKind = providerKind; + RequestedOrigin = OriginOf(requested); + FinalUrl = Sanitised(redirectsTo); + FinalOrigin = OriginOf(redirectsTo); + } + + private static string BuildMessage(ProviderKind? provider, Uri requested, Uri redirectsTo) + { + var host = provider?.ToString() ?? "the git host"; + var subject = char.ToUpperInvariant(host[0]) + host[1..]; + var edit = $"Edit the provider's base URL to the address {host} answers on ({OriginOf(redirectsTo)}), or remove the redirect on {host}'s side."; + + return OriginOf(requested) == OriginOf(redirectsTo) + ? $"{subject} answered a redirect from {Sanitised(requested)} to {Sanitised(redirectsTo)} that was not followed: the request changes something, or the redirects do not end, and a request is never sent again blindly. {edit}" + : $"{subject} answered a request to {OriginOf(requested)} with a redirect or link to {Sanitised(redirectsTo)}, which is another origin (scheme, host or port). A connected token is only ever sent to the origin it was connected for, so it was not followed. {edit} An edit that only upgrades http to https on the same host is allowed while tokens are connected."; + } + + /// scheme://host, or scheme://host:port when the port is not the scheme's own — never userinfo, path or query. + public static string OriginOf(Uri uri) => $"{uri.Scheme}://{HostOf(uri)}{(uri.IsDefaultPort ? "" : ":" + uri.Port)}"; + + /// The URL as scheme://host[:port]/path — no userinfo, query or fragment, which may carry a credential. + public static string Sanitised(Uri uri) => OriginOf(uri) + uri.AbsolutePath; + + private static string HostOf(Uri uri) => uri.HostNameType == UriHostNameType.IPv6 ? $"[{uri.IdnHost.Trim('[', ']')}]" : uri.IdnHost; +} diff --git a/backend/src/CodeSpace.Messages/Exceptions/ProviderEndpointUnreachableException.cs b/backend/src/CodeSpace.Messages/Exceptions/ProviderEndpointUnreachableException.cs new file mode 100644 index 000000000..a42804029 --- /dev/null +++ b/backend/src/CodeSpace.Messages/Exceptions/ProviderEndpointUnreachableException.cs @@ -0,0 +1,42 @@ +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Failures; + +namespace CodeSpace.Messages.Exceptions; + +/// +/// Thrown when an edit upgrades a git provider's address from http to https while tokens are connected through it, and the +/// new address does not answer HTTP from this server — a port that does not speak TLS, a certificate this server does not +/// trust, a host that is down. The upgrade is the one change of endpoint a connected token is allowed to follow, because it +/// sends the token nowhere new; but an address that cannot be reached breaks every connected token's calls, and putting it +/// back is itself a change of endpoint, refused while the tokens are connected. So the address is checked before it is kept, +/// and the stored one stays as it was. +/// +/// Names scheme, host and port alone: the address is whatever the owner typed and may carry userinfo or a query with +/// a credential in it, which this message, its details and the logs never repeat. +/// +public sealed class ProviderEndpointUnreachableException : Exception, IFailure +{ + public FailureKind Kind => FailureKind.Unprocessable; + + public string Code => FailureCodes.ProviderEndpointUnreachable; + + public IReadOnlyDictionary? Details => new Dictionary { ["provider"] = ProviderKind.ToString(), ["origin"] = Origin }; + + public ProviderKind ProviderKind { get; } + + /// Scheme, host and port (when not the scheme's own) of the address that did not answer. + public string Origin { get; } + + public ProviderEndpointUnreachableException(ProviderKind providerKind, Uri address, string reason) : base(BuildMessage(providerKind, address, reason)) + { + ProviderKind = providerKind; + Origin = ProviderBaseUrlRedirectsException.OriginOf(address); + } + + private static string BuildMessage(ProviderKind provider, Uri address, string reason) + { + var origin = ProviderBaseUrlRedirectsException.OriginOf(address); + + return $"{origin} did not answer from this server ({reason}), so the {provider} provider's address was not changed to it: tokens are connected through this provider, and an address that cannot be reached would break every one of them with no way back short of disconnecting them. Check that {origin} serves HTTPS with a certificate this server trusts, then save again."; + } +} diff --git a/backend/src/CodeSpace.Messages/Failures/FailureCodes.cs b/backend/src/CodeSpace.Messages/Failures/FailureCodes.cs index f83778220..3d4a47ce2 100644 --- a/backend/src/CodeSpace.Messages/Failures/FailureCodes.cs +++ b/backend/src/CodeSpace.Messages/Failures/FailureCodes.cs @@ -64,6 +64,12 @@ public static class FailureCodes /// An edit moved a git provider connection's endpoint (the scheme, host or port of its API or base URL) while credentials are still connected through it. Remedy: disconnect them first (members reconnect against the new host), or add the new host as its own provider; a connected token never follows a repoint. public const string ProviderEndpointCredentialsBound = "provider_endpoint_credentials_bound"; + /// A git provider's base URL answered a redirect to another origin (a different scheme, host or port). A credential is only ever sent to the origin it was connected for, so the redirect is not followed. Remedy: edit the provider's base URL to the address the host answers on (the error names it), or remove the redirect on the host; a retry as-is reads the same redirect. + public const string ProviderBaseUrlRedirects = "provider_base_url_redirects"; + + /// An edit upgraded a connected git provider's address from http to https and nothing answered HTTP at the new address (a port that does not speak TLS, a certificate this server does not trust, a host that is down). Remedy: make the new address serve HTTPS this server can reach and trust, then save again; the stored address is unchanged. + public const string ProviderEndpointUnreachable = "provider_endpoint_unreachable"; + /// An edit moved a git provider connection's endpoint without entering its stored OAuth client secret again. Remedy: send the secret (normally the new host's OAuth app's) with the new endpoint. public const string ProviderEndpointSecretRequired = "provider_endpoint_secret_required"; diff --git a/backend/tests/CodeSpace.E2ETests/Repositories/ListPagingPastTheCeilingE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Repositories/ListPagingPastTheCeilingE2ETests.cs index 18f38b343..11b2f5861 100644 --- a/backend/tests/CodeSpace.E2ETests/Repositories/ListPagingPastTheCeilingE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Repositories/ListPagingPastTheCeilingE2ETests.cs @@ -60,10 +60,13 @@ public async Task A_page_too_large_for_an_int_is_refused_and_not_answered_with_a await using var gitLab = LoopbackResponder.Start(OneRowPerList); var (viewer, team, repositoryId) = await SeedRepositoryAsync(gitLab.Port); + // Connecting the provider may already have asked GitLab (the endpoint probe on add); only what the refused list request asks counts. + var asked = gitLab.Requests.Count; + var response = await _tenants.SendAsync(HttpMethod.Get, $"/api/repositories/{repositoryId}/{list}?page=3000000000&perPage=100", viewer, team); response.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(response)); - gitLab.Requests.Count.ShouldBe(0, "a refused request reaches no provider"); + gitLab.Requests.Count.ShouldBe(asked, "a refused request reaches no provider"); } private async Task ListAsync(Guid viewer, Guid team, Guid repositoryId, string list, int page) diff --git a/backend/tests/CodeSpace.E2ETests/Repositories/ProviderBaseUrlRedirectE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Repositories/ProviderBaseUrlRedirectE2ETests.cs new file mode 100644 index 000000000..ff6df7a51 --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Repositories/ProviderBaseUrlRedirectE2ETests.cs @@ -0,0 +1,145 @@ +using System.Net; +using System.Net.Http.Json; +using System.Text.Json; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.E2ETests.Infrastructure; +using CodeSpace.IntegrationTests.Workflows.Infrastructure; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Enums; +using Microsoft.EntityFrameworkCore; +using Shouldly; + +namespace CodeSpace.E2ETests.Repositories; + +/// +/// A GitLab base URL that redirects to another origin — http to https, a host alias, another port. The credential is only +/// ever sent to the origin it was connected for, so the redirect is not followed; what the owner sees is the typed +/// provider_base_url_redirects failure naming the URL GitLab pointed at, not a 401 that reads as a revoked token and +/// sends them to re-issue one that was never wrong. And the owner meets it where they type the URL: adding a provider whose +/// address redirects stores nothing. +/// +/// Driven through the real HTTP surface (POST /api/provider-instances, GET /api/repositories/{id}/branches), the real +/// GitLab provider over NGitLab, the real endpoint probe and the real GlobalExceptionFilter, against two loopback hosts: the +/// GitLab that redirects, and the origin it points at — which must never be contacted. +/// +/// Fidelity: HIGH. Only GitLab is a stub, and both are real sockets the production client speaks HTTP to. +/// +[Trait("Category", "E2E")] +[Trait("Surface", "Http")] +public sealed class ProviderBaseUrlRedirectE2ETests : IClassFixture +{ + private readonly TenantHarness _tenants; + + public ProviderBaseUrlRedirectE2ETests(TaskLaunchApiFactory factory) { _tenants = new TenantHarness(factory); } + + [Theory] + [InlineData("http to https", "branches")] + [InlineData("host alias", "branches")] + [InlineData("other port", "tags")] + [InlineData("other port", "pull-requests")] + public async Task A_viewer_reads_where_gitlab_pointed_not_a_401_and_the_token_goes_nowhere_else(string shape, string page) + { + var token = $"glpat-REDIRECT-CANARY-{Guid.NewGuid():N}"; + await using var elsewhere = LoopbackResponder.Start(_ => LoopbackResponder.Ok("[]")); + var destination = DestinationOf(shape, elsewhere.Port); + var redirecting = false; + await using var gitLab = LoopbackResponder.Start(head => redirecting ? LoopbackResponder.Redirect(destination + "/api/v4/x") : LoopbackResponder.Ok("[]")); + var (viewer, _, team, instanceId, repositoryId) = await SeedConnectedRepositoryAsync($"http://127.0.0.1:{gitLab.Port}", token); + + redirecting = true; // the address answered when it was added; it redirects now, as a host does once its proxy or external_url is changed + var response = await _tenants.SendAsync(HttpMethod.Get, $"/api/repositories/{repositoryId}/{page}", viewer, team); + var body = await response.Content.ReadAsStringAsync(); + + response.StatusCode.ShouldBe(HttpStatusCode.UnprocessableEntity, body); + var failure = JsonDocument.Parse(body).RootElement; + failure.GetProperty("code").GetString().ShouldBe("provider_base_url_redirects", body); + body.ShouldContain(destination, Case.Sensitive, "the owner reads where GitLab pointed"); + body.ShouldNotContain(token, Case.Sensitive); + elsewhere.Accepted.ShouldBe(0, $"the redirect to {shape} was not followed, so nothing — no credential — went to {destination}"); + gitLab.Requests.ShouldContain(head => head.Contains(token), "fixture check: the token was presented to the origin it was connected for"); + (await _tenants.DbAsync(db => db.ProviderInstance.AsNoTracking().SingleAsync(p => p.Id == instanceId))).BaseUrl.ShouldBe($"http://127.0.0.1:{gitLab.Port}", "the stored base URL is never rewritten"); + } + + [Theory] + [InlineData("http to https")] + [InlineData("host alias")] + [InlineData("other port")] + public async Task Adding_a_provider_whose_address_redirects_is_refused_naming_where_it_points_and_stores_nothing(string shape) + { + await using var elsewhere = LoopbackResponder.Start(_ => LoopbackResponder.Ok("{}")); + var destination = DestinationOf(shape, elsewhere.Port); + await using var gitLab = LoopbackResponder.Start(_ => LoopbackResponder.Redirect(destination + "/api/v4/version")); + var owner = await _tenants.AccountAsync(); + var team = await _tenants.TeamAsync((owner, TeamRole.Owner)); + + var add = await _tenants.SendAsync(HttpMethod.Post, "/api/provider-instances", owner, team, new { provider = "GitLab", displayName = "gitlab", baseUrl = $"http://127.0.0.1:{gitLab.Port}" }); + var body = await add.Content.ReadAsStringAsync(); + + add.StatusCode.ShouldBe(HttpStatusCode.UnprocessableEntity, body); + JsonDocument.Parse(body).RootElement.GetProperty("code").GetString().ShouldBe("provider_base_url_redirects", body); + body.ShouldContain(destination, Case.Sensitive); + elsewhere.Accepted.ShouldBe(0, "the probe names the redirect, it never goes there"); + gitLab.Requests.ShouldHaveSingleItem().ShouldNotContain("PRIVATE-TOKEN", Case.Insensitive, "the probe sends no credential"); + (await _tenants.DbAsync(db => db.ProviderInstance.AsNoTracking().CountAsync(p => p.TeamId == team))).ShouldBe(0); + } + + [Fact] + public async Task An_https_upgrade_past_a_connected_token_is_refused_when_nothing_answers_over_tls_and_the_token_keeps_working() + { + // The self-service fix for a base URL that redirects to https is allowed past a connected token because it sends the + // token nowhere new. It is not allowed to land the token on an address nothing serves over TLS: undoing that is itself + // a refused move, so the owner would be stranded. The new address is asked first, and the stored one stays. + var token = $"glpat-UPGRADE-CANARY-{Guid.NewGuid():N}"; + await using var gitLab = LoopbackResponder.Start(_ => LoopbackResponder.Ok("[]")); // plain http on this port: a TLS handshake against it never completes + var (viewer, admin, team, instanceId, repositoryId) = await SeedConnectedRepositoryAsync($"http://127.0.0.1:{gitLab.Port}", token); + + var upgrade = await _tenants.SendAsync(HttpMethod.Patch, $"/api/provider-instances/{instanceId}", admin, team, new { baseUrl = $"https://127.0.0.1:{gitLab.Port}" }); + var body = await upgrade.Content.ReadAsStringAsync(); + + upgrade.StatusCode.ShouldBe(HttpStatusCode.UnprocessableEntity, body); + JsonDocument.Parse(body).RootElement.GetProperty("code").GetString().ShouldBe("provider_endpoint_unreachable", body); + body.ShouldContain($"https://127.0.0.1:{gitLab.Port}", Case.Sensitive, "the owner reads which address did not answer"); + body.ShouldNotContain(token, Case.Sensitive); + (await _tenants.DbAsync(db => db.ProviderInstance.AsNoTracking().SingleAsync(p => p.Id == instanceId))).BaseUrl.ShouldBe($"http://127.0.0.1:{gitLab.Port}", "the stored address is unchanged"); + + var browse = await _tenants.SendAsync(HttpMethod.Get, $"/api/repositories/{repositoryId}/branches", viewer, team); + + browse.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(browse)); + gitLab.Requests.ShouldContain(head => head.Contains(token), "fixture check: the connected token still reaches the address it was connected for"); + } + + private static string DestinationOf(string shape, int port) => shape switch + { + "http to https" => $"https://127.0.0.1:{port}", + "host alias" => $"http://localhost:{port}", + "other port" => $"http://127.0.0.1:{port}", + _ => throw new ArgumentOutOfRangeException(nameof(shape), shape, "an unknown redirect shape") + }; + + /// The Owner connects GitLab and adds a PAT through the API, as an operator would; the repository row is seeded directly so no bind round-trip to the stub is needed. + private async Task<(Guid Viewer, Guid Admin, Guid Team, Guid InstanceId, Guid RepositoryId)> SeedConnectedRepositoryAsync(string gitLabUrl, string token) + { + var owner = await _tenants.AccountAsync(); + var admin = await _tenants.AccountAsync(); + var viewer = await _tenants.AccountAsync(); + var team = await _tenants.TeamAsync((owner, TeamRole.Owner), (admin, TeamRole.Admin), (viewer, TeamRole.Viewer)); + var instanceId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/provider-instances", owner, team, new { provider = "GitLab", displayName = "gitlab", baseUrl = gitLabUrl })); + var credentialId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/credentials/pat", owner, team, new { providerInstanceId = instanceId, displayName = "owner pat", token })); + var repositoryId = Guid.NewGuid(); + + await _tenants.DbAsync(async db => + { + db.Repository.Add(new Repository { Id = repositoryId, TeamId = team, ProviderInstanceId = instanceId, CredentialId = credentialId, ExternalId = "42", NamespacePath = "acme", Name = "web", FullPath = "acme/web", DefaultBranch = "main", Visibility = RepositoryVisibility.Private, WebUrl = "http://127.0.0.1/acme/web", Status = RepositoryStatus.Active, CreatedBy = SystemUsers.SeederId, LastModifiedBy = SystemUsers.SeederId }); + return await db.SaveChangesAsync(); + }); + + return (viewer, admin, team, instanceId, repositoryId); + } + + private static async Task CreatedIdAsync(HttpResponseMessage response) + { + response.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(response)); + + return (await response.Content.ReadFromJsonAsync()).GetProperty("id").GetGuid(); + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Infrastructure/PostgresFixture.cs b/backend/tests/CodeSpace.IntegrationTests/Infrastructure/PostgresFixture.cs index 3c2d6bbb8..65a0a97ba 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Infrastructure/PostgresFixture.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Infrastructure/PostgresFixture.cs @@ -242,6 +242,13 @@ private static void RegisterTestAssemblyTypes(ContainerBuilder builder) builder.RegisterType().AsSelf().SingleInstance(); + // A provider add or edit asks its API address once whether it redirects to another origin. The suite's addresses + // exist only in tests, so the probe contacts nothing here: tests that care register an answer on this instance. + builder.RegisterType() + .As() + .AsSelf() + .SingleInstance(); + // In-memory store backing TestRepositoryProvider's remote webhook lookups. Singleton // per fixture (= per test DB) so tests within one fixture share state (e.g. assert // "registrar called exactly once" via RegisterCallCount across multiple operations) diff --git a/backend/tests/CodeSpace.IntegrationTests/Infrastructure/TestProviderEndpointProbe.cs b/backend/tests/CodeSpace.IntegrationTests/Infrastructure/TestProviderEndpointProbe.cs new file mode 100644 index 000000000..c9aae22ae --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Infrastructure/TestProviderEndpointProbe.cs @@ -0,0 +1,51 @@ +using System.Collections.Concurrent; +using CodeSpace.Core.Services.ProviderInstances; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; + +namespace CodeSpace.IntegrationTests.Infrastructure; + +/// +/// The suite's : it contacts nothing, so adding or editing a provider at an address +/// that only exists in a test (https://gitlab.acme.test) never waits on DNS or a socket. It records every address it +/// was asked about, and answers a redirect for the ones a test registered with — the same typed +/// refusal the real probe throws, and for the ones registered with the typed "does not answer" +/// refusal. One instance serves the fixture, so tests key their addresses by a name of their own. +/// +internal sealed class TestProviderEndpointProbe : IProviderEndpointProbe +{ + private readonly ConcurrentDictionary _redirects = new(StringComparer.Ordinal); + private readonly ConcurrentQueue _asked = new(); + private readonly ConcurrentQueue _askedToAnswer = new(); + private readonly ConcurrentDictionary _silent = new(StringComparer.Ordinal); + + /// Every address a provider add or edit asked about, in the order it was asked. + public IReadOnlyCollection Asked => _asked; + + /// Every address an https upgrade asked whether it answers, in the order it was asked. + public IReadOnlyCollection AskedToAnswer => _askedToAnswer; + + /// The address answers a redirect to when it is asked about. + public void RedirectsTo(string url, string target) => _redirects[url] = new Uri(target); + + /// The address never answers HTTP — a port that does not speak TLS, a host that is down. + public void NeverAnswers(string url) => _silent[url] = 0; + + public Task EnsureNoRedirectToAnotherOriginAsync(ProviderKind provider, string url, CancellationToken cancellationToken) + { + _asked.Enqueue(url); + + if (_redirects.TryGetValue(url, out var target)) throw new ProviderBaseUrlRedirectsException(provider, new Uri(url.TrimEnd('/') + "/api/v4/version"), target); + + return Task.CompletedTask; + } + + public Task EnsureAnswersAsync(ProviderKind provider, string url, CancellationToken cancellationToken) + { + _askedToAnswer.Enqueue(url); + + if (_silent.ContainsKey(url)) throw new ProviderEndpointUnreachableException(provider, new Uri(url), "HttpRequestException"); + + return Task.CompletedTask; + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/ProviderInstanceEndpointBindingFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/ProviderInstanceEndpointBindingFlowTests.cs index afc1c22c8..ccbf759b0 100644 --- a/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/ProviderInstanceEndpointBindingFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/ProviderInstanceEndpointBindingFlowTests.cs @@ -51,6 +51,98 @@ public async Task A_host_move_is_refused_while_a_token_is_connected_and_changes_ row.DisplayName.ShouldBe("GitLab", "a refused edit changes nothing, the rest of it included"); } + [Theory] + [InlineData("base", "http://gitlab.acme.test", "https://gitlab.acme.test")] + [InlineData("base", "http://gitlab.acme.test:8080", "https://gitlab.acme.test:8080")] + [InlineData("api", "http://gitlab.acme.test", "https://gitlab.acme.test")] + public async Task An_upgrade_of_http_to_https_on_the_same_host_is_allowed_while_a_token_is_connected(string field, string from, string to) + { + // The self-service fix for a base URL that redirects to https. Refused, it left the owner one way out — disconnecting + // every member's token, which strands every repository bound to one of them. + var (teamId, instanceId) = await SeedInstanceAsync(field == "base" ? from : Host, apiUrl: field == "api" ? from : null, withOAuth: true); + await SeedCredentialAsync(teamId, instanceId, CredentialStatus.Active); + + await UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, BaseUrl = field == "base" ? to : null, ApiUrl = field == "api" ? to : null, OauthClientSecret = "" }); + + var row = await LoadAsync(instanceId); + (field == "base" ? row.BaseUrl : row.ApiUrl).ShouldBe(to); + using var verify = _fixture.BeginScope(); + verify.Resolve().Decrypt(row.OauthClientSecretEnc!).ShouldBe("original-secret", "an upgrade of the same host needs no new client secret and replaces none"); + } + + [Theory] + [InlineData("base")] + [InlineData("api")] + public async Task An_upgrade_past_a_connected_token_is_saved_only_where_the_new_address_answers(string field) + { + // The carve-out sends a token nowhere new, but it can still land it on an address nothing serves over TLS — a host + // that does not do https, a certificate this server does not trust — and undoing that is itself an endpoint move, + // refused while the token is connected. So the new address must answer HTTP before the edit is kept. + var host = $"gitlab-{Guid.NewGuid():N}.acme.test"; + var (teamId, instanceId) = await SeedInstanceAsync(field == "base" ? $"http://{host}" : Host, apiUrl: field == "api" ? $"http://{host}" : null, withOAuth: false); + await SeedCredentialAsync(teamId, instanceId, CredentialStatus.Active); + Probe().NeverAnswers($"https://{host}"); + + var refusal = await Should.ThrowAsync(() => UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, BaseUrl = field == "base" ? $"https://{host}" : null, ApiUrl = field == "api" ? $"https://{host}" : null })); + + refusal.Origin.ShouldBe($"https://{host}"); + var row = await LoadAsync(instanceId); + (field == "base" ? row.BaseUrl : row.ApiUrl).ShouldBe($"http://{host}", "a refused edit changes nothing: the owner is never stranded on an address nothing answers at"); + } + + [Fact] + public async Task An_upgrade_past_a_connected_token_asks_whether_the_new_address_answers_and_is_kept_when_it_does() + { + var host = $"gitlab-{Guid.NewGuid():N}.acme.test"; + var (teamId, instanceId) = await SeedInstanceAsync($"http://{host}", apiUrl: null, withOAuth: false); + await SeedCredentialAsync(teamId, instanceId, CredentialStatus.Active); + + await UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, BaseUrl = $"https://{host}" }); + + Probe().AskedToAnswer.ShouldContain($"https://{host}"); + (await LoadAsync(instanceId)).BaseUrl.ShouldBe($"https://{host}"); + } + + [Fact] + public async Task An_upgrade_with_no_token_connected_is_not_asked_whether_it_answers_it_can_be_undone_freely() + { + var host = $"gitlab-{Guid.NewGuid():N}.acme.test"; + var (teamId, instanceId) = await SeedInstanceAsync($"http://{host}", apiUrl: null, withOAuth: false); + Probe().NeverAnswers($"https://{host}"); + + await UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, BaseUrl = $"https://{host}" }); + + Probe().AskedToAnswer.ShouldNotContain($"https://{host}", "with nothing bound a mistaken address is one more edit away, so nothing is asked"); + (await LoadAsync(instanceId)).BaseUrl.ShouldBe($"https://{host}"); + } + + [Fact] + public async Task An_edit_that_is_not_an_upgrade_is_not_asked_whether_it_answers() + { + var host = $"gitlab-{Guid.NewGuid():N}.acme.test"; + var (teamId, instanceId) = await SeedInstanceAsync($"https://{host}", apiUrl: null, withOAuth: false); + await SeedCredentialAsync(teamId, instanceId, CredentialStatus.Active); + + await UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, DisplayName = "renamed", BaseUrl = $"https://{host}/gitlab" }); + + Probe().AskedToAnswer.ShouldNotContain(a => a.Contains(host), "a rename, or a path on the same origin, relies on no carve-out"); + } + + [Theory] + [InlineData("http://gitlab.acme.test", "https://gitlab.acme.test:8443")] // another port + [InlineData("http://gitlab.acme.test:8080", "https://gitlab.acme.test")] + [InlineData("http://gitlab.acme.test", "https://gitlab-alias.acme.test")] // an alias host + [InlineData("https://gitlab.acme.test", "http://gitlab.acme.test")] // a downgrade + public async Task An_upgrade_that_changes_anything_else_is_still_refused_while_a_token_is_connected(string from, string to) + { + var (teamId, instanceId) = await SeedInstanceAsync(from, apiUrl: null, withOAuth: false); + await SeedCredentialAsync(teamId, instanceId, CredentialStatus.Active); + + await Should.ThrowAsync(() => UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, BaseUrl = to })); + + (await LoadAsync(instanceId)).BaseUrl.ShouldBe(from); + } + [Fact] public async Task A_disconnected_token_does_not_bind_the_host() { @@ -129,6 +221,13 @@ public async Task A_placeholder_left_in_an_edited_url_is_refused_rather_than_sto (await LoadAsync(instanceId)).BaseUrl.ShouldBe(baseUrl); } + private TestProviderEndpointProbe Probe() + { + using var scope = _fixture.BeginScope(); + + return scope.Resolve(); + } + private async Task UpdateAsync(Guid teamId, UpdateProviderInstanceCommand command) { using var scope = _fixture.BeginScopeAs(Guid.NewGuid(), teamId, Roles.Admin); diff --git a/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/ProviderInstanceRedirectProbeFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/ProviderInstanceRedirectProbeFlowTests.cs new file mode 100644 index 000000000..2d22b5aca --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/ProviderInstances/ProviderInstanceRedirectProbeFlowTests.cs @@ -0,0 +1,225 @@ +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.Messages.Commands.ProviderInstances; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; +using MediatR; +using Microsoft.EntityFrameworkCore; +using Shouldly; + +namespace CodeSpace.IntegrationTests.ProviderInstances; + +/// +/// A git provider's API address is asked once, when it is typed, whether it redirects to another origin — so the redirect is +/// named to the owner who typed it, with the URL it points at, rather than showing up later as a 401 on every call. An add +/// that redirects stores nothing; an edit that moves the address to one that redirects changes nothing. Existing +/// connections are never rewritten and never blocked: an edit that leaves the address as stored asks nothing, so a +/// connection whose stored URL already redirects keeps saving its other fields. A repoint the endpoint-binding rule refuses +/// is refused before anything is asked, so the host it named is never contacted. +/// +/// The probe is the suite's fake (), which records what it was asked; the real +/// probe's wire behaviour is pinned beside the class itself, against a loopback host. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public class ProviderInstanceRedirectProbeFlowTests +{ + private readonly PostgresFixture _fixture; + + public ProviderInstanceRedirectProbeFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + [Fact] + public async Task An_add_whose_address_redirects_to_another_origin_stores_nothing_and_names_the_origin() + { + var teamId = await SeedTeamAsync(); + var address = UniqueAddress("http"); + Probe().RedirectsTo(address, "https://gitlab.acme.test/api/v4/version"); + + var refusal = await Should.ThrowAsync(() => AddAsync(teamId, new AddProviderInstanceCommand { Provider = ProviderKind.GitLab, DisplayName = "GitLab", BaseUrl = address })); + + refusal.FinalOrigin.ShouldBe("https://gitlab.acme.test"); + refusal.Message.ShouldContain("https://gitlab.acme.test"); + (await CountAsync(teamId)).ShouldBe(0, "a refused add stores nothing"); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task An_add_that_does_not_redirect_is_stored_as_written_and_its_api_address_is_the_one_asked_about(bool withApiUrl) + { + var teamId = await SeedTeamAsync(); + var baseUrl = UniqueAddress("https"); + var apiUrl = withApiUrl ? UniqueAddress("https") + "/api-root" : null; + + var id = await AddAsync(teamId, new AddProviderInstanceCommand { Provider = ProviderKind.GitLab, DisplayName = "GitLab", BaseUrl = baseUrl + "/", ApiUrl = apiUrl }); + + var row = await LoadAsync(id); + row.BaseUrl.ShouldBe(baseUrl, "stored exactly as an add always stored it: trailing slash trimmed, nothing rewritten"); + row.ApiUrl.ShouldBe(apiUrl); + Probe().Asked.ShouldContain(apiUrl ?? baseUrl, "provider calls go to the API address when one is set, else the base URL"); + } + + [Fact] + public async Task A_duplicate_add_is_refused_before_anything_is_asked() + { + var teamId = await SeedTeamAsync(); + var address = UniqueAddress("https"); + await AddAsync(teamId, new AddProviderInstanceCommand { Provider = ProviderKind.GitLab, DisplayName = "GitLab", BaseUrl = address }); + var askedBefore = Probe().Asked.Count(a => a == address); + + await Should.ThrowAsync(() => AddAsync(teamId, new AddProviderInstanceCommand { Provider = ProviderKind.GitLab, DisplayName = "again", BaseUrl = address })); + + Probe().Asked.Count(a => a == address).ShouldBe(askedBefore, "the cheap local refusal comes first"); + } + + [Fact] + public async Task An_edit_that_moves_the_address_to_one_that_redirects_changes_nothing() + { + var (teamId, instanceId, stored) = await SeedInstanceAsync(); + var address = UniqueAddress("http"); + Probe().RedirectsTo(address, $"https://{new Uri(address).Host}:8443/api/v4/version"); + + var refusal = await Should.ThrowAsync(() => UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, DisplayName = "renamed", BaseUrl = address })); + + refusal.FinalOrigin.ShouldEndWith(":8443"); + var row = await LoadAsync(instanceId); + row.BaseUrl.ShouldBe(stored); + row.DisplayName.ShouldBe("GitLab", "a refused edit changes nothing, the rest of it included"); + } + + [Theory] + [InlineData("rename")] + [InlineData("same base url, trailing slash")] + [InlineData("same api url")] + public async Task An_edit_that_leaves_the_address_as_stored_asks_nothing_even_when_that_address_redirects(string edit) + { + // Existing connections are never blocked by this: a stored URL that already redirects keeps saving its other + // fields, and its calls name the redirect when they meet it. + var (teamId, instanceId, stored) = await SeedInstanceAsync(apiUrl: UniqueAddress("http") + "/api-root"); + var row = await LoadAsync(instanceId); + Probe().RedirectsTo(stored, "https://gitlab.acme.test/api/v4/version"); + Probe().RedirectsTo(row.ApiUrl!, "https://gitlab.acme.test/api/v4/version"); + var askedBefore = Probe().Asked.Count; + + await UpdateAsync(teamId, edit switch + { + "rename" => new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, DisplayName = "renamed" }, + "same base url, trailing slash" => new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, BaseUrl = stored + "/", DisplayName = "renamed" }, + _ => new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, ApiUrl = row.ApiUrl, DisplayName = "renamed" } + }); + + Probe().Asked.Count.ShouldBe(askedBefore); + (await LoadAsync(instanceId)).DisplayName.ShouldBe("renamed"); + (await LoadAsync(instanceId)).BaseUrl.ShouldBe(stored, "never rewritten"); + } + + [Fact] + public async Task A_repoint_refused_for_its_bound_token_is_refused_before_the_new_host_is_asked_about() + { + // The host an Admin names while a member's token is connected is never contacted at all — not even to ask. + var (teamId, instanceId, stored) = await SeedInstanceAsync(); + await SeedCredentialAsync(teamId, instanceId); + var attacker = UniqueAddress("https"); + + await Should.ThrowAsync(() => UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, BaseUrl = attacker })); + + Probe().Asked.ShouldNotContain(attacker); + (await LoadAsync(instanceId)).BaseUrl.ShouldBe(stored); + } + + [Fact] + public async Task An_allowed_https_upgrade_is_itself_asked_about_so_a_redirect_from_the_new_address_is_named_too() + { + var (teamId, instanceId, _) = await SeedInstanceAsync(baseUrl: UniqueAddress("http")); + await SeedCredentialAsync(teamId, instanceId); + var row = await LoadAsync(instanceId); + var upgraded = "https" + row.BaseUrl["http".Length..]; + Probe().RedirectsTo(upgraded, "https://elsewhere.acme.test/api/v4/version"); + + // Same host, so the endpoint-binding rule lets it through; the probe is the one that sees where it leads. + await Should.ThrowAsync(() => UpdateAsync(teamId, new UpdateProviderInstanceCommand { ProviderInstanceId = instanceId, BaseUrl = upgraded })); + + (await LoadAsync(instanceId)).BaseUrl.ShouldBe(row.BaseUrl); + } + + private TestProviderEndpointProbe Probe() + { + using var scope = _fixture.BeginScope(); + + return scope.Resolve(); + } + + /// An address of this test's own — the fake probe is one instance for the whole fixture, so tests key their answers by address. + private static string UniqueAddress(string scheme) => $"{scheme}://gitlab-{Guid.NewGuid():N}.acme.test"; + + private async Task AddAsync(Guid teamId, AddProviderInstanceCommand command) + { + using var scope = _fixture.BeginScopeAs(Guid.NewGuid(), teamId, Roles.Admin); + + return await scope.Resolve().Send(command); + } + + private async Task UpdateAsync(Guid teamId, UpdateProviderInstanceCommand command) + { + using var scope = _fixture.BeginScopeAs(Guid.NewGuid(), teamId, Roles.Admin); + + await scope.Resolve().Send(command); + } + + private async Task CountAsync(Guid teamId) + { + using var scope = _fixture.BeginScope(); + + return await scope.Resolve().ProviderInstance.AsNoTracking().CountAsync(p => p.TeamId == teamId && p.DeletedDate == null); + } + + private async Task LoadAsync(Guid instanceId) + { + using var scope = _fixture.BeginScope(); + + return await scope.Resolve().ProviderInstance.AsNoTracking().SingleAsync(p => p.Id == instanceId); + } + + private async Task SeedTeamAsync() + { + var (teamId, instanceId, _) = await SeedInstanceAsync(); + + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + db.ProviderInstance.Remove(await db.ProviderInstance.SingleAsync(p => p.Id == instanceId)); + await db.SaveChangesAsync(); + + return teamId; + } + + private async Task<(Guid TeamId, Guid InstanceId, string BaseUrl)> SeedInstanceAsync(string? baseUrl = null, string? apiUrl = null) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var suffix = Guid.NewGuid().ToString("N")[..8]; + var owner = new User { Id = Guid.NewGuid(), Email = $"owner-{suffix}@x", Name = "Owner" }; + var team = new Team { Id = Guid.NewGuid(), Slug = $"team-{suffix}", Name = "Team" }; + var instance = new ProviderInstance { Id = Guid.NewGuid(), TeamId = team.Id, Provider = ProviderKind.GitLab, DisplayName = "GitLab", BaseUrl = baseUrl ?? UniqueAddress("https"), ApiUrl = apiUrl }; + + db.User.Add(owner); + db.Team.Add(team); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = team.Id, UserId = owner.Id, Role = TeamRole.Owner }); + db.Project.Add(TestProjectSeed.BuildDefaultProject(team.Id, owner.Id)); + db.ProviderInstance.Add(instance); + await db.SaveChangesAsync(); + + return (team.Id, instance.Id, instance.BaseUrl); + } + + private async Task SeedCredentialAsync(Guid teamId, Guid instanceId) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + + db.Credential.Add(new Credential { Id = Guid.NewGuid(), TeamId = teamId, ProviderInstanceId = instanceId, AuthType = AuthType.Pat, DisplayName = "member pat", EncryptedPayload = "unused-by-this-test", Status = CredentialStatus.Active }); + await db.SaveChangesAsync(); + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Workflows/CloneUrlAuthorityFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Workflows/CloneUrlAuthorityFlowTests.cs new file mode 100644 index 000000000..4e74567db --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Workflows/CloneUrlAuthorityFlowTests.cs @@ -0,0 +1,166 @@ +using System.Net; +using System.Net.Sockets; +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Agents.Commands; +using CodeSpace.Core.Services.Agents.Workspace; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.IntegrationTests.Workflows.Infrastructure; +using CodeSpace.Messages.Agents; +using CodeSpace.Messages.Credentials; +using CodeSpace.Messages.Enums; +using Microsoft.Extensions.Logging; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Workflows; + +/// +/// The two places a repository becomes a clone request — the workspace resolver and the run-command service — against real +/// Postgres and the real provider-auth layer. When a GitLab repository is cloned WITH a credential from another origin than +/// its provider's base URL (GitLab's external_url disagrees with the base URL the owner typed, which is what +/// redirects), the resolve logs a Warning naming both origins; the request is unchanged — the clone URL is not rewritten, +/// the stored base URL is not touched. An anonymous clone, a clone from the base URL's own origin and a GitHub clone say nothing. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public class CloneUrlAuthorityFlowTests +{ + private const string BaseUrl = "http://gitlab.acme.test"; + + private readonly PostgresFixture _fixture; + + public CloneUrlAuthorityFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + [Fact] + public async Task The_resolver_warns_about_a_credentialed_clone_from_another_origin_and_leaves_the_request_as_it_was() + { + var teamId = await SeedTeamAsync(); + var repoId = await SeedRepoAsync(teamId, ProviderKind.GitLab, BaseUrl, "https://gitlab.acme.test/org/repo.git", token: "glpat_x"); + var logger = new CapturingLogger(); + + var request = await ResolveAsync(teamId, repoId, logger); + + request.RepositoryUrl.ShouldBe("https://gitlab.acme.test/org/repo.git", "never rewritten"); + request.Token.ShouldBe("glpat_x"); + var warning = logger.Warnings.ShouldHaveSingleItem(); + warning.ShouldContain("https://gitlab.acme.test:443"); + warning.ShouldContain("http://gitlab.acme.test:80"); + (await LoadBaseUrlAsync(repoId)).ShouldBe(BaseUrl, "the stored base URL is never rewritten"); + } + + [Theory] + [InlineData(ProviderKind.GitLab, "http://gitlab.acme.test/org/repo.git", "glpat_x")] // the base URL's own origin + [InlineData(ProviderKind.GitLab, "http://GITLAB.acme.test:80/org/repo.git", "glpat_x")] // another spelling of it + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test/org/repo.git", null)] // anonymous: a redirect harms nothing + [InlineData(ProviderKind.GitHub, "https://github.example.test/org/repo.git", "ghp_x")] // GitHub's hosts differ by design + public async Task The_resolver_says_nothing_when_there_is_nothing_to_say(ProviderKind provider, string cloneUrl, string? token) + { + var teamId = await SeedTeamAsync(); + var repoId = await SeedRepoAsync(teamId, provider, BaseUrl, cloneUrl, token); + var logger = new CapturingLogger(); + + await ResolveAsync(teamId, repoId, logger); + + logger.Warnings.ShouldBeEmpty(); + } + + [Fact] + public async Task The_run_command_service_warns_the_same_way_before_it_clones() + { + if (OperatingSystem.IsWindows()) return; + + var teamId = await SeedTeamAsync(); + var closed = ClosedLoopbackPort(); + var repoId = await SeedRepoAsync(teamId, ProviderKind.GitLab, $"http://127.0.0.1:{closed + 1}", $"http://127.0.0.1:{closed}/org/repo.git", token: "glpat_x"); + var logger = new CapturingLogger(); + + // The clone itself fails (nothing listens on the port), which is the shape of the failure the warning explains. + await Should.ThrowAsync(async () => + { + using var scope = _fixture.BeginScope(b => b.RegisterInstance>(logger)); + await scope.Resolve().RunAsync(new RunCommandRequest { RepositoryId = repoId, TeamId = teamId, Command = "true" }, CancellationToken.None); + }); + + var warning = logger.Warnings.ShouldHaveSingleItem(); + warning.ShouldContain($"http://127.0.0.1:{closed}"); + warning.ShouldContain($"http://127.0.0.1:{closed + 1}"); + } + + private async Task ResolveAsync(Guid teamId, Guid repoId, CapturingLogger logger) + { + using var scope = _fixture.BeginScope(b => b.RegisterInstance>(logger)); + + return (await scope.Resolve().ResolveByRepositoryIdAsync(repoId, teamId, CancellationToken.None))!; + } + + private async Task LoadBaseUrlAsync(Guid repoId) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + var repo = await db.Repository.FindAsync(repoId); + + return (await db.ProviderInstance.FindAsync(repo!.ProviderInstanceId))!.BaseUrl; + } + + private static int ClosedLoopbackPort() + { + var probe = new TcpListener(IPAddress.Loopback, 0); + probe.Start(); + var port = ((IPEndPoint)probe.LocalEndpoint).Port; + probe.Stop(); + + return port; + } + + private async Task SeedTeamAsync() => (await WorkflowsTestSeed.SeedTeamAsync(_fixture)).TeamId; + + private async Task SeedRepoAsync(Guid teamId, ProviderKind provider, string baseUrl, string cloneUrl, string? token) + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + + var instanceId = Guid.NewGuid(); + db.ProviderInstance.Add(new ProviderInstance { Id = instanceId, TeamId = teamId, Provider = provider, DisplayName = provider.ToString(), BaseUrl = baseUrl }); + + Guid? credentialId = null; + if (token is not null) + { + credentialId = Guid.NewGuid(); + db.Credential.Add(new Credential + { + Id = credentialId.Value, TeamId = teamId, ProviderInstanceId = instanceId, AuthType = AuthType.Pat, DisplayName = "cred", Status = CredentialStatus.Active, + EncryptedPayload = scope.Resolve().Encrypt(scope.Resolve().Serialize(new PatPayload { Token = token })), + }); + } + + var repoId = Guid.NewGuid(); + db.Repository.Add(new Repository + { + Id = repoId, TeamId = teamId, ProviderInstanceId = instanceId, CredentialId = credentialId, + ExternalId = repoId.ToString(), NamespacePath = "org", Name = "repo", FullPath = "org/repo", + DefaultBranch = "main", CloneUrlHttps = cloneUrl, WebUrl = $"{baseUrl}/org/repo", + }); + + await db.SaveChangesAsync(); + + return repoId; + } + + private sealed class CapturingLogger : ILogger + { + private readonly List _warnings = new(); + + public IReadOnlyList Warnings { get { lock (_warnings) return _warnings.ToList(); } } + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + if (logLevel == LogLevel.Warning) lock (_warnings) _warnings.Add(formatter(state, exception)); + } + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Workflows/GitPublishRemoteFixture.cs b/backend/tests/CodeSpace.IntegrationTests/Workflows/GitPublishRemoteFixture.cs index 1d2c5526b..b67f29065 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Workflows/GitPublishRemoteFixture.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Workflows/GitPublishRemoteFixture.cs @@ -36,6 +36,7 @@ internal sealed class GitPublishRemoteFixture : IAsyncDisposable private Task? _harvest; private string _harvesterAuthority = ""; private int _harvesterRequests; + private int _redirectsServed; private int _lfsBatchRequests; private int _lfsBatchRefusedCredentials; @@ -76,6 +77,22 @@ internal sealed class GitPublishRemoteFixture : IAsyncDisposable /// public bool RedirectsToHarvester { get; set; } + /// + /// When set, anything under is answered, before any authentication, with a 301 to the same path + /// under the remote's real URL — a redirect inside ONE authority, the shape of a repository that moved or a clone URL + /// without its .git. The credential still answers for that authority, so the operation must go through. + /// + public bool RedirectsWithinAuthority { get; set; } + + /// The remote's URL as the old address it redirects from once is set: the same authority, another path. + public string MovedUrl => Url.Replace("/remote.git", "/moved.git", StringComparison.Ordinal); + + /// Redirects served inside the authority — non-zero once git asked the old address. + public int RedirectsServed => Volatile.Read(ref _redirectsServed); + + /// The harvester's scheme and authority — the origin a redirect to it sends git to, which a failure naming the redirect must give. + public string HarvesterOrigin => _harvesterAuthority; + /// Requests the harvester served — non-zero once git followed the redirect to it. public int HarvesterRequests => Volatile.Read(ref _harvesterRequests); @@ -275,6 +292,7 @@ private async Task ServeAsync(HttpListenerContext context) var path = context.Request.Url!.AbsolutePath; + if (RedirectsWithinAuthority && path.StartsWith("/moved.git/", StringComparison.Ordinal)) { RedirectWithinAuthority(context, path); return; } if (path.EndsWith("/info/lfs/objects/batch", StringComparison.Ordinal)) { await ServeLfsBatchAsync(context); return; } if (path.Contains("/lfs-object/", StringComparison.Ordinal)) { await ServeLfsObjectAsync(context, path); return; } if (RedirectsToHarvester && IsRefAdvertisement(context, path)) { RedirectToHarvester(context, path); return; } @@ -304,6 +322,15 @@ private async Task ServeHarvesterAsync(HttpListenerContext context) finally { context.Response.Close(); } } + /// A 301 from 's path to the remote's own, the authority unchanged. + private void RedirectWithinAuthority(HttpListenerContext context, string path) + { + Interlocked.Increment(ref _redirectsServed); + + context.Response.StatusCode = 301; + context.Response.RedirectLocation = "/remote.git" + path["/moved.git".Length..] + context.Request.Url!.Query; + } + private static bool IsRefAdvertisement(HttpListenerContext context, string path) => context.Request.HttpMethod == "GET" && path.EndsWith("/info/refs", StringComparison.Ordinal); /// A 302 to the same path and query on the harvester's authority — the shape git needs to adopt it as the remote's new base. diff --git a/backend/tests/CodeSpace.IntegrationTests/Workflows/TokenedGitCredentialHelperFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Workflows/TokenedGitCredentialHelperFlowTests.cs index 32ac799be..9b0c61ef8 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Workflows/TokenedGitCredentialHelperFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Workflows/TokenedGitCredentialHelperFlowTests.cs @@ -10,6 +10,7 @@ using CodeSpace.Core.Services.Workflows.Artifacts; using CodeSpace.Messages.Agents; using CodeSpace.Messages.Agents.Benchmark; +using CodeSpace.Messages.Failures; using Microsoft.Extensions.Logging.Abstractions; using Shouldly; @@ -186,17 +187,24 @@ public async Task Integrating_over_lfs_history_downloads_through_origin_and_stor [InlineData("clone")] // the workspace clone, [InlineData("push")] // the publish push, [InlineData("integrate")] // and the integration clone, each redirected by the remote - public async Task A_redirect_to_another_authority_gets_no_credential(string operation) + public async Task A_redirect_to_another_authority_gets_no_credential_and_the_failure_names_it(string operation) { // git follows the redirect of the ref advertisement and makes the new authority the remote's base, so its next // request goes there. The credential answers for the remote's own scheme and authority only: when the harvester asks, - // no helper answers for it, and the operation fails rather than send the token anywhere. + // no helper answers for it, and the operation fails rather than send the token anywhere. git's own words for that + // are a prompt for a username; the failure says instead where the remote pointed — the typed redirect failure, still + // a WorkspaceException so every catch site that handles a git failure still does. if (OperatingSystem.IsWindows() || !await GitAvailableAsync()) return; await using var ctx = await OperatorHostContext.StartAsync(); ctx.Remote.StartHarvester(); - await Should.ThrowAsync(() => ctx.RedirectedAsync(operation)); + var failure = await Should.ThrowAsync(() => ctx.RedirectedAsync(operation)); + + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects", failure.Message); + ((IFailure)failure).Details!["finalOrigin"].ShouldBe(ctx.Remote.HarvesterOrigin); + failure.Message.ShouldContain(ctx.Remote.HarvesterOrigin); + failure.Message.ShouldNotContain(GitPublishRemoteFixture.FakeToken, Case.Sensitive); ctx.Remote.HarvesterRequests.ShouldBeGreaterThan(1, "fixture check: git followed the redirect and asked the harvester for more than the advertisement"); ctx.Remote.HarvestedAuthorizations.ShouldBeEmpty("a credential reached the authority the remote redirected to"); @@ -204,6 +212,34 @@ public async Task A_redirect_to_another_authority_gets_no_credential(string oper ctx.ShouldHoldTheToken(stored: false); } + [Theory] + [InlineData("clone")] // the workspace clone, + [InlineData("push")] // and the publish push from a clone made at the old address + public async Task A_redirect_inside_the_authority_keeps_the_credential(string operation) + { + // The other half of the redirect rule: the credential answers for the remote's scheme and authority, so a redirect to + // another PATH on the same authority (a moved repository, a clone URL without its .git) finds it. The remote demands + // the token for reads as well as writes, so an operation that gets through carried it past the redirect. + if (OperatingSystem.IsWindows() || !await GitAvailableAsync()) return; + + await using var ctx = await OperatorHostContext.StartAsync(); + ctx.Remote.RedirectsWithinAuthority = true; + + await using var handle = await ctx.ProvisionFromTheOldAddressAsync(); + + if (operation == "push") + { + await ctx.AgentCommitsAsync(handle.Directory); + (await ((IWorkspacePushHandle)handle).PushChangesAsync(ctx.BranchName, CancellationToken.None)).ShouldBe(ctx.BranchName); + ctx.Remote.AuthenticatedPushRequests.ShouldBeGreaterThan(0, "the push authenticated with the token after the redirect"); + } + + ctx.Remote.RedirectsServed.ShouldBeGreaterThan(0, "fixture check: git asked the old address and was redirected"); + File.ReadAllText(Path.Combine(handle.Directory, "README.md")).ShouldBe("base, revised\n", "the clone authenticated past the redirect and read the repository"); + ctx.ShouldKeepTheTokenOffEveryArgv(); + ctx.ShouldHoldTheToken(stored: false); + } + [Fact] public async Task The_harvester_collects_the_token_a_redirected_url_carries() { @@ -680,6 +716,10 @@ public async Task RewriteTheRemoteAsync(LoopbackSink sink) public void SeedTheOperatorsStore() => File.WriteAllText(GlobalStore, $"http://x-access-token:{GitPublishRemoteFixture.FakeToken}@{new Uri(Remote.Url).Authority}\n"); + /// A depth-1 clone of the remote named by its OLD address, which redirects inside the authority once is set. + public Task ProvisionFromTheOldAddressAsync() => + Provider.PrepareAsync(WorkspaceProvisionRequest.FromSingle(new WorkspaceRequest { RepositoryUrl = Remote.MovedUrl, Token = GitPublishRemoteFixture.FakeToken, TokenUsername = "x-access-token", Ref = "main", Depth = 1 }), CancellationToken.None); + /// A soft ref with a fallback (so the probe runs), a depth-1 clone, and a pin to the base's parent (so the fetch rungs run). public Task ProvisionAsync(string token) => Provider.PrepareAsync(WorkspaceProvisionRequest.FromSingle(new WorkspaceRequest diff --git a/backend/tests/CodeSpace.UnitTests/Agents/CloneUrlAuthorityTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/CloneUrlAuthorityTests.cs new file mode 100644 index 000000000..fe8012be3 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Agents/CloneUrlAuthorityTests.cs @@ -0,0 +1,74 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Agents.Workspace; +using CodeSpace.Messages.Enums; +using Microsoft.Extensions.Logging; +using Shouldly; + +namespace CodeSpace.UnitTests.Agents; + +/// +/// A GitLab repository cloned from another origin than its provider's base URL. git presents the credential to the clone +/// URL's origin only, so when the host redirects between the two origins the clone fails asking for credentials — while the +/// REST calls name the redirect. The resolve says so, once per clone and only when a credential is about to be presented, +/// naming both origins; it never changes the request. +/// +[Trait("Category", "Unit")] +public class CloneUrlAuthorityTests +{ + [Theory] + [InlineData(ProviderKind.GitLab, "http://gitlab.acme.test", "https://gitlab.acme.test/org/repo.git", true)] // http base URL, https clone URL + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test", "http://gitlab.acme.test/org/repo.git", true)] + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test", "https://gitlab.acme.test:8443/org/repo.git", true)] // another port + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test", "https://gitlab-alias.acme.test/org/repo.git", true)] // another host + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test", "https://gitlab.acme.test/org/repo.git", false)] + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test/", "https://GITLAB.acme.test:443/org/repo.git", false)] // case and the default port are one origin + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test/gitlab", "https://gitlab.acme.test/gitlab/org/repo.git", false)] + [InlineData(ProviderKind.GitLab, "https://svc:pw@gitlab.acme.test", "https://gitlab.acme.test/org/repo.git", false)] // userinfo is not part of an origin + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test", "file:///tmp/origin", false)] // not an http clone + [InlineData(ProviderKind.GitLab, "https://gitlab.acme.test", null, false)] + [InlineData(ProviderKind.GitLab, null, "https://gitlab.acme.test/org/repo.git", false)] + [InlineData(ProviderKind.GitHub, "https://github.example.test", "https://elsewhere.example.test/org/repo.git", false)] // GitHub's API and clone hosts differ by design + [InlineData(ProviderKind.Git, "https://local", "https://elsewhere.example.test/org/repo.git", false)] + public void A_clone_url_is_foreign_when_a_gitlab_base_url_and_it_name_different_origins(ProviderKind provider, string? baseUrl, string? cloneUrl, bool expected) => + CloneUrlAuthority.IsForeign(provider, baseUrl, cloneUrl).ShouldBe(expected); + + [Theory] + [InlineData(true, true, 1)] + [InlineData(false, true, 0)] // an anonymous clone follows a redirect without harm + [InlineData(true, false, 0)] + public void The_warning_is_logged_once_for_a_credentialed_foreign_clone_and_names_both_origins_only(bool credentialed, bool foreign, int expectedWarnings) + { + var logger = new CapturingLogger(); + var repository = new Repository + { + Id = Guid.NewGuid(), + CloneUrlHttps = foreign ? "https://svc:CANARY-PW@gitlab.acme.test:8443/org/repo.git?private_token=CANARY-Q" : "http://gitlab.acme.test/org/repo.git", + ProviderInstance = new ProviderInstance { Provider = ProviderKind.GitLab, BaseUrl = "http://gitlab.acme.test" } + }; + + CloneUrlAuthority.WarnWhenForeign(logger, repository, credentialed); + + logger.Warnings.Count.ShouldBe(expectedWarnings); + + if (expectedWarnings == 0) return; + + logger.Warnings[0].ShouldContain("https://gitlab.acme.test:8443"); + logger.Warnings[0].ShouldContain("http://gitlab.acme.test:80"); + logger.Warnings[0].ShouldContain(repository.Id.ToString()); + logger.Warnings[0].ShouldNotContain("CANARY", Case.Sensitive, "the clone URL's userinfo and query are never logged"); + } + + private sealed class CapturingLogger : ILogger + { + public List Warnings { get; } = new(); + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception? exception, Func formatter) + { + if (logLevel == LogLevel.Warning) Warnings.Add(formatter(state, exception)); + } + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Agents/DefaultRunnerKindTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/DefaultRunnerKindTests.cs index a48615d44..1e99bbb0b 100644 --- a/backend/tests/CodeSpace.UnitTests/Agents/DefaultRunnerKindTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Agents/DefaultRunnerKindTests.cs @@ -7,6 +7,7 @@ using CodeSpace.Core.Settings; using CodeSpace.Messages.Agents; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging.Abstractions; using Shouldly; namespace CodeSpace.UnitTests.Agents; @@ -73,7 +74,7 @@ public void The_configured_kind_arrives_through_the_environment_form_of_the_key( public async Task RunCommandService_resolves_the_deployment_default_only_when_the_request_pins_none(string? requestKind, string expectedKind) { var runners = new RecordingRunnerRegistry(); - var service = new RunCommandService(null!, null!, runners, null!, Setting("cfg-runner"), new CallerCommandLanes()); + var service = new RunCommandService(null!, null!, runners, null!, Setting("cfg-runner"), new CallerCommandLanes(), NullLogger.Instance); // Ephemeral (no repositoryId) so the DbContext / auth / workspace collaborators are never touched. await service.RunAsync(new RunCommandRequest { Command = "true", RunnerKind = requestKind }, CancellationToken.None); diff --git a/backend/tests/CodeSpace.UnitTests/Agents/RunCommandCallerPostureTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/RunCommandCallerPostureTests.cs index 6bbe25fd7..85586dbea 100644 --- a/backend/tests/CodeSpace.UnitTests/Agents/RunCommandCallerPostureTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Agents/RunCommandCallerPostureTests.cs @@ -6,6 +6,7 @@ using CodeSpace.Core.Settings; using CodeSpace.Messages.Agents; using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging.Abstractions; using Shouldly; namespace CodeSpace.UnitTests.Agents; @@ -178,7 +179,7 @@ public async Task Two_overlapping_commands_from_one_run_run_one_after_the_other( // Each command gets a cgroup leaf of its own, beside the agent's, carrying the run's whole tier row. Two // commands a run starts at once would each get a full row; queued, they never hold more than one between them. var runner = new OverlapRunner(expectedConcurrency: 2); - var service = new RunCommandService(null!, null!, new SingleRunnerRegistry(runner), null!, LocalDefault(), new CallerCommandLanes()); + var service = new RunCommandService(null!, null!, new SingleRunnerRegistry(runner), null!, LocalDefault(), new CallerCommandLanes(), NullLogger.Instance); var runId = Guid.NewGuid(); await Task.WhenAll(service.RunAsync(AgentAsks(PostureOf(AgentAutonomyLevel.Trusted) with { RunId = runId }), CancellationToken.None), service.RunAsync(AgentAsks(PostureOf(AgentAutonomyLevel.Trusted) with { RunId = runId }), CancellationToken.None)); @@ -191,7 +192,7 @@ public async Task Two_overlapping_commands_from_one_run_run_one_after_the_other( public async Task Commands_from_two_runs_and_workflow_node_commands_are_not_queued_behind_each_other() { var runner = new OverlapRunner(expectedConcurrency: 3); - var service = new RunCommandService(null!, null!, new SingleRunnerRegistry(runner), null!, LocalDefault(), new CallerCommandLanes()); + var service = new RunCommandService(null!, null!, new SingleRunnerRegistry(runner), null!, LocalDefault(), new CallerCommandLanes(), NullLogger.Instance); await Task.WhenAll( service.RunAsync(AgentAsks(PostureOf(AgentAutonomyLevel.Trusted) with { RunId = Guid.NewGuid() }), CancellationToken.None), diff --git a/backend/tests/CodeSpace.UnitTests/Agents/WorkspaceSpecTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/WorkspaceSpecTests.cs index fe46dde99..5d2b8bab2 100644 --- a/backend/tests/CodeSpace.UnitTests/Agents/WorkspaceSpecTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Agents/WorkspaceSpecTests.cs @@ -2,6 +2,7 @@ using CodeSpace.Core.Services.Agents; using CodeSpace.Core.Services.Agents.Workspace; using CodeSpace.Messages.Agents; +using Microsoft.Extensions.Logging.Abstractions; using Shouldly; namespace CodeSpace.UnitTests.Agents; @@ -338,7 +339,7 @@ public void CanonicalWorkspace_is_null_for_a_no_repo_run() [Fact] public async Task ResolveAsync_returns_null_for_a_no_repo_run() { - var resolver = new RepositoryWorkspaceResolver(db: Infrastructure.EmptyTestDb.New(), auth: null!); + var resolver = new RepositoryWorkspaceResolver(db: Infrastructure.EmptyTestDb.New(), auth: null!, logger: NullLogger.Instance); (await resolver.ResolveAsync(new AgentTask { Goal = "g", Harness = "claude-code" }, Guid.NewGuid(), CancellationToken.None)) .ShouldBeNull(); @@ -350,7 +351,7 @@ public async Task ResolveAsync_returns_null_for_a_no_repo_run() [Fact] public async Task ResolveAsync_refuses_an_empty_repositories_spec_with_a_distinct_error() { - var resolver = new RepositoryWorkspaceResolver(db: Infrastructure.EmptyTestDb.New(), auth: null!); + var resolver = new RepositoryWorkspaceResolver(db: Infrastructure.EmptyTestDb.New(), auth: null!, logger: NullLogger.Instance); var task = new AgentTask { Goal = "g", diff --git a/backend/tests/CodeSpace.UnitTests/Architecture/FailureTaxonomyTests.cs b/backend/tests/CodeSpace.UnitTests/Architecture/FailureTaxonomyTests.cs index e4ab727be..b0ba2e7ac 100644 --- a/backend/tests/CodeSpace.UnitTests/Architecture/FailureTaxonomyTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Architecture/FailureTaxonomyTests.cs @@ -78,6 +78,8 @@ public void The_wire_codes_are_pinned() FailureCodes.GitCredentialRejected.ShouldBe("git_credential_rejected"); FailureCodes.ModelEndpointKeyRequired.ShouldBe("model_endpoint_key_required"); FailureCodes.ProviderEndpointCredentialsBound.ShouldBe("provider_endpoint_credentials_bound"); + FailureCodes.ProviderBaseUrlRedirects.ShouldBe("provider_base_url_redirects"); + FailureCodes.ProviderEndpointUnreachable.ShouldBe("provider_endpoint_unreachable"); FailureCodes.ProviderEndpointSecretRequired.ShouldBe("provider_endpoint_secret_required"); FailureCodes.WorkflowDefinitionInvalid.ShouldBe("workflow_definition_invalid"); FailureCodes.TaskRouteConfirmationRequired.ShouldBe("task_route_confirmation_required"); diff --git a/backend/tests/CodeSpace.UnitTests/OutboundHttp/OriginScopedRedirectHandlerTests.cs b/backend/tests/CodeSpace.UnitTests/OutboundHttp/OriginScopedRedirectHandlerTests.cs index 034e21c27..6d6493178 100644 --- a/backend/tests/CodeSpace.UnitTests/OutboundHttp/OriginScopedRedirectHandlerTests.cs +++ b/backend/tests/CodeSpace.UnitTests/OutboundHttp/OriginScopedRedirectHandlerTests.cs @@ -117,9 +117,109 @@ public async Task A_response_that_is_not_a_redirect_is_returned_as_is() inner.Hops.Count.ShouldBe(1); } - private static async Task Send(ScriptedHandler inner, HttpRequestMessage request) + [Theory] + [InlineData(HttpStatusCode.MovedPermanently, "http://front.example.test/a", "http://other.example.test/b")] // host changes + [InlineData(HttpStatusCode.Found, "http://front.example.test/a", "http://front.example.test:8080/b")] // port changes + [InlineData(HttpStatusCode.Found, "http://front.example.test/a", "https://front.example.test/b")] // http to https on the same host + [InlineData(HttpStatusCode.TemporaryRedirect, "http://front.example.test/a", "http://other.example.test/b")] + [InlineData(HttpStatusCode.Found, "https://front.example.test/a", "http://front.example.test/b")] // a downgrade + public async Task A_caller_that_keeps_its_credential_to_one_origin_gets_a_cross_origin_redirect_back_unfollowed(HttpStatusCode status, string from, string to) + { + // A git provider's token belongs to the one origin it was connected for. Followed anonymously a cross-origin hop + // is a 401 that reads as a bad token; returned, the caller can name the URL — and nothing was sent there. + var inner = new ScriptedHandler(Redirect(status, to), Ok()); + + using var response = await Send(inner, AuthoredRequest(HttpMethod.Get, from), followsCrossOrigin: false); + + response.StatusCode.ShouldBe(status); + response.Headers.Location.ShouldBe(new Uri(to)); + inner.Hops.Count.ShouldBe(1, "no request was made to the other origin"); + } + + [Fact] + public async Task A_caller_that_keeps_its_credential_to_one_origin_still_follows_a_same_origin_redirect_with_every_header() + { + var inner = new ScriptedHandler(Redirect(HttpStatusCode.MovedPermanently, "/v2/items"), Redirect(HttpStatusCode.Found, "http://API.example.test:80/v3/items"), Ok()); + + using var response = await Send(inner, AuthoredRequest(HttpMethod.Get, "http://api.example.test/v1/items"), followsCrossOrigin: false); + + response.StatusCode.ShouldBe(HttpStatusCode.OK); + inner.Hops.Select(h => h.Uri.AbsolutePath).ShouldBe(new[] { "/v1/items", "/v2/items", "/v3/items" }, "a spelling of the same origin (case, the default port) is the same origin"); + + foreach (var hop in inner.Hops) hop.Headers.Keys.ShouldBe(new[] { "Authorization", "Cookie", "PRIVATE-TOKEN", "X-Api-Key" }, ignoreOrder: true); + } + + [Fact] + public async Task A_caller_that_keeps_its_credential_to_one_origin_re_sends_a_body_inside_the_origin() + { + var inner = new ScriptedHandler(Redirect(HttpStatusCode.TemporaryRedirect, "/next"), Ok()); + var request = AuthoredRequest(HttpMethod.Post, "http://api.example.test/start"); + request.Content = new StringContent("{\"a\":1}", Encoding.UTF8, "application/json"); + + using var response = await Send(inner, request, followsCrossOrigin: false); + + response.StatusCode.ShouldBe(HttpStatusCode.OK); + inner.Hops[1].Body.ShouldBe("{\"a\":1}"); + } + + [Theory] + [InlineData(HttpStatusCode.MovedPermanently, "POST")] + [InlineData(HttpStatusCode.Found, "POST")] + [InlineData(HttpStatusCode.SeeOther, "POST")] + [InlineData(HttpStatusCode.MultipleChoices, "POST")] + [InlineData(HttpStatusCode.SeeOther, "PUT")] + [InlineData(HttpStatusCode.Found, "PUT")] // a PUT is not turned into a GET, but the new request is the client's guess + [InlineData(HttpStatusCode.MovedPermanently, "DELETE")] + public async Task A_caller_whose_writes_are_answered_by_the_write_gets_a_rewriting_redirect_back_unfollowed(HttpStatusCode status, string method) + { + // A POST that a 301 or 302 turns into a GET would return that GET's answer as the write's: a markdown render, an + // unapprove, a hook create that "succeeded" because a list came back. Returned, the caller can say so. + var inner = new ScriptedHandler(Redirect(status, "/next"), Ok()); + var request = AuthoredRequest(new HttpMethod(method), "http://api.example.test/start"); + request.Content = new StringContent("{\"a\":1}", Encoding.UTF8, "application/json"); + + using var response = await Send(inner, request, repeatsWritesOnly: true); + + response.StatusCode.ShouldBe(status); + inner.Hops.Count.ShouldBe(1, "the write was sent once, and nothing was sent after it"); + } + + [Theory] + [InlineData(HttpStatusCode.TemporaryRedirect, "POST")] + [InlineData(HttpStatusCode.PermanentRedirect, "POST")] + [InlineData(HttpStatusCode.TemporaryRedirect, "PUT")] + [InlineData(HttpStatusCode.MovedPermanently, "GET")] + [InlineData(HttpStatusCode.Found, "HEAD")] + [InlineData(HttpStatusCode.SeeOther, "GET")] + public async Task A_caller_whose_writes_are_answered_by_the_write_still_follows_a_redirect_that_repeats_the_request(HttpStatusCode status, string method) + { + // 307 and 308 mean "send this same request there"; a read is the same read at any status. + var inner = new ScriptedHandler(Redirect(status, "/next"), Ok()); + var request = AuthoredRequest(new HttpMethod(method), "http://api.example.test/start"); + if (method is "POST" or "PUT") request.Content = new StringContent("{\"a\":1}", Encoding.UTF8, "application/json"); + + using var response = await Send(inner, request, repeatsWritesOnly: true); + + response.StatusCode.ShouldBe(HttpStatusCode.OK); + inner.Hops[1].Method.ShouldBe(method); + } + + [Fact] + public void By_default_a_write_may_be_rewritten_by_a_redirect() + { + new OriginScopedRedirectHandler().RepeatsWritesOnly.ShouldBeFalse(); + } + + [Fact] + public void By_default_a_cross_origin_redirect_is_followed() + { + // http.request has always followed them (dropping every header); the new mode is opt-in. + new OriginScopedRedirectHandler().FollowsCrossOrigin.ShouldBeTrue(); + } + + private static async Task Send(ScriptedHandler inner, HttpRequestMessage request, bool followsCrossOrigin = true, bool repeatsWritesOnly = false) { - using var invoker = new HttpMessageInvoker(new OriginScopedRedirectHandler { InnerHandler = inner }); + using var invoker = new HttpMessageInvoker(new OriginScopedRedirectHandler { InnerHandler = inner, FollowsCrossOrigin = followsCrossOrigin, RepeatsWritesOnly = repeatsWritesOnly }); return await invoker.SendAsync(request, CancellationToken.None); } diff --git a/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabCredentialHeaderTests.cs b/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabCredentialHeaderTests.cs new file mode 100644 index 000000000..c3eab5562 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabCredentialHeaderTests.cs @@ -0,0 +1,83 @@ +using CodeSpace.IntegrationTests.Webhooks; +using CodeSpace.Messages.Enums; +using Shouldly; +using static CodeSpace.IntegrationTests.Webhooks.StubProviderHost; +using static CodeSpace.UnitTests.Providers.GitLab.GitLabLoopback; + +namespace CodeSpace.UnitTests.Providers.GitLab; + +/// +/// Which header a GitLab credential travels in. GitLab 13.0 to 17.7 reads an Authorization: Bearer value of 41 or +/// more characters as an OAuth token alone, so a personal, project or group access token that long (a custom prefix, a +/// 66-character token) is refused as a Bearer however valid it is — it belongs in PRIVATE-TOKEN. An OAuth token +/// belongs in Authorization: Bearer. Every call sends the one header its credential type names and never both, +/// through both of NGitLab's request paths (the typed client, and the raw paged reader built from its internals) and +/// through each raw HTTP call the provider makes, against a loopback GitLab. +/// +[Trait("Category", "Unit")] +public sealed class GitLabCredentialHeaderTests : IDisposable +{ + private readonly StubProviderHost _gitlab = Start(); + + public void Dispose() => _gitlab.Dispose(); + + public static TheoryData EveryOperationForEveryCredentialType() + { + var data = new TheoryData(); + + foreach (var operation in Operations.Keys) + foreach (var type in new[] { AuthType.Pat, AuthType.ProjectAccessToken, AuthType.GroupAccessToken, AuthType.OAuth }) + data.Add(operation, type); + + return data; + } + + [Theory] + [MemberData(nameof(EveryOperationForEveryCredentialType))] + public async Task Every_request_carries_the_one_header_its_credential_type_names(string operation, AuthType type) + { + await TryAsync(() => Operations[operation](Provider(), Context(_gitlab.BaseUrl, type))); + + _gitlab.Requests.ShouldNotBeEmpty($"fixture check: '{operation}' reached GitLab"); + + foreach (var request in _gitlab.Requests) ShouldCarryOnly(request, type); + } + + [Theory] + [InlineData(AuthType.Pat)] + [InlineData(AuthType.ProjectAccessToken)] + [InlineData(AuthType.GroupAccessToken)] + public async Task A_credential_probe_sends_its_two_requests_the_same_way(AuthType type) + { + var probe = await Provider().ProbeCredentialAsync(Context(_gitlab.BaseUrl, type), CancellationToken.None); + + probe.IsValid.ShouldBeTrue(probe.Error); + _gitlab.Requests.Select(r => r.PathAndQuery).ShouldBe(new[] { "/api/v4/user", "/api/v4/personal_access_tokens/self" }, "the identity read (typed client), then the scope read (raw)"); + + foreach (var request in _gitlab.Requests) ShouldCarryOnly(request, type); + } + + [Theory] + [InlineData(AuthType.Pat)] + [InlineData(AuthType.ProjectAccessToken)] + [InlineData(AuthType.GroupAccessToken)] + public async Task A_long_access_token_is_not_read_as_an_oauth_token_by_the_legacy_finder(AuthType type) + { + // GitLab 13.0 to 17.7: an Authorization header of 41+ characters is looked up as an OAuth token alone, so this + // 66-character token is a 401 when sent as a Bearer. The typed client and a raw call both used to send one. + Token.Length.ShouldBeGreaterThan(41, "fixture check: the token is long enough to take the legacy finder's OAuth-only path"); + using var legacy = new StubProviderHost().Answer("GET", "/api/v4/", Legacy); + + (await Provider().GetByExternalIdAsync(Context(legacy.BaseUrl, type), "42", CancellationToken.None)).FullPath.ShouldBe("g/r", "the typed client's call"); + (await Provider().GetMemberStandingAsync(Context(legacy.BaseUrl, type), Repository, "7", CancellationToken.None)).ShouldBe(PullRequestAuthorAssociation.Member, "a raw call"); + } + + /// GitLab 13.0 to 17.7's finder: any Authorization header is an OAuth lookup that a PAT cannot pass; PRIVATE-TOKEN is the PAT lookup. + private static StubReply Legacy(RecordedRequest request) => request.AuthorizationHeader != null || request.PrivateTokenHeader != Token ? new StubReply(401, """{"message":"401 Unauthorized"}""") : Answer(request); + + private static async Task TryAsync(Func operation) + { + try { await operation(); } + catch (Exception) { /* what GitLab answers is not what this test reads — only the request it was sent */ } + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabCredentialTests.cs b/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabCredentialTests.cs new file mode 100644 index 000000000..f5f8722ec --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabCredentialTests.cs @@ -0,0 +1,84 @@ +using System.Net; +using CodeSpace.Core.Services.Providers.GitLab; +using CodeSpace.Messages.Enums; +using NGitLab; +using Shouldly; + +namespace CodeSpace.UnitTests.Providers.GitLab; + +/// +/// The credential value every GitLab call presents: which header its type names, that it replaces whatever credential header +/// a request already carries (NGitLab writes Authorization: Bearer before anything else sees the request), and that +/// formatting one never prints the token. The wire behaviour is pinned in . +/// +[Trait("Category", "Unit")] +public class GitLabCredentialTests +{ + private const string Token = "glpat-UNIT-CANARY-0123456789"; + + [Theory] + [InlineData(AuthType.Pat, "PRIVATE-TOKEN", Token)] + [InlineData(AuthType.ProjectAccessToken, "PRIVATE-TOKEN", Token)] + [InlineData(AuthType.GroupAccessToken, "PRIVATE-TOKEN", Token)] + [InlineData(AuthType.OAuth, "Authorization", "Bearer " + Token)] + public void The_credential_type_names_the_one_header_it_travels_in(AuthType type, string header, string value) + { + var credential = new GitLabCredential(Token, type); + + credential.HeaderName.ShouldBe(header); + credential.HeaderValue.ShouldBe(value); + } + + [Theory] + [InlineData(AuthType.Pat)] + [InlineData(AuthType.OAuth)] + public void Attaching_replaces_any_credential_header_the_request_already_carries(AuthType type) + { + using var request = new HttpRequestMessage(HttpMethod.Get, "http://gitlab.test/api/v4/user"); + request.Headers.TryAddWithoutValidation("Authorization", "Bearer stale"); + request.Headers.TryAddWithoutValidation("PRIVATE-TOKEN", "stale"); + + new GitLabCredential(Token, type).Attach(request); + + request.Headers.Select(h => h.Key).Order().ShouldBe(type == AuthType.OAuth ? new[] { "Authorization" } : new[] { "PRIVATE-TOKEN" }); + request.Headers.GetValues(type == AuthType.OAuth ? "Authorization" : "PRIVATE-TOKEN").ShouldHaveSingleItem().ShouldContain(Token); + } + + [Theory] + [InlineData(AuthType.Pat)] + [InlineData(AuthType.OAuth)] + public void Attaching_to_the_headers_NGitLab_wrote_leaves_exactly_one_credential_header(AuthType type) + { + var headers = new WebHeaderCollection { { HttpRequestHeader.Authorization, "Bearer " + Token }, { HttpRequestHeader.AcceptEncoding, "gzip" } }; + + new GitLabCredential(Token, type).Attach(headers); + + headers["Accept-Encoding"].ShouldBe("gzip", "what NGitLab set besides the credential is kept"); + (headers["Authorization"] is not null).ShouldBe(type == AuthType.OAuth); + (headers["PRIVATE-TOKEN"] is not null).ShouldBe(type != AuthType.OAuth); + } + + [Fact] + public void Formatting_a_credential_never_prints_the_token() + { + var credential = new GitLabCredential(Token, AuthType.Pat); + + credential.ToString().ShouldNotContain(Token); + $"{credential}".ShouldNotContain(Token); + } + + [Fact] + public void The_typed_clients_options_are_the_defaults_it_replaces() + { + // NGitLab's own default: no retries of its own (the resilience wrapper retries) and its five-minute timeout. + var ours = new GitLabRequestOptions(new GitLabCredential(Token, AuthType.Pat), "https://gitlab.acme.test"); + var theirs = RequestOptions.Default; + + ours.RetryCount.ShouldBe(theirs.RetryCount); + ours.RetryInterval.ShouldBe(theirs.RetryInterval); + ours.IsIncremental.ShouldBe(theirs.IsIncremental); + ours.HttpClientTimeout.ShouldBe(theirs.HttpClientTimeout); + ours.Proxy.ShouldBe(theirs.Proxy); + ours.UserAgent.ShouldBe(theirs.UserAgent); + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabLoopback.cs b/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabLoopback.cs new file mode 100644 index 000000000..f0e03fb20 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabLoopback.cs @@ -0,0 +1,104 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Providers; +using CodeSpace.Core.Services.Providers.Auth; +using CodeSpace.Core.Services.Providers.Errors; +using CodeSpace.Core.Services.Providers.Events; +using CodeSpace.Core.Services.Providers.GitLab; +using CodeSpace.Core.Services.Providers.Resilience; +using CodeSpace.IntegrationTests.Webhooks; +using CodeSpace.Messages.Dtos.Providers; +using CodeSpace.Messages.Enums; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; +using static CodeSpace.IntegrationTests.Webhooks.StubProviderHost; + +namespace CodeSpace.UnitTests.Providers.GitLab; + +/// +/// The real over a loopback GitLab that answers every route one call below can +/// reach, shared by the tests that read what GitLab is SENT (which header, which URL after a redirect) rather than what it +/// answers. The token is deliberately longer than the 41 characters GitLab 13.0 to 17.7 reads as an OAuth token alone. +/// +internal static class GitLabLoopback +{ + public const string Token = "glpat-LOOPBACK-MATRIX-0123456789abcdef0123456789abcdef0123456789"; + + public const string ProjectPath = "/api/v4/projects/42"; + + public static readonly RemoteRepository Repository = new() { ExternalId = "42", NamespacePath = "g", Name = "r", FullPath = "g/r", DefaultBranch = "main", Visibility = RepositoryVisibility.Private, WebUrl = "http://gitlab.test/g/r" }; + + public static readonly string ProjectJson = """{"id":42,"name":"r","path":"r","path_with_namespace":"g/r","namespace":{"id":7,"name":"g","path":"g","kind":"group","full_path":"g"},"default_branch":"main","visibility":"private","web_url":"http://gitlab.test/g/r","http_url_to_repo":"http://gitlab.test/g/r.git","archived":false}"""; + + /// A GitLab that answers each route a provider call below sends — and 501 for any other. + public static StubProviderHost Start() => new StubProviderHost().Answer("GET", "/api/v4/", Answer).Answer("POST", "/api/v4/", Answer); + + public static StubReply Answer(RecordedRequest request) + { + var path = request.PathAndQuery.Split('?')[0]; + + return (request.Method, path) switch + { + ("GET", ProjectPath) => new StubReply(200, ProjectJson), + ("GET", ProjectPath + "/members/all/7") => new StubReply(200, """{"id":7,"access_level":30}"""), + ("GET", ProjectPath + "/repository/commits") => new StubReply(200, "[]"), + ("GET", ProjectPath + "/languages") => new StubReply(200, "{}"), + ("GET", ProjectPath + "/issues") => new StubReply(200, "[]"), + ("GET", "/api/v4/user") => new StubReply(200, """{"id":7,"username":"nacho"}"""), + ("GET", "/api/v4/personal_access_tokens/self") => new StubReply(200, """{"scopes":["api"]}"""), + ("GET", "/api/v4/groups/acme%2Fplatform/hooks") => new StubReply(200, "[]"), + ("POST", ProjectPath + "/hooks") => new StubReply(201, """{"id":9}"""), + ("POST", "/api/v4/markdown") => new StubReply(200, """{"html":"

x

"}"""), + ("POST", ProjectPath + "/merge_requests/7/unapprove") => new StubReply(201, "{}"), + _ => new StubReply(501, """{"message":"no stub configured for this route"}""") + }; + } + + /// + /// Every provider call that reaches GitLab, by the path it takes: NGitLab's typed client, NGitLab's raw paged reader + /// (a second construction site), and each of the raw HTTP calls. The probe makes one of each. + /// + public static readonly IReadOnlyDictionary> Operations = new Dictionary> + { + ["ngitlab: project by id"] = (p, c) => p.GetByExternalIdAsync(c, "42", CancellationToken.None), + ["ngitlab: paged issue list"] = (p, c) => p.ListIssuesAsync(c, Repository, null, 1, 20, CancellationToken.None), + ["raw: member standing"] = (p, c) => p.GetMemberStandingAsync(c, Repository, "7", CancellationToken.None), + ["raw: latest commit"] = (p, c) => p.GetLatestCommitAsync(c, Repository, null, null, CancellationToken.None), + ["raw: languages"] = (p, c) => p.GetLanguagesAsync(c, Repository, CancellationToken.None), + ["raw: markdown render"] = (p, c) => p.RenderMarkdownAsync(c, Repository, "# hi", CancellationToken.None), + ["raw: actor access"] = (p, c) => p.GetActorAccessAsync(c, Repository, CancellationToken.None), + ["raw: group hook list"] = (p, c) => p.FindConnectionWebhookByCallbackUrlAsync(c, "acme/platform", "https://codespace.test/hook", CancellationToken.None), + ["raw: project hook create"] = (p, c) => p.RegisterWebhookAsync(c, Repository, new WebhookRegistration { CallbackUrl = "https://codespace.test/hook", Secret = "whsec", SubscribedEvents = ["Push Hook"] }, CancellationToken.None), + ["raw: unapprove"] = (p, c) => p.SubmitReviewAsync(c, Repository, 7, new SubmitPullRequestReviewInput { Verdict = PullRequestReviewVerdict.RequestChanges, Body = "no" }, CancellationToken.None), + }; + + /// What GitLab was sent for : an OAuth token as a Bearer alone, any access token as PRIVATE-TOKEN alone. + public static void ShouldCarryOnly(RecordedRequest request, AuthType type) + { + var call = $"{request.Method} {request.PathAndQuery}"; + + if (type == AuthType.OAuth) + { + request.AuthorizationHeader.ShouldBe($"Bearer {Token}", $"{call}: an OAuth token travels as a Bearer"); + request.PrivateTokenHeader.ShouldBeNull($"{call}: an OAuth token is never also a PRIVATE-TOKEN"); + return; + } + + request.PrivateTokenHeader.ShouldBe(Token, $"{call}: an access token travels as PRIVATE-TOKEN"); + request.AuthorizationHeader.ShouldBeNull($"{call}: an access token is never also a Bearer — GitLab 13.0 to 17.7 reads a long one as OAuth alone"); + } + + public static ProviderContext Context(string baseUrl, AuthType type) => new(new ProviderInstance { Id = Guid.NewGuid(), TeamId = Guid.NewGuid(), Provider = ProviderKind.GitLab, DisplayName = "loopback", BaseUrl = baseUrl }, new Credential { Id = Guid.NewGuid(), AuthType = type, DisplayName = "token", EncryptedPayload = "unused" }); + + public static GitLabRepositoryProvider Provider() + { + var resilience = new ExternalCallResilience(new ProviderErrorMapperRegistry(new IProviderErrorMapper[] { new GitLabErrorMapper() }), NullLogger.Instance); + var normalizer = new GitLabEventNormalizer(new ProviderEventSubscriptionRegistry(Array.Empty())); + + return new GitLabRepositoryProvider(new StaticTokenAuth(), resilience, new GitLabSignatureVerifier(), normalizer, new GitLabWebhookRepositoryIdentifier()); + } + + private sealed class StaticTokenAuth : IProviderAuthResolver + { + public Task ResolveAsync(ProviderContext context, CancellationToken cancellationToken) => Task.FromResult(new ResolvedAuth { Token = Token }); + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabRedirectTests.cs b/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabRedirectTests.cs new file mode 100644 index 000000000..643c43677 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Providers/GitLab/GitLabRedirectTests.cs @@ -0,0 +1,295 @@ +using CodeSpace.IntegrationTests.Webhooks; +using CodeSpace.Messages.Dtos.Providers; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; +using CodeSpace.Messages.Failures; +using CodeSpace.UnitTests.OutboundHttp; +using Shouldly; +using static CodeSpace.IntegrationTests.Webhooks.StubProviderHost; +using static CodeSpace.UnitTests.Providers.GitLab.GitLabLoopback; + +namespace CodeSpace.UnitTests.Providers.GitLab; + +/// +/// A GitLab base URL that redirects. The typed client (NGitLab, on HttpWebRequest) dropped its Authorization on a +/// 301 and arrived anonymous; a raw call dropped its Bearer but kept PRIVATE-TOKEN and carried it on to wherever the +/// redirect named — so a PAT worked, an OAuth token read 401 and looked like a random failure, and the PAT went to a host +/// the redirect picked. The provider now follows a redirect only inside one origin (scheme, host and port), with the +/// credential attached again on each hop; a redirect to another origin is not followed — no request is made to it — and +/// the call fails with , which names the URL GitLab pointed at so the owner +/// can edit the base URL. The stub redirects for http to https, a host alias and another port, as a proxy or an +/// external_url that disagrees with the base URL would. +/// +[Trait("Category", "Unit")] +public sealed class GitLabRedirectTests : IDisposable +{ + private readonly List _gitlabs = new(); + private readonly List _elsewhere = new(); + + public void Dispose() + { + foreach (var gitlab in _gitlabs) gitlab.Dispose(); + + foreach (var responder in _elsewhere) responder.DisposeAsync().AsTask().GetAwaiter().GetResult(); + } + + // ── Inside one origin: followed, credential attached again ── + + public static TheoryData SameOriginRedirects() + { + var data = new TheoryData(); + + foreach (var operation in new[] { "ngitlab: project by id", "ngitlab: paged issue list", "raw: member standing", "raw: latest commit", "raw: languages" }) + foreach (var type in new[] { AuthType.Pat, AuthType.OAuth }) + data.Add(operation, type); + + return data; + } + + [Theory] + [MemberData(nameof(SameOriginRedirects))] + public async Task A_redirect_inside_one_origin_is_followed_with_the_credential_attached_again(string operation, AuthType type) + { + var gitlab = RedirectingEveryGet(request => request.PathAndQuery + (request.PathAndQuery.Contains('?') ? "&" : "?") + "via=redirect"); + + await Operations[operation](Provider(), Context(gitlab.BaseUrl, type)); + + var hops = gitlab.Requests.Where(r => r.PathAndQuery.Contains("via=redirect")).ToList(); + hops.ShouldNotBeEmpty($"fixture check: '{operation}' followed the 301 to the same origin"); + gitlab.Requests.Count.ShouldBe(hops.Count * 2, "each call is one request that was redirected and one that followed"); + + foreach (var request in gitlab.Requests) ShouldCarryOnly(request, type); + } + + [Fact] + public async Task A_raw_post_redirected_inside_one_origin_is_sent_again_with_its_body_and_credential() + { + // 307 keeps the method and the body: a raw call's own content can be read twice, so it is re-sent. + var gitlab = Track(new StubProviderHost().Answer("POST", "/api/v4/", request => request.PathAndQuery.Contains("via=redirect") ? Answer(request) : Redirect(307, request.PathAndQuery + "?via=redirect"))); + + var rendered = await Provider().RenderMarkdownAsync(Context(gitlab.BaseUrl, AuthType.OAuth), Repository, "# hello", CancellationToken.None); + + rendered.Html.ShouldBe("

x

"); + gitlab.Requests.Select(r => r.Method).ShouldBe(new[] { "POST", "POST" }); + gitlab.Requests[1].Body.ShouldBe(gitlab.Requests[0].Body, "the redirected request carries the body again"); + gitlab.Requests[1].Body.ShouldContain("# hello"); + + foreach (var request in gitlab.Requests) ShouldCarryOnly(request, AuthType.OAuth); + } + + [Theory] + [InlineData(300, "markdown render", AuthType.Pat)] + [InlineData(301, "markdown render", AuthType.OAuth)] + [InlineData(302, "markdown render", AuthType.Pat)] + [InlineData(303, "markdown render", AuthType.OAuth)] + [InlineData(301, "unapprove", AuthType.Pat)] + [InlineData(302, "unapprove", AuthType.OAuth)] + [InlineData(302, "project hook create", AuthType.Pat)] + public async Task A_raw_write_that_a_redirect_inside_one_origin_would_turn_into_a_get_is_refused_not_read_as_the_writes_answer(int status, string call, AuthType type) + { + // A client that follows a 301 or 302 after a POST sends a GET and returns its answer: here the markdown render or the + // unapprove would "succeed" with whatever a read of the redirected path says. The redirect is named instead, and the + // write was sent once. + var gitlab = Track(new StubProviderHost().Answer("POST", "/api/v4/", request => Redirect(status, request.PathAndQuery + "?via=redirect")).Answer("GET", "/api/v4/", request => Answer(request))); + var context = Context(gitlab.BaseUrl, type); + var provider = Provider(); + + var failure = call switch + { + "markdown render" => await Should.ThrowAsync(() => provider.RenderMarkdownAsync(context, Repository, "# hello", CancellationToken.None)), + "unapprove" => await Should.ThrowAsync(() => provider.SubmitReviewAsync(context, Repository, 7, new SubmitPullRequestReviewInput { Verdict = PullRequestReviewVerdict.RequestChanges, Body = "no" }, CancellationToken.None)), + _ => (await Should.ThrowAsync(() => provider.RegisterWebhookAsync(context, Repository, new WebhookRegistration { CallbackUrl = "https://codespace.test/hook", Secret = "whsec", SubscribedEvents = ["Push Hook"] }, CancellationToken.None))).InnerException.ShouldBeOfType() + }; + + failure.RequestedOrigin.ShouldBe(gitlab.BaseUrl); + failure.FinalOrigin.ShouldBe(gitlab.BaseUrl, "inside one origin: the failure says it was not followed, not that it went elsewhere"); + failure.Message.ShouldContain("never sent again blindly"); + gitlab.Requests.Select(r => r.Method).ShouldBe(new[] { "POST" }, "the write was sent once and nothing followed it"); + } + + [Fact] + public async Task A_typed_client_write_redirected_inside_one_origin_is_refused_not_replayed() + { + // NGitLab has written its body to a stream it does not give back, so a write is never re-sent on a redirect: a + // second copy of a create that the first redirect may already have landed is worse than a refusal naming the URL. + var gitlab = Track(new StubProviderHost().Answer("POST", "/api/v4/", request => Redirect(307, request.PathAndQuery + "?via=redirect")).Answer("GET", "/api/v4/", _ => new StubReply(200, "[]"))); + + var failure = await Should.ThrowAsync(() => Provider().CreateIssueAsync(Context(gitlab.BaseUrl, AuthType.Pat), Repository, new CreateIssueInput { Title = "t", Body = "b" }, CancellationToken.None)); + + failure.FinalUrl.ShouldBe($"{gitlab.BaseUrl}{ProjectPath}/issues"); + gitlab.Requests.Count(r => r.Method == "POST").ShouldBe(1, "the create was sent once and never again"); + } + + // ── To another origin: not followed, nothing sent there, the URL named ── + + public static TheoryData CrossOriginRedirects() + { + var data = new TheoryData(); + + foreach (var operation in new[] { "ngitlab: project by id", "ngitlab: paged issue list", "raw: member standing", "raw: latest commit", "raw: markdown render" }) + foreach (var target in new[] { "http to https", "host alias", "other port" }) + foreach (var type in new[] { AuthType.Pat, AuthType.OAuth }) + data.Add(operation, target, type); + + return data; + } + + [Theory] + [MemberData(nameof(CrossOriginRedirects))] + public async Task A_redirect_to_another_origin_is_an_error_naming_it_and_nothing_is_sent_there(string operation, string target, AuthType type) + { + var elsewhere = Elsewhere(); + var origin = OriginOf(target, elsewhere.Port); + var gitlab = RedirectingEveryRoute(request => origin + request.PathAndQuery.Split('?')[0]); + + var failure = await Should.ThrowAsync(() => Bounded(() => Operations[operation](Provider(), Context(gitlab.BaseUrl, type)))); + + failure.FinalOrigin.ShouldBe(origin); + failure.FinalUrl.ShouldStartWith(origin + "/api/v4/"); + failure.Message.ShouldContain(origin, Case.Sensitive, "the owner reads where GitLab pointed"); + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects"); + ((IFailure)failure).Kind.ShouldBe(FailureKind.Unprocessable); + elsewhere.Accepted.ShouldBe(0, "no connection was opened to the other origin, so no credential could have reached it"); + gitlab.Requests.Count.ShouldBe(1, "a redirect is a verdict on the base URL, not a transient fault to retry"); + } + + [Theory] + [InlineData("http to https", AuthType.Pat)] + [InlineData("host alias", AuthType.OAuth)] + [InlineData("other port", AuthType.Pat)] + [InlineData("other port", AuthType.OAuth)] + public async Task A_paging_link_to_another_origin_is_an_error_naming_it_and_nothing_is_sent_there(string target, AuthType type) + { + // NGitLab follows `Link: ; rel="next"` by building a request for whatever URL the answer names: the response is + // the other origin's to write, so the credential must not follow it there however the page was reached. + var elsewhere = Elsewhere(); + var origin = OriginOf(target, elsewhere.Port); + var gitlab = Track(new StubProviderHost().Answer("GET", "/api/v4/", _ => PageWithNext("[]", $"{origin}{ProjectPath}/repository/branches?page=2"))); + + var failure = await Should.ThrowAsync(() => Bounded(() => Provider().ListBranchesAsync(Context(gitlab.BaseUrl, type), Repository, CancellationToken.None))); + + failure.FinalOrigin.ShouldBe(origin); + failure.FinalUrl.ShouldBe($"{origin}{ProjectPath}/repository/branches"); + failure.RequestedOrigin.ShouldBe(gitlab.BaseUrl); + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects"); + elsewhere.Accepted.ShouldBe(0, "no connection was opened to the origin the page link named, so no credential could have reached it"); + gitlab.Requests.Count.ShouldBe(1, "the first page was read once and the link was not taken"); + } + + [Theory] + [InlineData(AuthType.Pat)] + [InlineData(AuthType.OAuth)] + public async Task A_paging_link_inside_the_origin_is_followed_with_the_credential_attached_again(AuthType type) + { + var gitlab = Track(new StubProviderHost()); + gitlab.Answer("GET", "/api/v4/", request => request.PathAndQuery.Contains("page=2") + ? new StubReply(200, """[{"name":"two"}]""") + : PageWithNext("""[{"name":"one"}]""", $"{gitlab.BaseUrl}{ProjectPath}/repository/branches?page=2")); + + var branches = await Provider().ListBranchesAsync(Context(gitlab.BaseUrl, type), Repository, CancellationToken.None); + + branches.Select(b => b.Name).ShouldBe(new[] { "one", "two" }); + gitlab.Requests.Count.ShouldBe(2); + + foreach (var request in gitlab.Requests) ShouldCarryOnly(request, type); + } + + [Theory] + [InlineData("group hook create", AuthType.Pat)] + [InlineData("group hook create", AuthType.OAuth)] + [InlineData("project hook create", AuthType.Pat)] + [InlineData("project hook create", AuthType.OAuth)] + public async Task A_hook_registration_through_a_cross_origin_redirect_fails_with_the_redirect_as_its_failure_and_is_not_sent_again(string hook, AuthType type) + { + // The registrar persists this failure as the attempt's error: it must read as the redirect, and the create is sent + // once — a redirect is not the transient fault the non-idempotent wrapper retries after probing for a landed hook. + var elsewhere = Elsewhere(); + var origin = OriginOf("other port", elsewhere.Port); + var gitlab = RedirectingEveryRoute(request => origin + request.PathAndQuery.Split('?')[0]); + var registration = new WebhookRegistration { CallbackUrl = "https://codespace.test/hook", Secret = "whsec", SubscribedEvents = ["Push Hook"] }; + + var failure = hook == "group hook create" + ? await Should.ThrowAsync(() => Bounded(() => Provider().RegisterConnectionWebhookAsync(Context(gitlab.BaseUrl, type), "acme/platform", registration, CancellationToken.None))) + : (await Should.ThrowAsync(() => Bounded(() => Provider().RegisterWebhookAsync(Context(gitlab.BaseUrl, type), Repository, registration, CancellationToken.None)))).InnerException.ShouldBeOfType(); + + failure.FinalOrigin.ShouldBe(origin); + gitlab.Requests.Count(r => r.Method == "POST").ShouldBe(1, "the create was sent once"); + elsewhere.Accepted.ShouldBe(0); + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects"); + } + + [Theory] + [InlineData("http to https", AuthType.Pat)] + [InlineData("host alias", AuthType.OAuth)] + [InlineData("other port", AuthType.Pat)] + public async Task A_credential_probe_through_a_cross_origin_redirect_reports_where_gitlab_pointed(string target, AuthType type) + { + // "Use a token" validates through this probe; its answer is the dialog's message — a bare 401 before. + var elsewhere = Elsewhere(); + var origin = OriginOf(target, elsewhere.Port); + var gitlab = RedirectingEveryRoute(request => origin + request.PathAndQuery.Split('?')[0]); + + var probe = await Bounded(() => Provider().ProbeCredentialAsync(Context(gitlab.BaseUrl, type), CancellationToken.None)); + + probe.IsValid.ShouldBeFalse(); + probe.Error.ShouldNotBeNull().ShouldContain(origin + "/api/v4/user", Case.Sensitive); + elsewhere.Accepted.ShouldBe(0); + } + + [Theory] + [InlineData("ngitlab: project by id")] + [InlineData("raw: member standing")] + public async Task A_redirect_to_another_origin_does_not_leak_the_message_credentials_query_or_userinfo(string operation) + { + // The Location is the redirecting server's to write, and the message is shown to the owner and logged: the URL it + // names is scheme, host, port and path alone. + var gitlab = RedirectingEveryRoute(_ => "https://svc:hunter2@gitlab.example.test:8443/api/v4/projects/42?private_token=SECRET#frag"); + + var failure = await Should.ThrowAsync(() => Operations[operation](Provider(), Context(gitlab.BaseUrl, AuthType.Pat))); + + failure.FinalUrl.ShouldBe("https://gitlab.example.test:8443/api/v4/projects/42"); + failure.Message.ShouldNotContain("hunter2"); + failure.Message.ShouldNotContain("SECRET"); + } + + // ── Rig ── + + /// A call that follows a redirect to the https origin dials a listener that never answers a TLS handshake, and NGitLab's calls block their caller; run off the test thread and bounded, so a regression there fails fast instead of waiting out the client's own timeout. + private static async Task Bounded(Func> call) => await Task.Run(call).WaitAsync(TimeSpan.FromSeconds(15)); + + private static async Task Bounded(Func call) => await Task.Run(call).WaitAsync(TimeSpan.FromSeconds(15)); + + private StubProviderHost RedirectingEveryGet(Func location) => + Track(new StubProviderHost().Answer("GET", "/api/v4/", request => request.PathAndQuery.Contains("via=redirect") ? Answer(request) : Redirect(301, location(request)))); + + private StubProviderHost RedirectingEveryRoute(Func location) => + Track(new StubProviderHost().Answer("GET", "/api/v4/", request => Redirect(301, location(request))).Answer("POST", "/api/v4/", request => Redirect(301, location(request)))); + + private static StubReply PageWithNext(string json, string next) => new(200, json) { Headers = new Dictionary { ["Link"] = $"<{next}>; rel=\"next\"" } }; + + private static StubReply Redirect(int status, string location) => new(status, string.Empty) { Headers = new Dictionary { ["Location"] = location } }; + + private StubProviderHost Track(StubProviderHost gitlab) + { + _gitlabs.Add(gitlab); + + return gitlab; + } + + /// A listener on another port that answers 200 to anything — and counts the connections it is dialled on, which must stay at none. + private LoopbackResponder Elsewhere() + { + var responder = LoopbackResponder.Start(_ => LoopbackResponder.Ok("{}")); + _elsewhere.Add(responder); + + return responder; + } + + private static string OriginOf(string target, int port) => target switch + { + "http to https" => $"https://127.0.0.1:{port}", + "host alias" => $"http://localhost:{port}", + "other port" => $"http://127.0.0.1:{port}", + _ => throw new ArgumentOutOfRangeException(nameof(target), target, "an unknown redirect shape") + }; +} diff --git a/backend/tests/CodeSpace.UnitTests/Providers/ProviderBaseUrlRedirectsExceptionTests.cs b/backend/tests/CodeSpace.UnitTests/Providers/ProviderBaseUrlRedirectsExceptionTests.cs new file mode 100644 index 000000000..5c2f87793 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Providers/ProviderBaseUrlRedirectsExceptionTests.cs @@ -0,0 +1,92 @@ +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; +using CodeSpace.Messages.Failures; +using Shouldly; + +namespace CodeSpace.UnitTests.Providers; + +/// +/// The typed failure for a provider base URL that answers a redirect which is not followed: what it tells the owner to do, +/// and that the URLs it names are only ever scheme, host, port and path — the redirect's Location is the redirecting +/// server's to write, so a credential in its userinfo or query must never reach a message the owner reads or a log line. +/// +[Trait("Category", "Unit")] +public class ProviderBaseUrlRedirectsExceptionTests +{ + [Fact] + public void A_redirect_to_another_origin_is_an_unprocessable_failure_that_names_where_the_provider_pointed() + { + var failure = new ProviderBaseUrlRedirectsException(ProviderKind.GitLab, new Uri("http://gitlab.acme.test/api/v4/projects/42"), new Uri("https://gitlab.acme.test/api/v4/projects/42")); + + ((IFailure)failure).Kind.ShouldBe(FailureKind.Unprocessable); + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects"); + failure.RequestedOrigin.ShouldBe("http://gitlab.acme.test"); + failure.FinalOrigin.ShouldBe("https://gitlab.acme.test"); + failure.FinalUrl.ShouldBe("https://gitlab.acme.test/api/v4/projects/42"); + failure.Message.ShouldContain("Edit the provider's base URL to the address GitLab answers on (https://gitlab.acme.test)"); + failure.Message.ShouldContain("only upgrades http to https on the same host is allowed", Case.Sensitive, "the self-service fix is named"); + + var details = ((IFailure)failure).Details!; + details["provider"].ShouldBe("GitLab"); + details["finalUrl"].ShouldBe(failure.FinalUrl); + details["finalOrigin"].ShouldBe("https://gitlab.acme.test"); + details["requestedOrigin"].ShouldBe("http://gitlab.acme.test"); + } + + [Theory] + [InlineData("https://svc:hunter2@gitlab.acme.test:8443/api/v4/projects/42?private_token=SECRET#frag", "https://gitlab.acme.test:8443/api/v4/projects/42", "https://gitlab.acme.test:8443")] + [InlineData("https://gitlab.acme.test:443/a/b", "https://gitlab.acme.test/a/b", "https://gitlab.acme.test")] // a scheme's own port is not spelled + [InlineData("http://gitlab.acme.test:80/", "http://gitlab.acme.test/", "http://gitlab.acme.test")] + [InlineData("http://GITLAB.acme.test:8080/x", "http://gitlab.acme.test:8080/x", "http://gitlab.acme.test:8080")] // host case is not the owner's to match + [InlineData("http://[::1]:8080/x", "http://[::1]:8080/x", "http://[::1]:8080")] + public void The_url_it_names_is_scheme_host_port_and_path_alone(string location, string expectedUrl, string expectedOrigin) + { + var failure = new ProviderBaseUrlRedirectsException(ProviderKind.GitLab, new Uri("http://127.0.0.1:9/api/v4/user"), new Uri(location)); + + failure.FinalUrl.ShouldBe(expectedUrl); + failure.FinalOrigin.ShouldBe(expectedOrigin); + failure.Message.ShouldNotContain("hunter2"); + failure.Message.ShouldNotContain("SECRET"); + failure.Message.ShouldNotContain("frag"); + ((IFailure)failure).Details!.Values.OfType().ShouldAllBe(value => !value.Contains("hunter2") && !value.Contains("SECRET")); + } + + [Fact] + public void A_page_link_to_another_origin_reads_as_a_redirect_or_link_that_was_not_followed() + { + // NGitLab takes the next page of a list from a Link header, which is the answering server's to write like a Location. + var failure = new ProviderBaseUrlRedirectsException(ProviderKind.GitLab, new Uri("http://gitlab.acme.test"), new Uri("http://collector.test/api/v4/projects/42/repository/branches?page=2")); + + failure.Message.ShouldContain("redirect or link"); + failure.Message.ShouldContain("http://collector.test/api/v4/projects/42/repository/branches"); + failure.Message.ShouldNotContain("page=2"); + } + + [Fact] + public void A_redirect_the_git_transport_met_names_the_git_host_when_no_provider_is_known() + { + // git's own transport does not know which provider a remote belongs to: it knows the origin it was sent to and asked for + // a credential at. + var failure = new ProviderBaseUrlRedirectsException(new Uri("http://127.0.0.1:18102/g/r.git"), new Uri("https://127.0.0.1:18443")); + + failure.ProviderKind.ShouldBeNull(); + failure.RequestedOrigin.ShouldBe("http://127.0.0.1:18102"); + failure.FinalOrigin.ShouldBe("https://127.0.0.1:18443"); + failure.Message.ShouldStartWith("The git host answered"); + failure.Message.ShouldContain("Edit the provider's base URL to the address the git host answers on (https://127.0.0.1:18443)"); + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects"); + ((IFailure)failure).Details!.ContainsKey("provider").ShouldBeFalse("no provider is known, so none is claimed"); + ((IFailure)failure).Details!["finalOrigin"].ShouldBe("https://127.0.0.1:18443"); + } + + [Fact] + public void A_redirect_that_stays_inside_one_origin_is_named_as_one_that_was_not_followed_not_as_another_origin() + { + // Not followed all the same: a write is never sent again blindly, and a redirect that does not end is not a route. + var failure = new ProviderBaseUrlRedirectsException(ProviderKind.GitLab, new Uri("https://gitlab.acme.test/api/v4/projects/42/issues"), new Uri("https://gitlab.acme.test/other/issues")); + + failure.Message.ShouldContain("not followed"); + failure.Message.ShouldContain("never sent again blindly"); + failure.Message.ShouldNotContain("another origin"); + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Providers/ProviderEndpointProbeTests.cs b/backend/tests/CodeSpace.UnitTests/Providers/ProviderEndpointProbeTests.cs new file mode 100644 index 000000000..b5d81843a --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Providers/ProviderEndpointProbeTests.cs @@ -0,0 +1,255 @@ +using System.Net; +using System.Net.Sockets; +using CodeSpace.Core.Services.ProviderInstances; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; +using CodeSpace.UnitTests.OutboundHttp; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; + +namespace CodeSpace.UnitTests.Providers; + +/// +/// The real probe against loopback hosts: it asks a GitLab's API root, with no credential, whether the address redirects to +/// another origin — and says nothing else, ever. Anything that is not a redirect to another origin (any answer, a host that +/// is down, one that never answers, a URL that is not http(s), a provider with no API root to ask) leaves the address as the +/// owner wrote it. +/// +[Trait("Category", "Unit")] +public sealed class ProviderEndpointProbeTests : IAsyncDisposable +{ + private readonly List _hosts = new(); + + public async ValueTask DisposeAsync() + { + foreach (var host in _hosts) await host.DisposeAsync(); + } + + [Theory] + [InlineData(200)] + [InlineData(401)] // GitLab wants a token for /version: the answer is not read, only whether it redirects + [InlineData(404)] + [InlineData(500)] + public async Task An_answer_that_is_not_a_redirect_is_no_verdict_whatever_its_status(int status) + { + var gitlab = Host(_ => $"HTTP/1.1 {status} X\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"); + + await Probe().EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, $"http://127.0.0.1:{gitlab.Port}", CancellationToken.None); + + gitlab.Requests.ShouldHaveSingleItem().ShouldStartWith("GET /api/v4/version HTTP/1.1"); + } + + [Theory] + [InlineData("http to https", "https://127.0.0.1:{0}/api/v4/version")] + [InlineData("host alias", "http://localhost:{0}/api/v4/version")] + [InlineData("other port", "http://127.0.0.1:{0}/api/v4/version")] + public async Task A_redirect_to_another_origin_is_named_and_not_followed(string shape, string locationTemplate) + { + var elsewhere = Host(_ => LoopbackResponder.Ok("{}")); + var location = string.Format(locationTemplate, elsewhere.Port); + var gitlab = Host(_ => LoopbackResponder.Redirect(location, 301)); + + var failure = await Should.ThrowAsync(() => Probe().EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, $"http://127.0.0.1:{gitlab.Port}", CancellationToken.None)); + + failure.FinalUrl.ShouldBe(location, shape); + failure.RequestedOrigin.ShouldBe($"http://127.0.0.1:{gitlab.Port}"); + elsewhere.Accepted.ShouldBe(0, "the probe never goes where it was pointed"); + } + + [Fact] + public async Task A_redirect_inside_the_origin_is_followed_and_what_it_leads_to_is_the_verdict() + { + var elsewhere = Host(_ => LoopbackResponder.Ok("{}")); + var gitlab = Host(head => head.StartsWith("GET /api/v4/version ") ? LoopbackResponder.Redirect("/api/v4/version/", 301) : LoopbackResponder.Redirect($"http://127.0.0.1:{elsewhere.Port}/x", 302)); + + var failure = await Should.ThrowAsync(() => Probe().EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, $"http://127.0.0.1:{gitlab.Port}", CancellationToken.None)); + + failure.FinalUrl.ShouldBe($"http://127.0.0.1:{elsewhere.Port}/x"); + gitlab.Requests.Count.ShouldBe(2, "the path that normalises on the way was followed once"); + } + + [Fact] + public async Task A_redirect_inside_the_origin_that_ends_in_an_answer_is_no_verdict() + { + var gitlab = Host(head => head.StartsWith("GET /api/v4/version ") ? LoopbackResponder.Redirect("/api/v4/version/", 301) : LoopbackResponder.Ok("{}")); + + await Probe().EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, $"http://127.0.0.1:{gitlab.Port}", CancellationToken.None); + } + + [Fact] + public async Task The_path_of_a_relative_url_root_is_kept_and_the_query_userinfo_and_secrets_are_not_sent() + { + var gitlab = Host(_ => LoopbackResponder.Ok("{}")); + + await Probe().EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, $"http://svc:hunter2@127.0.0.1:{gitlab.Port}/gitlab/?private_token=SECRET#frag", CancellationToken.None); + + var head = gitlab.Requests.ShouldHaveSingleItem(); + head.ShouldStartWith("GET /gitlab/api/v4/version HTTP/1.1"); + head.ToLowerInvariant().ShouldNotContain("authorization"); + head.ToLowerInvariant().ShouldNotContain("private-token"); + head.ToLowerInvariant().ShouldNotContain("cookie"); + head.ShouldNotContain("hunter2"); + head.ShouldNotContain("SECRET"); + } + + [Fact] + public async Task A_host_that_is_down_is_no_verdict_and_answers_fast() + { + var closed = new TcpListener(IPAddress.Loopback, 0); + closed.Start(); + var port = ((IPEndPoint)closed.LocalEndpoint).Port; + closed.Stop(); + + await Probe().EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, $"http://127.0.0.1:{port}", CancellationToken.None).WaitAsync(TimeSpan.FromSeconds(15)); + } + + [Fact] + public async Task A_host_that_never_answers_is_no_verdict_after_the_timeout() + { + using var silent = new TcpListener(IPAddress.Loopback, 0); + silent.Start(); + var port = ((IPEndPoint)silent.LocalEndpoint).Port; + var probe = new ProviderEndpointProbe(NullLogger.Instance, TimeSpan.FromMilliseconds(400)); + + await probe.EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, $"http://127.0.0.1:{port}", CancellationToken.None).WaitAsync(TimeSpan.FromSeconds(15)); + } + + [Fact] + public async Task A_caller_that_cancels_is_not_swallowed() + { + using var silent = new TcpListener(IPAddress.Loopback, 0); + silent.Start(); + var port = ((IPEndPoint)silent.LocalEndpoint).Port; + using var cancel = new CancellationTokenSource(TimeSpan.FromMilliseconds(100)); + + await Should.ThrowAsync(() => Probe().EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, $"http://127.0.0.1:{port}", cancel.Token)); + } + + [Theory] + [InlineData("gitlab.example.com")] // no scheme + [InlineData("ftp://gitlab.example.com")] + [InlineData("")] + public async Task An_address_that_is_not_an_http_url_is_not_asked_about(string url) + { + await Probe().EnsureNoRedirectToAnotherOriginAsync(ProviderKind.GitLab, url, CancellationToken.None); + } + + [Theory] + [InlineData(ProviderKind.GitHub)] + public async Task A_provider_with_no_api_root_to_ask_is_not_asked(ProviderKind provider) + { + var host = Host(_ => LoopbackResponder.Redirect("https://elsewhere.example.test/", 301)); + + await Probe().EnsureNoRedirectToAnotherOriginAsync(provider, $"http://127.0.0.1:{host.Port}", CancellationToken.None); + + host.Accepted.ShouldBe(0); + } + + // ── Does the address answer HTTP at all — asked of an https upgrade while tokens are connected ── + + [Theory] + [InlineData(200)] + [InlineData(401)] // an answer is an answer: the route may want a token, the host is there and speaks HTTP + [InlineData(404)] + [InlineData(500)] + public async Task Any_http_answer_is_an_answer_whatever_its_status(int status) + { + var gitlab = Host(_ => $"HTTP/1.1 {status} X\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"); + + await Probe().EnsureAnswersAsync(ProviderKind.GitLab, $"http://127.0.0.1:{gitlab.Port}/gitlab", CancellationToken.None); + + gitlab.Requests.ShouldHaveSingleItem().ShouldStartWith("GET /gitlab HTTP/1.1"); + } + + [Fact] + public async Task A_redirect_to_another_origin_is_an_answer_and_is_not_followed() + { + var elsewhere = Host(_ => LoopbackResponder.Ok("{}")); + var gitlab = Host(_ => LoopbackResponder.Redirect($"http://127.0.0.1:{elsewhere.Port}/x", 301)); + + await Probe().EnsureAnswersAsync(ProviderKind.GitLab, $"http://127.0.0.1:{gitlab.Port}", CancellationToken.None); + + elsewhere.Accepted.ShouldBe(0, "where a redirect leads is the redirect probe's to say, not this one's"); + } + + [Fact] + public async Task An_https_address_that_speaks_plain_http_is_not_an_answer() + { + // The address a mistaken upgrade names: a host whose port does not do TLS. The handshake never completes. + var plain = Host(_ => LoopbackResponder.Ok("{}")); + var probe = new ProviderEndpointProbe(NullLogger.Instance, TimeSpan.FromMilliseconds(500)); + + var failure = await Should.ThrowAsync(() => probe.EnsureAnswersAsync(ProviderKind.GitLab, $"https://127.0.0.1:{plain.Port}", CancellationToken.None).WaitAsync(TimeSpan.FromSeconds(15))); + + failure.Origin.ShouldBe($"https://127.0.0.1:{plain.Port}"); + failure.ProviderKind.ShouldBe(ProviderKind.GitLab); + } + + [Fact] + public async Task A_host_that_is_down_is_not_an_answer() + { + var closed = new TcpListener(IPAddress.Loopback, 0); + closed.Start(); + var port = ((IPEndPoint)closed.LocalEndpoint).Port; + closed.Stop(); + + await Should.ThrowAsync(() => Probe().EnsureAnswersAsync(ProviderKind.GitLab, $"https://127.0.0.1:{port}", CancellationToken.None).WaitAsync(TimeSpan.FromSeconds(15))); + } + + [Fact] + public async Task A_host_that_never_answers_is_not_an_answer_after_the_timeout() + { + using var silent = new TcpListener(IPAddress.Loopback, 0); + silent.Start(); + var port = ((IPEndPoint)silent.LocalEndpoint).Port; + var probe = new ProviderEndpointProbe(NullLogger.Instance, TimeSpan.FromMilliseconds(400)); + + await Should.ThrowAsync(() => probe.EnsureAnswersAsync(ProviderKind.GitLab, $"http://127.0.0.1:{port}", CancellationToken.None).WaitAsync(TimeSpan.FromSeconds(15))); + } + + [Fact] + public async Task A_caller_that_cancels_an_answer_check_is_not_swallowed() + { + using var silent = new TcpListener(IPAddress.Loopback, 0); + silent.Start(); + var port = ((IPEndPoint)silent.LocalEndpoint).Port; + using var cancel = new CancellationTokenSource(TimeSpan.FromMilliseconds(100)); + + await Should.ThrowAsync(() => Probe().EnsureAnswersAsync(ProviderKind.GitLab, $"http://127.0.0.1:{port}", cancel.Token)); + } + + [Fact] + public async Task The_answer_check_sends_no_credential_userinfo_query_or_cookie() + { + var gitlab = Host(_ => LoopbackResponder.Ok("{}")); + + await Probe().EnsureAnswersAsync(ProviderKind.GitHub, $"http://svc:hunter2@127.0.0.1:{gitlab.Port}/gitlab/?private_token=SECRET#frag", CancellationToken.None); + + var head = gitlab.Requests.ShouldHaveSingleItem(); + head.ShouldStartWith("GET /gitlab HTTP/1.1"); + head.ToLowerInvariant().ShouldNotContain("authorization"); + head.ToLowerInvariant().ShouldNotContain("private-token"); + head.ToLowerInvariant().ShouldNotContain("cookie"); + head.ShouldNotContain("hunter2"); + head.ShouldNotContain("SECRET"); + } + + [Theory] + [InlineData("gitlab.example.com")] // no scheme + [InlineData("ftp://gitlab.example.com")] + [InlineData("")] + public async Task An_address_that_is_not_an_http_url_has_nothing_to_answer(string url) + { + await Probe().EnsureAnswersAsync(ProviderKind.GitLab, url, CancellationToken.None); + } + + private static ProviderEndpointProbe Probe() => new(NullLogger.Instance); + + private LoopbackResponder Host(Func respond) + { + var host = LoopbackResponder.Start(respond); + _hosts.Add(host); + + return host; + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Providers/ProviderEndpointUnreachableExceptionTests.cs b/backend/tests/CodeSpace.UnitTests/Providers/ProviderEndpointUnreachableExceptionTests.cs new file mode 100644 index 000000000..d5aa73802 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Providers/ProviderEndpointUnreachableExceptionTests.cs @@ -0,0 +1,30 @@ +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Exceptions; +using CodeSpace.Messages.Failures; +using Shouldly; + +namespace CodeSpace.UnitTests.Providers; + +/// +/// The typed failure for an edit that upgrades a connected provider's address to https when nothing answers HTTP there: what +/// it names, what it tells the owner to check, and that it never repeats more of the address than scheme, host and port. +/// +[Trait("Category", "Unit")] +public class ProviderEndpointUnreachableExceptionTests +{ + [Fact] + public void An_address_that_does_not_answer_is_an_unprocessable_failure_naming_the_origin_and_what_to_check() + { + var failure = new ProviderEndpointUnreachableException(ProviderKind.GitLab, new Uri("https://gitlab.acme.test:8443/gitlab?private_token=SECRET"), "AuthenticationException"); + + ((IFailure)failure).Kind.ShouldBe(FailureKind.Unprocessable); + ((IFailure)failure).Code.ShouldBe("provider_endpoint_unreachable"); + failure.Origin.ShouldBe("https://gitlab.acme.test:8443"); + failure.Message.ShouldContain("https://gitlab.acme.test:8443"); + failure.Message.ShouldContain("AuthenticationException"); + failure.Message.ShouldContain("certificate this server trusts"); + failure.Message.ShouldNotContain("SECRET"); + ((IFailure)failure).Details!["provider"].ShouldBe("GitLab"); + ((IFailure)failure).Details!["origin"].ShouldBe("https://gitlab.acme.test:8443"); + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Providers/ProviderInstanceEndpointBindingTests.cs b/backend/tests/CodeSpace.UnitTests/Providers/ProviderInstanceEndpointBindingTests.cs index 0eb347809..52e7eb6f2 100644 --- a/backend/tests/CodeSpace.UnitTests/Providers/ProviderInstanceEndpointBindingTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Providers/ProviderInstanceEndpointBindingTests.cs @@ -32,9 +32,39 @@ public class ProviderInstanceEndpointBindingTests // Conservative on purpose: the rule reads URLs, not each provider's derivation, so it cannot know github.com's API // already lives at api.github.com — spelling it out reads as a move, refused only while a token or client secret is bound. [InlineData("https://github.com", null, "https://github.com", "https://api.github.com", true)] + // The one move that is not one: an upgrade of http to https on the SAME host, which a redirect from the old URL asks for + // and which sends a token nowhere it was not already going — only over TLS. Default ports (80 to 443) or the same + // explicit port; the host, case aside, must be the very same. + [InlineData("http://gitlab.acme.test", null, "https://gitlab.acme.test", null, false)] + [InlineData("http://gitlab.acme.test:80", null, "https://gitlab.acme.test:443", null, false)] + [InlineData("http://gitlab.acme.test:8080", null, "https://gitlab.acme.test:8080", null, false)] + [InlineData("http://GITLAB.acme.test/", null, "https://gitlab.acme.test/gitlab", null, false)] + [InlineData("http://svc:pw@gitlab.acme.test", null, "https://gitlab.acme.test", null, false)] + [InlineData("http://gitlab.acme.test", "http://api.acme.test", "https://gitlab.acme.test", "https://api.acme.test", false)] + [InlineData("http://gitlab.acme.test", "http://api.acme.test", "https://gitlab.acme.test", "http://api.acme.test", false)] + [InlineData("http://gitlab.acme.test", null, "https://gitlab.acme.test", "http://gitlab.acme.test", false)] + // Anything else about the move still is one: another port, another host (an alias included), a downgrade, a sibling domain. + [InlineData("http://gitlab.acme.test", null, "https://gitlab.acme.test:8443", null, true)] + [InlineData("http://gitlab.acme.test:8080", null, "https://gitlab.acme.test", null, true)] + [InlineData("http://gitlab.acme.test:8080", null, "https://gitlab.acme.test:8443", null, true)] + [InlineData("http://gitlab.acme.test:443", null, "https://gitlab.acme.test", null, true)] + [InlineData("http://gitlab.acme.test", null, "https://gitlab-alias.acme.test", null, true)] + [InlineData("http://gitlab.acme.test", null, "https://gitlab.acme.test.attacker.test", null, true)] + [InlineData("http://gitlab.acme.test", null, "https://attacker.test", null, true)] + [InlineData("http://gitlab.acme.test", "http://api.acme.test", "https://gitlab.acme.test", "https://attacker.test", true)] + [InlineData("http://gitlab.acme.test", "http://api.acme.test", "https://gitlab.acme.test", "https://api.acme.test:8443", true)] + [InlineData("http://gitlab.acme.test", "https://api.acme.test", "https://gitlab.acme.test", "http://api.acme.test", true)] + [InlineData("http://gitlab.acme.test", null, "ftp://gitlab.acme.test", null, true)] public void An_edit_moves_the_endpoint_when_the_api_or_the_base_url_changes_scheme_host_or_port(string storedBaseUrl, string? storedApiUrl, string nextBaseUrl, string? nextApiUrl, bool expected) => ProviderInstanceService.EndpointMoves(storedBaseUrl, storedApiUrl, nextBaseUrl, nextApiUrl).ShouldBe(expected); + [Theory] + [InlineData("http://gitlab.acme.test", null, "https://gitlab.acme.test", null, true)] + [InlineData("http://gitlab.acme.test", null, "https://gitlab.acme.test:8443", null, true)] + [InlineData("http://gitlab.acme.test", null, "http://gitlab.acme.test/gitlab", null, false)] + public void An_upgrade_is_still_an_endpoint_change_for_the_audit_line(string storedBaseUrl, string? storedApiUrl, string nextBaseUrl, string? nextApiUrl, bool expected) => + ProviderInstanceService.EndpointChanges(storedBaseUrl, storedApiUrl, nextBaseUrl, nextApiUrl).ShouldBe(expected); + [Fact] public void Bound_credentials_refuse_the_move_as_a_precondition_naming_how_many() { diff --git a/backend/tests/CodeSpace.UnitTests/Redaction/CredentialUrlTests.cs b/backend/tests/CodeSpace.UnitTests/Redaction/CredentialUrlTests.cs index aed6c008e..3fd1c253a 100644 --- a/backend/tests/CodeSpace.UnitTests/Redaction/CredentialUrlTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Redaction/CredentialUrlTests.cs @@ -64,6 +64,29 @@ public void Masked_never_contains_the_embedded_credentials() [InlineData("gw.example:4000", "gw.example:4000", true)] public void SameEndpoint_compares_scheme_host_and_port(string? stored, string? next, bool expected) => CredentialUrl.SameEndpoint(stored, next).ShouldBe(expected); + [Theory] + [InlineData("http://gw.example", "https://gw.example", true)] // the default ports, 80 to 443 + [InlineData("http://gw.example:80", "https://gw.example:443", true)] // spelled out + [InlineData("http://gw.example:8080", "https://gw.example:8080", true)] // the same explicit port + [InlineData("http://GW.example/v1", "https://gw.EXAMPLE/other?x=1", true)] // host case, path and query are not part of it + [InlineData("http://a:b@gw.example", "https://gw.example", true)] + [InlineData("http://gw.example", "https://gw.example:8443", false)] // another port + [InlineData("http://gw.example:8080", "https://gw.example", false)] + [InlineData("http://gw.example:8080", "https://gw.example:8443", false)] + [InlineData("http://gw.example:443", "https://gw.example", false)] // an explicit 443 on http is not the https default + [InlineData("http://gw.example", "https://gw.example:80", false)] + [InlineData("https://gw.example", "http://gw.example", false)] // a downgrade + [InlineData("https://gw.example", "https://gw.example", false)] // not a move at all + [InlineData("http://gw.example", "http://gw.example", false)] + [InlineData("http://gw.example", "https://gw2.example", false)] // another host, an alias included + [InlineData("http://gw.example", "https://gw.example.evil.example", false)] + [InlineData("http://gw.example", "ftp://gw.example", false)] + [InlineData("gw.example:4000", "https://gw.example:4000", false)] // not an absolute http(s) URL + [InlineData(null, "https://gw.example", false)] + [InlineData("http://gw.example", null, false)] + [InlineData("", "", false)] + public void IsHttpsUpgrade_is_the_same_host_and_port_moved_from_http_to_https(string? stored, string? next, bool expected) => CredentialUrl.IsHttpsUpgrade(stored, next).ShouldBe(expected); + private const string StoredWithCredentials = "https://svc:CANARY-PW-11@gw.example/v1?api-key=CANARY-Q-22&api-version=2024-01-01"; [Theory] diff --git a/backend/tests/CodeSpace.UnitTests/Workflows/GitRemoteRedirectTests.cs b/backend/tests/CodeSpace.UnitTests/Workflows/GitRemoteRedirectTests.cs new file mode 100644 index 000000000..cef735aca --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Workflows/GitRemoteRedirectTests.cs @@ -0,0 +1,143 @@ +using CodeSpace.Core.Services.Agents.Sandbox; +using CodeSpace.Core.Services.Agents.Workspace; +using CodeSpace.Core.Services.Agents.Workspace.Providers; +using CodeSpace.Messages.Agents; +using CodeSpace.Messages.Exceptions; +using CodeSpace.Messages.Failures; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; + +namespace CodeSpace.UnitTests.Workflows; + +/// +/// A tokened git command whose remote redirects to another origin. git follows the redirect itself and then asks for a +/// credential at the origin it was sent to; the credential helper answers for the remote's own origin only, so none is found +/// and git says could not read Username for '<that origin>': terminal prompts disabled — git's own text, which +/// reads like a credential that was never configured. The failure names the redirect instead (the typed +/// provider_base_url_redirects and the origin git was sent to), as a provider's REST call to the same address does, +/// and stays a so every catch site that treats a git failure as one still does. A prompt at +/// the remote's OWN origin is not a redirect — a remote that refused the credential once is not asked again — and is left as +/// the plain failure it was. +/// +[Trait("Category", "Unit")] +public sealed class GitRemoteRedirectTests +{ + private const string Remote = "http://127.0.0.1:18102/g/r.git"; + + [Theory] + [InlineData("fatal: could not read Username for 'https://127.0.0.1:18443': terminal prompts disabled", "https://127.0.0.1:18443")] // http to https + [InlineData("fatal: could not read Username for 'http://localhost:18102': terminal prompts disabled", "http://localhost:18102")] // a host alias + [InlineData("fatal: could not read Username for 'http://127.0.0.1:18103': No such device or address", "http://127.0.0.1:18103")] // another port, git without a terminal + [InlineData("remote: HTTP Basic: Access denied\nfatal: could not read Password for 'http://svc@other.test': terminal prompts disabled", "http://other.test")] // userinfo is not part of an origin + [InlineData("fatal: could not read Username for 'http://OTHER.test:80': terminal prompts disabled", "http://other.test")] // a scheme's own port is not spelled + public void The_origin_git_was_sent_to_is_named_when_it_is_not_the_remotes_own(string stderr, string expectedOrigin) + { + var redirect = GitRemoteRedirect.Named(Remote, stderr).ShouldNotBeNull(); + + redirect.FinalOrigin.ShouldBe(expectedOrigin); + redirect.RequestedOrigin.ShouldBe("http://127.0.0.1:18102"); + redirect.Message.ShouldContain(expectedOrigin); + redirect.Message.ShouldNotContain("svc", Case.Sensitive, "a userinfo git echoed is not repeated"); + } + + [Theory] + [InlineData("fatal: could not read Username for 'http://127.0.0.1:18102': terminal prompts disabled")] // the remote's own origin: it refused the credential, not a redirect + [InlineData("fatal: could not read Username for 'HTTP://127.0.0.1:18102': terminal prompts disabled")] // another spelling of it + [InlineData("fatal: repository 'http://127.0.0.1:18102/g/r.git/' not found")] // some other failure + [InlineData("fatal: could not read Username for 'not a url': terminal prompts disabled")] // nothing parseable to name + [InlineData("fatal: could not read Username for 'ssh://git@host/g/r.git': terminal prompts disabled")] // not an http(s) origin + [InlineData("")] + public void Nothing_is_named_when_git_did_not_leave_the_remotes_origin(string stderr) => + GitRemoteRedirect.Named(Remote, stderr).ShouldBeNull(); + + [Theory] + [InlineData("git@github.com:org/repo.git")] + [InlineData("/srv/git/repo.git")] + [InlineData("file:///srv/git/repo.git")] + public void A_remote_that_is_not_http_has_no_origin_to_have_left(string remote) => + GitRemoteRedirect.Named(remote, "fatal: could not read Username for 'https://elsewhere.test': terminal prompts disabled").ShouldBeNull(); + + [Fact] + public void The_failure_is_still_a_workspace_exception_that_carries_the_redirect_as_its_code_and_details() + { + var failure = GitRemoteRedirect.Failure(Remote, "fatal: could not read Username for 'https://127.0.0.1:18443': terminal prompts disabled", "git clone failed (exit 128): fatal: could not read Username"); + + failure.Message.ShouldStartWith("git clone failed (exit 128)"); + failure.Message.ShouldContain("https://127.0.0.1:18443"); + failure.Message.ShouldContain("Edit the provider's base URL"); + failure.InnerException.ShouldBeOfType(); + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects"); + ((IFailure)failure).Kind.ShouldBe(FailureKind.Unprocessable); + ((IFailure)failure).Details!["finalOrigin"].ShouldBe("https://127.0.0.1:18443"); + } + + [Fact] + public void Any_other_failure_keeps_its_own_message_and_code() + { + var failure = GitRemoteRedirect.Failure(Remote, "fatal: could not read Username for 'http://127.0.0.1:18102': terminal prompts disabled", "git clone failed (exit 128): the plain failure"); + + failure.Message.ShouldBe("git clone failed (exit 128): the plain failure"); + failure.InnerException.ShouldBeNull(); + ((IFailure)failure).Code.ShouldBe("workspace_unresolvable"); + ((IFailure)failure).Details.ShouldBeNull(); + } + + [Fact] + public async Task The_launch_base_probe_names_the_redirect_git_met() + { + var runner = new FailingRunner("fatal: could not read Username for 'https://127.0.0.1:18443': terminal prompts disabled"); + + var failure = await Should.ThrowAsync(() => new RemoteTipResolver(new SandboxRunnerRegistry(new[] { runner })).ResolveTipShaAsync(new WorkspaceRequest { RepositoryUrl = Remote, Token = "t0ken", TokenUsername = "oauth2" }, refRequired: true, CancellationToken.None)); + + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects"); + failure.Message.ShouldContain("git ls-remote failed"); + failure.Message.ShouldContain("https://127.0.0.1:18443"); + failure.Message.ShouldNotContain("t0ken", Case.Sensitive); + } + + [Fact] + public async Task The_launch_base_probe_leaves_a_prompt_at_the_remotes_own_origin_as_the_plain_failure() + { + var runner = new FailingRunner("fatal: could not read Username for 'http://127.0.0.1:18102': terminal prompts disabled"); + + var failure = await Should.ThrowAsync(() => new RemoteTipResolver(new SandboxRunnerRegistry(new[] { runner })).ResolveTipShaAsync(new WorkspaceRequest { RepositoryUrl = Remote, Token = "t0ken", TokenUsername = "oauth2" }, refRequired: true, CancellationToken.None)); + + ((IFailure)failure).Code.ShouldBe("workspace_unresolvable"); + failure.Message.ShouldContain("git ls-remote failed"); + } + + [Fact] + public async Task The_workspace_clone_names_the_redirect_git_met() + { + var runner = new FailingRunner("fatal: could not read Username for 'http://localhost:18102': terminal prompts disabled", failingArg: "clone"); + var provider = new LocalGitWorkspaceProvider(new SandboxRunnerRegistry(new[] { runner }), NullLogger.Instance); + + var failure = await Should.ThrowAsync(() => provider.PrepareAsync(WorkspaceProvisionRequest.FromSingle(new WorkspaceRequest { RepositoryUrl = Remote, Token = "t0ken", TokenUsername = "oauth2" }), CancellationToken.None)); + + ((IFailure)failure).Code.ShouldBe("provider_base_url_redirects"); + failure.Message.ShouldContain("git clone failed"); + ((IFailure)failure).Details!["finalOrigin"].ShouldBe("http://localhost:18102"); + runner.Invocations.ShouldContain(args => args.Contains("clone"), "fixture check: the clone ran and is what failed"); + } + + /// A runner on which every git command succeeds with no output — except the ones naming failingArg (every one when null), which fail with stderr. + private sealed class FailingRunner(string stderr, string? failingArg = null) : ISandboxRunner + { + public string Kind => SandboxKinds.Local; + + public List> Invocations { get; } = new(); + + public Task RunAsync(SandboxSpec spec, CancellationToken cancellationToken) + { + Invocations.Add(spec.Args); + + if (spec.WorkingDirectory is { Length: > 0 } directory) Directory.CreateDirectory(directory); + + var fails = failingArg is null || spec.Args.Contains(failingArg); + + return Task.FromResult(fails + ? new SandboxResult { Status = SandboxStatus.Failed, ExitCode = 128, Stdout = "", Stderr = stderr } + : new SandboxResult { Status = SandboxStatus.Success, ExitCode = 0, Stdout = "", Stderr = "" }); + } + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Workflows/TokenedGitCommandTests.cs b/backend/tests/CodeSpace.UnitTests/Workflows/TokenedGitCommandTests.cs index 3fb4c3592..85aaf17e1 100644 --- a/backend/tests/CodeSpace.UnitTests/Workflows/TokenedGitCommandTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Workflows/TokenedGitCommandTests.cs @@ -59,6 +59,22 @@ public void A_tokened_command_carries_the_credential_in_its_environment_for_the_ TokenedGitSpecs.CarriesTheCredential(tokened, username, token).ShouldBeTrue(); } + [Theory] + [InlineData("http://gitlab.acme.test/org/repo.git", "https://gitlab.acme.test/org/repo.git", false)] // http to https: another scheme + [InlineData("http://127.0.0.1:8080/org/repo.git", "http://localhost:8080/org/repo.git", false)] // a host alias + [InlineData("http://gitlab.acme.test/org/repo.git", "http://gitlab.acme.test:8080/org/repo.git", false)] // another port + [InlineData("https://gitlab.acme.test/org/repo.git", "https://gitlab.acme.test/org/repo/", true)] // the same authority: another path + [InlineData("https://GitLab.Acme.test/org/repo.git", "https://gitlab.acme.test:443/org/moved.git", true)] // case and the default port are the same authority + public void A_redirect_is_inside_the_credentials_scope_only_when_the_scheme_and_authority_are_the_same(string url, string redirectsTo, bool credentialed) + { + // The three shapes a GitLab base URL redirects in are, for git, all outside the scope the credential answers for: the + // helper is keyed by scheme and authority, so the command fails at the redirect rather than send the token to it. + // A redirect to another path on the same authority (a moved repository, a URL without .git) stays inside. + string ScopeKey(string remoteUrl) => TokenedGitCommand.CredentialEnvironment(TokenedGitCommand.RemoteFor(remoteUrl, "oauth2", "glpat-fake")).Single(kv => kv.Key == "GIT_CONFIG_KEY_1").Value; + + (ScopeKey(url) == ScopeKey(redirectsTo)).ShouldBe(credentialed); + } + [Fact] public void A_tokened_command_maps_its_remote_to_itself_so_no_operator_rewrite_moves_it() { diff --git a/frontend/src/_imported/ai-code-space/connect-remote-modal.tsx b/frontend/src/_imported/ai-code-space/connect-remote-modal.tsx index 2d93123d0..4af431a8e 100644 --- a/frontend/src/_imported/ai-code-space/connect-remote-modal.tsx +++ b/frontend/src/_imported/ai-code-space/connect-remote-modal.tsx @@ -9,7 +9,7 @@ import { useAddGroupAccessToken, useAddProviderInstance, useCredentialCapabiliti import { useMe } from "@/hooks/use-me"; import { TeamPermissions, useTeamPermissions } from "@/hooks/use-team-management"; import { OAuthFlowError, useOAuthFlow } from "@/hooks/use-oauth-flow"; -import { endpointChanged, providerSecretMustBeReentered, showsMaskedCredentials } from "@/lib/credentialEndpoint"; +import { providerEndpointMoves, providerSecretMustBeReentered, showsMaskedCredentials } from "@/lib/credentialEndpoint"; import { providerSupportsTeamServiceCredential } from "@/lib/teamCredentials"; import { Ic } from "./icons"; @@ -1006,7 +1006,7 @@ function EditProviderStep({ instance, onBack, onClose, onSaved }: EditProviderSt const blanksOnImmutable = !displayName.trim() || !baseUrl.trim(); // A stored OAuth secret is only ever sent to the host it was entered for: moving the host needs it typed again. const nextBaseUrl = baseUrl.trim().replace(/\/$/, ""); - const hostMoves = endpointChanged(instance.baseUrl, nextBaseUrl); + const hostMoves = providerEndpointMoves(instance.baseUrl, nextBaseUrl); const secretMustFollow = providerSecretMustBeReentered(instance, nextBaseUrl); const missingSecretForMove = secretMustFollow && !secretFilled; const submitDisabled = update.isPending || blanksOnImmutable || oauthPairIncomplete || missingRequiredForConfig || missingSecretForMove; diff --git a/frontend/src/lib/credentialEndpoint.test.ts b/frontend/src/lib/credentialEndpoint.test.ts index d38cb0161..6d58fac91 100644 --- a/frontend/src/lib/credentialEndpoint.test.ts +++ b/frontend/src/lib/credentialEndpoint.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from "vitest"; -import { endpointChanged, keyMustBeReentered, providerSecretMustBeReentered, showsMaskedCredentials } from "./credentialEndpoint"; +import { endpointChanged, isHttpsUpgrade, keyMustBeReentered, providerEndpointMoves, providerSecretMustBeReentered, showsMaskedCredentials } from "./credentialEndpoint"; describe("endpointChanged", () => { it.each([ @@ -26,6 +26,40 @@ describe("endpointChanged", () => { }); }); +describe("isHttpsUpgrade", () => { + it.each([ + ["http://gitlab.acme.test", "https://gitlab.acme.test"], + ["http://gitlab.acme.test:80", "https://gitlab.acme.test:443"], + ["http://gitlab.acme.test:8080", "https://gitlab.acme.test:8080"], + ["http://GitLab.Acme.test/", "https://gitlab.acme.test/gitlab"], + ])("treats %s → %s as an upgrade of the same host", (original, next) => { + expect(isHttpsUpgrade(original, next)).toBe(true); + }); + + it.each([ + ["http://gitlab.acme.test", "https://gitlab.acme.test:8443"], + ["http://gitlab.acme.test:8080", "https://gitlab.acme.test"], + ["http://gitlab.acme.test:443", "https://gitlab.acme.test"], + ["http://gitlab.acme.test", "https://gitlab-alias.acme.test"], + ["https://gitlab.acme.test", "http://gitlab.acme.test"], + ["https://gitlab.acme.test", "https://gitlab.acme.test"], + ["gitlab.acme.test", "https://gitlab.acme.test"], + [null, "https://gitlab.acme.test"], + ["http://gitlab.acme.test", ""], + ])("does not treat %s → %s as one", (original, next) => { + expect(isHttpsUpgrade(original, next)).toBe(false); + }); +}); + +describe("providerEndpointMoves", () => { + it("is a change of endpoint bar an http to https upgrade of the same host", () => { + expect(providerEndpointMoves("http://gitlab.acme.test", "https://gitlab.acme.test")).toBe(false); + expect(providerEndpointMoves("http://gitlab.acme.test", "https://gitlab.acme.test:8443")).toBe(true); + expect(providerEndpointMoves("https://gitlab.acme.test", "https://attacker.test")).toBe(true); + expect(providerEndpointMoves("https://gitlab.acme.test", "https://gitlab.acme.test/gitlab")).toBe(false); + }); +}); + describe("keyMustBeReentered", () => { it("asks for the key when a stored key would follow the endpoint somewhere new", () => { expect(keyMustBeReentered({ keyHint: "····a1b2", keyUnreadable: false, baseUrl: "https://gw.example/v1" }, "https://attacker.example/v1")).toBe(true); @@ -53,6 +87,11 @@ describe("providerSecretMustBeReentered", () => { expect(providerSecretMustBeReentered({ oauthEnabled: false, baseUrl: "https://gitlab.acme.test" }, "https://gitlab.new.test")).toBe(false); }); + it("never asks for an http to https upgrade of the same host — the backend lets it through while tokens are connected", () => { + expect(providerSecretMustBeReentered({ oauthEnabled: true, baseUrl: "http://gitlab.acme.test" }, "https://gitlab.acme.test")).toBe(false); + expect(providerSecretMustBeReentered({ oauthEnabled: true, baseUrl: "http://gitlab.acme.test" }, "https://gitlab.acme.test:8443")).toBe(true); + }); + it("never asks for a path change or the masked URL sent back", () => { expect(providerSecretMustBeReentered({ oauthEnabled: true, baseUrl: "https://gitlab.acme.test" }, "https://gitlab.acme.test/gitlab")).toBe(false); expect(providerSecretMustBeReentered({ oauthEnabled: true, baseUrl: "https://***:***@gitlab.acme.test" }, "https://***:***@gitlab.acme.test")).toBe(false); diff --git a/frontend/src/lib/credentialEndpoint.ts b/frontend/src/lib/credentialEndpoint.ts index 21441cc32..ccda20dd0 100644 --- a/frontend/src/lib/credentialEndpoint.ts +++ b/frontend/src/lib/credentialEndpoint.ts @@ -28,6 +28,36 @@ export function endpointChanged(original: string | null | undefined, next: strin return endpointOf(original) !== endpointOf(next); } +function parseHttp(url: string | null | undefined): URL | null { + try { + const parsed = new URL((url ?? "").trim()); + return parsed.protocol === "http:" || parsed.protocol === "https:" ? parsed : null; + } catch { + return null; + } +} + +/** + * Whether `next` is `original` with its scheme moved from http to https and nothing else: the same host (case aside) and the + * same port — each scheme's own default (80 then 443), or one explicit port both spell. Mirrors the backend's + * `CredentialUrl.IsHttpsUpgrade`. A git provider connection may make exactly this move while tokens are connected through + * it (it sends them nowhere new, only over TLS); a model credential may not. + */ +export function isHttpsUpgrade(original: string | null | undefined, next: string | null | undefined): boolean { + const from = parseHttp(original); + const to = parseHttp(next); + + if (from === null || to === null || from.protocol !== "http:" || to.protocol !== "https:") return false; + + // `URL.port` is "" for the scheme's own default port, so 80 to 443 and an unspelled port on both sides compare equal. + return from.hostname.toLowerCase() === to.hostname.toLowerCase() && from.port === to.port; +} + +/** Whether saving `next` moves a git provider connection to another endpoint — a change of scheme, host or port, bar an http to https upgrade of the same host. */ +export function providerEndpointMoves(original: string | null | undefined, next: string | null | undefined): boolean { + return endpointChanged(original, next) && !isHttpsUpgrade(original, next); +} + /** Whether saving `nextBaseUrl` on this credential needs the key typed again: a key is stored (readable or not) and the endpoint moves. */ export function keyMustBeReentered(credential: Pick, nextBaseUrl: string | null | undefined): boolean { const hasStoredKey = credential.keyHint != null || credential.keyUnreadable; @@ -43,8 +73,9 @@ export function showsMaskedCredentials(url: string | null | undefined): boolean /** * Whether saving `nextBaseUrl` on a git provider connection moves it to another host, which its stored OAuth client * secret may not follow: the save needs the secret (the new host's OAuth app's, with its client ID) typed again. The - * edit form changes only the base URL, so this is the backend's `ProviderInstanceService.EndpointMoves` for that one URL. + * edit form changes only the base URL, so this is the backend's `ProviderInstanceService.EndpointMoves` for that one URL — + * which leaves an http to https upgrade of the same host alone. */ export function providerSecretMustBeReentered(instance: Pick, nextBaseUrl: string): boolean { - return instance.oauthEnabled && endpointChanged(instance.baseUrl, nextBaseUrl); + return instance.oauthEnabled && providerEndpointMoves(instance.baseUrl, nextBaseUrl); }