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

Base64 optional padding tests #44503

Merged
merged 5 commits into from
Mar 8, 2022
Merged

Base64 optional padding tests #44503

merged 5 commits into from
Mar 8, 2022

Conversation

fonsp
Copy link
Member

@fonsp fonsp commented Mar 7, 2022

The pad character = is required in base64 encoding, but not in decoding. Padding characters are not needed to correctly decode: https://en.wikipedia.org/wiki/Base64#Output_padding

This PR makes it possible to decode base64-encoded strings/streams without padding 🎉, matching the behaviour in V8 and in the browser (in data urls).

(The official spec for Base64 states that padding is required for base64-encoding, but it does not specify a requirement for decoding.)

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM. In fact, it appears the standard is emphatic about this padding being optional:

In some environments, the alteration is critical and therefore
   decoders MAY chose to reject an encoding if the pad bits have not
   been set to zero.

@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Mar 7, 2022
@fonsp
Copy link
Member Author

fonsp commented Mar 8, 2022

Thanks! I believe that sentence is about pad bits, not the padding character =. For example, base64-encoding the single-byte input 0b11111111 becomes

First     Second    Third    Fourth
   /
111111    11xxxx    yyyyyy   yyyyyy       

Here, x refers to pad bits, y to pad characters. With the default encoding, x is 0, and yyyyyy is the = character, giving /w==.

The first character should always be /, but the second character depends on the choice of padding bits (x). The spec states that during encoding, these should be zero (thus giving 110000, which is w), but during decoding, pad bits should be ignored, so any character between 110000 and 111111 should lead to the same result.

Although this was not part of my PR (and it was already implemented correctly), I added some tests about decoding pad bits here: https://github.com/JuliaLang/julia/pull/44503/files#diff-4fcfc93f2fff4705dceba7d3c1ef7d3aa0c7653ccff897ddc7ab664c4f9b1e37R102 , and cross-checked these with Chrome's implementation.

@vtjnash vtjnash merged commit 88062ea into JuliaLang:master Mar 8, 2022
@fonsp fonsp deleted the patch-9 branch March 8, 2022 22:28
@DilumAluthge DilumAluthge removed the status:merge me PR is reviewed. Merge when all tests are passing label Mar 10, 2022
Vaibhavdixit02 added a commit to JuliaHealth/SMARTAppLaunch.jl that referenced this pull request Apr 14, 2022
Currently we can't decode without padding character, it has been fixed on master JuliaLang/julia#44503 but for now we'll need this to make it work.
@DilumAluthge
Copy link
Member

This seems like a bug fix. Can we backport it to 1.6, 1.7, and 1.8?

@DilumAluthge DilumAluthge added kind:bugfix This change fixes an existing bug backport 1.6 Change should be backported to release-1.6 backport 1.7 backport 1.8 Change should be backported to release-1.8 labels Apr 14, 2022
Vaibhavdixit02 added a commit to JuliaHealth/SMARTAppLaunch.jl that referenced this pull request Apr 14, 2022
* Add padding to support decoding with Base64 stdlib

Currently we can't decode without padding character, it has been fixed on master JuliaLang/julia#44503 but for now we'll need this to make it work.

* Add comment to explain why padding is needed
@vtjnash
Copy link
Sponsor Member

vtjnash commented Apr 14, 2022

It is a new feature mostly

@DilumAluthge DilumAluthge added kind:feature Indicates new feature / enhancement requests and removed backport 1.6 Change should be backported to release-1.6 backport 1.7 backport 1.8 Change should be backported to release-1.8 kind:bugfix This change fixes an existing bug labels Apr 14, 2022
@DilumAluthge DilumAluthge added the stdlib Julia's standard library label May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature Indicates new feature / enhancement requests stdlib Julia's standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants