Skip to content

Commit

Permalink
Merge pull request from GHSA-v627-69v2-xx37
Browse files Browse the repository at this point in the history
Fixes #GHSA-v627-69v2-xx37

Change GetRepositoryByRepoName query to include project ID. This enforces that
the user can only access the repos in the project they authorized with.
This fixes a security issue where several API endpoints allowed any
authenticated user to manipulate any repo in the DB, irrespective of who
owned it.
  • Loading branch information
dmjb authored Mar 4, 2024
1 parent 5fd0f74 commit 45750b4
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 11 deletions.
2 changes: 1 addition & 1 deletion database/query/repositories.sql
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ SELECT * FROM repositories WHERE id = $1;
SELECT * FROM repositories WHERE repo_id = $1;

-- name: GetRepositoryByRepoName :one
SELECT * FROM repositories WHERE provider = $1 AND repo_owner = $2 AND repo_name = $3;
SELECT * FROM repositories WHERE provider = $1 AND repo_owner = $2 AND repo_name = $3 AND project_id = $4;

-- name: GetRepositoryByIDAndProject :one
SELECT * FROM repositories WHERE provider = $1 AND repo_id = $2 AND project_id = $3;
Expand Down
4 changes: 4 additions & 0 deletions internal/controlplane/handlers_artifacts.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,14 @@ func (s *Server) GetArtifactByName(ctx context.Context, in *pb.GetArtifactByName
return nil, util.UserVisibleError(codes.InvalidArgument, "invalid artifact name user repoOwner/repoName/artifactName")
}

entityCtx := engine.EntityFromContext(ctx)
projectID := entityCtx.Project.ID

repo, err := s.store.GetRepositoryByRepoName(ctx, db.GetRepositoryByRepoNameParams{
Provider: in.GetContext().GetProvider(),
RepoOwner: nameParts[0],
RepoName: nameParts[1],
ProjectID: projectID,
})
if err != nil {
if errors.Is(err, sql.ErrNoRows) {
Expand Down
16 changes: 12 additions & 4 deletions internal/controlplane/handlers_repositories.go
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,12 @@ func (s *Server) GetRepositoryByName(ctx context.Context,
return nil, providerError(err)
}

repo, err := s.store.GetRepositoryByRepoName(ctx,
db.GetRepositoryByRepoNameParams{Provider: provider.Name, RepoOwner: fragments[0], RepoName: fragments[1]})
repo, err := s.store.GetRepositoryByRepoName(ctx, db.GetRepositoryByRepoNameParams{
Provider: provider.Name,
RepoOwner: fragments[0],
RepoName: fragments[1],
ProjectID: projectID,
})

if errors.Is(err, sql.ErrNoRows) {
return nil, status.Errorf(codes.NotFound, "repository not found")
Expand Down Expand Up @@ -352,8 +356,12 @@ func (s *Server) DeleteRepositoryByName(ctx context.Context,
return nil, providerError(err)
}

repo, err := s.store.GetRepositoryByRepoName(ctx,
db.GetRepositoryByRepoNameParams{Provider: provider.Name, RepoOwner: fragments[0], RepoName: fragments[1]})
repo, err := s.store.GetRepositoryByRepoName(ctx, db.GetRepositoryByRepoNameParams{
Provider: provider.Name,
RepoOwner: fragments[0],
RepoName: fragments[1],
ProjectID: projectID,
})

if errors.Is(err, sql.ErrNoRows) {
return nil, status.Errorf(codes.NotFound, "repository not found")
Expand Down
16 changes: 11 additions & 5 deletions internal/db/repositories.sql.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion internal/db/repositories_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,11 @@ func TestGetRepositoryByRepoName(t *testing.T) {
repo1 := createRandomRepository(t, project.ID, prov.Name)

repo2, err := testQueries.GetRepositoryByRepoName(context.Background(), GetRepositoryByRepoNameParams{
Provider: repo1.Provider, RepoOwner: repo1.RepoOwner, RepoName: repo1.RepoName})
Provider: repo1.Provider,
RepoOwner: repo1.RepoOwner,
RepoName: repo1.RepoName,
ProjectID: project.ID,
})
require.NoError(t, err)
require.NotEmpty(t, repo2)

Expand Down

0 comments on commit 45750b4

Please sign in to comment.