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

[5.x] Use multibyte methods for obfuscate #10201

Merged
merged 5 commits into from
May 30, 2024

Conversation

lakkes-ra
Copy link
Contributor

I encountered a bug with using livewire (v3) with Statamic v5 that told me "Malformed UTF-8 characters, possibly incorrectly encoded". After some digging I found out that the obfuscate modifier caused the trouble. I also found out that since PHP 8.2 the utf8_encode() is deprecated and should be replaced with a multibyte encoding method. All of this led me to trying mb_str_split() and mb_ord() for the obfuscate modifier. I reckon the error was caused by a multibyte character in my content and with the multibyte method equivalents I was able to fix it.

Unfortunately I can't really expand on that problem any further, but I think it should be safely replaceable, since the methods just widen the range of values above 128.

@lakkes-ra lakkes-ra changed the title use multibyte methods for obfuscate [5.x] Use multibyte methods for obfuscate May 27, 2024
@duncanmcclean
Copy link
Member

Are you able to provide an example that was previously erroring before this fix?

@lakkes-ra
Copy link
Contributor Author

Hi @duncanmcclean, I'm going to create an example repo. Might take a while though.

@jasonvarga
Copy link
Member

Or update ObfuscateTest.

@lakkes-ra
Copy link
Contributor Author

Hi @jasonvarga, I added a test with mutlibyte characters é and ß. They fail if I use str_split() and ord(). With the multibyte methods, they are successful. Thanks for pointing out – hope that helps?

@lakkes-ra
Copy link
Contributor Author

Hm, now the random test is failing 🤔 "The same value was returned multiple times." I can't really grasp, how my changes affect the random test with the query builder input. Did the random method return the same value by coincidence? 🤔 Do you have an idea to that?

@duncanmcclean
Copy link
Member

Hm, now the random test is failing 🤔 "The same value was returned multiple times." I can't really grasp, how my changes affect the random test with the query builder input. Did the random method return the same value by coincidence? 🤔 Do you have an idea to that?

It's probably not related to your changes. That test fails every now and then 😅 . I've re-run the failing tests.

@jasonvarga
Copy link
Member

I noticed that the test we had already wasn't really testing it properly:

$modified = $this->modify('A');
$this->assertTrue(in_array($modified, ['A', 'A', 'A']));

The modify('A') would randomly return one of the 3 options (intentionally) and then we were asserting that it was one of them. This really didn't test anything. We could have just not obfuscated at all and it would pass since A (the original value) was one of the options

When you added your test, it was still passing because one of the options was é. Yours really didn't obfuscate it at all since the if ($ordValue) condition was still there.

On top of all that, the if ($ordValue) thing did return $letter; which broke the whole thing anyway. It should have been $safe .= $letter; continue;.

This modifier is from an old package that we brought into core. We didn't actually write it. 😅

Anyway! I've updated the test and implementation. Now stuff gets obfuscated and tested correctly.

@jasonvarga jasonvarga merged commit 3de4abf into statamic:5.x May 30, 2024
16 checks passed
@lakkes-ra lakkes-ra deleted the fix/malformed-utf8-character-bug branch May 31, 2024 07:13
@lakkes-ra
Copy link
Contributor Author

Awesome, thanks you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants