Logging in fails for an account created with non-ascii last-character password

RESOLVED FIXED

Status

Cloud Services
Server: Sync
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Mardak, Assigned: telliott)

Tracking

unspecified
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
Here's the body of the POST to create the account that successfully gets created:

2010-06-18 13:13:15     Net.Resource         TRACE      PUT Body: {"password":"testingé","email":"edtest3@agadak.net","captcha-challenge":"02A_dq3UyEAzpRCZzzGIqUK_RfDvLKuky
ELxWBPknA8RBJN9bvcttyXwMYcsMO2QMPegOJPXB4sC_E8U7zvmwOEuOQt6W86D0o7xlY2dRJVfc3AZqzaMXB29oHcKc6KY1HIYnCbIAyC4C7rKnf5PkmxMMlkTjZqE-wVFezu_cPVsZZwKNM8q9-ot_ll_DUJYrjS0CzSgacemFRoaNZyqu-wtvhGb7G39oYokUykDhikMzjVuzbRtiC_ylQTTmqBBEXxOacZNMb-EHNZENLqd9hqe2sIP8B","captcha-response":"giamo woodcut"}
2010-06-18 13:13:15	Net.Resource         DEBUG	PUT success 200 https://auth.services.mozilla.com/user/1.0/edtest3

Auth fails for verifying login:
2010-06-18 13:17:03     Net.Resource         TRACE      HTTP Header Authorization: Basic ZWR0ZXN0Mzp0ZXN0aW5n6Q==
2010-06-18 13:17:03     Net.Resource         DEBUG      GET fail 401 https://phx-sync129.services.mozilla.com/1.0/edtest3/info/collections

atob('ZWR0ZXN0Mzp0ZXN0aW5n6Q==') == edtest3:testingé


Perhaps related is that using the reset-password interface, using a password with non-ascii as the last character fails but using the client's change-password interface, things works fine.
(Assignee)

Comment 1

8 years ago
Update here. We've definitely found a problem.

We have the following function for fixing UTF-8

if(mb_detect_encoding($string, 'UTF-8,ISO-8859-1') == 'UTF-8')
	return $string;
else
	return utf8_encode($string);

On the sync server, if I use the password testingé, mb_detect encoding claims it is utf8 and returns testing\xe9

If I change the line to 

if(mb_detect_encoding($string . " ", 'UTF-8,ISO-8859-1') == 'UTF-8')

it evaluates to false, at which point utf8_encode changes the string to testing\xc3\xa9

(I also note that we're not doing any utf8 detection on the password in the reg server, which we should probably be doing)

Les, any thoughts?
(Assignee)

Comment 2

8 years ago
More details:

[22-Jun-2010 15:57:20] Sync received password: testing�
[22-Jun-2010 15:57:20] Sync base string encoding: UTF-8
[22-Jun-2010 15:57:20] Forced utf8 encode: testingé

So the first is registering as a utf8 string, but it's not the same string as the one when run through utf8 encode :P
(Assignee)

Comment 3

8 years ago
And, if it's not at the end:

[22-Jun-2010 16:03:45] Sync received password: testingéf
[22-Jun-2010 16:03:45] Sync SSHA: {SSHA-256}5hgqdHMAPBY9oU5QVq3MuPmYP0QwBOrplIcfYu1sLqs/em3FNeYwLA==

wtf?

Updated

8 years ago
Flags: blocking-fx-sync1.4?

Updated

8 years ago
Blocks: 561880

Comment 4

8 years ago
(In reply to comment #3)
> And, if it's not at the end:
> 
> [22-Jun-2010 16:03:45] Sync received password: testingéf
> [22-Jun-2010 16:03:45] Sync SSHA:
> {SSHA-256}5hgqdHMAPBY9oU5QVq3MuPmYP0QwBOrplIcfYu1sLqs/em3FNeYwLA==
> 
> wtf?

Is there some difference in the PHP versions between SJC and PHX that could be causing this?
(In reply to comment #4)

> Is there some difference in the PHP versions between SJC and PHX that could be
> causing this?

Not sure I understand how that might come into play, but...

[root@wp-web01 ~]# php -v
PHP 5.2.9 (cli) (built: Feb  1 2010 15:52:33) 
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.2.0, Copyright (c) 1998-2009 Zend Technologies
    with eAccelerator v0.9.5.2, Copyright (c) 2004-2006 eAccelerator, by eAccelerator

[root@pm-weave01 ~]# php -v
PHP 5.2.9 (cli) (built: Jun 23 2009 14:49:15) 
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.2.0, Copyright (c) 1998-2009 Zend Technologies
(In reply to comment #1)

> On the sync server, if I use the password testingé, mb_detect encoding claims
> it is utf8 and returns testing\xe9

Looks like \xe9 is the ISO-8859-1 encoding of "é"

> If I change the line to 
> if(mb_detect_encoding($string . " ", 'UTF-8,ISO-8859-1') == 'UTF-8')
> it evaluates to false, at which point utf8_encode changes the string to
> testing\xc3\xa9

And \xc3\xa9 is the UTF-8 encoding of "é", so the bugfix here is actually to append a space in the detection call, as above.

This is admittedly hacky, but it looks like the PHP function mb_detect_encoding() is itself buggy:

http://www.php.net/manual/en/function.mb-detect-encoding.php#81936

> (I also note that we're not doing any utf8 detection on the password in the
> reg server, which we should probably be doing)

Can you point me to where you see the reg server is missing UTF8 munging on the password? It's there for account creation / modification via get_http_body() which calls fix_utf8_encoding() 

> Les, any thoughts?

Ultimately, I think need to give up on UTF-8 detection, because in reality there's no such thing - there's only UTF-8 educated guessing, and it's fallible (and buggy in the case of PHP).

Really, a client should send a header like Content-Type: application/json;charset=iso-8859-1 or Content-Type: application/json;charset=utf-8 to explicitly declare how to interpret the request.
(In reply to comment #5)
> Not sure I understand how that might come into play, but...

FWIW: Because point releases of PHP sometime offer unpleasant surprises. (eg. changes in method signatures, "fixed" bugs, and new bugs)

But, that doesn't appear to be the case here
Attachment #453394 - Flags: review? → review?(telliott)
(Assignee)

Comment 10

8 years ago
Comment on attachment 453394 [details] [diff] [review]
Hacky bugfix for buggy mb_detect_encoding

I was afraid of this :)
Attachment #453394 - Flags: review?(telliott) → review+
https://stage-auth.services.mozilla.com (really dm-weave-stage-reg01 and -adm01) have been updated to tip.

Comment 13

8 years ago
So, we never pushed this to prod? Can we please push this as soon as possible? And why did we resolve it fixed before it was actually deployed in production?
Process should be FIXED>push to stage>VERIFIED>push to prod.

Never VERIFIED, so never pushed.

We did a push to reg and sreg yesterday. There are a LOT of changes on sync since we did a push there, so I'm not so gung-ho to just grab tip.

Will look at diffs and figure it out now.
We don't have a production branch, so I pushed 7d3551d57958 to production.

Someone VERIFY for me?

Updated

7 years ago
Flags: in-testsuite?

Updated

7 years ago
Flags: blocking-fx-sync1.4?
You need to log in before you can comment on or make changes to this bug.