Closed Bug 741858 Opened 9 years ago Closed 8 years ago

Change stored password doorhanger doesn't handle large password string very well

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 17

People

(Reporter: martijn.martijn, Assigned: capella)

References

()

Details

(Keywords: testcase, Whiteboard: [good first bug][mentor=margaret][lang=js])

Attachments

(4 files)

Attached file testcase
See testcase, steps to reproduce:
- Tap on the Submit button, the remember password doorhanger pops up
- Tap on 'Remember'
- Tap on the Android back button
- Tap on the password input, type some text in there
- Tap on the Submit button, the change stored password doorhanger pops up

Expected result:
- The text input value that is shown is truncated just like in the remember password doorhanger popup

Actual result:
- The text input value that is shown is not truncated.

Tested in today's trunk build on the Samsung Galaxy Nexus.
This is a also a toolkit login manager bug. I'll file another bug for that. Good find!

For the save login dialog, we call _sanitizeUsername:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/LoginManagerPrompter.js#236

but we don't do that for the change login dialog:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/LoginManagerPrompter.js#374
Whiteboard: [good first bug][mentor=margaret][lang=js]
(In reply to Margaret Leibovic [:margaret] from comment #1)
> This is a also a toolkit login manager bug. I'll file another bug for that.

Filed bug 742396.
Martijn, does this affect the password/login if it gets cut off?  Or is this only a visual effect?  Trying to understand how critical this bug is for triage.
there was a similar bug on firefox (for ubuntu) and that's how it looked like. i believe it should look like that on fennec too.
(In reply to Naoki Hirata :nhirata from comment #3)
> Martijn, does this affect the password/login if it gets cut off?  Or is this
> only a visual effect?  Trying to understand how critical this bug is for
> triage.

Yes, this just looks bad. It makes it so that you potentially have to scroll the doorhanger notification to get to the buttons, but you would need a ridiculously long username.
This is not a bug that users would encounter, I think.
I have an incredibly long user name of nhirata.bugzilla@gmail.com  would that count?  :)  Just want to make sure that there's no login issues due to this.  I guess a better question is what's the upper limit of characters?
Hi. I'm a begginer here but i wanna work on this bug.
I'm confused if it's have solved or not. If not, where i look initially(in code)?
Hey Raphael,

This bug is still open (it's marked NEW). I would most definitely have a look at the LoginManagerPrompter.js code that Margaret linked to in comment #1 as the place for change. If you have questions, feel free to hop on IRC @ irc.mozilla.org #mobile and talk to us.
(In reply to Raphael "Gans" from comment #8)
> Hi. I'm a begginer here but i wanna work on this bug.
> I'm confused if it's have solved or not. If not, where i look initially(in
> code)?

Hi Raphael! (Or do you prefer Gans?)

As Aaron mentioned, I linked to the relevant code in comment 1. You basically want to do the exact same thing to the mobile version of LoginManagerPrompter.js that Veeraya did to fix bug 742396 for desktop.

Do you have a mobile build environment set up? As Aaron said, you can find us on IRC, but build instructions are also on our wiki here: https://wiki.mozilla.org/Mobile/Fennec/Android
Thanks for feedback! I prefer Gans, it's how my friends call me. ^^
I look at the other bug(bug 742396) and "LoginManagePrompter.js". I modified the code for sanitize the new password. I believe it's ok but I'm having troubles about the mobile build environment.

My (small) experience with android's environment is only with Eclipse/ADT Plugin. Because some problems with my Internet's connection, the download of NDK are impossible in the moment. May i execute the code on Eclipse? Or upload the patch without testing it? The last option isn't good but i'm only question about to understand the process here, at Bugzilla. ;)

Oh, i forgot to say before: I'm sorry about my terrible english. I'm brazilian and don't speak english very well. =)

Thanks for answers!
Thanks for feedback! I prefer Gans, it's how my friends call me. ^^
I look at the other bug(bug 742396) and "LoginManagePrompter.js". I modified the code for sanitize the new password. I believe it's ok but I'm having troubles about the mobile build environment.

My (small) experience with android's environment is only with Eclipse/ADT Plugin. Because some problems with my Internet's connection, the download of NDK are impossible in the moment. May i execute the code on Eclipse? Or upload the patch without testing it? The last option isn't good but i'm only question about to understand the process here, at Bugzilla. ;)

Oh, i forgot to say before: I'm sorry about my terrible english. I'm brazilian and don't speak english very well. =)

Thanks for answers!
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch Patch (v1)Splinter Review
This small change tests out and works nice on my Galaxy device.

fyi, the DIFF will show that I modifed both the _showChangeLoginNotification() and the _showChangeLoginDialog() member functions, as the _sanitizeUsername() call was currently in place for both the _showSaveLoginNotification() and the _showSaveLoginDialog() functions.

I mention this because though the changes look trivial, my testing may have only exercised the changed portion in the notification doorhanger routine.
Attachment #652335 - Flags: review?(margaret.leibovic)
More fyi, I hacked the code to force display of the _showChangeLoginDialog() routine and it works also, sort of. The sanitized 30-char name + "..." shows up but takes two lines in the dialog.
(In reply to Mark Capella [:capella] from comment #13)
> Created attachment 652335 [details] [diff] [review]
> Patch (v1)
> 
> This small change tests out and works nice on my Galaxy device.
> 
> fyi, the DIFF will show that I modifed both the
> _showChangeLoginNotification() and the _showChangeLoginDialog() member
> functions, as the _sanitizeUsername() call was currently in place for both
> the _showSaveLoginNotification() and the _showSaveLoginDialog() functions.

Nice catch! That's something I didn't notice when I filed bug 742396, so that problem still exists in the toolkit version of _showChangeLoginDialog (although I'm not sure if there are still toolkit apps that go through that code path).
Comment on attachment 652335 [details] [diff] [review]
Patch (v1)

This looks good, but I have a few style nits to adhere to our JS style conventions.

>diff --git a/mobile/android/components/LoginManagerPrompter.js b/mobile/android/components/LoginManagerPrompter.js

>@@ -329,20 +329,22 @@ LoginManagerPrompter.prototype = {
>+            var displayUser = this._sanitizeUsername(aOldLogin.username);

s/var/let

>             notificationText  = this._getLocalizedString(
>                                           "passwordChangeText",
>-                                          [aOldLogin.username]);
>+                                          [displayUser]);
>+        }
>         else

Put the else on the same line as the brace, and surround the else block in braces as well.

>@@ -387,20 +389,22 @@ LoginManagerPrompter.prototype = {

>+            var displayUser = this._sanitizeUsername(aOldLogin.username);
>             dialogText  = this._getLocalizedString(
>                                     "passwordChangeText",
>-                                    [aOldLogin.username]);
>+                                    [displayUser]);
>+        }
>         else
>             dialogText  = this._getLocalizedString(
>                                     "passwordChangeTextNoUser");

Same comments apply to this hunk as well.
Attachment #652335 - Flags: review?(margaret.leibovic) → review+
Attached patch Patch (v2)Splinter Review
As corrected, and pushing to TRY:
https://tbpl.mozilla.org/?tree=Try&rev=46901461801b
https://hg.mozilla.org/mozilla-central/rev/5228384d4da8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.