415943 - Test that password field text is not readable through accessibility APIs

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

The new text getters should not expose the actual text in a password field -- just *'s.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Which (sub)interface is exposing the actual text?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> Which (sub)interface is exposing the actual text?

nsIFrame::GetRenderedText. In the meantime accessible value exposes *** based on role. Since nsIFrame::GetRenderedText is used in a11y then it's possible we expose *** in nsIAccessibleText methods. But I'm not sure. Example is appreciated.

Comment 3

[Marco Zehe (:MarcoZ)]

I just looked at a login form with AccProbe on Windows, and IAccessibleText::Text gives me **** for a 4 letter password field. I can turn this into a mochitest and attach it to this bug, basically turning it into a "test that we expose passwords as *" one. What do you think?

Comment 4

[alexander :surkov (:asurkov)]

Sounds great. Thank you.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Mass un-assigning bugs assigned to Aaron.

Comment 6

[alexander :surkov (:asurkov)]

Marco, would be great if you finish mochitest ;)

Comment 7

[Marco Zehe (:MarcoZ)]

Taking!

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Taking!

thank you!

Comment 9

[Marco Zehe (:MarcoZ)]

Attachment: Mochitest for getText for both a regular input and a password input

I believe I need some help with the password characters I should be testing for. The log file shows I get a char of decimal value 207, or 0xCF, but when I put that in, as in the patch, it claims I am supplying a character 0xFFFD. The other tests pass. Surkov, David, any ideas?

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Ehsan tells me we get the character from the platform, so probably a safer test here is to simply test that the password itself is not exposed. The actual bogus characters aren't that important.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> Ehsan tells me we get the character from the platform, so probably a safer test
> here is to simply test that the password itself is not exposed. The actual
> bogus characters aren't that important.

sounds reasonable change

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 518489 [details] [diff] [review]
Mochitest for getText for both a regular input and a password input


>+  <title>nsIAccessibleText getText related function tests for text inputs</title>

for password fields?

>+      ////////////////////////////////////////////////////////////////////////
>+      // characterCount and getText for password field
>+
>+      IDs = [ "password" ];
>+      testCharacterCount(IDs, 4);
>+      testText(IDs, 0, 4, "ÃÃÃÃ");

it's ok to go with David's suggestion here

>+  <a target="_blank"
>+     title="Fix getText"
>+     href="https://bugzilla.mozilla.org/show_bug.cgi?id=630001">Mozilla Bug 630001, part3</a>

please update this

Comment 13

[Marco Zehe (:MarcoZ)]

Attachment: Updated Mochitest with comments

I added a function to text.js that tests if the given text is anything but the text passed in the last parameter. I also added documentation for both the testText and testPasswordText functions. Rerequesting review just in case...

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 518667 [details] [diff] [review]
Updated Mochitest with comments

>diff --git a/accessible/tests/mochitest/text.js b/accessible/tests/mochitest/text.js
>--- a/accessible/tests/mochitest/text.js
>+++ b/accessible/tests/mochitest/text.js
>@@ -21,16 +21,24 @@ function testCharacterCount(aIDs, aCount
> {
>   for (var i = 0; i < aIDs.length; i++) {
>     var textacc = getAccessible(aIDs[i], [nsIAccessibleText]);
>     is(textacc.characterCount, aCount,
>        "Wrong character count for " + prettyName(aIDs[i]));
>   }
> }
> 
>+/**
>+ * Test text between two given offsets
>+ *
>+ * @param aIDs  [in] an array of accessible IDs to test
>+ * @param aStartOffset  [in] the start offset within the text to test
>+ * @param aEndOffset  [in] the end offset up to which the text is tested
>+ * @param aText  [in] the expected result from the test
>+ */

nit: please align all [in] description

 * @param aIDs          [in] an array of accessible IDs to test
 * @param aStartOffset  [in] the start offset within the text to test
 * @param aEndOffset    [in] the end offset up to which the text is tested
 * @param aText         [in] the expected result from the test


> /**
>+ * Test password text between two given offsets
>+ *
>+ * @param aIDs  [in] an array of accessible IDs to test
>+ * @param aStartOffset  [in] the start offset within the text to test
>+ * @param aEndOffset  [in] the end offset up to which the text is tested
>+ * @param aText  [in] the expected result from the test

the same

Comment 15

[Marco Zehe (:MarcoZ)]

Landed test-only patch on mozilla-central: http://hg.mozilla.org/mozilla-central/rev/c8bef0022a62