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

[TIMOB-25727] iOS/Android: Fix Base64 behavior and accepted parameter types #9773

Merged
merged 5 commits into from Feb 5, 2018

Conversation

hansemannn
Copy link
Collaborator

@hansemannn hansemannn added this to the 7.1.0 milestone Jan 26, 2018
Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

You already have a manual example in the JIRA ticket, please add it to our Base64 unit tests.

Should be pretty easy to add a few tests for improperly padded encoded strings and verify we handle them and decode them as expected.

[str appendString:@"="];
}

if ([str rangeOfString:@"="].length > 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't you want to check the string before adding padding?

NSMutableString *str = [[NSMutableString alloc] initWithString:[[self convertToString:args]
stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]]];
// Fill padding for backwards compatibility
while ([str length] % 4 != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is right. Base64 encoded strings will only have one or two '=' characters as padding, which suggest modulo 3, not 4.

https://stackoverflow.com/questions/4080988/why-does-base64-encoding-require-padding-if-the-input-length-is-not-divisible-by

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The RFC sounds helpful:

(1) the final quantum of encoding input is an integral multiple of 24
   bits; here, the final unit of encoded output will be an integral
   multiple of 4 characters with no "=" padding,

   (2) the final quantum of encoding input is exactly 8 bits; here, the
   final unit of encoded output will be two characters followed by two
   "=" padding characters, or

   (3) the final quantum of encoding input is exactly 16 bits; here, the
   final unit of encoded output will be three characters followed by one
   "=" padding character.

@sgtcoolguy
Copy link
Contributor

Some possible unit test examples:

Decoded Properly padded Improperly Padded
f Zg== Zg
fo Zm8= Zm8
foo Zm9v N/A
foob Zm9vYg== Zm9vYg
fooba Zm9vYmE= Zm9vYmE
foobar Zm9vYmFy N/A

@hansemannn
Copy link
Collaborator Author

@sgtcoolguy Fixed the iOS padding for all cases, verified the unit-tests and fixed a few more of them (all iOS and Android ones). iOS now accepts the "TiFile" parameter as well and Android does for the decoding as well (only worked for encoding on Android). Test-case is also listed on JIRA.

@hansemannn hansemannn changed the title [TIMOB-10368] iOS: Apply base64 padding manually to restore old behavior [TIMOB-10368] iOS/Android: Fix Base64 behavior and accepted parameter types Jan 28, 2018
@hansemannn hansemannn changed the title [TIMOB-10368] iOS/Android: Fix Base64 behavior and accepted parameter types [TIMOB-25727] iOS/Android: Fix Base64 behavior and accepted parameter types Jan 29, 2018
@build
Copy link
Contributor

build commented Jan 29, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

# Conflicts:
#	android/modules/utils/src/java/ti/modules/titanium/utils/UtilsModule.java

- name: base64encode
summary: Returns the specified data encoded to Base64.
description: |
Starting from Titanium 3.3.0, `obj` can be a [File](Titanium.Filesystem.File) object
on Android.
The parameter can be a String, Ti.Blob or Ti.File (iOS and Android only).
Copy link
Contributor

Choose a reason for hiding this comment

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

May still want to retain note that Ti.Filesystem/File is supported on Android 3.3+, iOS 7.0.3+ (or whatever version this PR and cherry-picks gets into).

});

// FIXME Windows gives: 'base64decode: attempt to decode a value not in base64 char set'
it.windowsBroken('#base64decode(Ti.Blob)', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add tests for #base64decode(Ti.Filesystem.File)

should(test.getText()).be.eql('dGVzdA==');
});

it('#base64encode(Ti.Blob)', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add test for #base64encode(Ti.Filesystem.File)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

Looks good. I'd add tests for base64encode/decode with a Ti.Filesystem.File argument as well for completeness (we already have Ti.Blob test variants, so just pass the file in rather than convert the file to a Ti.Blob first)

@ewieberappc
Copy link
Contributor

FR Passed on 7_0_X branch

@ewieberappc ewieberappc merged commit 5c974f8 into tidev:master Feb 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants