triplea-game / dice-server-js Goto Github PK

Just noticed that the current MARTI allows the client to pass a custom subject for the mail being sent.
Of course we should limit it to a fixed amount of chars so this feature can't be abused to send spam.
Also user input should probably get escaped, just to be sure.

Note to myself:
The max and times parameter for rolls should be limited to 100 each, so we generate a number between 0 and 10000 per request, no more.

Just thought about it: The Front-End should currently be vulnerable to XSS.
Not really a security concern because this software doesn't use sensitive data like passwords and everything that could be done via an injected script could as well just be done using a normal request without any further authorization.

But really in the end parameter input should be sanitized, the HTML should be escaped.

The only thing to discuss is whether or not we should do the escaping in the actual template or the users.js module, both would be possible, liquid provides an easy "escape"-filter while node doesn't really have a built-in function for that but it would be a single place to fix everything at the cost of a potentially higher complexity for more complicated URL parameters.

I was thinking about this for quite some time now.
The "To: " field in the SMTP protocol allows a comma separated to send an emial to multiple recipients at once.
I don't think there's a limit to this, so a maliciously crafted parameter could trick the server into sending a registration email to hundreds of people using just a single request.
Likely not what we want, even if this wouldn't even have a drawback for those people.
I haven't really found a definite answer to the question how one would escape such an input, MARTI simply splits an input string at whitespaces, not more and merges the resulting arrays afterwards, as If it's designed for more than 2 emails.
Ideas?

@ssoloff Unrelated to that:
I'm mostly done with the "backend" for now, it would be nice if you could have an in-depth look at the logic and escaping of user-input once I documented how the routes work, to make sure I haven't accidentally built in any obvious security-realted design flaw.

The readme could use the following items to help document how to operate this dice server:

• overview of the repo contents and how it fits in with the rest of the system
• compile and build
• test locally
• deploy
• restart
• check status (is it alive?)
• check logs

(readme audit ticket where we discussed what should be in each readme: triplea-game/triplea#3289)

Currently the server expects this sort of config:

email: {
    display: {
      server: {
        protocol: 'http',
        host: 'localhost',
        port: 7654,
        baseurl: '',
      },
    },
},

It would be much better, if simply

email: {
    display: {
      server: 'http://localhost/'
    },
},

could be used instead, all we do is append the path anyways

Mandatory:

• Better Error Messages (Currently a lot error messages is just passed through). We need to seperate "internal issues" which should just be printed to the log from "user-related issues" which the user should be informed about
• Send dice emails after rolling/Check if users are registered
• Prettier Emails in general (Maybe use a templating engine?)
• HTML sites allowing users to register their emails (Templating engine?)
• Verify user input is actually emails, currently there are no checks which verify the emails are actual single emails and not a list which would be processed by our sendEmail command and sent to several users, also we need to limit registration requests, so our server can't be used to spam innocent people (also hurts our reputation), also don't send a registration email if the user is already registered, and return an error instead for consinstency.

Optional:

• Improve the error logging system internally, currently a lot of logging is done "by hand", we don't have something like an error builder, which returns the result if everything works out, and returns an error status with message otherwhise. This includes automated checking of params while only passing maps of predicates and error messages. The error-handler.js file was my "might finish sometime" attempt to adress this, haven't had the time.
• Have user interface and UI in parallel: This means, without the /api prefix an html page based on the JSON should be displayed instead of the raw JSON
• Have an automated setup command. Currently a config.json needs to be created manually, we should provide a setup command or something similar
• TESTS. (Also travis integration). It might work out on our test enviroment, but nothing is better thana good amount of tests. (Haven't ever used test on a node environment, so any insights would be helpful)
• Restructuring. If everything is done, we might should consider on improving the folder structure instead of storing everything in the src folder.
• Cryptographic dice rolls? Might be overkill, but it might be "more random" if we didn't rely on Math.random() to generate our numbers, but on crypto.randomBytes() instead.