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

Fix URI decoding for auth_digest #1717

Merged
merged 3 commits into from Jun 18, 2018

Conversation

Projects
None yet
3 participants
@teoric
Contributor

teoric commented Jun 12, 2018

decoding URIs in auth header breaks e.g. if it contains slashes

  • What kind of change does this PR introduce?

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

#1716

  • What is the current behavior? (You can also link to an open issue here)

Authentication loop, see #1716

  • What is the new behavior (if this is a feature change)?

Authentication, as before 14.2

  • Other information:

  • Checklist:

    • I think the code is trivial
@jaraco

This comment has been minimized.

Member

jaraco commented Jun 17, 2018

I did some digging into the changes for 14.2, but according to the PR and the changelog, that would affect basic auth, not digest auth. How is that change implicated?

@teoric

This comment has been minimized.

Contributor

teoric commented Jun 17, 2018

Please consider dda3364 (Add support for UTF-8 in auth_digest tool), which introduced the change I am talking about. I suppose the Changelog is just not up complete. In the PR #1683, dda3364 and three other changes to auth_digest are listed among the changes on 22nd April.

jaraco added a commit that referenced this pull request Jun 17, 2018

@jaraco

This comment has been minimized.

Member

jaraco commented Jun 17, 2018

Thanks for the clarification. Now that I understand it a bit better, I'm pretty sure this change isn't the right one. I'm not sure what the right change is, though. Let's continue the discussion in the ticket.

@jaraco jaraco closed this Jun 17, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jun 17, 2018

+1 I also felt like this is a weird place for trying to fix such issue.

@teoric

This comment has been minimized.

Contributor

teoric commented Jun 17, 2018

Well, if it fixes a regression, would it not be sensible to accept a hacky fix first and search for the one true way later?

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jun 17, 2018

@teoric,

I'm really grateful that you spend efforts trying to figure this out and help us. We are short on maintainers and are really lucky to have such people like you contributing to the project and making it better!

This change, however, doesn't fix a regression, it monkey-patches what's related/unique to your concrete application, not CherryPy. And it patches the wrong place meaning that something in someone else's workflow gets broken. It's dangerous/harmful to throw in patches like this (even with no tests covering this case), this will likely cause another regression, which would be much harder to identify and eliminte.

As maintainers, @jaraco and I are responsible for the changes we accept and we do our best to avoid mistakes, which might be hard to fix in future. We have to weight every decision we make to ensure we don't introduce sudden regressions, don't break pre-decided design considerations, keep the code base in a good shape.

I hope that this issue gets fixed soon and as a quick workaround for your app I may suggest you to copy-paste this tool into your app, do whatever temporary patches necessary and use it instead of a built-in tool and then switch back to native tool once the fix is released.

Have a nice day and again thank you for your contributions!

@webknjaz webknjaz reopened this Jun 17, 2018

@codecov

This comment has been minimized.

codecov bot commented Jun 17, 2018

Codecov Report

Merging #1717 into master will decrease coverage by 1.35%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1717      +/-   ##
==========================================
- Coverage   80.81%   79.45%   -1.36%     
==========================================
  Files         105      105              
  Lines       13566    13563       -3     
==========================================
- Hits        10963    10777     -186     
- Misses       2603     2786     +183
Fix URI decoding for auth_digest
decoding URIs in auth header breaks e.g. if it contains slashes

@webknjaz webknjaz force-pushed the teoric:master branch from 9d912a7 to 8ea08c6 Jun 17, 2018

webknjaz added a commit to teoric/cherrypy that referenced this pull request Jun 17, 2018

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jun 18, 2018

I've added changes, which should completely eliminate the issue and take into account the format in which browsers send unicode digest auth

webknjaz added some commits Jun 17, 2018

@webknjaz webknjaz force-pushed the teoric:master branch from 43680c7 to 2b903bc Jun 18, 2018

@teoric

This comment has been minimized.

Contributor

teoric commented Jun 18, 2018

Thank you very much for unmonkeying and fixing this!

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jun 18, 2018

Could you please check this across browsers, at least FireFox, because I don't have one on the current machine.
Also, I'm going to wait until @jaraco reviews this again to double-check changes.

@teoric

This comment has been minimized.

Contributor

teoric commented Jun 18, 2018

Works for me on Firefox and Chrome; thank you!

@webknjaz

This comment has been minimized.

Member

webknjaz commented Jun 18, 2018

Great! Let's wait till @jaraco validates it as well then :)

@jaraco

jaraco approved these changes Jun 18, 2018

Although I'm not happy with the decode routine, since this addresses the issue, I'm happy to accept it to address the regression.

k: tonative(v, enc)
for k, v in param_map.items()
}
return tonative(ntob(tonative(header, 'latin1'), 'latin1'), enc)

This comment has been minimized.

@jaraco

jaraco Jun 18, 2018

Member

This code is clearly not optimal or intuitive... and int introduces two additional deprecated calls. I want to clean this up.

This comment has been minimized.

@webknjaz

webknjaz Jun 18, 2018

Member

Yeah, I tried that. But it's a can of worms, there's too many moving parts. Let's do refactoring separately from the bug fix, please.

@webknjaz webknjaz merged commit 2b903bc into cherrypy:master Jun 18, 2018

2 of 4 checks passed

LGTM analysis: Python Analysis Failed (could not build the base commit (b53123f))
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

webknjaz added a commit that referenced this pull request Jun 18, 2018

Stop %-decoding Authorization header @ Digest Auth
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

Stop %-decoding Authorization header @ Digest Auth
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