Skip to content

Fix db query injections #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TheKing-OfTime wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix db query injections #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f125074
Fixing existing DB query injections
TheKing-OfTime Feb 6, 2023
4644c43
Disable debug mode
TheKing-OfTime Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions commands/role/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ class Roles extends BaseCommand {
* @param {CommandInteraction} int
*/
async slash (int) {
console.time('1')
const member = int.member;
const permission = this.permission(member);

Expand All @@ -79,7 +80,8 @@ class Roles extends BaseCommand {
if (members) {
members = members.replace(/[^-_\w]/g, ' ').match(/[0-9]+/g);
}

console.timeEnd('1')
console.time('2')
if (!role) {
if (permission && create) {
role = (await this.create(
Expand All @@ -99,8 +101,8 @@ class Roles extends BaseCommand {
});
}
}


console.timeEnd('2')
console.time('3')
if (!members || !permission) {
toggleRole(role, member, int.member).then(result => {
int.reply({
Expand All @@ -115,10 +117,10 @@ class Roles extends BaseCommand {
});
} else {
if (!int.replied) {
await int.reply({
int.reply({
content: 'Запускаю выдачу ролей',
allowedMentions: constants.AM_NONE
});
}).catch(e => {console.log(e)});
}

members.forEach(user => {
Expand All @@ -136,6 +138,7 @@ class Roles extends BaseCommand {
});
});
}
console.timeEnd('3')
}

/**
Expand Down
12 changes: 6 additions & 6 deletions commands/voice/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ class Voice extends BaseCommand {
async slash (int) {
if (int.options.getSubcommand() === 'auto-sync') {
await int.deferReply({ ephemeral: true });
DB.query(`UPDATE users SET mode = "${int.options.getString('mode')}" WHERE id = ${int.user.id};`)[0];
DB.query(`UPDATE users SET mode = ? WHERE id = ?;`, [int.options.getString('mode'), int.user.id])[0];
Copy link
Copy Markdown
Collaborator

@ruskotwo ruskotwo Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Гравис лучше заменять на обычные одинарные кавычки.
И зачем мы в конце имеем [0]?

await int.editReply({
content: reaction.emoji.success + ' ' + int.str('Settings changed'),
ephemeral: true
Expand Down Expand Up @@ -193,7 +193,7 @@ class Voice extends BaseCommand {
async create (data) {
let preset;
try {
preset = DB.query(`SELECT * FROM users WHERE id = '${data.member.id}';`)[0];
preset = DB.query(`SELECT * FROM users WHERE id = ?;`, [data.member.id])[0];
} catch (e) {
console.log('DB error occurred:\n' + e);
}
Expand Down Expand Up @@ -286,10 +286,10 @@ class Voice extends BaseCommand {
userLimit: voice.channel.userLimit
});
if (DB.query(
`SELECT * FROM users WHERE id = ${voice.member.user.id};`)[0]) {
`SELECT * FROM users WHERE id = ?;`, [voice.member.user.id])[0]) {
DB.query(
`UPDATE users SET voice_data = ? WHERE id = ${voice.member.user.id};`,
[voice_data]
`UPDATE users SET voice_data = ? WHERE id = ?;`,
[voice_data, voice.member.user.id]
)[0];
} else {
DB.query(
Expand All @@ -305,7 +305,7 @@ class Voice extends BaseCommand {
*/
async sync (voice) {
let voiceConfiguration = JSON.parse((
DB.query(`SELECT * FROM users WHERE id = ${voice.member.user.id};`)[0]
DB.query(`SELECT * FROM users WHERE id = ?;`, [voice.member.user.id])[0]
).voice_data);
if (!voiceConfiguration) return 'There is no data entry in the database associated with you. Use `/upload` to fix it.';

Expand Down
8 changes: 4 additions & 4 deletions commands/warn/Warn.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,9 +274,9 @@ class Warn {
*/
static last (target) {
const query = target
? `SELECT * FROM warns WHERE id = (SELECT MAX(id) FROM warns WHERE target = ${target})`
? `SELECT * FROM warns WHERE id = (SELECT MAX(id) FROM warns WHERE target = ?)`
: `SELECT * FROM warns WHERE id = (SELECT MAX(id) FROM warns)`;
const data = DB.query(query);
const data = DB.query(query, target ? [target] : undefined);
Comment on lines -277 to +279
Copy link
Copy Markdown
Collaborator

@ruskotwo ruskotwo Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Это выглядит крайне сомнительно. По сути ты делаешь 2 проверки.
Лучше заменить сразу на что то такое:

const data = target
	? DB.query('SELECT * FROM warns WHERE id = (SELECT MAX(id) FROM warns WHERE target = ?)', target)
	: DB.query('SELECT * FROM warns WHERE id = (SELECT MAX(id) FROM warns)');

И сам запрос выглядит как лютый ужас. По сути, твоя задача - получить последнюю добавленную строку.
В итоге ты делаешь запрос, который содержит второй запрос, который функцией MAX находит максимальное значение и возвращает тебе.

Хотя для твоей цели подойдёт такой запрос:

SELECT * FROM warns WHERE target = ? ORDER BY id DESC LIMIT 1;
SELECT * FROM warns ORDER BY id DESC LIMIT 1;

Тут ты сортируешь от максимального айдишника и лимитом забираешь лишь одну строку. Этот запрос будет в тысячи раз менее ресурсоёмким.

if (!data[0]) return undefined;

return new this(data[0]);
Expand All @@ -290,9 +290,9 @@ class Warn {
*/
static all (target) {
const query = target
? `SELECT * FROM warns WHERE NOT flags & 4 AND target = ${target}`
? `SELECT * FROM warns WHERE NOT flags & 4 AND target = ?`
: `SELECT * FROM warns WHERE NOT flags & 4`;
const data = DB.query(query);
const data = DB.query(query, target ? [target] : undefined);
Comment on lines -293 to +295
Copy link
Copy Markdown
Collaborator

@ruskotwo ruskotwo Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Та же история с двумя проверками и лишней константой query, которая более не будет использоваться


let warns = [];
for (let i = data.length; i >= 0; i--) {
Expand Down
6 changes: 3 additions & 3 deletions commands/warn/WarnPagination.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ class WarnPagination {
const skip = this.pageCount * (this.pageNumber - 1);

const query = target
? `FROM warns WHERE target = ${this.target.id} AND NOT flags & 4`
? `FROM warns WHERE target = ? AND NOT flags & 4`
: `FROM warns WHERE NOT flags & 4`;
this.count = DB.query('SELECT COUNT(*) AS count ' + query)[0].count;
this.count = DB.query('SELECT COUNT(*) AS count ' + query, target ? [this.target.id] : undefined)[0].count;
const data = DB.query(
'SELECT * ' + query + ' ORDER BY id DESC LIMIT ?, ?',
[skip, this.pageCount]
target ? [this.target.id, skip, this.pageCount] : [skip, this.pageCount]
);

for (const row of data) {
Expand Down