Skip to content

refactor/nullable repo key #3826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fiftin wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

refactor/nullable repo key #3826

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0e5316
refactor(repos): allow nil for key id
fiftin May 3, 2026
2a0dc9d
feat(migration): nullable key column in repo
fiftin May 3, 2026
44c37bf
test(unit): fix tests after refactoring
fiftin May 3, 2026
616d2a6
Merge branch 'develop' into refactor/nullable_repo_key
fiftin May 4, 2026
7ee53d2
fix(repos): nil key correct handling
fiftin May 4, 2026
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
2 changes: 1 addition & 1 deletion api/projects/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ func (c *KeyController) UpdateKey(w http.ResponseWriter, r *http.Request) {
}

for _, repo := range repos {
if repo.SSHKeyID != key.ID {
if repo.SSHKeyID == nil || *repo.SSHKeyID != key.ID {
continue
}
err = repo.ClearCache()
Expand Down
36 changes: 18 additions & 18 deletions api/projects/projects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func (c *ProjectsController) createDemoProject(projectID int, noneKeyID int, emp
ProjectID: projectID,
GitURL: "https://github.com/semaphoreui/semaphore-demo.git",
GitBranch: "main",
SSHKeyID: noneKeyID,
SSHKeyID: nil,
})

if err != nil {
Expand Down Expand Up @@ -149,15 +149,15 @@ func (c *ProjectsController) createDemoProject(projectID int, noneKeyID int, emp

desc = "Pings the website to provide a real-world example of using Semaphore."
_, err = store.CreateTemplate(db.Template{
Name: "Ping semaphoreui.com",
Playbook: "ping.yml",
Description: &desc,
ProjectID: projectID,
InventoryID: &prodInv.ID,
Name: "Ping semaphoreui.com",
Playbook: "ping.yml",
Description: &desc,
ProjectID: projectID,
InventoryID: &prodInv.ID,
EnvironmentIDs: []int{emptyEnvID},
RepositoryID: demoRepo.ID,
App: db.AppAnsible,
ViewID: &toolsView.ID,
RepositoryID: demoRepo.ID,
App: db.AppAnsible,
ViewID: &toolsView.ID,
})

if err != nil {
Expand All @@ -168,16 +168,16 @@ func (c *ProjectsController) createDemoProject(projectID int, noneKeyID int, emp

var startVersion = "1.0.0"
buildTpl, err := store.CreateTemplate(db.Template{
Name: "Build demo app",
Playbook: "build.yml",
Type: db.TemplateBuild,
ProjectID: projectID,
InventoryID: &buildInv.ID,
Name: "Build demo app",
Playbook: "build.yml",
Type: db.TemplateBuild,
ProjectID: projectID,
InventoryID: &buildInv.ID,
EnvironmentIDs: []int{emptyEnvID},
RepositoryID: demoRepo.ID,
StartVersion: &startVersion,
App: db.AppAnsible,
ViewID: &buildView.ID,
RepositoryID: demoRepo.ID,
StartVersion: &startVersion,
App: db.AppAnsible,
ViewID: &buildView.ID,
})

if err != nil {
Expand Down
10 changes: 6 additions & 4 deletions api/runners/runners.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,8 @@ func (c *RunnerController) GetRunner(w http.ResponseWriter, r *http.Request) {
}
}

if tsk.Inventory.RepositoryID != nil {
err := c.encryptionService.DeserializeSecret(&tsk.Inventory.Repository.SSHKey)
if tsk.Inventory.RepositoryID != nil && tsk.Inventory.Repository.SSHKeyID != nil {
err := c.encryptionService.DeserializeSecret(tsk.Inventory.Repository.SSHKey)
if err != nil {
log.WithFields(log.Fields{
"runner_id": runner.ID,
Expand All @@ -220,10 +220,12 @@ func (c *RunnerController) GetRunner(w http.ResponseWriter, r *http.Request) {
helpers.WriteError(w, err)
return
}
data.AccessKeys[tsk.Inventory.Repository.SSHKeyID] = tsk.Inventory.Repository.SSHKey
data.AccessKeys[*tsk.Inventory.Repository.SSHKeyID] = *tsk.Inventory.Repository.SSHKey
}

data.AccessKeys[tsk.Repository.SSHKeyID] = tsk.Repository.SSHKey
if tsk.Repository.SSHKeyID != nil {
data.AccessKeys[*tsk.Repository.SSHKeyID] = *tsk.Repository.SSHKey
}

} else {
data.CurrentJobs = append(data.CurrentJobs, runners.JobState{
Expand Down
1 change: 1 addition & 0 deletions db/Migration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ func GetMigrations(dialect string) []Migration {
{Version: "2.18.2"},
{Version: "2.18.4"},
{Version: "2.18.5"},
{Version: "2.18.7"},
}

return append(initScripts, commonScripts...)
Expand Down
4 changes: 2 additions & 2 deletions db/Repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ type Repository struct {
ProjectID int `db:"project_id" json:"project_id" backup:"-"`
GitURL string `db:"git_url" json:"git_url" binding:"required"`
GitBranch string `db:"git_branch" json:"git_branch" binding:"required"`
SSHKeyID int `db:"ssh_key_id" json:"ssh_key_id" binding:"required" backup:"-"`
SSHKeyID *int `db:"ssh_key_id" json:"ssh_key_id" binding:"required" backup:"-"`

SSHKey AccessKey `db:"-" json:"-" backup:"-"`
SSHKey *AccessKey `db:"-" json:"-" backup:"-"`
}

func (r Repository) ClearCache() error {
Copy link
Copy Markdown

@cursor cursor Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: Medium — denial of service / unsafe git URL handling

For RepositoryHTTP, GetGitURL(false) uses r.SSHKey.Type and r.SSHKey.LoginPassword without checking SSHKey != nil. With nullable ssh_key_id, SSHKey can be nil when this runs (e.g. cmd-git GetGitURL(false)), causing a nil pointer panic or undefined behavior.

Impact: Same DoS class as GoGitClient.getAuthMethod; also risks incorrect clone URLs if execution continued without crashing.

Expand Down
4 changes: 2 additions & 2 deletions db/Repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func TestRepository_GetGitURL(t *testing.T) {
ExpectedGitUrl string
}{
{
Repository: Repository{GitURL: "https://github.com/user/project.git", SSHKey: AccessKey{
Repository: Repository{GitURL: "https://github.com/user/project.git", SSHKey: &AccessKey{
Type: AccessKeyLoginPassword,
LoginPassword: LoginPassword{
Login: "login",
Expand All @@ -58,7 +58,7 @@ func TestRepository_GetGitURL(t *testing.T) {
ExpectedGitUrl: "https://login:password@github.com/user/project.git",
},
{
Repository: Repository{GitURL: "https://github.com/user/project.git", SSHKey: AccessKey{
Repository: Repository{GitURL: "https://github.com/user/project.git", SSHKey: &AccessKey{
Type: AccessKeyLoginPassword,
LoginPassword: LoginPassword{
Password: "password",
Expand Down
6 changes: 5 additions & 1 deletion db/Store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,7 +790,11 @@ func StoreSession(store Store, token string, callback func()) {
}

func ValidateRepository(store Store, repo *Repository) (err error) {
_, err = store.GetAccessKey(repo.ProjectID, repo.SSHKeyID)
if repo.SSHKeyID == nil {
return
}

_, err = store.GetAccessKey(repo.ProjectID, *repo.SSHKeyID)

return
}
Expand Down
7 changes: 6 additions & 1 deletion db/bolt/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ func (d *BoltDb) GetRepository(projectID int, repositoryID int) (repository db.R
if err != nil {
return
}
repository.SSHKey, err = d.GetAccessKey(projectID, repository.SSHKeyID)

if repository.SSHKeyID != nil {
var k db.AccessKey
k, err = d.GetAccessKey(projectID, *repository.SSHKeyID)
repository.SSHKey = &k
}
return
}

Expand Down
18 changes: 17 additions & 1 deletion db/sql/migration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,13 @@ func (d *SqlDb) ApplyMigration(migration db.Migration) error {
}
}

if d.GetDialect() == util.DbDriverSQLite {
_, err = d.Sql().Exec("PRAGMA foreign_keys = OFF")
Copy link
Copy Markdown

@cursor cursor Bot May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — integrity / defense-in-depth

PRAGMA foreign_keys = OFF is executed before Begin(), but if Begin() fails (return err at ~202) or any of the early return err paths after Rollback() (~213–214, ~247–248, ~253–254) run, the connection never reaches the new PRAGMA foreign_keys = ON block (post-commit). SQLite keeps FK enforcement off for that connection until something else turns it on.

Impact: After a failed migration, subsequent operations on the same pool connection can insert/update rows that violate FK constraints, weakening DB integrity and complicating exploitation of other bugs.

Fix direction: defer a function that sets foreign_keys = ON when SQLite, or use a small helper that always restores on every exit path (including Begin failure and rollbacks).

if err != nil {
Comment on lines +193 to +195
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore SQLite foreign_keys when migration exits early

SQLite foreign keys are turned off before starting the transaction, but turned back on only after a successful commit. Several earlier return paths in this function (e.g., begin failure, pre/post-apply errors, insert failure) bypass the re-enable step, so a migration error can leave the process with foreign key enforcement disabled.

Useful? React with 👍 / 👎.

panic(err)
}
}

tx, err := d.Sql().Begin()
if err != nil {
return err
Expand Down Expand Up @@ -249,7 +256,16 @@ func (d *SqlDb) ApplyMigration(migration db.Migration) error {

fmt.Println()

return tx.Commit()
res := tx.Commit()

if d.GetDialect() == util.DbDriverSQLite {
_, err = d.Sql().Exec("PRAGMA foreign_keys = ON")
if err != nil {
panic(err)
}
}

return res
}

// TryRollbackMigration attempts to rollback the database to an earlier version if a rollback exists
Expand Down
1 change: 1 addition & 0 deletions db/sql/migrations/v.2.18.7.err.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-- do nothing
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Rename 2.18.7 rollback file to expected pattern

The rollback loader resolves files as v<version>.err.sql (via getVersionErrPath), so v.2.18.7.err.sql is never discovered. That means rollback handling for migration 2.18.7 silently skips its SQL, which can leave schema changes in place while rollback flow proceeds as if it succeeded.

Useful? React with 👍 / 👎.

31 changes: 31 additions & 0 deletions db/sql/migrations/v2.18.7.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{{if .Sqlite}}
create table project__repository_dg_tmp
(
id INTEGER
primary key autoincrement,
project_id INTEGER not null
references project
on delete cascade,
git_url TEXT not null,
ssh_key_id INTEGER
references access_key,
name VARCHAR(255),
git_branch VARCHAR(255) default '' not null
);

insert into project__repository_dg_tmp(id, project_id, git_url, ssh_key_id, name, git_branch)
select id, project_id, git_url, ssh_key_id, name, git_branch
from project__repository;

drop table project__repository;

alter table project__repository_dg_tmp rename to project__repository;

create index project__repository__project_id on project__repository (project_id);

create index project__repository__ssh_key_id on project__repository (ssh_key_id);
{{else if .Mysql}}
alter table project__repository modify ssh_key_id int null;
{{else if .Postgresql}}
alter table public.project__repository alter column ssh_key_id drop not null;
{{end}}
6 changes: 5 additions & 1 deletion db/sql/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ func (d *SqlDb) GetRepository(projectID int, repositoryID int) (db.Repository, e
return repository, err
}

repository.SSHKey, err = d.GetAccessKey(projectID, repository.SSHKeyID)
if repository.SSHKeyID != nil {
var key db.AccessKey
key, err = d.GetAccessKey(projectID, *repository.SSHKeyID)
repository.SSHKey = &key
}

return repository, err
}
Expand Down
22 changes: 16 additions & 6 deletions db_lib/CmdGitClient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,14 @@ func (c CmdGitClient) makeCmd(

func (c CmdGitClient) run(r GitRepository, targetDir GitRepositoryDirType, args ...string) error {
var err error
keyInstallation, err := c.keyInstaller.Install(r.Repository.SSHKey, db.AccessKeyRoleGit, r.Logger)
var keyInstallation ssh.AccessKeyInstallation

if err != nil {
return err
if r.Repository.SSHKey != nil {
keyInstallation, err = c.keyInstaller.Install(*r.Repository.SSHKey, db.AccessKeyRoleGit, r.Logger)

if err != nil {
return err
}
}

defer keyInstallation.Destroy() //nolint: errcheck
Expand All @@ -77,9 +81,15 @@ func (c CmdGitClient) run(r GitRepository, targetDir GitRepositoryDirType, args
}

func (c CmdGitClient) output(r GitRepository, targetDir GitRepositoryDirType, args ...string) (out string, err error) {
keyInstallation, err := c.keyInstaller.Install(r.Repository.SSHKey, db.AccessKeyRoleGit, r.Logger)
if err != nil {
return

var keyInstallation ssh.AccessKeyInstallation

if r.Repository.SSHKey != nil {
keyInstallation, err = c.keyInstaller.Install(*r.Repository.SSHKey, db.AccessKeyRoleGit, r.Logger)

if err != nil {
return
}
}

defer keyInstallation.Destroy() //nolint: errcheck
Expand Down
12 changes: 7 additions & 5 deletions db_lib/GoGitClient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,14 @@ func (c GoGitClient) getAuthMethod(r GitRepository) (transport.AuthMethod, error
switch r.Repository.SSHKey.Type {
Copy link
Copy Markdown

@cursor cursor Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: Medium — denial of service (availability)

ValidateRepository returns without loading a key when SSHKeyID is nil, so Repository.SSHKey can be nil at runtime for normal API-created repos. This function still does switch r.Repository.SSHKey.Type, which panics on nil before any auth branch. Any authenticated user who can add a remote repository without ssh_key_id can cause the server process to crash when schedules, tasks, or branch listing run git via the default go-git client.

Impact: Process panic → denial of service for the Semaphore instance.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Guard nil SSH key before selecting auth method

Repository.SSHKey is now nullable, but getAuthMethod immediately dereferences it in switch r.Repository.SSHKey.Type. For repositories created without a key (e.g., public HTTPS repos), any go-git operation that calls this path will panic before reaching the later nil check, so clone/pull/fetch can crash instead of using unauthenticated access.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@cursor cursor Bot May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity: High (availability / DoS)

Repository.SSHKey is now optional, but this line still dereferences it unconditionally. For a repo with SSHKeyID == nil and a normal https:// URL, GetType() is not RepositoryLocal, so git operations call getAuthMethod, hit switch r.Repository.SSHKey.Type, and panic (nil pointer). Any project user who can create such a repo can break task execution for runners using go_git.

Fix direction: Guard the whole switch with if r.Repository.SSHKey == nil { return nil, nil } (or equivalent) before reading .Type, and treat nil key like AccessKeyNone for HTTPS.

Copy link
Copy Markdown

@cursor cursor Bot May 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Medium — denial of service (nil dereference)

Repositories can now have ssh_key_id / SSHKey unset. This function still does switch r.Repository.SSHKey.Type and later dereferences r.Repository.SSHKey (e.g. SSH branch). If SSHKey is nil, the server panics on clone/pull/list branches for that repo.

Impact: Any user who can create/update a repo to omit the key (or any code path that loads such a row) can trigger repeated crashes against endpoints using the Go Git client (e.g. branch listing), affecting availability.

Fix direction: Guard at the top of getAuthMethod (nil SSHKey → return nil, nil or explicit error) before the switch, and ensure SSH/HTTP branches never dereference nil.

case db.AccessKeySSH:

install, err := c.keyInstaller.Install(r.Repository.SSHKey, db.AccessKeyRoleGit, r.Logger)
if err != nil {
return nil, err
}
if r.Repository.SSHKey != nil {
install, err := c.keyInstaller.Install(*r.Repository.SSHKey, db.AccessKeyRoleGit, r.Logger)
if err != nil {
return nil, err
}

defer install.Destroy()
defer install.Destroy()
}

var sshKeyBuff = r.Repository.SSHKey.SshKey.PrivateKey

Expand Down
10 changes: 7 additions & 3 deletions services/export/Repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,13 @@ func (e *RepositoryExporter) restoreValue(val EntityObject[db.Repository], store
return err
}

old.SSHKeyID, err = exporter.getNewKeyInt(AccessKey, val.scope, old.SSHKeyID)
if err != nil {
return err
if old.SSHKeyID != nil {
var k int
k, err = exporter.getNewKeyInt(AccessKey, val.scope, *old.SSHKeyID)
if err != nil {
return err
}
old.SSHKeyID = &k
}

newObj, err := store.CreateRepository(old)
Expand Down
7 changes: 5 additions & 2 deletions services/project/backup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,13 @@ func (b *BackupDB) format() (*BackupFormat, error) {

repositories := make([]BackupRepository, len(b.repositories))
for i, o := range b.repositories {
SSHKey, _ := findNameByID[db.AccessKey](o.SSHKeyID, b.keys)
var key *string
if o.SSHKeyID != nil {
key, _ = findNameByID[db.AccessKey](*o.SSHKeyID, b.keys)
}
repositories[i] = BackupRepository{
Repository: o,
SSHKey: SSHKey,
SSHKey: key,
}
}

Expand Down
2 changes: 1 addition & 1 deletion services/project/backup_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func TestBackupProject(t *testing.T) {

repo, err := store.CreateRepository(db.Repository{
ProjectID: proj.ID,
SSHKeyID: key.ID,
SSHKeyID: &key.ID,
Name: "Test",
GitURL: "git@example.com:test/test",
GitBranch: "master",
Expand Down
13 changes: 8 additions & 5 deletions services/project/restore.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,11 +218,14 @@ func (e BackupRepository) Verify(backup *BackupFormat) error {
}

func (e BackupRepository) Restore(store db.Store, b *BackupDB) error {
var SSHKeyID int
if k := findEntityByName[db.AccessKey](e.SSHKey, b.keys); k == nil {
return fmt.Errorf("SSHKey does not exist in keys[].Name")
} else {
SSHKeyID = (*k).ID
var SSHKeyID *int

if e.SSHKey != nil {
if k := findEntityByName[db.AccessKey](e.SSHKey, b.keys); k == nil {
return fmt.Errorf("SSHKey does not exist in keys[].Name")
} else {
SSHKeyID = &(*k).ID
}
}

repo := e.Repository
Expand Down
10 changes: 7 additions & 3 deletions services/runners/job_pool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,10 @@ func (p *JobPool) checkNewJobs() {
},
}

taskRunner.job.Repository.SSHKey = response.AccessKeys[taskRunner.job.Repository.SSHKeyID]
if taskRunner.job.Repository.SSHKeyID != nil {
k := response.AccessKeys[*taskRunner.job.Repository.SSHKeyID]
taskRunner.job.Repository.SSHKey = &k
}

if taskRunner.job.Inventory.SSHKeyID != nil {
taskRunner.job.Inventory.SSHKey = response.AccessKeys[*taskRunner.job.Inventory.SSHKeyID]
Expand All @@ -702,8 +705,9 @@ func (p *JobPool) checkNewJobs() {
}
taskRunner.job.Template.Vaults = vaults

if taskRunner.job.Inventory.RepositoryID != nil {
taskRunner.job.Inventory.Repository.SSHKey = response.AccessKeys[taskRunner.job.Inventory.Repository.SSHKeyID]
if taskRunner.job.Inventory.RepositoryID != nil && taskRunner.job.Inventory.Repository.SSHKeyID != nil {
k := response.AccessKeys[*taskRunner.job.Inventory.Repository.SSHKeyID]
taskRunner.job.Inventory.Repository.SSHKey = &k
}

p.queue = append(p.queue, &taskRunner)
Expand Down
4 changes: 3 additions & 1 deletion services/schedules/SchedulePool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,9 @@ func (r ScheduleRunner) tryUpdateScheduleCommitHash(schedule db.Schedule) (updat
return
}

err = r.pool.encryptionService.DeserializeSecret(&repo.SSHKey)
if repo.SSHKey != nil {
err = r.pool.encryptionService.DeserializeSecret(repo.SSHKey)
}
if err != nil {
return
}
Expand Down
Loading
Loading