884332 - Limit <input type='email'>'s value to have labels of 63 chars max

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

Following bug 854812 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=21617 we should not limit the <input type='email'> value to have 63 characters for each label.

Don't mind the spec that shows 61 charecters in the regexp ;)

Comment 1

[Mounir Lamouri (:mounir)]

(In reply to Mounir Lamouri (:mounir) from comment #0)
> Don't mind the spec that shows 61 charecters in the regexp ;)

Forget that, there is no typo here.

Comment 2

[Mounir Lamouri (:mounir)]

To attack this bug, you should look at ConvertUTF8toACE() usage in HTMLInputElement::IsValidEmailAddress(). Right now, we just don't puny-encode if that call fails but it would actually fail if there is a label that doesn't match the requested size. Ideally, the different failure case should be analysed to see if we should always mark the element as invalid if the conversion fails.

Comment 3

[Biru Mohanathas [:poiru]]

Attachment: Patch v1

Hi. I've attached a patch to limit labels to 63 chars.

(In reply to Mounir Lamouri (:mounir) from comment #2)
> To attack this bug, you should look at ConvertUTF8toACE() usage in
> HTMLInputElement::IsValidEmailAddress(). Right now, we just don't
> puny-encode if that call fails but it would actually fail if there is a
> label that doesn't match the requested size. Ideally, the different failure
> case should be analysed to see if we should always mark the element as
> invalid if the conversion fails.

I'm not quite sure how best to handle UTF-8 input if ConvertUTF8toACE() fails. I added tests with UTF-8 input, but they assume that ConvertUTF8toACE() succeeds.

Comment 4

[Mounir Lamouri (:mounir)]

Comment on attachment 766466 [details] [diff] [review]
Patch v1

Review of attachment 766466 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for this patch Birunthan. Unfortunately, as said in comment 2, ConvertUTF8toACE() already has that kind of checks, see https://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIDNService.cpp#600 We should make sure we use this instead of re-checking that in HTMLInputElement::IsValidEmailAddress().

Let me know if you need any other pointer.

Comment 5

[Biru Mohanathas [:poiru]]

Attachment: Part 1: Use puny-encoding only for the domain part in HTMLInputElement::IsValidEmailAddress (v1)

(In reply to Mounir Lamouri (:mounir) from comment #4)
> Thank you for this patch Birunthan. Unfortunately, as said in comment 2,
> ConvertUTF8toACE() already has that kind of checks, see
> https://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIDNService.
> cpp#600 We should make sure we use this instead of re-checking that in
> HTMLInputElement::IsValidEmailAddress().

Whoops, I misunderstood what you meant. While taking another look, I noticed that we do not currently conform to the HTML spec in hadling the local part of e-mail addresses. In accorance with the HTML spec, this change excludes the local part when puny-encoding and subsequently disallows addresses such as "föö@foo.com". International characters above U+007F are permitted by RFC 6531, but the HTML spec does not currently take this into account.

I will attach the other parts of the patch after we've decided if we want to go through with this part.

Comment 6

[Mounir Lamouri (:mounir)]

Comment on attachment 775202 [details] [diff] [review]
Part 1: Use puny-encoding only for the domain part in HTMLInputElement::IsValidEmailAddress (v1)

Review of attachment 775202 [details] [diff] [review]:
-----------------------------------------------------------------

This patch doesn't seem to be doing the right thing. What we want is to limit sub-domains (labels) to 63 characters, not forbidding UTF-8 characters in the email's username. I am not sure why you want to remove this feature but if we have to do that, it should definitely be in another bug, it is not the same issue.

Feel free to ping me on IRC if you want to discuss this.

Comment 7

[Biru Mohanathas [:poiru]]

As discussed in IRC, the problem is that ConvertUTF8toACE() treats "user@domain" as a single label so it may exceed the label length. For example "[e-mail]" would be deemed invalid.

Anne, could you clarify if this is intentional? If so, we can implement a workaround for this specific purpose.

(In reply to Mounir Lamouri (:mounir) from comment #6)
> This patch doesn't seem to be doing the right thing. What we want is to
> limit sub-domains (labels) to 63 characters, not forbidding UTF-8 characters
> in the email's username.

Indeed. I seem to have suffered from a brain fart and forgot we were puny-encoding the username.

Comment 8

[Anne (:annevk)]

The labels the domain consist of are each restricted to 63 characters. The domain is what comes after the at-sign. Labels of a domain are its individual dot-separated bits (dot can be another code point for IDNs).

Comment 9

[Biru Mohanathas [:poiru]]

Attachment: Limit domain labels to 63 chars in HTMLInputElement::IsValidEmailAddress

Updated patch.

(In reply to Anne (:annevk) from comment #8)
> The labels the domain consist of are each restricted to 63 characters. The
> domain is what comes after the at-sign. Labels of a domain are its
> individual dot-separated bits (dot can be another code point for IDNs).

Thanks!

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 781011 [details] [diff] [review]
Limit domain labels to 63 chars in HTMLInputElement::IsValidEmailAddress

Review of attachment 781011 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments fixed. Do not forget to file the bug and put the bug number in the TODO.

::: content/html/content/src/HTMLInputElement.cpp
@@ +5785,5 @@
> +    return false;
> +  }
> +
> +  uint32_t atPos = value.FindChar('@');
> +  if (atPos <= 0) {

Can you add a comment saying that an email address can't start nor end with an '@' and the '@' is mandatory so it needs one?
You should also change the check to:
  if (atPos <= 0 || atPos == length - 1) {

Also, given that check, I think you can remove those lines:
  // If there is no domain name, that's not a valid email address.
  if (++i >= length) {
    return false;
  }

@@ +5799,5 @@
>      bool ace;
> +    if (NS_SUCCEEDED(idnSrv->IsACE(username, &ace)) && !ace) {
> +      nsAutoCString usernameACE;
> +      if (NS_SUCCEEDED(idnSrv->ConvertUTF8toACE(username, usernameACE))) {
> +        value.Replace(0, username.Length(), usernameACE);

FWIW, you could as well use atPos instead of username.Length().

@@ +5804,5 @@
> +        atPos = usernameACE.Length();
> +      } else {
> +        // FIXME: Usernames longer than 63 chars are not converted by
> +        // ConvertUTF8toACE(). For now, continue on even though the conversion
> +        // failed.

Sadly, email standards accept 64 characters ;)

Please, don't have an empty |else| statement. Also, don't add FIXME but add a TODO and file a bug so you can put the bug number in the comments.

@@ +5814,5 @@
> +      nsAutoCString domainACE;
> +      if (NS_SUCCEEDED(idnSrv->ConvertUTF8toACE(domain, domainACE))) {
> +        value.Replace(atPos + 1, domain.Length(), domainACE);
> +      } else {
> +        return false;

I would do:
  if (NS_FAILED(....) {
    return false;
  }
  value.Replace(...);

Comment 11

[Biru Mohanathas [:poiru]]

Attachment: Limit domain labels to 63 chars in HTMLInputElement::IsValidEmailAddress

Addresses comment #10.

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 785521 [details] [diff] [review]
Limit domain labels to 63 chars in HTMLInputElement::IsValidEmailAddress

Review of attachment 785521 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks :)

::: content/html/content/src/HTMLInputElement.cpp
@@ +5761,5 @@
> +  if (length == 0 || value[length - 1] == '.' || value[length - 1] == '-') {
> +    return false;
> +  }
> +
> +  uint32_t atPos = (uint32_t)value.FindChar('@');

Keep this a signed int.

Comment 13

[Biru Mohanathas [:poiru]]

(In reply to Mounir Lamouri (:mounir) from comment #12)
> Comment on attachment 785521 [details] [diff] [review]
> Limit domain labels to 63 chars in HTMLInputElement::IsValidEmailAddress
> 
> Review of attachment 785521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks :)
> 
> ::: content/html/content/src/HTMLInputElement.cpp
> @@ +5761,5 @@
> > +  if (length == 0 || value[length - 1] == '.' || value[length - 1] == '-') {
> > +    return false;
> > +  }
> > +
> > +  uint32_t atPos = (uint32_t)value.FindChar('@');
> 
> Keep this a signed int.

As discussed on IRC: FindChar returns a int32_t so the greatest valid index can be 2147483647. (uint32_t)-1 would be 4294967295 and cannot therefore be a valid index. Also, if we change |atPos| to a int32_t, we'd have to do something about `atPos = usernameACE.Length()` and `for (; i < atPos; ++i) {` due to the int/uint mismatch.

Should I change it to a int32_t or do you think it would be OK as is?

Comment 14

[Mounir Lamouri (:mounir)]

(In reply to Birunthan Mohanathas [:poiru] from comment #13)
> (In reply to Mounir Lamouri (:mounir) from comment #12)
> > Comment on attachment 785521 [details] [diff] [review]
> > Limit domain labels to 63 chars in HTMLInputElement::IsValidEmailAddress
> > 
> > Review of attachment 785521 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks :)
> > 
> > ::: content/html/content/src/HTMLInputElement.cpp
> > @@ +5761,5 @@
> > > +  if (length == 0 || value[length - 1] == '.' || value[length - 1] == '-') {
> > > +    return false;
> > > +  }
> > > +
> > > +  uint32_t atPos = (uint32_t)value.FindChar('@');
> > 
> > Keep this a signed int.
> 
> As discussed on IRC: FindChar returns a int32_t so the greatest valid index
> can be 2147483647. (uint32_t)-1 would be 4294967295 and cannot therefore be
> a valid index. Also, if we change |atPos| to a int32_t, we'd have to do
> something about `atPos = usernameACE.Length()` and `for (; i < atPos; ++i)
> {` due to the int/uint mismatch.
> 
> Should I change it to a int32_t or do you think it would be OK as is?

I guess you can keep it as is.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/2de45d78653d

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-1 asserts.
https://hg.mozilla.org/integration/b2g-inbound/rev/1a7502e440e3

https://tbpl.mozilla.org/php/getParsedLog.php?id=26537755&tree=B2g-Inbound

Since it's kind of buried, the assert is:

07:07:50     INFO -  113036 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | Element should be valid
07:07:50     INFO -  113037 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | Element should be valid
07:07:50     INFO -  113038 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | The invalid event should not have been thrown
07:07:50     INFO -  113039 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | Validation message should be the empty string
07:07:50     INFO -  113040 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | :valid pseudo-class should apply
07:07:50     INFO -  [Parent 1153] ###!!! ASSERTION: input too big, the result truncated: 'Error', file ../../../netwerk/dns/nsIDNService.cpp, line 428
07:07:50     INFO -  nsIDNService::stringPrepAndACE(nsAString_internal const&, nsACString_internal&, bool, bool) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/netwerk/dns/../../../netwerk/dns/nsIDNService.cpp:619]
07:07:50     INFO -  nsIDNService::UTF8toACE(nsACString_internal const&, nsACString_internal&, bool, bool) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/netwerk/dns/../../../netwerk/dns/nsIDNService.cpp:206]
07:07:50     INFO -  mozilla::dom::HTMLInputElement::IsValidEmailAddress(nsAString_internal const&) [content/html/content/src/HTMLInputElement.cpp:5768]
07:07:50     INFO -  mozilla::dom::HTMLInputElement::HasTypeMismatch() const [content/html/content/src/HTMLInputElement.cpp:5277]
07:07:50     INFO -  mozilla::dom::HTMLInputElement::UpdateAllValidityStates(bool) [content/html/content/src/HTMLInputElement.cpp:5454]
07:07:50     INFO -  _ZThn272_N7mozilla3dom16HTMLInputElement14OnValueChangedEb [obj-firefox/dist/include/nsINode.h:1323]
07:07:50     INFO -  nsTextEditorState::SetValue(nsAString_internal const&, bool, bool) [content/html/content/src/nsTextEditorState.cpp:1925]
07:07:50     INFO -  mozilla::dom::HTMLInputElement::SetValueInternal(nsAString_internal const&, bool, bool) [content/html/content/src/HTMLInputElement.cpp:2212]
07:07:50     INFO -  mozilla::dom::HTMLInputElement::SetValue(nsAString_internal const&, mozilla::ErrorResult&) [content/html/content/src/HTMLInputElement.cpp:1400]
07:07:50     INFO -  mozilla::dom::HTMLInputElementBinding::set_value [obj-firefox/dom/bindings/HTMLInputElementBinding.cpp:1625]
07:07:50     INFO -  mozilla::dom::HTMLInputElementBinding::genericSetter [obj-firefox/dom/bindings/HTMLInputElementBinding.cpp:3066]
07:07:50     INFO -  js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/js/src/../../../js/src/jscntxtinlines.h:218]
07:07:50     INFO -  js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/js/src/../../../js/src/vm/Interpreter.cpp:482]
07:07:50     INFO -  js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/js/src/../../../js/src/vm/Interpreter.cpp:539]
07:07:50     INFO -  js::InvokeGetterOrSetter(JSContext*, JSObject*, JS::Value, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/js/src/../../../js/src/vm/Interpreter.cpp:610]
07:07:50     INFO -  js::Shape::set(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, bool, JS::MutableHandle<JS::Value>) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/js/src/../../../js/src/vm/Shape-inl.h:275]
07:07:50     INFO -  js::baseops::SetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<jsid>, unsigned int, JS::MutableHandle<JS::Value>, bool) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/js/src/../../../js/src/jsobj.cpp:4626]
07:07:50     INFO -  bool js::SetProperty<false>(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Value const&) [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/js/src/../../../js/src/jsobj.h:859]
07:07:50     INFO -  js::ion::DoSetPropFallback [/builds/slave/m-in-osx64-d-00000000000000000/build/obj-firefox/js/src/../../../js/src/jit/BaselineIC.cpp:6335]
07:07:50     INFO -  [Parent 1153] WARNING: IDN node too large: file ../../../netwerk/dns/nsIDNService.cpp, line 633
07:07:50     INFO -  [Parent 1153] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file ../../../netwerk/dns/nsIDNService.cpp, line 208
07:07:50     INFO -  113041 INFO TEST-KNOWN-FAIL | /tests/content/html/content/test/forms/test_input_email.html | value should be valid - got false, expected true
07:07:50     INFO -  113042 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | Element should not suffer from type mismatch (with value='foo@thislabelisexactly63characterssssssssssssssssssssssssssssssssss')
07:07:50     INFO -  113043 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | Element should be valid
07:07:50     INFO -  113044 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | Element should be valid
07:07:50     INFO -  113045 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | The invalid event should not have been thrown
07:07:50     INFO -  113046 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | Validation message should be the empty string
07:07:50     INFO -  113047 INFO TEST-PASS | /tests/content/html/content/test/forms/test_input_email.html | :valid pseudo-class should apply

Comment 17

[Mounir Lamouri (:mounir)]

Birunthan, are you able to reproduce this locally?

Comment 18

[Biru Mohanathas [:poiru]]

(In reply to Mounir Lamouri (:mounir) from comment #17)
> Birunthan, are you able to reproduce this locally?

Sadly, no. I might be missing some build option (right now, I only have --enable-debug and --disable-optimize).

Comment 19

[Mounir Lamouri (:mounir)]

(In reply to Birunthan Mohanathas [:poiru] from comment #18)
> (In reply to Mounir Lamouri (:mounir) from comment #17)
> > Birunthan, are you able to reproduce this locally?
> 
> Sadly, no. I might be missing some build option (right now, I only have
> --enable-debug and --disable-optimize).

You need to add --enable-tests and actually run the tests, see:
https://developer.mozilla.org/en/docs/Mochitest

Comment 20

[Biru Mohanathas [:poiru]]

(In reply to Mounir Lamouri (:mounir) from comment #19)
> You need to add --enable-tests and actually run the tests, see:
> https://developer.mozilla.org/en/docs/Mochitest

I meant that I did run the tests, but I'm not sure if I'm missing a build option needed to trigger the NS_ERROR assertion (AFAIK, --enable-debug should suffice). test_input_email.html passes all tests here.

Comment 21

[Mounir Lamouri (:mounir)]

(In reply to Birunthan Mohanathas [:poiru] from comment #20)
> (In reply to Mounir Lamouri (:mounir) from comment #19)
> > You need to add --enable-tests and actually run the tests, see:
> > https://developer.mozilla.org/en/docs/Mochitest
> 
> I meant that I did run the tests, but I'm not sure if I'm missing a build
> option needed to trigger the NS_ERROR assertion (AFAIK, --enable-debug
> should suffice). test_input_email.html passes all tests here.

I just tried locally and I am able to reproduce the assert. However, the test shows as green so maybe you also have the assert but did not notice. Look at the log when you run the test. Otherwise, run the tests from content/html/content/test/forms/ instead of just your individual test. It marks the test as failing with the assertion in that case.

Comment 22

[Mounir Lamouri (:mounir)]

I've opened bug 906623 about the assert issue.

Comment 23

[Biru Mohanathas [:poiru]]

Attachment: Part 1: Check and handle buffer truncation without asserting when converting UTF-16 to UCS-4 in nsIDNService.cpp

(In reply to Mounir Lamouri (:mounir) from comment #21)
> I just tried locally and I am able to reproduce the assert. However, the
> test shows as green so maybe you also have the assert but did not notice.
> Look at the log when you run the test. Otherwise, run the tests from
> content/html/content/test/forms/ instead of just your individual test. It
> marks the test as failing with the assertion in that case.

Ah, I see. Thanks for looking into it.

I've attached a patch to remove the assert and replace it with error propagation.

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=c10f2538e8a2

Comment 24

[Biru Mohanathas [:poiru]]

Attachment: Part 2: Limit domain labels to 63 chars in HTMLInputElement::IsValidEmailAddress

Updated commit summary.

Comment 25

[Mounir Lamouri (:mounir)]

Comment on attachment 793036 [details] [diff] [review]
Part 1: Check and handle buffer truncation without asserting when converting UTF-16 to UCS-4 in nsIDNService.cpp

Review of attachment 793036 [details] [diff] [review]:
-----------------------------------------------------------------

I am not sure if this is the right thing to do but I am not a peer for this file so not a proper reviewer anyway.

Comment 26

[Mounir Lamouri (:mounir)]

Comment on attachment 793037 [details] [diff] [review]
Part 2: Limit domain labels to 63 chars in HTMLInputElement::IsValidEmailAddress

No need to ask a review if you only change the commit summary ;)

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 793036 [details] [diff] [review]
Part 1: Check and handle buffer truncation without asserting when converting UTF-16 to UCS-4 in nsIDNService.cpp

Review of attachment 793036 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/dns/nsIDNService.cpp
@@ +432,2 @@
>        out[outBufLen-1] = (uint32_t)'\0';
>        *outLen = outBufLen-1;

nit: If we return NS_ERROR_FAILURE then we don't need to change out or outLen, right? The caller isn't allowed to look at the value anyway, so we can just remove these three lines.

@@ +453,5 @@
>  }
>  
>  static nsresult punycode(const char* prefix, const nsAString& in, nsACString& out)
>  {
> +  nsresult rv = NS_OK;

nit: don't initialize this to NS_OK when you are going to change the value immediately below.

nit: declare rv at the same time you initialize it below.

Comment 28

[Biru Mohanathas [:poiru]]

Attachment: Part 1: Check and handle buffer truncation without asserting when converting UTF-16 to UCS-4 in nsIDNService.cpp

(In reply to Brian Smith (:briansmith, was :[e-mail]) from comment #27)
> Comment on attachment 793036 [details] [diff] [review]
> Part 1: Check and handle buffer truncation without asserting when converting
> UTF-16 to UCS-4 in nsIDNService.cpp
> 
> Review of attachment 793036 [details] [diff] [review]:

Thanks, updated patch to fix the nits.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/26b6835a6dd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/675c4547ff7d

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/26b6835a6dd3
https://hg.mozilla.org/mozilla-central/rev/675c4547ff7d