Open ChrisRx opened 4 years ago
Merging #39 into master will decrease coverage by
0.14%
. The diff coverage is75.00%
.
@@ Coverage Diff @@
## master #39 +/- ##
==========================================
- Coverage 59.26% 59.11% -0.15%
==========================================
Files 6 6
Lines 518 521 +3
==========================================
+ Hits 307 308 +1
- Misses 208 209 +1
- Partials 3 4 +1
Impacted Files | Coverage Δ | |
---|---|---|
random/random.go | 87.50% <75.00%> (-12.50%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 4919956...57f6fdb. Read the comment docs.
I tried to make codecov happy locally, however, I think the difference it calculates in coverage is unavoidable. I think it is mistaken about the error being handled for the call to ReadByte()
, because I believe the only safe thing to do should it return any error is for the program to crash. Is there anything that I need to do to help get this merged in?
This replaces the usage of math/rand with crypto/rand to support downstream usages of the random package that have security implications, such as the csrf middleware.
I don't see it used a lot of places, the only place outside of the csrf middleware appears to be the request_id middleware. If it makes more sense to ensure performance for non-crypographic usage of this package (which request_id middleware appears to be), I can create a second type and constructor for
SecureRandom
that uses the crypto/rand source, leaving the existing behavior for the Random type. Just let me know what you prefer.