chore: Add missing error checks identified by CodeQL#1381
Conversation
jkennedyvz
force-pushed
the
jk/code-quality
branch
from
e5fd3bf to
a68db34
Compare
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.
jkennedyvz
force-pushed
the
jk/code-quality
branch
from
a68db34 to
c415de4
Compare
| Limit(50)) | ||
| if err != nil { | ||
| w.logger.Error("Failed to fetch emails from queue", "error", err) | ||
| time.Sleep(sleepDuration * time.Second) |
There was a problem hiding this comment.
It might be better to be explicit about the wait here, instead of just using the last value. I don't know which is more correct, but I lean towards this:
time.Sleep(w.SleepAfterWorkDuration* time.Millisecond)
on the basis that this error likely means that the database is down. Probably it's going to take minutes to come back up, so maybe wait the full minute? But I can see alternate arguments.
There was a problem hiding this comment.
Using exponential backoff might make sense as well so we're not hammering it
jrozner
left a comment
jrozner
left a comment
There was a problem hiding this comment.
Adding this error handling is fine and we should do it. Go 1.13 introduced error wrapping and there's been consistent work toward shifting in this direction. We have some wrapping indirection here to support HTTP errors transparently. I'm wondering if we still need this or we can simplify it? Either way, adding these errors in does no harm, we'll just need to update these in the future if we rework the error wrapping.
3 participants
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:
Related to security assessment code quality findings.
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.