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

added testcase for newline in api key #185

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Conversation

MKLepium
Copy link
Contributor

@MKLepium MKLepium commented Nov 21, 2023

Addition to what is written in #184

The current implementation of the authorization header results in an edge case where, when the resulting header is deemed invalid from the underlying HTTP library (urllib3), a value error is thrown, which is simply printed. This includes the content of the authorization header (the API key).

By simply stripping any leading or trailing white spaces (which include newlines), this should get rid of the error.

I can also not think of any edge cases where stripping the API key might lead to problems, since only whitespace leading or trailing the actual API key will be removed.

One could make the case that the strip is unnecessary for the bearer token, but my argument for keeping it is that it

  1. can not hurt to have it
  2. the time complexity of stripping the string once can be disregarded.

Last point:
Why did I choose strip instead of strip? If we believe that a user, while pasting the API key, might make the mistake of entering a newline at the end of the API key, he might also paste a newline at the beginning.

Copy link
Contributor

@SirOibaf SirOibaf left a comment

Choose a reason for hiding this comment

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

Thanks

@SirOibaf SirOibaf merged commit 384ae81 into logicalclocks:main Nov 21, 2023
2 checks passed
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.

2 participants