Skip to content

Allow email as username#2407

Merged
Alkarex merged 3 commits intoFreshRSS:devfrom
aledeg:fix/email-as-username
Jun 16, 2019
Merged

Allow email as username#2407
Alkarex merged 3 commits intoFreshRSS:devfrom
aledeg:fix/email-as-username

Conversation

@aledeg
Copy link
Member

@aledeg aledeg commented Jun 12, 2019

Before, it was possible to register email as username on cli but not in the
interface. This was caused by a bug in the pattern which was not working as
expected. If your input was "user@example.com", the PHP verification was
catching only "user" and was acting like the whole thing was catched. But
on the interface, the catching was unsuccesful.
Now, the catching should be working properly.

I needed to add "$|^" in the pattern because without, I was catching either
the beginning of a string either the last char. This was introduced as a
workaround for IE/Edge pattern matching on April 27, 2017. See #1511 for
more information.

I tested it only on FF. Tests on other browsers wanted.

See #2391

Before, it was possible to register email as username on cli but not in the
interface. This was caused by a bug in the pattern which was not working as
expected. If your input was "user@example.com", the PHP verification was
catching only "user" and was acting like the whole thing was catched. But
on the interface, the catching was unsuccesful.
Now, the catching should be working properly.

I needed to add "$|^" in the pattern because without, I was catching either
the beginning of a string either the last char. This was introduced as a
workaround for IE/Edge pattern matching on April 27, 2017. See FreshRSS#1511 for
more information.

I tested it only on FF. Tests on other browsers wanted.

See FreshRSS#2391
@Alkarex Alkarex added this to the 1.15.0 milestone Jun 14, 2019
@Alkarex
Copy link
Member

Alkarex commented Jun 14, 2019

We also need to double-check with the 3 databases: SQLite, MySQL, PostgreSQL

@aledeg
Copy link
Member Author

aledeg commented Jun 14, 2019

I was thinking about it and we should even write the pattern completely. Meaning we should provide ^[0-9a-zA-Z_][0-9a-zA-Z_.@]{1,38}$|^[0-9a-zA-Z]$.

@Frenzie
Copy link
Member

Frenzie commented Jun 14, 2019

Is there a specific reason to disallow a username like @usern@m3@ if the @ character is not problematic?

On the subject of e-mails itself, both of the suggested patterns disallow common e-mails with a + or - in the pattern, typically in the form of aaa+bbb@ccc.ddd. The whole of

!#$%&'*+-/=?^_`{|}~

is valid.

This might seem like a minor semantic point (allow some e-mails vs. allow e-mails) but + and - are definitely quite popular in Western addresses, while UTF-8 is popular in the East, so I don't think it's nearly as minor as it might seem at first glance.

@aledeg
Copy link
Member Author

aledeg commented Jun 14, 2019

Keep in mind that the user has a folder on disk. There might be a limitation regarding what we can write on the file system.

@Frenzie
Copy link
Member

Frenzie commented Jun 14, 2019

Only / is problematic for that purpose.

But anyway, I'm not necessarily arguing it should all be allowed, although I would like to see + and -. Just that it shouldn't be presented to the user as e-mails can be used as usernames.

@aledeg
Copy link
Member Author

aledeg commented Jun 14, 2019

Agreed. I'll modify the code to allow + and - signs. I think it's a good compromise.

@Alkarex
Copy link
Member

Alkarex commented Jun 16, 2019

I have just suggested a new pattern, allowing + and -:

([0-9a-zA-Z_][0-9a-zA-Z_.@+-]{1,38}|[0-9a-zA-Z])

The ^ and $ are automatically added by browsers, and we also do it in PHP.
It seems to work fine in Firefox, Chrome, Edge, IE11, and PHP.

We should double-check with the 3 database types, and check whether the username appears anywhere else potentially problematic.

In any case, we should not allow / \ : ? " * < > | anywhere, not allow that the full username is _ (used internally as default), not allow non-printable / control chars, etc.
Actually, we are not strict enough on Windows :-( https://stackoverflow.com/questions/1976007/what-characters-are-forbidden-in-windows-and-linux-directory-names

@Alkarex
Copy link
Member

Alkarex commented Jun 16, 2019

Arg, the + gives a problem in the login (special character in URLs)

@Alkarex
Copy link
Member

Alkarex commented Jun 16, 2019

I am (gladly) surprised to see that @ in username even works with HTTP Basic Auth in IE11

@Alkarex
Copy link
Member

Alkarex commented Jun 16, 2019

Seems to work fine in SQLite, MySQL, PostgreSQL.

@Alkarex
Copy link
Member

Alkarex commented Jun 16, 2019

Let's add only @- for now, and potentially come back to more characters later if needed.
For Unicode usernames, we could consider using Punycode (which we already use in FreshRSS for international DNS). Otherwise we will have to transition to using some hashing.

@Alkarex Alkarex merged commit 7f1ff77 into FreshRSS:dev Jun 16, 2019
@aledeg aledeg deleted the fix/email-as-username branch June 21, 2019 07:14
@Offerel
Copy link
Contributor

Offerel commented Jun 21, 2019

I tried the dev branch and created a user with e-mail as login. However i cant login. I get

[Fri, 21 Jun 2019 10:55:37 +0200] [warning] --- Password mismatch for user=user@mydomain.de, nonce=, c=$2a$04$tTX99qYMYRJOYHew.zm4POdfg54647756745466A0y/Hy/Fz16

The user directory is created as "usermydomainde". I can't check the database now, but i think, its the same there. Please take also in account that there can be several ASCII signs in the password, like ?$_!'"%.... However, for this test, is uses only numbers and letters, so that the password should be no problem.

Update: The above is the result of creating the first time user during installation. If i create a user with e-mail after that, it seems to work.

@Alkarex
Copy link
Member

Alkarex commented Jun 21, 2019

@Offerel Could you please open a new issue for that? It seems to be install-specific

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Jun 21, 2019
Alkarex added a commit that referenced this pull request Jun 21, 2019
@Alkarex Alkarex modified the milestones: 1.15.0, 1.14.3 Jul 13, 2019
mdemoss pushed a commit to mdemoss/FreshRSS that referenced this pull request Mar 25, 2021
* Allow email as username

Before, it was possible to register email as username on cli but not in the
interface. This was caused by a bug in the pattern which was not working as
expected. If your input was "user@example.com", the PHP verification was
catching only "user" and was acting like the whole thing was catched. But
on the interface, the catching was unsuccesful.
Now, the catching should be working properly.

I needed to add "$|^" in the pattern because without, I was catching either
the beginning of a string either the last char. This was introduced as a
workaround for IE/Edge pattern matching on April 27, 2017. See FreshRSS#1511 for
more information.

I tested it only on FF. Tests on other browsers wanted.

See FreshRSS#2391

* Relax and fix username check

Allow @ + -

* Remove + for now

FreshRSS#2407 (comment)
mdemoss pushed a commit to mdemoss/FreshRSS that referenced this pull request Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants