Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Can't use normal email addresses to sign up, create account, or create ticket. #130

Closed
1 of 2 tasks
apboon opened this issue Jan 22, 2019 · 3 comments
Closed
1 of 2 tasks

Comments

@apboon
Copy link

apboon commented Jan 22, 2019

Is this a BUG REPORT or FEATURE REQUEST?:

  • BUG
  • FEATURE

What happened:
Can't use normal email addresses to sign up, create account, or create ticket.

What did you expect to happen:
Email address to be evaluated as valid.

How to reproduce it (as minimally and precisely as possible):
Sign up with a modern (post 2014) email address like person@company.agency or person@local-company.amsterdam.

Anything else we need to know?:
Trudesk looks like a wonderful piece of software. Been searching far and wide for exactly this: NodeJS, MongoDB, SPA, JSON API, very user friendly, a breeze to work with, wonderful! :)
But at the moment unable to use the software because our company uses modern email address.. minor problem though..

The project's code contains 3 instances of the RegEx ^\w+([\.-]?\w+)*@\w+([\.-]?\w+)*(\.\w{2,3})+$ of which I notice:

  • Many characters with special meaning lose their special meaning inside of character classes ([]). The dot (.) does not require escaping inside of a character class. Luckily the stray slash is being ignored and email addresses like john\doe@example.com aren't considered valid. The original RegEx can be written down as ^\w+([.-]?\w+)*@\w+([.-]?\w+)*(\.\w{2,3})+$.

  • Taking another look I notice the ? modifier behind the character classes which does not seem to have a purpose. Partial RegEx \w+([.-]?\w+)* works exactly the same as \w+([.-]\w+)* (without the ?). If no dots or dashes are found, the first part of the RegEx \w+ already actures everything, after which the capturing group optionally captures any trailing .something or -something. Making the dot/dash optional is similar to matching (\w+)*, which is already covered by the initial \w+ part. The original RegEx can therefore be simplified as ^\w+([.-]\w+)*@\w+([.-]\w+)*(\.\w{2,3})+$ without losing any of its meaning. Same outcome as the original; just slightly simplified notation.

  • I took a list of example email addresses from the internet (https://gist.github.com/cjaoude/fd9910626629b53c4d25) and tested the regular expression using https://regex101.com/. Simply updating the \w{2,3} to \w+ (and leaving everything else in tact) makes the RegEx compatible with most modern (post 2014) email addresses (with TLDs like .cafe, etc). The resulting RegEx would be: ^\w+([.-]\w+)*@\w+([.-]\w+)*(\.\w+)+$

Conclusion/solution:
Change from ^\w+([\.-]?\w+)*@\w+([\.-]?\w+)*(\.\w{2,3})+$ to ^\w+([.-]\w+)*@\w+([.-]\w+)*(\.\w+)+$

Some might argue that the regular expression would still not cover email addresses like john+doe@example.com, but there might be good reason to exclude those as the plus sign has special meaning; mail sent to john+doe@example.com is actually being received in the john@example.com mailbox and flagged with tag doe.

Cheers! :)

Environment:

  • Trudesk Version: Latest/master
  • OS (e.g. from /etc/os-release): Not relevant
  • Node.JS Version: Not relevant
  • MongoDB Version: Not relevant
  • Is this hosted on cloud.trudesk.io: http://docker.trudesk.io/
@polonel
Copy link
Owner

polonel commented Jan 22, 2019

Thank you. I will include these fixes in the next update. I appreciate the diving into the code and helping point out these flaws. This is a huge help right now as I'm refactoring a lot of outdated code.

@apboon
Copy link
Author

apboon commented Jan 22, 2019

Awesomeness! ^^
Hey question.. if I see something that could be improved, do you want me to fork and create a pull request with a fix? Or you rather have people open an issue?
I would not have mind the former.

@polonel
Copy link
Owner

polonel commented Jan 22, 2019

I prefer a pull request. Help tracks your contribution. Just make sure it's against develop branch.

@polonel polonel changed the title Can't use normal email addresses to sign up, create account, or create ticket. [BUG] Can't use normal email addresses to sign up, create account, or create ticket. Jan 28, 2019
polonel added a commit that referenced this issue Feb 2, 2019
## [1.0.6](v1.0.5...v1.0.6) (2019-02-02)

### Bug Fixes

* **attachments:** uploading office mime-types [#140](#140) ([b47da40](b47da40))
* **chat:** chat boxes under some buttons ([c337c76](c337c76))
* **dates:** overdue on dashboard ([921e258](921e258))
* **groups:** issue preventing save ([7208253](7208253))
* **ldap:** crash if no results are returned ([8ff63ba](8ff63ba))
* **login:** username whitespaces ([282d725](282d725))
* **messages:** remove ajax link from start conversation ([988dfa9](988dfa9))
* **notifications:** unable to clear all notifications ([4f24c8c](4f24c8c))
* **reports:** invalid date format ([808a740](808a740))
* **reports:** invalid date string ([0914d91](0914d91))
* **socket:** high memory usage on notification updates ([b647d4c](b647d4c))
* **ticket:** priority not updating in realtime ([721f42d](721f42d))
* **unzip:** out dated dependency [#139](#139) ([b0aab01](b0aab01))
* **url:** invalid parse ([1738287](1738287))
* **validation:** email validation for modern tlds [#130](#130) ([febcbdf](febcbdf))
polonel added a commit that referenced this issue Feb 2, 2019
## [1.0.6](v1.0.5...v1.0.6) (2019-02-02)

### Bug Fixes

* **attachments:** uploading office mime-types [#140](#140) ([b47da40](b47da40))
* **chat:** chat boxes under some buttons ([c337c76](c337c76))
* **dates:** overdue on dashboard ([921e258](921e258))
* **editor:** crash on invalid directory ([bc60899](bc60899))
* **groups:** issue preventing save ([7208253](7208253))
* **ldap:** crash if no results are returned ([8ff63ba](8ff63ba))
* **login:** username whitespaces ([282d725](282d725))
* **messages:** remove ajax link from start conversation ([988dfa9](988dfa9))
* **notifications:** unable to clear all notifications ([4f24c8c](4f24c8c))
* **reports:** invalid date format ([808a740](808a740))
* **reports:** invalid date string ([0914d91](0914d91))
* **socket:** high memory usage on notification updates ([b647d4c](b647d4c))
* **ticket:** priority not updating in realtime ([721f42d](721f42d))
* **unzip:** out dated dependency [#139](#139) ([b0aab01](b0aab01))
* **url:** invalid parse ([1738287](1738287))
* **validation:** email validation for modern tlds [#130](#130) ([febcbdf](febcbdf))
polonel added a commit that referenced this issue Feb 2, 2019
* **attachments:** uploading office mime-types [#140](#140) ([b47da40](b47da40))
* **chat:** chat boxes under some buttons ([c337c76](c337c76))
* **dates:** overdue on dashboard ([921e258](921e258))
* **editor:** crash on invalid directory ([bc60899](bc60899))
* **groups:** issue preventing save ([7208253](7208253))
* **ldap:** crash if no results are returned ([8ff63ba](8ff63ba))
* **login:** username whitespaces ([282d725](282d725))
* **messages:** remove ajax link from start conversation ([988dfa9](988dfa9))
* **notifications:** unable to clear all notifications ([4f24c8c](4f24c8c))
* **reports:** invalid date format ([808a740](808a740))
* **reports:** invalid date string ([0914d91](0914d91))
* **socket:** high memory usage on notification updates ([b647d4c](b647d4c))
* **ticket:** priority not updating in realtime ([721f42d](721f42d))
* **unzip:** out dated dependency [#139](#139) ([b0aab01](b0aab01))
* **url:** invalid parse ([1738287](1738287))
* **validation:** email validation for modern tlds [#130](#130) ([febcbdf](febcbdf))
@polonel polonel closed this as completed Feb 2, 2019
@polonel polonel added production and removed review labels Feb 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants