Skip to content

fix: normalize SodiumHandler params and padding failures#10321

Open
gr8man wants to merge 12 commits into
codeigniter4:developfrom
gr8man:fix/sodium-handler-encrypt-decrypt-bugs
Open

fix: normalize SodiumHandler params and padding failures#10321
gr8man wants to merge 12 commits into
codeigniter4:developfrom
gr8man:fix/sodium-handler-encrypt-decrypt-bugs

Conversation

@gr8man

@gr8man gr8man commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

This PR fixes bugs and improves the validation and error handling logic of the encrypt and decrypt methods in the SodiumHandler.

Key changes:

  • Fixed runtime params handling: blockSize can now be passed without also passing key, while ['key' => null] falls back to the configured instance key.
  • Unified Key Validation: Added key length validation (SODIUM_CRYPTO_SECRETBOX_KEYBYTES) to ensure invalid Sodium keys throw EncryptionException instead of native Sodium errors.
  • Normalized Padding Errors: Wrapped sodium_unpad() failures so mismatched blockSize during decrypt throws EncryptionException.
  • Refactored Params Parsing: Simplified the extraction of $key and $blockSize from the $params array.
  • Code Cleanup: Removed redundant inline comments to improve code readability.

@gr8man gr8man changed the title Fix: Prevent internal key destruction and unify validation in SodiumHandler fix: prevent internal key destruction and unify validation in SodiumHandler Jun 17, 2026
Comment thread system/Encryption/Handlers/SodiumHandler.php Outdated
Comment thread system/Encryption/Handlers/SodiumHandler.php
Comment thread system/Encryption/Handlers/SodiumHandler.php Outdated
Comment thread system/Encryption/Handlers/SodiumHandler.php
Comment thread tests/system/Encryption/Handlers/SodiumHandlerTest.php Outdated
Comment thread system/Encryption/Handlers/SodiumHandler.php Outdated

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rename the new testBug* methods to behavior-focused names. For example, testBug2InvalidKeyLengthThrowsSodiumException() expects EncryptionException, so the current name is misleading.

I also think testBug1MemzeroZeroesInternalKey() and testBug4OriginalDataIsNotZeroed() are questionable as regression tests. Can we either remove them or rewrite them around the actual behavior we want to protect?

Co-authored-by: Michal Sniatala <michal@sniatala.pl>
@michalsn michalsn added the bug Verified issues on the current code behavior or pull requests that will fix them label Jun 18, 2026
@gr8man gr8man requested a review from michalsn June 19, 2026 19:39
@michalsn michalsn changed the title fix: prevent internal key destruction and unify validation in SodiumHandler fix: normalize SodiumHandler params and padding failures Jun 19, 2026

@michalsn michalsn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, thank you. Please add a changelog entry.

For future PRs, please use the PR template when writing the description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants