It's not possible to store an encrypted (serialized) value into MySQL longtext field, I get:

SQLSTATE[HY000]: General error: 1366 Incorrect string value: '\x89\xDF\x16\x09\xD1T...' for column 'keydata' at row 1HY000

It's the part of ['text'] in the serialized array, it seems not UTF-8?

CommentFileSizeAuthor
#15 encrypt-use-base64-by-default-2138495-14.patch3.15 KBrlhawk
#11 encrypt-base64default-2138495-11.patch1.32 KBCellar Door
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Can you test this on the 7.x-2.x version?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Title: Encrypted text is not UTF8 for MySQL longtext storage » Encrypted text is not UTF8 for MySQL storage
Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Actually, it doesn't work. Same problem in 2.x dev branch. Encrypted text is not UTF8.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Version: 7.x-1.x-dev » 7.x-2.x-dev

OK, so still needs work.

BTW, did you try the base64 encoding setting? I believe that makes this work out OK.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

Yes, that works.

jonloh’s picture

jonloh CreditAttribution: jonloh commented

I've tried base64_encode, and when I base64_decode and decrypt, the string is empty. Is there an additional setting to support base64 function?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

@#6, use the function like this:

$encryption = encrypt($message, array('base64' => TRUE));
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Title: Encrypted text is not UTF8 for MySQL storage » Default base64 to true, update docs to discuss why
Category: Bug report » Task

Given that this seems to cause repeated problems and is super confusing to try to debug, I think we should make base64 => TRUE the default.

What do you think?

jonloh’s picture

jonloh CreditAttribution: jonloh commented

#7 Found out the reason - My decoding "steps" were wrong. It should be working fine now.

And yes, I would actually suggest to base64_encode by default, and provide an option for developer to either enable or disable.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I agree to make it default.

Cellar Door’s picture

Cellar Door CreditAttribution: Cellar Door commented
FileSize
encrypt-base64default-2138495-11.patch1.32 KB

I've attached a patch for this to the 2.x-dev branch. It just changes the settings check to look for a FALSE instead of TRUE. This by nature defaults it to base 64 unless someone overrides by passing in an override. Simple variable rename and check is all it took.

Cellar Door’s picture

Cellar Door CreditAttribution: Cellar Door commented
Status: Active » Needs review

Forgot to update the status

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

Looks good to me. There are some uses of base64 option in encryptfapi so we probably need to have a followup issue to fix those.

I feel like the phpseclib.inc should probably also have this option added (right?). I'm not sure why it's not in there now. And we should also document this (though I realize there's currently no documentation about this option, so its a new section).

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Needs review » Needs work

Needs work for the items in #13.
* followup issue in encryptfapi
* modifying phpseclib.inc
* documentation

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
CreditAttribution: rlhawk commented
FileSize
encrypt-use-base64-by-default-2138495-14.patch3.15 KB

Here's a new patch that addresses the above three work items:

1) Encrypt FAPI is no longer included in this project, so any changes will need to be done in that module.
2) I've made Base64-encoding enabled by default for phpseclib.
3) I added notes about Base64-encoding in the PHP doc blocks for the appropriate functions.

rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
CreditAttribution: rlhawk commented
Status: Needs work » Needs review

Changing status to needs review.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented

On visual review this looks solid to me.

I filed #2323313: Handle that base64 is enabled by default after #2138495 as a followup in the encrypt form api queue.

crashtest_’s picture

crashtest_ CreditAttribution: crashtest_ commented
Status: Needs review » Reviewed & tested by the community

I applied the patch, read the super interesting docblock comments, and had a merry old time trying to make anything break while using this. I did find an error in my encrypt_password module, but none here. LOOKS GREAT!

  • rlhawk committed 3e8e16f on 7.x-2.x 
    Issue #2138495 by Cellar Door, greggles, rlhawk: Default Base64 to true.
rlhawk’s picture

rlhawk
Primary language English
Location Seattle, Washington, United States
CreditAttribution: rlhawk commented
Status: Reviewed & tested by the community » Fixed

Committed and marked as fixed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.