alveusgg / chatbot

A Twitch Chat bot for Alveus Sanctuary, allowing stream viewers to control the tech running the Live Cams.
Other
5 stars 7 forks source link

Fixed very silly, theoretical privilege escalation #14

Closed changesbyjames closed 2 months ago

changesbyjames commented 2 months ago

Hey all, Firstly thank you for open sourcing this, I've always been interested in how all of this works!

As I was reading through the code I noticed a very small privilege escalation that you might want to fix. I don't think it's critical but it could have caused some mischief. Currently, twitch.tv/mods can perform commands as a mod, twitch.tv/vips can perform commands as a vip, etc.

function isAllowed(userCommand,userProfile){
    //user = {name,isMod,isVip,isSub};
    userCommand = userCommand || "";
    for (const permission in config.commandPermissions){
        //go through all commands
        for (const command of config.commandPermissions[permission]){
            //see if in correct category
            if (userCommand.toLowerCase() == command.toLowerCase()){
                //go through ranks to check if user is allowed
                for (const priority of config.userPermissions.commandPriority){
                    let userRank = getUserRank(userProfile);

                    /*
                        Issue:
                        as the userPermissions[priority] includes "mods", "vips", etc as special cases,
                        because the username is processed first any username that matches those
                        special strings will have elevated permissions.
                    */

                    if (config.userPermissions[priority].includes(userProfile.userName.toLowerCase())){
                        return {allowed:true,accessLevel:priority};
                    } else if (config.userPermissions[priority].includes("mods") && userRank >= 4){
                        return {allowed:true,accessLevel:priority};
                    } else if (config.userPermissions[priority].includes("vips") && userRank >= 3){
                        return {allowed:true,accessLevel:priority};
                    } else if (config.userPermissions[priority].includes("subs") && userRank >= 2){
                        return {allowed:true,accessLevel:priority};
                    } else if (config.userPermissions[priority].includes("all")){
                        return {allowed:true,accessLevel:priority};
                    } 
                    if (priority == permission){
                        //stop when priority rank reached
                        break;
                    }
                }
            }
        }
    }
    return {allowed:false,accessLevel:null};
}

My suggestion is to switch this over to symbols to indicate the unique, special nature of this allowing the class of "mods" rather the string "mods".

No worries if this isn't a priority and I haven't quite got my development environment set up properly so I haven't been able to test it end-to-end but I have tested the function itself.

// test.js
const { isAllowed, commandCheck } = require("./src/utils/helper"); // without changes

let command = "!apreset";
command = commandCheck(command);
console.log(isAllowed(command, { userName: "mods" }));  // log: { allowed: true, accessLevel: 'commandMods' }
// test.js
const { isAllowed, commandCheck } = require("./src/utils/helper"); // with changes

let command = "!apreset";
command = commandCheck(command);
console.log(isAllowed(command, { userName: "mods" }));  // log: { allowed: false, accessLevel: null }
merger3 commented 2 months ago

Good catch, you might wanna add your twitch name to the description too so everyone knows who you are!