Skip to content
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

Add Base64URLSafe decoder #1292

Merged

Conversation

nyanshak
Copy link
Contributor

  • Add decoder that can decode base64 strings with '_' and '-' instead of of '+' and '/'.

Addresses #144.

WIP but looking for early feedback:

  • should this functionality be merged into the existing base64 decoder or split into a separate Base64URLSafe decoder? There is a lot of shared code and wanted to see if maintainers have preferences towards making separate decoders (like UTF8/UTF16 decoders) or a single Base64 decoder before polishing it up, wiring into all the places (detectorspb, decoders, engine, detectors.proto, etc.

* Add decoder that can decode base64 strings with '_' and '-' instead of
  of '+' and '/'.
@nyanshak nyanshak requested a review from a team as a code owner April 26, 2023 19:44
@dustin-decker
Copy link
Contributor

If it's possible to combine the two types of b64 correctly that would be preferred, because there is a decent amount of overhead introduced with each decoder. The UTF8/16 ones are different enough to be separate.

@nyanshak
Copy link
Contributor Author

Okay @dustin-decker - I combined them. I think there's a possible caveat, which is mostly that combining them adds potentially a little overhead in attempting to decode strings that would not be valid b64-encoded strings.

For example, this is an invalid b64 encoded string: abcdef-ghijkl_mnop+qrs/tuvwxyz. In the separate decoder version, we wouldn't waste time trying to decode this, because once we hit a character outside of the character set, we'd try to decode the substring if it was longer than the threshold, but we wouldn't try to decode the entire string.

If the combined version is good enough, then I think this is ready to go. Otherwise, may need to revert the last commit, then wire up the separate B64URLSafe decoder 🤷‍♀️

@nyanshak nyanshak changed the title WIP: Add Base64URLSafe decoder Add Base64URLSafe decoder May 8, 2023
Copy link
Contributor

@dustin-decker dustin-decker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Brendan!

@dustin-decker dustin-decker merged commit 195f9f0 into trufflesecurity:main May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants