ensure decode_password function properly handles plaintext but valid base64 passwords
#5698
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…also valid base64
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
adamsachs
commented
Jan 21, 2025
| @@ -104,7 +104,12 @@ def test_str_to_b64_str() -> None: | |||
| "password, expected", | |||
| [ | |||
| ("Testpassword1!", "Testpassword1!"), | |||
| ( | |||
There was a problem hiding this comment.
this new test was failing before the fix
| @@ -13,7 +13,7 @@ def decode_password(password: str) -> str: | |||
| """ | |||
| try: | |||
| return b64_str_to_str(password) | |||
| except Error: | |||
| except (Error, UnicodeDecodeError): | |||
There was a problem hiding this comment.
decided to keep this minimal in scope - only catching the error i know can be thrown in the error case. handling more generic errors/exceptions would be more likely to lead to unintended side effects, IMO.
also didn't add any sort of logging here as it could get very noisy, but i'd be open to an argument for that
adamsachs
changed the title
decode_password function properly handles plaintext but valid base64 passwords
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
refs/pull/5698/merge
|
| Run status |
|
| Run duration | 01m 08s |
| Commit |
|
| Committer | Adam Sachs |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
1
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
3
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Tests for review
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5698 +/- ##
=======================================
Coverage 87.14% 87.14%
=======================================
Files 388 388
Lines 24034 24034
Branches 2594 2594
=======================================
Hits 20944 20944
Misses 2529 2529
Partials 561 561 ☔ View full report in Codecov by Sentry. |
|
failing tests look flaky to me, merging 👍 |
fides
|
||||||||||||||||||||||||||||||
| Project |
fides
|
| Branch Review |
main
|
| Run status |
|
| Run duration | 00m 51s |
| Commit |
|
| Committer | Adam Sachs |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
4
|
Upgrade your plan to view test results. | |
| View all changes introduced in this branch ↗︎ | |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes HJ-397
Description Of Changes
Handles the
UnicodeDecodeErrorthat is raised bydecode_passwordif it is given a plaintext password that's also valid base64. The exception is caught, and the input string is returned as it was provided, under the assumption that it is a plaintext (and not base64-encoded) password.See issue description for more details. This impacts some user-related endpoints (e.g. reset password, accept invite) if the user provides a password that also happens to be valid base64. Notably, the utilities also ignore invalid base64 characters like
_, so a value like e.g.Test_1234is treated as valid base64, since the_is ignored and its length of8is a multiple of 4.Code Changes
UnicodeDecodeErrorindecode_passwordutility function, assuming input string is a plaintext passwordSteps to Confirm
Test_1234(i think this was working before, because we send a base64 encoded password in the request from the UI in this workflow)Test_1234, which i also confirmed was broken before:Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works