Merge lp:~vila/bzr/1606203-long-auth into lp:bzr/2.7

Proposed by Vincent Ladeuil
Status: Merged
Approved by: Richard Wilbur
Approved revision: no longer in the source branch.
Merged at revision: 6617
Proposed branch: lp:~vila/bzr/1606203-long-auth
Merge into: lp:bzr/2.7
Diff against target: 41 lines (+12/-1)
2 files modified
bzrlib/tests/test_http.py (+10/-0)
bzrlib/transport/http/_urllib2_wrappers.py (+2/-1)
To merge this branch: bzr merge lp:~vila/bzr/1606203-long-auth
Reviewer Review Type Date Requested Status
Richard Wilbur Approve
Colin Watson (community) Approve
Review via email: mp+305328@code.launchpad.net

Commit message

Fix lp:1606203 caused by using a user/pass combination for http auth longer than ~57 chars. (Vincent Ladeuil)

Description of the change

This fixes bug #1606203 caused by using a user/pass combination for http auth longer than ~57 chars.

To post a comment you must log in.
Revision history for this message
Colin Watson (cjwatson) :
review: Approve
Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

Vincent, What did the problem turn out to actually involve? Was it credentials "longer than ~57 chars" as the bug says or "creating a header value with an embedded '\n'" as a comment in the patch says? Or was it a combination of the two?

If it was the embedded '\n', how did that get in there?

Revision history for this message
Colin Watson (cjwatson) wrote :

str.encode('base64') is equivalent to base64.encodestring(str), which is a legacy interface (https://docs.python.org/2/library/base64.html#base64.encodestring) that returns "one or more lines of base64-encoded data". In particular, it splits the input string into 57-character chunks and encodes each one using binascii.b2a_base64, which adds a newline after each encoded chunk. Thus, if the input string is longer than 57 characters, str.encode('base64').strip() will end up containing embedded newlines.

base64.encodestring exists to encode strings according to RFC 1521's base64 Content-Transfer-Encoding, which has a 76-character limit for each encoded line. It does not make sense to use this for HTTP headers. For that, the base64 module's modern interface, which encodes according to RFC 3548, is more appropriate; and Vincent's patch switches to that.

Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

@Colin, thanks for the explanation.
@Vincent, thanks for the patch.
+1

review: Approve
Revision history for this message
Richard Wilbur (richard-wilbur) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/tests/test_http.py'
2--- bzrlib/tests/test_http.py 2016-02-01 18:06:32 +0000
3+++ bzrlib/tests/test_http.py 2016-09-09 13:24:23 +0000
4@@ -260,6 +260,16 @@
5 self.assertEqual('basic', scheme)
6 self.assertEqual('realm="Thou should not pass"', remainder)
7
8+ def test_build_basic_header_with_long_creds(self):
9+ handler = _urllib2_wrappers.BasicAuthHandler()
10+ user = 'user' * 10 # length 40
11+ password = 'password' * 5 # length 40
12+ header = handler.build_auth_header(
13+ dict(user=user, password=password), None)
14+ # https://bugs.launchpad.net/bzr/+bug/1606203 was caused by incorrectly
15+ # creating a header value with an embedded '\n'
16+ self.assertFalse('\n' in header)
17+
18 def test_basic_extract_realm(self):
19 scheme, remainder = self.parse_header(
20 'Basic realm="Thou should not pass"',
21
22=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
23--- bzrlib/transport/http/_urllib2_wrappers.py 2016-01-31 12:55:31 +0000
24+++ bzrlib/transport/http/_urllib2_wrappers.py 2016-09-09 13:24:23 +0000
25@@ -48,6 +48,7 @@
26 # actual code more or less do that, tests should be written to
27 # ensure that.
28
29+import base64
30 import errno
31 import httplib
32 import os
33@@ -1491,7 +1492,7 @@
34
35 def build_auth_header(self, auth, request):
36 raw = '%s:%s' % (auth['user'], auth['password'])
37- auth_header = 'Basic ' + raw.encode('base64').strip()
38+ auth_header = 'Basic ' + base64.b64encode(raw)
39 return auth_header
40
41 def extract_realm(self, header_value):

Subscribers

People subscribed via source and target branches