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), }); }