Add null check for ApiKeyPair in getUserByApiKey#12938
Add null check for ApiKeyPair in getUserByApiKey#12938daviftorres wants to merge 9 commits intoapache:mainfrom
Conversation
Pulling upstream.
The function getUserByApiKey was not checking for null pointers. I think this is safe from the perspective of security, but this is a smell. And it can be easily addressed with the same solution used in line 3160.
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm,
however, it is not needed because the only place AccountManagerImpl.getUserByApiKey(..) is from the API and the GetUserCmd has a required Parameter apiKey. That said, it doesn’t hurt to protect the system agains prodigy and add the null check. It would make more sense before usage in that case.
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #12938 +/- ##
=============================================
- Coverage 18.02% 3.52% -14.50%
=============================================
Files 5973 464 -5509
Lines 537466 40063 -497403
Branches 65991 7534 -58457
=============================================
- Hits 96855 1414 -95441
+ Misses 429689 38461 -391228
+ Partials 10922 188 -10734
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: dahn <daan.hoogland@gmail.com>
Description
The function
getUserByApiKeydoes not check fornullpointers. This is likely safe from a security perspective, but it’s still a code smell and should be improved. It can be fixed easily using the same approach as in line 3160.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
No, it was not tested, it is just a static analysis of the code.
How did you try to break this feature and the system with this change?
No.