-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #51 from pebosi/master
Adding apostroph as valid email char in local part
- Loading branch information
Showing
2 changed files
with
1 addition
and
2 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -225,7 +225,6 @@ public function testAddUserWrongPassword($userLogin, $password, $email, $alias) | |
public function getWrongEmailTestData() | ||
{ | ||
return array( | ||
array("geggeqgeqag", "geqgeagae", "ema'[email protected]", "alias"), | ||
array("geggeqgeqag", "geqgeagae", "@email.com", "alias"), | ||
array("geggeqgeqag", "geqgeagae", "[email protected]", "alias"), | ||
array("geggeqgeqag", "geqgeagae", "email@4.", "alias"), | ||
|
35bcaca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for REVERT since nobody ever asked for ' in email addresses, and im concerned it would allow XSS in some ways... unless we escape all the email adresses in the output?
35bcaca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 2.0 we can use filter_var as mentioned by the author of the PR. I will look at where we need to escape addresses in the output.
35bcaca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better not using filter_var, the current mechanism is really good, but we can benefit from twig auto escaping hopefully.
In 2.0, for user email and other user submitted attributes, maybe have a macro for each that outputs this value (so we define escaping only once in twig templates)
in the meantime please take a look at all output in templates of user email, but I would prefer to simply revert the fix and postpone it to later ( priorities thing)
35bcaca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter_var is php 5.2+ so would need to be wrapped for Piwik 1.x since its minimum supported version is 5.1.3
35bcaca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW twig can be configured to auto escape by default.
35bcaca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robocoder That's why I said "for 2.0 we can use filter_var", Piwik 2.0 will require 5.3. Twig is set up to autoescape in my current implementation.
35bcaca
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant to qualify that in the context of keeping the change instead of reverting.