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

Introduce a checkbox in Basic Auth form to choose ISO-8859-1 encoding for credentials. #2642

Merged

Conversation

jgiovaresco
Copy link
Contributor

Introduce a checkbox in Basic Auth form to choose ISO-8859-1 encoding for credentials.

Screenshot 2020-09-19 at 19 57 37

Screenshot 2020-09-19 at 19 57 46

I had to update network's "Handle Authorization header" to use getAuthHeader instead of libcurl options for Basic Http
because it seems that lib-curl does not let us choose the encoding for Basic Auth credentials

Closes #1558

@jgiovaresco jgiovaresco force-pushed the fix/1558-change-basic-auth-encoding branch from 6c67c84 to 0cde4ce Compare September 19, 2020 18:03
@develohpanda develohpanda self-requested a review September 22, 2020 04:57
@nijikokun
Copy link
Contributor

nijikokun commented Sep 23, 2020

I'm actually wondering if we should follow suit and simply apply UTF-8 encoding by default, and have a checkbox to fallback to the previous encoding.

Actually, after looking through the PR, it appears that the default is utf8 and this is a fallback to latin1. Interesting that utf8 isn't properly encoding french accents as it should (see link above for Firefox reasoning). I suspect the server expects the encoding format from latin1 like the listed exception jboss.

Looks good from a product perspective, with one small suggestion that we put Use in front of the encoding.

@netlify
Copy link

netlify bot commented Sep 24, 2020

Deploy preview for insomnia-storybook ready!

Built with commit 9361c8b

https://deploy-preview-2642--insomnia-storybook.netlify.app

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for doing this!

Small change I think is worth addressing, otherwise good to go.

I had to update network's "Handle Authorization header" to use getAuthHeader instead of libcurl options for Basic Http
because it seems that libcurl does not let us choose the encoding.
@jgiovaresco jgiovaresco force-pushed the fix/1558-change-basic-auth-encoding branch from 9361c8b to d0add1b Compare September 24, 2020 18:18
@jgiovaresco
Copy link
Contributor Author

@develohpanda I've rebased the PR and amended the 1st commit (#fef8cd3) with your suggestions. I let you take a look ;)

Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

Nice! 💯

I'm going to merge this in, and will explore adding an end-to-end test for Core in the packages/insomnia-smoke-test project. Feel free to give that a go separately, but it's a little new so there isn't much documentation or instructions as yet.

@develohpanda develohpanda merged commit 9d4390d into Kong:develop Sep 24, 2020
@jgiovaresco jgiovaresco deleted the fix/1558-change-basic-auth-encoding branch December 2, 2020 16:45
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.

Basic authentication doesn't handle special French characters in password
3 participants