Skip to content

fix: issue#62 strip auth key #64

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timazhum
Copy link

Fixing the issue raised in #62

Problem: auth key with leading / trailing whitespaces is throwing exceptions. For example, assigning the secret via echo leaves a trailing \n.

Solution: strip input auth key as an input sanitization procedure

Notice: Java 8 does not support String.strip(), while String.trim() does not support Unicode whitespaces. Therefore, using regex expressions for stripping.

@JanEbbing
Copy link
Member

Oh, this looks very nice, thanks for that! I will review on monday

Copy link
Member

@JanEbbing JanEbbing left a comment

Choose a reason for hiding this comment

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

Fundamentally looks good to me, thanks :)

I'll just get a second opinion from a colleague by tomorrow.

@timazhum
Copy link
Author

Amended the commit, using trim() instead. Also updated a null/empty string checker to match with C# implementation (https://github.com/DeepLcom/deepl-dotnet/blob/main/DeepL/Translator.cs#L438)

@JanEbbing
Copy link
Member

Happy to merge this once Leon's comment is addressed :) thanks for your work!

Fixing the issue raised in DeepLcom#62

Problem: auth key with leading / trailing whitespaces is throwing exceptions. For example, assigning the secret via `echo` leaves a trailing `\n`.

Solution: strip input auth key as an input sanitization procedure

Notice: Java 8 does not support `String.strip()`, while `String.trim()` does not support Unicode whitespaces. We anticipate ASCII only input and utilizing `trim()` for simplicity.
@timazhum
Copy link
Author

Happy to merge this once Leon's comment is addressed :) thanks for your work!

✅ Addressed, thanks for all the feedback provided

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants