fix: scope unscoped proposal and test-run lists to the caller's projects (#482) - #489
Merged
Merged
Conversation
…projects `GET /api/proposals` and `GET /api/test-runs` were the two list endpoints still reading `GetAccessibleProjectIdsAsync` directly and *demanding* a filter: a non-admin who sent neither `projectId` nor `agentId` got a successful response with an empty list, rather than the rows of the projects they can actually see. The callers that hit this are the ones with no reason to send a filter — a REST API key, confined to one project, and integrations driving `/api/*`. Both now resolve a scope through `ResolveListScopeAsync` like every other list endpoint, and apply it in the query via new set-aware repository methods so the page is computed over the union rather than merged after the fact. `ProposalsController` gains the `ListScopeAsync` shape `TestRunGroupsController` already uses, which also tightens a case it got wrong before: a named `agentId` outside the requested project no longer bypasses that project filter. This makes the claim already in docs/architecture.md — that no controller re-implements the check — true. Refs #482 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /api/proposalsandGET /api/test-runswere the last two list endpoints readingGetAccessibleProjectIdsAsyncdirectly and demanding a filter — a non-admin sending neitherprojectIdnoragentIdgot a successful200with an empty list instead of the rows of the projects they can see. The eight controllers named in the issue were already fixed by #484 (which closed no issue, so #482 stayed open); these two were not. Both now resolve a scope throughResolveListScopeAsynclike every other list endpoint and apply it inside the query.Closes #482
Changes
ProposalsController— replaces theCanListAsync"return nothing" helper with theListScopeAsyncshapeTestRunGroupsControlleralready uses. Side effect worth noting: a namedagentIdoutside the requestedprojectIdno longer bypasses that project filter.TestRunsController— an unfiltered list now resolvesResolveListScopeAsync(requestedProjectId: null)instead of short-circuiting every non-admin to an empty page.IOptimizationProposalRepository.GetByProjectsAsyncandITestRunRepository.GetByProjectsPagedAsync— set-aware lookups so the scope is anINpredicate in SQL and the page is computed over the union, not merged from per-project pages.GetByProjectAsyncnow delegates to the set overload.includeSystembehaviour for runs).docs/architecture.md— documents the related-entity (agentId/suiteId) narrowing shape and the no-projectId-parameter case; the "no controller re-implements the check" claim is now actually true.manual/admin/providers-and-api-keys.md+CHANGELOG.md— extend the existing Unscoped list endpoints return an empty page instead of the caller's accessible rows for non-admins #482 entry rather than duplicating it.Verification
dotnet build Proxytrace.sln— 24 projects, 0 errors, 0 warningsdotnet test Proxytrace.sln) — 2,797 passed, 0 failed, 6 skipped. Ran the full suite because the change alters two shared repository interfaces. The four new controller tests were confirmed to fail with the controller fix reverted.docs/architecture.md,manual/admin/providers-and-api-keys.md(npm run docs:buildgreen),CHANGELOG.md[Unreleased]. No UI strings, so no i18n extract.Not done
No perf metric was added. The high-volume path for multi-project scope is already covered by
agentCallsListByProjects;TestRunEntitypaging has never had perf coverage (GetAllPagedAsync/GetByAgentPagedAsyncincluded), and the new query is the same join shape as the existingTestRunGroupRepository.GetByProjectsPagedAsyncwithINinstead of=. Adding one would need the perf seeder to build run chains at scale — worth doing, but a separate change.🤖 Generated with Claude Code