Instrument repo sync and re-add ignore a group-scoped grant, letting it re-sync or replace the token of any repository

POST /v1/instrument-repos/:id/sync and POST /v1/instrument-repos ignore the group scope of the grant that admits the caller. A per-user permission on InstrumentRepo can be confined to one group: GROUP_SCOPED_CONDITIONS maps it to { groupIds: { hasSome: [groupId] } }, and isGrantablePermission allows it. The @RouteAccess guard only checks the subject type, so a scoped grant passes it. After that, deleteById and findAll apply accessibleQuery, but sync reads the repository by bare id and the controller never forwards the ability. A user granted update InstrumentRepo for group A can therefore re-sync any repository on the instance. A sync can create new instrument editions, and InstrumentsService.create adds those to every group that had the previous edition. A user granted create InstrumentRepo for group A can post the URL of any already-registered repository. create then overwrites that repository's stored access token with whatever the caller supplied and syncs it. Replacing a working token with an invalid one breaks every later sync of a private repository for all groups that use it.

Where

apps/api/src/instrument-repos/instrument-repos.controller.ts:35-40: no ability is forwarded:

@Post(':id/sync')
@RouteAccess({ action: 'update', subject: 'InstrumentRepo' })
sync(@Param('id') id: string) {
  return this.instrumentReposService.sync(id);
}

apps/api/src/instrument-repos/instrument-repos.service.ts:117-121:

async sync(id: string) {
  const repo = await this.instrumentRepoModel.findFirst({ where: { id } });

apps/api/src/instrument-repos/instrument-repos.service.ts:43-51: an existing URL overwrites the token and syncs, whoever the caller is:

const existing = await this.instrumentRepoModel.findFirst({ where: { url: normalizedUrl } });
if (existing) {
  if (accessToken) {
    await this.instrumentRepoModel.update({
      data: { accessToken: this.encrypt(accessToken) },
      where: { id: existing.id }
    });
  }
  return this.sync(existing.id);
}

Reproduce

As an admin, register a private repository R and assign it only to group B. Create a STANDARD user in group A and grant them update InstrumentRepo and create InstrumentRepo, both scoped to group A.
As that user, POST /v1/instrument-repos/<R id>/sync.
As that user, POST /v1/instrument-repos with { "url": "<R url>", "accessToken": "invalid" }.

Actual: step 2 syncs R. Step 3 replaces R's stored token, and every later sync of R fails with 502 for everyone.
Expected: step 2 returns 404 because R is outside the grant's scope, which is how deleteById already treats a repository outside a scoped delete grant. Step 3 is refused with 409 (the URL is @unique), and R's token is left untouched.

Tests

apps/api/src/instrument-repos/__tests__/instrument-repos.service.test.ts: it('should scope sync by the caller's update rules, so a group-scoped grant cannot reach another group's repository'), asserting accessibleQuery(ability, 'update', 'InstrumentRepo') in the findFirst where. Add a matching case for create re-syncing an existing URL.
testing/src/specs/authorization.spec.ts: a user holding a group-scoped update InstrumentRepo grant gets 404 when syncing a repository outside that group.

Suggested fix

Forward @CurrentUser('ability') to sync and create, and put accessibleQuery(ability, 'update', 'InstrumentRepo') in both findFirst calls. In create, refuse an existing URL the caller cannot update with a ConflictException before touching its token.

Still reproduces on main (d86740e). Nothing under apps/api/src/instrument-repos/, ability.factory.ts or ability.utils.ts has changed since this was filed, and the cited lines are the same.

`create` needs an object-level check, not a scoped lookup

The suggested fix puts accessibleQuery(ability, 'update', 'InstrumentRepo') in both findFirst calls. That is right for sync, because the guard guarantees the caller holds an update rule. In create it fails in two ways:

If the caller holds create InstrumentRepo and no update rule, accessibleQuery throws CASL's ForbiddenError, which comes back as a 500 (.agents/docs/architecture/auth-and-permissions.md, Layer 2).
If the caller's update scope excludes the repository, the scoped lookup returns null. create then downloads and imports the repository, which creates instruments, and only fails afterwards on the @unique url.

Keep the lookup by url unscoped. Before the token write, throw the ConflictException unless ability.can('update', existing). Prisma results already carry __modelName, so this check works on the row as returned. Then call this.sync(existing.id, { ability }).

A new URL ignores the scope too

create sets no groupIds on a new repository, and nothing checks the condition on the create rule. So a create InstrumentRepo grant confined to group A can register any repository. This hole does more damage than the other two. The import runs every instrument through InstrumentsService.create (instruments.service.ts:121-135). The edition push described above for sync therefore happens here too, with content the grantee chooses. Anyone who controls a GitHub repository can publish a new edition of any instrument in use into every group.

POST /v1/groups had the same shape and was made ADMIN_ONLY (#1468). A narrower fix keeps instance-wide grants working. When the URL is not yet registered, refuse with 403 before the download unless ability.can('create', forcedAppSubject('InstrumentRepo', { groupIds: [] })). A scoped rule's hasSome is false on an empty list, and an unconditioned rule passes. assignments.service.ts:86 uses the same pattern for an assignment with no group. After this change, a group-scoped create InstrumentRepo reaches nothing. auth-and-permissions.md says such a pair belongs in isGrantablePermission (packages/schemas/src/core/core.ts:73-74), which will need to take groupId to express it.

Open question: an instance-wide create InstrumentRepo grant is also an instrument upload by another route, while POST /v1/instruments requires manage Instrument. Should this route require both, using the array form of @RouteAccess?

The scope can be widened from the group side

A group-scoped grant matches on InstrumentRepo.groupIds. That field has one writer: the nested instrumentRepos: { set } in GroupsService.updateById (groups.service.ts:134-138). It accepts any ids. Every GROUP_MANAGER holds manage Group on their own groups (ability.factory.ts:59). A manager of A who holds a scoped update InstrumentRepo for A can PATCH /v1/groups/<A> with instrumentRepoIds: [<R id>], and R is then in scope for both routes. Repository ids are not secret, because instrument info returns them as sourceRepo.id (instruments.service.ts:576-578). Only /admin/groups sends instrumentRepoIds (apps/web/src/routes/_app/admin/groups/index.tsx:33-40), and /group/manage sends only accessibleInstrumentIds and settings. Refusing that field unless ability.can('manage', 'all') would therefore close this with no client change. It deserves its own issue, landed together with this one.

Tests

instrument-repos.service.test.ts: cover
- sync scoping (as proposed);
- a re-add refused with 409 that never calls update;
- a group-scoped create for a new URL refused before importInstruments;
- create with no ability still working, since setup.service.ts:157 calls it that way.
A re-add case that passes an ability needs a mock with __modelName: 'InstrumentRepo' and groupIds. The existing { id: 'existing' } mocks resolve to subject type Object. In instrument-repos.controller.test.ts, the create and sync cases should expect { ability }.
The proposed e2e cannot fail. The e2e stack never holds a repository (admin-instrument-repos.spec.ts:3-6), so syncing any id is a 404 with or without the fix. Pin the sync 404, and the re-add 409 with the stored token unchanged, in a new apps/api/test/suites/04-instrument-repo-scoping.suite.ts. That suite can insert the InstrumentRepo row through Prisma, as 03-record-file-deletion.suite.ts does for its rows. Once fixed, both refusals happen before any download.
e2e: in testing/src/specs/authorization.spec.ts, under server-side authorization, a user granted create InstrumentRepo scoped to their own group gets 403 from POST /v1/instrument-repos. feat: add the instrument hub and record provenance #1807 also edits this file, so expect a rebase.

#1633 has the same shape on PATCH /v1/instrument-records/:id (an unscoped findFirst ahead of the write) and can land alongside.

_{Triage note written with Claude Code. Code references point at main @ d86740e.}

fix(api): refuse re-adding an instrument repo the caller cannot update

Navigation Menu

Repository navigation

Instrument repo sync and re-add ignore a group-scoped grant, letting it re-sync or replace the token of any repository #1728

Where

Reproduce

Tests

Suggested fix

`create` needs an object-level check, not a scoped lookup

A new URL ignores the scope too

The scope can be widened from the group side

Tests

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Instrument repo sync and re-add ignore a group-scoped grant, letting it re-sync or replace the token of any repository #1728

Description

Where

Reproduce

Tests

Suggested fix

Activity

joshunrau commented on Oct 26, 2026

create needs an object-level check, not a scoped lookup

A new URL ignores the scope too

The scope can be widened from the group side

Tests

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

`create` needs an object-level check, not a scoped lookup