From b8a37f1bc5ecf19616de466d2e336ab999eb4f59 Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Wed, 7 Oct 2026 07:02:29 +0800 Subject: [PATCH] Keep pasted pack tokens out of import errors, helpers and config An operator can import an agent pack from a pasted git URL that carries a token in its userinfo: x-access-token:@, oauth2:@, or the token alone as the user. Several places kept it or passed it on. A failed clone's PackImportException named the pasted URL verbatim, and that message reaches the API error body, the UI and the mediator's error log. git hides a password in its own output, but when a token is pasted as the user alone it asks for a password and names that user (decoded by git 2.33, re-encoded by 2.50), so git's stderr carried it as well. The host allowlist echoed a URL it could not parse, and named the scheme it refused; a URL pasted without https:// parses the token before its colon as that scheme. A token pasted as the user alone carries no password, so the clone did not run as a tokened command: git handed the token to the operator's credential helpers when it asked them for the missing password, and wrote it to their trace2 targets. The clone's .git/config held the tokened origin from before the transfer until after the import walked the checkout, under the worker's temp dir with default permissions, and until the janitor's sweep when a worker died mid-import. The failure now names the URL without its userinfo, through the same SanitizeUrl the launch-base resolver uses. git's stderr loses the userinfo of every http(s) URL in it, then the part that carries the credential (the password, or the user when none is given) as bare text in each spelling git echoes it; a user named beside a password, an account and never echoed outside a URL, is left so git's reason stays readable. Neither allowlist refusal echoes the input. The clone runs as a tokened command whenever the URL carries any credential. TokenedGitCommand.IsTokened is unchanged: a stored https://user@mirror workspace URL may authenticate through an operator helper. The clone directory is owner-only before git runs, and once cloned, origin is rewritten through the workspace provider's own fail-closed strip, so a clone that can shed it neither way is refused and deleted. An ssh URL's git@ names an account and is left as written. Not changed here: - A successful import stores the pasted URL, token included, as pack.url, and the pack list and detail queries return it to every team member, Viewers included. Sync and the add-from-sync flow re-clone from that URL, so storing or returning it without the token needs the credential kept server-side (an encrypted reference, a migration, and an add that resolves the pack instead of its URL). - During the clone the token is still in git's argv and in .git/config, now readable by the worker's own uid only. Passing it through the environment as an http.extraHeader would close that, for the workspace provider's clones too. - For a token pasted as the user alone, git with no helper left to ask falls through to a core.askPass the operator's config may set. Verified against git 2.33.0 and 2.50.1. --- .../Services/Agents/PackCloneFetcher.cs | 118 ++++++- .../Services/Agents/PackHostAllowlist.cs | 7 +- .../Agents/Workspace/TokenedGitCommand.cs | 13 +- .../Agents/PackCloneCredentialFlowTests.cs | 287 ++++++++++++++++++ .../Agents/PackCloneFetcherArgsTests.cs | 1 + .../Agents/PackCloneFetcherCredentialTests.cs | 184 +++++++++++ .../Agents/PackHostAllowlistTests.cs | 33 +- .../Workflows/TokenedGitCommandTests.cs | 12 + 8 files changed, 633 insertions(+), 22 deletions(-) create mode 100644 backend/tests/CodeSpace.IntegrationTests/Agents/PackCloneCredentialFlowTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Agents/PackCloneFetcherCredentialTests.cs 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(); + } }