Bug 150346 – Do not sanitize user input for input[type=url]

• Component: Forms
• Status: RESOLVED FIXED

[Keith Rollin]

See Bug 148864. In addressing that bug, input[type=url] values are now sanitized when they are set. However, as Kent Tamura ([e-mail]) points out:

-----
The specification and the test ask to sanitize
 a) text set by |value| IDL attribute
 b) text set by |value| content attribute

but not 
 c) text set by user.

In WebKit and Blink, InputType::sanitizeValue is called in all of three cases for now.

The problem after your change is that selection API won't work well.
e.g.
  we have input[type=url].  A user puts "   http://apple.com/   " (23 characters).  JavaScript code runs:

  var length = input.value.length;  // 17 because whitepsaces are stripped.
  input.setSelectionRange(length, length);  // Move the caret at the end

This sets the caret on "o", not the end of the value.

So, I proposed we didn't strip whitespaces for user input.  input[type=email] doesn't have this issue because it doesn't support selection API.
-----

This proposal seems supported by the HTML spec, which says of input[type=url]: "The value attribute, if specified and not empty, must have a value that is a valid URL potentially surrounded by spaces that is also an absolute URL." Since text is sanitized when setting element.value or when initialized from the content attribute, the only way these potential spaces can appear is from user input.

As part of this, see also:

https://bugs.webkit.org/show_bug.cgi?id=148864
https://code.google.com/p/chromium/issues/detail?id=446108
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28401

[Radar WebKit Bug Importer]

<rdar://problem/23243240>

[Keith Rollin]

Created attachment 263959 [details]
Patch

[Build Bot]

Comment on attachment 263959 [details]
Patch

Attachment 263959 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/329157

New failing tests:
fast/forms/input-user-input-sanitization.html

[Build Bot]

Created attachment 263968 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Comment on attachment 263959 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263959&action=review

Looks like the mac-debug bot is crashing when running this test. That sounds like an insertion failure.

> Source/WebCore/html/TextFieldInputType.cpp:507
> +    // Do not sanitize input values if the element supports the selection interface:
> +    //      https://bugs.webkit.org/show_bug.cgi?id=150346
> +    // The selection interface is supported by BaseTextInputType, which is the parent
> +    // class for email, password, search, telephone, text, and url. However, email
> +    // explicitly turns off support, so only the latter five are affected by this check.

This comment seems too oblique. I think the point is that when we support the selection interface, the “sanitize” concept is not good. But is that part of HTML’s design or is it something we decided on our own? Where does the rationale for tying these two concepts together come from? I assume it has something to do with the fact that if you are able to use the selection interface then you require that offsets within the string you set match offsets you use with the selection API. So that explains why sanitization causes a problem, but it does not explain why it’s then optional. Why do we ever sanitize any field type, if there is sometimes a problem like this? We need a short comment, but it needs to focus on why the code does what it does. I don’t think the comment needs to mention that email is not affected; that’s important for testing this change, but it’s also something that could change if code elsewhere changed and so should be documented by the tests. The main purpose of this code would be to answer the question “why?” rather than saying what this code does. I can already read the code below and see that it says “don’t sanitize if the object supports the selection API”, so that is the thing that the comment does *not* need to say. What the comment needs to say is why.

> LayoutTests/fast/forms/input-user-input-sanitization-expected.txt:11
> +PASS input.value is result
> +PASS input.value is result
> +PASS input.value is result
> +PASS input.value is result
> +PASS input.value is result
> +PASS input.value is result

We try to structure our tests so the output makes it clear what the test is testing. That’s why it logs the expression. Thus we write test expressions that include the specifics of the test.

> LayoutTests/fast/forms/input-user-input-sanitization.html:38
> +function testOne(id, text, expected)
> +{
> +    input = focusAndType(id, text);
> +    result = expected || text;
> +    shouldBe('input.value', 'result');
> +}

We need to fix this so the shouldBe gets expressions that include what’s being tested and the actual expected result. Using it like this means we can’t see what’s being tested. Something like:

    shouldBe('focusAndType("' + id + '", "' + text + '").value', '"' + result + '"');

Not sure if exactly will work, but something like that.

[Keith Rollin]

Created attachment 264080 [details]
Patch

[Keith Rollin]

* Addressed crash.
* Reworded comment.
* Changed test as suggested.

[Chris Dumez]

Comment on attachment 264080 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264080&action=review

> Source/WebCore/html/HTMLInputElement.cpp:1046
> +    // values in order to retain parity between what's in the model and what's

The comment should indicate that it does not sanitize *user input* only.

> Source/WebCore/html/HTMLInputElement.cpp:1048
> +    if (!m_inputType->supportsSelectionAPI())

No if

> Source/WebCore/html/HTMLInputElement.cpp:1049
> +        ASSERT(value == sanitizeValue(value) || sanitizeValue(value).isEmpty());

ASSERT(m_inputType->supportsSelectionAPI() || value == sanitizeValue(value) || sanitizeValue(value).isEmpty());

> Source/WebCore/html/HTMLInputElement.cpp:1052
> +    // The assert macro above may also be simplified to: value == santizeValue(value)

TYPO!

> LayoutTests/fast/forms/input-user-input-sanitization.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

<!DOCTYPE html>

> LayoutTests/fast/forms/input-user-input-sanitization.html:5
> +<script src="resources/common.js"></script>

What is this used for?

> LayoutTests/fast/forms/input-user-input-sanitization.html:10
> +<p id="description"></p>

Not needed

> LayoutTests/fast/forms/input-user-input-sanitization.html:11
> +<div id="console"></div>

Not needed.

> LayoutTests/fast/forms/input-user-input-sanitization.html:27
> +    for (var i = 0, len = text.length; i < len; i++) {

no abbreviations in variable names please.

> LayoutTests/fast/forms/input-user-input-sanitization.html:36
> +    shouldBe('focusAndType("' + id + '", "' + text + '").value', '"' + result + '"');

Could use shouldBeEqualToString()

[Kent Tamura]

Comment on attachment 264080 [details]
Patch

The approach looks ok to me.

[Keith Rollin]

Created attachment 264133 [details]
Patch

[Keith Rollin]

Updated for Chris's comments.

[Keith Rollin]

> no abbreviations in variable names please.

Arg. I missed this one. I hope that's OK.

[Chris Dumez]

Comment on attachment 264133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264133&action=review

> LayoutTests/fast/forms/input-user-input-sanitization.html:1
> +<!DOCTYPE HTML>

FYI, we usually use lowercase html but it is case-insensitive as per the spec so fine:
https://html.spec.whatwg.org/#the-doctype

[Build Bot]

Comment on attachment 264133 [details]
Patch

Attachment 264133 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/344926

New failing tests:
animations/multiple-backgrounds.html

[Build Bot]

Created attachment 264140 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[WebKit Commit Bot]

Comment on attachment 264133 [details]
Patch

Clearing flags on attachment: 264133

Committed r191626: <http://trac.webkit.org/changeset/191626>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The test added with this change is failing on win:
https://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/54767

--- /home/buildbot/slave/win-release-tests/build/layout-test-results/fast/forms/input-user-input-sanitization-expected.txt
+++ /home/buildbot/slave/win-release-tests/build/layout-test-results/fast/forms/input-user-input-sanitization-actual.txt
@@ -3,12 +3,12 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS focusAndType("email", "   [e-mail]   ").value is "[e-mail]"
+FAIL focusAndType("email", "   [e-mail]   ").value should be [e-mail]. Was foobar2example.com.
 PASS focusAndType("password", "   foobar   ").value is "   foobar   "
 PASS focusAndType("search", "   foobar   ").value is "   foobar   "
 PASS focusAndType("telephone", "   123-456-7890   ").value is "   123-456-7890   "
 PASS focusAndType("text", "   foobar   ").value is "   foobar   "
-PASS focusAndType("url", "   https://foobar.example.com   ").value is "   https://foobar.example.com   "
+FAIL focusAndType("url", "   https://foobar.example.com   ").value should be    https://foobar.example.com   . Was    https;//foobar.example.com   .
 PASS successfullyParsed is true
 
 TEST COMPLETE

[Keith Rollin]

Created attachment 264247 [details]
Patch

[Chris Dumez]

Comment on attachment 264247 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264247&action=review

> Tools/ChangeLog:12
> +

Please explain what the behavior change is. Why is your approach different from isASCIIUpper()?

[Chris Dumez]

Comment on attachment 264247 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=264247&action=review

> Tools/DumpRenderTree/win/EventSender.cpp:514
> +        const char* shiftedUSCharacters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ~!@#$%^&*()_+{}|:\"<>?";

const char shiftedUSCharacters[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ~!@#$%^&*()_+{}|:\"<>?";

would be better since we don't modify it.

> Tools/DumpRenderTree/win/EventSender.cpp:517
> +        if (strchr(shiftedUSCharacters, charCode) != nullptr)

Also in WebKit, we usually don't compare to nullptr. This would be:
if (strchr(shiftedUSCharacters, charCode))

[Keith Rollin]

Created attachment 264259 [details]
Patch

[Chris Dumez]

Comment on attachment 264259 [details]
Patch

LGTM but I'll let Brent review since this is Windows.

[Brent Fulgham]

Comment on attachment 264259 [details]
Patch

r=me. Thank you for fixing this!

[WebKit Commit Bot]

Comment on attachment 264259 [details]
Patch

Clearing flags on attachment: 264259

Committed r191704: <http://trac.webkit.org/changeset/191704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.