From ce3b1cfe9dd70745a59c1747fb73086dee425a5a Mon Sep 17 00:00:00 2001 From: "Mars.P" Date: Wed, 7 Oct 2026 13:38:47 +0800 Subject: [PATCH] Keep pasted pack tokens out of the stored and returned URL A pack imported from a git URL that embedded a token stored that URL verbatim in pack.url. The pack list and detail returned it to every team member, Viewers included; the Library rendered it as a link; and the add-after-sync flow sent it back to import-url. pack.url now holds the URL without userinfo and is the pack's identity. The URL exactly as cloned is sealed with IPayloadEncryptor in pack.encrypted_clone_url, set only when it carried userinfo. Sync and the new POST /api/packs/{id}/import decrypt it just for the clone, so a private pack keeps syncing and the add imports into the pack by id rather than re-resolving a URL that can no longer clone. Re-pasting with a rotated token now updates the same pack instead of forking one. Both hand the decrypted URL to PackCloneFetcher, so this builds on #2082, which names a failed clone's URL without its userinfo, redacts it from git's stderr and strips it from the checkout's origin. Without it, a Sync or an add whose clone fails after authenticating (a deleted ref, say) returns the token in the error body to any member who may write agents, and logs it. SQL cannot run the encryptor, so existing rows are sealed by a ten-minute recurring backfill. Until it reaches a row, the row still syncs from its URL and the read model strips userinfo on the way out. A re-import of that repository lands in the row and seals it: the lookup also matches a stored URL that differs only by its credential, so pasting the same or a rotated token neither forks the pack nor leaves its history on the pack the backfill would mark a duplicate. Legacy forks of one repository settle into a holder plus duplicates (duplicate_of_pack_id) that keep syncing from their own source. Pods that predate this change cannot read the seal. Until the rollout completes they fail to Sync a sealed private pack, and their import-url fails for a repository whose legacy fork became a holder plus a duplicate. The backfill runs only where Hangfire processes jobs, so in an Api/Worker split roll the Api pods out first. Tokens pasted before this change were already exposed and should be rotated. --- .../Controllers/PacksController.cs | 8 + .../BackfillPackCloneUrlsCommandHandler.cs | 21 ++ .../ImportPackArtifactsCommandHandler.cs | 24 ++ .../PackCloneUrlBackfillRecurringJob.cs | 17 + .../DbUpFiles/0241_pack_sealed_clone_url.sql | 40 +++ .../Persistence/Entities/Pack.cs | 8 +- .../Agents/IPackCloneUrlBackfillService.cs | 19 + .../Services/Agents/IPackCloneUrlProtector.cs | 19 + .../Services/Agents/IPackImportService.cs | 3 + .../Agents/PackCloneUrlBackfillService.cs | 91 +++++ .../Services/Agents/PackCloneUrlProtector.cs | 53 +++ .../Agents/PackImportService.Commit.cs | 92 ++++- .../Services/Agents/PackImportService.Sync.cs | 25 +- .../Services/Agents/PackImportService.cs | 4 +- .../Services/Agents/PackService.cs | 5 +- .../Agents/BackfillPackCloneUrlsCommand.cs | 21 ++ .../Agents/ImportPackArtifactsCommand.cs | 23 ++ .../Agents/PrivatePackCredentialE2ETests.cs | 332 ++++++++++++++++++ .../Infrastructure/GitTestRemoteServer.cs | 12 +- .../Agents/PackCloneUrlBackfillFlowTests.cs | 276 +++++++++++++++ .../Agents/PackImportServiceFlowTests.cs | 3 +- .../Agents/PackSealedSourceFlowTests.cs | 256 ++++++++++++++ .../Agents/PrivatePackSource.cs | 304 ++++++++++++++++ .../Workflows/GitPublishRemoteFixture.cs | 15 + .../SweepCommandTransactionFlowTests.cs | 3 +- .../Agents/PackCloneUrlProtectorTests.cs | 130 +++++++ .../Agents/PackSummaryProjectionTests.cs | 48 +++ .../RequestAuthorizationInventoryTests.cs | 1 + .../Jobs/PackCloneUrlBackfillDispatchTests.cs | 104 ++++++ frontend/src/api/packs.ts | 7 + .../library/SyncResultModal.test.tsx | 79 +++++ .../components/library/SyncResultModal.tsx | 13 +- frontend/src/hooks/use-packs.ts | 32 +- 33 files changed, 2041 insertions(+), 47 deletions(-) create mode 100644 backend/src/CodeSpace.Core/Handlers/CommandHandlers/Agents/BackfillPackCloneUrlsCommandHandler.cs create mode 100644 backend/src/CodeSpace.Core/Handlers/CommandHandlers/Agents/ImportPackArtifactsCommandHandler.cs create mode 100644 backend/src/CodeSpace.Core/Jobs/RecurringJobs/PackCloneUrlBackfillRecurringJob.cs create mode 100644 backend/src/CodeSpace.Core/Persistence/DbUpFiles/0241_pack_sealed_clone_url.sql create mode 100644 backend/src/CodeSpace.Core/Services/Agents/IPackCloneUrlBackfillService.cs create mode 100644 backend/src/CodeSpace.Core/Services/Agents/IPackCloneUrlProtector.cs create mode 100644 backend/src/CodeSpace.Core/Services/Agents/PackCloneUrlBackfillService.cs create mode 100644 backend/src/CodeSpace.Core/Services/Agents/PackCloneUrlProtector.cs create mode 100644 backend/src/CodeSpace.Messages/Commands/Agents/BackfillPackCloneUrlsCommand.cs create mode 100644 backend/src/CodeSpace.Messages/Commands/Agents/ImportPackArtifactsCommand.cs create mode 100644 backend/tests/CodeSpace.E2ETests/Agents/PrivatePackCredentialE2ETests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Agents/PackCloneUrlBackfillFlowTests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Agents/PackSealedSourceFlowTests.cs create mode 100644 backend/tests/CodeSpace.IntegrationTests/Agents/PrivatePackSource.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Agents/PackCloneUrlProtectorTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Agents/PackSummaryProjectionTests.cs create mode 100644 backend/tests/CodeSpace.UnitTests/Jobs/PackCloneUrlBackfillDispatchTests.cs create mode 100644 frontend/src/components/library/SyncResultModal.test.tsx diff --git a/backend/src/CodeSpace.Api/Controllers/PacksController.cs b/backend/src/CodeSpace.Api/Controllers/PacksController.cs index 54d42149b..77ad19795 100644 --- a/backend/src/CodeSpace.Api/Controllers/PacksController.cs +++ b/backend/src/CodeSpace.Api/Controllers/PacksController.cs @@ -48,4 +48,12 @@ public async Task Sync([FromRoute] Guid packId, CancellationToken var result = await _mediator.Send(new SyncPackCommand { PackId = packId }, cancellationToken).ConfigureAwait(false); return Ok(result); } + + /// Add the selected artifacts a Sync discovered to this pack, cloned from the pack's saved source and ref — the body carries only { "sourcePaths": [...] }, never a URL. The route's id is authoritative. + [HttpPost("{packId:guid}/import")] + public async Task Import([FromRoute] Guid packId, [FromBody] ImportPackArtifactsCommand command, CancellationToken cancellationToken) + { + var result = await _mediator.Send(command with { PackId = packId }, cancellationToken).ConfigureAwait(false); + return Ok(result); + } } diff --git a/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Agents/BackfillPackCloneUrlsCommandHandler.cs b/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Agents/BackfillPackCloneUrlsCommandHandler.cs new file mode 100644 index 000000000..ccfb47e99 --- /dev/null +++ b/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Agents/BackfillPackCloneUrlsCommandHandler.cs @@ -0,0 +1,21 @@ +using CodeSpace.Core.Services.Agents; +using CodeSpace.Messages.Commands.Agents; +using MediatR; + +namespace CodeSpace.Core.Handlers.CommandHandlers.Agents; + +/// Thin dispatcher (Rule 16) — the production caller of . +public sealed class BackfillPackCloneUrlsCommandHandler : IRequestHandler +{ + private readonly IPackCloneUrlBackfillService _backfill; + + public BackfillPackCloneUrlsCommandHandler(IPackCloneUrlBackfillService backfill) + { + _backfill = backfill; + } + + public async Task Handle(BackfillPackCloneUrlsCommand request, CancellationToken cancellationToken) + { + return await _backfill.BackfillAsync(request.BatchSize, cancellationToken).ConfigureAwait(false); + } +} diff --git a/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Agents/ImportPackArtifactsCommandHandler.cs b/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Agents/ImportPackArtifactsCommandHandler.cs new file mode 100644 index 000000000..2554cdf99 --- /dev/null +++ b/backend/src/CodeSpace.Core/Handlers/CommandHandlers/Agents/ImportPackArtifactsCommandHandler.cs @@ -0,0 +1,24 @@ +using CodeSpace.Core.Services.Agents; +using CodeSpace.Core.Services.Identity; +using CodeSpace.Messages.Agents; +using CodeSpace.Messages.Commands.Agents; +using MediatR; + +namespace CodeSpace.Core.Handlers.CommandHandlers.Agents; + +public sealed class ImportPackArtifactsCommandHandler : IRequestHandler +{ + private readonly IPackImportService _service; + private readonly ICurrentTeam _currentTeam; + private readonly ICurrentUser _currentUser; + + public ImportPackArtifactsCommandHandler(IPackImportService service, ICurrentTeam currentTeam, ICurrentUser currentUser) + { + _service = service; + _currentTeam = currentTeam; + _currentUser = currentUser; + } + + public Task Handle(ImportPackArtifactsCommand request, CancellationToken cancellationToken) => + _service.ImportFromPackAsync(_currentTeam.Id!.Value, request.PackId, request.SourcePaths, _currentUser.Id!.Value, cancellationToken); +} diff --git a/backend/src/CodeSpace.Core/Jobs/RecurringJobs/PackCloneUrlBackfillRecurringJob.cs b/backend/src/CodeSpace.Core/Jobs/RecurringJobs/PackCloneUrlBackfillRecurringJob.cs new file mode 100644 index 000000000..1640ef02a --- /dev/null +++ b/backend/src/CodeSpace.Core/Jobs/RecurringJobs/PackCloneUrlBackfillRecurringJob.cs @@ -0,0 +1,17 @@ +using CodeSpace.Messages.Commands.Agents; +using MediatR; + +namespace CodeSpace.Core.Jobs.RecurringJobs; + +/// Every ten minutes: seal the clone URL of any pack row that still holds a pasted token in plaintext — the rows imported before the seal existed, and any an older pod writes during a rolling deploy (thin Rule-14 dispatcher). A sealed row is no longer a candidate, so an idle tick is one small read. +public sealed class PackCloneUrlBackfillRecurringJob : IRecurringJob +{ + private readonly IMediator _mediator; + + public PackCloneUrlBackfillRecurringJob(IMediator mediator) { _mediator = mediator; } + + public string JobId => nameof(PackCloneUrlBackfillRecurringJob); + public string CronExpression => "*/10 * * * *"; + + public async Task Execute() => await _mediator.Send(new BackfillPackCloneUrlsCommand()).ConfigureAwait(false); +} diff --git a/backend/src/CodeSpace.Core/Persistence/DbUpFiles/0241_pack_sealed_clone_url.sql b/backend/src/CodeSpace.Core/Persistence/DbUpFiles/0241_pack_sealed_clone_url.sql new file mode 100644 index 000000000..27250587a --- /dev/null +++ b/backend/src/CodeSpace.Core/Persistence/DbUpFiles/0241_pack_sealed_clone_url.sql @@ -0,0 +1,40 @@ +-- 0241_pack_sealed_clone_url.sql +-- +-- A pack imported from a pasted git URL that embedded a token stored that URL verbatim in pack.url, which every team +-- member (Viewers included) reads and the Library renders as a link. From now on pack.url holds the URL with its +-- userinfo removed, and the URL exactly as cloned is sealed with the platform's credential encryptor +-- (IPayloadEncryptor) in encrypted_clone_url, which Sync and import-from-pack decrypt just in time and nothing returns. +-- +-- SQL cannot run that encryptor, so existing rows are sealed by the application: PackCloneUrlBackfillRecurringJob +-- rewrites each one with a conditional UPDATE, and a re-import of the same repository seals the row it lands in. Until +-- then, Sync keeps working on an unsealed row (its url still carries the token it needs), and the read model strips +-- userinfo on the way out. +-- +-- duplicate_of_pack_id marks a legacy fork: the same repository imported once with a token and once without (or with +-- two tokens) became two packs, whose urls collide once both are credential-free. The clean pack (else the oldest) +-- holds the source identity; the other keeps its artifacts and its own sealed source, points at the holder, and +-- leaves the unique index — so the index is recreated with that predicate. Its predicate covers a subset of the rows the old +-- index did, so recreating it cannot fail on existing data. +-- +-- Additive: two nullable columns. Idempotent (IF NOT EXISTS / IF EXISTS). An older pod ignores the columns but not what +-- the new code writes into the rows: until the rollout completes it cannot Sync a sealed private pack (it clones +-- pack.url, which no longer carries the token), and its import-url fails ("more than one element") for a repository whose +-- legacy fork has become a holder plus a duplicate. In an Api/Worker split, roll the Api pods out before the Worker pods, +-- which run the backfill. + +ALTER TABLE pack ADD COLUMN IF NOT EXISTS encrypted_clone_url TEXT NULL; + +ALTER TABLE pack ADD COLUMN IF NOT EXISTS duplicate_of_pack_id UUID NULL REFERENCES pack(id); + +DROP INDEX IF EXISTS uq_pack_team_source; + +CREATE UNIQUE INDEX IF NOT EXISTS uq_pack_team_source + ON pack(team_id, url, COALESCE(subpath, '')) WHERE deleted_date IS NULL AND url IS NOT NULL AND duplicate_of_pack_id IS NULL; + +COMMENT ON COLUMN pack.encrypted_clone_url IS + 'The URL the pack''s last successful import cloned, sealed with IPayloadEncryptor (purpose CodeSpace.Credentials.v1); ' + 'set only when that URL carried userinfo. pack.url is the same URL without it. Never returned by any API.'; + +COMMENT ON COLUMN pack.duplicate_of_pack_id IS + 'The pack holding this pack''s source identity when a legacy import forked one repository into two packs. ' + 'A duplicate keeps syncing from its own sealed source and is excluded from uq_pack_team_source.'; diff --git a/backend/src/CodeSpace.Core/Persistence/Entities/Pack.cs b/backend/src/CodeSpace.Core/Persistence/Entities/Pack.cs index e26dbe1bc..02ae39c16 100644 --- a/backend/src/CodeSpace.Core/Persistence/Entities/Pack.cs +++ b/backend/src/CodeSpace.Core/Persistence/Entities/Pack.cs @@ -25,9 +25,15 @@ public class Pack : IEntity, IAuditable /// Human-readable library name (e.g. the repo name) — what the UI groups skills under. public string Name { get; set; } = default!; - /// The source location: owner/repo for or a clone URL for . NULL for the pack. + /// The source location: owner/repo for or a clone URL for . NULL for the pack. Never carries a credential: it is shown to every team member and is the pack's identity, so a pasted URL's userinfo lives sealed in . public string? Url { get; set; } + /// The URL the last successful import cloned, sealed with IPayloadEncryptor — present only when that URL carried userinfo (a pasted token). Sync and import-from-pack clone from it; nothing returns it. Read and written only through IPackCloneUrlProtector. + public string? EncryptedCloneUrl { get; set; } + + /// The pack that holds this pack's source identity, when an earlier import forked the same repository into two packs (one with a token and one without, or with two tokens). A duplicate keeps syncing from its own source; a new import resolves to the holder. NULL for every other pack. + public Guid? DuplicateOfPackId { get; set; } + /// The git ref synced (branch / tag / commit). NULL → the source's default branch. public string? Reference { get; set; } diff --git a/backend/src/CodeSpace.Core/Services/Agents/IPackCloneUrlBackfillService.cs b/backend/src/CodeSpace.Core/Services/Agents/IPackCloneUrlBackfillService.cs new file mode 100644 index 000000000..a80f6b9ee --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Agents/IPackCloneUrlBackfillService.cs @@ -0,0 +1,19 @@ +namespace CodeSpace.Core.Services.Agents; + +/// +/// Seals the clone URL of pack rows that still hold a credential in plaintext: every row imported before the pack kept +/// its pasted token sealed, soft-deleted ones included, and any an older pod writes during a rolling deploy. On a pod +/// running this code each row keeps syncing throughout — before its seal it clones from its URL, after it from the +/// sealed copy. +/// +/// A pod that predates the seal cannot read it. Until the rollout completes, such a pod fails to Sync a sealed +/// private pack (it clones pack.url, which no longer carries the token), and its import-url fails for a +/// repository whose legacy fork this pass turned into a holder plus a duplicate (two active rows under one URL). This +/// job runs only where Hangfire processes jobs, so in an Api/Worker split, rolling the Api pods out before the Worker +/// pods keeps the backfill from sealing anything an old Api pod can still serve. +/// +public interface IPackCloneUrlBackfillService +{ + /// Seal up to credential-carrying rows. Idempotent and safe on several workers at once: a sealed row is no longer a candidate, and each write is conditional on the row still holding the URL it read. Returns how many rows this pass sealed. + Task BackfillAsync(int batchSize, CancellationToken cancellationToken); +} diff --git a/backend/src/CodeSpace.Core/Services/Agents/IPackCloneUrlProtector.cs b/backend/src/CodeSpace.Core/Services/Agents/IPackCloneUrlProtector.cs new file mode 100644 index 000000000..c4ca86391 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Agents/IPackCloneUrlProtector.cs @@ -0,0 +1,19 @@ +using CodeSpace.Core.Persistence.Entities; + +namespace CodeSpace.Core.Services.Agents; + +/// +/// Splits a pack's clone URL into the credential-free URL the pack stores, shows and is identified by, and the sealed +/// URL it clones from. A pasted git URL may embed a token in its userinfo; that URL is what the import clones, and it +/// is the only thing that can clone a private pack again — but pack.url reaches every team member. The split +/// keeps the clone lossless (Sync clones the exact string the import cloned) without the token ever being stored, +/// returned or rendered in plaintext. +/// +public interface IPackCloneUrlProtector +{ + /// The URL a pack stores for (its userinfo removed; byte-identical when it carries none) and, only when it carried userinfo, the whole URL sealed. + (string Url, string? EncryptedCloneUrl) Seal(string cloneUrl); + + /// The URL to clone from: its sealed source decrypted, or its URL when it has none. Throws , naming neither URL nor ciphertext, when the sealed source can no longer be read. + string CloneUrlOf(Pack pack); +} diff --git a/backend/src/CodeSpace.Core/Services/Agents/IPackImportService.cs b/backend/src/CodeSpace.Core/Services/Agents/IPackImportService.cs index 010bee9bd..3bc3df8b0 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/IPackImportService.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/IPackImportService.cs @@ -18,4 +18,7 @@ public interface IPackImportService /// Re-pull the pack from its saved source: refresh every already-imported artifact in place (kept handles) and return what changed plus the discovered-but-not-imported artifacts as a preview to add. Task SyncAsync(Guid teamId, Guid packId, Guid actorUserId, CancellationToken cancellationToken); + + /// Re-clone the pack from its saved source at its saved ref and persist exactly the chosen into THAT pack — the add-new step after a Sync. The pack is never resolved again by URL, so a private pack (whose stored URL cannot clone) and a legacy duplicate both import into themselves. Returns a per-path outcome. + Task ImportFromPackAsync(Guid teamId, Guid packId, IReadOnlyList sourcePaths, Guid actorUserId, CancellationToken cancellationToken); } diff --git a/backend/src/CodeSpace.Core/Services/Agents/PackCloneUrlBackfillService.cs b/backend/src/CodeSpace.Core/Services/Agents/PackCloneUrlBackfillService.cs new file mode 100644 index 000000000..71fec41f7 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Agents/PackCloneUrlBackfillService.cs @@ -0,0 +1,91 @@ +using CodeSpace.Core.DependencyInjection; +using CodeSpace.Core.Persistence.Db; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; + +namespace CodeSpace.Core.Services.Agents; + +/// +/// The bounded seal pass behind . A candidate is a row whose URL carries +/// userinfo, judged by the writer's own rule () in memory BEFORE +/// the batch is cut, so a row with an '@' only in its path never crowds out a real one. A sealed row's URL no longer +/// carries userinfo, so it leaves the set: the pass is self-terminating. +/// +/// Each row is written by one UPDATE conditional on its id AND the URL the pass read, so a second worker sealing +/// the same row matches nothing and the ciphertext is written once. When sealing makes an active row's URL equal to +/// another active pack's (a legacy fork of one repository), the row becomes that pack's duplicate; when two rows of one +/// group race to hold it, the unique index refuses one, and the next pass finds the holder. No lock is needed. +/// +/// Per-row try/catch: a failing row stays a candidate and never aborts the pass. The log names the pack and the +/// team, never the URL. +/// +public sealed class PackCloneUrlBackfillService : IPackCloneUrlBackfillService, IScopedDependency +{ + private readonly CodeSpaceDbContext _db; + private readonly IPackCloneUrlProtector _protector; + private readonly ILogger _logger; + + public PackCloneUrlBackfillService(CodeSpaceDbContext db, IPackCloneUrlProtector protector, ILogger logger) + { + _db = db; + _protector = protector; + _logger = logger; + } + + public async Task BackfillAsync(int batchSize, CancellationToken cancellationToken) + { + var candidates = await LoadCandidatesAsync(batchSize, cancellationToken).ConfigureAwait(false); + + var sealedCount = 0; + + foreach (var candidate in candidates) + { + try + { + if (await SealAsync(candidate, cancellationToken).ConfigureAwait(false)) sealedCount++; + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + _logger.LogWarning("Pack clone-URL backfill failed for pack {PackId} in team {TeamId} ({ExceptionType}); the pass continues — the pack stays a candidate", candidate.Id, candidate.TeamId, ex.GetType().Name); + } + } + + return sealedCount; + } + + /// The oldest rows, active or soft-deleted, whose URL carries a credential. The '@' filter is a sound superset (userinfo needs one) that keeps the read small; the real rule runs in memory before the batch is cut. + private async Task> LoadCandidatesAsync(int batchSize, CancellationToken cancellationToken) + { + var rows = await _db.Pack.AsNoTracking() + .Where(p => p.Url != null && p.Url.Contains("@")) + .OrderBy(p => p.CreatedDate).ThenBy(p => p.Id) + .Select(p => new Candidate(p.Id, p.TeamId, p.Url!, p.Subpath, p.DeletedDate == null)) + .ToListAsync(cancellationToken).ConfigureAwait(false); + + return rows.Where(r => PackCloneUrlProtector.CarriesCredential(r.Url)).Take(batchSize).ToList(); + } + + /// Rewrite one row to its credential-free URL plus its sealed original, marking it a duplicate when another active pack already holds that URL. False when another worker sealed it first. + private async Task SealAsync(Candidate candidate, CancellationToken cancellationToken) + { + var source = _protector.Seal(candidate.Url); + + var holderId = candidate.IsActive ? await FindHolderAsync(candidate, source.Url, cancellationToken).ConfigureAwait(false) : null; + + var written = await _db.Pack + .Where(p => p.Id == candidate.Id && p.Url == candidate.Url) + .ExecuteUpdateAsync(set => set.SetProperty(p => p.Url, source.Url).SetProperty(p => p.EncryptedCloneUrl, source.EncryptedCloneUrl).SetProperty(p => p.DuplicateOfPackId, holderId), cancellationToken).ConfigureAwait(false); + + return written == 1; + } + + /// The active pack that already holds as its source identity in the candidate's team and subpath — the one a sealed duplicate points at. + private async Task FindHolderAsync(Candidate candidate, string cleanUrl, CancellationToken cancellationToken) => + await _db.Pack.AsNoTracking() + .Where(p => p.TeamId == candidate.TeamId && p.Url == cleanUrl && (p.Subpath ?? "") == (candidate.Subpath ?? "") && p.DuplicateOfPackId == null && p.DeletedDate == null && p.Id != candidate.Id) + .OrderBy(p => p.CreatedDate).ThenBy(p => p.Id) + .Select(p => (Guid?)p.Id) + .FirstOrDefaultAsync(cancellationToken).ConfigureAwait(false); + + private sealed record Candidate(Guid Id, Guid TeamId, string Url, string? Subpath, bool IsActive); +} diff --git a/backend/src/CodeSpace.Core/Services/Agents/PackCloneUrlProtector.cs b/backend/src/CodeSpace.Core/Services/Agents/PackCloneUrlProtector.cs new file mode 100644 index 000000000..b9440b748 --- /dev/null +++ b/backend/src/CodeSpace.Core/Services/Agents/PackCloneUrlProtector.cs @@ -0,0 +1,53 @@ +using System.Security.Cryptography; +using CodeSpace.Core.DependencyInjection; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Agents.Workspace; +using CodeSpace.Core.Services.Credentials; + +namespace CodeSpace.Core.Services.Agents; + +/// +/// over the platform's credential encryptor (, whose +/// key ring every pod shares) — the same storage a model API key or a webhook secret has. One rule decides whether a +/// URL carries a credential: removing its userinfo changes it. That covers a token pasted as the password and one +/// pasted as the user alone. +/// +/// The WHOLE URL is sealed, not just its userinfo: removing the userinfo also normalizes the URL (host case, +/// escaping), so recomposing it would clone a different string than the import cloned. +/// +public sealed class PackCloneUrlProtector : IPackCloneUrlProtector, ISingletonDependency +{ + private const string UnreadableSourceMessage = "This pack's saved source credential can no longer be read; import it again from a URL with a current token."; + + private readonly IPayloadEncryptor _encryptor; + + public PackCloneUrlProtector(IPayloadEncryptor encryptor) { _encryptor = encryptor; } + + public (string Url, string? EncryptedCloneUrl) Seal(string cloneUrl) + { + var url = WithoutCredential(cloneUrl); + + return (url, url == cloneUrl ? null : _encryptor.Encrypt(cloneUrl)); + } + + public string CloneUrlOf(Pack pack) + { + if (pack.EncryptedCloneUrl is null) return pack.Url!; + + try + { + return _encryptor.Decrypt(pack.EncryptedCloneUrl); + } + catch (CryptographicException) + { + // The inner exception is dropped on purpose: it can quote the payload, and this message reaches the API. + throw new PackImportException(UnreadableSourceMessage); + } + } + + /// with its userinfo removed; unchanged when it carries none (or is not an absolute URL). Pure + internal so the read model and the backfill share the writer's rule. + internal static string WithoutCredential(string url) => RemoteTipResolver.SanitizeUrl(url); + + /// True when carries userinfo — the backfill's candidate test. Pure + internal so it is unit-pinned against . + internal static bool CarriesCredential(string url) => WithoutCredential(url) != url; +} diff --git a/backend/src/CodeSpace.Core/Services/Agents/PackImportService.Commit.cs b/backend/src/CodeSpace.Core/Services/Agents/PackImportService.Commit.cs index a0ebda1b4..c1b4a7c80 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/PackImportService.Commit.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/PackImportService.Commit.cs @@ -13,6 +13,10 @@ namespace CodeSpace.Core.Services.Agents; /// duplicates. Agent + skill row-writing is the import concern's own logic; slug derivation reuses /// . /// +/// The pack stores the URL without its credential; a pasted token is kept only sealed +/// (), and the add-after-sync path imports by pack id from that sealed source, +/// never by resolving a URL again. +/// /// ATOMIC by design — the whole import is ONE SaveChangesAsync inside the command's ambient /// transaction (TransactionalBehavior). Known handle collisions are decided IN MEMORY before the save /// (a handle that already belongs to a DIFFERENT active definition, or a second artifact in the same pack that @@ -29,7 +33,7 @@ public sealed partial class PackImportService { public async Task ImportFromUrlAsync(string url, string? reference, IReadOnlyList sourcePaths, Guid teamId, Guid actorUserId, CancellationToken cancellationToken) { - var selected = sourcePaths.Distinct(StringComparer.Ordinal).OrderBy(p => p, StringComparer.Ordinal).ToList(); + var selected = NormalizeSelection(sourcePaths); // Nothing selected → a clean no-op: never clone or touch a Pack for an empty commit. if (selected.Count == 0) return new PackImportResult { PackId = Guid.Empty, Items = Array.Empty() }; @@ -38,8 +42,38 @@ public async Task ImportFromUrlAsync(string url, string? refer var discovered = await _walker.WalkAsync(checkout.Directory, cancellationToken).ConfigureAwait(false); + // The pasted URL is what cloned; the pack stores it without its credential and keeps the original sealed. + var source = _protector.Seal(url); var now = DateTimeOffset.UtcNow; - var (pack, packIsNew) = await ResolvePackTargetAsync(url, reference, teamId, actorUserId, now, cancellationToken).ConfigureAwait(false); + var (pack, packIsNew) = await ResolvePackTargetAsync(source.Url, reference, teamId, actorUserId, now, cancellationToken).ConfigureAwait(false); + + return await LandSelectionAsync(new ImportTarget(pack, packIsNew, reference, source, actorUserId, now), discovered, selected, cancellationToken).ConfigureAwait(false); + } + + public async Task ImportFromPackAsync(Guid teamId, Guid packId, IReadOnlyList sourcePaths, Guid actorUserId, CancellationToken cancellationToken) + { + var selected = NormalizeSelection(sourcePaths); + + var pack = await LoadRemotePackAsync(teamId, packId, cancellationToken).ConfigureAwait(false); + + if (selected.Count == 0) return new PackImportResult { PackId = pack.Id, Items = Array.Empty() }; + + using var checkout = await _fetcher.FetchAsync(_protector.CloneUrlOf(pack), pack.Reference, cancellationToken).ConfigureAwait(false); + + var discovered = await _walker.WalkAsync(checkout.Directory, cancellationToken).ConfigureAwait(false); + + // The pack's own source and ref cloned, so they are recorded unchanged. + return await LandSelectionAsync(new ImportTarget(pack, IsNew: false, pack.Reference, (pack.Url!, pack.EncryptedCloneUrl), actorUserId, DateTimeOffset.UtcNow), discovered, selected, cancellationToken).ConfigureAwait(false); + } + + /// The selection as a distinct, ordinal-sorted list — the order every import upserts in. + private static List NormalizeSelection(IReadOnlyList sourcePaths) => + sourcePaths.Distinct(StringComparer.Ordinal).OrderBy(p => p, StringComparer.Ordinal).ToList(); + + /// Upsert each selected artifact of into the target pack on its (pack, source-path) sync identity and, when at least one landed, stage the pack write and the declared-skill bindings and save once. + private async Task LandSelectionAsync(ImportTarget target, DiscoveredPack discovered, IReadOnlyList selected, CancellationToken cancellationToken) + { + var (pack, teamId, actorUserId, now) = (target.Pack, target.Pack.TeamId, target.ActorUserId, target.Now); var agentsByPath = IndexByPath(discovered.Agents, a => a.SourcePath); var skillsByPath = IndexByPath(discovered.Skills, s => s.SourcePath); @@ -49,8 +83,8 @@ public async Task ImportFromUrlAsync(string url, string? refer // never picked up here — otherwise the lookup would match that live bench agent and the upsert would take the // UPDATE branch, clobbering its content (and creating no snapshot) instead of adding a fresh Store snapshot // beside it. A brand-new pack has none yet, so skip the lookup. - var existingAgents = packIsNew ? EmptyByPath() : await ExistingByPathAsync(_db.AgentDefinition.Where(a => a.PackId == pack.Id && a.Scope == DefinitionScope.Store && a.DeletedDate == null), a => a.SourcePath!, cancellationToken).ConfigureAwait(false); - var existingSkills = packIsNew ? EmptyByPath() : await ExistingByPathAsync(_db.SkillDefinition.Where(s => s.PackId == pack.Id && s.Scope == DefinitionScope.Store && s.DeletedDate == null), s => s.SourcePath!, cancellationToken).ConfigureAwait(false); + var existingAgents = target.IsNew ? EmptyByPath() : await ExistingByPathAsync(_db.AgentDefinition.Where(a => a.PackId == pack.Id && a.Scope == DefinitionScope.Store && a.DeletedDate == null), a => a.SourcePath!, cancellationToken).ConfigureAwait(false); + var existingSkills = target.IsNew ? EmptyByPath() : await ExistingByPathAsync(_db.SkillDefinition.Where(s => s.PackId == pack.Id && s.Scope == DefinitionScope.Store && s.DeletedDate == null), s => s.SourcePath!, cancellationToken).ConfigureAwait(false); // Skill handles → id, loaded once and EXTENDED in memory as new skills are added this batch, so an imported // agent's declared skills resolve against this map (existing + same-batch). Store snapshots are no longer @@ -80,11 +114,14 @@ public async Task ImportFromUrlAsync(string url, string? refer } // Only create/touch the Pack when something actually landed — an all-Skipped/all-Failed selection leaves - // no phantom library and no misleading sync timestamp. + // no phantom library, no misleading sync timestamp, and no changed clone source (the command's transaction + // saves every tracked change, so nothing may be staged on the pack before this point). if (!items.Any(i => i.Outcome is PackImportOutcome.Imported or PackImportOutcome.Updated)) - return new PackImportResult { PackId = packIsNew ? Guid.Empty : pack.Id, Items = items }; + return new PackImportResult { PackId = target.IsNew ? Guid.Empty : pack.Id, Items = items }; - PersistPackSync(pack, packIsNew, reference, actorUserId, now); + PersistPackSync(target); + + RecordCloneSource(target); BindDeclaredSkills(newAgentSkills, skillSlugToId, actorUserId, now); @@ -93,10 +130,10 @@ public async Task ImportFromUrlAsync(string url, string? refer return new PackImportResult { PackId = pack.Id, Items = items }; } - /// Resolve the team's active pack for this source (one per team+url+subpath, per the unique index), or build a NEW unsaved one. Read-only: neither mutates nor adds — the actual write is deferred to so a no-op commit never touches a pack. The URL flow has no subpath (the whole repo). + /// Resolve the team's active pack for this source (), or build a NEW unsaved one. is the credential-free URL, so a re-paste with a rotated token resolves to the SAME pack, and a legacy duplicate is never the match (a new import lands on the pack holding the source). Read-only: neither mutates nor adds — the actual write is deferred to and so a no-op commit never touches a pack. The URL flow has no subpath (the whole repo). private async Task<(Pack Pack, bool IsNew)> ResolvePackTargetAsync(string url, string? reference, Guid teamId, Guid actorUserId, DateTimeOffset now, CancellationToken cancellationToken) { - var existing = await _db.Pack.SingleOrDefaultAsync(p => p.TeamId == teamId && p.Url == url && p.Subpath == null && p.DeletedDate == null, cancellationToken).ConfigureAwait(false); + var existing = await FindSourcePackAsync(teamId, url, cancellationToken).ConfigureAwait(false); if (existing != null) return (existing, false); @@ -118,21 +155,44 @@ public async Task ImportFromUrlAsync(string url, string? refer return (pack, true); } + /// + /// The team's active pack holding (credential-free) as its source: the one stored under it, else the + /// oldest whose URL is it plus a pasted credential — a row the clone-URL backfill has not sealed yet. A re-paste of the same + /// or a rotated token lands in that row and seals it () instead of creating a pack the + /// backfill would then make the row's holder. The stored match wins, since sealing an unsealed fork beside it would collide + /// on the source index; oldest-first is the order the backfill picks a holder in. '@' is the backfill's own sound prefilter. + /// + private async Task FindSourcePackAsync(Guid teamId, string url, CancellationToken cancellationToken) + { + var candidates = await _db.Pack + .Where(p => p.TeamId == teamId && p.Subpath == null && p.DuplicateOfPackId == null && p.DeletedDate == null && (p.Url == url || p.Url!.Contains("@"))) + .OrderBy(p => p.CreatedDate).ThenBy(p => p.Id) + .ToListAsync(cancellationToken).ConfigureAwait(false); + + return candidates.FirstOrDefault(p => p.Url == url) ?? candidates.FirstOrDefault(p => PackCloneUrlProtector.WithoutCredential(p.Url!) == url); + } + /// Stage the pack write that accompanies a non-empty import: add the new pack, or refresh the existing pack's ref + sync timestamp. Flushed by the single import save alongside the artifact rows. - private void PersistPackSync(Pack pack, bool isNew, string? reference, Guid actorUserId, DateTimeOffset now) + private void PersistPackSync(ImportTarget target) { - if (isNew) + if (target.IsNew) { - _db.Pack.Add(pack); + _db.Pack.Add(target.Pack); return; } - pack.Reference = reference; - pack.LastSyncedDate = now; - pack.LastModifiedDate = now; - pack.LastModifiedBy = actorUserId; + target.Pack.Reference = target.Reference; + target.Pack.LastSyncedDate = target.Now; + target.Pack.LastModifiedDate = target.Now; + target.Pack.LastModifiedBy = target.ActorUserId; } + /// The pack's source is the URL its last successful import cloned: a tokened paste replaces the sealed credential (a rotated token), a clean paste that cloned clears it (the repository was readable without one), and an unsealed legacy row it landed in gets its credential-free URL. + private static void RecordCloneSource(ImportTarget target) => (target.Pack.Url, target.Pack.EncryptedCloneUrl) = target.Source; + + /// Where and as whom a selection lands: the resolved pack (new or existing), the ref and source (the credential-free URL and the sealed one) that cloned it, the actor, and the import's timestamp. + private sealed record ImportTarget(Pack Pack, bool IsNew, string? Reference, (string Url, string? EncryptedCloneUrl) Source, Guid ActorUserId, DateTimeOffset Now); + private PackArtifactImportResult UpsertAgent(Pack pack, ParsedAgentDefinition parsed, IReadOnlyDictionary existingByPath, Guid teamId, Guid actorUserId, DateTimeOffset now) { if (existingByPath.TryGetValue(parsed.SourcePath, out var existing)) diff --git a/backend/src/CodeSpace.Core/Services/Agents/PackImportService.Sync.cs b/backend/src/CodeSpace.Core/Services/Agents/PackImportService.Sync.cs index 1066dcda3..e72eb61af 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/PackImportService.Sync.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/PackImportService.Sync.cs @@ -7,8 +7,9 @@ namespace CodeSpace.Core.Services.Agents; /// -/// The SYNC half of — the store's Sync button. Re-clones the pack's SAVED url+ref -/// (the same allowlist-guarded fetch the import uses), re-walks it, and REFRESHES every already-imported artifact +/// The SYNC half of — the store's Sync button. Re-clones the pack's SAVED source+ref +/// (the same allowlist-guarded fetch the import uses; a private pack's sealed URL is decrypted just for the clone and +/// never leaves this call), re-walks it, and REFRESHES every already-imported artifact /// in place: its content is re-applied only when a projected field actually changed (so the result honestly /// splits up-to-date vs updated), and the handle never moves. Discovered artifacts NOT yet imported are returned /// as a for the operator to select + add — a sync never auto-imports anything new. @@ -27,13 +28,9 @@ public sealed partial class PackImportService { public async Task SyncAsync(Guid teamId, Guid packId, Guid actorUserId, CancellationToken cancellationToken) { - var pack = await _db.Pack.SingleOrDefaultAsync(p => p.Id == packId && p.TeamId == teamId && p.DeletedDate == null, cancellationToken).ConfigureAwait(false) - ?? throw new KeyNotFoundException($"Pack {packId} not found or not accessible."); + var pack = await LoadRemotePackAsync(teamId, packId, cancellationToken).ConfigureAwait(false); - if (string.IsNullOrWhiteSpace(pack.Url)) - throw new PackImportException("This pack has no remote source to sync from."); - - using var checkout = await _fetcher.FetchAsync(pack.Url, pack.Reference, cancellationToken).ConfigureAwait(false); + using var checkout = await _fetcher.FetchAsync(_protector.CloneUrlOf(pack), pack.Reference, cancellationToken).ConfigureAwait(false); var discovered = await _walker.WalkAsync(checkout.Directory, cancellationToken).ConfigureAwait(false); @@ -93,6 +90,18 @@ public async Task SyncAsync(Guid teamId, Guid packId, Guid actor }; } + /// The team's active pack (tracked), refusing one with no remote source — what Sync and import-from-pack clone. Another team's pack is not found, never leaked. + private async Task LoadRemotePackAsync(Guid teamId, Guid packId, CancellationToken cancellationToken) + { + var pack = await _db.Pack.SingleOrDefaultAsync(p => p.Id == packId && p.TeamId == teamId && p.DeletedDate == null, cancellationToken).ConfigureAwait(false) + ?? throw new KeyNotFoundException($"Pack {packId} not found or not accessible."); + + if (string.IsNullOrWhiteSpace(pack.Url)) + throw new PackImportException("This pack has no remote source to sync from."); + + return pack; + } + /// True when the persisted agent matches the parsed artifact on its PROJECTED fields, so re-applying would be a no-op. RawFrontmatter is excluded — jsonb round-trips reorder keys, so comparing it would falsely report "updated"; a refresh re-applies it anyway whenever a projected field changes. private static bool AgentContentEquals(AgentDefinition row, ParsedAgentDefinition parsed) => row.Name == parsed.Name diff --git a/backend/src/CodeSpace.Core/Services/Agents/PackImportService.cs b/backend/src/CodeSpace.Core/Services/Agents/PackImportService.cs index 77b77711f..0ea115301 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/PackImportService.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/PackImportService.cs @@ -18,12 +18,14 @@ public sealed partial class PackImportService : IPackImportService, IScopedDepen private readonly IPackSourceFetcher _fetcher; private readonly IPackSourceWalker _walker; private readonly CodeSpaceDbContext _db; + private readonly IPackCloneUrlProtector _protector; - public PackImportService(IPackSourceFetcher fetcher, IPackSourceWalker walker, CodeSpaceDbContext db) + public PackImportService(IPackSourceFetcher fetcher, IPackSourceWalker walker, CodeSpaceDbContext db, IPackCloneUrlProtector protector) { _fetcher = fetcher; _walker = walker; _db = db; + _protector = protector; } public async Task PreviewFromUrlAsync(string url, string? reference, Guid teamId, CancellationToken cancellationToken) diff --git a/backend/src/CodeSpace.Core/Services/Agents/PackService.cs b/backend/src/CodeSpace.Core/Services/Agents/PackService.cs index 9e9da2dfc..130045317 100644 --- a/backend/src/CodeSpace.Core/Services/Agents/PackService.cs +++ b/backend/src/CodeSpace.Core/Services/Agents/PackService.cs @@ -101,12 +101,13 @@ private static async Task> CountByPackAsync(IQueryable r.PackId, r => r.Count); } - private static PackSummary ToSummary(Persistence.Entities.Pack pack, int agentCount, int skillCount) => new() + /// The read model every team member — Viewers included — receives. The URL is stripped of userinfo even though the writer already stores it clean: a legacy row the clone-URL backfill has not sealed yet still holds its token, and this closes the API the moment the code deploys. Internal so it is unit-pinned. + internal static PackSummary ToSummary(Persistence.Entities.Pack pack, int agentCount, int skillCount) => new() { Id = pack.Id, Kind = pack.Kind, Name = pack.Name, - Url = pack.Url, + Url = pack.Url is null ? null : PackCloneUrlProtector.WithoutCredential(pack.Url), Reference = pack.Reference, LastSyncedSha = pack.LastSyncedSha, LastSyncedDate = pack.LastSyncedDate, diff --git a/backend/src/CodeSpace.Messages/Commands/Agents/BackfillPackCloneUrlsCommand.cs b/backend/src/CodeSpace.Messages/Commands/Agents/BackfillPackCloneUrlsCommand.cs new file mode 100644 index 000000000..73a61d2d5 --- /dev/null +++ b/backend/src/CodeSpace.Messages/Commands/Agents/BackfillPackCloneUrlsCommand.cs @@ -0,0 +1,21 @@ +using CodeSpace.Messages.Mediation; + +namespace CodeSpace.Messages.Commands.Agents; + +/// +/// Seal the clone URL of every pack row that still holds a credential in its url column: the rows imported +/// before a pack kept its credential sealed, and any an older pod writes during a rolling deploy. Each row gets the +/// credential-free URL plus the sealed original, so it keeps syncing on every pod that reads the seal (a pod that +/// predates it cannot — see IPackCloneUrlBackfillService). Idempotent and bounded: a sealed row no longer +/// carries a credential, so it leaves the candidate set and the backlog only shrinks. NOT tenant-scoped: a +/// system-wide repair that runs without an actor context (mirrors BackfillRunScorecardsCommand). +/// +/// NOT transactional (): each row is sealed by its own conditional UPDATE, +/// and a row that fails stays a candidate while the pass carries on. One transaction around the pass would let one +/// failing row undo every other row's seal. +/// +public sealed record BackfillPackCloneUrlsCommand : ICommand, INonTransactionalCommand +{ + /// Packs sealed per tick — bounds each pass. + public int BatchSize { get; init; } = 50; +} diff --git a/backend/src/CodeSpace.Messages/Commands/Agents/ImportPackArtifactsCommand.cs b/backend/src/CodeSpace.Messages/Commands/Agents/ImportPackArtifactsCommand.cs new file mode 100644 index 000000000..8184302de --- /dev/null +++ b/backend/src/CodeSpace.Messages/Commands/Agents/ImportPackArtifactsCommand.cs @@ -0,0 +1,23 @@ +using CodeSpace.Messages.Agents; +using CodeSpace.Messages.Authorization; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Mediation; + +namespace CodeSpace.Messages.Commands.Agents; + +/// +/// Add the selected artifacts a Sync discovered to the pack they were discovered in: re-clone the pack's SAVED source +/// at its saved ref (the clone uses the pack's sealed credential when it has one; the request carries no URL) and +/// upsert exactly the chosen into THAT pack. Resolving the pack again by URL would be wrong +/// for a private pack, whose stored URL no longer carries the credential it needs. Returns a per-path outcome. +/// +public sealed record ImportPackArtifactsCommand : ICommand, IRequireTeamPermission +{ + public string RequiredPermission => TeamPermissions.AgentsWrite; + + /// The pack to import into. Not required: the body carries only the selection, and the controller sets this from the route. + public Guid PackId { get; init; } + + /// The SourcePaths the operator selected from the sync's new artifacts, agents or skills. + public IReadOnlyList SourcePaths { get; init; } = Array.Empty(); +} diff --git a/backend/tests/CodeSpace.E2ETests/Agents/PrivatePackCredentialE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Agents/PrivatePackCredentialE2ETests.cs new file mode 100644 index 000000000..253bea97c --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Agents/PrivatePackCredentialE2ETests.cs @@ -0,0 +1,332 @@ +using System.Net; +using System.Net.Http.Headers; +using System.Net.Http.Json; +using System.Text.Json; +using Autofac; +using CodeSpace.Core.Jobs.RecurringJobs; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Agents; +using CodeSpace.Core.Services.Agents.Sandbox.Runners; +using CodeSpace.E2ETests.Infrastructure; +using CodeSpace.Messages.Agents; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Enums; +using Microsoft.Extensions.DependencyInjection; +using Npgsql; +using Shouldly; + +namespace CodeSpace.E2ETests.Agents; + +/// +/// A private agent pack imported by pasting a git URL that embeds a token, through the whole HTTP surface. +/// +/// Tier: 🟢 High-fidelity (Rule 12) — the real ASP.NET pipeline (routing, JWT auth, the X-Team-Id scope, +/// model binding, the controllers, the exception filter, the mediator and its authorization behaviors), the production +/// on the real local runner and real git, and real Postgres, against a loopback +/// smart-HTTP remote that answers 401 to any request without the fake token. One documented seam: the pack-host +/// allowlist accepts that loopback http remote (production admits https hosts only). The backfill job is resolved from +/// a host DI scope with no HTTP context, as a worker's job scope resolves it (its Hangfire registration is covered by +/// RecurringJobWorkerSmokeE2ETests). +/// +/// Must hold: no response a Viewer can request carries the token, nor does the stored row, nor the error body of a +/// Sync or an add whose clone from the sealed source fails; a Member can still sync the pack and add what the sync +/// discovered; a legacy row that stored the token verbatim is sealed by the job and +/// keeps syncing. Each test owns its remote (GUID-suffixed temp root, a loopback port allocated by the server) and +/// removes it on every path. Only the token-carrying URL is ever cloned here, so git never asks a credential helper +/// (the tokened clone resets them) — the refused-without-token check is an HTTP probe, not a clone. +/// +[Trait("Category", "E2E")] +[Trait("Surface", "Http")] +public sealed class PrivatePackCredentialE2ETests : IClassFixture +{ + private const string Token = "fake-e2e-pack-token-0123456789"; + private const string Agent = "agents/reviewer.md"; + private const string NewSkill = "skills/new-skill/SKILL.md"; + + private readonly PrivatePackApiFactory _factory; + + public PrivatePackCredentialE2ETests(PrivatePackApiFactory factory) { _factory = factory; } + + [Fact] + public async Task A_pasted_token_is_never_returned_and_the_private_pack_keeps_syncing_over_http() + { + if (!await GitReadyAsync()) return; + + using var remote = await PrivateRemote.StartAsync(); + var world = await SeedAsync(); + + (await remote.AnonymousProbeAsync()).ShouldBe(HttpStatusCode.Unauthorized, "fixture check: the remote refuses a read that presents no token"); + + var import = await SendAsync(world.MemberId, world.TeamId, HttpMethod.Post, "/api/agents/import-url", new { url = remote.TokenedUrl, sourcePaths = new[] { Agent } }); + var importBody = await ShouldSucceedAsync(import, "import-url"); + var packId = JsonDocument.Parse(importBody).RootElement.GetProperty("packId").GetGuid(); + + foreach (var path in new[] { "/api/packs", $"/api/packs/{packId}" }) + { + var body = await ShouldSucceedAsync(await SendAsync(world.ViewerId, world.TeamId, HttpMethod.Get, path), $"a Viewer's GET {path}"); + body.ShouldContain(remote.CleanUrl, Case.Sensitive, $"GET {path} still shows the pack's source, without its userinfo"); + } + + await remote.PublishUpstreamChangeAsync(); + + var syncBody = await ShouldSucceedAsync(await SendAsync(world.MemberId, world.TeamId, HttpMethod.Post, $"/api/packs/{packId}/sync"), "sync"); + var sync = JsonDocument.Parse(syncBody).RootElement; + sync.GetProperty("updated").GetInt32().ShouldBe(1, "the sealed token cloned the private remote and the changed agent was refreshed"); + sync.GetProperty("newArtifacts").GetProperty("skills").EnumerateArray().Select(s => s.GetProperty("sourcePath").GetString()).ShouldContain(NewSkill); + + var addBody = await ShouldSucceedAsync(await SendAsync(world.MemberId, world.TeamId, HttpMethod.Post, $"/api/packs/{packId}/import", new { sourcePaths = new[] { NewSkill } }), "import from the pack"); + JsonDocument.Parse(addBody).RootElement.GetProperty("packId").GetGuid().ShouldBe(packId, "the discovered artifact is added to the pack it came from"); + + var detail = await ShouldSucceedAsync(await SendAsync(world.ViewerId, world.TeamId, HttpMethod.Get, $"/api/packs/{packId}"), "a Viewer's GET of the pack after the add"); + JsonDocument.Parse(detail).RootElement.GetProperty("artifacts").EnumerateArray().Select(a => a.GetProperty("sourcePath").GetString()).ShouldContain(NewSkill); + + (await SendAsync(world.ViewerId, world.TeamId, HttpMethod.Post, $"/api/packs/{packId}/sync")).StatusCode.ShouldBe(HttpStatusCode.Forbidden, "a Viewer cannot make the server spend the pack's token"); + + ShouldHoldNoToken(await RawRowAsync(packId), "the stored pack row"); + } + + [Fact] + public async Task A_failed_clone_from_the_sealed_source_names_no_token_in_the_http_error_body() + { + if (!await GitReadyAsync()) return; + + using var remote = await PrivateRemote.StartAsync(); + var world = await SeedAsync(); + + var import = await SendAsync(world.MemberId, world.TeamId, HttpMethod.Post, "/api/agents/import-url", new { url = remote.TokenedUrl, sourcePaths = new[] { Agent } }); + var packId = JsonDocument.Parse(await ShouldSucceedAsync(import, "import-url")).RootElement.GetProperty("packId").GetGuid(); + + // The token stays valid; the saved ref is gone upstream, so each clone authenticates with the decrypted source and then fails. + await ExecuteAsync("UPDATE pack SET reference = 'deleted-branch' WHERE id = @id", packId); + + foreach (var (path, body) in new (string, object?)[] { ($"/api/packs/{packId}/sync", null), ($"/api/packs/{packId}/import", new { sourcePaths = new[] { NewSkill } }) }) + { + var response = await SendAsync(world.MemberId, world.TeamId, HttpMethod.Post, path, body); + var text = await response.Content.ReadAsStringAsync(); + + response.StatusCode.ShouldBe(HttpStatusCode.BadRequest, $"POST {path}: {text}"); + text.ShouldContain("deleted-branch", Case.Sensitive, $"fixture check: POST {path} reports git's reason, so the scan below reads the clone failure"); + ShouldHoldNoToken(text, $"the POST {path} error body"); + } + } + + [Fact] + public async Task A_legacy_row_holding_a_token_is_sealed_by_the_backfill_job_and_keeps_syncing_over_http() + { + if (!await GitReadyAsync()) return; + + using var remote = await PrivateRemote.StartAsync(); + var world = await SeedAsync(); + var packId = await SeedLegacyPackAsync(world, remote.TokenedUrl); + + ShouldHoldNoToken(await ShouldSucceedAsync(await SendAsync(world.ViewerId, world.TeamId, HttpMethod.Get, $"/api/packs/{packId}"), "a Viewer's GET of the unsealed legacy pack"), "the read model, before the backfill reaches the row"); + (await RawRowAsync(packId)).ShouldContain(Token, Case.Sensitive, "positive control: the legacy row holds the token at rest"); + + await RunBackfillJobUntilSealedAsync(packId); + + var row = await RawRowAsync(packId); + ShouldHoldNoToken(row, "the sealed row"); + JsonDocument.Parse(row).RootElement.GetProperty("encrypted_clone_url").GetString().ShouldNotBeNullOrEmpty("the job sealed the token rather than dropping it"); + + var syncBody = await ShouldSucceedAsync(await SendAsync(world.MemberId, world.TeamId, HttpMethod.Post, $"/api/packs/{packId}/sync"), "sync of the sealed legacy pack"); + JsonDocument.Parse(syncBody).RootElement.GetProperty("newArtifacts").GetProperty("agents").EnumerateArray().Select(a => a.GetProperty("sourcePath").GetString()).ShouldContain(Agent, "the sealed token cloned the private remote"); + } + + /// The production job, resolved from a host scope with no HTTP context — the shape a worker's job scope has — until the owned row is sealed. The sweep is deployment-wide; this waits on the row the test owns. + private async Task RunBackfillJobUntilSealedAsync(Guid packId) + { + const int maxTicks = 5; + + for (var tick = 0; tick < maxTicks; tick++) + { + using (var scope = _factory.Services.CreateScope()) + await scope.ServiceProvider.GetRequiredService().Execute(); + + if (!(await RawRowAsync(packId)).Contains(Token, StringComparison.Ordinal)) return; + } + + throw new ShouldAssertException($"pack {packId} still holds the token after {maxTicks} ticks of {nameof(PackCloneUrlBackfillRecurringJob)} — check the host log for 'Pack clone-URL backfill failed for pack {packId}' and run `SELECT url, encrypted_clone_url FROM pack WHERE id = '{packId}'` against the fixture database"); + } + + private async Task ExecuteAsync(string sql, Guid packId) + { + await using var connection = new NpgsqlConnection(_factory.ConnectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand(sql, connection); + command.Parameters.AddWithValue("id", packId); + + (await command.ExecuteNonQueryAsync()).ShouldBe(1, $"fixture check: '{sql}' updated pack {packId}"); + } + + private async Task RawRowAsync(Guid packId) + { + await using var connection = new NpgsqlConnection(_factory.ConnectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand("SELECT row_to_json(p)::text FROM pack p WHERE id = @id", connection); + command.Parameters.AddWithValue("id", packId); + + return (string)(await command.ExecuteScalarAsync()).ShouldNotBeNull($"pack {packId} must exist"); + } + + /// A pack row exactly as the code before the seal wrote one: the pasted URL verbatim, none of the new columns. + private async Task SeedLegacyPackAsync(World world, string url) + { + var id = Guid.NewGuid(); + var created = DateTimeOffset.UtcNow.AddYears(-30); + + await using var connection = new NpgsqlConnection(_factory.ConnectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand("INSERT INTO pack (id, team_id, kind, name, url, created_date, created_by, last_modified_date, last_modified_by) VALUES (@id, @team, 'GitUrl', 'remote', @url, @created, @user, @created, @user)", connection); + command.Parameters.AddWithValue("id", id); + command.Parameters.AddWithValue("team", world.TeamId); + command.Parameters.AddWithValue("url", url); + command.Parameters.AddWithValue("created", created); + command.Parameters.AddWithValue("user", world.MemberId); + await command.ExecuteNonQueryAsync(); + + return id; + } + + private async Task ShouldSucceedAsync(HttpResponseMessage response, string what) + { + var body = await response.Content.ReadAsStringAsync(); + + response.StatusCode.ShouldBe(HttpStatusCode.OK, $"{what} failed: {body}"); + ShouldHoldNoToken(body, $"the {what} response"); + return body; + } + + private static void ShouldHoldNoToken(string text, string what) => text.ShouldNotContain(Token, Case.Insensitive, $"{what} must not hold the pasted token"); + + private async Task SendAsync(Guid userId, Guid teamId, HttpMethod method, string path, object? body = null) + { + var request = new HttpRequestMessage(method, path) { Content = body is null ? null : JsonContent.Create(body) }; + request.Headers.Authorization = new AuthenticationHeaderValue("Bearer", TestToken.Mint(userId, TestToken.SeedStamp)); + request.Headers.Add("X-Team-Id", teamId.ToString()); + return await _factory.CreateClient().SendAsync(request); + } + + private async Task SeedAsync() + { + using var scope = _factory.Services.CreateScope(); + var db = scope.ServiceProvider.GetRequiredService(); + var world = new World(Guid.NewGuid(), Guid.NewGuid(), Guid.NewGuid()); + var suffix = Guid.NewGuid().ToString("N")[..8]; + + db.Team.Add(new Team { Id = world.TeamId, Slug = $"private-pack-{suffix}", Name = "Private Pack E2E", Kind = TeamKind.Workspace, CreatedBy = SystemUsers.SeederId, LastModifiedBy = SystemUsers.SeederId }); + + foreach (var (userId, role) in new[] { (world.MemberId, TeamRole.Member), (world.ViewerId, TeamRole.Viewer) }) + { + db.User.Add(new User { Id = userId, SecurityStamp = TestToken.SeedStamp, Email = $"private-pack-{userId:N}@test.local", Name = $"Private Pack {role}", CreatedBy = SystemUsers.SeederId, LastModifiedBy = SystemUsers.SeederId }); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = world.TeamId, UserId = userId, Role = role, CreatedBy = SystemUsers.SeederId, LastModifiedBy = SystemUsers.SeederId }); + } + + await db.SaveChangesAsync(); + return world; + } + + private static async Task GitReadyAsync() + { + if (OperatingSystem.IsWindows()) return false; + + try { return (await new LocalProcessRunner().RunAsync(new SandboxSpec { Command = "git", Args = new[] { "--version" }, TimeoutSeconds = 10 }, CancellationToken.None)).Status == SandboxStatus.Success; } + catch { return false; } + } + + private sealed record World(Guid TeamId, Guid MemberId, Guid ViewerId); + + /// The app with one seam: the pack-host allowlist also admits the loopback http test remote. + public sealed class PrivatePackApiFactory : TaskLaunchApiFactory + { + protected override void ConfigureContractTestServices(ContainerBuilder builder) => + builder.RegisterType().As().SingleInstance(); + } + + /// Admits only the loopback http remote these tests start — every other URL is refused, as the production allowlist would refuse an http one. + private sealed class LoopbackRemoteAllowlist : IPackHostAllowlist + { + public bool IsAllowed(string url) => Uri.TryCreate(url, UriKind.Absolute, out var uri) && uri.Scheme == Uri.UriSchemeHttp && uri.Host == "127.0.0.1"; + + public void EnsureAllowed(string url) + { + if (!IsAllowed(url)) throw new PackImportException("Only the loopback test remote is an allowed pack source in this fixture."); + } + } + + /// A bare repository holding a pack (an agent and a skill), served over smart HTTP behind the fake token. GUID-suffixed temp root; disposal stops the server and removes the root. + private sealed class PrivateRemote : IDisposable + { + private readonly string _root = Path.Combine(Path.GetTempPath(), "cs-private-pack-" + Guid.NewGuid().ToString("N")); + private readonly GitTestRemoteServer _server; + + private PrivateRemote() + { + Directory.CreateDirectory(_root); + _server = new GitTestRemoteServer(_root, requiredBasicCredential: $"x-access-token:{Token}"); + } + + public string CleanUrl => _server.Url; + public string TokenedUrl => _server.Url.Replace("http://", $"http://x-access-token:{Token}@", StringComparison.Ordinal); + + private string Bare => Path.Combine(_root, "remote.git"); + private string Seed => Path.Combine(_root, "seed"); + + public static async Task StartAsync() + { + var remote = new PrivateRemote(); + + try + { + await Git(remote._root, "init", "--bare", "-b", "main", remote.Bare); + await Git(remote._root, "init", "-b", "main", remote.Seed); + await remote.CommitAndPushAsync("pack", new Dictionary + { + [Agent] = "---\nname: reviewer\ndescription: Reviews PRs.\n---\nYou review v1.", + ["skills/tdd/SKILL.md"] = "---\nname: tdd\ndescription: Use when implementing.\n---\n# TDD v1", + }); + return remote; + } + catch + { + remote.Dispose(); + throw; + } + } + + public Task PublishUpstreamChangeAsync() => CommitAndPushAsync("upstream change", new Dictionary + { + [Agent] = "---\nname: reviewer\ndescription: Reviews PRs.\n---\nYou review v2 now.", + [NewSkill] = "---\nname: new-skill\ndescription: Brand new.\n---\n# New", + }); + + /// The smart-HTTP ref advertisement a clone starts with, asked for without credentials. + public async Task AnonymousProbeAsync() + { + using var http = new HttpClient(); + return (await http.GetAsync($"{CleanUrl}/info/refs?service=git-upload-pack")).StatusCode; + } + + private async Task CommitAndPushAsync(string message, IReadOnlyDictionary files) + { + foreach (var (path, content) in files) + { + var full = Path.Combine(Seed, path.Replace('/', Path.DirectorySeparatorChar)); + Directory.CreateDirectory(Path.GetDirectoryName(full)!); + await File.WriteAllTextAsync(full, content); + } + + await Git(Seed, "add", "-A"); + await Git(Seed, "-c", "user.email=test@codespace.dev", "-c", "user.name=Test", "-c", "commit.gpgsign=false", "commit", "-m", message); + await Git(Seed, "push", Bare, "main"); + } + + private static Task Git(string workdir, params string[] args) => GitTestRemoteServer.RunFixtureGitAsync(workdir, args); + + public void Dispose() + { + _server.Dispose(); + try { Directory.Delete(_root, recursive: true); } catch { /* best-effort */ } + } + } +} diff --git a/backend/tests/CodeSpace.E2ETests/Infrastructure/GitTestRemoteServer.cs b/backend/tests/CodeSpace.E2ETests/Infrastructure/GitTestRemoteServer.cs index d135cd3da..215a28459 100644 --- a/backend/tests/CodeSpace.E2ETests/Infrastructure/GitTestRemoteServer.cs +++ b/backend/tests/CodeSpace.E2ETests/Infrastructure/GitTestRemoteServer.cs @@ -13,12 +13,16 @@ internal sealed class GitTestRemoteServer : IDisposable private readonly CancellationTokenSource _stopping = new(); private readonly List _requests = new(); private Task? _accept; + private readonly string? _requiredAuthorization; public string Root { get; } public string Url { get; private set; } = ""; - public GitTestRemoteServer(string root) + /// The directory served as GIT_PROJECT_ROOT. + /// When set (user:password), every request — a clone or fetch included — must present it as Basic auth or is answered 401: a private remote, so a read that succeeds proves the credential authenticated. Null (the default) serves anonymously. + public GitTestRemoteServer(string root, string? requiredBasicCredential = null) { Root = root; + _requiredAuthorization = requiredBasicCredential is null ? null : "Basic " + Convert.ToBase64String(Encoding.UTF8.GetBytes(requiredBasicCredential)); // Reserve an ephemeral loopback port; retry binding only if another process won the close/bind race. for (var attempt = 0; ; attempt++) { @@ -57,6 +61,12 @@ private async Task ServeAsync(HttpListenerContext context) { using var input = new MemoryStream(); await context.Request.InputStream.CopyToAsync(input, _stopping.Token); + if (_requiredAuthorization is not null && !string.Equals(context.Request.Headers["Authorization"], _requiredAuthorization, StringComparison.Ordinal)) + { + context.Response.StatusCode = 401; + context.Response.Headers["WWW-Authenticate"] = "Basic realm=\"fixture\""; + return; + } var info = StartInfo(Root, new[] { "http-backend" }); info.RedirectStandardInput = true; info.Environment["GIT_PROJECT_ROOT"] = Root; diff --git a/backend/tests/CodeSpace.IntegrationTests/Agents/PackCloneUrlBackfillFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Agents/PackCloneUrlBackfillFlowTests.cs new file mode 100644 index 000000000..797bb2aa9 --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Agents/PackCloneUrlBackfillFlowTests.cs @@ -0,0 +1,276 @@ +using Autofac; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Services.Agents; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.Messages.Commands.Agents; +using CodeSpace.Messages.Enums; +using MediatR; +using Microsoft.EntityFrameworkCore; +using Npgsql; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Agents; + +/// +/// HIGH fidelity (Rule 12): the clone-URL backfill through the REAL mediator ( +/// → its handler → ) over real Postgres, against pack rows seeded by raw SQL in +/// exactly the shape the code before the seal wrote — the pasted URL verbatim in url, no new column set. A sealed +/// row must hold no token anywhere at rest, decrypt through the production to the +/// URL it was imported with, and keep syncing against a remote that demands that token (). +/// Legacy forks of one repository must settle into one holder plus duplicates that each keep syncing; a re-import that +/// arrives before the backfill must land in the unsealed row (the clean pack, when one exists) rather than fork it; several +/// workers at once must seal each row once and converge. +/// +/// The sweep is deployment-wide and these tests share a database, so every assertion is on rows the test owns, +/// never on a pass's tally; passes are looped with a bounded count and an actionable failure. Seeded rows are dated +/// decades back so they sort first into every batch. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public sealed class PackCloneUrlBackfillFlowTests +{ + private static readonly DateTimeOffset LongAgo = DateTimeOffset.UtcNow.AddYears(-30); + + private readonly PostgresFixture _fixture; + + public PackCloneUrlBackfillFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + [Fact] + public async Task A_legacy_tokened_row_syncs_before_and_after_it_is_sealed() + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + var packId = await harness.SeedLegacyPackAsync(team, source.TokenedUrl, LongAgo); + + (await harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = packId })).NewArtifacts.Agents.ShouldContain(a => a.SourcePath == PrivatePackSource.Agent, "an unsealed legacy row still clones from the token in its URL"); + + await harness.BackfillUntilSealedAsync(packId); + + var columns = await harness.ColumnsAsync(packId); + columns.Url.ShouldBe(source.CleanUrl); + columns.DuplicateOfPackId.ShouldBeNull("nothing else holds this source"); + (await harness.CloneUrlOfAsync(packId)).ShouldBe(source.TokenedUrl, "the production protector decrypts the seal to the URL the pack was imported with"); + PrivatePackSource.ShouldHoldNoToken(await harness.RawRowAsync(packId), "the sealed row"); + + (await harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = packId })).NewArtifacts.Agents.ShouldContain(a => a.SourcePath == PrivatePackSource.Agent, "the sealed row clones with its sealed token"); + } + + [Fact] + public async Task A_second_pass_leaves_a_sealed_row_exactly_as_the_first_left_it() + { + var harness = new PackCredentialHarness(_fixture); + var team = await harness.SeedTeamAsync(); + var packId = await harness.SeedLegacyPackAsync(team, $"https://x-access-token:{PrivatePackSource.Token}@git.example.test/acme/agents", LongAgo); + + await harness.BackfillUntilSealedAsync(packId); + var first = await harness.RawRowAsync(packId); + + await harness.BackfillPassAsync(); + + (await harness.RawRowAsync(packId)).ShouldBe(first, "a sealed row is no candidate — no column, the ciphertext included, is rewritten"); + } + + [Fact] + public async Task A_soft_deleted_tokened_row_is_sealed_too() + { + var harness = new PackCredentialHarness(_fixture); + var team = await harness.SeedTeamAsync(); + var packId = await harness.SeedLegacyPackAsync(team, $"https://x-access-token:{PrivatePackSource.Token}@git.example.test/acme/agents", LongAgo, deleted: true); + + await harness.BackfillUntilSealedAsync(packId); + + var columns = await harness.ColumnsAsync(packId); + columns.DeletedDate.ShouldNotBeNull("fixture check: the row stays soft-deleted"); + columns.Url.ShouldBe("https://git.example.test/acme/agents"); + columns.EncryptedCloneUrl.ShouldNotBeNull(); + PrivatePackSource.ShouldHoldNoToken(await harness.RawRowAsync(packId), "a soft-deleted row — plaintext at rest either way"); + } + + [Theory] + [InlineData(false)] // the repository was imported once without a token (a clean holder exists) and once with one + [InlineData(true)] // the repository was imported twice, with two tokens + public async Task A_legacy_fork_settles_into_one_holder_and_a_duplicate_that_both_keep_syncing(bool twoTokens) + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(authenticateReads: twoTokens); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + + // The tokened row is the OLDER one in the clean-holder case, so the clean row holding proves "clean wins", not "oldest wins". + var older = await harness.SeedLegacyPackAsync(team, source.TokenedUrl, LongAgo); + var newer = await harness.SeedLegacyPackAsync(team, twoTokens ? source.UrlWith("x-access-token:fake%2Dpublish-token-0123456789") : source.CleanUrl, LongAgo.AddMinutes(1)); + + await harness.BackfillUntilSealedAsync(older, newer); + + var (holder, duplicate) = twoTokens ? (older, newer) : (newer, older); + (await harness.ColumnsAsync(holder)).DuplicateOfPackId.ShouldBeNull(twoTokens ? "with no clean row, the oldest sealed row holds the source" : "the clean row holds the source"); + (await harness.ColumnsAsync(duplicate)).DuplicateOfPackId.ShouldBe(holder); + (await harness.ColumnsAsync(duplicate)).Url.ShouldBe(source.CleanUrl, "both rows now show the same credential-free URL"); + + foreach (var packId in new[] { holder, duplicate }) + (await harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = packId })).NewArtifacts.Agents.ShouldContain(a => a.SourcePath == PrivatePackSource.Agent, $"pack {packId} keeps syncing from its own source"); + + (await harness.ImportAsync(team, source.TokenedUrl, PrivatePackSource.Agent)).ShouldBe(holder, "a new import resolves to the holder"); + + var added = await harness.SendAsync(team.OwnerId, team.TeamId, new ImportPackArtifactsCommand { PackId = duplicate, SourcePaths = new[] { PrivatePackSource.Skill } }); + added.PackId.ShouldBe(duplicate, "an import from the duplicate lands on the duplicate, not on the holder"); + added.Items.ShouldHaveSingleItem().Outcome.ShouldBe(PackImportOutcome.Imported); + } + + [Theory] + [InlineData(null)] // the same tokened URL pasted again: the code before the seal updated this row in place + [InlineData("x-access-token:fake%2Dpublish-token-0123456789")] // a rotated token (a new spelling the remote still accepts), as the rotation advice prompts + public async Task A_re_import_before_the_backfill_lands_in_the_unsealed_legacy_pack_and_seals_it(string? rotatedUserInfo) + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + var legacy = await harness.ImportAsync(team, source.TokenedUrl, PrivatePackSource.Agent); + await harness.MakeLegacyAsync(legacy, source.TokenedUrl); + + var secondUrl = rotatedUserInfo is null ? source.TokenedUrl : source.UrlWith(rotatedUserInfo); + var again = await harness.ImportAsync(team, secondUrl, PrivatePackSource.Agent, PrivatePackSource.Skill); + + again.ShouldBe(legacy, "the unsealed row holds this repository's history; a second pack would be marked its holder once the backfill seals it"); + + using (var scope = _fixture.BeginScope()) + { + var db = scope.Resolve(); + (await db.Pack.AsNoTracking().CountAsync(p => p.TeamId == team.TeamId && p.DeletedDate == null)).ShouldBe(1, "no second pack is created beside the legacy one"); + (await db.AgentDefinition.AsNoTracking().CountAsync(a => a.TeamId == team.TeamId && a.Scope == DefinitionScope.Store && a.DeletedDate == null)).ShouldBe(1, "the re-selected agent is refreshed in place, not copied"); + } + + var columns = await harness.ColumnsAsync(legacy); + columns.Url.ShouldBe(source.CleanUrl, "the import seals the row it lands in, as the backfill would"); + columns.DuplicateOfPackId.ShouldBeNull(); + (await harness.CloneUrlOfAsync(legacy)).ShouldBe(secondUrl, "the sealed source is the URL this import cloned"); + PrivatePackSource.ShouldHoldNoToken(await harness.RawRowAsync(legacy), "the legacy row after the re-import"); + + (await harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = legacy })).UpToDate.ShouldBe(2, "the pack keeps syncing from the source it now records"); + } + + [Fact] + public async Task A_re_import_before_the_backfill_prefers_the_clean_pack_over_an_unsealed_fork() + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + + // The tokened fork is the OLDER one, so landing on the clean pack proves "clean wins", not "oldest wins". + var fork = await harness.SeedLegacyPackAsync(team, source.TokenedUrl, LongAgo); + var clean = await harness.SeedLegacyPackAsync(team, source.CleanUrl, LongAgo.AddMinutes(1)); + + (await harness.ImportAsync(team, source.TokenedUrl, PrivatePackSource.Agent)).ShouldBe(clean, "the clean pack holds the source; sealing the fork in place would collide with it"); + + (await harness.ColumnsAsync(fork)).Url.ShouldBe(source.TokenedUrl, "the fork is left to the backfill"); + + await harness.BackfillUntilSealedAsync(fork); + + (await harness.ColumnsAsync(fork)).DuplicateOfPackId.ShouldBe(clean, "the backfill settles the fork as the clean pack's duplicate"); + } + + [Fact] + public async Task Concurrent_passes_seal_each_row_once_and_converge_on_one_holder() + { + var harness = new PackCredentialHarness(_fixture); + var team = await harness.SeedTeamAsync(); + const string host = "git.example.test/acme/agents"; + + var forkA = await harness.SeedLegacyPackAsync(team, $"https://x-access-token:{PrivatePackSource.Token}@{host}", LongAgo); + var forkB = await harness.SeedLegacyPackAsync(team, $"https://oauth2:{PrivatePackSource.Token}@{host}", LongAgo); // same instant: the holder race has no tie-break to lean on + var alone = await harness.SeedLegacyPackAsync(team, $"https://x-access-token:{PrivatePackSource.Token}@git.example.test/acme/other", LongAgo); + var owned = new[] { forkA, forkB, alone }; + var originals = new Dictionary + { + [forkA] = $"https://x-access-token:{PrivatePackSource.Token}@{host}", + [forkB] = $"https://oauth2:{PrivatePackSource.Token}@{host}", + [alone] = $"https://x-access-token:{PrivatePackSource.Token}@git.example.test/acme/other", + }; + + await Task.WhenAll(Enumerable.Range(0, 4).Select(_ => Task.Run(harness.BackfillPassAsync))); + + await harness.BackfillUntilSealedAsync(owned); + + var settled = new Dictionary(); + foreach (var packId in owned) + { + (await harness.CloneUrlOfAsync(packId)).ShouldBe(originals[packId], $"pack {packId}'s seal decrypts to its own original — no worker overwrote it with another row's"); + settled[packId] = await harness.RawRowAsync(packId); + } + + var forks = await Task.WhenAll(new[] { forkA, forkB }.Select(async id => (Id: id, Columns: await harness.ColumnsAsync(id)))); + forks.Count(f => f.Columns.DuplicateOfPackId is null).ShouldBe(1, "exactly one fork holds the source"); + forks.Single(f => f.Columns.DuplicateOfPackId is not null).Columns.DuplicateOfPackId.ShouldBe(forks.Single(f => f.Columns.DuplicateOfPackId is null).Id); + (await harness.ColumnsAsync(alone)).DuplicateOfPackId.ShouldBeNull(); + + await Task.WhenAll(Enumerable.Range(0, 4).Select(_ => Task.Run(harness.BackfillPassAsync))); + + foreach (var packId in owned) + (await harness.RawRowAsync(packId)).ShouldBe(settled[packId], $"pack {packId} was sealed once: later passes, concurrent ones included, rewrite nothing"); + } + + [Fact] + public async Task A_sync_that_loaded_the_row_before_its_seal_loses_on_the_concurrency_token_and_a_retry_succeeds() + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + var packId = await harness.SeedLegacyPackAsync(team, source.TokenedUrl, LongAgo); + + // The seal lands between the sync's read of the row and its write: the fetch runs after the load. + var sealDuringFetch = new SealingFetcher(source.Fetcher, () => harness.BackfillUntilSealedAsync(packId)); + + await Should.ThrowAsync(() => SyncThroughAsync(sealDuringFetch, team, packId), "the sync's write is fenced by xmin, so it never writes back over the seal"); + + PrivatePackSource.ShouldHoldNoToken(await harness.RawRowAsync(packId), "the row after the lost sync"); + + (await harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = packId })).NewArtifacts.Agents.ShouldContain(a => a.SourcePath == PrivatePackSource.Agent, "the retry clones with the sealed token"); + } + + [Fact] + public async Task The_source_index_admits_a_duplicate_beside_its_holder_and_nothing_else() + { + var harness = new PackCredentialHarness(_fixture); + var team = await harness.SeedTeamAsync(); + const string url = "https://git.example.test/acme/indexed"; + + var holder = await harness.SeedLegacyPackAsync(team, url, LongAgo); + var duplicate = await harness.SeedLegacyPackAsync(team, url + "-dup", LongAgo); + (await harness.ExecuteAsync("UPDATE pack SET url = @url, duplicate_of_pack_id = @holder WHERE id = @id", ("url", url), ("holder", holder), ("id", duplicate))).ShouldBe(1, "a duplicate coexists with its holder"); + + var clash = await Should.ThrowAsync(() => harness.SeedLegacyPackAsync(team, url, LongAgo)); + clash.SqlState.ShouldBe(PostgresErrorCodes.UniqueViolation, "two packs holding one source is still refused"); + } + + private async Task SyncThroughAsync(IPackSourceFetcher fetcher, PackCredentialTeam team, Guid packId) + { + using var scope = _fixture.BeginScope(b => + { + b.RegisterInstance(new TestCurrentUser(team.OwnerId, "pack-credential")).As().SingleInstance(); + b.RegisterInstance(new TestCurrentTeam(team.TeamId)).As().SingleInstance(); + b.RegisterInstance(fetcher).As().SingleInstance(); + }); + + await scope.Resolve().Send(new SyncPackCommand { PackId = packId }); + } + + /// The production fetcher, with run first — after the sync has loaded its row. + private sealed class SealingFetcher(IPackSourceFetcher inner, Func beforeClone) : IPackSourceFetcher + { + public async Task FetchAsync(string url, string? reference, CancellationToken cancellationToken) + { + await beforeClone(); + return await inner.FetchAsync(url, reference, cancellationToken); + } + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Agents/PackImportServiceFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Agents/PackImportServiceFlowTests.cs index 01d2edd09..ffc9db236 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Agents/PackImportServiceFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Agents/PackImportServiceFlowTests.cs @@ -50,7 +50,8 @@ public async Task Previews_agents_and_skills_from_a_url_as_importable_store_snap var service = new PackImportService( new PackCloneFetcher(new AllowAll(), scope.Resolve(), NullLogger.Instance), scope.Resolve(), - scope.Resolve()); + scope.Resolve(), + scope.Resolve()); preview = await service.PreviewFromUrlAsync(src, reference: null, teamId, CancellationToken.None); } diff --git a/backend/tests/CodeSpace.IntegrationTests/Agents/PackSealedSourceFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Agents/PackSealedSourceFlowTests.cs new file mode 100644 index 000000000..c9bb2ba88 --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Agents/PackSealedSourceFlowTests.cs @@ -0,0 +1,256 @@ +using CodeSpace.Core.Authorization; +using CodeSpace.Core.Failures; +using CodeSpace.Core.Persistence.Db; +using CodeSpace.Core.Services.Agents; +using CodeSpace.IntegrationTests.Infrastructure; +using CodeSpace.Messages.Commands.Agents; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Queries.Agents; +using Autofac; +using Microsoft.EntityFrameworkCore; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Agents; + +/// +/// HIGH fidelity (Rule 12): a pack imported from a pasted URL that embeds a token, through the REAL mediator, the real +/// and real git, into real Postgres, against a loopback remote that refuses every +/// request without that token (). The pack must store its URL without the token and keep +/// the token only sealed; no read a team member can make — a Viewer's included — may return it; Sync and the add-after- +/// sync import must still authenticate, and when either fails after decrypting the sealed source, neither the error body +/// nor the mediator's log may name the token; and a URL that carries no credential must be stored exactly as pasted. +/// +/// Positive controls: the remote refuses a clone without the token (so every later success proves the sealed +/// token cloned); the same pack with its sealed source removed cannot sync; a row put back in the legacy shape holds the +/// token (so the raw-row scans that find none can see one). The one seam is the allowlist accepting the loopback http +/// remote. Each test owns its remote, scratch HOME and team, and removes the remote on every path. +/// +[Collection(PostgresCollection.Name)] +[Trait("Category", "Integration")] +public sealed class PackSealedSourceFlowTests +{ + private readonly PostgresFixture _fixture; + + public PackSealedSourceFlowTests(PostgresFixture fixture) { _fixture = fixture; } + + [Fact] + public async Task The_private_remote_refuses_a_clone_without_the_token() + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + + await Should.ThrowAsync(() => source.Fetcher.FetchAsync(source.CleanUrl, null, CancellationToken.None), "fixture check: a clone that presents no token is refused"); + + using var checkout = await source.Fetcher.FetchAsync(source.TokenedUrl, null, CancellationToken.None); + File.Exists(Path.Combine(checkout.Directory, PrivatePackSource.Agent)).ShouldBeTrue("fixture check: the token clones the pack"); + } + + [Fact] + public async Task A_tokened_import_stores_the_url_without_the_token_and_keeps_it_only_sealed() + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + + var result = await harness.SendAsync(team.OwnerId, team.TeamId, new ImportPackFromUrlCommand { Url = source.TokenedUrl, SourcePaths = new[] { PrivatePackSource.Agent, PrivatePackSource.Skill } }); + + result.Items.ShouldAllBe(i => i.Outcome == PackImportOutcome.Imported); + PrivatePackSource.ShouldHoldNoToken(PackCredentialHarness.Serialize(result), "the import result"); + + PrivatePackSource.ShouldHoldNoToken(await harness.RawRowAsync(result.PackId), "the stored pack row"); + + var columns = await harness.ColumnsAsync(result.PackId); + columns.Url.ShouldBe(source.CleanUrl, "the pack is identified and shown by the URL without its userinfo"); + columns.EncryptedCloneUrl.ShouldNotBeNull("the token the import cloned with is kept, sealed"); + + (await harness.CloneUrlOfAsync(result.PackId)).ShouldBe(source.TokenedUrl, "the sealed source decrypts to exactly the URL the import cloned"); + + using var scope = _fixture.BeginScope(); + var pack = await scope.Resolve().Pack.AsNoTracking().SingleAsync(p => p.Id == result.PackId); + pack.Name.ShouldBe("remote", "the name is derived from the path, which the token never touched"); + pack.Kind.ShouldBe(PackKind.GitUrl); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] // a legacy row the backfill has not sealed yet: the read side is closed before the backfill runs + public async Task A_viewer_reads_the_pack_without_its_token_and_cannot_spend_it(bool legacyUnsealed) + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + var packId = await harness.ImportAsync(team, source.TokenedUrl, PrivatePackSource.Agent); + + if (legacyUnsealed) + { + await harness.MakeLegacyAsync(packId, source.TokenedUrl); + (await harness.RawRowAsync(packId)).ShouldContain(PrivatePackSource.Token, Case.Sensitive, "positive control: a legacy row holds the token at rest"); + } + + var list = await harness.SendAsync(team.ViewerId, team.TeamId, new ListPacksQuery()); + var detail = await harness.SendAsync(team.ViewerId, team.TeamId, new GetPackQuery { PackId = packId }); + + list.Single(p => p.Id == packId).Url.ShouldBe(source.CleanUrl); + detail.ShouldNotBeNull().Pack.Url.ShouldBe(source.CleanUrl); + PrivatePackSource.ShouldHoldNoToken(PackCredentialHarness.Serialize(list), "a Viewer's pack list"); + PrivatePackSource.ShouldHoldNoToken(PackCredentialHarness.Serialize(detail), "a Viewer's pack detail"); + + await Should.ThrowAsync(() => harness.SendAsync(team.ViewerId, team.TeamId, new SyncPackCommand { PackId = packId }), "a Viewer cannot make the server clone with the pack's token"); + await Should.ThrowAsync(() => harness.SendAsync(team.ViewerId, team.TeamId, new ImportPackArtifactsCommand { PackId = packId, SourcePaths = new[] { PrivatePackSource.Skill } }), "nor import with it"); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] // positive control: the same pack with its sealed source removed cannot sync + public async Task Sync_clones_with_the_sealed_token(bool sealedSourceRemoved) + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + var packId = await harness.ImportAsync(team, source.TokenedUrl, PrivatePackSource.Agent, PrivatePackSource.Skill); + var sealedBefore = (await harness.ColumnsAsync(packId)).EncryptedCloneUrl; + + await source.PublishUpstreamChangeAsync(); + + if (sealedSourceRemoved) + { + await harness.ClearSealedSourceAsync(packId); + + var failure = await Should.ThrowAsync(() => harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = packId })); + PrivatePackSource.ShouldHoldNoToken(failure.Message, "the failed sync's message"); + return; + } + + var sync = await harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = packId }); + + sync.Updated.ShouldBe(1, "the agent changed upstream, so the authenticated clone refreshed it"); + sync.UpToDate.ShouldBe(1); + sync.NewArtifacts.Skills.ShouldContain(s => s.SourcePath == PrivatePackSource.NewSkill); + PrivatePackSource.ShouldHoldNoToken(PackCredentialHarness.Serialize(sync), "the sync result"); + + using (var scope = _fixture.BeginScope()) + (await scope.Resolve().AgentDefinition.AsNoTracking().SingleAsync(a => a.PackId == packId && a.DeletedDate == null)).SystemPrompt.ShouldContain("v2 now"); + + PrivatePackSource.ShouldHoldNoToken(await harness.RawRowAsync(packId), "the pack row after a sync"); + (await harness.ColumnsAsync(packId)).EncryptedCloneUrl.ShouldBe(sealedBefore, "a sync spends the sealed source; it never re-seals it"); + } + + [Theory] + [InlineData(false)] // Sync + [InlineData(true)] // the add-after-sync import from the pack + public async Task A_failed_clone_from_the_sealed_source_names_no_token_to_the_caller_or_in_the_log(bool importFromPack) + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + var packId = await harness.ImportAsync(team, source.TokenedUrl, PrivatePackSource.Agent); + + // The token stays valid; the saved ref is gone upstream, so the clone authenticates with the decrypted URL and then fails. + await harness.ExecuteAsync("UPDATE pack SET reference = 'deleted-branch' WHERE id = @id", ("id", packId)); + + var log = new CapturedLog(); + var failure = importFromPack + ? await Should.ThrowAsync(() => harness.SendAsync(team.OwnerId, team.TeamId, new ImportPackArtifactsCommand { PackId = packId, SourcePaths = new[] { PrivatePackSource.Skill } }, log)) + : await Should.ThrowAsync(() => harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = packId }, log)); + + failure.Message.ShouldContain("deleted-branch", Case.Sensitive, "fixture check: the clone failed on the missing ref, so the sealed URL was decrypted and handed to git"); + log.Text.ShouldContain(failure.Message, Case.Sensitive, "fixture check: the pipeline logged the failure, so the scan below reads a log that holds it"); + + PrivatePackSource.ShouldHoldNoToken(FailureClassifier.Classify(failure).ClientMessage, "the error body the caller and the Library see"); + PrivatePackSource.ShouldHoldNoToken(log.Text, "the mediator's error log"); + } + + [Fact] + public async Task Importing_from_the_pack_lands_a_new_artifact_in_that_same_pack() + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + var packId = await harness.ImportAsync(team, source.TokenedUrl, PrivatePackSource.Agent); + var sealedBefore = (await harness.ColumnsAsync(packId)).EncryptedCloneUrl; + + await source.PublishUpstreamChangeAsync(); + + var result = await harness.SendAsync(team.OwnerId, team.TeamId, new ImportPackArtifactsCommand { PackId = packId, SourcePaths = new[] { PrivatePackSource.NewSkill } }); + + result.PackId.ShouldBe(packId); + result.Items.ShouldHaveSingleItem().Outcome.ShouldBe(PackImportOutcome.Imported, "the clone authenticated with the pack's sealed token"); + + using var scope = _fixture.BeginScope(); + var db = scope.Resolve(); + (await db.SkillDefinition.AsNoTracking().SingleAsync(s => s.Id == result.Items[0].DefinitionId)).PackId.ShouldBe(packId, "the artifact lands in the pack it was discovered in"); + (await db.Pack.AsNoTracking().CountAsync(p => p.TeamId == team.TeamId)).ShouldBe(1, "no second pack is resolved or created"); + + (await harness.ColumnsAsync(packId)).EncryptedCloneUrl.ShouldBe(sealedBefore, "the pack's own source cloned, so it is recorded unchanged"); + PrivatePackSource.ShouldHoldNoToken(await harness.RawRowAsync(packId), "the pack row after an import from it"); + } + + [Fact] + public async Task Importing_from_another_teams_pack_is_not_found() + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(); + var harness = new PackCredentialHarness(_fixture, source); + var owner = await harness.SeedTeamAsync(); + var other = await harness.SeedTeamAsync(); + var packId = await harness.ImportAsync(owner, source.TokenedUrl, PrivatePackSource.Agent); + + await Should.ThrowAsync(() => harness.SendAsync(other.OwnerId, other.TeamId, new ImportPackArtifactsCommand { PackId = packId, SourcePaths = new[] { PrivatePackSource.Skill } }), "another team's pack id resolves nothing — its sealed token is never spent"); + } + + [Theory] + [InlineData(true, "x-access-token:fake%2Dpublish-token-0123456789")] // a rotated token (here: a new spelling the remote still accepts) + [InlineData(false, null)] // the token dropped from a repository that is readable without it + public async Task Re_importing_with_a_rotated_or_dropped_token_keeps_the_same_pack(bool privateRemote, string? secondUserInfo) + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(authenticateReads: privateRemote); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + var first = await harness.ImportAsync(team, source.TokenedUrl, PrivatePackSource.Agent); + + var secondUrl = secondUserInfo is null ? source.CleanUrl : source.UrlWith(secondUserInfo); + var second = await harness.ImportAsync(team, secondUrl, PrivatePackSource.Agent, PrivatePackSource.Skill); + + second.ShouldBe(first, "the credential-free URL is the pack's identity, so a re-paste resolves to the same pack instead of forking one"); + + var columns = await harness.ColumnsAsync(first); + columns.Url.ShouldBe(source.CleanUrl); + + if (secondUserInfo is null) columns.EncryptedCloneUrl.ShouldBeNull("a clean re-import that cloned clears the stored credential"); + else (await harness.CloneUrlOfAsync(first)).ShouldBe(secondUrl, "the sealed source is now the URL the last successful import cloned"); + + var sync = await harness.SendAsync(team.OwnerId, team.TeamId, new SyncPackCommand { PackId = first }); + sync.UpToDate.ShouldBe(2, "the pack keeps syncing from the source it now records"); + } + + [Fact] + public async Task A_url_without_a_credential_is_stored_exactly_as_pasted_and_nothing_is_sealed() + { + if (!await PrivatePackSource.GitAvailableAsync()) return; + + await using var source = await PrivatePackSource.StartAsync(authenticateReads: false); + var harness = new PackCredentialHarness(_fixture, source); + var team = await harness.SeedTeamAsync(); + + var packId = await harness.ImportAsync(team, source.CleanUrl, PrivatePackSource.Agent); + + var columns = await harness.ColumnsAsync(packId); + columns.Url.ShouldBe(source.CleanUrl, "a public URL is byte-identical to the paste"); + columns.EncryptedCloneUrl.ShouldBeNull(); + (await harness.SendAsync(team.ViewerId, team.TeamId, new GetPackQuery { PackId = packId })).ShouldNotBeNull().Pack.Url.ShouldBe(source.CleanUrl); + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Agents/PrivatePackSource.cs b/backend/tests/CodeSpace.IntegrationTests/Agents/PrivatePackSource.cs new file mode 100644 index 000000000..7714511a1 --- /dev/null +++ b/backend/tests/CodeSpace.IntegrationTests/Agents/PrivatePackSource.cs @@ -0,0 +1,304 @@ +using System.Collections.Concurrent; +using System.Text.Json; +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.Enums; +using MediatR; +using Microsoft.EntityFrameworkCore; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; +using Npgsql; +using Shouldly; + +namespace CodeSpace.IntegrationTests.Agents; + +/// +/// A pack source for the pack-credential flows: the loopback smart-HTTP remote () +/// seeded with a pack — an agent and a skill — that, when is asked for a PRIVATE source, +/// refuses every request without the fake token, so a clone that succeeds proves the token authenticated it. The clone +/// is the production on the real , run under a scratch +/// HOME whose global config is its own and with system config off: no clone here — the refused, token-less ones +/// included — reads the real global config or asks the real keychain. The allowlist accepts the loopback http remote, +/// the one seam (the same AllowAll the sibling pack flows use). +/// +internal sealed class PrivatePackSource : IAsyncDisposable +{ + public const string Token = GitPublishRemoteFixture.FakeToken; + + public const string Agent = "agents/reviewer.md"; + public const string Skill = "skills/tdd/SKILL.md"; + public const string NewSkill = "skills/new-skill/SKILL.md"; + + private static readonly IReadOnlyDictionary InitialFiles = new Dictionary + { + [Agent] = "---\nname: reviewer\ndescription: Reviews PRs.\n---\nYou review v1.", + [Skill] = "---\nname: tdd\ndescription: Use when implementing.\n---\n# TDD v1", + }; + + private static readonly IReadOnlyDictionary UpstreamChange = new Dictionary + { + [Agent] = "---\nname: reviewer\ndescription: Reviews PRs.\n---\nYou review v2 now.", + [NewSkill] = "---\nname: new-skill\ndescription: Brand new.\n---\n# New", + }; + + private readonly string _home = Directory.CreateTempSubdirectory("cs-packcred-home-").FullName; + + private PrivatePackSource(bool authenticateReads) + { + Remote = new GitPublishRemoteFixture { AuthenticateReads = authenticateReads }; + + var environment = new Dictionary { ["HOME"] = _home, ["GIT_CONFIG_GLOBAL"] = Path.Combine(_home, ".gitconfig"), ["GIT_CONFIG_NOSYSTEM"] = "1" }; + Fetcher = new PackCloneFetcher(new AllowAll(), new SandboxRunnerRegistry(new ISandboxRunner[] { new ScratchHostRunner(environment) }), NullLogger.Instance); + } + + public GitPublishRemoteFixture Remote { get; } + + /// The production fetcher every flow in these tests clones through. + public IPackSourceFetcher Fetcher { get; } + + public string CleanUrl => Remote.Url; + + public string TokenedUrl => UrlWith($"x-access-token:{Token}"); + + /// The remote's URL with pasted into it, the way an operator would. + public string UrlWith(string userInfo) => Remote.Url.Replace("http://", $"http://{userInfo}@", StringComparison.Ordinal); + + public static async Task StartAsync(bool authenticateReads = true) + { + var source = new PrivatePackSource(authenticateReads); + + try + { + await source.Remote.StartAsync(); + await source.Remote.CommitFilesAsync("pack", InitialFiles); + return source; + } + catch + { + await source.DisposeAsync(); + throw; + } + } + + /// Change the agent and add a skill upstream — what a Sync must then see. + public Task PublishUpstreamChangeAsync() => Remote.CommitFilesAsync("upstream change", UpstreamChange); + + public static async Task GitAvailableAsync() + { + if (OperatingSystem.IsWindows()) return false; + + try { return (await new LocalProcessRunner().RunAsync(new SandboxSpec { Command = "git", Args = new[] { "--version" }, TimeoutSeconds = 15 }, CancellationToken.None)).Status == SandboxStatus.Success; } + catch { return false; } + } + + /// Every spelling of a leak could take — pasted, and percent-encoded the way the rotation test pastes it. + public static IEnumerable TokenSpellings() => new[] { Token, Token.Replace("-", "%2D", StringComparison.Ordinal), Token.Replace("-", "%2d", StringComparison.Ordinal) }; + + public static void ShouldHoldNoToken(string text, string what) + { + foreach (var spelling in TokenSpellings()) text.ShouldNotContain(spelling, Case.Insensitive, $"{what} must not hold the token ('{spelling}')"); + } + + public async ValueTask DisposeAsync() + { + await Remote.DisposeAsync(); + try { Directory.Delete(_home, recursive: true); } catch { /* best-effort */ } + } + + private sealed class AllowAll : IPackHostAllowlist + { + public bool IsAllowed(string url) => true; + public void EnsureAllowed(string url) { } + } + + /// The real local runner, with the scratch HOME and global config layered over every command's environment. + private sealed class ScratchHostRunner(IReadOnlyDictionary environment) : ISandboxRunner + { + private readonly LocalProcessRunner _inner = new(); + + public string Kind => LocalProcessRunner.LocalKind; + + public Task RunAsync(SandboxSpec spec, CancellationToken cancellationToken) + { + var env = new Dictionary(spec.Environment); + foreach (var (key, value) in environment) env[key] = value; + + return _inner.RunAsync(spec with { Environment = env }, cancellationToken); + } + } +} + +/// A team with an Owner (who imports and syncs) and a Viewer (who may only read). +internal sealed record PackCredentialTeam(Guid TeamId, Guid OwnerId, Guid ViewerId); + +/// The pack columns the credential flows assert on, read back without tracking. +internal sealed record PackColumns(string? Url, string? EncryptedCloneUrl, Guid? DuplicateOfPackId, DateTimeOffset? DeletedDate); + +/// +/// The real mediator, the real Postgres and the real production protector, as the pack-credential flows drive them: +/// requests are sent as a NON-Admin team member (Admin bypasses tenancy, so a Viewer's read is a real membership check), +/// and rows are read back both through EF and as raw SQL — the row exactly as stored, every column included. Without a +/// it serves the flows that never clone. +/// +internal sealed class PackCredentialHarness(PostgresFixture fixture, PrivatePackSource? source = null) +{ + public async Task SeedTeamAsync() + { + using var scope = fixture.BeginScope(); + var db = scope.Resolve(); + + var team = new PackCredentialTeam(Guid.NewGuid(), Guid.NewGuid(), Guid.NewGuid()); + + foreach (var (userId, role) in new[] { (team.OwnerId, TeamRole.Owner), (team.ViewerId, TeamRole.Viewer) }) + { + db.User.Add(new User { Id = userId, Email = $"packcred-{userId:N}@test.local", Name = $"packcred-{userId:N}" }); + db.TeamMembership.Add(new TeamMembership { Id = Guid.NewGuid(), TeamId = team.TeamId, UserId = userId, Role = role }); + } + + db.Team.Add(new Team { Id = team.TeamId, Slug = $"packcred-{team.TeamId:N}", Name = "Pack Credential Team", Kind = TeamKind.Workspace }); + + await db.SaveChangesAsync(); + return team; + } + + /// Send as in ; with , every line the pipeline logs for it lands there too. + public async Task SendAsync(Guid userId, Guid teamId, IRequest request, CapturedLog? log = null) + { + using var scope = fixture.BeginScope(b => + { + b.RegisterInstance(new TestCurrentUser(userId, "pack-credential")).As().SingleInstance(); + b.RegisterInstance(new TestCurrentTeam(teamId)).As().SingleInstance(); + if (source is not null) b.RegisterInstance(source.Fetcher).As().SingleInstance(); + if (log is not null) log.Register(b); + }); + + return await scope.Resolve().Send(request); + } + + /// Import from as the Owner; every path must land. + public async Task ImportAsync(PackCredentialTeam team, string url, params string[] sourcePaths) + { + var result = await SendAsync(team.OwnerId, team.TeamId, new ImportPackFromUrlCommand { Url = url, SourcePaths = sourcePaths }); + + result.Items.ShouldAllBe(i => i.Outcome == PackImportOutcome.Imported || i.Outcome == PackImportOutcome.Updated, "fixture check: the import landed every selected artifact"); + return result.PackId; + } + + /// One backfill pass through the real mediator, in its own scope — as a worker tick runs it. + public async Task BackfillPassAsync() + { + using var scope = fixture.BeginScope(); + await scope.Resolve().Send(new BackfillPackCloneUrlsCommand()); + } + + /// Run backfill passes until none of still carries a credential in its URL. The sweep is deployment-wide, so this waits on the rows the test owns, never on the pass's tally. + public async Task BackfillUntilSealedAsync(params Guid[] packIds) + { + const int maxPasses = 5; + + for (var pass = 0; pass < maxPasses; pass++) + { + await BackfillPassAsync(); + + var urls = await Task.WhenAll(packIds.Select(async id => (await ColumnsAsync(id)).Url!)); + if (!urls.Any(PackCloneUrlProtector.CarriesCredential)) return; + } + + throw new ShouldAssertException($"packs {string.Join(", ", packIds)} still carry a credential in pack.url after {maxPasses} backfill passes — check the candidate filter in PackCloneUrlBackfillService.LoadCandidatesAsync and the warnings it logged (a pack it could not seal stays a candidate)"); + } + + public async Task ColumnsAsync(Guid packId) + { + using var scope = fixture.BeginScope(); + + return await scope.Resolve().Pack.AsNoTracking().Where(p => p.Id == packId).Select(p => new PackColumns(p.Url, p.EncryptedCloneUrl, p.DuplicateOfPackId, p.DeletedDate)).SingleAsync(); + } + + /// The URL the production protector would clone the pack from. + public async Task CloneUrlOfAsync(Guid packId) + { + using var scope = fixture.BeginScope(); + var pack = await scope.Resolve().Pack.AsNoTracking().SingleAsync(p => p.Id == packId); + + return scope.Resolve().CloneUrlOf(pack); + } + + /// The row exactly as Postgres stores it, every column included — what a database dump or a backup would hold. + public async Task RawRowAsync(Guid packId) + { + await using var connection = new NpgsqlConnection(fixture.ConnectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand("SELECT row_to_json(p)::text FROM pack p WHERE id = @id", connection); + command.Parameters.AddWithValue("id", packId); + + return (string)(await command.ExecuteScalarAsync()).ShouldNotBeNull($"pack {packId} must exist"); + } + + /// Insert a pack row exactly as the code before the seal wrote one: the pasted URL verbatim, none of the new columns. + public async Task SeedLegacyPackAsync(PackCredentialTeam team, string url, DateTimeOffset createdDate, bool deleted = false) + { + var id = Guid.NewGuid(); + + await ExecuteAsync( + "INSERT INTO pack (id, team_id, kind, name, url, created_date, created_by, last_modified_date, last_modified_by, deleted_date) VALUES (@id, @team, 'GitUrl', 'remote', @url, @created, @user, @created, @user, @deleted)", + ("id", id), ("team", team.TeamId), ("url", url), ("created", createdDate), ("user", team.OwnerId), ("deleted", deleted ? (object)createdDate : DBNull.Value)); + + return id; + } + + /// Turn an imported pack back into the shape a legacy import left: the pasted URL in url, nothing sealed. + public Task MakeLegacyAsync(Guid packId, string url) => + ExecuteAsync("UPDATE pack SET url = @url, encrypted_clone_url = NULL, duplicate_of_pack_id = NULL WHERE id = @id", ("url", url), ("id", packId)); + + public Task ClearSealedSourceAsync(Guid packId) => ExecuteAsync("UPDATE pack SET encrypted_clone_url = NULL WHERE id = @id", ("id", packId)); + + public async Task ExecuteAsync(string sql, params (string Name, object Value)[] parameters) + { + await using var connection = new NpgsqlConnection(fixture.ConnectionString); + await connection.OpenAsync(); + await using var command = new NpgsqlCommand(sql, connection); + foreach (var (name, value) in parameters) command.Parameters.AddWithValue(name, value); + + return await command.ExecuteNonQueryAsync(); + } + + public static string Serialize(object value) => JsonSerializer.Serialize(value, new JsonSerializerOptions(JsonSerializerDefaults.Web)); +} + +/// +/// Every line the mediator pipeline logs inside one request scope — the logging behavior, the transaction behavior and the +/// failure observer, each with its exception rendered as a sink renders it — so a test can scan what an operator's log would +/// hold. It replaces the scope's and , which those scoped behaviors resolve. +/// +internal sealed class CapturedLog : ILoggerProvider, ILogger +{ + private readonly ConcurrentQueue _lines = new(); + + public string Text => string.Join('\n', _lines); + + public void Register(ContainerBuilder builder) + { + builder.RegisterInstance(LoggerFactory.Create(b => b.SetMinimumLevel(LogLevel.Trace).AddProvider(this))).As(); + builder.RegisterGeneric(typeof(Logger<>)).As(typeof(ILogger<>)); + } + + public ILogger CreateLogger(string categoryName) => this; + + 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) => + _lines.Enqueue($"{logLevel}: {formatter(state, exception)} {exception}"); + + public void Dispose() { } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Workflows/GitPublishRemoteFixture.cs b/backend/tests/CodeSpace.IntegrationTests/Workflows/GitPublishRemoteFixture.cs index ac53256e8..fb8403969 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Workflows/GitPublishRemoteFixture.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Workflows/GitPublishRemoteFixture.cs @@ -95,6 +95,21 @@ public async Task StartAsync() _accept = AcceptAsync(); } + /// Commit (repo-relative path → content) on top of main and publish it — content a test clones back through the remote, such as a pack's agents and skills. + public async Task CommitFilesAsync(string message, IReadOnlyDictionary files) + { + foreach (var (path, content) in files) + { + var full = Path.Combine(Seed, path.Replace('/', Path.DirectorySeparatorChar)); + Directory.CreateDirectory(Path.GetDirectoryName(full)!); + await File.WriteAllTextAsync(full, content); + } + + await GitAsync(Seed, new[] { "add", "." }); + await GitAsync(Seed, new[] { "commit", "-m", message }); + await PublishSeedAsync(); + } + /// The number of objects in the remote's LFS store that match . public bool HasLfsObject(string oid) => File.Exists(Path.Combine(LfsStore, oid)); diff --git a/backend/tests/CodeSpace.IntegrationTests/Workflows/SweepCommandTransactionFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Workflows/SweepCommandTransactionFlowTests.cs index ea16c78ad..f3c5503a8 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Workflows/SweepCommandTransactionFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Workflows/SweepCommandTransactionFlowTests.cs @@ -110,7 +110,7 @@ public async Task Budget_settlement_releases_the_terminal_map_branch_reservation /// /// Every sweep marked , dispatched through the real pipeline. The cases - /// differ only by which command is sent, so they are one Theory rather than nineteen copies of one fact. + /// differ only by which command is sent, so they are one Theory rather than twenty copies of one fact. /// /// Completing is the whole assertion, and it is not a weak one: three of these commands do NOT complete /// without the marker. Two of them open their own transaction on the scoped DbContext, which EF refuses while @@ -120,6 +120,7 @@ public async Task Budget_settlement_releases_the_terminal_map_branch_reservation /// can be seeded; this one measures only that the tick survives its own pipeline. /// [Theory] + [InlineData(typeof(BackfillPackCloneUrlsCommand))] [InlineData(typeof(BackfillRunScorecardsCommand))] [InlineData(typeof(DistillLessonsCommand))] [InlineData(typeof(MaterializeWorkflowRunModelCallBodiesCommand))] diff --git a/backend/tests/CodeSpace.UnitTests/Agents/PackCloneUrlProtectorTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/PackCloneUrlProtectorTests.cs new file mode 100644 index 000000000..063f7f23a --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Agents/PackCloneUrlProtectorTests.cs @@ -0,0 +1,130 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Agents; +using CodeSpace.Core.Services.Credentials; +using CodeSpace.Messages.Enums; +using Microsoft.AspNetCore.DataProtection; +using Shouldly; + +namespace CodeSpace.UnitTests.Agents; + +/// +/// 🟢 Unit: is the one place a pack's clone URL is split into the credential-free +/// URL the pack stores and shows, and the sealed URL it clones from. It runs over the REAL +/// (an ephemeral key ring, no mock), so a round trip proves the ciphertext +/// is what production would decrypt. The corpus covers every spelling a pasted token takes in a URL's userinfo, plus +/// the shapes that carry none — a public URL, an '@' in the path only, a local path — whose stored URL must stay +/// byte-identical to the paste. +/// +[Trait("Category", "Unit")] +public class PackCloneUrlProtectorTests +{ + /// A fake token; only its spellings are asserted on. + private const string Token = "ghp_FakePackToken0123456789"; + + /// A fake token whose URL spelling is percent-encoded (/ and @), so its decoded spelling differs from its pasted one. + private const string EncodedToken = "Fake%2Fpack%40Token"; + + private static readonly PackCloneUrlProtector Protector = new(new DataProtectionPayloadEncryptor(new EphemeralDataProtectionProvider())); + + /// (pasted URL, the secret it carries or null, the URL the pack must store). + public static TheoryData Corpus => new() + { + { "https://github.com/acme/agents", null, "https://github.com/acme/agents" }, + { $"https://x-access-token:{Token}@github.com/acme/agents", Token, "https://github.com/acme/agents" }, + { $"https://oauth2:{Token}@gitlab.com/acme/agents.git", Token, "https://gitlab.com/acme/agents.git" }, + { $"https://{Token}@github.com/acme/agents", Token, "https://github.com/acme/agents" }, + { $"https://{Token}:x-oauth-basic@github.com/acme/agents", Token, "https://github.com/acme/agents" }, + { $"https://x-access-token:{EncodedToken}@github.com/acme/agents", EncodedToken, "https://github.com/acme/agents" }, + { $"https://{Token}:@github.com/acme/agents", Token, "https://github.com/acme/agents" }, + { $"https://x-access-token:{Token}@git.example.com:8443/acme/agents.git", Token, "https://git.example.com:8443/acme/agents.git" }, + { $"https://x-access-token:{Token}@github.com./acme/agents", Token, "https://github.com./acme/agents" }, + { $"https://x-access-token:{Token}@github.com/acme/agents?ref=main", Token, "https://github.com/acme/agents?ref=main" }, + { $"http://x-access-token:{Token}@127.0.0.1:8080/remote.git", Token, "http://127.0.0.1:8080/remote.git" }, + { "https://github.com/acme/agents@v2", null, "https://github.com/acme/agents@v2" }, + { "/tmp/packs/a@b", null, "/tmp/packs/a@b" }, + }; + + [Theory] + [MemberData(nameof(Corpus))] + public void Sealing_keeps_the_credential_out_of_the_stored_url_and_clones_the_exact_paste(string pasted, string? secret, string expectedUrl) + { + var source = Protector.Seal(pasted); + + source.Url.ShouldBe(expectedUrl, "the pack stores the pasted URL with its userinfo removed — and a URL that carries none byte-identical"); + + if (secret is null) + { + source.EncryptedCloneUrl.ShouldBeNull("a URL that carries no credential has nothing to seal"); + return; + } + + foreach (var spelling in Spellings(secret)) + source.Url.ShouldNotContain(spelling, Case.Insensitive, $"the stored URL must not hold the token in any spelling ('{spelling}')"); + + source.EncryptedCloneUrl.ShouldNotBeNull("a URL that carries a credential is sealed"); + source.EncryptedCloneUrl.ShouldNotContain(secret, Case.Insensitive, "the sealed column holds ciphertext, not the URL"); + + Protector.CloneUrlOf(PackFrom(source)).ShouldBe(pasted, "Sync clones the exact string the import cloned — nothing is recomposed"); + } + + [Theory] + [MemberData(nameof(Corpus))] + public void CarriesCredential_agrees_with_whether_sealing_changes_the_url(string pasted, string? secret, string expectedUrl) + { + PackCloneUrlProtector.CarriesCredential(pasted).ShouldBe(secret is not null); + PackCloneUrlProtector.CarriesCredential(pasted).ShouldBe(Protector.Seal(pasted).Url != pasted, "the backfill's filter and the writer's split are one rule"); + PackCloneUrlProtector.WithoutCredential(pasted).ShouldBe(expectedUrl); + } + + [Fact] + public void The_corpus_discriminates_both_ways() + { + // Without rows of both kinds, a CarriesCredential that answered a constant would pass the agreement theory. + var secrets = Corpus.Select(row => (string?)row[1]).ToList(); + + secrets.Count(s => s is null).ShouldBeGreaterThanOrEqualTo(2, "the corpus needs URLs that carry no credential"); + secrets.Count(s => s is not null).ShouldBeGreaterThanOrEqualTo(2, "the corpus needs URLs that carry one"); + } + + [Fact] + public void An_unsealed_pack_clones_from_its_url() + { + // A legacy row the backfill has not reached yet keeps its token in the URL — and must keep syncing from it. + var legacy = $"https://x-access-token:{Token}@github.com/acme/agents"; + + Protector.CloneUrlOf(new Pack { Url = legacy, EncryptedCloneUrl = null }).ShouldBe(legacy); + } + + [Fact] + public void A_ciphertext_that_no_longer_decrypts_fails_without_naming_the_url_or_the_ciphertext() + { + var source = Protector.Seal($"https://x-access-token:{Token}@github.com/acme/agents"); + var tampered = source.EncryptedCloneUrl![..^4] + "AAAA"; + + var ex = Should.Throw(() => Protector.CloneUrlOf(new Pack { Url = source.Url, EncryptedCloneUrl = tampered })); + + ex.Message.ShouldNotContain(Token, Case.Insensitive); + ex.Message.ShouldNotContain(tampered); + ex.Message.ShouldNotContain(source.Url, Case.Insensitive, "the message is about the credential, not the source"); + ex.Message.ShouldContain("import it again", Case.Insensitive, "the operator is told how to recover"); + } + + [Fact] + public void A_ciphertext_from_another_key_ring_fails_the_same_way() + { + var foreign = new PackCloneUrlProtector(new DataProtectionPayloadEncryptor(new EphemeralDataProtectionProvider())).Seal($"https://x-access-token:{Token}@github.com/acme/agents"); + + Should.Throw(() => Protector.CloneUrlOf(PackFrom(foreign))).Message.ShouldNotContain(Token, Case.Insensitive); + } + + private static Pack PackFrom((string Url, string? EncryptedCloneUrl) source) => + new() { Id = Guid.NewGuid(), Kind = PackKind.GitUrl, Name = "pack", Url = source.Url, EncryptedCloneUrl = source.EncryptedCloneUrl }; + + /// The secret as pasted, decoded, and re-encoded — any of them in the stored URL would leak it. + private static IEnumerable Spellings(string secret) + { + var decoded = Uri.UnescapeDataString(secret); + + return new[] { secret, decoded, Uri.EscapeDataString(decoded) }.Distinct(StringComparer.Ordinal); + } +} diff --git a/backend/tests/CodeSpace.UnitTests/Agents/PackSummaryProjectionTests.cs b/backend/tests/CodeSpace.UnitTests/Agents/PackSummaryProjectionTests.cs new file mode 100644 index 000000000..0b78e208d --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Agents/PackSummaryProjectionTests.cs @@ -0,0 +1,48 @@ +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.Core.Services.Agents; +using CodeSpace.Messages.Enums; +using Shouldly; + +namespace CodeSpace.UnitTests.Agents; + +/// +/// 🟢 Unit: the pack read model every team member — Viewers included — receives never carries a URL's userinfo, even +/// for a row the clone-URL backfill has not sealed yet. The writer already stores a clean URL; this pins the read side +/// so the API is closed the moment the code deploys, not when the backfill reaches the row. +/// +[Trait("Category", "Unit")] +public class PackSummaryProjectionTests +{ + private const string Token = "ghp_FakeSummaryToken0123456789"; + + [Theory] + [InlineData("https://x-access-token:" + Token + "@github.com/acme/agents", "https://github.com/acme/agents")] // a legacy, unsealed row + [InlineData("https://" + Token + "@gitlab.com/acme/agents.git", "https://gitlab.com/acme/agents.git")] // a token pasted as the user alone + public void A_url_that_still_carries_a_credential_is_summarized_without_it(string stored, string expected) + { + var summary = PackService.ToSummary(PackWithUrl(stored), agentCount: 1, skillCount: 0); + + summary.Url.ShouldBe(expected); + summary.Url.ShouldNotContain(Token, Case.Insensitive); + } + + [Theory] + [InlineData("https://github.com/acme/agents")] + [InlineData("https://github.com/acme/agents@v2")] + [InlineData(null)] + public void A_clean_or_absent_url_is_summarized_byte_identical(string? stored) + { + PackService.ToSummary(PackWithUrl(stored), agentCount: 0, skillCount: 1).Url.ShouldBe(stored); + } + + [Fact] + public void The_summary_carries_no_sealed_source() + { + // The read model has no field the ciphertext could land in — a property added for it would show up here. + typeof(CodeSpace.Messages.Dtos.Agents.PackSummary).GetProperties().Select(p => p.Name) + .ShouldNotContain(name => name.Contains("Encrypted", StringComparison.OrdinalIgnoreCase) || name.Contains("Clone", StringComparison.OrdinalIgnoreCase) || name.Contains("Credential", StringComparison.OrdinalIgnoreCase)); + } + + private static Pack PackWithUrl(string? url) => + new() { Id = Guid.NewGuid(), TeamId = Guid.NewGuid(), Kind = url is null ? PackKind.Custom : PackKind.GitUrl, Name = "agents", Url = url, EncryptedCloneUrl = url is null ? null : "sealed" }; +} diff --git a/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs b/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs index eda533c36..e93c40ad3 100644 --- a/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Architecture/RequestAuthorizationInventoryTests.cs @@ -74,6 +74,7 @@ public class RequestAuthorizationInventoryTests ["ReconcileStuckWebhookRegistrationsCommand"] = "sweep", ["SweepBudgetSettlementCommand"] = "sweep", ["SweepCompletionShadowCommand"] = "sweep", + ["BackfillPackCloneUrlsCommand"] = "seals the clone URL of pack rows that still hold a pasted token in plaintext, across every team, in bounded batches; it changes where a pack's existing credential is kept, never which source a pack clones or what any user can reach, and a sealed row is no longer a candidate.", ["BackfillRunScorecardsCommand"] = "projects the observation-only north-star row for terminal runs that terminalized before the table existed, across every team, in bounded batches; a run that already has a row is not a candidate, so it can only ever add a measurement of a settled run — it authors nothing a user owns and changes no run's outcome.", ["SweepStaleAgentWorkspacesCommand"] = "sweep", ["TierStaleModelCapabilitiesCommand"] = "sweep", diff --git a/backend/tests/CodeSpace.UnitTests/Jobs/PackCloneUrlBackfillDispatchTests.cs b/backend/tests/CodeSpace.UnitTests/Jobs/PackCloneUrlBackfillDispatchTests.cs new file mode 100644 index 000000000..a35bf6c3f --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Jobs/PackCloneUrlBackfillDispatchTests.cs @@ -0,0 +1,104 @@ +using CodeSpace.Core.Handlers.CommandHandlers.Agents; +using CodeSpace.Core.Jobs.RecurringJobs; +using CodeSpace.Core.Services.Agents; +using CodeSpace.Messages.Commands.Agents; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Mediation; +using MediatR; +using Shouldly; + +namespace CodeSpace.UnitTests.Jobs; + +/// +/// 🟢 Unit: the pack clone-URL backfill is the standard job → command → service chain (Rule 14 + Rule 16) — the job +/// holds no query, the handler holds no logic — and the import-from-pack command is gated like every other pack write. +/// Hand-rolled recording doubles, matching the codebase convention. +/// +[Trait("Category", "Unit")] +public class PackCloneUrlBackfillDispatchTests +{ + [Fact] + public async Task The_backfill_job_dispatches_its_command_every_ten_minutes_and_holds_no_logic() + { + var mediator = new RecordingMediator(); + var job = new PackCloneUrlBackfillRecurringJob(mediator); + + job.JobId.ShouldBe(nameof(PackCloneUrlBackfillRecurringJob), "Hangfire indexes the schedule by this id — a rename strands the old one"); + job.CronExpression.ShouldBe("*/10 * * * *", "a committed cadence: plaintext tokens left by an older pod are sealed within ten minutes"); + + await job.Execute(); + + mediator.Sent.ShouldHaveSingleItem().ShouldBeOfType("the job is a thin dispatcher — it only sends the command"); + } + + [Fact] + public async Task The_backfill_handler_forwards_the_batch_size_and_returns_the_sealed_count() + { + var backfill = new RecordingBackfill { ToReturn = 3 }; + var handler = new BackfillPackCloneUrlsCommandHandler(backfill); + + var sealedCount = await handler.Handle(new BackfillPackCloneUrlsCommand { BatchSize = 7 }, CancellationToken.None); + + backfill.BatchSizes.ShouldHaveSingleItem().ShouldBe(7, "the handler delegates the whole pass to the service (Rule 16)"); + sealedCount.ShouldBe(3, "the handler surfaces the service's count verbatim"); + } + + [Fact] + public void The_backfill_command_is_bounded_and_settles_each_pack_on_its_own() + { + new BackfillPackCloneUrlsCommand().BatchSize.ShouldBe(50, "an unbounded pass would let one tick sweep every pack in the deployment"); + + typeof(INonTransactionalCommand).IsAssignableFrom(typeof(BackfillPackCloneUrlsCommand)) + .ShouldBeTrue("each pack is sealed by its own conditional UPDATE; one transaction around the pass would let one failing pack undo the rest"); + } + + [Fact] + public void Importing_from_a_saved_pack_needs_the_same_permission_as_importing_from_a_url() + { + new ImportPackArtifactsCommand().RequiredPermission.ShouldBe(TeamPermissions.AgentsWrite, "it spends the pack's saved credential, exactly like Sync"); + new ImportPackArtifactsCommand().RequiredPermission.ShouldBe(new ImportPackFromUrlCommand { Url = "https://github.com/acme/agents" }.RequiredPermission); + new ImportPackArtifactsCommand().RequiredPermission.ShouldBe(new SyncPackCommand { PackId = Guid.NewGuid() }.RequiredPermission); + } + + /// Records the requests sent through the mediator; the rest of the surface is unreachable in these tests. + private sealed class RecordingMediator : IMediator + { + public List Sent { get; } = new(); + + public Task Send(IRequest request, CancellationToken cancellationToken = default) + { + Sent.Add(request); + return Task.FromResult(default(TResponse)!); + } + + public Task Send(object request, CancellationToken cancellationToken = default) + { + Sent.Add(request); + return Task.FromResult(null); + } + + public Task Send(TRequest request, CancellationToken cancellationToken = default) where TRequest : IRequest + { + Sent.Add(request!); + return Task.CompletedTask; + } + + public IAsyncEnumerable CreateStream(IStreamRequest request, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + public IAsyncEnumerable CreateStream(object request, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + public Task Publish(object notification, CancellationToken cancellationToken = default) => throw new NotSupportedException(); + public Task Publish(TNotification notification, CancellationToken cancellationToken = default) where TNotification : INotification => throw new NotSupportedException(); + } + + /// Records the batch sizes asked for + returns a canned count, so the handler test asserts pure delegation. + private sealed class RecordingBackfill : IPackCloneUrlBackfillService + { + public List BatchSizes { get; } = new(); + public int ToReturn; + + public Task BackfillAsync(int batchSize, CancellationToken cancellationToken) + { + BatchSizes.Add(batchSize); + return Task.FromResult(ToReturn); + } + } +} diff --git a/frontend/src/api/packs.ts b/frontend/src/api/packs.ts index 20a3de1bf..d101a2b2d 100644 --- a/frontend/src/api/packs.ts +++ b/frontend/src/api/packs.ts @@ -143,4 +143,11 @@ export const packsApi = { /** Re-pull a pack from its saved source — refresh its imported artifacts, return what changed + the new ones. */ sync: (packId: string) => fetchJson(`/api/packs/${packId}/sync`, { method: "POST" }), + + /** + * Add the selected artifacts a sync discovered to that same pack. The server clones the pack's own saved source + ref + * (a private pack's credential never leaves the server), so the body carries only the selection — never a URL. + */ + importFromPack: (packId: string, sourcePaths: string[]) => + fetchJson(`/api/packs/${packId}/import`, { method: "POST", body: JSON.stringify({ sourcePaths }) }), }; diff --git a/frontend/src/components/library/SyncResultModal.test.tsx b/frontend/src/components/library/SyncResultModal.test.tsx new file mode 100644 index 000000000..18aefc50d --- /dev/null +++ b/frontend/src/components/library/SyncResultModal.test.tsx @@ -0,0 +1,79 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import type { PackSummary, PackSyncResult } from "@/api/packs"; + +import { SyncResultModal } from "./SyncResultModal"; + +const TOKEN = "fake-pack-token-0123456789"; +const NEW_SKILL = "skills/new-skill/SKILL.md"; + +const pack: PackSummary = { + id: "pack-1", kind: "GitUrl", name: "acme/agents", url: "https://git.example.test/acme/agents", reference: "main", + lastSyncedSha: null, lastSyncedDate: null, agentCount: 1, skillCount: 0, +}; + +const result: PackSyncResult = { + packId: pack.id, reference: "main", upToDate: 1, updated: 0, + newArtifacts: { + reference: "main", + agents: [], + skills: [{ sourcePath: NEW_SKILL, name: "new-skill", derivedSlug: "new-skill", description: "Brand new.", body: "# New", category: null, rawFrontmatterJson: "{}", diagnostics: [], slugConflict: false, importable: true }], + }, +}; + +interface Call { path: string; method: string; body: string } + +function json(body: unknown) { + return new Response(JSON.stringify(body), { status: 200, headers: { "Content-Type": "application/json" } }); +} + +function renderModal(shown: PackSummary) { + const calls: Call[] = []; + const onClose = vi.fn(); + localStorage.setItem("codespace.jwt", "test-jwt"); + vi.stubGlobal("fetch", vi.fn(async (input: RequestInfo | URL, init: RequestInit = {}) => { + const path = new URL(typeof input === "string" ? input : input.toString(), "http://test.local").pathname; + calls.push({ path, method: init.method ?? "GET", body: init.body ? String(init.body) : "" }); + return json({ packId: shown.id, items: [{ sourcePath: NEW_SKILL, kind: "Skill", outcome: "Imported", definitionId: "s1", reason: null }] }); + })); + + const client = new QueryClient({ defaultOptions: { queries: { retry: false, gcTime: 0 }, mutations: { retry: false } } }); + render( + + + , + ); + return { calls, onClose }; +} + +afterEach(() => { localStorage.clear(); vi.unstubAllGlobals(); }); + +describe("SyncResultModal", () => { + // The add-after-sync used to post the pack's saved URL back to /api/agents/import-url. A private pack's URL no longer + // carries the credential its clone needs, and the URL could resolve a different pack — so the add names the pack by id + // and lets the server clone the pack's own sealed source. + it("adds the selection to the synced pack by id and sends only the selection", async () => { + const { calls, onClose } = renderModal(pack); + + fireEvent.click(screen.getByRole("button", { name: /Add 1/ })); + + await waitFor(() => expect(onClose).toHaveBeenCalled()); + expect(calls).toHaveLength(1); + expect(calls[0].path).toBe("/api/packs/pack-1/import"); + expect(calls[0].method).toBe("POST"); + expect(JSON.parse(calls[0].body)).toEqual({ sourcePaths: [NEW_SKILL] }); + }); + + it("never sends the pack's URL, even one cached from before the server stripped its credential", async () => { + const { calls, onClose } = renderModal({ ...pack, url: `https://x-access-token:${TOKEN}@git.example.test/acme/agents` }); + + fireEvent.click(screen.getByRole("button", { name: /Add 1/ })); + + await waitFor(() => expect(onClose).toHaveBeenCalled()); + expect(calls.map((c) => c.path)).not.toContain("/api/agents/import-url"); + expect(calls.map((c) => c.body).join("\n")).not.toContain(TOKEN); + expect(Object.keys(JSON.parse(calls[0].body))).toEqual(["sourcePaths"]); + }); +}); diff --git a/frontend/src/components/library/SyncResultModal.tsx b/frontend/src/components/library/SyncResultModal.tsx index 8ac14a357..f03a52578 100644 --- a/frontend/src/components/library/SyncResultModal.tsx +++ b/frontend/src/components/library/SyncResultModal.tsx @@ -6,15 +6,16 @@ import type { PackSummary, PackSyncResult } from "@/api/packs"; import { ApiError } from "@/api/request"; import { defaultSelectedPaths, toRows } from "@/components/agents/packPreview"; import { PreviewGroup } from "@/components/agents/packPreviewRows"; -import { useImportPack } from "@/hooks/use-packs"; +import { useImportFromPack } from "@/hooks/use-packs"; import { newArtifactCount, syncSummaryLabel } from "./syncView"; /** * Sync result modal — shown after re-pulling a pack. The header reports what changed (up to date / updated / - * new), and any discovered-but-not-imported artifacts are listed for the operator to select and add (committed - * via the same import path as a first import, so the pack's saved URL + ref drive the commit). A pure-refresh - * sync with nothing new is a one-line "everything's in sync" with a Done button. + * new), and any discovered-but-not-imported artifacts are listed for the operator to select and add — committed into + * THIS pack by id, so the server clones the pack's own saved source + ref (a private pack's credential never reaches the + * browser, and the add never resolves a different pack). A pure-refresh sync with nothing new is a one-line + * "everything's in sync" with a Done button. */ export function SyncResultModal({ pack, result, onClose }: { pack: PackSummary; result: PackSyncResult; onClose: () => void }) { const rows = toRows(result.newArtifacts); @@ -23,7 +24,7 @@ export function SyncResultModal({ pack, result, onClose }: { pack: PackSummary; const hasNew = newArtifactCount(result) > 0; const [selected, setSelected] = useState>(() => new Set(defaultSelectedPaths(result.newArtifacts))); - const importPack = useImportPack(); + const importPack = useImportFromPack(); // While an add is committing, dismissal is blocked so a partial-failure error can't be lost with the modal. const dismiss = () => { if (!importPack.isPending) onClose(); }; @@ -43,7 +44,7 @@ export function SyncResultModal({ pack, result, onClose }: { pack: PackSummary; } function add() { - importPack.mutate({ url: pack.url ?? "", reference: result.reference ?? "", sourcePaths: [...selected] }, { onSuccess: onClose }); + importPack.mutate({ packId: pack.id, sourcePaths: [...selected] }, { onSuccess: onClose }); } const importErr = importPack.error instanceof ApiError ? importPack.error.message : importPack.error ? "Couldn't add the selected artifacts." : null; diff --git a/frontend/src/hooks/use-packs.ts b/frontend/src/hooks/use-packs.ts index 19383e02d..d271c6028 100644 --- a/frontend/src/hooks/use-packs.ts +++ b/frontend/src/hooks/use-packs.ts @@ -1,4 +1,4 @@ -import { keepPreviousData, useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; +import { keepPreviousData, type QueryClient, useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { packsApi, type PackArtifactKind } from "@/api/packs"; @@ -52,19 +52,31 @@ export function usePreviewPack() { }); } +/** + * An import creates/updates a pack AND its artifacts — refresh the packs rail + every pack detail (the just-imported + * pack is on the Library page the user is looking at), the paged artifact lists, and the agents library. + */ +function invalidateImported(queryClient: QueryClient) { + queryClient.invalidateQueries({ queryKey: ["packs"] }); + queryClient.invalidateQueries({ queryKey: ["pack"] }); + queryClient.invalidateQueries({ queryKey: ["pack-artifacts"] }); + queryClient.invalidateQueries({ queryKey: ["agents"] }); +} + export function useImportPack() { const queryClient = useQueryClient(); return useMutation({ mutationFn: ({ url, reference, sourcePaths }: { url: string; reference: string; sourcePaths: string[] }) => packsApi.importFromUrl(url, reference, sourcePaths), - onSuccess: () => { - // An import creates/updates a pack AND its artifacts — refresh the packs rail + every pack detail - // (the just-imported pack is on the Library page the user is looking at), the paged artifact lists, and - // the agents library. - queryClient.invalidateQueries({ queryKey: ["packs"] }); - queryClient.invalidateQueries({ queryKey: ["pack"] }); - queryClient.invalidateQueries({ queryKey: ["pack-artifacts"] }); - queryClient.invalidateQueries({ queryKey: ["agents"] }); - }, + onSuccess: () => invalidateImported(queryClient), + }); +} + +/** Add the artifacts a sync discovered to the pack it synced — cloned server-side from that pack's saved source, so no URL is sent. */ +export function useImportFromPack() { + const queryClient = useQueryClient(); + return useMutation({ + mutationFn: ({ packId, sourcePaths }: { packId: string; sourcePaths: string[] }) => packsApi.importFromPack(packId, sourcePaths), + onSuccess: () => invalidateImported(queryClient), }); }