0

I am trying to make a TS3 bot with Node.js Library from Multivit4min (github) The bot should do this: (Here's table: nick (txt)| cldbid (int)| clid(txt) | lastChannel (int) btw. clid which is client id is supposed to be unique ID of client)

Event #1 - client connects to a server:

  • Check if record with client.UID exists in database: yes: update its lastChannel to event.channel.channelId no: insert client.nick,cldbid,UID and event.channel.channelId

Event #2 - client moves to another channel:

  • Update lastChannel to event.channel.channelId

Event #3 - client moves to a specific channel:

  • search record by his UID and save his lastChannel into a variable

Generally I know how to do parts with Node but I struggle with SQL because all the time it shows some errors or doesn't work as it should.

I tried many queries but none of them worked. The best I have done is some code from other thread here on stack that was 'REPLACE' and it should automatically check if it exists and then update or if doesn't exist - then insert. But always it was creating more and more copies of a record.

Here's some of my code:

ts3.on("clientconnect", event => {
    
        sql.query("SELECT * FROM `users` WHERE `clid`= " + event.client.getUID(), 

        function(err, result){
            if(err){
                console.log(time.get() + "Error SQL: " + err)
            }else{
                if(row && row.length){
                    sql.query("REPLACE INTO users (nick, cldbid, clid, lastChannel) VALUES ('" + event.client.nickname + "','" + event.client.getDBID() + "','" + event.client.getUID() + "','" + config.lastChannel + "');",
                    function(err,result){
                        if (err){
                            console.log(time.get()+err)
                        }else{
                            console.log(time.get() + `Dodano użytkownika ${event.client.nickname} do bazy danych`)
                        }
                    })
                }
            }
        })

})

I don't really want you to create whole code for me. I am just asking for SQL queries with eventual "ifs" because as you can see, my 4th idea (I said earlier I was trying many ways) was based on SELECT and depending on a result, UPDATE or INSERT a record.

Feel free to ask about vars, code etc., I got whole night ..


I came up with code like here:

sql.query("INSERT INTO users (nick,cldbid,clid,lastChannel) VALUES('"+event.client.nickname+"',"+event.client.getDBID()+",'"+event.client.getUID()+
        "',"+config.lastChannel+") ON DUPLICATE KEY UPDATE lastchannel="+event.channel.cid+";", 

        function(err, result){
            if(err){
                log.clog("Blad SQL #clientmoved: " + err)
            }else{
                log.clog("Pomyslnie addded/updated record of "+event.client.nickname)
            }
        })

And now I need help how to save 1 parameter from column x of a record to a variable. (int)

Samuel Liew
  • 76,741
  • 107
  • 159
  • 260
Janekk
  • 15
  • 1
  • 4
  • 1
    ON DUPLICATE KEY UPDATE is what you are looking for. Besides, take a look at prepared statements. You are leaving a giant sql injection hole here. – glee8e Jul 19 '19 at 22:16
  • Possible duplicate of [Insert into a MySQL table or update if exists](https://stackoverflow.com/questions/4205181/insert-into-a-mysql-table-or-update-if-exists) – glee8e Jul 19 '19 at 22:17
  • Actually it's almost impossible because maximum nickname on teamspeak is not really long. Most of users doesn't even know about SQL Injections. Only like 1% of users can use it but I don't think they would try nicknames just to drop database that isn't critical error in my code, not even minor error because I will use try{}catch(){} to prevent errors with SQL. – Janekk Jul 19 '19 at 22:19
  • 2
    Dropping the database is by far not the worst thing the sql injection could do, it's actually one of the milder outcomes. Theft of sensitive user data and arbitrary code execution on the server (yes, this is also possible) is what you should fear. As for `REPLACE INTO`, it requires a unique key on the column you are monitoring. – Timekiller Jul 19 '19 at 22:54
  • The data there is public, everyone on ts server can check it and also even if somebody would manage to execute some code with node.js that wouldn't be REALLY harmful to my machine, the only thing i keep there is this bot. @Timekiller does that rule works also with DUPLICATE ON KEY ? – Janekk Jul 19 '19 at 23:16
  • Edit: Yes it does.. And probably this little thing took me 3 days to solve :) I miss-translated polish to english, because I thought "primary" is some sort of "unique" – Janekk Jul 19 '19 at 23:21
  • Making your code safe from SQL injection is not harder than making it broken. The risk in allowing arbitrary code execution on your machine goes much wider than breaking whatever service you're running. People can also use your machine to hide the origin of their attacks on other sites. Not only that, but sharing code that is vulnerable encourages others to ignore this vulnerability. It's ok that you didn't know about it, and you shouldn't feel bad about it, but you do have an ethical responsibility to fix the mistake now you're aware. – ForbesLindesay Jul 26 '19 at 14:00

1 Answers1

0

To answer the original question, you want ON DUPLICATE KEY UPDATE. More importantly though, your code is vulnerable to SQL injection.

You should never insert the values directly into your query like that. Consider the example:

db.query("INSERT INTO messages (message, username) VALUES ('" + message + "', "' + username + ')");

If the username was my authenticated username, but the message was whatever value I typed into the UI, I could pretend to be someone else by sending a message like: I am stupid', 'someone_else') --

The SQL would then look like:

INSERT INTO messages (message, username)
VALUES ('I am stupid', 'someone_else') --', 'my_username')

The --', 'my_username') bit is treated as a comment, so it looks like someone_else said I am stupid. This is one of the most common and easily exploitable vulnerabilities in web applications.

Solution 1

You could parameterise your query:

db.query(`
  INSERT INTO messages (message, username)
  VALUES (?, ?)
`, [message, username]);

This is secure, but harder to read (in my opinion) and you have to be very careful to always do this consistently.

For your example, this would look like:

sql.query("INSERT INTO users (nick,cldbid,clid,lastChannel) VALUES(?,?,?,?) ON DUPLICATE KEY UPDATE lastchannel=?;", 
  [event.client.nickname, event.client.getDBID(), event.client.getUID(), config.lastChannel, event.channel.cid],
        function(err, result){
            if(err){
                log.clog("Blad SQL #clientmoved: " + err)
            }else{
                log.clog("Pomyslnie addded/updated record of "+event.client.nickname)
            }
        })

Solution 2

https://www.atdatabases.org provides database APIs that are relatively easy to use, and totally safe from this kind of attack. You would just do:

import connect, {sql} from '@databases/pg';

const db = connect();

db.query(sql`
  INSERT INTO messages (message, username)
  VALUES (${message}, ${username})
`);

to safely execute the same query. The sql tag ensures the data is properly handled/escaped and @databases/pg automatically enforces that you always add the sql tag. N.B. there are then no quotes around the parameters.

For your example that would be something like

db.query(sql`INSERT INTO users (nick,cldbid,clid,lastChannel) VALUES(${event.client.nickname},${event.client.getDBID()},${event.client.getUID()},${config.lastChannel}) ON DUPLICATE KEY UPDATE lastchannel=${event.channel.cid};`).then(
  result => log.clog("Pomyslnie addded/updated record of "+event.client.nickname),
  err => log.clog("Blad SQL #clientmoved: " + err),
);
ForbesLindesay
  • 10,482
  • 3
  • 47
  • 74