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

Support unicode in login/password during HTTP Auth #1683

Merged
merged 16 commits into from Apr 22, 2018

Conversation

@webknjaz
Copy link
Member

@webknjaz webknjaz commented Jan 3, 2018

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    Bugfix

  • What is the related issue number (starting with #)
    #1680

  • What is the current behavior? (You can also link to an open issue here)
    creds are decoded using latin-1

  • What is the new behavior (if this is a feature change)?
    Creds are decoded according to RFC7617, supporting UTF-8.

  • Other information:
    This should close #1680 once polished.

@webknjaz webknjaz added the bug label Jan 3, 2018
@webknjaz webknjaz self-assigned this Jan 3, 2018
@webknjaz webknjaz requested a review from cherrypy/contributors Jan 3, 2018
@codecov
Copy link

@codecov codecov bot commented Jan 3, 2018

Codecov Report

Merging #1683 into master will decrease coverage by 1.04%.
The diff coverage is 94.23%.

@@            Coverage Diff             @@
##           master    #1683      +/-   ##
==========================================
- Coverage   78.17%   77.12%   -1.05%     
==========================================
  Files         106      105       -1     
  Lines       13796    13725      -71     
==========================================
- Hits        10785    10586     -199     
- Misses       3011     3139     +128
@webknjaz webknjaz force-pushed the bugfix/1680-unicode-in-http-auth branch from e4c9177 to f8172a0 Jan 3, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@cherrypy cherrypy deleted a comment Jan 4, 2018
@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Jan 4, 2018

This pull request introduces 1 alert - view on lgtm.com

new alerts:

  • 1 for Illegal raise

Comment posted by lgtm.com

@cherrypy cherrypy deleted a comment Jan 5, 2018
@cherrypy cherrypy deleted a comment Jan 5, 2018
@cherrypy cherrypy deleted a comment Jan 5, 2018
@cherrypy cherrypy deleted a comment Jan 5, 2018
@cherrypy cherrypy deleted a comment Jan 5, 2018
@cherrypy cherrypy deleted a comment Jan 5, 2018
@cherrypy cherrypy deleted a comment Jan 5, 2018
@cherrypy cherrypy deleted a comment Jan 5, 2018
@cherrypy cherrypy deleted a comment Jan 5, 2018
@PJaros
Copy link

@PJaros PJaros commented Jan 5, 2018

I've tested this PR successfully. Decoding the €-sign doesn't work with 8859-1 which is no surprise as this was introduced with 8859-15. I think it's still an interesting test-case ;)

I've the detailed findings and some test-code in this gist: https://gist.github.com/PJaros/b624e1b600d197c227b132f3c34ba3db

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Jan 5, 2018

@PJaros You are not right regarding removal of 4-5-6. I took this from the RFC: the input should be normalized using NFC.
And the reason is that Unicode allows to produce single visual symbol with several different sequences of characters. For example, ä could typed as a one Unicode character or a sequence of a and :, this would lead to comparison operation claiming that strings are different, while they're not (visually, for end-user). NFC will convert everything into the same type of sequences to avoid ambiguity.

@PJaros
Copy link

@PJaros PJaros commented Jan 5, 2018

@webknjaz Ah, thanks for the explanation concerning the steps 4, 5 and 6. I have yet much to learn about Unicode and UTF-8.

@webknjaz
Copy link
Member Author

@webknjaz webknjaz commented Jan 5, 2018

Yeah, unicode is hard :)

@webknjaz webknjaz force-pushed the bugfix/1680-unicode-in-http-auth branch from 3fe46b8 to 285f7b1 Jan 13, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@cherrypy cherrypy deleted a comment Apr 22, 2018
@webknjaz webknjaz merged commit 7247bf7 into master Apr 22, 2018
2 of 9 checks passed
2 of 9 checks passed
lgtm analysis: Python Analysis Failed (could not build the base commit (1af9a1d))
Details
ci/circleci: linux-build CircleCI is running your tests
Details
ci/circleci: macos-build CircleCI is running your tests
Details
continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
WIP ready for review
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details
@teoric teoric mentioned this pull request Jun 17, 2018
2 of 2 tasks complete
webknjaz added a commit that referenced this pull request Jun 18, 2018
This fixes #1717, which is a regression introduced earlier in #1683
(it affects versions 14.2->16.0).

Co-authored-by: Bernhard Fisseni <bernhard.fisseni@mail.de>
Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>
webknjaz added a commit that referenced this pull request Jun 18, 2018
This fixes #1717, which is a regression introduced earlier in #1683
(it affects versions 14.2->16.0).

Co-authored-by: Bernhard Fisseni <bernhard.fisseni@mail.de>
Co-authored-by: @teoric <code.teoric@gmail.com>
Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.