Skip to content

Commit e5fd3bf

Browse files
committed
Add missing error checks identified by CodeQL
CodeQL identified 8 instances where error values were assigned but never checked before using the returned data. This commit fixes 7 of them (the 8th is in a separate PR for TOTP nil pointer dereference). Fixed locations: - backend/authschemes/localauth/services.go:53 Check error from FindUserAuthByUserID before using authData - backend/services/operation_vars.go:37, 58, 170 Check error from lookupOperation before using operation.ID in auth checks Check error from ListOperationVars before iterating over results - backend/services/user_groups.go:321, 365 Check error from lookupOperation before using operation.ID in auth checks - backend/workers/email.go:111 Check error from database Select, log and continue on failure These were all potential bugs where: 1. nil pointer dereferences could occur (services.go) 2. authorization checks could use invalid operation IDs (operation_vars, user_groups) 3. database errors were silently ignored (email worker) Related to security assessment code quality findings.
1 parent ddfc8a2 commit e5fd3bf

4 files changed

Lines changed: 23 additions & 0 deletions

File tree

backend/authschemes/localauth/services.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,9 @@ func deleteUserTotp(ctx context.Context, bridge authschemes.AShirtAuthBridge, us
5151
return backend.WrapError("Non-Admin tried to delete totp status for another user", backend.UnauthorizedWriteErr(err))
5252
}
5353
authData, err := bridge.FindUserAuthByUserID(userID)
54+
if err != nil {
55+
return backend.WrapError("Unable to find user authentication data", err)
56+
}
5457

5558
if authData.TOTPSecret == nil {
5659
return backend.BadInputErr(

backend/services/operation_vars.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,9 @@ type DeleteOperationVarInput struct {
3535

3636
func CreateOperationVar(ctx context.Context, db *database.Connection, i CreateOperationVarInput) (*dtos.OperationVar, error) {
3737
operation, err := lookupOperation(db, i.OperationSlug)
38+
if err != nil {
39+
return nil, backend.WrapError("Unable to create operation variable", err)
40+
}
3841
if err := policy.Require(middleware.Policy(ctx), policy.CanCreateOpVars{OperationID: operation.ID}); err != nil {
3942
return nil, backend.WrapError("Unable to create operation variable", backend.UnauthorizedWriteErr(err))
4043
}
@@ -53,6 +56,9 @@ func CreateOperationVar(ctx context.Context, db *database.Connection, i CreateOp
5356
var varID int64
5457

5558
listOfVarsInOperation, err := ListOperationVars(ctx, db, i.OperationSlug)
59+
if err != nil {
60+
return nil, backend.WrapError("Unable to list existing operation variables", err)
61+
}
5662
for _, varInOperation := range listOfVarsInOperation {
5763
if varInOperation.Name == formattedName {
5864
return nil, backend.BadInputErr(errors.New("Unable to create operation variable. Invalid operation variable name"), "A variable with this name already exists in the operation")
@@ -159,6 +165,9 @@ func UpdateOperationVar(ctx context.Context, db *database.Connection, i UpdateOp
159165
formattedName := helpers.StrToUpperCaseUnderscore(i.Name)
160166

161167
listOfVarsInOperation, err := ListOperationVars(ctx, db, i.OperationSlug)
168+
if err != nil {
169+
return backend.WrapError("Unable to list existing operation variables", err)
170+
}
162171
for _, varInOperation := range listOfVarsInOperation {
163172
if varInOperation.Name == formattedName {
164173
return backend.BadInputErr(errors.New("Unable to update operation variable. Invalid operation variable name"), "A variable with this name already exists in the operation")

backend/services/user_groups.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,9 @@ func SortUsersInToGroups(slugMap SlugMap) ([]dtos.UserGroupAdminView, error) {
319319
// Lists all user groups for an operation; op admins and sys admins can view
320320
func ListUserGroupsForOperation(ctx context.Context, db *database.Connection, i ListUserGroupsForOperationInput) ([]*dtos.UserGroupOperationRole, error) {
321321
operation, err := lookupOperation(db, i.OperationSlug)
322+
if err != nil {
323+
return nil, backend.WrapError("Unable to list usergroups", err)
324+
}
322325
if err := policyRequireWithAdminBypass(ctx, policy.CanListUserGroupsOfOperation{OperationID: operation.ID}); err != nil {
323326
return nil, backend.WrapError("Unwilling to list usergroups", backend.UnauthorizedReadErr(err))
324327
}
@@ -357,6 +360,9 @@ func wrapListUserGroupsForOperationResponse(userGroups []userGroupAndRole) []*dt
357360
// lists all user groups that can be added to an operation
358361
func ListUserGroups(ctx context.Context, db *database.Connection, i ListUserGroupsInput) ([]*dtos.UserGroupAdminView, error) {
359362
operation, err := lookupOperation(db, i.OperationSlug)
363+
if err != nil {
364+
return nil, backend.WrapError("Unable to list usergroups", err)
365+
}
360366
if err := policyRequireWithAdminBypass(ctx, policy.CanListUserGroupsOfOperation{OperationID: operation.ID}); err != nil {
361367
return nil, backend.WrapError("Unwilling to list usergroups", backend.UnauthorizedReadErr(err))
362368
}

backend/workers/email.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,11 @@ func (w *EmailWorker) run() {
114114
Where(sq.Expr("error_count < ?", 3)).
115115
OrderBy("updated_at ASC"). // grab the oldest emails first, prefer jobs that have not errored out
116116
Limit(50))
117+
if err != nil {
118+
w.logger.Error("Failed to fetch emails from queue", "error", err)
119+
time.Sleep(emailPollDelay)
120+
continue
121+
}
117122

118123
if len(emails) > 0 {
119124
for _, email := range emails {

0 commit comments

Comments
 (0)