diff --git a/README.md b/README.md index b0cf78752..e23c53912 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,7 @@ Note that this example creates the `fullchain.pem` and `privkey.pem` files in a directory one level higher from the current, so that you don't accidentally commit your certificates to `solid` while you're developing. -If you would like to get rid of the browser warnings, import your fullchain.pem certificate into your 'Trusted Root Certificate' store. +If you would like to get rid of the browser warnings, import your fullchain.pem certificate into your 'Trusted Root Certificate' store. ### Run multi-user server (intermediate) @@ -375,6 +375,12 @@ In order to test a single component, you can run npm run test-(acl|formats|params|patch) ``` +## Blacklisted usernames + +By default Solid will not allow [certain usernames as they might cause +confusion or allow vulnerabilies for social engineering](https://github.com/marteinn/The-Big-Username-Blacklist). +This list is configurable via `config/usernames-blacklist.json`. Solid does not +blacklist profanities by default. ## Contributing diff --git a/config/usernames-blacklist.json b/config/usernames-blacklist.json new file mode 100644 index 000000000..c1b469a1e --- /dev/null +++ b/config/usernames-blacklist.json @@ -0,0 +1,4 @@ +{ + "useTheBigUsernameBlacklist": true, + "customBlacklistedUsernames": [] +} diff --git a/lib/create-app.js b/lib/create-app.js index 2a0b15da7..c8074395b 100644 --- a/lib/create-app.js +++ b/lib/create-app.js @@ -12,8 +12,8 @@ const authProxy = require('./handlers/auth-proxy') const SolidHost = require('./models/solid-host') const AccountManager = require('./models/account-manager') const vhost = require('vhost') -const EmailService = require('./models/email-service') -const TokenService = require('./models/token-service') +const EmailService = require('./services/email-service') +const TokenService = require('./services/token-service') const capabilityDiscovery = require('./capability-discovery') const API = require('./api') const errorPages = require('./handlers/error-pages') diff --git a/lib/requests/create-account-request.js b/lib/requests/create-account-request.js index 68997822c..63e8a8e2b 100644 --- a/lib/requests/create-account-request.js +++ b/lib/requests/create-account-request.js @@ -3,6 +3,7 @@ const AuthRequest = require('./auth-request') const WebIdTlsCertificate = require('../models/webid-tls-certificate') const debug = require('../debug').accounts +const blacklistService = require('../services/blacklist-service') /** * Represents a 'create new user account' http request (either a POST to the @@ -115,30 +116,28 @@ class CreateAccountRequest extends AuthRequest { /** * Creates an account for a given user (from a POST to `/api/accounts/new`) * - * @throws {Error} An http 400 error if an account already exists + * @throws {Error} If errors were encountering while validating the username. * * @return {Promise} Resolves with newly created account instance */ - createAccount () { + async createAccount () { let userAccount = this.userAccount let accountManager = this.accountManager - return Promise.resolve(userAccount) - .then(this.cancelIfUsernameInvalid.bind(this)) - .then(this.cancelIfAccountExists.bind(this)) - .then(this.createAccountStorage.bind(this)) - .then(this.saveCredentialsFor.bind(this)) - .then(this.sendResponse.bind(this)) - .then(userAccount => { - // 'return' not used deliberately, no need to block and wait for email - if (userAccount && userAccount.email) { - debug('Sending Welcome email') - accountManager.sendWelcomeEmail(userAccount) - } - }) - .then(() => { - return userAccount - }) + this.cancelIfUsernameInvalid(userAccount) + this.cancelIfBlacklistedUsername(userAccount) + await this.cancelIfAccountExists(userAccount) + await this.createAccountStorage(userAccount) + await this.saveCredentialsFor(userAccount) + await this.sendResponse(userAccount) + + // 'return' not used deliberately, no need to block and wait for email + if (userAccount && userAccount.email) { + debug('Sending Welcome email') + accountManager.sendWelcomeEmail(userAccount) + } + + return userAccount } /** @@ -196,12 +195,33 @@ class CreateAccountRequest extends AuthRequest { * @throws {Error} If errors were encountering while validating the * username. * - * @return {Promise} Chainable + * @return {UserAccount} Chainable */ cancelIfUsernameInvalid (userAccount) { if (!userAccount.username || !/^[a-z0-9]+(?:-[a-z0-9]+)*$/.test(userAccount.username)) { debug('Invalid username ' + userAccount.username) - const error = new Error('Invalid username') + const error = new Error('Invalid username (contains invalid characters)') + error.status = 400 + throw error + } + + return userAccount + } + + /** + * Check if a username is a valid slug. + * + * @param userAccount {UserAccount} Instance of the account to be created + * + * @throws {Error} If username is blacklisted + * + * @return {UserAccount} Chainable + */ + cancelIfBlacklistedUsername (userAccount) { + const validUsername = blacklistService.validate(userAccount.username) + if (!validUsername) { + debug('Invalid username ' + userAccount.username) + const error = new Error('Invalid username (username is blacklisted)') error.status = 400 throw error } diff --git a/lib/services/blacklist-service.js b/lib/services/blacklist-service.js new file mode 100644 index 000000000..caef80ba7 --- /dev/null +++ b/lib/services/blacklist-service.js @@ -0,0 +1,33 @@ +const blacklistConfig = require('../../config/usernames-blacklist.json') +const blacklist = require('the-big-username-blacklist').list + +class BlacklistService { + constructor () { + this.reset() + } + + addWord (word) { + this.list.push(BlacklistService._prepareWord(word)) + } + + reset (config) { + this.list = BlacklistService._initList(config) + } + + validate (word) { + return this.list.indexOf(BlacklistService._prepareWord(word)) === -1 + } + + static _initList (config = blacklistConfig) { + return [ + ...(config.useTheBigUsernameBlacklist ? blacklist : []), + ...config.customBlacklistedUsernames + ] + } + + static _prepareWord (word) { + return word.trim().toLocaleLowerCase() + } +} + +module.exports = new BlacklistService() diff --git a/lib/models/email-service.js b/lib/services/email-service.js similarity index 99% rename from lib/models/email-service.js rename to lib/services/email-service.js index 69166793a..d0fe448f7 100644 --- a/lib/models/email-service.js +++ b/lib/services/email-service.js @@ -2,7 +2,7 @@ const nodemailer = require('nodemailer') const path = require('path') -const debug = require('./../debug').email +const debug = require('../debug').email /** * Models a Nodemailer-based email sending service. diff --git a/lib/models/token-service.js b/lib/services/token-service.js similarity index 100% rename from lib/models/token-service.js rename to lib/services/token-service.js diff --git a/package-lock.json b/package-lock.json index 5bd3d23cb..e1def6b06 100644 --- a/package-lock.json +++ b/package-lock.json @@ -8366,6 +8366,11 @@ "integrity": "sha1-f17oI66AUgfACvLfSoTsP8+lcLQ=", "dev": true }, + "the-big-username-blacklist": { + "version": "1.5.1", + "resolved": "https://registry.npmjs.org/the-big-username-blacklist/-/the-big-username-blacklist-1.5.1.tgz", + "integrity": "sha1-uqanyWHRpJo1OdFUJMNMjTPHdtU=" + }, "then-fs": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/then-fs/-/then-fs-2.0.0.tgz", diff --git a/package.json b/package.json index 626ae6f8f..5f7bc16c3 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "solid-permissions": "^0.6.0", "solid-ws": "^0.2.3", "text-encoder-lite": "^1.0.1", + "the-big-username-blacklist": "^1.5.1", "ulid": "^0.1.0", "uuid": "^3.0.0", "valid-url": "^1.0.9", diff --git a/test/unit/account-manager-test.js b/test/unit/account-manager-test.js index 90cb8d639..6d69bd391 100644 --- a/test/unit/account-manager-test.js +++ b/test/unit/account-manager-test.js @@ -15,7 +15,7 @@ const LDP = require('../../lib/ldp') const SolidHost = require('../../lib/models/solid-host') const AccountManager = require('../../lib/models/account-manager') const UserAccount = require('../../lib/models/user-account') -const TokenService = require('../../lib/models/token-service') +const TokenService = require('../../lib/services/token-service') const WebIdTlsCertificate = require('../../lib/models/webid-tls-certificate') const testAccountsDir = path.join(__dirname, '../resources/accounts') diff --git a/test/unit/blacklist-service-test.js b/test/unit/blacklist-service-test.js new file mode 100644 index 000000000..a2db974ca --- /dev/null +++ b/test/unit/blacklist-service-test.js @@ -0,0 +1,49 @@ +'use strict' + +const chai = require('chai') +const expect = chai.expect + +const blacklist = require('the-big-username-blacklist').list +const blacklistService = require('../../lib/services/blacklist-service') + +describe('BlacklistService', () => { + afterEach(() => blacklistService.reset()) + + describe('addWord', () => { + it('allows adding words', () => { + const numberOfBlacklistedWords = blacklistService.list.length + blacklistService.addWord('foo') + expect(blacklistService.list.length).to.equal(numberOfBlacklistedWords + 1) + }) + }) + + describe('reset', () => { + it('will reset list of blacklisted words', () => { + blacklistService.addWord('foo') + blacklistService.reset() + expect(blacklistService.list.length).to.equal(blacklist.length) + }) + + it('can configure service via reset', () => { + blacklistService.reset({ + useTheBigUsernameBlacklist: false, + customBlacklistedUsernames: ['foo'] + }) + expect(blacklistService.list.length).to.equal(1) + expect(blacklistService.validate('admin')).to.equal(true) + }) + + it('is a singleton', () => { + const instanceA = blacklistService + blacklistService.reset({ customBlacklistedUsernames: ['foo'] }) + expect(instanceA.validate('foo')).to.equal(blacklistService.validate('foo')) + }) + }) + + describe('validate', () => { + it('validates given a default list of blacklisted usernames', () => { + const validWords = blacklist.reduce((memo, word) => memo + (blacklistService.validate(word) ? 1 : 0), 0) + expect(validWords).to.equal(0) + }) + }) +}) diff --git a/test/unit/create-account-request-test.js b/test/unit/create-account-request-test.js index 469fe7822..892219b37 100644 --- a/test/unit/create-account-request-test.js +++ b/test/unit/create-account-request-test.js @@ -7,12 +7,14 @@ const sinonChai = require('sinon-chai') chai.use(sinonChai) chai.should() const HttpMocks = require('node-mocks-http') +const blacklist = require('the-big-username-blacklist') const LDP = require('../../lib/ldp') const AccountManager = require('../../lib/models/account-manager') const SolidHost = require('../../lib/models/solid-host') const defaults = require('../../config/defaults') const { CreateAccountRequest } = require('../../lib/requests/create-account-request') +const blacklistService = require('../../lib/services/blacklist-service') describe('CreateAccountRequest', () => { let host, store, accountManager @@ -127,6 +129,45 @@ describe('CreateAccountRequest', () => { expect(invalidUsernamesCount).to.eq(invalidUsernames.length) }) }) + + describe('Blacklisted usernames', () => { + const invalidUsernames = [...blacklist.list, 'foo'] + + before(() => { + const accountManager = AccountManager.from({ host }) + accountManager.accountExists = sinon.stub().returns(Promise.resolve(false)) + blacklistService.addWord('foo') + }) + + after(() => blacklistService.reset()) + + it('should return a 400 error if a username is blacklisted', async () => { + const locals = { authMethod: defaults.auth, accountManager, oidc: { users: {} } } + + let invalidUsernamesCount = 0 + + const requests = invalidUsernames.map((username) => { + let req = HttpMocks.createRequest({ + app: { locals }, + body: { username, password: '1234' } + }) + let request = CreateAccountRequest.fromParams(req, res) + + return request.createAccount() + .then(() => { + throw new Error('should not happen') + }) + .catch(err => { + invalidUsernamesCount++ + expect(err.message).to.match(/Invalid username/) + expect(err.status).to.equal(400) + }) + }) + + await Promise.all(requests) + expect(invalidUsernamesCount).to.eq(invalidUsernames.length) + }) + }) }) }) diff --git a/test/unit/email-service-test.js b/test/unit/email-service-test.js index 97de0ca80..27f08da26 100644 --- a/test/unit/email-service-test.js +++ b/test/unit/email-service-test.js @@ -1,4 +1,4 @@ -const EmailService = require('../../lib/models/email-service') +const EmailService = require('../../lib/services/email-service') const path = require('path') const sinon = require('sinon') const chai = require('chai') diff --git a/test/unit/email-welcome-test.js b/test/unit/email-welcome-test.js index 86e3935b2..7317a48c3 100644 --- a/test/unit/email-welcome-test.js +++ b/test/unit/email-welcome-test.js @@ -10,7 +10,7 @@ chai.should() const SolidHost = require('../../lib/models/solid-host') const AccountManager = require('../../lib/models/account-manager') -const EmailService = require('../../lib/models/email-service') +const EmailService = require('../../lib/services/email-service') const templatePath = path.join(__dirname, '../../default-templates/emails') diff --git a/test/unit/token-service-test.js b/test/unit/token-service-test.js index ea84fe9a7..2751a2abb 100644 --- a/test/unit/token-service-test.js +++ b/test/unit/token-service-test.js @@ -6,7 +6,7 @@ const dirtyChai = require('dirty-chai') chai.use(dirtyChai) chai.should() -const TokenService = require('../../lib/models/token-service') +const TokenService = require('../../lib/services/token-service') describe('TokenService', () => { describe('constructor()', () => {