fix: more forgiving base64 transformation [custom implementation] #944
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #944 +/- ##
==========================================
+ Coverage 82.65% 82.71% +0.06%
==========================================
Files 162 162
Lines 9028 9062 +34
==========================================
+ Hits 7462 7496 +34
Misses 1317 1317
Partials 249 249
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| return stringsutil.WrapUnsafe(dec), true, nil | ||
|
|
||
| // Handle any remaining characters | ||
| if n == 2 { |
There was a problem hiding this comment.
Does it still make sense to executer these when we break above on illegal character?
There was a problem hiding this comment.
Early returning dst.String() when we break above on illegal character (so not when padding reached) leads to some failing tests:
--- FAIL: TestBase64Decode (0.00s)
--- FAIL: TestBase64Decode/decoded_up_to_the_space_(invalid_character) (0.00s)
/Users/matteopace/Repo/coraza/internal/transformations/base64decode_test.go:83: Expected "<T", but got ""
--- FAIL: TestBase64Decode/decoded_up_to_the_dot_(invalid_character)#02 (0.00s)
/Users/matteopace/Repo/coraza/internal/transformations/base64decode_test.go:83: Expected "<script", but got "<scrip"
--- FAIL: TestBase64Decode/decoded_up_to_the_dash_(invalid_character_for_base64,_only_valid_for_Base64url) (0.00s)
/Users/matteopace/Repo/coraza/internal/transformations/base64decode_test.go:83: Expected "<TEST", but got "<TE"
Trailing characters require to be rearranged even if that case, as if the end of the string was reached
| @@ -31,7 +100,7 @@ func BenchmarkB64Decode(b *testing.B) { | |||
|
|
|||
| func FuzzB64Decode(f *testing.F) { | |||
| for _, tc := range b64DecodeTests { | |||
| f.Add(tc) | |||
| f.Add(tc.input) | |||
There was a problem hiding this comment.
Interestingly the fuzz test was still set up for this forgiving version, that's nice
|
|
||
| for ; srcc < slen; srcc++ { | ||
| // If invalid character or padding reached, we stop decoding | ||
| if src[srcc] == '=' || src[srcc] == ' ' || src[srcc] > 127 || base64DecMap[src[srcc]] == 127 { |
There was a problem hiding this comment.
Extract variable for src[srcc]
There was a problem hiding this comment.
Also while it means having two conditionals that can break, I think it's worth extracting a variable for base64DecMap[src[srcc]] rather than do it twice
|
Thanks @anuraaga for the review and all the guidance. I will wait a bit for any other feedback from others before merging this one and closing the other |
|
Please add a reference or a link to he golang tests we borrowed, a brief
explanation on why we do this vs using std and I think we are ready to go.
Maybe adding a doc.go vs the readme is more idiomatic.
…On Wed, 27 Dec 2023, 13:28 Felipe Zipitría, ***@***.***> wrote:
***@***.**** approved this pull request.
IMHO, this is ready to go.
—
Reply to this email directly, view it on GitHub
<#944 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYARCYMMNO6XDB6UUZITYLQH57AVCNFSM6AAAAABA2H42RCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOOJXGE2DCNRUGQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
Sibling of #940, providing a custom implementation of base64 decoding. The same tests are executed.
The implementation is a refactored version of #758 to allow missing padding and decode up to an illegal character.
We have to decide between:
Benchmarks:
PR: custom implementation (this PR)
PR: Std library (#940)
Tentatively closes #926, it should also fix 934131-5 and 934131-7 CRS 4.0.0-rc2 failing test (#899)