Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Poor email validation #12288

Closed
alwye opened this issue Jul 7, 2015 · 5 comments
Closed

Poor email validation #12288

alwye opened this issue Jul 7, 2015 · 5 comments

Comments

@alwye
Copy link

alwye commented Jul 7, 2015

Hi

I was watching a tutorial video on codeschool about form validation in Angular, when I found out emails were validated way too poorly.
For example, you needn't type the email address completely: foo@bar is enough to make form validated.
Just a simple form:

{{test}}
<form name="testForm" ng-submit="return false;" novalidate>
Enter your email:<br>
<input type="email" ng-model="test" required>
<div>form is {{testForm.$valid}}</div>
<input type="submit" class="btn btn-submit" value="Submit">
</form>

Result is that testForm.$valid is TRUE

Thanks.

@caitp
Copy link
Contributor

caitp commented Jul 7, 2015

"foo@bar" is a valid email address, per spec. You can add a custom ng-pattern validator (or otherwise extend the email validator or add another parser/formatter) to be strict about requiring a top-level domain.

@caitp
Copy link
Contributor

caitp commented Jul 7, 2015

Dupe of #11510 / #11385 / #10052 / numerous others

@caitp
Copy link
Contributor

caitp commented Jul 7, 2015

@Narretz you mentioned previously that we should allow customization of this, did you have a plan for an api to make this customization easy?

@gkalpak
Copy link
Member

gkalpak commented Jul 7, 2015

Just in case @zverevalexei is not aware of that, there is an example that shows how to overwrite e-mail validation in the Forms section of the Developer Guide.

@alwye
Copy link
Author

alwye commented Jul 7, 2015

@caitp I did look for other topics, but it seems like I should have done it more properly :) Thanks for your advice!
@gkalpak I'm learning the nature of Angular atm, and certainly, it's useful to me. Thank you!

petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Nov 1, 2017
Chrome 62 was not sanitizing dangerous URLs containing
JavaScript, if they started with these "whitespace" characters.

---

Browsers convert `&angular#12288;javascript:alert(1)` as an attribute value to
`javascript:alert(1)`. So the sanitizer gets the second string and is
able to strip the javascript from the attribute.

But Chrome (<62) only did this after you read it and wrote it back again.
So we added a bit of code that tried to get Chrome to do its conversion
before sanitizing it:
https://github.com/angular/angular.js/blob/817ac56719505680ac4c9997972e8f39eb40a6d0/src/ngSanitize/sanitize.js#L406-L417

I believe that Chrome 62 now does not do this conversion any more, instead
it just leaves the attribute value alone, whatever you do to it.

This fix uses `trim()` to remove the problematic whitespace before
sanitizing, which appears to solve the problem.

Closes angular#16288
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Nov 1, 2017
Chrome 62 was not sanitizing dangerous URLs containing
JavaScript, if they started with these "whitespace" characters.

---

Browsers convert `&angular#12288;javascript:alert(1)` as an attribute value to
`javascript:alert(1)`. So the sanitizer gets the second string and is
able to strip the javascript from the attribute.

But Chrome (<62) only did this after you read it and wrote it back again.
So we added a bit of code that tried to get Chrome to do its conversion
before sanitizing it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

I believe that Chrome 62 now does not do this conversion any more, instead
it just leaves the attribute value alone, whatever you do to it.

This fix uses `trim()` to remove the problematic whitespace before
sanitizing, which appears to solve the problem.

Closes angular#16288
petebacondarwin added a commit to petebacondarwin/angular.js that referenced this issue Nov 2, 2017
Browsers mutate attributes values such as `&angular#12288;javascript:alert(1)`
when they are written to the DOM via `innerHTML` in various vendor specific
ways.

In Chrome (<62), this mutation removed the preceding "whitespace" resulting
in a value that could end up being executed as JavaScript.

Here is an example of what could happen:
https://plnkr.co/edit/Y6EsbsuDgd18YTn1oARu?p=preview
If you run that in Chrome 61 you will get a dialog box pop up.

There is background here:
http://www.nds.rub.de/media/emma/veroeffentlichungen/2013/12/10/mXSS-CCS13.pdf

The sanitizer has a bit of code that triggers this mutation on an inert piece
of DOM, before we try to sanitize it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

Chrome 62 does not appear to mutate this particular string any more, instead
it just leaves the "whitespace" in place. This probably means that Chrome 62
is no longer vulnerable to this specific attack vector; but there may be
other mutating strings that we haven't found, which are vulnerable.

Since we are leaving the mXSS check in place, the sanitizer should still
be immune to any strings that try to utilise this attack vector.

This commit uses `trim()` to remove the IDEOGRAPHIC SPACE "whitespace"
before sanitizing, which allows us to expose this mXSS test to all browsers
rather than just Chrome.

Closes angular#16288
petebacondarwin added a commit that referenced this issue Nov 3, 2017
Browsers mutate attributes values such as `&#12288;javascript:alert(1)`
when they are written to the DOM via `innerHTML` in various vendor specific
ways.

In Chrome (<62), this mutation removed the preceding "whitespace" resulting
in a value that could end up being executed as JavaScript.

Here is an example of what could happen:
https://plnkr.co/edit/Y6EsbsuDgd18YTn1oARu?p=preview
If you run that in Chrome 61 you will get a dialog box pop up.

There is background here:
http://www.nds.rub.de/media/emma/veroeffentlichungen/2013/12/10/mXSS-CCS13.pdf

The sanitizer has a bit of code that triggers this mutation on an inert piece
of DOM, before we try to sanitize it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

Chrome 62 does not appear to mutate this particular string any more, instead
it just leaves the "whitespace" in place. This probably means that Chrome 62
is no longer vulnerable to this specific attack vector; but there may be
other mutating strings that we haven't found, which are vulnerable.

Since we are leaving the mXSS check in place, the sanitizer should still
be immune to any strings that try to utilise this attack vector.

This commit uses `trim()` to remove the IDEOGRAPHIC SPACE "whitespace"
before sanitizing, which allows us to expose this mXSS test to all browsers
rather than just Chrome.

Closes #16288
petebacondarwin added a commit that referenced this issue Nov 3, 2017
Browsers mutate attributes values such as `&#12288;javascript:alert(1)`
when they are written to the DOM via `innerHTML` in various vendor specific
ways.

In Chrome (<62), this mutation removed the preceding "whitespace" resulting
in a value that could end up being executed as JavaScript.

Here is an example of what could happen:
https://plnkr.co/edit/Y6EsbsuDgd18YTn1oARu?p=preview
If you run that in Chrome 61 you will get a dialog box pop up.

There is background here:
http://www.nds.rub.de/media/emma/veroeffentlichungen/2013/12/10/mXSS-CCS13.pdf

The sanitizer has a bit of code that triggers this mutation on an inert piece
of DOM, before we try to sanitize it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

Chrome 62 does not appear to mutate this particular string any more, instead
it just leaves the "whitespace" in place. This probably means that Chrome 62
is no longer vulnerable to this specific attack vector; but there may be
other mutating strings that we haven't found, which are vulnerable.

Since we are leaving the mXSS check in place, the sanitizer should still
be immune to any strings that try to utilise this attack vector.

This commit uses `trim()` to remove the IDEOGRAPHIC SPACE "whitespace"
before sanitizing, which allows us to expose this mXSS test to all browsers
rather than just Chrome.

Closes #16288
cwick pushed a commit to udemy/angular.js that referenced this issue Nov 23, 2019
Browsers mutate attributes values such as `&angular#12288;javascript:alert(1)`
when they are written to the DOM via `innerHTML` in various vendor specific
ways.

In Chrome (<62), this mutation removed the preceding "whitespace" resulting
in a value that could end up being executed as JavaScript.

Here is an example of what could happen:
https://plnkr.co/edit/Y6EsbsuDgd18YTn1oARu?p=preview
If you run that in Chrome 61 you will get a dialog box pop up.

There is background here:
http://www.nds.rub.de/media/emma/veroeffentlichungen/2013/12/10/mXSS-CCS13.pdf

The sanitizer has a bit of code that triggers this mutation on an inert piece
of DOM, before we try to sanitize it:
https://github.com/angular/angular.js/blob/817ac567/src/ngSanitize/sanitize.js#L406-L417

Chrome 62 does not appear to mutate this particular string any more, instead
it just leaves the "whitespace" in place. This probably means that Chrome 62
is no longer vulnerable to this specific attack vector; but there may be
other mutating strings that we haven't found, which are vulnerable.

Since we are leaving the mXSS check in place, the sanitizer should still
be immune to any strings that try to utilise this attack vector.

This commit uses `trim()` to remove the IDEOGRAPHIC SPACE "whitespace"
before sanitizing, which allows us to expose this mXSS test to all browsers
rather than just Chrome.

Closes angular#16288
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants