Breezy

http decorators incorrectly handled when authentication is used

Bug #122258 reported by Vincent Ladeuil
2
Affects Status Importance Assigned to Milestone
Bazaar
Confirmed
Low
Vincent Ladeuil
Breezy
Fix Released
Low
Jelmer Vernooij
Breezy 3.0.0 "Pelican"

Bug Description

bzr use urlparse.urlsplit or urlparse.urlparse (which calls urlsplit) to extract scheme, host, path etc from the url

urlparse.urlsplit fails to recognize http+urllib or http+pycurl as valid schemes and returns an empty netloc (user + password + host + port).

As the netloc is prepended to the returned path and bzr concatenate both when needed, this has not been noticed until now.

While no authentication is used, there is no consequences, but failing to extract the user and password from the netloc causes problems when authentication is needed.

But again, this occurs only when a user needs to use an http implementation explicitly, if the user relies on the default implementation by using no decorators, all is fine.

It's arguably a python bug but we have to deal with it.

Related branches

lp://staging/~jelmer/brz/kill-pycurl
Martin Packman: Approve
Diff: 991 lines (+20/-691)
9 files modified
breezy/tests/features.py (+0/-1)
breezy/tests/http_server.py (+0/-22)
breezy/tests/https_server.py (+0/-14)
breezy/tests/test_bzrdir.py (+0/-12)
breezy/tests/test_http.py (+11/-169)
breezy/transport/__init__.py (+4/-18)
breezy/transport/http/__init__.py (+1/-1)
breezy/transport/http/_pycurl.py (+0/-454)
doc/en/release-notes/brz-3.0.txt (+4/-0)
Vincent Ladeuil (vila)
Changed in bzr:
assignee: nobody → v-ladeuil
importance: Undecided → Low
status: New → Confirmed
Revision history for this message
John A Meinel (jameinel) wrote :

I think a simple "bzrlib.transport.register_urlparse_netloc_protocol('http+urllib')" et al will suffice (you need +urllib and +pycurl as well as https).

And then some tests that "get_transport(https+pycurl://user@host...)" is properly parsed for the different variants.

Revision history for this message
Vincent Ladeuil (vila) wrote :

We alreday have the appropriate register_urlparse_netloc_protocol :

grep -nH -e register_urlparse_netloc_protocol -r .
./transport/__init__.py:153:def register_urlparse_netloc_protocol(protocol):
./transport/http/_pycurl.py:47:from bzrlib.transport import register_urlparse_netloc_protocol
./transport/http/_pycurl.py:78:register_urlparse_netloc_protocol('http+pycurl')
./transport/http/_urllib.py:23:from bzrlib.transport import register_urlparse_netloc_protocol
./transport/http/_urllib.py:35:register_urlparse_netloc_protocol('http+urllib')
./transport/remote.py:38: transport.register_urlparse_netloc_protocol(scheme)
./transport/sftp.py:54: register_urlparse_netloc_protocol,
./transport/sftp.py:73:register_urlparse_netloc_protocol('sftp')

Unfortunately it's not enough .

Revision history for this message
Ian Clatworthy (ian-clatworthy) wrote :

Following a review of some proposed changes to bzrdir.py, here are some associated comments on IRC:

(17:00:30) poolie_: it's about, say, if it was http+pycurl then we should preserve the 'pycurl' bit
(17:00:30) igc: when vila get's to it
(17:01:15) poolie_: hm
(17:01:28) poolie_: i think the main thing is that it ought to be a call to something like
(17:01:48) poolie_: target = urlutils.copy_url_qualifiers(original, target)
(17:01:57) poolie_: and then that can be tested separately
(17:02:08) poolie_: rather than doing string slicing inline here

The code in question (which was previously executed but the result was not used), has been removed and replaced with a comment.

Revision history for this message
Vincent Ladeuil (vila) wrote :

The above comment does not strictly apply to this bug (redirection instead of authentication), but both are linked to decorators handling.

The two problems *can* be addressed separately but that would be nice to solve them both in one shot.

Jelmer Vernooij (jelmer)
Changed in brz:
status: New → Triaged
importance: Undecided → Low
milestone: none → 3.0.0
Jelmer Vernooij (jelmer)
Changed in brz:
status: Triaged → Fix Released
assignee: nobody → Jelmer Vernooij (jelmer)
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.
Subscribing...

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.