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

RESOLVED FIXED in Firefox 17

Status

()

RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: martijn.martijn, Assigned: capella)

Tracking

({testcase})

Trunk
Firefox 17
ARM
Android
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments)

(Reporter)

Description

7 years ago
Created attachment 611862 [details]
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.

Comment 1

7 years ago
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]

Comment 2

7 years ago
(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.

Comment 4

7 years ago
Created attachment 616741 [details]
screenshot (on pc, not android)

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.

Comment 5

7 years ago
(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.
(Reporter)

Comment 6

7 years ago
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?

Comment 8

7 years ago
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.

Comment 10

7 years ago
(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

Comment 11

7 years ago
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!

Comment 12

7 years ago
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)

Updated

6 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 13

6 years ago
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.

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)
(Assignee)

Comment 14

6 years ago
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.

Comment 15

6 years ago
(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 16

6 years ago
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+
(Assignee)

Comment 17

6 years ago
Created attachment 653704 [details] [diff] [review]
Patch (v2)

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
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
You need to log in before you can comment on or make changes to this bug.