Node JS routing system

Question

I am doing a website for an association and I have never done the server side before. Here I made a very simple routing system:

app.set('port',(process.env.PORT || 5000));


app.use(express.static(__dirname + '/public' ));
app.use(express.static(__dirname + '/views' ));


app.get('/',function(req,res,next) {
    res.redirect('/en/insa');
})

.get('/:language/:page?',function(req, res, next){

    var path = __dirname+'/views/'+req.params.language+'/'+req.params.page+'.ejs';

    fs.access(path,function(err) {
        if (err) {
            res.status(404);
            console.error('404 : /' + req.params.page);
            res.render('404.ejs', {
                page: req.params.page
            }); 
            return; 
        }
        res.charset = "utf-8";

        res.render(path ,{language: req.params.language});
    });
})


.listen(app.get('port'),function() {
    console.log('Server is running, server is listening on port ',app.get('port'));
})

I just redirect automatically the client if he hits the main www.site.com/ page and then I just check for the language and page in the URL like www.site.com/en/description for instance. The fs.access function checks if the EJS (that's a template generator for HTML pages) page actually exists in the folder before rendering it.

Is there anything that could go wrong with this code, or something that could be done better?

Answer 1

It seems a little risky to me to just blindly use the language designation that the URL contains. If it's possible to trick Express into looking somewhere in your file system besides where you expect (perhaps with dots in the path), then you could have a security issue. It would be safer if you checked to see if it was one of a list of known-safe designations ("en", "fr", "ge", "sp", etc...). If you wanted, you could build a list of known safe designations by looking in the the file system at server startup and then just check it against that.

I would also recommend using path.join() instead of adding in the path separator yourself. That will make your code more platform friendly should you ever want to run it on Windows.

Answer 2

Definitely do not create the URL that way without sanitizing the input. You can a package like sanitize-filename to prevent malicious users from messing with things on your server.

There are a few ways you can address this:

1. Have a list of available language and page files to check against.

   const LANGUAGES = ['en', 'fr', ...];
   if (LANGUAGES.includes(req.params.language)) {
       res.render(...)
   } else {
       res.status(404).render('404', { ... });
   }
   

2. try/catch to render the page.

   const sanitize = require('sanitize-filename');
   
   var sanitizedPath = sanitize(path.join(__dirname, 'views', ...));
   try {
       res.render(sanitizedPath, {
           ...
       });
   } catch (e) {
       if (/* template not found error */) {
           res.status(404).render('404', { ... });
       } else {
           // Handle other exception
       }
   }
   

The first option is preferable.