diff --git a/backend/src/CodeSpace.Api/Controllers/AuthController.cs b/backend/src/CodeSpace.Api/Controllers/AuthController.cs index 5969db89d..20682332e 100644 --- a/backend/src/CodeSpace.Api/Controllers/AuthController.cs +++ b/backend/src/CodeSpace.Api/Controllers/AuthController.cs @@ -25,7 +25,9 @@ public class AuthController : ControllerBase // Every endpoint that mints a session hands it to the browser in the HttpOnly session cookie, which its scripts never // see (SessionCookie), and to a caller that keeps it itself — an API client, the CLI — in the body as well. Never in - // the body to a request the cookie carried: that caller may be a script in the page, and it could read a body. + // the body to a request the cookie carried: that caller may be a script in the page, and it could read a body. And + // the cookie only to a request that carries the SPA's CSRF header: a client that predates the cookie could never + // clear it, so it keeps the body's session alone (SessionCookie.IssueIfRequested). [HttpPost("sign-in")] [AllowAnonymous] @@ -33,7 +35,7 @@ public async Task SignIn([FromBody] SignInCommand command, Cancel { var result = await _mediator.Send(command, cancellationToken).ConfigureAwait(false); - SessionCookie.Issue(HttpContext, result.Token!, result.ExpiresAt); + SessionCookie.IssueIfRequested(HttpContext, result.Token!, result.ExpiresAt); return Ok(SessionCookie.Authenticated(HttpContext) ? result with { Token = null } : result); } @@ -48,7 +50,7 @@ public async Task ChangePassword([FromBody] ChangePasswordCommand { var result = await _mediator.Send(command, cancellationToken).ConfigureAwait(false); - SessionCookie.Issue(HttpContext, result.Token!, result.ExpiresAt); + SessionCookie.IssueIfRequested(HttpContext, result.Token!, result.ExpiresAt); return Ok(SessionCookie.Authenticated(HttpContext) ? result with { Token = null } : result); } diff --git a/backend/src/CodeSpace.Api/Controllers/InvitationsController.cs b/backend/src/CodeSpace.Api/Controllers/InvitationsController.cs index 2c0c6ad62..052b383a0 100644 --- a/backend/src/CodeSpace.Api/Controllers/InvitationsController.cs +++ b/backend/src/CodeSpace.Api/Controllers/InvitationsController.cs @@ -38,16 +38,17 @@ public async Task Preview([FromRoute] string token, CancellationT /// /// Spends the invitation and answers with a session, so the invitee lands signed in: as the browser's HttpOnly session - /// cookie, and in the body too unless the cookie carried the request. A signed-in caller accepts as itself, and any - /// account can invite any address, so a page script accepting an invitation its author sent would otherwise read a - /// fresh session for the person whose page it runs in. + /// cookie when the request carries the SPA's CSRF header (a client that predates the cookie could never clear one), and + /// in the body too unless the cookie carried the request. A signed-in caller accepts as itself, and any account can + /// invite any address, so a page script accepting an invitation its author sent would otherwise read a fresh session + /// for the person whose page it runs in. /// [HttpPost("{token}/accept")] public async Task Accept([FromRoute] string token, [FromBody] AcceptInvitationCommand command, CancellationToken cancellationToken) { var result = await _mediator.Send(command with { Token = token }, cancellationToken).ConfigureAwait(false); - SessionCookie.Issue(HttpContext, result.Token!, result.ExpiresAt); + SessionCookie.IssueIfRequested(HttpContext, result.Token!, result.ExpiresAt); return Ok(SessionCookie.Authenticated(HttpContext) ? result with { Token = null } : result); } diff --git a/backend/src/CodeSpace.Core/Handlers/QueryHandlers/Repositories/ListIssuesQueryHandler.cs b/backend/src/CodeSpace.Core/Handlers/QueryHandlers/Repositories/ListIssuesQueryHandler.cs index 39a2c6eb8..782f8b9b6 100644 --- a/backend/src/CodeSpace.Core/Handlers/QueryHandlers/Repositories/ListIssuesQueryHandler.cs +++ b/backend/src/CodeSpace.Core/Handlers/QueryHandlers/Repositories/ListIssuesQueryHandler.cs @@ -16,8 +16,11 @@ public sealed class ListIssuesQueryHandler : IRequestHandler> Handle(ListIssuesQuery request, CancellationToken cancellationToken) { // Same clamp discipline as ListPullRequestsQueryHandler — handler is dispatch-only per Rule 16. The ceilings live on - // the query record itself; the handler holds the caller to them. - var page = Math.Clamp(request.Page, 1, ListIssuesQuery.MaxPage); + // the query record itself; the handler holds the caller to them. A page past the last one the list allows is empty, + // not the last page again. + if (request.Page > ListIssuesQuery.MaxPage) return []; + + var page = Math.Max(request.Page, 1); var perPage = Math.Clamp(request.PerPage, 1, ListIssuesQuery.MaxPerPage); return await _service.ListAsync(request.RepositoryId, _currentTeam.Id!.Value, request.State, page, perPage, cancellationToken).ConfigureAwait(false); } diff --git a/backend/src/CodeSpace.Core/Handlers/QueryHandlers/Repositories/ListPullRequestsQueryHandler.cs b/backend/src/CodeSpace.Core/Handlers/QueryHandlers/Repositories/ListPullRequestsQueryHandler.cs index 261e53c84..1c21ce2fd 100644 --- a/backend/src/CodeSpace.Core/Handlers/QueryHandlers/Repositories/ListPullRequestsQueryHandler.cs +++ b/backend/src/CodeSpace.Core/Handlers/QueryHandlers/Repositories/ListPullRequestsQueryHandler.cs @@ -15,8 +15,12 @@ public sealed class ListPullRequestsQueryHandler : IRequestHandler> Handle(ListPullRequestsQuery request, CancellationToken cancellationToken) { - // The ceilings live on the query record itself; the handler holds the caller to them. - var page = Math.Clamp(request.Page, 1, ListPullRequestsQuery.MaxPage); + // The ceilings live on the query record itself; the handler holds the caller to them. A page past the last one the + // list allows is empty: clamping it to the last page would hand back that page's rows again, as the answer for a page + // further on, to a caller who pages until one comes back empty. + if (request.Page > ListPullRequestsQuery.MaxPage) return []; + + var page = Math.Max(request.Page, 1); var perPage = Math.Clamp(request.PerPage, 1, ListPullRequestsQuery.MaxPerPage); return await _service.ListAsync(request.RepositoryId, _currentTeam.Id!.Value, request.State, page, perPage, cancellationToken).ConfigureAwait(false); } diff --git a/backend/src/CodeSpace.Core/Services/Auth/SessionCookie.cs b/backend/src/CodeSpace.Core/Services/Auth/SessionCookie.cs index 1a84a6cc1..99638cdba 100644 --- a/backend/src/CodeSpace.Core/Services/Auth/SessionCookie.cs +++ b/backend/src/CodeSpace.Core/Services/Auth/SessionCookie.cs @@ -37,6 +37,18 @@ public static class SessionCookie /// Hands the browser the session , to expire with the token itself. public static void Issue(HttpContext context, string token, DateTimeOffset expiresAt) => context.Response.Cookies.Append(Name, token, Options(context, expiresAt - ClockOf(context).GetUtcNow())); + /// + /// for an endpoint that mints a session for whoever calls it, only when the request carries + /// : the SPA that knows the cookie sends it on every request, and nothing older does. A client + /// that predates the cookie — an earlier SPA bundle, an API client, the CLI — cannot end one: only the API can clear an + /// HttpOnly cookie, and that bundle's sign-out empties its own storage and nothing else. Handed one, it would hold a + /// 24-hour session it can never sign out of. It keeps the session the body carries, as it always did. + /// + public static void IssueIfRequested(HttpContext context, string token, DateTimeOffset expiresAt) + { + if (CarriesCsrfHeader(context.Request)) Issue(context, token, expiresAt); + } + /// Tells the browser to drop the session cookie. Written with the attributes it was set with, or the browser treats it as a different cookie and keeps the original. public static void Clear(HttpContext context) => context.Response.Cookies.Delete(Name, Options(context, maxAge: null)); @@ -55,7 +67,7 @@ public static class SessionCookie public static bool Authenticated(HttpContext context) => context.User.Identity?.IsAuthenticated == true && ReadToken(context.Request) != null; /// True for a request that would change something on the strength of the cookie alone: any method not known to be safe, without . - public static bool IsUnguardedWrite(HttpRequest request) => !IsSafeMethod(request.Method) && string.IsNullOrWhiteSpace(request.Headers[CsrfHeader]); + public static bool IsUnguardedWrite(HttpRequest request) => !IsSafeMethod(request.Method) && !CarriesCsrfHeader(request); /// /// Secure exactly when the request arrived over HTTPS — the effective scheme, after whatever forwarded-headers @@ -73,6 +85,9 @@ public static class SessionCookie IsEssential = true, }; + /// A blank header is no header: it proves nothing a form could not have sent, which is what the header is for. + private static bool CarriesCsrfHeader(HttpRequest request) => !string.IsNullOrWhiteSpace(request.Headers[CsrfHeader]); + private static bool IsSafeMethod(string method) => HttpMethods.IsGet(method) || HttpMethods.IsHead(method) || HttpMethods.IsOptions(method) || HttpMethods.IsTrace(method); private static TimeProvider ClockOf(HttpContext context) => context.RequestServices?.GetService() ?? TimeProvider.System; diff --git a/backend/src/CodeSpace.Core/Services/Workflows/Nodes/Builtin/GitListPullRequestsNode.cs b/backend/src/CodeSpace.Core/Services/Workflows/Nodes/Builtin/GitListPullRequestsNode.cs index 3024cee02..984030b4c 100644 --- a/backend/src/CodeSpace.Core/Services/Workflows/Nodes/Builtin/GitListPullRequestsNode.cs +++ b/backend/src/CodeSpace.Core/Services/Workflows/Nodes/Builtin/GitListPullRequestsNode.cs @@ -1,6 +1,7 @@ using System.Text.Json; using CodeSpace.Core.Services.PullRequests; using CodeSpace.Core.Services.Workflows.Runtime; +using CodeSpace.Messages.Dtos.Providers; using CodeSpace.Messages.Enums; using CodeSpace.Messages.Queries.Repositories; using Microsoft.Extensions.Logging; @@ -16,7 +17,8 @@ namespace CodeSpace.Core.Services.Workflows.Nodes.Builtin; /// (e.g. "review every open PR": list → loop → fetch diff → agent → post review). Read-only; reuses /// the same catalog capability the Pulls tab uses, so it's provider-agnostic (GitHub + GitLab). Page and page size are /// held to the Pulls tab's own ceilings (, ): -/// called by an agent, they are the model's numbers. +/// called by an agent, they are the model's numbers. A page size past its ceiling is the ceiling; a page past its is an +/// empty page, with no request to the provider. /// public sealed class GitListPullRequestsNode : INodeRuntime { @@ -59,7 +61,7 @@ public GitListPullRequestsNode(IPullRequestService prService) "properties": { "repositoryId": { "type": "string", "format": "uuid", "x-selector": "repository", "description": "The repository. Pick one, or switch to Expression to bind from the trigger (e.g. {{trigger.repositoryId}})." }, "state": { "type": "string", "enum": ["Open","Draft","Merged","Closed"], "description": "Only list requests in this state. Leave empty to list all." }, - "page": { "type": "integer", "minimum": 1, "maximum": 1000, "x-control": "stepper", "x-advanced": true, "description": "Page of results (default 1, max 1000)." }, + "page": { "type": "integer", "minimum": 1, "maximum": 1000, "x-control": "stepper", "x-advanced": true, "description": "Page of results (default 1, max 1000; a later page is empty)." }, "perPage": { "type": "integer", "minimum": 1, "maximum": 100, "default": 30, "x-control": "stepper", "x-advanced": true, "description": "Results per page (default 30, max 100)." } }, "required": ["repositoryId"] @@ -103,9 +105,11 @@ public async Task RunAsync(NodeRunContext context, CancellationToken if (!TryReadState(context, out var state)) return NodeResult.Fail("Input 'state' must be one of Open, Draft, Merged, Closed."); if (!NodeScopeReader.TryReadTeamId(context, out var teamId)) return NodeResult.Fail("This run has no team context, so a repository can't be resolved."); - var page = Math.Min(ReadPositiveInt(context, "page", DefaultPage), ListPullRequestsQuery.MaxPage); + var page = ReadPositiveInt(context, "page", DefaultPage); var perPage = Math.Min(ReadPositiveInt(context, "perPage", DefaultPerPage), ListPullRequestsQuery.MaxPerPage); + if (page > ListPullRequestsQuery.MaxPage) return PastTheLastPage(context, repoId, page); + var pullRequests = await context.Observability.TraceExternalCallAsync( target: $"git.list_prs:{repoId}", method: "list_pull_requests", @@ -117,17 +121,29 @@ public async Task RunAsync(NodeRunContext context, CancellationToken }, cancellationToken: cancellationToken).ConfigureAwait(false); - var outputs = new Dictionary - { - ["pullRequests"] = JsonSerializer.SerializeToElement(pullRequests, WorkflowJson.Options), - ["count"] = JsonSerializer.SerializeToElement(pullRequests.Count) - }; - context.Logger.LogInformation("Listed {Count} pull request(s) for repo {RepoId} (state {State})", pullRequests.Count, repoId, state?.ToString() ?? "any"); - return NodeResult.Ok(outputs); + return NodeResult.Ok(OutputsOf(pullRequests)); } + /// + /// A page past the last one the list allows is an empty page. Reading the last page for it instead returned the rows the + /// caller had just seen under a page number that said it was further on, so a loop paging until it got an empty page + /// never got one. + /// + private static NodeResult PastTheLastPage(NodeRunContext context, Guid repoId, int page) + { + context.Logger.LogInformation("Page {Page} of the pull requests for repo {RepoId} is past the last page the list allows ({MaxPage}); returning an empty page", page, repoId, ListPullRequestsQuery.MaxPage); + + return NodeResult.Ok(OutputsOf([])); + } + + private static Dictionary OutputsOf(IReadOnlyList pullRequests) => new() + { + ["pullRequests"] = JsonSerializer.SerializeToElement(pullRequests, WorkflowJson.Options), + ["count"] = JsonSerializer.SerializeToElement(pullRequests.Count) + }; + private static bool TryReadRepositoryId(NodeRunContext context, out Guid repoId) { repoId = Guid.Empty; @@ -154,12 +170,17 @@ private static bool TryReadState(NodeRunContext context, out PullRequestState? s return true; } - /// Read an optional positive-int input, falling back to when absent or non-positive. + /// + /// Read an optional positive-int input, falling back to when absent, not a number, or below one. + /// A number is read by its value, not by how it is written: one an Int32 cannot hold (3000000000) is the very large number + /// it is — — and not a missing one, which would turn "a page far past the last" into page 1; + /// one written with a fraction or an exponent (1001.0, 1.001e3, 1001.5) is read at its whole part. + /// private static int ReadPositiveInt(NodeRunContext context, string key, int fallback) { - if (context.Inputs.TryGetValue(key, out var value) && value.ValueKind == JsonValueKind.Number && value.TryGetInt32(out var n) && n > 0) - return n; + if (!context.Inputs.TryGetValue(key, out var value) || value.ValueKind != JsonValueKind.Number) return fallback; + if (value.TryGetInt32(out var whole)) return whole > 0 ? whole : fallback; - return fallback; + return value.TryGetDouble(out var number) && number >= 1 ? (int)Math.Min(number, int.MaxValue) : fallback; } } diff --git a/backend/src/CodeSpace.Messages/Dtos/Invitations/InvitationDtos.cs b/backend/src/CodeSpace.Messages/Dtos/Invitations/InvitationDtos.cs index 4ddc1079b..dec86779d 100644 --- a/backend/src/CodeSpace.Messages/Dtos/Invitations/InvitationDtos.cs +++ b/backend/src/CodeSpace.Messages/Dtos/Invitations/InvitationDtos.cs @@ -31,6 +31,17 @@ public sealed record InvitationPreview /// could invite an arbitrary address and read the answer. /// public required bool ForSignedInAccount { get; init; } + + /// + /// under the name the invite page that shipped before it reads. The API is replaced + /// before the SPA, so that page meets this response for the length of a rollout, and without the name it reads + /// undefined and asks a signed-in invitee to "Create your account" for an address that already has one. + /// + /// Derived, so it can never say more than does: the same fact about the caller, + /// not the old answer to "does this address have an account". Remove it once no deployed SPA predates the rename. + /// + [Obsolete("Read ForSignedInAccount. Kept for one release for the invite page that predates it.")] + public bool AccountExists => ForSignedInAccount; } /// diff --git a/backend/src/CodeSpace.Messages/Queries/Repositories/ListIssuesQuery.cs b/backend/src/CodeSpace.Messages/Queries/Repositories/ListIssuesQuery.cs index a622a8341..3b5a815c1 100644 --- a/backend/src/CodeSpace.Messages/Queries/Repositories/ListIssuesQuery.cs +++ b/backend/src/CodeSpace.Messages/Queries/Repositories/ListIssuesQuery.cs @@ -21,7 +21,8 @@ public sealed record ListIssuesQuery : IQuery>, IRequ /// /// The furthest page a caller may name (with , the newest 100,000 issues), as for - /// . A page past it reads this one: no list is paged this deep. + /// . A page past it is an empty page, and the provider is not asked: no + /// list is paged this deep. /// public const int MaxPage = 1000; diff --git a/backend/src/CodeSpace.Messages/Queries/Repositories/ListPullRequestsQuery.cs b/backend/src/CodeSpace.Messages/Queries/Repositories/ListPullRequestsQuery.cs index 872e6b924..ee9da298f 100644 --- a/backend/src/CodeSpace.Messages/Queries/Repositories/ListPullRequestsQuery.cs +++ b/backend/src/CodeSpace.Messages/Queries/Repositories/ListPullRequestsQuery.cs @@ -21,7 +21,9 @@ public sealed record ListPullRequestsQuery : IQuery /// The furthest page a caller may name (with , the newest 100,000 requests). A page past it - /// reads this one: the number cannot run past what a page index and offset hold, and no list is paged this deep. + /// is an empty page, and the provider is not asked: the number cannot run past what a page index and offset hold, + /// and no list is paged this deep. (Reading this page again for it would answer a caller who pages on with rows it + /// already has.) /// public const int MaxPage = 1000; diff --git a/backend/tests/CodeSpace.E2ETests/Auth/InvitationEndpointE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Auth/InvitationEndpointE2ETests.cs index 1d9a005b3..8244dfcaa 100644 --- a/backend/tests/CodeSpace.E2ETests/Auth/InvitationEndpointE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Auth/InvitationEndpointE2ETests.cs @@ -42,7 +42,7 @@ public async Task Accepting_over_http_creates_the_account_and_returns_a_session( { var (token, email) = await SeedInvitationAsync(); - var response = await _factory.CreateClient().PostAsJsonAsync($"/api/invitations/{token}/accept", new { name = "Wire Newcomer", password = "correct-horse-battery" }); + var response = await AcceptAsync(token, "Wire Newcomer", asBrowser: true); response.StatusCode.ShouldBe(HttpStatusCode.OK, await DescribeAsync(response, "200 with a session")); @@ -60,6 +60,20 @@ public async Task Accepting_over_http_creates_the_account_and_returns_a_session( body.GetProperty("user").GetProperty("teams").GetArrayLength().ShouldBeGreaterThanOrEqualTo(2, "the invited team plus the personal workspace acceptance creates"); } + [Fact] + public async Task Accepting_without_the_csrf_header_returns_the_session_in_the_body_and_sets_no_cookie() + { + // The invite page that shipped before the cookie, running against an API that is already newer, sends no CSRF header + // and signs out by emptying its own storage. A cookie it cannot clear would outlive the sign-out for a day. + var (token, _) = await SeedInvitationAsync(); + + var response = await AcceptAsync(token, "Older Page Newcomer", asBrowser: false); + + response.StatusCode.ShouldBe(HttpStatusCode.OK, await DescribeAsync(response, "200 with a session")); + (response.Headers.TryGetValues("Set-Cookie", out var setCookies) ? setCookies : []).ShouldNotContain(c => c.StartsWith($"{SessionCookie.Name}=", StringComparison.Ordinal)); + JsonDocument.Parse(await response.Content.ReadAsStringAsync()).RootElement.GetProperty("token").GetString().ShouldNotBeNullOrWhiteSpace("the page reads its session from the body, as it always did"); + } + [Fact] public async Task The_body_does_not_have_to_repeat_the_token_that_is_already_in_the_route() { @@ -148,6 +162,16 @@ public async Task A_malformed_accept_answers_the_same_whether_or_not_the_address // ─── Helpers ──────────────────────────────────────────────────────────────── + /// Accepts as the SPA does (with the CSRF header it sends on every request) or as a client that predates the cookie does (without). + private async Task AcceptAsync(string token, string name, bool asBrowser) + { + var request = new HttpRequestMessage(HttpMethod.Post, $"/api/invitations/{token}/accept") { Content = JsonContent.Create(new { name, password = "correct-horse-battery" }) }; + + if (asBrowser) request.Headers.Add(SessionCookie.CsrfHeader, "1"); + + return await _factory.CreateClient().SendAsync(request); + } + /// /// Seeds a pending invitation straight to the row, hashing the token the same way the service does. /// Going through the create endpoint would need a signed-in member with members.manage, which is a diff --git a/backend/tests/CodeSpace.E2ETests/Auth/InvitationReachE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Auth/InvitationReachE2ETests.cs index 916cbeb33..3d271c615 100644 --- a/backend/tests/CodeSpace.E2ETests/Auth/InvitationReachE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Auth/InvitationReachE2ETests.cs @@ -124,7 +124,10 @@ public async Task The_anonymous_preview_reads_the_same_whether_or_not_the_addres var existingPreview = await PreviewAsync(forExisting.Token, userId: null); var nobodyPreview = await PreviewAsync(forNobody.Token, userId: null); - existingPreview.TryGetProperty("accountExists", out _).ShouldBeFalse("an anonymous preview must not say whether the address has an account — it is an existence oracle for any address"); + // accountExists is only the old spelling of forSignedInAccount, kept for the invite page that shipped before it. Were it + // ever the address's own fact again, this is the assertion that goes red: an existence oracle for any address. + existingPreview.GetProperty("accountExists").GetBoolean().ShouldBeFalse("an anonymous preview must not say whether the address has an account — it is an existence oracle for any address"); + nobodyPreview.GetProperty("accountExists").GetBoolean().ShouldBeFalse(); PropertyNames(existingPreview).ShouldBe(PropertyNames(nobodyPreview), "the two previews must have the same shape"); existingPreview.GetProperty("forSignedInAccount").GetBoolean().ShouldBeFalse(); nobodyPreview.GetProperty("forSignedInAccount").GetBoolean().ShouldBeFalse(); @@ -143,6 +146,22 @@ public async Task The_invited_account_signed_in_is_told_the_invitation_is_its_ow (await PreviewAsync(invitation.Token, bystander)).GetProperty("forSignedInAccount").GetBoolean().ShouldBeFalse("a different signed-in account learns nothing about the invited address"); } + [Fact] + public async Task The_old_spelling_the_previous_invite_page_reads_says_the_same_thing_to_the_same_caller() + { + // The API is replaced before the SPA, so the page that shipped before forSignedInAccount reads accountExists from this + // response: without it, a signed-in invitee is asked to "Create your account" for an address that already has one. + var team = await SeedTeamAsync(); + var inviteeEmail = $"invitee-{Guid.NewGuid():N}@tenant.test"; + var invitee = await _tenants.AccountAsync(email: inviteeEmail); + var bystander = await _tenants.AccountAsync(); + var invitation = await InviteAsync(team.Owner, team.TeamId, TeamRole.Member, inviteeEmail); + + (await PreviewAsync(invitation.Token, invitee)).GetProperty("accountExists").GetBoolean().ShouldBeTrue("the signed-in invitee is who the older page was written for"); + (await PreviewAsync(invitation.Token, bystander)).GetProperty("accountExists").GetBoolean().ShouldBeFalse("a different signed-in account learns nothing about the invited address"); + (await PreviewAsync(invitation.Token, userId: null)).GetProperty("accountExists").GetBoolean().ShouldBeFalse("nor does a stranger"); + } + // ─── Plumbing ──────────────────────────────────────────────────────────────── private sealed record SeededTeam(Guid TeamId, Guid Owner, Guid Admin); diff --git a/backend/tests/CodeSpace.E2ETests/Auth/OpenAWorkspaceAndFillItE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Auth/OpenAWorkspaceAndFillItE2ETests.cs index 29a2a676d..2a4565b8a 100644 --- a/backend/tests/CodeSpace.E2ETests/Auth/OpenAWorkspaceAndFillItE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Auth/OpenAWorkspaceAndFillItE2ETests.cs @@ -76,7 +76,7 @@ public async Task Someone_who_may_open_a_workspace_can_staff_it() var offer = JsonDocument.Parse(await preview.Content.ReadAsStringAsync()).RootElement; offer.GetProperty("teamName").GetString().ShouldBe("Design Guild", "a person deciding whether to accept is owed the name of what they are joining"); - offer.TryGetProperty("accountExists", out _).ShouldBeFalse("an anonymous preview never says whether the address has an account"); + offer.GetProperty("accountExists").GetBoolean().ShouldBeFalse("an anonymous preview never says whether the address has an account (the old spelling of forSignedInAccount, kept for the previous invite page)"); offer.GetProperty("forSignedInAccount").GetBoolean().ShouldBeFalse("nobody is signed in"); // ── And accepts ─────────────────────────────────────────────────────────── diff --git a/backend/tests/CodeSpace.E2ETests/Auth/SessionCookieE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Auth/SessionCookieE2ETests.cs index 6fd1c19be..8c3793706 100644 --- a/backend/tests/CodeSpace.E2ETests/Auth/SessionCookieE2ETests.cs +++ b/backend/tests/CodeSpace.E2ETests/Auth/SessionCookieE2ETests.cs @@ -44,7 +44,10 @@ public async Task Signing_in_hands_the_browser_an_http_only_strict_cookie_that_i { var account = await AccountAsync(); - var signIn = await _tenants.Client(new Uri($"{scheme}://localhost")).PostAsJsonAsync("/api/auth/sign-in", new { name = account.Email, password = Password }); + var request = new HttpRequestMessage(HttpMethod.Post, "/api/auth/sign-in") { Content = JsonContent.Create(new { name = account.Email, password = Password }) }; + request.Headers.Add(SessionCookie.CsrfHeader, "1"); // the SPA sends it on every request, sign-in included + + var signIn = await _tenants.Client(new Uri($"{scheme}://localhost")).SendAsync(request); signIn.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(signIn)); @@ -96,7 +99,7 @@ public async Task A_bearer_client_writes_without_the_header_and_a_stray_cookie_n var browserUser = await AccountAsync(); var apiClient = await AccountAsync(); var strayCookie = await SignInAsync(browserUser.Email); - var bearer = await TokenInBodyAsync(await SignInResponseAsync(apiClient.Email)); + var bearer = await TokenInBodyAsync(await SignInResponseAsync(apiClient.Email, csrf: false)); var me = await SendAsync(HttpMethod.Get, "/api/users/me", cookie: strayCookie, bearer: bearer); var write = await SendAsync(HttpMethod.Post, "/api/auth/change-password", cookie: strayCookie, bearer: bearer, body: new { currentPassword = Password, newPassword = "a-brand-new-passphrase" }); @@ -156,7 +159,8 @@ public async Task A_session_minted_for_a_request_the_cookie_carried_reaches_the_ var session = await SignInAsync(account.Email); var (path, body) = await SessionMintingRequestAsync(endpoint, account.Email); - var response = ridesCookie ? await SendAsync(HttpMethod.Post, path, cookie: session, csrf: true, body: body) : await SendAsync(HttpMethod.Post, path, bearer: session, body: body); + // Off the cookie it is a browser still on its token (a header-carrying client that asks for the cookie by sending the CSRF header). + var response = ridesCookie ? await SendAsync(HttpMethod.Post, path, cookie: session, csrf: true, body: body) : await SendAsync(HttpMethod.Post, path, bearer: session, csrf: true, body: body); response.StatusCode.ShouldBe(HttpStatusCode.OK, $"{endpoint} — {await TenantHarness.DescribeAsync(response)}"); @@ -169,6 +173,46 @@ public async Task A_session_minted_for_a_request_the_cookie_carried_reaches_the_ else JsonDocument.Parse(raw).RootElement.GetProperty("token").GetString().ShouldBe(minted, "a bearer client reads the session it was minted from the body, exactly as before"); } + /// + /// A client that predates the cookie signs in without the CSRF header, and has no way to sign out of a cookie: the + /// older SPA's sign-out only empties its own storage, an API client has no cookie jar, and only the API can clear an + /// HttpOnly cookie. Handed one anyway, it holds a 24-hour session it cannot end — through a rolling deploy, API + /// first, that is every browser with the older bundle open. It gets the session in the body, as it always did. + /// + [Theory] + [InlineData("sign-in")] + [InlineData("change-password")] + [InlineData("accept")] + public async Task A_client_that_does_not_send_the_csrf_header_gets_its_session_in_the_body_and_never_a_cookie(string endpoint) + { + var account = await AccountAsync(); + var bearer = await TokenInBodyAsync(await SignInResponseAsync(account.Email, csrf: false)); + var (path, body) = await SessionMintingRequestAsync(endpoint, account.Email); + + var response = await SendAsync(HttpMethod.Post, path, bearer: endpoint == "sign-in" ? null : bearer, body: body); + + response.StatusCode.ShouldBe(HttpStatusCode.OK, $"{endpoint} — {await TenantHarness.DescribeAsync(response)}"); + SessionCookieOf(response).ShouldBeNull($"{endpoint} without {SessionCookie.CsrfHeader} must not set a cookie — Set-Cookie: {string.Join(" | ", SetCookies(response))}"); + + var minted = await TokenInBodyAsync(response); + + (await SendAsync(HttpMethod.Get, "/api/users/me", bearer: minted)).StatusCode.ShouldBe(HttpStatusCode.OK, "fixture check: the body's session is a working one, so nothing is lost by withholding the cookie"); + } + + [Fact] + public async Task A_failed_sign_in_sets_no_cookie_whoever_asked() + { + var account = await AccountAsync(); + + var wrong = new HttpRequestMessage(HttpMethod.Post, "/api/auth/sign-in") { Content = JsonContent.Create(new { name = account.Email, password = "not-the-password-at-all" }) }; + wrong.Headers.Add(SessionCookie.CsrfHeader, "1"); + + var response = await _tenants.Client().SendAsync(wrong); + + response.StatusCode.ShouldBe(HttpStatusCode.Unauthorized, await TenantHarness.DescribeAsync(response)); + SessionCookieOf(response).ShouldBeNull(); + } + [Fact] public async Task Changing_the_password_through_the_cookie_replaces_it_and_kills_the_one_before() { @@ -205,7 +249,7 @@ public async Task A_pre_cookie_token_is_exchanged_for_a_cookie_carrying_that_sam // A browser signed in before the cookie existed holds its token in localStorage. It trades it, once, for the // cookie. The SAME token: minting a fresh one here would turn the exchange into a way to extend a stolen token. var account = await AccountAsync(); - var bearer = await TokenInBodyAsync(await SignInResponseAsync(account.Email)); + var bearer = await TokenInBodyAsync(await SignInResponseAsync(account.Email, csrf: false)); var exchange = await SendAsync(HttpMethod.Post, "/api/auth/session", bearer: bearer); @@ -271,9 +315,10 @@ public async Task The_dev_spa_origin_may_send_the_header_and_receive_the_cookie_ return (await _tenants.AccountAsync(email: email, password: Password), email); } - private async Task SignInResponseAsync(string email) + /// Signs in as the SPA does (with the CSRF header it sends on every request) unless is false: an API client, the CLI, or the SPA bundle that predates the cookie. + private async Task SignInResponseAsync(string email, bool csrf = true) { - var response = await _tenants.Client().PostAsJsonAsync("/api/auth/sign-in", new { name = email, password = Password }); + var response = await SendAsync(HttpMethod.Post, "/api/auth/sign-in", csrf: csrf, body: new { name = email, password = Password }); response.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(response)); diff --git a/backend/tests/CodeSpace.E2ETests/Repositories/ListPagingPastTheCeilingE2ETests.cs b/backend/tests/CodeSpace.E2ETests/Repositories/ListPagingPastTheCeilingE2ETests.cs new file mode 100644 index 000000000..18f38b343 --- /dev/null +++ b/backend/tests/CodeSpace.E2ETests/Repositories/ListPagingPastTheCeilingE2ETests.cs @@ -0,0 +1,117 @@ +using System.Net; +using System.Net.Http.Json; +using System.Text.Json; +using CodeSpace.Core.Persistence.Entities; +using CodeSpace.E2ETests.Infrastructure; +using CodeSpace.IntegrationTests.Workflows.Infrastructure; +using CodeSpace.Messages.Constants; +using CodeSpace.Messages.Enums; +using CodeSpace.Messages.Queries.Repositories; +using Shouldly; + +namespace CodeSpace.E2ETests.Repositories; + +/// +/// The Pulls and Issues tabs' lists end where the API says they do. A page past the last one a list allows used to read +/// the last page again, so a client that kept turning pages kept receiving the same rows under a page number that said +/// it was further on. It is now a 200 with an empty page, and the provider is not asked. +/// +/// Driven through the real HTTP surface (query binding, the access behavior, the handler, the service, the real +/// GitLab provider over NGitLab), against a loopback GitLab that holds one row per list. +/// +/// Fidelity: HIGH. Only GitLab is a stub, and it is a real socket the production client speaks HTTP to. +/// +[Trait("Category", "E2E")] +[Trait("Surface", "Http")] +public sealed class ListPagingPastTheCeilingE2ETests : IClassFixture +{ + private readonly TenantHarness _tenants; + + public ListPagingPastTheCeilingE2ETests(TaskLaunchApiFactory factory) { _tenants = new TenantHarness(factory); } + + [Theory] + [InlineData("pull-requests", ListPullRequestsQuery.MaxPage)] + [InlineData("issues", ListIssuesQuery.MaxPage)] + public async Task A_page_past_the_last_one_the_list_allows_is_an_empty_200_and_GitLab_is_not_asked(string list, int lastPage) + { + await using var gitLab = LoopbackResponder.Start(OneRowPerList); + var (viewer, team, repositoryId) = await SeedRepositoryAsync(gitLab.Port); + + var last = await ListAsync(viewer, team, repositoryId, list, lastPage); + + last.GetArrayLength().ShouldBe(1, "fixture check: the last page the list allows still answers, so an empty page past it is the ceiling and not an empty GitLab"); + var asked = gitLab.Requests.Count; + + asked.ShouldBeGreaterThan(0, "fixture check: serving the last page reached GitLab"); + + foreach (var page in new[] { lastPage + 1, 1_000_000, int.MaxValue }) + (await ListAsync(viewer, team, repositoryId, list, page)).GetArrayLength().ShouldBe(0, $"/{list}?page={page}"); + + gitLab.Requests.Count.ShouldBe(asked, "a page past the last one is answered without a request to GitLab"); + } + + [Theory] + [InlineData("pull-requests")] + [InlineData("issues")] + public async Task A_page_too_large_for_an_int_is_refused_and_not_answered_with_another_pages_rows(string list) + { + // Past what a page index holds there is no page to name: the query's Page is an int, so binding refuses it before any + // handler could read it as a missing page, and GitLab is not asked either. + await using var gitLab = LoopbackResponder.Start(OneRowPerList); + var (viewer, team, repositoryId) = await SeedRepositoryAsync(gitLab.Port); + + var response = await _tenants.SendAsync(HttpMethod.Get, $"/api/repositories/{repositoryId}/{list}?page=3000000000&perPage=100", viewer, team); + + response.StatusCode.ShouldBe(HttpStatusCode.BadRequest, await TenantHarness.DescribeAsync(response)); + gitLab.Requests.Count.ShouldBe(0, "a refused request reaches no provider"); + } + + private async Task ListAsync(Guid viewer, Guid team, Guid repositoryId, string list, int page) + { + var response = await _tenants.SendAsync(HttpMethod.Get, $"/api/repositories/{repositoryId}/{list}?page={page}&perPage=100", viewer, team); + + response.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(response)); + + return await response.Content.ReadFromJsonAsync(); + } + + /// One merge request and one issue, whatever page is asked for — a stub that cannot tell the ceiling's page from any other, so only the API's own ceiling can make a page empty. + private static string OneRowPerList(string head) + { + const string author = """{ "id": 1, "username": "dev", "name": "Dev" }"""; + var requestLine = head.Split("\r\n")[0]; + + if (requestLine.Contains("/merge_requests")) return LoopbackResponder.Ok($$"""[{ "id": 7, "iid": 7, "project_id": 42, "title": "Seven", "state": "opened", "source_branch": "f7", "target_branch": "main", "author": {{author}}, "labels": [], "created_at": "2026-09-24T08:00:00.000Z", "updated_at": "2026-09-24T08:00:00.000Z", "web_url": "https://gitlab.test/acme/web/-/merge_requests/7" }]"""); + if (requestLine.Contains("/issues")) return LoopbackResponder.Ok($$"""[{ "id": 7, "iid": 7, "project_id": 42, "title": "Issue 7", "state": "opened", "author": {{author}}, "labels": [], "created_at": "2026-09-24T08:00:00.000Z", "updated_at": "2026-09-24T08:00:00.000Z", "web_url": "https://gitlab.test/acme/web/-/issues/7" }]"""); + if (requestLine.Contains("/labels")) return LoopbackResponder.Ok("[]"); + + return "HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"; + } + + /// An Owner connects GitLab and adds a PAT through the API, as an operator would; the repository row is seeded directly so no bind round-trip to the stub is needed. + private async Task<(Guid Viewer, Guid Team, Guid RepositoryId)> SeedRepositoryAsync(int port) + { + var owner = await _tenants.AccountAsync(); + var viewer = await _tenants.AccountAsync(); + var team = await _tenants.TeamAsync((owner, TeamRole.Owner), (viewer, TeamRole.Viewer)); + + var instanceId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/provider-instances", owner, team, new { provider = "GitLab", displayName = "paging gitlab", baseUrl = $"http://127.0.0.1:{port}" })); + var credentialId = await CreatedIdAsync(await _tenants.SendAsync(HttpMethod.Post, "/api/credentials/pat", owner, team, new { providerInstanceId = instanceId, displayName = "pat", token = $"glpat-FAKE-{Guid.NewGuid():N}" })); + var repositoryId = Guid.NewGuid(); + + await _tenants.DbAsync(async db => + { + db.Repository.Add(new Repository { Id = repositoryId, TeamId = team, ProviderInstanceId = instanceId, CredentialId = credentialId, ExternalId = "42", NamespacePath = "acme", Name = "web", FullPath = "acme/web", DefaultBranch = "main", Visibility = RepositoryVisibility.Private, WebUrl = "http://127.0.0.1/acme/web", Status = RepositoryStatus.Active, CreatedBy = SystemUsers.SeederId, LastModifiedBy = SystemUsers.SeederId }); + return await db.SaveChangesAsync(); + }); + + return (viewer, team, repositoryId); + } + + private static async Task CreatedIdAsync(HttpResponseMessage response) + { + response.StatusCode.ShouldBe(HttpStatusCode.OK, await TenantHarness.DescribeAsync(response)); + + return (await response.Content.ReadFromJsonAsync()).GetProperty("id").GetGuid(); + } +} diff --git a/backend/tests/CodeSpace.IntegrationTests/Agents/AgentToolReadBoundsFlowTests.cs b/backend/tests/CodeSpace.IntegrationTests/Agents/AgentToolReadBoundsFlowTests.cs index 9e8c48b33..7e0f50cb0 100644 --- a/backend/tests/CodeSpace.IntegrationTests/Agents/AgentToolReadBoundsFlowTests.cs +++ b/backend/tests/CodeSpace.IntegrationTests/Agents/AgentToolReadBoundsFlowTests.cs @@ -26,9 +26,10 @@ namespace CodeSpace.IntegrationTests.Agents; /// GitListPullRequestsNode → PullRequestService → the real GitLab provider and NGitLab) and the real mediator /// pipeline behind the Pulls tab, on Postgres, against a loopback GitLab that holds as many merge requests as anyone asks for. /// -/// Covers: a page and page size an agent names are held to the list's ceilings and read as one request for that one -/// page (the deep-page walk spent a request per page before it); the Pulls tab's and the Issues tab's queries are held the -/// same way; one agent run spends only its share of the connection's requests a minute, so the Pulls tab is still served +/// Covers: a page size an agent names is held to the list's ceiling and the page it names is read as one request for +/// that one page (the deep-page walk spent a request per page before it), while a page past the last one the list allows is +/// an empty page that asks the provider nothing (it used to read the last page again); the Pulls tab's and the Issues tab's +/// queries are held the same way; one agent run spends only its share of the connection's requests a minute, so the Pulls tab is still served /// while it lists; a list too large for one tool result reaches the model cut, as an error that says so; and a command /// whose output is too large is answered as done, cut, and never invited to run again. /// @@ -43,13 +44,13 @@ public sealed class AgentToolReadBoundsFlowTests(PostgresFixture fixture) private const string IssuesPath = "/api/v4/projects/4242/issues"; [Fact] - public async Task An_agents_list_asks_GitLab_for_one_bounded_page_whatever_it_names() + public async Task An_agents_list_asks_GitLab_for_one_bounded_page_whatever_size_it_names() { using var gitlab = LoopbackGitLab(titleLength: 8); var world = await SeedWorldAsync(gitlab.BaseUrl); using var scope = fixture.BeginScope(); - var result = await CallToolAsync(Handler(scope, world), "git.list_prs", new { repositoryId = world.RepositoryId.ToString(), page = 1_000_000, perPage = int.MaxValue }); + var result = await CallToolAsync(Handler(scope, world), "git.list_prs", new { repositoryId = world.RepositoryId.ToString(), page = ListPullRequestsQuery.MaxPage, perPage = int.MaxValue }); result.GetProperty("isError").GetBoolean().ShouldBeFalse(Text(result)); var page = ForgeQueryOf(gitlab.Requests.Where(r => r.PathAndQuery.Contains(MergeRequestsPath)).ShouldHaveSingleItem("one request for the one page, never a walk through the pages before it")); @@ -59,6 +60,33 @@ public async Task An_agents_list_asks_GitLab_for_one_bounded_page_whatever_it_na gitlab.Requests.Count.ShouldBe(2, string.Join("\n", gitlab.Requests.Select(r => r.PathAndQuery))); } + [Fact] + public async Task An_agent_paging_past_the_last_page_is_given_an_empty_one_and_GitLab_is_not_asked() + { + using var gitlab = LoopbackGitLab(titleLength: 8); + var world = await SeedWorldAsync(gitlab.BaseUrl); + + using var scope = fixture.BeginScope(); + var handler = Handler(scope, world); + var last = await CallToolAsync(handler, "git.list_prs", new { repositoryId = world.RepositoryId.ToString(), page = ListPullRequestsQuery.MaxPage, perPage = 100 }); + var asked = gitlab.Requests.Count; + + last.GetProperty("structuredContent").GetProperty("count").GetInt32().ShouldBe(ListPullRequestsQuery.MaxPerPage, "fixture check: the last page the list allows holds a full page, so an empty answer past it is the guard and not an empty loopback"); + + // Past what an Int32 holds too: an agent names the number, and a JSON number is not limited to an int. It used to be + // read as no page at all, so GitLab was asked for page 1 and the first page's rows came back as "a page far on". + foreach (var page in new long[] { ListPullRequestsQuery.MaxPage + 1, 1_000_000, int.MaxValue, 2_147_483_648, 3_000_000_000 }) + { + var past = await CallToolAsync(handler, "git.list_prs", new { repositoryId = world.RepositoryId.ToString(), page, perPage = 100 }); + + past.GetProperty("isError").GetBoolean().ShouldBeFalse(Text(past)); + past.GetProperty("structuredContent").GetProperty("count").GetInt32().ShouldBe(0, $"page {page}"); + past.GetProperty("structuredContent").GetProperty("pullRequests").GetArrayLength().ShouldBe(0, $"page {page}"); + } + + gitlab.Requests.Count.ShouldBe(asked, "a page past the last one is answered without a request to GitLab"); + } + [Fact] public async Task The_pulls_tab_asks_GitLab_for_one_bounded_page_too() { @@ -66,7 +94,7 @@ public async Task The_pulls_tab_asks_GitLab_for_one_bounded_page_too() var world = await SeedWorldAsync(gitlab.BaseUrl); using var scope = fixture.BeginScopeAs(world.OwnerId, world.TeamId, Roles.Admin); - var listed = await scope.Resolve().Send(new ListPullRequestsQuery { RepositoryId = world.RepositoryId, Page = 1_000_000, PerPage = int.MaxValue }); + var listed = await scope.Resolve().Send(new ListPullRequestsQuery { RepositoryId = world.RepositoryId, Page = ListPullRequestsQuery.MaxPage, PerPage = int.MaxValue }); listed.Count.ShouldBe(ListPullRequestsQuery.MaxPerPage); var page = ForgeQueryOf(gitlab.Requests.Where(r => r.PathAndQuery.Contains(MergeRequestsPath)).ShouldHaveSingleItem()); @@ -91,6 +119,24 @@ public async Task A_list_too_large_for_one_tool_result_reaches_the_model_cut_and text.ShouldContain("\"number\":1,", customMessage: "the start of the list is kept"); } + [Fact] + public async Task The_pulls_tab_paging_past_the_last_page_is_given_an_empty_one_and_GitLab_is_not_asked() + { + using var gitlab = LoopbackGitLab(titleLength: 8); + var world = await SeedWorldAsync(gitlab.BaseUrl); + + using var scope = fixture.BeginScopeAs(world.OwnerId, world.TeamId, Roles.Admin); + var mediator = scope.Resolve(); + + (await mediator.Send(new ListPullRequestsQuery { RepositoryId = world.RepositoryId, Page = ListPullRequestsQuery.MaxPage, PerPage = 100 })).Count.ShouldBe(ListPullRequestsQuery.MaxPerPage, "fixture check: the last page the list allows holds a full page"); + var asked = gitlab.Requests.Count; + + foreach (var page in new[] { ListPullRequestsQuery.MaxPage + 1, 1_000_000, int.MaxValue }) + (await mediator.Send(new ListPullRequestsQuery { RepositoryId = world.RepositoryId, Page = page, PerPage = 100 })).ShouldBeEmpty($"page {page}"); + + gitlab.Requests.Count.ShouldBe(asked, "a page past the last one is answered without a request to GitLab"); + } + [Fact] public async Task The_issues_tab_asks_GitLab_for_one_bounded_page_too() { @@ -98,7 +144,7 @@ public async Task The_issues_tab_asks_GitLab_for_one_bounded_page_too() var world = await SeedWorldAsync(gitlab.BaseUrl); using var scope = fixture.BeginScopeAs(world.OwnerId, world.TeamId, Roles.Admin); - var listed = await scope.Resolve().Send(new ListIssuesQuery { RepositoryId = world.RepositoryId, Page = 1_000_000, PerPage = int.MaxValue }); + var listed = await scope.Resolve().Send(new ListIssuesQuery { RepositoryId = world.RepositoryId, Page = ListIssuesQuery.MaxPage, PerPage = int.MaxValue }); listed.Count.ShouldBe(ListIssuesQuery.MaxPerPage); var page = ForgeQueryOf(gitlab.Requests.Where(r => r.PathAndQuery.Contains(IssuesPath)).ShouldHaveSingleItem("one request for the one page, never a walk through the pages before it")); @@ -106,6 +152,24 @@ public async Task The_issues_tab_asks_GitLab_for_one_bounded_page_too() page["per_page"].ShouldBe(ListIssuesQuery.MaxPerPage.ToString()); } + [Fact] + public async Task The_issues_tab_paging_past_the_last_page_is_given_an_empty_one_and_GitLab_is_not_asked() + { + using var gitlab = LoopbackGitLab(titleLength: 8); + var world = await SeedWorldAsync(gitlab.BaseUrl); + + using var scope = fixture.BeginScopeAs(world.OwnerId, world.TeamId, Roles.Admin); + var mediator = scope.Resolve(); + + (await mediator.Send(new ListIssuesQuery { RepositoryId = world.RepositoryId, Page = ListIssuesQuery.MaxPage, PerPage = 100 })).Count.ShouldBe(ListIssuesQuery.MaxPerPage, "fixture check: the last page the list allows holds a full page"); + var asked = gitlab.Requests.Count; + + foreach (var page in new[] { ListIssuesQuery.MaxPage + 1, 1_000_000, int.MaxValue }) + (await mediator.Send(new ListIssuesQuery { RepositoryId = world.RepositoryId, Page = page, PerPage = 100 })).ShouldBeEmpty($"page {page}"); + + gitlab.Requests.Count.ShouldBe(asked, "a page past the last one is answered without a request to GitLab"); + } + [Fact] public async Task One_agent_run_spends_only_its_share_of_the_connection_and_the_pulls_tab_is_still_served() { diff --git a/backend/tests/CodeSpace.UnitTests/Auth/SessionCookieTests.cs b/backend/tests/CodeSpace.UnitTests/Auth/SessionCookieTests.cs index 7321bb1b4..074203469 100644 --- a/backend/tests/CodeSpace.UnitTests/Auth/SessionCookieTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Auth/SessionCookieTests.cs @@ -137,6 +137,41 @@ public void A_write_with_the_header_is_guarded(string method) SessionCookie.IsUnguardedWrite(RequestFor(method, csrfHeader: "1")).ShouldBeFalse(); } + /// + /// A client that predates the cookie — an older SPA bundle, an API client, the CLI — has no way to end one: only the + /// API can clear an HttpOnly cookie, and the older SPA's sign-out only empties its own storage. So a sign-in that hands + /// it a cookie hands it a 24-hour session it can never sign out of. The SPA that knows the cookie sends the header on + /// every request, which is the one thing that tells the two apart. + /// + [Theory] + [InlineData("1", true)] + [InlineData("anything", true)] // presence is the signal, as it is for the write guard + [InlineData(null, false)] + [InlineData("", false)] + [InlineData(" ", false)] // a blank header is no header, here as in IsUnguardedWrite + public void A_session_is_handed_over_as_a_cookie_only_to_a_request_that_carries_the_csrf_header(string? csrfHeader, bool expected) + { + var context = ContextAt("https"); + + if (csrfHeader != null) context.Request.Headers[SessionCookie.CsrfHeader] = csrfHeader; + + SessionCookie.IssueIfRequested(context, "the-jwt", Now.AddHours(24)); + + SetCookieOf(context).StartsWith($"{SessionCookie.Name}=the-jwt;", StringComparison.Ordinal).ShouldBe(expected, $"header {csrfHeader ?? "(none)"} — Set-Cookie: {SetCookieOf(context)}"); + context.Response.Headers.SetCookie.Count.ShouldBe(expected ? 1 : 0); + } + + [Fact] + public void Asking_for_the_cookie_outright_never_needs_the_header() + { + // POST /api/auth/session is the one endpoint whose whole job is to set the cookie: whoever calls it asked. + var context = ContextAt("https"); + + SessionCookie.Issue(context, "the-jwt", Now.AddHours(24)); + + SetCookieOf(context).ShouldStartWith($"{SessionCookie.Name}=the-jwt;"); + } + [Theory] [InlineData(true, true, null, true)] // the browser on its cookie: the session must not come back in the body [InlineData(true, true, "Bearer header-jwt", false)] // a header client with a stray cookie: the header carried it diff --git a/backend/tests/CodeSpace.UnitTests/Contracts/InvitationPreviewWireTests.cs b/backend/tests/CodeSpace.UnitTests/Contracts/InvitationPreviewWireTests.cs new file mode 100644 index 000000000..1a7b4b3e3 --- /dev/null +++ b/backend/tests/CodeSpace.UnitTests/Contracts/InvitationPreviewWireTests.cs @@ -0,0 +1,45 @@ +using System.Reflection; +using System.Text.Json; +using System.Text.Json.Serialization; +using CodeSpace.Messages.Dtos.Invitations; +using CodeSpace.Messages.Enums; +using Shouldly; + +namespace CodeSpace.UnitTests.Contracts; + +/// +/// The invitation preview as the SPA reads it. The page that shipped before ForSignedInAccount reads +/// accountExists, and the API is always replaced before the SPA, so for the length of a rollout (and for any tab +/// opened before it) that page meets this response. Without the old name it reads undefined and asks a signed-in +/// invitee to "Create your account" for an address that already has one. +/// +[Trait("Category", "Unit")] +public sealed class InvitationPreviewWireTests +{ + private static readonly JsonSerializerOptions Api = new(JsonSerializerDefaults.Web) { Converters = { new JsonStringEnumConverter() } }; + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void The_old_name_is_on_the_wire_and_says_exactly_what_the_new_one_says(bool forSignedInAccount) + { + var wire = JsonSerializer.SerializeToElement(Preview(forSignedInAccount), Api); + + wire.GetProperty("forSignedInAccount").GetBoolean().ShouldBe(forSignedInAccount, "fixture check: the new name carries the fact"); + wire.GetProperty("accountExists").GetBoolean().ShouldBe(forSignedInAccount, "the old name is the new fact, never the answer to 'does this address have an account', which was an existence oracle"); + } + + [Fact] + public void The_old_name_is_marked_obsolete_so_the_next_release_removes_it_on_purpose() + { + var alias = typeof(InvitationPreview).GetProperty("AccountExists", BindingFlags.Public | BindingFlags.Instance).ShouldNotBeNull("the alias the previous SPA bundle reads"); + + alias.GetCustomAttribute().ShouldNotBeNull("an alias nobody is told is temporary is a second field forever"); + alias.CanWrite.ShouldBeFalse("it is derived from ForSignedInAccount, so the two can never disagree"); + } + + private static InvitationPreview Preview(bool forSignedInAccount) => new() + { + TeamName = "Platform", InvitedByName = "Mars", Role = TeamRole.Member, Email = "maya@team.dev", ExpiresAt = DateTimeOffset.UtcNow.AddDays(7), ForSignedInAccount = forSignedInAccount, + }; +} diff --git a/backend/tests/CodeSpace.UnitTests/Handlers/Repositories/ListIssuesQueryHandlerTests.cs b/backend/tests/CodeSpace.UnitTests/Handlers/Repositories/ListIssuesQueryHandlerTests.cs index ad47eedba..c6b6abe48 100644 --- a/backend/tests/CodeSpace.UnitTests/Handlers/Repositories/ListIssuesQueryHandlerTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Handlers/Repositories/ListIssuesQueryHandlerTests.cs @@ -26,8 +26,8 @@ public void The_page_ceilings_are_pinned() [InlineData(3, 30, 3, 30)] // in-range values pass through [InlineData(1, 0, 1, 1)] // perPage < 1 clamps up to 1 [InlineData(1, 1000, 1, 100)] // perPage > Max clamps down to MaxPerPage - [InlineData(1_000_000, 100, 1000, 100)] // a page past the ceiling is the ceiling - [InlineData(int.MaxValue, int.MaxValue, 1000, 100)] + [InlineData(1000, 100, 1000, 100)] // the last page the list allows is still a page + [InlineData(1000, int.MaxValue, 1000, 100)] public async Task Clamps_page_and_perPage_then_forwards_with_the_current_team(int page, int perPage, int expectedPage, int expectedPerPage) { var team = Guid.NewGuid(); @@ -45,6 +45,23 @@ public async Task Clamps_page_and_perPage_then_forwards_with_the_current_team(in service.PerPage.ShouldBe(expectedPerPage); } + [Theory] + [InlineData(1001)] + [InlineData(1_000_000)] + [InlineData(int.MaxValue)] + public async Task A_page_past_the_last_one_the_list_allows_is_empty_and_asks_no_provider(int page) + { + // It used to read page 1000 again, so a client paging on kept receiving the same rows under a new page number, + // for as long as it cared to ask. + var service = new StubIssueService(); + var handler = new ListIssuesQueryHandler(service, new StubCurrentTeam(Guid.NewGuid())); + + var listed = await handler.Handle(new ListIssuesQuery { RepositoryId = Guid.NewGuid(), Page = page, PerPage = 100 }, CancellationToken.None); + + listed.ShouldBeEmpty(); + service.Calls.ShouldBe(0, "there is nothing at that page to fetch, so nothing is fetched"); + } + /// Hand-rolled stub (no mocking lib) — records the list call; writes/counts throw. private sealed class StubIssueService : IIssueService { diff --git a/backend/tests/CodeSpace.UnitTests/Handlers/Repositories/ListPullRequestsQueryHandlerTests.cs b/backend/tests/CodeSpace.UnitTests/Handlers/Repositories/ListPullRequestsQueryHandlerTests.cs index 5be6be419..b9b66ecae 100644 --- a/backend/tests/CodeSpace.UnitTests/Handlers/Repositories/ListPullRequestsQueryHandlerTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Handlers/Repositories/ListPullRequestsQueryHandlerTests.cs @@ -25,8 +25,8 @@ public void The_page_ceilings_are_pinned() [InlineData(3, 30, 3, 30)] // in-range values pass through [InlineData(1, 0, 1, 1)] // perPage < 1 clamps up to 1 [InlineData(1, int.MaxValue, 1, 100)] // perPage past the providers' cap is their cap - [InlineData(1_000_000, 100, 1000, 100)] // a page past the ceiling is the ceiling - [InlineData(int.MaxValue, int.MaxValue, 1000, 100)] + [InlineData(1000, 100, 1000, 100)] // the last page the list allows is still a page + [InlineData(1000, int.MaxValue, 1000, 100)] public async Task Clamps_page_and_perPage_then_forwards_with_the_current_team(int page, int perPage, int expectedPage, int expectedPerPage) { var team = Guid.NewGuid(); @@ -43,6 +43,22 @@ public async Task Clamps_page_and_perPage_then_forwards_with_the_current_team(in service.PerPage.ShouldBe(expectedPerPage); } + [Theory] + [InlineData(1001)] + [InlineData(1_000_000)] + [InlineData(int.MaxValue)] + public async Task A_page_past_the_last_one_the_list_allows_is_empty_and_asks_no_provider(int page) + { + // It used to read page 1000 again, so a client paging on kept receiving the same rows under a new page number, + // for as long as it cared to ask. + var service = new StubPullRequestService(); + + var listed = await new ListPullRequestsQueryHandler(service, new StubCurrentTeam(Guid.NewGuid())).Handle(new ListPullRequestsQuery { RepositoryId = Guid.NewGuid(), Page = page, PerPage = 100 }, CancellationToken.None); + + listed.ShouldBeEmpty(); + service.Calls.ShouldBe(0, "there is nothing at that page to fetch, so nothing is fetched"); + } + /// Records the list call; every other member throws (the handler only lists). private sealed class StubPullRequestService : IPullRequestService { diff --git a/backend/tests/CodeSpace.UnitTests/Workflows/GitListPullRequestsNodeTests.cs b/backend/tests/CodeSpace.UnitTests/Workflows/GitListPullRequestsNodeTests.cs index df00918a2..1fcb2c9e7 100644 --- a/backend/tests/CodeSpace.UnitTests/Workflows/GitListPullRequestsNodeTests.cs +++ b/backend/tests/CodeSpace.UnitTests/Workflows/GitListPullRequestsNodeTests.cs @@ -187,9 +187,9 @@ public async Task Falls_back_to_defaults_for_non_positive_pagination() } [Theory] - [InlineData(1_000_000, 4, ListPullRequestsQuery.MaxPage, 4)] + [InlineData(ListPullRequestsQuery.MaxPage, 4, ListPullRequestsQuery.MaxPage, 4)] // the last page the list allows is still a page [InlineData(2, int.MaxValue, 2, ListPullRequestsQuery.MaxPerPage)] - [InlineData(int.MaxValue, 101, ListPullRequestsQuery.MaxPage, ListPullRequestsQuery.MaxPerPage)] + [InlineData(ListPullRequestsQuery.MaxPage, 101, ListPullRequestsQuery.MaxPage, ListPullRequestsQuery.MaxPerPage)] public async Task Clamps_pagination_to_the_ceilings_the_list_query_declares(int page, int perPage, int expectedPage, int expectedPerPage) { // An agent names page and perPage itself. Unclamped, one call asked a provider for its whole pull request list in a @@ -207,6 +207,59 @@ public async Task Clamps_pagination_to_the_ceilings_the_list_query_declares(int stub.PerPage.ShouldBe(expectedPerPage); } + [Theory] + [InlineData("1001")] + [InlineData("1000000")] + [InlineData("2147483647")] + [InlineData("2147483648")] // the first number an Int32 cannot hold: it used to be read as no page at all, and so as page 1 + [InlineData("3000000000")] + [InlineData("1e12")] + [InlineData("1e999")] // past what a double holds as well + [InlineData("1001.0")] // a spelling is not a different number + [InlineData("1.001e3")] + [InlineData("1001.5")] + public async Task A_page_past_the_last_one_the_list_allows_is_an_empty_page_and_asks_no_provider(string page) + { + // It used to read page 1000 again — the same rows an agent had just seen, under a number that said it was further on, + // so a loop paging until an empty page never reached one. A number too large for an Int32 fell through to page 1. + var stub = new StubPrService(); + + var result = await new GitListPullRequestsNode(stub).RunAsync(ContextFrom(new() + { + ["repositoryId"] = JsonSerializer.SerializeToElement(Repo), + ["page"] = Raw(page), + ["perPage"] = JsonSerializer.SerializeToElement(100), + }), CancellationToken.None); + + result.Status.ShouldBe(NodeStatus.Success); + result.Outputs["pullRequests"].GetArrayLength().ShouldBe(0); + result.Outputs["count"].GetInt32().ShouldBe(0); + stub.Calls.ShouldBe(0, "there is nothing at that page to fetch, so nothing is fetched"); + } + + [Theory] + [InlineData("2.0", "30.0", 2, 30)] // written with a fraction, the whole number it says + [InlineData("1e1", "5E1", 10, 50)] // or with an exponent + [InlineData("2.5", "50.9", 2, 50)] // a number with a fraction is read at its whole part + [InlineData("3", "3000000000", 3, ListPullRequestsQuery.MaxPerPage)] // a size too large for an Int32 is the ceiling, as 101 is — not the default + [InlineData("1", "1e12", 1, ListPullRequestsQuery.MaxPerPage)] + [InlineData("0.5", "0.5", 1, 30)] // below one it is not a page or a size, as 0 is not + [InlineData("-3000000000", "-1e12", 1, 30)] // and a number far below one is no more one than -5 is + public async Task Reads_a_number_by_its_value_and_not_by_how_it_is_written(string page, string perPage, int expectedPage, int expectedPerPage) + { + var stub = new StubPrService(); + + await new GitListPullRequestsNode(stub).RunAsync(ContextFrom(new() + { + ["repositoryId"] = JsonSerializer.SerializeToElement(Repo), + ["page"] = Raw(page), + ["perPage"] = Raw(perPage), + }), CancellationToken.None); + + stub.Page.ShouldBe(expectedPage); + stub.PerPage.ShouldBe(expectedPerPage); + } + [Fact] public void The_input_schema_advertises_the_ceilings_the_node_enforces() { @@ -237,6 +290,9 @@ public async Task Fails_when_repository_id_is_not_a_uuid() result.Error.ShouldContain("repositoryId"); } + /// A JSON value exactly as a caller wrote it — 1001.0 and 1001 are different tokens to a reader. + private static JsonElement Raw(string json) => JsonDocument.Parse(json).RootElement.Clone(); + private static NodeRunContext Context(string repositoryId, string? state = null) { var inputs = new Dictionary { ["repositoryId"] = JsonSerializer.SerializeToElement(repositoryId) }; diff --git a/frontend/docker/csp-api-origin.mjs b/frontend/docker/csp-api-origin.mjs index 3b1d48f71..93d9396ea 100644 --- a/frontend/docker/csp-api-origin.mjs +++ b/frontend/docker/csp-api-origin.mjs @@ -1,6 +1,6 @@ // Names the API origin for the SPA's Content-Security-Policy, at container start (docker/init.sh). // -// node csp-api-origin.mjs dist/env.js > /etc/nginx/codespace/api-origin.conf +// node csp-api-origin.mjs dist/env.js > /tmp/codespace/api-origin.conf // // The origin is read from the SPA's own env script AFTER @import-meta-env/cli has substituted it, so the CSP allows // exactly the backend the app was configured to call — the same .env / process env, resolved by the same tool — and no diff --git a/frontend/docker/e2e/run.sh b/frontend/docker/e2e/run.sh index 5694a79ef..a42284520 100755 --- a/frontend/docker/e2e/run.sh +++ b/frontend/docker/e2e/run.sh @@ -1,10 +1,11 @@ #!/usr/bin/env bash # SPA image E2E: build the REAL frontend image and start it the way the cluster does (the ConfigMap's .env mounted at # /usr/share/nginx/html/.env, the image's own CMD /init.sh), then prove over HTTP what static-hosting.test.ts can only -# pin as source text: init.sh derives the API origin from the substituted env, nginx accepts the config and reads that -# origin, and every response carries the CSP, nosniff and Referrer-Policy, with /env.js handing the SPA its env. It also -# reads the container log back: a token in an invitation, reset or callback path, or in a Referer naming one, is logged as -# {token}, and every other request verbatim. +# pin as source text: init.sh derives the API origin from the substituted env (into /tmp/codespace, nothing under /etc/nginx), +# nginx accepts the config and reads that origin, and every response carries the CSP, nosniff and Referrer-Policy, with +# /env.js handing the SPA its env. It also reads the container log back: a token in an invitation, reset or callback +# path, or in a Referer naming one, is logged as {token}, and every other request verbatim. The container is started with a +# writable root filesystem: the image does not yet start with a read-only one (see nginx.conf, at the include). # # Fidelity: high. The real Dockerfile, init.sh, @import-meta-env/cli (fetched by npx at start, as in production), # csp-api-origin.mjs and nginx. The expected CSP is nginx.conf's own header with the derived source filled in, so the @@ -86,6 +87,11 @@ expect_served() { # $1 = case, $2 = VITE_API_URL, $3 = the CSP source init.sh mu docker exec "$name" nginx -t >/dev/null 2>&1 || fail "$1: nginx -t rejects the config init.sh completed (run 'docker exec $name nginx -t')" "$name" + # The API-origin include nginx.conf reads is the one write init.sh used to make under /etc/nginx; it is under /tmp now, and + # nothing is left behind at the old path. This checks where that file is, not that the image starts read-only. + docker exec "$name" test -s /tmp/codespace/api-origin.conf || fail "$1: init.sh wrote no API-origin include at /tmp/codespace/api-origin.conf, where nginx.conf reads it" "$name" + ! docker exec "$name" test -e /etc/nginx/codespace || fail "$1: init.sh still writes under /etc/nginx/codespace, the path it moved off" "$name" + expected="${CSP_TEMPLATE//\$codespace_api_origin/$3}" for path in / /teams/demo/runs /env.js; do diff --git a/frontend/docker/init.sh b/frontend/docker/init.sh index ff960afd3..e63921bd6 100755 --- a/frontend/docker/init.sh +++ b/frontend/docker/init.sh @@ -8,8 +8,10 @@ cd /usr/share/nginx/html npx -y @import-meta-env/cli@0.7.4 -x .env -e .env -p dist/env.js # Name the API origin that env points the SPA at, for the CSP in nginx.conf (which includes this file). A malformed -# VITE_API_URL stops the container here instead of serving a policy that blocks the app. -mkdir -p /etc/nginx/codespace -node /usr/local/lib/codespace/csp-api-origin.mjs dist/env.js > /etc/nginx/codespace/api-origin.conf +# VITE_API_URL stops the container here instead of serving a policy that blocks the app. Written under /tmp, nginx.conf's +# include path, so this step needs nothing under /etc to be writable. It is not what makes a read-only root filesystem +# work: the substitution above still writes dist/env.js in place (and npx its cache), and nginx its pid file and cache. +mkdir -p /tmp/codespace +node /usr/local/lib/codespace/csp-api-origin.mjs dist/env.js > /tmp/codespace/api-origin.conf nginx -g "daemon off;" diff --git a/frontend/nginx.conf b/frontend/nginx.conf index 6ff1bb0b4..09dfa3021 100644 --- a/frontend/nginx.conf +++ b/frontend/nginx.conf @@ -25,8 +25,11 @@ server { access_log /var/log/nginx/access.log codespace; # Sets $codespace_api_origin — the API origin the SPA was configured with ("" when same-origin). Written at - # container start by docker/init.sh from the substituted env.js; nginx refuses to start without it. - include /etc/nginx/codespace/api-origin.conf; + # container start by docker/init.sh from the substituted env.js; nginx refuses to start without it. Under /tmp, not + # /etc, so generating it needs nothing under /etc to be writable. That is the only write moved: the image does not yet + # start with a read-only root filesystem (init.sh still substitutes dist/env.js in place and runs npx, which keeps a + # cache under $HOME, and nginx writes its pid file and /var/cache/nginx). + include /tmp/codespace/api-origin.conf; location / { root /usr/share/nginx/html/dist; @@ -43,12 +46,16 @@ server { # declares none of its own. # script-src 'self' no inline script, no eval — the runtime env is /env.js for exactly this reason. # style-src 'unsafe-inline' because Base UI (Select, ScrollArea) renders