Mounir Lamouri (:mounir) Reporter
	Description • 11 years ago

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 ;)

	Mounir Lamouri (:mounir) Reporter
	Comment 1 • 11 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 2 • 11 years ago

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.

	Biru [:poiru] Assignee
	Comment 3 • 11 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 4 • 11 years ago

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.

	Biru [:poiru] Assignee
	Comment 5 • 11 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 6 • 11 years ago

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.

	Biru [:poiru] Assignee
	Comment 7 • 11 years ago

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 "40-chars-here@40-chars-here.com" 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.

	Anne (:annevk)
	Comment 8 • 11 years ago

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).

	Biru [:poiru] Assignee
	Comment 9 • 11 years ago

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!

	Mounir Lamouri (:mounir) Reporter
	Comment 10 • 11 years ago

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(...);

	Biru [:poiru] Assignee
	Comment 11 • 11 years ago

Addresses comment #10.

	Mounir Lamouri (:mounir) Reporter
	Comment 12 • 11 years ago

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.

	Biru [:poiru] Assignee
	Comment 13 • 11 years ago

(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?

	Mounir Lamouri (:mounir) Reporter
	Comment 14 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 16 • 11 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 17 • 11 years ago

Birunthan, are you able to reproduce this locally?

	Biru [:poiru] Assignee
	Comment 18 • 11 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 19 • 11 years ago

(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

	Biru [:poiru] Assignee
	Comment 20 • 11 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 21 • 11 years ago

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

	Mounir Lamouri (:mounir) Reporter
	Comment 22 • 11 years ago

I've opened bug 906623 about the assert issue.

	Biru [:poiru] Assignee
	Comment 23 • 11 years ago

(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

	Biru [:poiru] Assignee
	Comment 24 • 11 years ago

Updated commit summary.

	Mounir Lamouri (:mounir) Reporter
	Comment 25 • 11 years ago

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.

	Mounir Lamouri (:mounir) Reporter
	Comment 26 • 11 years ago

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 ;)

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 27 • 11 years ago

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.

	Biru [:poiru] Assignee
	Comment 28 • 11 years ago

(In reply to Brian Smith (:briansmith, was :bsmith@mozilla.com) 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.

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

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