0
\$\begingroup\$

I'm coding a simple websocket chat app. I was wondering if my code is secure and efficient. I also have a question about the way how I'm sending objects and then checking the type, is there anything better/efficient I can do? Such as making my own events?

Please tell me any improvements I can do.

This is the chat.js (client side script):

I'm most concerned about this line: ws.send(JSON.stringify({type:"connectBoth", senderid:res.receiverid, receiverid:res.senderid}))

There is no direct input into that line, but can't they just open console and go execute that function themselves? With their own parameters they choose, and thus result in a connection with somoene that didn't agree?

let id = document.getElementById("chatid");
function copyID(){
    navigator.clipboard.writeText(id.innerHTML);

    alert("Copied!");
}


(function() {
    const sendBtn = document.getElementById("send");
    const messages = document.getElementById("chatModule");
    const messageBox = document.getElementById("message");
    const friendIDBox = document.getElementById("friend-id");
    const connectBtn = document.getElementById("connect");
    const connStatus = document.getElementById("conn-status");

    let ws;

    

    function showMessage(message, side){
        var newMessage = document.createElement("div");
        if(side == "sender"){
            newMessage.setAttribute("class", "sendermessage");
        }
        else if(side == "receiver"){
            newMessage.setAttribute("class", "receivermessage"); 
        }
        newMessage.textContent = message;
        messages.appendChild(newMessage);
        messageBox.value = '';
    }

    function init(){
        if(ws){
            ws.onerror = ws.onopen = ws.onclose = null;
            ws.close();
        }

        ws = new WebSocket('ws://localhost:8080/?token=2121');
        ws.onopen = () => {
            console.log('Connection opened!')
        }
        // ws.onmessage =  ({ data }) => showMessage(data, "receiver");
        ws.onmessage = object => {
            res = JSON.parse(object.data);
            console.log(res);
            switch(res.type){
                case "id":
                    id.innerText = res.data;
                break;
                case "connectionPrompt":
                    let agree = confirm("Do you want to connect with: " + res.senderid);
                    if(agree){
                        ws.send(JSON.stringify({type:"connectBoth", senderid:res.receiverid, receiverid:res.senderid}))
                        connStatus.innerText = "Connection status: Connected to " + res.senderid;
                    }
                break;
                case "connAccepted":
                    connStatus.innerText = "Connection status: Connected to " + res.senderid;
                break;
                case "msg":
                    showMessage(res.msg, "receiver");
                break;

            }
        } 
        ws.onclose = function(){
            ws = null;
        }
    }

    sendBtn.onclick = () => {
        if(!ws){
            showMessage("No WebSocket connection!");
            return;
        }

        ws.send(JSON.stringify({type:"msg", data:messageBox.value}));
        showMessage(messageBox.value, "sender");
    } 

    connectBtn.onclick = () => {
        ws.send(JSON.stringify({type:"connectRequest", data:friendIDBox.value}));
        connStatus.innerText = "Connection status: Pending..."
        alert("Request sent!");
    }

    

    init();
})();

This is the server.js (server side-script)

const express = require('express');
const { json } = require('express/lib/response');
const http = require('http');
const WebSocket = require('ws');
//Web server socket = wss
const port = 8080;
const server = http.createServer(express);
const wss = new WebSocket.Server({ server });




function heartbeat() {
    this.isAlive = true;
}

function generateID(){
    return Math.floor((1 + Math.random()) * 0x10000).toString(16).substring(1);
}

wss.getUniqueID = () => {
    return generateID() + generateID() + generateID();
}
    


wss.on("connection", function connection(ws){
    ws.id = wss.getUniqueID();
    ws.send(JSON.stringify({type:"id", data: ws.id}));
    console.log("New client connected with id: " + ws.id);
    ws.isAlive = true;
    ws.on('pong', heartbeat);
    ws.on("message", function incoming(data, isBinary){
        let message = isBinary ? data : data.toString();
        message = JSON.parse(message);
        console.log(message);
        switch(message.type){
            case "connectRequest":
                wss.clients.forEach(client => {
                    if(client.id == message.data)
                    client.send(JSON.stringify({type:"connectionPrompt", senderid:ws.id, receiverid:message.data }))
                })
                // ws.friendid = message.data;

            break;
            case "connectBoth":
                ws.friendid = message.receiverid;
                wss.clients.forEach(client => {
                    if(client.id == message.receiverid){
                        client.friendid = message.senderid;
                        client.send(JSON.stringify({type:"connAccepted", senderid:message.senderid}))
                    }
                })

            break;
            case "msg":
                wss.clients.forEach(client => {
                    if(client != ws && client.readyState == WebSocket.OPEN){
                        console.log("Client id: " + client.id + " and " + client.friendid);
                        if (client.id == ws.friendid){
                            client.send(JSON.stringify({type:"msg", msg:message.data}));
                        }
                   }
                    
                })
            break;
            
        }
        
        
        
        // const message = isBinary ? data : data.toString();
        // wss.clients.forEach(function each(client){
        //     if(client != ws && client.readyState == WebSocket.OPEN){
        //         client.send(message);
        //     }
        // })
    })
    

})

const interval = setInterval(function ping() {
    wss.clients.forEach(function each(ws) {
      if (ws.isAlive === false) return ws.terminate();
  
      ws.isAlive = false;
      ws.ping();
    });
  }, 1000);
  

  wss.on('close', function close() {
    clearInterval(interval);
    console.log("Connection lost!");
  });
  

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

Thanks, Kind Regards!

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

There is no direct input into that line, but can't they just open console and go execute that function themselves?

Yes, but if I was a malicious actor, I would just take your websocket url and build my own script based off of yours.

How can you stop this?

You can't.

But you can make it more complex to use to dissuade malicious actors.


Before we get into how to make things more complex, lets first ask the question.

"What are the potential damages a malicious actor can do?"

  1. ✅ You're not using a database... so no sql/no-sql injections are possible
  2. ⚠️ Users don't seem to be authenticated... A user could potentially create an infinite amount of websocket connections.
  3. ⚠️ Users don't seem to be rate limited... A user could poetentially send an infinite amount of messages within a short amount of time.
  4. ⚠️ Messages are forwarded to all clients and inserted in DOM, potential XSS (if innerHTML is used)

Require users to be authenticated

  1. Require users to have an account
  2. Generate a webtoken to be used to authenticate the user for the initial websocket handhake
  3. Set rate limit per account, to ensure that they cannot brute force a hack

This will allow you better identify a user and (if necessary) to properly ban/suspend them (forcing them to create a new email address, get verified, etc).


XSS attacks

You seem to be using textContent which is already a good habit and will allow avoiding XSS attacks. Definitely do not use innerHTML. However, if you would like users to write formatted text.. consider, sanitizing incoming messages.

\$\endgroup\$
6
  • \$\begingroup\$ Thannks, what else do you think about the code? You only think I should add authentication and more basic security like rate limits, ...? You also said you could build your own script, and alter the code, the thing is that it will only work if the code is accepted by the server, so isn't there anything additional I can do to make that line extra secure? Thanks! \$\endgroup\$
    – Jiry_XD
    Jun 30, 2022 at 20:07
  • \$\begingroup\$ Yes, I'd build my own script to connect to your server via websocket conection you're exposing in your javascript file. I would then send different kinds of payloads to see how your server reacts. Will it work? I don't know till it's tried.. but my point is.. you can't really stop people from doing that. You can try and limit it.. for example by generating a temporary token (server side) and placed in your HTML. You could use the token and use the same origin checks to ensure that the user is attempting to connect through your script. \$\endgroup\$ Jul 1, 2022 at 8:56
  • \$\begingroup\$ "what else do you think about the code?" - I think your code is inconsistent with modern standards of javascript. For example, we don't really use this in a function anymore. I probably would have written it differently, but it's important to have something that works initially. I think the concept of using type to differentiate the payloads is a good idea. \$\endgroup\$ Jul 1, 2022 at 9:00
  • \$\begingroup\$ "by generating a temporary token (server side) and place in your HTML. You could use the token and use the same origin checks to ensure that the user is attempting to connect through your script." - So you mean if I give each user a secret token that only they and the server know, I could only let them connect if they know the key and it compares rightly so with the server. Because as of right now anyonne could just copy the code I wrote there, input the id of another user they know and connect, and that is what concerns me. Also what did you mean with placed in your HTML, hidden in there or? \$\endgroup\$
    – Jiry_XD
    Jul 1, 2022 at 13:43
  • \$\begingroup\$ @Jiry_XD this token would work similar to a CSRF token used in forms: stackoverflow.com/questions/5207160/… The token just increases the complexity of creating a valid connection. It's just an "extra" step. The whole point is simply to dissuade malicious users to try and hack by adding more and more steps. That's it. You can be quite creative with this. \$\endgroup\$ Jul 1, 2022 at 15:05

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.