diff --git a/backend/src/CodeSpace.Core/Services/Agents/PackCloneFetcher.cs b/backend/src/CodeSpace.Core/Services/Agents/PackCloneFetcher.cs index 00bcf86a6..d9d7e65b2 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/PackCloneFetcher.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/PackCloneFetcher.cs @@ -1,6 +1,8 @@ +using System.Text.RegularExpressions; using CodeSpace.Core.DependencyInjection; using CodeSpace.Core.Services.Agents.Sandbox; using CodeSpace.Core.Services.Agents.Workspace; +using CodeSpace.Core.Services.Agents.Workspace.Providers; using CodeSpace.Messages.Agents; using Microsoft.Extensions.Logging; @@ -16,8 +18,14 @@ namespace CodeSpace.Core.Services.Agents; /// FAILURE (or any throw mid-clone) reclaims the partial dir immediately; (3) — the /// crash-safety backstop: the recurring sweep (which fans out over every janitor) ages out a clone orphaned by a /// worker that died between clone and dispose. +/// +/// A pasted URL can carry a credential in its userinfo (). The clone runs as a tokened command, +/// so the operator's credential helpers and trace2 targets never see it, in a directory only this worker's uid can read; +/// once cloned, origin is rewritten to the URL without it, so the checkout the import walks holds none; a clone failure names +/// the URL without it and redacts it from git's stderr, since that message reaches the API error body, the UI and the +/// mediator's error log. /// -public sealed class PackCloneFetcher : IPackSourceFetcher, IWorkspaceJanitor, ISingletonDependency +public sealed partial class PackCloneFetcher : IPackSourceFetcher, IWorkspaceJanitor, ISingletonDependency { /// Operators tune how long an orphaned pack clone lingers before the janitor reclaims it (a TimeSpan, e.g. "00:30:00"); default 1h. Pinned by a test (Rule 8). MUST exceed the maximum possible import duration so the age-based sweep never deletes a live clone. public const string StaleThresholdEnvVar = "CODESPACE_PACK_CLONE_STALE_THRESHOLD"; @@ -50,25 +58,54 @@ public async Task FetchAsync(string url, string? reference, Cancel Directory.CreateDirectory(PackClonesRoot); var dir = Path.Combine(PackClonesRoot, Guid.NewGuid().ToString("N")); - SandboxResult result; try { - Directory.CreateDirectory(dir); - result = await _runners.Resolve(SandboxKinds.Local).RunAsync(BuildCloneSpec(url, reference, dir), cancellationToken).ConfigureAwait(false); + CreateOwnerOnlyDirectory(dir); + await CloneAsync(url, reference, dir, cancellationToken).ConfigureAwait(false); + await StripPastedCredentialAsync(url, dir, cancellationToken).ConfigureAwait(false); } catch { - TryDeleteDirectory(dir); // never leak a partial clone, even on an unexpected throw / cancellation + TryDeleteDirectory(dir); // never leak a partial clone, or one still holding a pasted credential, even on an unexpected throw / cancellation throw; } + return new PackCheckout(dir); + } + + /// + /// The clone's directory, readable by this worker's uid alone. git writes the pasted URL, credential included, into + /// .git/config before the transfer starts, and origin is stripped only once it ends — up to the clone timeout later, + /// or never when the worker dies mid-clone and leaves it to the janitor — so it is owner-only before git runs. + /// + private static void CreateOwnerOnlyDirectory(string dir) + { + Directory.CreateDirectory(dir); + if (!OperatingSystem.IsWindows()) File.SetUnixFileMode(dir, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + } + + /// Run the clone; a failure throws a that names no pasted credential. + private async Task CloneAsync(string url, string? reference, string dir, CancellationToken cancellationToken) + { + var result = await _runners.Resolve(SandboxKinds.Local).RunAsync(BuildCloneSpec(url, reference, dir), cancellationToken).ConfigureAwait(false); + if (result.Status != SandboxStatus.Success) - { - TryDeleteDirectory(dir); // clone failed → reclaim the partial dir immediately - throw new PackImportException($"git clone of '{url}' failed ({result.Status}, exit {result.ExitCode}): {Summarize(result.Stderr)}"); - } + throw new PackImportException(CloneFailedMessage(url, result)); + } - return new PackCheckout(dir); + /// + /// git writes the pasted URL, credential included, into the clone's origin, and the import then walks that checkout (a + /// worker that dies mid-import leaves it on disk for the janitor). Rewrite origin to the URL without the credential through + /// the workspace provider's own strip: set-url, else remove origin, else a — and the + /// caller deletes the clone on the way out. + /// + private async Task StripPastedCredentialAsync(string url, string dir, CancellationToken cancellationToken) + { + var cleanUrl = WithoutPastedCredential(url); + + if (cleanUrl == url) return; + + await LocalGitWorkspaceProvider.StripTokenFromRemoteAsync(_runners.Resolve(SandboxKinds.Local), CloneTimeoutSeconds, _logger, cleanUrl, dir, cancellationToken).ConfigureAwait(false); } /// @@ -93,9 +130,18 @@ internal static IReadOnlyList BuildCloneArgs(string url, string? referen return args; } - /// The clone as the runner gets it: in , with the network. A pasted URL carrying a token clones as a , so no credential helper stores it and no trace2 target records it. - internal static SandboxSpec BuildCloneSpec(string url, string? reference, string dir) => - TokenedGitCommand.Spec(url, new SandboxSpec { Command = "git", Args = BuildCloneArgs(url, reference, dir), WorkingDirectory = dir, TimeoutSeconds = CloneTimeoutSeconds, AllowNetwork = true }); + /// + /// The clone as the runner gets it: in , with the network. A pasted URL + /// carrying a credential () clones as a , so no credential helper sees + /// it and no trace2 target records it — a token pasted as the user alone too, which carries no password for + /// to find, yet git hands it to every helper it asks for the missing one. + /// + internal static SandboxSpec BuildCloneSpec(string url, string? reference, string dir) + { + var spec = new SandboxSpec { Command = "git", Args = BuildCloneArgs(url, reference, dir), WorkingDirectory = dir, TimeoutSeconds = CloneTimeoutSeconds, AllowNetwork = true }; + + return PastedSecret(url) is null ? spec : TokenedGitCommand.AsTokened(url, spec); + } // ── IWorkspaceJanitor: reclaim pack clones orphaned by a crashed worker ────────────────────────── @@ -145,6 +191,52 @@ private static void TryDeleteDirectory(string directory) } } + /// + /// The clone failure as the operator reads it — in the API error body, the UI and the mediator's error log: the URL + /// without its pasted credential, and git's stderr with that credential redacted. git hides a password, but when a token + /// is pasted as the user alone it asks for a password and names that user. Pure + internal so it is unit-pinned. + /// + internal static string CloneFailedMessage(string url, SandboxResult result) => + $"git clone of '{WithoutPastedCredential(url)}' failed ({result.Status}, exit {result.ExitCode}): {RedactPastedCredential(url, Summarize(result.Stderr))}"; + + /// + /// The part of a pasted http(s) URL's userinfo that carries its credential: the password when one is given + /// (x-access-token:<token>@, oauth2:<token>@), else the user — a token pasted as the user alone + /// (<token>@). Null for a URL without userinfo and for any other scheme: git never sends an ssh URL's user as a + /// credential, and its git@ names an account. + /// + private static string? PastedSecret(string url) + { + if (!Uri.TryCreate(url, UriKind.Absolute, out var uri) || (uri.Scheme != Uri.UriSchemeHttps && uri.Scheme != Uri.UriSchemeHttp)) return null; + + var (user, password) = uri.UserInfo.Split(':', 2) is [var u, var p] ? (u, p) : (uri.UserInfo, ""); + var secret = password.Length > 0 ? password : user; + + return secret.Length > 0 ? secret : null; + } + + /// + /// without the pasted credential. First the userinfo of every http(s) URL in it (), + /// whichever part carries the token — <token>:x-oauth-basic@ puts it in the user — and in whatever spelling. Then + /// as bare text in each spelling git or a remote may echo it: a decoded user can hold the '/' or + /// '@' that ends a URL's userinfo, and a remote can echo the token it was handed. A user beside a password is not bare + /// text to redact: it names an account, and git names it only inside a URL, since it asks for a password only when none + /// was given — masking it would mask every 'a' in git's reason, or the owner in the repository's path. + /// + private static string RedactPastedCredential(string url, string text) => + new SecretRedactor(Spellings(PastedSecret(url))).Redact(UrlUserInfo().Replace(text, "${scheme}" + SecretRedactor.Placeholder + "@")); + + /// in each spelling git may echo it: as written, decoded (git 2.33 names a user decoded) and re-encoded (later git re-encodes it). + private static IEnumerable Spellings(string? secret) => + secret is null ? Array.Empty() : new[] { secret, Uri.UnescapeDataString(secret), Uri.EscapeDataString(Uri.UnescapeDataString(secret)) }; + + /// An http(s) URL's userinfo in free text: what follows the scheme up to an '@', with no '/', '?', '#', whitespace or quote between. + [GeneratedRegex(@"(?https?://)[^/?#@\s'""]+@", RegexOptions.IgnoreCase | RegexOptions.CultureInvariant)] + private static partial Regex UrlUserInfo(); + + /// The URL origin keeps and an error names: without its userinfo () when that carries a , otherwise as written. + private static string WithoutPastedCredential(string url) => PastedSecret(url) is null ? url : RemoteTipResolver.SanitizeUrl(url); + private static string Summarize(string stderr) => string.IsNullOrWhiteSpace(stderr) ? "(no stderr)" : stderr.Trim().Replace("\n", " "); } diff --git a/backend/src/CodeSpace.Core/Services/Agents/PackHostAllowlist.cs b/backend/src/CodeSpace.Core/Services/Agents/PackHostAllowlist.cs index 70c4c160b..b912bed42 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/PackHostAllowlist.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/PackHostAllowlist.cs @@ -42,15 +42,18 @@ internal static IReadOnlySet BuildHosts(string? rawOverride) /// True when is a well-formed absolute https URL whose host is on ; else false with an actionable . Pure + internal so it's unit-pinned. internal static bool TryValidate(string url, IReadOnlySet hosts, out string reason) { + // Neither reason below names the input: a pasted token would otherwise ride into the API error body, the UI and the + // mediator's error log. An unparseable URL has no userinfo to strip, and a URL pasted without "https://" parses the + // token before its colon as the scheme. if (!Uri.TryCreate(url, UriKind.Absolute, out var uri)) { - reason = $"'{url}' is not a valid absolute URL."; + reason = "The pack source is not a valid absolute URL."; return false; } if (uri.Scheme != Uri.UriSchemeHttps) { - reason = $"Only https pack sources are allowed (got scheme '{uri.Scheme}'). Paste an https git URL."; + reason = "Only https pack sources are allowed. Paste an https git URL."; return false; } diff --git a/backend/src/CodeSpace.Core/Services/Agents/Workspace/TokenedGitCommand.cs b/backend/src/CodeSpace.Core/Services/Agents/Workspace/TokenedGitCommand.cs index 23f945683..45fc0220c 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/Workspace/TokenedGitCommand.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/Workspace/TokenedGitCommand.cs @@ -50,11 +50,16 @@ internal static IReadOnlyList CredentialHelperReset(string remoteUrl) return new[] { "-c", $"credential.{uri.Scheme}://{uri.Authority}.helper=" }; } - /// as a tokened command when the remote it can reach is tokened — ahead of its arguments, over its environment — otherwise unchanged. - internal static SandboxSpec Spec(string remoteUrl, SandboxSpec spec) - { - if (!IsTokened(remoteUrl)) return spec; + /// as a tokened command () when the remote it can reach is tokened, otherwise unchanged. + internal static SandboxSpec Spec(string remoteUrl, SandboxSpec spec) => IsTokened(remoteUrl) ? AsTokened(remoteUrl, spec) : spec; + /// + /// as a tokened command for — ahead of + /// its arguments, over its environment — whatever says: for a caller that knows + /// the URL's userinfo is a credential without a password, as a token pasted as the user alone is. + /// + internal static SandboxSpec AsTokened(string remoteUrl, SandboxSpec spec) + { var environment = new Dictionary(spec.Environment); foreach (var (name, value) in TraceOff) environment[name] = value; diff --git a/backend/tests/CodeSpace.IntegrationTests/Agents/PackCloneCredentialFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Agents/PackCloneCredentialFlowTests.cs new file mode 100644 index 000000000..27fda3a9d --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Agents/PackCloneCredentialFlowTests.cs @@ -0,0 +1,287 @@ +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Agents; +using CodeSpace.Core.Services.Agents.Sandbox; +using CodeSpace.Core.Services.Agents.Sandbox.Runners; +using CodeSpace.Core.Services.Identity; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.IntegrationTests.Workflows; +using CodeSpace.Messages.Agents; +using CodeSpace.Messages.Commands.Agents; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Enums; +using MediatR; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Agents; + +/// +/// HIGH fidelity: the REAL on the real and real git, +/// against a loopback smart-HTTP remote () that demands a FAKE token for every read, so a +/// clone that succeeds proves the pasted token authenticated it. An operator who pastes a URL carrying a token into the pack +/// import must get a checkout that holds no token once cloned, in a directory no other uid can read; a failed import must name +/// no token in its message — the text that reaches the API error body, the UI and the mediator's error log — even where git +/// itself echoes it (a token pasted as the user alone, in any spelling); and that user-alone token must reach neither the +/// operator's credential helpers nor their trace2 targets. +/// +/// Positive controls: the remote refuses a clone without the token; the same production clone with origin left as git +/// wrote it holds the token, so the scan that finds none can see one; git's raw stderr carried the token wherever the message +/// is clean of an echo, so the clean message is the redaction's doing; and the clone run without the helper reset and with +/// trace2 on hands the token to the operator's helper and trace2 targets, so their silence is the tokened clone's doing. Each +/// test owns its remote and a scratch HOME (a global config of its own, system config off), and removes both on every path; +/// nothing reads or writes the real global config or keychain. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public sealed class PackCloneCredentialFlowTests +{ + /// In every fake credential below, in every spelling git echoes it in, so one absence check covers them all. + private const string Marker = "token-0123456789"; + + private readonly PostgresFixture _fixture; + + public PackCloneCredentialFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + [Theory] + [InlineData(false)] + [InlineData(true)] // positive control: origin left as git wrote it + public async Task A_pasted_token_clones_and_the_checkout_holds_no_token(bool originAsCloned) + { + if (OperatingSystem.IsWindows() || !await GitAvailableAsync()) return; + + await using var ctx = await ScratchHostContext.StartAsync(); + ctx.Runner.KeepOriginAsCloned = originAsCloned; + + await Should.ThrowAsync(() => ctx.Fetcher.FetchAsync(ctx.Remote.Url, null, CancellationToken.None), "fixture check: the remote refuses a clone that presents no token"); + + using var checkout = await ctx.Fetcher.FetchAsync(ctx.UrlWith($"x-access-token:{GitPublishRemoteFixture.FakeToken}"), null, CancellationToken.None); + + File.ReadAllText(Path.Combine(checkout.Directory, "README.md")).ShouldBe("base, revised\n", "the clone authenticated with the pasted token"); + ctx.Runner.Ran("remote").ShouldBeTrue("fixture check: the fetcher asked for origin to be rewritten"); + FilesHolding(checkout.Directory, GitPublishRemoteFixture.FakeToken).ShouldBe(originAsCloned ? new[] { ".git/config" } : Array.Empty(), originAsCloned ? "positive control: git writes the pasted URL into origin" : "the checkout the import walks holds the pasted token"); + + if (!originAsCloned) (await ctx.OriginUrlAsync(checkout.Directory)).ShouldBe(ctx.Remote.Url, "origin was rewritten to the remote without its token, not removed"); + + File.GetUnixFileMode(checkout.Directory).ShouldBe(UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute, "git cloned into the owner-only directory without widening it, so no other uid read .git/config while it held the token"); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] // positive control: the clone run without the helper reset and with trace2 on + public async Task A_token_pasted_as_the_user_alone_reaches_no_credential_helper_or_trace2_target(bool unreset) + { + // git asks the credential helpers for the password the URL lacks, naming the user, and writes the clone's argv and + // its remote-http child's to the trace2 targets — both from the operator's global config. + if (OperatingSystem.IsWindows() || !await GitAvailableAsync()) return; + + await using var ctx = await ScratchHostContext.StartAsync(); + ctx.ConfigureLoggingHelperAndTrace2(); + + await Should.ThrowAsync(() => ctx.Fetcher.FetchAsync(ctx.Remote.Url, null, CancellationToken.None), "fixture check: the remote refuses a clone that presents no token"); + ctx.HelperLog().ShouldContain("op=get", Case.Sensitive, "fixture check: an untokened clone of the remote asks the operator's helper"); + ctx.OperatorTrace().ShouldContain(ctx.Remote.Url, Case.Sensitive, "fixture check: the operator's trace2 targets record an untokened clone"); + + ctx.Runner.Unreset = unreset; + await Should.ThrowAsync(() => ctx.Fetcher.FetchAsync(ctx.UrlWith("fake-pasted-token-0123456789"), null, CancellationToken.None), "git asks for a password the pasted URL does not carry"); + + ctx.HelperLog().Contains(Marker, StringComparison.Ordinal).ShouldBe(unreset, unreset ? "positive control: without the reset git hands the pasted user to the operator's helper" : "the pasted token reached the operator's credential helper"); + ctx.OperatorTrace().Contains(Marker, StringComparison.Ordinal).ShouldBe(unreset, unreset ? "positive control: with trace2 on git records the pasted URL" : "the pasted token reached the operator's trace2 targets"); + } + + [Theory] + [InlineData("x-access-token:fake-wrong-token-0123456789", false)] // a wrong or revoked token: git hides the password, the message used to name it in the URL + [InlineData("fake-pasted-token-0123456789", true)] // a token pasted as the user: git asks for a password and names the user + [InlineData("fake%2fpasted%40token-0123456789", true)] // the same, percent-encoded: git echoes it decoded or re-encoded + public async Task A_failed_clone_names_no_pasted_token(string userInfo, bool gitEchoesIt) + { + if (OperatingSystem.IsWindows() || !await GitAvailableAsync()) return; + + await using var ctx = await ScratchHostContext.StartAsync(); + + var failure = await Should.ThrowAsync(() => ctx.Fetcher.FetchAsync(ctx.UrlWith(userInfo), null, CancellationToken.None)); + + ctx.Runner.Stderr.Contains(Marker, StringComparison.Ordinal).ShouldBe(gitEchoesIt, "fixture check: where git echoed the pasted token"); + failure.Message.ShouldNotContain(Marker, Case.Sensitive, "the message reaches the API error body, the UI and the mediator's error log"); + failure.Message.ShouldContain($"'{ctx.Remote.Url}'", Case.Sensitive, "the message still names the remote, without its userinfo"); + failure.Message.ShouldContain("exit 128"); + } + + [Fact] + public async Task A_failed_import_through_the_mediator_names_no_pasted_token() + { + if (OperatingSystem.IsWindows() || !await GitAvailableAsync()) return; + + await using var ctx = await ScratchHostContext.StartAsync(); + var (teamId, userId) = await SeedTeamAsync(); + + using var scope = _fixture.BeginScope(b => + { + b.RegisterInstance(new TestCurrentUser(userId, "test", Roles.Admin)).As().SingleInstance(); + b.RegisterInstance(new TestCurrentTeam(teamId)).As().SingleInstance(); + b.RegisterInstance(ctx.Fetcher).As(); + }); + + var import = new ImportPackFromUrlCommand { Url = ctx.UrlWith("fake-pasted-token-0123456789"), SourcePaths = new[] { "agents/reviewer.md" } }; + + var failure = await Should.ThrowAsync(() => scope.Resolve().Send(import)); + + ctx.Runner.Stderr.ShouldContain("fake-pasted-token-0123456789", Case.Sensitive, "fixture check: git echoed the pasted token"); + failure.Message.ShouldNotContain(Marker, Case.Sensitive, "what the import surfaces to the API error body and the mediator's error log"); + } + + /// Every file under whose bytes contain , relative and with forward slashes — the whole checkout, not just .git/config. + private static string[] FilesHolding(string directory, string text) => + Directory.EnumerateFiles(directory, "*", SearchOption.AllDirectories) + .Where(file => File.ReadAllText(file).Contains(text, StringComparison.Ordinal)) + .Select(file => Path.GetRelativePath(directory, file).Replace(Path.DirectorySeparatorChar, '/')) + .Order(StringComparer.Ordinal) + .ToArray(); + + private static async Task GitAvailableAsync() + { + try { return (await new LocalProcessRunner().RunAsync(new SandboxSpec { Command = "git", Args = new[] { "--version" }, TimeoutSeconds = 15 }, CancellationToken.None)).Status == SandboxStatus.Success; } + catch { return false; } + } + + private async Task<(Guid TeamId, Guid UserId)> SeedTeamAsync() + { + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + + var userId = Guid.NewGuid(); + db.User.Add(new User { Id = userId, Email = $"packcred-{userId:N}@test.local", Name = $"packcred-{userId:N}" }); + + var teamId = Guid.NewGuid(); + db.Team.Add(new Team { Id = teamId, Slug = $"packcred-{teamId:N}", Name = "Pack Credential Team", Kind = TeamKind.Workspace }); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = teamId, UserId = userId, Role = TeamRole.Owner }); + + await db.SaveChangesAsync(); + return (teamId, userId); + } + + /// The remote, a scratch host (HOME, a global config absent until a test writes one, system config off) and the production fetcher running on it. + private sealed class ScratchHostContext : IAsyncDisposable + { + private readonly string _home = Directory.CreateTempSubdirectory("cs-packcred-home-").FullName; + + private ScratchHostContext() + { + Runner = new ScratchHostRunner(new Dictionary { ["HOME"] = _home, ["GIT_CONFIG_GLOBAL"] = GlobalConfig, ["GIT_CONFIG_NOSYSTEM"] = "1" }); + Fetcher = new PackCloneFetcher(new AllowAll(), new SandboxRunnerRegistry(new ISandboxRunner[] { Runner }), NullLogger.Instance); + } + + public GitPublishRemoteFixture Remote { get; } = new() { AuthenticateReads = true }; + public ScratchHostRunner Runner { get; } + public PackCloneFetcher Fetcher { get; } + + public static async Task StartAsync() + { + var ctx = new ScratchHostContext(); + + try + { + await ctx.Remote.StartAsync(); + return ctx; + } + catch + { + await ctx.DisposeAsync(); + throw; + } + } + + private string GlobalConfig => Path.Combine(_home, "global-config"); + private string HelperLogFile => Path.Combine(_home, "helper.log"); + private string TraceFile(string target) => Path.Combine(_home, "trace2-" + target); + + /// + /// The operator's global config: a credential helper that logs every request git makes of it (the operation, then + /// what git sends: protocol, host, username) and answers none, and all three trace2 targets pointed at scratch files. + /// + public void ConfigureLoggingHelperAndTrace2() + { + var helper = Path.Combine(_home, "logging-helper.sh"); + File.WriteAllText(helper, $"#!/bin/sh\necho \"op=$1\" >> '{HelperLogFile}'\ncat >> '{HelperLogFile}'\n"); + if (!OperatingSystem.IsWindows()) File.SetUnixFileMode(helper, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + + File.WriteAllText(GlobalConfig, $"[credential]\n\thelper = {helper}\n[trace2]\n\tnormalTarget = {TraceFile("normal")}\n\teventTarget = {TraceFile("event")}\n\tperfTarget = {TraceFile("perf")}\n"); + } + + /// Every request git made of the operator's helper. + public string HelperLog() => File.Exists(HelperLogFile) ? File.ReadAllText(HelperLogFile) : ""; + + /// Everything the operator's trace2 targets recorded. + public string OperatorTrace() => string.Concat(new[] { "normal", "event", "perf" }.Select(TraceFile).Where(File.Exists).Select(File.ReadAllText)); + + /// The remote's URL as an operator would paste it with in it. + public string UrlWith(string userInfo) => Remote.Url.Replace("http://", $"http://{userInfo}@", StringComparison.Ordinal); + + /// The clone's origin URL as git reads it, on the scratch host; never recorded by the runner. + public async Task OriginUrlAsync(string cloneDir) + { + var result = await new LocalProcessRunner().RunAsync(new SandboxSpec { Command = "git", Args = new[] { "-C", cloneDir, "config", "--get", "remote.origin.url" }, Environment = Runner.Environment, TimeoutSeconds = 30 }, CancellationToken.None); + + result.Status.ShouldBe(SandboxStatus.Success, $"git config --get remote.origin.url failed: {result.Stderr}"); + return result.Stdout.Trim(); + } + + public async ValueTask DisposeAsync() + { + await Remote.DisposeAsync(); + try { Directory.Delete(_home, recursive: true); } catch { /* best-effort */ } + } + } + + /// + /// The real local runner on the scratch host, recording each subcommand and git's raw stderr. With + /// set, the git remote edits that strip a pasted credential report success without + /// running, so origin stays as git wrote it; with set, the clone runs without the credential-helper + /// reset and with trace2 on — the positive controls. + /// + private sealed class ScratchHostRunner(IReadOnlyDictionary environment) : ISandboxRunner + { + private static readonly string[] TraceOff = { "GIT_TRACE2", "GIT_TRACE2_EVENT", "GIT_TRACE2_PERF" }; + + private readonly LocalProcessRunner _inner = new(); + private readonly List _specs = new(); + + public string Kind => "local"; + public IReadOnlyDictionary Environment => environment; + public bool KeepOriginAsCloned { get; set; } + public bool Unreset { get; set; } + public string Stderr { get; private set; } = ""; + + public bool Ran(string subcommand) => _specs.Any(s => s.Args.Contains(subcommand)); + + public async Task RunAsync(SandboxSpec spec, CancellationToken cancellationToken) + { + _specs.Add(spec); + + if (KeepOriginAsCloned && spec.Args.Contains("remote")) return new SandboxResult { Status = SandboxStatus.Success, ExitCode = 0, Stdout = "", Stderr = "" }; + + var unreset = Unreset && spec.Args.Contains("clone"); + var env = new Dictionary(spec.Environment); + foreach (var (key, value) in environment) env[key] = value; + if (unreset) foreach (var key in TraceOff) env.Remove(key); + + var result = await _inner.RunAsync(spec with { Args = unreset ? WithoutTheReset(spec.Args) : spec.Args, Environment = env }, cancellationToken); + Stderr += result.Stderr; + return result; + } + + private static IReadOnlyList WithoutTheReset(IReadOnlyList args) + { + var at = Enumerable.Range(0, Math.Max(0, args.Count - 1)).FirstOrDefault(i => args[i] == "-c" && args[i + 1].StartsWith("credential.", StringComparison.Ordinal) && args[i + 1].EndsWith(".helper=", StringComparison.Ordinal), -1); + + return at < 0 ? args : args.Take(at).Concat(args.Skip(at + 2)).ToList(); + } + } + + private sealed class AllowAll : IPackHostAllowlist + { + public bool IsAllowed(string url) => true; + public void EnsureAllowed(string url) { } + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Agents/PackCloneFetcherArgsTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/PackCloneFetcherArgsTests.cs index 35ad05483..e9d89c52f 100644 --- a/backend/tests/CodeSpace.UnitTests/Agents/PackCloneFetcherArgsTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Agents/PackCloneFetcherArgsTests.cs @@ -49,6 +49,7 @@ public void Clone_argv_passes_a_branch_reference_when_set() [Theory] [InlineData("https://someone:ghp_pasted_token@github.com/owner/repo", true)] // a pasted URL with a personal token in it + [InlineData("https://ghp_pasted_token@github.com/owner/repo", true)] // the token alone as the user: git hands it to the helpers it asks for a password [InlineData("https://github.com/owner/repo", false)] public void Only_a_clone_url_carrying_a_token_runs_as_a_tokened_command(string url, bool tokened) { diff --git a/backend/tests/CodeSpace.UnitTests/Agents/PackCloneFetcherCredentialTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/PackCloneFetcherCredentialTests.cs new file mode 100644 index 000000000..d4e1c938c --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Agents/PackCloneFetcherCredentialTests.cs @@ -0,0 +1,184 @@ +using CodeSpace.Core.Services.Agents; +using CodeSpace.Core.Services.Agents.Sandbox; +using CodeSpace.Core.Services.Agents.Workspace; +using CodeSpace.Core.Services.Agents.Workspace.Providers; +using CodeSpace.Messages.Agents; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; + +namespace CodeSpace.UnitTests.Agents; + +/// +/// A pasted pack URL can carry a credential in its userinfo: as the password (x-access-token:<token>@, +/// oauth2:<token>@) or as the user (<token>@, <token>:x-oauth-basic@). Pins that a clone +/// failure names the URL without it and redacts it from git's stderr in every spelling git echoes it in, while a user named +/// beside a password leaves git's reason readable; that the clone runs in an owner-only directory and its origin is pointed +/// at the URL without the credential once cloned (and the clone refused when neither rewrite nor removal works); and that a +/// URL with no credential, an ssh URL's git@ included, is left as written. PackCloneCredentialFlowTests proves +/// the same against real git and a remote that demands the token. +/// +[Trait("Category", "Unit")] +public sealed class PackCloneFetcherCredentialTests +{ + /// In every fake credential below, in every spelling, so one absence check covers them all. + private const string Marker = "token-0123456789"; + + private const string Tokened = "https://x-access-token:fake-pasted-token-0123456789@github.com/owner/repo.git"; + + [Theory] + [InlineData("https://x-access-token:fake-pasted-token-0123456789@github.com/owner/repo.git", "fatal: Authentication failed for 'https://github.com/owner/repo.git/'")] + [InlineData("https://oauth2:fake-pasted-token-0123456789@github.com/owner/repo.git", "fatal: Authentication failed for 'https://github.com/owner/repo.git/'")] + [InlineData("https://fake-pasted-token-0123456789@github.com/owner/repo.git", "fatal: could not read Password for 'https://fake-pasted-token-0123456789@github.com': terminal prompts disabled")] + [InlineData("https://fake-pasted-token-0123456789:x-oauth-basic@github.com/owner/repo.git", "fatal: unable to access 'https://fake-pasted-token-0123456789:x-oauth-basic@github.com/owner/repo.git/': error: 403")] + public void A_clone_failure_names_the_url_without_its_credential(string url, string stderr) + { + var message = PackCloneFetcher.CloneFailedMessage(url, Failed(stderr)); + + message.ShouldNotContain(Marker, Case.Sensitive, "the message reaches the API error body, the UI and the mediator's error log"); + message.ShouldStartWith("git clone of 'https://github.com/owner/repo.git' failed (Failed, exit 128): ", Case.Sensitive, "the URL is still named, without its userinfo"); + } + + [Theory] + [InlineData("fake/pasted@token-0123456789")] // git 2.33 echoes the user decoded + [InlineData("fake%2Fpasted%40token-0123456789")] // later git re-encodes it + [InlineData("fake%2fpasted%40token-0123456789")] // as pasted + public void A_percent_encoded_credential_is_redacted_in_every_spelling_git_echoes(string echoed) + { + const string url = "https://fake%2fpasted%40token-0123456789@gitlab.com/group/pack.git"; + + var message = PackCloneFetcher.CloneFailedMessage(url, Failed($"fatal: could not read Password for 'https://{echoed}@gitlab.com': terminal prompts disabled")); + + message.ShouldNotContain(Marker); + message.ShouldEndWith("fatal: could not read Password for 'https://***@gitlab.com': terminal prompts disabled", Case.Sensitive, "git's reason stays readable around the redaction"); + } + + [Theory] + [InlineData("fake%2fpasted%40token-0123456789")] // as pasted + [InlineData("fake/pasted@token-0123456789")] // decoded + [InlineData("fake%2Fpasted%40token-0123456789")] // re-encoded + public void A_password_echoed_outside_a_url_is_redacted_in_every_spelling(string echoed) + { + // git keeps a password out of the URLs it names, but a remote can echo the token it was handed. + const string url = "https://x-access-token:fake%2fpasted%40token-0123456789@github.com/owner/repo.git"; + + var message = PackCloneFetcher.CloneFailedMessage(url, Failed($"remote: Invalid token {echoed}. fatal: Authentication failed for 'https://github.com/owner/repo.git/'")); + + message.ShouldNotContain(Marker); + message.ShouldEndWith("remote: Invalid token ***. fatal: Authentication failed for 'https://github.com/owner/repo.git/'", Case.Sensitive); + } + + [Theory] + [InlineData("https://a:fake-pasted-token-0123456789@gitlab.com/group/pack.git", "fatal: Authentication failed for 'https://gitlab.com/group/pack.git/'")] + [InlineData("https://owner:fake-pasted-token-0123456789@github.com/owner/repo.git", "fatal: repository 'https://github.com/owner/repo.git/' not found")] + public void A_user_named_beside_a_password_leaves_gits_reason_readable(string url, string stderr) + { + // The password carries the credential; the user beside it names an account, and git names it only inside a URL. + // Redacting it as bare text would mask every 'a' in the reason, or the owner in the repository's path. + PackCloneFetcher.CloneFailedMessage(url, Failed(stderr)).ShouldEndWith($"): {stderr}", Case.Sensitive); + } + + [Fact] + public async Task The_clone_directory_is_owner_only_before_git_writes_the_pasted_url_into_it() + { + // git writes the tokened origin into .git/config before the transfer starts, and the strip runs only after it, so + // for the whole clone the token is on disk under the worker's temp dir. + if (OperatingSystem.IsWindows()) return; + + var runner = new ScriptedRunner(); + + using var checkout = await Fetcher(runner).FetchAsync(Tokened, null, CancellationToken.None); + + runner.CloneDirectoryMode.ShouldBe(UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute, "no other uid on the host can read the clone while git writes it"); + } + + [Theory] + [InlineData("https://github.com/owner/repo.git", "fatal: repository 'https://github.com/owner/repo.git/' not found")] + [InlineData("ssh://git@github.com/owner/repo.git", "git@github.com: Permission denied (publickey).")] + [InlineData("git@github.com:owner/repo.git", "git@github.com: Permission denied (publickey).")] + public void A_url_without_a_credential_is_named_as_written(string url, string stderr) + { + // Positive control for the redaction: an ssh URL's user is an account, never a token, so neither it nor git's + // stderr naming it is touched. + PackCloneFetcher.CloneFailedMessage(url, Failed(stderr)).ShouldBe($"git clone of '{url}' failed (Failed, exit 128): {stderr}"); + } + + [Fact] + public async Task A_failed_clone_throws_without_the_credential_and_leaves_no_clone() + { + var runner = new ScriptedRunner { CloneStderr = "fatal: could not read Password for 'https://fake-pasted-token-0123456789@github.com': terminal prompts disabled" }; + + var failure = await Should.ThrowAsync(() => Fetcher(runner).FetchAsync("https://fake-pasted-token-0123456789@github.com/owner/repo.git", null, CancellationToken.None)); + + failure.Message.ShouldNotContain(Marker); + Directory.Exists(runner.Specs.Single().WorkingDirectory).ShouldBeFalse("a failed clone is reclaimed before the throw"); + } + + [Theory] + [InlineData(Tokened)] + [InlineData("https://fake-pasted-token-0123456789@github.com/owner/repo.git")] // a token pasted as the user lands in .git/config just the same + public async Task A_pasted_credential_is_stripped_from_origin_once_cloned(string url) + { + var runner = new ScriptedRunner(); + + using var checkout = await Fetcher(runner).FetchAsync(url, null, CancellationToken.None); + + runner.Specs.Count.ShouldBe(2, "the clone, then one rewrite of origin"); + runner.Specs[1].Args.ShouldBe(new[] { "-C", checkout.Directory, "remote", "set-url", "origin", "https://github.com/owner/repo.git" }); + } + + [Theory] + [InlineData("https://github.com/owner/repo.git")] + [InlineData("ssh://git@github.com/owner/repo.git")] + public async Task A_url_without_a_credential_is_cloned_with_origin_as_written(string url) + { + var runner = new ScriptedRunner(); + + using var checkout = await Fetcher(runner).FetchAsync(url, null, CancellationToken.None); + + runner.Specs.Count.ShouldBe(1, "nothing to strip, so no rewrite runs"); + } + + [Fact] + public async Task A_clone_that_cannot_shed_the_credential_is_refused_and_reclaimed() + { + var runner = new ScriptedRunner { RemoteEditsFail = true }; + + var failure = await Should.ThrowAsync(() => Fetcher(runner).FetchAsync(Tokened, null, CancellationToken.None)); + + failure.Message.ShouldContain(LocalGitWorkspaceProvider.TokenStripFailedDetail, Case.Sensitive, "the workspace provider's own fail-closed strip"); + runner.Specs.Count.ShouldBe(3, "the clone, the rewrite, then the removal"); + Directory.Exists(runner.Specs[0].WorkingDirectory).ShouldBeFalse("a clone still holding the credential is deleted before the throw"); + } + + private static PackCloneFetcher Fetcher(ScriptedRunner runner) => + new(new AllowAll(), new SandboxRunnerRegistry(new ISandboxRunner[] { runner }), NullLogger.Instance); + + private static SandboxResult Failed(string stderr) => new() { Status = SandboxStatus.Failed, ExitCode = 128, Stdout = "", Stderr = stderr }; + + private sealed class AllowAll : IPackHostAllowlist + { + public bool IsAllowed(string url) => true; + public void EnsureAllowed(string url) { } + } + + /// Records every spec, and the clone directory's mode as git would find it; the clone succeeds unless is set, and the git remote edits succeed unless . + private sealed class ScriptedRunner : ISandboxRunner + { + public string Kind => "local"; + public string? CloneStderr { get; init; } + public bool RemoteEditsFail { get; init; } + public List Specs { get; } = new(); + public UnixFileMode? CloneDirectoryMode { get; private set; } + + public Task RunAsync(SandboxSpec spec, CancellationToken cancellationToken) + { + if (Specs.Count == 0 && !OperatingSystem.IsWindows()) CloneDirectoryMode = File.GetUnixFileMode(spec.WorkingDirectory!); + + Specs.Add(spec); + + var fails = spec.Args.Contains("remote") ? RemoteEditsFail : CloneStderr is not null; + + return Task.FromResult(fails ? Failed(CloneStderr ?? "error: could not lock config file") : new SandboxResult { Status = SandboxStatus.Success, ExitCode = 0, Stdout = "", Stderr = "" }); + } + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Agents/PackHostAllowlistTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/PackHostAllowlistTests.cs index 64742dbfe..c6d8988e3 100644 --- a/backend/tests/CodeSpace.UnitTests/Agents/PackHostAllowlistTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Agents/PackHostAllowlistTests.cs @@ -25,9 +25,9 @@ public void Allows_https_github_and_gitlab(string url) } [Theory] - [InlineData("http://github.com/owner/repo", "scheme")] // not https - [InlineData("file:///etc/passwd", "scheme")] // not https - [InlineData("ssh://git@github.com/owner/repo", "scheme")] // not https + [InlineData("http://github.com/owner/repo", "Only https")] // not https + [InlineData("file:///etc/passwd", "Only https")] // not https + [InlineData("ssh://git@github.com/owner/repo", "Only https")] // not https [InlineData("https://internal.corp/secret", "allowlist")] // not an allowlisted host (SSRF / internal) [InlineData("https://169.254.169.254/latest/meta-data", "allowlist")] // cloud metadata host [InlineData("not-a-url", "valid absolute URL")] @@ -40,6 +40,33 @@ public void Refuses_disallowed_urls_with_an_actionable_reason(string url, string ex.Message.ShouldContain(reasonFragment); } + [Fact] + public void A_malformed_url_is_refused_without_echoing_the_credential_it_carries() + { + // An unparseable URL has no userinfo to strip, so the refusal names none of it: the message reaches the API error + // body, the UI and the mediator's error log, and the operator still has what they pasted. + const string url = "https://x-access-token:fake-pasted-token-0123456789@github.com:notaport/owner/repo"; + + var ex = Should.Throw(() => new PackHostAllowlist(rawAllowedHostsOverride: null).EnsureAllowed(url)); + + ex.Message.ShouldContain("valid absolute URL"); + ex.Message.ShouldNotContain("fake-pasted-token-0123456789"); + } + + [Theory] + [InlineData("fake-pasted-token-0123456789:x-oauth-basic@github.com/owner/repo.git")] + [InlineData("Fake-Pasted-Token-0123456789:x@gitlab.com/group/repo.git")] // the scheme comes back lowercased + public void A_url_pasted_without_https_is_refused_without_echoing_the_token_read_as_its_scheme(string url) + { + // Without "https://" the token before the colon parses as the URL's scheme, so naming the scheme names the token. + Uri.TryCreate(url, UriKind.Absolute, out _).ShouldBeTrue("fixture check: the token parses as the scheme"); + + var ex = Should.Throw(() => new PackHostAllowlist(rawAllowedHostsOverride: null).EnsureAllowed(url)); + + ex.Message.ShouldContain("Only https"); + ex.Message.ShouldNotContain("pasted-token-0123456789", Case.Insensitive); + } + [Fact] public void Operator_env_override_adds_hosts_to_the_defaults() { diff --git a/backend/tests/CodeSpace.UnitTests/Workflows/TokenedGitCommandTests.cs b/backend/tests/CodeSpace.UnitTests/Workflows/TokenedGitCommandTests.cs index 28101e5e2..e134408fb 100644 --- a/backend/tests/CodeSpace.UnitTests/Workflows/TokenedGitCommandTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Workflows/TokenedGitCommandTests.cs @@ -80,4 +80,16 @@ public void An_untokened_command_is_left_as_written() TokenedGitCommand.Spec("https://host/r.git", spec).ShouldBeSameAs(spec, "the operator's helpers may be how an untokened remote authenticates"); } + + [Fact] + public void A_caller_that_knows_a_bare_user_is_a_token_marks_the_command_tokened() + { + // A stored https://user@mirror URL may authenticate through the operator's helper, so Spec leaves it alone; a pasted + // pack URL's bare user is a token, and its caller marks the command itself. + const string url = "https://ghp_pasted@host/r.git"; + var spec = new SandboxSpec { Command = "git", Args = new[] { "clone", url, "/tmp/x" } }; + + TokenedGitCommand.Spec(url, spec).ShouldBeSameAs(spec); + TokenedGitSpecs.RunsTokened(TokenedGitCommand.AsTokened(url, spec), "https://host").ShouldBeTrue(); + } }