Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions src/auth/guards/repo-scope.guard.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,10 @@ describe('RepoScopeGuard', () => {
);
});

it('claims an unassigned tracked repository for the first maintainer to sync it', async () => {
// Ownership is no longer a sync side-effect (#565) — an unclaimed repo is
// syncable but NOT auto-assigned to whoever happens to sync it first;
// maintainerId stays null until it's set explicitly.
it('allows an unassigned tracked repository without silently claiming it', async () => {
userRepo.findOne.mockResolvedValue({
id: 'user-1',
roles: [UserRole.MAINTAINER],
Expand All @@ -113,10 +116,7 @@ describe('RepoScopeGuard', () => {
});

await expect(guard.canActivate(maintainerContext())).resolves.toBe(true);
expect(repositoryRepo.update).toHaveBeenCalledWith(
{ id: 'repo-uuid-1' },
{ maintainerId: 'user-1' },
);
expect(repositoryRepo.update).not.toHaveBeenCalled();
});

it('refuses a first-time sync of an untracked repository', async () => {
Expand Down Expand Up @@ -168,9 +168,9 @@ describe('RepoScopeGuard', () => {
});

it('refuses an unauthenticated request', async () => {
await expect(guard.canActivate(context({ owner: 'acme', repo: 'repo' }))).rejects.toThrow(
/Authentication is required/,
);
await expect(
guard.canActivate(context({ owner: 'acme', repo: 'repo' })),
).rejects.toThrow(/Authentication is required/);
});

it('refuses a request whose authenticated user no longer exists', async () => {
Expand All @@ -182,8 +182,8 @@ describe('RepoScopeGuard', () => {
});

it('refuses a request with no repository parameters', async () => {
await expect(guard.canActivate(context({ user: { userId: 'user-1' } }))).rejects.toThrow(
NotFoundException,
);
await expect(
guard.canActivate(context({ user: { userId: 'user-1' } })),
).rejects.toThrow(NotFoundException);
});
});
24 changes: 16 additions & 8 deletions src/auth/guards/repo-scope.guard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { Reflector } from '@nestjs/core';
import { InjectRepository } from '@nestjs/typeorm';
import { ConfigService } from '@nestjs/config';
import { Request } from 'express';
import { Repository } from 'typeorm';
import { ILike, Repository } from 'typeorm';
import { Repository as RepositoryEntity } from '../../common/entities';
import { User } from '../../common/entities';
import { UserRole } from '../../common/enums';
Expand Down Expand Up @@ -75,13 +75,17 @@ export class RepoScopeGuard implements CanActivate {
where: { id: request.user.userId },
select: { id: true, roles: true },
});
if (!user) throw new ForbiddenException('Authenticated user no longer exists');
if (!user)
throw new ForbiddenException('Authenticated user no longer exists');
if (!user.roles.includes(UserRole.MAINTAINER)) {
throw new ForbiddenException('Maintainer role required');
}

// GitHub owner/repo names are case-insensitive — the isAllowlisted check
// lowercases both sides but this lookup didn't, so `Owner/Repo` and
// `owner/repo` resolved differently (#564). Match case-insensitively.
const tracked = await this.repositoryRepo.findOne({
where: { fullName },
where: { fullName: ILike(fullName) },
select: { id: true, maintainerId: true },
});

Expand All @@ -102,16 +106,20 @@ export class RepoScopeGuard implements CanActivate {
);
}

// Tracked with no maintainer yet: allow, and claim it for this user so the
// scope is explicit from here on.
await this.repositoryRepo.update({ id: tracked.id }, { maintainerId: user.id });
// Tracked with no maintainer yet: the old code silently assigned it to
// the first requester ("lets the first claimant take it", #565) — any
// maintainer could claim ownership of any unclaimed repo just by syncing
// it. Don't self-assign; a repo's owner must be set explicitly. Allow the
// sync but leave maintainerId null — ownership assignment is a separate,
// deliberate action.
return true;
}

/** Exact `owner/repo` entries from `GITHUB_SYNC_ALLOWED_REPOS`. */
private isAllowlisted(fullName: string): boolean {
const raw = this.configService.get('github', { infer: true })
.syncAllowedRepos;
const raw = this.configService.get('github', {
infer: true,
}).syncAllowedRepos;
if (!raw) return false;
return raw
.split(',')
Expand Down