2
\$\begingroup\$

This is an online implementation of a game: Compatibility. You can find the rules here. The idea is to have a browser that perform all the game's mechanics and to discuss with your friends through a video call. In my version of the game, I only implemented the part of the rules for individual play.

Here is the workflow:

  1. Every player is discussing in a video call
  2. One of the player is the master of the game and will create a new game with its browser using the route /compatibility
  3. When done, the master will send a link to all the players via the video call
  4. Each player can click on its name to begin the game
  5. The master has a dedicated web page to start a new round with a new theme
  6. At each round, each player selects five images corresponding to the theme
  7. When every player has send their images, everyone can review the results and the score
  8. The game continues from step 5

Here are the different files in my project:

  • app.js: it is the server of the application. It initializes the game and receives every request from the clients and manages them. It also contains the class Game that is used to represent a current game and a utility function getGetParameters.
  • master.js and player.js: these files corresponds to the client applications. We have two kind of client: the master of the game and the players. These files will be included respectively by master.html and player.html.
  • master.html and player.html: represent the data the master and the players will see on their browser.
  • start.tmpl and play.tmpl: template files used during the creation of the game. These files will be modified at run time to generate a dynamic HTML page.
  • create.html: HTML form to create a new game.
  • dico.txt: text file containing the list of the themes.

There is also an img folder containing all the images of the game named like n.jpg with n a number between 1 and 52.

This is my first application using Node.js. I followed these tips to stay focused and finish your hobby project and specially the idea of coming up with a working prototype as quickly as you can. I know there is a lot to improve but I'm specially interested on the following aspects:

  • Route management: in my code, app.js parses the request URL to determine which page it should respond. I'm wondering how it is possible to do it better.
  • Client/Server workflow: the clients of the application, when they are waiting for an event from the server, send a request every second. Does it exist something to prevent that?
  • Template management: I created some .tmpl file that app.js will uses to produce dynamic HTML files. Maybe something more efficient exists.

However I'm also interested by every reviews about security (how to not cheat, input validation,...), good coding practices in JavaScript, locale management (the project is in French but I translated every messages and labels manually for this review) and anything else you find important. Just, I'm currently not interested about CSS considerations.

You can find the full code of the project on its GitHub page. As the project is in French, here is the English translation (I just omit the dictionary and the images here):

  • app.js
const http = require('http');
const fs = require('fs');

const port = 3000;

var games = {};

var words = [];

fs.readFile('dico.txt', 'utf8', (err, data) => {
    words = data.split('\n');
});

class Game {
    constructor(players) {
        this.state = "starting";
        this.word = '';
        this.remainingWords = words;
        this.send = {};
        this.matcher = '';
        this.matcher_id = 0;

        this.players = {};

        decodeURIComponent(players).split(';').forEach((player) => {
            this.players[player] = -1;
        });
    }

    newWord() {
        var idx = Math.floor(Math.random() * Math.floor(this.remainingWords.length));
        this.word = this.remainingWords.splice(idx, 1)[0];
        this.state = "waitingPlayers";
        this.matcher = Object.keys(this.players)[this.matcher_id];
        this.matcher_id = (this.matcher_id + 1) % Object.keys(this.players).length;
        this.send = {};

        return this.word + '#' + this.matcher;
    }
}

function getGetParameters(req) {
    var get = {};
    var idx;

    if(idx = req.url.indexOf('?')) {
        var paramList = req.url.substring(idx+1);
        if(paramList.indexOf('&')) {
            paramList.split('&').forEach((param) => {
                get[param.split('=')[0]] = decodeURIComponent(param.split('=')[1]);
            });
        } else {
            get[paramList.split('=')[0]] = decodeURIComponent(paramList.split('=')[1]);
        }
    }

    return get;
}

const server = http.createServer((req, res) => {
    const get = getGetParameters(req);

    /*
        /compatibility
        Print the form to create the game
    */
    if(req.url == '/compatibility') {
        fs.readFile('create.html', 'utf8', (err, data) => {
            if(err) {
                res.statusCode = 500;
                console.log('Unable to read create.html');
                res.end('Server error');
            } else {
                res.statusCode = 200;
                res.end(data);
            }
        });
    }
    /*
        /compatibility/create
        Create a new game and print the link to give to the players
    */
    else if(req.url == '/compatibility/create') {
        var body = '';
        req.on('data', (chunk) => {
            body += chunk;
        });

        req.on('end', () => {
            var players = decodeURIComponent(body.split('=')[1]);
            if(!games[players]) {
                games[players] = new Game(players);
            }
            fs.readFile('play.tmpl', 'utf8', (err, data) => {
                if(err) {
                    res.statusCode = 500;
                    console.log('Unable to read play.tmpl');
                    res.end('Server error');
                } else {
                    res.statusCode = 200;
                    res.end(data.replace(/<PLAYERS>/g, players));
                }
            });
        });
    }
    /*
        /compatibility/start
        Print one link for each players in the game
    */
    else if(req.url.startsWith('/compatibility/start')) {
        var links = '';
        get["players"].split(';').forEach((player) => {
            links += '<p><a href="/compatibility/game?player='+player+'&amp;players='+encodeURIComponent(get["players"])+'">'+player+'</a></p>';
        });

        fs.readFile('start.tmpl', 'utf8', (err, data) => {
            if(err) {
                res.statusCode = 500;
                console.log('Unable to read start.tmpl');
                res.end('Server error');
            } else {
                res.statusCode = 200;
                res.end(data.replace(/<LINKS>/g, links));
            }
        });
    }
    /*
        /compatibility/game
        Load the HTML page corresponding to the player
    */
    else if(req.url.startsWith('/compatibility/game')) {
        var file;
        if(get['master'] == 1) {
            file = 'master.html';
        } else {
            file = 'player.html';
        }

        fs.readFile(file, 'utf8', (err, data) => {
            if(err) {
                res.statusCode = 500;
                console.log('Unable to read ' + file);
                res.end('Server error');
            } else {
                if(get['master'] != 1) {
                    // This is a player
                    // We need to replace <IMAGES>

                    var html = '';

                    for(i = 1; i < 53; i++) {
                        html += '<img src="img/' + i.toString() + '.jpg" />';
                    }

                    res.end(data.replace(/<IMAGES>/g, html));
                } else {
                    res.statusCode = 200;
                    res.end(data);
                }
            }
        });
    }
    /*
        /compatibility/load
        Returns the Game object
    */
    else if(req.url.startsWith('/compatibility/load')) {
        res.statusCode = 200;
        res.setHeader('Content-Type', 'application/json');
        res.end(JSON.stringify(games[get['players']]));
    }
    /*
        /comptatibility/connect
        A new player is connecting
    */
    else if(req.url.startsWith('/compatibility/connect')) {
        if(games[get['players']].players[get['player']] == -1) {
            games[get['players']].players[get['player']] = 0;

            var allConnected = 1;

            for(const [player, score] of Object.entries(games[get['players']].players)) {
                if(score == -1) {
                    allConnected = 0;
                    break;
                }
            }

            if(allConnected) {
                games[get['players']].state = "waitingWord";
            }
        }
        res.statusCode = 200;
        res.end();
    }
    /*
        /compatibility/newWord
        Start a new round with a new word
    */
    else if(req.url.startsWith('/compatibility/newWord')) {
        var currentGame = games[get['players']];
        if(currentGame.state != "waitingWord" && currentGame.state != "computingResults") {
            res.statusCode = 500;
            console.log("Bad /compatibility/newWord");
            console.log(req);
            res.end();
        } else {
            var data = currentGame.newWord();
            res.statusCode = 200;
            res.setHeader('Content-Type', 'text/plain');
            res.end(data);
        }
    }
    else if(req.url.startsWith('/compatibility/send')) {
        var currentGame = games[get['players']];
        currentGame.send[get['player']] = get['images'];

        if(currentGame.state == 'waitingPlayers' && Object.keys(currentGame.send).length == Object.keys(currentGame.players).length) {
            currentGame.state = 'computingResults';

            const matcher_images = currentGame.send[currentGame.matcher];

            for(const [player, images] of Object.entries(currentGame.send)) {
                if(player != currentGame.matcher) {
                    images.split('-').forEach((image, idx) => {
                        matcher_images.split('-').forEach((matcher_image, matcher_idx) => {
                            if(image == matcher_image) {
                                if(idx == matcher_idx) {
                                    currentGame.players[player] += 3;
                                } else {
                                    currentGame.players[player] += 2;
                                }
                            }
                        });
                    });
                }
            }
        }

        res.statusCode = 200;
        res.end();
    }

    /*
        Load *.js file
    */
    else if(req.url.startsWith('/compatibility/') && req.url.endsWith('.js')) {
        var file = req.url.replace(/\/compatibility\//, '');
        fs.readFile(file, 'utf8', (err, data) => {
            if(err) {
                res.statusCode = 500;
                console.log('Unable to read ' + file);
                res.end('Server error');
            } else {
                res.statusCode = 200;
                res.end(data);
            }
        });
    }
    /*
        Load *.img file
    */
    else if(req.url.startsWith('/compatibility/img/') && req.url.endsWith('.jpg')) {
        var file = req.url.replace(/\/compatibility\//, '');
        fs.readFile(file, (err, data) => {
            if(err) {
                res.statusCode = 500;
                console.log('Unable to read ' + file);
                res.end('Server error');
            } else {
                res.statusCode = 200;
                res.end(data);
            }
        });
    }
    else {
        res.statusCode = 404;
        res.end('Not found');
    }

});

server.listen(port);
  • master.js:
/*
    getUrlParameter
        Return the corresponding HTTP GET parameter.
*/
var getUrlParameter = function getUrlParameter(sParam) {
    var sPageURL = window.location.search.substring(1),
        sURLVariables = sPageURL.split('&'),
        sParameterName,
        i;

    for (i = 0; i < sURLVariables.length; i++) {
        sParameterName = sURLVariables[i].split('=');

        if (sParameterName[0] === sParam) {
            return sParameterName[1] === undefined ? true : decodeURIComponent(sParameterName[1]);
        }
    }
};

var refreshInterval;

/*
    refreshGame
        Function called every seconds to update the data of the game
*/
function refreshGame() {
    $.get("/compatibility/load", { players: getUrlParameter('players') }).done( game => {
        $('#load').hide();
        if(game['state'] == 'starting') {
            var html = '';
            for(const [player, score] of Object.entries(game['players'])) {
                html += '<p>' + player + ' : ' + ( score == -1 ? 'Not connected' : 'Connected' ) + '</p>';
            }
            $('#starting').html(html);
        } else if(game['state'] == 'waitingWord') {
            clearInterval(refreshInterval);
            $('#starting').hide();

            var html = '';
            for(const [player, score] of Object.entries(game['players'])) {
                html += '<p>' + player + ' : ' + score.toString() + '</p>';
            }

            $('#ranking').html(html);
            $('#newWord').show();
        } else if(game['state'] == 'waitingPlayers') {
            var html = '';
            for(const [player, images] of Object.entries(game['send'])) {
                html += '<p>' + player + ' : ' + images + '</p>';
            }
            $('#results').html(html);
        } else if(game['state'] == 'computingResults') {
            var html = '';
            for(const [player, images] of Object.entries(game['send'])) {
                html += '<p>' + player + ' : ' + images + '</p>';
            }
            $('#results').html(html);

            html = '';
            for(const [player, score] of Object.entries(game['players'])) {
                html += '<p>' + player + ' : ' + score.toString() + '</p>';
            }
            $('#ranking').html(html);

            $('#newWord').show();
            $('#game').show();
            clearInterval(refreshInterval);
        }
    });

}

/*
    newWord
        Ask to the server a new word
*/
function newWord() {
    $.get("/compatibility/newWord", { players: getUrlParameter('players') }).done ( data => {
        $('#word').text(data.split('#')[0]);
        $('#matcher').text(data.split('#')[1]);
        $('#game').show();
        $('#newWord').hide();
        $('#results').html('');
    });

    refreshInterval = setInterval(refreshGame, 1000);
}

/*
    loadGame
        Get the informations of the current game to the server and display them.
        This function is launched only once at the loading of the page
*/
function loadGame() {
    refreshInterval = setInterval(refreshGame, 1000);
}
  • master.html:
<html>
    <head>
        <meta charset="UTF-8">
        <script type="text/javascript" src="jquery-3.5.1.min.js"></script>
        <script type="text/javascript" src="master.js"></script>
    </head>
    <body onLoad="loadGame()">
        <div id="load">
            <p>Loading...</p>
        </div>
        <div id="starting"></div>
        <div id="ranking"></div>
        <div id="newWord" style="display: none">
            <input type="button" value="Next word" onclick="newWord()" />
        </div>
        <div id="game" style="display: none">
            <p>Current word: <b><span id="word" /></b>
            <p>Matcher: <b><span id="matcher" /></b>
            <div id="results"></div>
        </div>
    </body>
</html>
  • player.js
/*
    getUrlParameter
        Return the corresponding HTTP GET parameter.
*/
var getUrlParameter = function getUrlParameter(sParam) {
    var sPageURL = window.location.search.substring(1),
        sURLVariables = sPageURL.split('&'),
        sParameterName,
        i;

    for (i = 0; i < sURLVariables.length; i++) {
        sParameterName = sURLVariables[i].split('=');

        if (sParameterName[0] === sParam) {
            return sParameterName[1] === undefined ? true : decodeURIComponent(sParameterName[1]);
        }
    }
};

var refreshInterval;

/*
    waiting
        Function called every seconds when waiting something from the server
*/
function waiting() {
    $.get("/compatibility/load", { players: getUrlParameter('players') }).done( game => {
        if(game['state'] == 'starting') {
            $('#results').hide();
            if(game['players'][getUrlParameter('player')] == -1) {
                $.get("/compatibility/connect", { players: getUrlParameter('players'), player: getUrlParameter('player') }).done((data) => {
                });
            }
            $('#starting').show();
        } else if(game['state'] == 'waitingWord') {
            $('#results').hide();
            $('#starting').hide();
            $('#waiting').show();
        } else if(game['state'] == 'waitingPlayers') {
            $('#results').hide();
            if(!game['send'][getUrlParameter('player')]) {
                $('#waiting').hide();
                $('#word').text(game['word']);
                $('#matcher').text(game['matcher']);
                $('#game').show();

                clearInterval(refreshInterval);
            } else {
                $('#game').hide();
                $('#starting').show();
            }
        } else if(game['state'] == 'computingResults') {
            $('#starting').hide();
            $('#waiting').hide();
            
            var html = '';
            for(const [player, images] of Object.entries(game['send'])) {
                html += '<p>' + player + ' :</p>';// + images + '</p>';
                images.split('-').forEach((image) => {
                    html += '<img src="img/'+image+'.jpg" />';
                });
            }

            for(const [player, score] of Object.entries(game['players'])) {
                html += '<p>' + player + ' : ' + score.toString() + '</p>';
            }

            $('#results').html(html);
            $('#results').show();
        }
    });
    
    $('#load').hide();
}

/*
    sendImages
        Send the list of images to the server.
*/
function sendImages() {
    var images = [];

    for(i = 1; i < 6; i++) {
        var image = $('#image' + i.toString()).val();

        if(image == '') {
            alert("You have to fill all the fields");
            return;
        }

        if(!/^[0-9]+$/.test(image)) {
            alert(image + " is not a number");
            return;
        }

        var n = parseInt(image);

        if(n < 1 || n > 52) {
            alert(image + " should be between 1 and 52");
            return;
        }

        if(images.includes(n)) {
            alert(image + " is used multiple times");
            return;
        }

        images.push(n);
    }
    var images_text = images.join('-');
    console.log(images_text);
    $.get("/compatibility/send", { players: getUrlParameter('players'), player: getUrlParameter('player'), images: images_text }).done((data) => {
        $('#game').hide();
        $('#starting').show();
        refreshInterval = setInterval(waiting, 1000);
    });
}

/*
    loadGame
        Get the informations of the current game to the server and display them.
        This function is launched only once at the loading of the page
*/
function loadGame() {
    waiting();
    refreshInterval = setInterval(waiting, 1000);
}
  • player.html
<html>
    <head>
        <meta charset="UTF-8">
        <script type="text/javascript" src="jquery-3.5.1.min.js"></script>
        <script type="text/javascript" src="player.js"></script>
    </head>
    <body onLoad="loadGame()">
        <div id="load">
            <p>Loading...</p>
        </div>
        <div id="starting" style="display: none;">
            <p>Waiting for the other players</p>
        </div>
        <div id="waiting" style="display: none;">
            <p>Waiting for a new word</p>
        </div>
        <div id="game" style="display: none;">
            <p>Current word: <b><span id="word"></span></b></p>
            <p>Matcher: <b><span id="matcher"></span></b></p>
            <p>Ordered list of the five images corresponding to the word:</p>
            <input type="text" id="image1" />
            <input type="text" id="image2" />
            <input type="text" id="image3" />
            <input type="text" id="image4" />
            <input type="text" id="image5" />
            <input type="button" value="Send" onclick="sendImages()" />
            <div id="image_list">
                <IMAGES>
            </div>
        </div>
        <div id="results" style="display: none;">
        </div>
    </body>
</html>
  • start.tmpl
<html>
    <head>
        <meta charset="UTF-8">
    </head>
    <body>
        <LINKS>
    </body>
</html>
  • play.tmpl
<html>
    <head>
        <meta charset="UTF-8">
    </head>
    <body>
        <p><a href="/compatibility/start?players=<PLAYERS>">Link of the game</a></p>
        <p><a href="/compatibility/game?master=1&amp;players=<PLAYERS>">Link for the master of the game</a></p>
    </body>
</html>
  • create.html
<html>
    <head>
        <meta charset="UTF-8">
    </head>
    <body>
        <form action="compatibility/create" method="POST">
            <p>Name of the players (separated by ';') :</p>
            <input type="text" name="players" />
            <input value="Create" type="submit" />
        </form>
    </body>
</html>
\$\endgroup\$
2
\$\begingroup\$

I'll start out by doing some a general code-quality, just with your app.js. Then I'll go over some of the other questions you had.

app.js code quality suggestions

Let's start with this chunk:

var words = [];

fs.readFile('dico.txt', 'utf8', (err, data) => {
    words = data.split('\n');
});

While often asynchronous versions of a function is preferred, they're not always necessary (like when the server is starting up). In this particular case you have a minor race condition - the words array could technically get accessed from an endpoint before disco.txt gets read. You can just use readFileSync(), it'll make the code a little more concise too:

const words = fs.readFileSync('disco.txt', 'utf8');

With this chunk of code:

var idx = Math.floor(Math.random() * Math.floor(this.remainingWords.length));
this.word = this.remainingWords.splice(idx, 1)[0];

There's no reason to floor the length of the array, you can take that function call out. Then, I would move this kind of thing out into a little helper function at the top of the file, so you could write this instead as this.word = removeRandomListEntry(this.remainingWords) - it makes functions like newWord() read better when it's content focuses just on the task their trying to perform instead of lower-level details. See if you can find other places in your code where splitting out helper functions can make it more readable (like a function that'll collect the body of a request for you, so you don't have to do all that boilerplate work in the middle of an endpoint handler).

newWord() is returning the chosen word with matcher joined by a "#". Why not just return an object literal instead? return { word: this.word, matcher: this.matcher }. One of your API endpoints returns that same joined string, you can instead have it return this object as a JSON string also.

this.matcher_id is never the id for this.matcher - that's misleading. Maybe you were wanting to call it this.next_matcher_id? In general, there's a number of variables that I can't figure out what they do without looking at how they're used, which means their names aren't doing a very good job of explaining their purpose. Naming is one of the trickier parts to coding, but doing a good job can make a huge difference.

Your getGetParameters() (and getUrlParameter() from master.js) can be replaced by using URLSearchParams() and/or URL(). Some example usage:

> params = new URLSearchParams('?a=2&b=3')
URLSearchParams { 'a' => '2', 'b' => '3' }
> params.get('a')
'2'
> Object.fromEntries(params)
{ a: '2', b: '3' }
> url = new URL('https://example.com?a=2&b=3') // If you have a full URL, use the URL constructor instead
URL {
  ...
}
> url.searchParams.get('a') // url.searchParams is an instance of URLSearchParams()
'2'

I would recommend making your request handler callback into an async function, then using promise-based APIs instead of callbacks (i.e. use fs.promises instead of fs).

For example:

const server = http.createServer(async (req, res) => {
    ...
    if(req.url == '/compatibility') {
        let data;
        try {
            data = await fs.promises.readFile('create.html', 'utf8');
        } catch (err) {
            res.statusCode = 500;
            console.log('Unable to read create.html');
            res.end('Server error');
            return;
        }
        res.statusCode = 200;
        res.end(data);
    }
    ...
}

This isn't super important, but it does read nicer, and it will help with my next tip: Be careful with your status codes. 5xx errors are mainly used for unexpected server errors. If the error is the client's fault, use a 4xx error. I would just wrap your entire request handler callback in a giant try-catch, and in the catch, log the error and return a 500 status code. You shouldn't need to use 500 anywhere else. (Using await makes this even better as it allows the catch to catch asynchronous errors - something that can't be done as easily when using callbacks).

For example:

const server = http.createServer(async (req, res) => {
    try {
        await requestHandler(req, res);
    } catch (err) {
        res.statusCode = 500;
        console.error(error.message);
        console.error(error.stack);
        res.end('Server error');
    }
}

async function requestHandler(req, res) {
    ...
    if(req.url == '/compatibility') {
        const data = await fs.promises.readFile('create.html', 'utf8');
        res.statusCode = 200;
        res.end(data);
    }
    ...
}

Notice how when reading the file we no longer need to specifically handle what happens if create.html is missing? It really shouldn't ever be missing, it's part of the repo, but if it was, then that's a fatal error, and we'll just let the general try-catch handle the issue.

Lets look at some of the other places where you use the 500 code:

  • In /compatibility/newWord, it the endpoint is used at the wrong time (i.e. before the game has started), that should be a 400 error - it's the client that's misusing the endpoint, there's nothing wrong with the server.
  • In /compatibility/<file.js> and /compatibility/img/<image>, if the resource is missing, it should be a 404 error - again, it's the client's fault for trying to access a non-existent resource, the server hasn't done anything wrong.

Some other miscelaneous items:

  • prefer === over ==, "==" has some magical behavior, which is why javascript authors introduced "===" to replace it.
  • You may want to take a look into const and let, many argue that var shouldn't be used anymore.
  • inconsistant naming: Sometimes you use snake_case for variables names, other times its camelCase. You'll most commonly find javascript users using camelCase (and often endpoints are done with kebab-case.)

Answering other questions

Route management: in my code, app.js parses the request URL to determine which page it should respond. I'm wondering how it is possible to do it better.

If the project is small enough and the endpoints are simple enough, I actually don't have a problem with the way you did it. A more scaleable solution would be to keep your if-then chain, but make the only thing inside the if block a call to some other function that has the actual logic for handling the request. Then place these functions into one or more "controller" files. You could do more to make things even more scaleable, but then you're getting into the territory of reinventing existing frameworks, like express, that have a nicer system in place to handle routing for you.

I do want to note that your route-matching logic is too lenient. You're often using req.url.startsWith('/my/route'), which will cause your if-then to match against routes like '/my/route/x/y/z'. At the top of your function, you can do something like the following:

const path = req.url.split('?')[0].split('#')[0]

Then, you can just do path === '/my/route' in your "if" conditions. You may also want to remove trailing slashes from the path too.

Client/Server workflow: the clients of the application, when they are waiting for an event from the server, send a request every second. Does it exist something to prevent that?

In some cases, polling works just fine, like you're doing. But, what you're looking for is either websockets (which allow for two-way communication) or server events (which only allows one-way communication from server to client). Either one would do the trick.

Template management: I created some .tmpl file that app.js will uses to produce dynamic HTML files. Maybe something more efficient exists.

When coding up a website, you can either design it to have a "thin client" or a "thick client", which both have pros and cons.

  • In a thin client (what you have), most of the code gets run on the server (within node, not the browser). node uses a template to generate a page that gets returned to the browser. A really thin client can have little-to-no javascript running in the browser, making all interactions with the server happen through page loads and html forms. Coding like this can sometimes be simpler (depending on what you're doing), but can also be limiting.
  • A thick client means all the logic to build a view lives in the javascript that runs in the browser. The server will never return HTML, instead, it exposes a number of API endpoints that the client can interact with (through fetch()) to gather all the data it needs to build a view. Then, it uses DOM manipulation functions (like document.createElement()) to modify the page and show the data it fetched. These have more power, especially when dealing with page transitions (you don't have to load an entire new page, just the new content that's needed).

If you prefer to stick with a thin client, then I would recommend the following changes:

  • You're never escaping any data that you put into your template. If all the data is trusted, then that's fine, but if not, you'll need to make sure you follow these escaping rules that were put together to help prevent XSS attacks. (Templating languages like PHP come with native escaping functions to help out. Node doesn't have those, so you'll have to find some hand-made ones online). Note that this whole escaping business is common practice in thin client websites, but it's also risky - one missup and you have a vunerability, which is why it's as dangourous as the often discouraged use of setting .innerHTML. One safer alternative would be to use a templating system like Handlebars that'll take care of escaping for you.
  • This isn't necessasry, but I would separate API endpoints (endpoints that retrieve data and do business logic) from view endpoints (endpoints that return HTML). You can put all API endpoints under api/ (e.g. /compatibility/api/connect or /api/connect), and keeping your UI endpoints where they are. In your request handler callback, you can cause it to either call a handleApiRequest() callback or handleUiRequest() callback, depending on if the path started with api/ or not. Such a separation would then let you do things such as making the UI endpoints can give back 500 or 404 errors in HTML while the API endpoints give back JSON or no content.

You may wish to change to a thick client. This would mean:

  • You would only have API endpoints, no endpoints will ever serve server-generated HTML. (though, you will still need to serve some static HTML files)
  • It'll be easier to defend against XSS attacks as you won't have to worry about properly escaping strings before inserting them into a template (as you can just build the UI with native DOM functions). Though, you could still use a templating system like handlebars on the client to generate your HTML, which handles the escaping for you.

I'm also interested by every reviews about security (how to not cheat, input validation,...)

In your generic endpoints to load *.js or image files, you're not escaping the path used at all. So, there's nothing to stop someone from using a path like /compatibility/../../path/to/private/data.txt to load any file on the file system that node has permission to access. Only allow the user to access what you want them to access and nothing more. Try putting such files into a public/ directory and have your generic compatibility/*.js endpoint only load files from public/, and make sure it only allows characters such as letters and numbers for the file name (no slashes).

I haven't looked too closely at other areas of the app, but here's some things I would look for:

  • Is it possible for a user to upgrade themselves to a master by modifying the URL given to them to have "master=1" in the query parameter? Or to another player by just putting that player's name in the URL? Would they get caught?
  • You rarely escape strings before inserting them into a template - there's likely some potential XSS attacks in there. Is it possible for a master to construct a very special URL to give to other players, that allows them to take control of the other player's machines? i.e. what if the players GET parameter was set to be player1;player2;masterPlayer<script>doEvilStuff()</script> - is it possible to then distribute URLs to other players that bring them to the game, but with the master running arbitrary logic on their machines? This kind of thing matters more when the page might have private information that the client would not want to get leaked, or auth cookies, etc, but it's still important to prevent this kind of behavior.

locale management

I don't have too much experience in this area, but I know how WordPress handles it, and I know how that might get interpreted to javascript. You can of course look up articles if you wish to dive into this more yourself.

But the basic idea is to have JSON files for each language you want to support, which maps a unique tag to a phrase that gets used in the interface.

For example, you might have a languages/en-us.json with the following:

{
  "linkToGame": "Link of the game",
  "serverError": "Server error",
  ...
}

Then, whenever you might hard-code a string that eventually gets shown to the user (in error messages, template files, etc), you lookup the message from a loaded language file. e.g.

res.end(lang.serverError)

(assume lang contains one of the language mapping files)

Conclusion

Whoops, this ran on a little long. Don't feel like you need to do all these changes to your current project, except for maybe some of the security issues. Instead, think of these as concepts that you can apply to your future projects. Hopefully this can give you some ideas of how you might improve your craft.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.