-
-
Notifications
You must be signed in to change notification settings - Fork 177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Trim = from Base64UrlSafeEncode #691
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add to the unit tests?
Hell -> SGVsbA== -> SGVsbA
Hello -> SGVsbG8= -> SGVsbG8
Fluid/Filters/MiscFilters.cs
Outdated
@@ -196,7 +196,7 @@ public static ValueTask<FluidValue> Base64UrlSafeEncode(FluidValue input, Filter | |||
encodedBase64StringBuilder.Replace('+', '-'); | |||
encodedBase64StringBuilder.Replace('/', '_'); | |||
|
|||
return new StringValue(encodedBase64StringBuilder.ToString()); | |||
return new StringValue(encodedBase64StringBuilder.ToString().TrimEnd('=')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the string builder have something like it? That would prevent the extra string allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we know that base64 encoded version ends with ==
(it should) it should suffice to encodedBase64StringBuilder.Length -= 2;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good option too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not always the case, could be zero, one or two chars. See the test cases I shared for instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I see what you mean now, sorry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue that might be raise during decoding how we know there was zero, one or two "=" on the encoded URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI there's Base64.IsValid()
introduced in .NET 8.0 but it will not solve the problem in our case
Feel free to merge when you add the test, thanks. |
Sure, I will add a test then merge |
Added the same tests to ensure the decoding was fine too and it happens it was not as the Took the time to implement @lahma 's suggestion to skip the string allocation. |
Fixes #690