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.

Sign up for GitHub

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

Jump to bottom

Blacklist System as Username #9060

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Blacklist System as Username #9060

merged 1 commit into from
Jul 9, 2018

Conversation

DSchalla
Copy link
Member

@DSchalla DSchalla commented Jul 6, 2018

Summary

This PR blacklists System as a username. The justification is that system notifications, e.g. user join messages are sent with that nickname, allowing an attacker to act as a system user for social engineering, an example would be:

Example of Abuse

Implementation was discussed with @crspeller, there is no ticket or issue right now for it.

Checklist

  • Added or updated unit tests (required for all new features)

@jwilander jwilander added the 2: Dev Review Requires review by a developer label Jul 6, 2018
crspeller
crspeller approved these changes Jul 6, 2018
View reviewed changes
Copy link
Member

@crspeller crspeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reasonable precaution. Thanks!

@grundleborg
Copy link
Contributor

Looks good. Thanks @DSchalla

@grundleborg grundleborg merged commit e8b8ef3 into mattermost:master Jul 9, 2018
@lindalumitchell
Copy link
Contributor

@DSchalla @crspeller should this be showing a friendly error when testing on master (bladekick, built today Jul 10)? In place of the expected error messaging, I see printed below the Create button: model.user.is_valid.username.app_error.
unfriendly-username-error

When I use another blacklisted username matterbot, I see error messaging printed below the username field: This username is reserved, please choose a new one.
expected-reserved-error

Should this be filed as a ticket?

@lindalumitchell lindalumitchell added the Tests/Done Required release tests have been written label Jul 11, 2018
@crspeller
Copy link
Member

@lindalumitchell Looks like we are doing some client side magic to show that message. The error the server is sending back is correct. We can make a ticket for that.
@DSchalla Care to make the fix?

@DSchalla DSchalla mentioned this pull request Jul 11, 2018
Merged
3 tasks
@DSchalla
Copy link
Member Author

DSchalla commented Jul 11, 2018 •
edited
Loading

@crspeller Sure, done: mattermost/mattermost-webapp#1437
@lindalumitchell That should fix it.

@lindalumitchell
Copy link
Contributor

Excellent, thanks! Verified on master after mattermost/mattermost-webapp#1437 was merged that the friendly error message appears.

@amyblais amyblais added Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation labels Jul 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crspeller crspeller crspeller approved these changes

@grundleborg grundleborg grundleborg approved these changes

Assignees
No one assigned
Labels
2: Dev Review Requires review by a developer Changelog/Not Needed Does not require a changelog entry Docs/Not Needed Does not require documentation Tests/Done Required release tests have been written
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@DSchalla @grundleborg @lindalumitchell @crspeller @jwilander @amyblais