Skip to content

Fix IllegalArgumentException message for unknown Argon2 types #16971

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

Merged
merged 2 commits into from
Apr 24, 2025

Conversation

rntrp
Copy link

@rntrp rntrp commented Apr 21, 2025

Parsing Argon2 params relies on String[] parts = encodedHash.split("\\$"). For a string beginning with a $, the first element of the resulting array will be empty. This is already addressed in the code, with the variable int currentPart being initialized with 1. But the IllegalArgumentException message incorrectly refers to part[0], which will be empty for a well-formed Argon2 string starting with $.

The fix is trivial in this case, just use the array index 1.

P.S. one could also check for the 0-th array element to be an empty string and throw an IllegalArgumentException if it's not. However, this requires additional domain expertise and as such goes beyond the scope.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 21, 2025
@jzheaux
Copy link
Contributor

jzheaux commented Apr 21, 2025

Hi, @rntrp, thanks for the fix. Will you please add a unit test that confirms the behavior? It should be a test that fails before your fix and passes after the fix is applied.

@rntrp
Copy link
Author

rntrp commented Apr 22, 2025

Hello @jzheaux, thanks for the advice. I think it should be sufficient here to extend the existing test case. AssertJ failure messages are concise enough to see whether the right exception wasn't thrown or the message didn't contain the string. Please review the new commit.

@jzheaux jzheaux self-assigned this Apr 22, 2025
@jzheaux jzheaux added in: crypto An issue in spring-security-crypto type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 22, 2025
@jzheaux jzheaux added this to the 6.3.10 milestone Apr 22, 2025
Array index 0 points to an empty string. Use index 1 instead.

Signed-off-by: Roman Trapickin <[email protected]>
@jzheaux jzheaux force-pushed the rntrp-fix-argon2-error-message branch from fae403a to 9d5c2d9 Compare April 22, 2025 19:21
@jzheaux jzheaux changed the base branch from main to 6.3.x April 22, 2025 19:22
@jzheaux jzheaux merged commit 547d174 into spring-projects:6.3.x Apr 24, 2025
6 checks passed
@jzheaux
Copy link
Contributor

jzheaux commented Apr 24, 2025

Thanks, @rntrp ! This is now merged into main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: crypto An issue in spring-security-crypto type: bug A general bug
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants