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.

@gr8man gr8man force-pushed the fix/sodium-handler-encrypt-decrypt-bugs branch from 8fd295e to 4c47247 Compare June 21, 2026 19:06
@gr8man gr8man requested a review from michalsn June 21, 2026 20:03
@michalsn

Copy link
Copy Markdown
Member

For future, when updating your PR, please use rebase instead of merge:

git remote add upstream https://github.com/codeigniter4/CodeIgniter4.git
git fetch upstream

git switch fix/problem123
git rebase upstream/develop

git push --force-with-lease origin fix/problem123

The upstream remote only needs to be added once. If you already added it before, you can skip the first command.

- **Commands:** Fixed a bug where ``spark lang:find`` treated translation keys already provided by the framework or another namespace (such as ``Errors.*`` in ``system/Language``) as new, listing them under ``--show-new`` and writing untranslated placeholders into ``app/Language`` that overrode the existing translations.
- **Config:** Fixed a bug where ``BaseService::injectMock`` did not apply ``strtolower`` consistently, causing inconsistent Service and Mock registration and resolution.
- **Database:** Fixed a bug where ``updateBatch()`` could be called after Query Builder ``where()`` conditions, even though it's not supported. In this situation, now the ``DatabaseException`` is thrown.
- **Encryption:** Fixed bugs in ``SodiumHandler`` where incorrect key handling and mismatched block sizes caused encryption and decryption failures.

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.

Suggested change
- **Encryption:** Fixed bugs in ``SodiumHandler`` where incorrect key handling and mismatched block sizes caused encryption and decryption failures.
- **Encryption:** Fixed bugs in ``SodiumHandler`` where runtime ``blockSize`` overrides without ``key`` were handled incorrectly, invalid key lengths could leak native Sodium errors, and mismatched decrypt ``blockSize`` values could throw ``SodiumException`` instead of ``EncryptionException``.

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