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

SAK-49345 Switch from Base62 to a variant of Base64 #11976

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

csev
Copy link
Contributor

@csev csev commented Oct 7, 2023

Hey @ern and @jonespm - You were right. This commit switches to using Base64 URL Safe Encoding with a small adjustment to make double url encoding not a problem but without building our own Base62 implementation. This in essence reverts the Base62 patch and replace it with an adjusted Base64 encoder.

@csev csev requested review from jonespm, ern and ottenhoff October 7, 2023 18:53
"Testing encoding and decoding.",
"12345",
"This is a longer string with more characters.",
"A tree 🌲 was here",
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't one of these be a really complex url esp one with an underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

Comment on lines 11 to 13
/* This code was written by ChatGPT with the following prompt
* Please write a unit test for a Base64DoubleUrlEncodeSafe class
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

useless comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup.

Base64DoubleUrlEncodeSafe.decode(invalidBase64DoubleUrlEncodeSafeString);
fail("Expected IllegalArgumentException for invalid Base64DoubleUrlEncodeSafe string");
} catch (Exception e) {
// Expected exception
Copy link
Contributor

Choose a reason for hiding this comment

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

you could also assert that an exception was caught, this is a good idea when an exception is expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@csev csev changed the title SAK-49345 Switch from Base62 to a safer Base64 SAK-49345 Switch from Base62 to a variant of Base64 Oct 14, 2023
@csev csev requested review from ern and ottenhoff October 17, 2023 13:58
@ern ern merged commit 2ea87c8 into sakaiproject:master Oct 17, 2023
3 checks passed
ern pushed a commit that referenced this pull request Oct 19, 2023
@csev csev deleted the SAK-49345 branch November 19, 2024 11:23
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