Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
3 changes: 3 additions & 0 deletions code/__DEFINES/discord.dm
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,6 @@

/// Send to the mentor Discord webhook
#define DISCORD_WEBHOOK_MENTOR "MENTOR"

/// Send to the bugreport Discord webhook
#define DISCORD_WEBHOOK_BUGREPORT "BUGREPORT"
4 changes: 4 additions & 0 deletions code/controllers/configuration/entries/config.dm
Original file line number Diff line number Diff line change
Expand Up @@ -652,6 +652,10 @@
/// Webhook URLs for the requests webhook
/datum/config_entry/str_list/discord_requests_webhook_urls

//Needs attention
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

🟡 Рекомендация: В коде остался комментарий //Needs attention. Такие комментарии следует удалять перед слиянием в основную ветку, чтобы поддерживать чистоту кодовой базы.

Copy link
Contributor

Choose a reason for hiding this comment

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

medium

🟡 Рекомендация по стилю: Комментарий //Needs attention выглядит как временная пометка и не соответствует стайлгайду. Рекомендую его удалить, чтобы код оставался чистым и понятным. Согласно стайлгайду (п. 4.1, строка 68), комментарии должны быть на английском языке и объяснять, почему код написан так, а не что он делает.

References
  1. Комментарии должны быть на английском языке и объяснять, почему код написан так, а не что он делает. (link)

/// Webhook URLs for the bugreport webhook
/datum/config_entry/str_list/discord_bugreport_webhook_urls

/// Do we want to forward all adminhelps to the discord or just ahelps when admins are offline.
/// (This does not mean all ahelps are pinged, only ahelps sent when staff are offline get the ping, regardless of this setting)
/datum/config_entry/flag/discord_forward_all_ahelps
Expand Down
2 changes: 2 additions & 0 deletions code/controllers/subsystem/non-firing/discord.dm
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ SUBSYSTEM_DEF(discord)
webhook_urls = CONFIG_GET(str_list/discord_main_webhook_urls)
if(DISCORD_WEBHOOK_MENTOR)
webhook_urls = CONFIG_GET(str_list/discord_mentor_webhook_urls)
if(DISCORD_WEBHOOK_BUGREPORT)
webhook_urls = CONFIG_GET(str_list/discord_bugreport_webhook_urls)

var/datum/discord_webhook_payload/dwp = new()
dwp.webhook_content = content
Expand Down
26 changes: 26 additions & 0 deletions code/modules/admin/verbs/bugreport.dm
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/client/verb/bugreport()
set name = "Баг-репорт"
set category = ADMIN_CATEGORY_TICKETS

if(check_mute(ckey, MUTE_ADMINHELP))
to_chat(src, span_red("Error: Admin-PM: You cannot send adminhelps (Muted)."), MESSAGE_TYPE_ADMINPM, confidential = TRUE)
return

var/msg

var/description = tgui_input_text(src, "Опишите баг/недочет:", "Баг-репорт", max_length=700, encode = FALSE)
var/correct_desc = tgui_input_text(src, "Опишите ожидаемое поведение (как должно работать):", "Баг-репорт", max_length=700, encode = FALSE)
var/discord = tgui_input_text(src, "Ваш дискорд для связи (обязательно):", "Баг-репорт", max_length=100, encode = FALSE)
Copy link
Contributor

Choose a reason for hiding this comment

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

security-critical critical

This section is critically vulnerable to Discord ping and mention bypasses. The use of encode = FALSE in tgui_input_text combined with the Discord subsystem's html_decode() on content allows attackers to use HTML entities (e.g., @everyone) to bypass mention protections. This also creates a risk of malicious Markdown injection and potential XSS if the input is ever rendered as HTML in-game. It's crucial to ensure proper encoding of user input to prevent these vulnerabilities.

var/description = tgui_input_text(src, "Опишите баг/недочет:", "Баг-репорт", max_length=700)
	var/correct_desc = tgui_input_text(src, "Опишите ожидаемое поведение (как должно работать):", "Баг-репорт", max_length=700)
	var/discord =  tgui_input_text(src, "Ваш дискорд для связи (обязательно):", "Баг-репорт", max_length=100)

var/have_screens = tgui_alert(src, "Есть ли у вас скрины?", "Баг-репорт", list("Да", "Нет"))=="Да"

if(!description || !correct_desc || !discord)
tgui_alert(src, "Вы пропустили один из пунктов! Попробуйте сделать баг-репорт еще раз, заполняя все поля", "Баг-репорт")
return
Copy link
Contributor

Choose a reason for hiding this comment

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

high

🔴 Критично (потенциальная уязвимость и улучшение UX): Код в этом блоке имеет две проблемы:

  1. Уязвимость: Входные данные от пользователя (description, correct_desc, discord) не проходят санитарную обработку. Это может привести к злоупотреблению форматированием в Discord. Рекомендуется использовать sanitize_simple().
  2. UX: Все окна ввода появляются последовательно, даже если пользователь отменил предыдущий шаг. Лучше проверять ввод после каждого шага и прерывать процедуру при отмене. Это соответствует правилу о ранних возвратах (стайлгайд, п. 4.1, строка 74).

Предлагаю объединить исправления в одном изменении.

	var/description = sanitize_simple(tgui_input_text(src, "Опишите баг/недочет:", "Баг-репорт", max_length=700, encode = FALSE))
	if(!description) return
	var/correct_desc = sanitize_simple(tgui_input_text(src, "Опишите ожидаемое поведение (как должно работать):", "Баг-репорт", max_length=700, encode = FALSE))
	if(!correct_desc) return
	var/discord =  sanitize_simple(tgui_input_text(src, "Ваш дискорд для связи (обязательно):", "Баг-репорт", max_length=100, encode = FALSE))
	if(!discord) return
	var/have_screens = tgui_alert(src, "Есть ли у вас скрины?", "Баг-репорт", list("Да", "Нет"))=="Да"
References
  1. Предпочитать ранние возвраты (return) глубокой вложенности условий, а также continue в циклах. (link)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sanitize_simple() убирает \n и \t, скорее всего из-за траблов в игре. У вебхука это не вызывает никаких проблем, я протестил


msg = "[key_name(src)]\nID раунда: [GLOB.round_id] \n1. [description]\n2. [correct_desc]\n3. [discord]\n4. Скрины: [have_screens ? "Да" : "Нет"]"

if(handle_spam_prevention(msg, MUTE_ADMINHELP, OOC_COOLDOWN))
return

if(tgui_alert(src, "Ваш репорт выглядит так:\n[msg]\nВы уверены что все заполнено правильно?", "Баг-репорт", list("Да", "Нет"))=="Да")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if(tgui_alert(src, "Ваш репорт выглядит так:\n[msg]\nВы уверены что все заполнено правильно?", "Баг-репорт", list("Да", "Нет"))=="Да")
if(tgui_alert(src, "Ваш репорт выглядит так:\n[msg]\nВы уверены что все заполнено правильно?", "Баг-репорт", list("Да", "Нет")) == "Да")

Copy link
Collaborator

Choose a reason for hiding this comment

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

Сделай нормальный тгуи интерфейс. Нет нужды плодить кучу инпутов

SSdiscord.send2discord_simple(DISCORD_WEBHOOK_BUGREPORT, msg)
Comment on lines +1 to +34
Copy link
Contributor

Choose a reason for hiding this comment

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

security-high high

The /client/verb/bugreport() verb lacks any form of rate limiting (cooldown) or mute checks. This allows malicious users to spam the Discord webhook, potentially leading to a Denial of Service or the webhook being deleted. Additionally, the input fields have no max_length specified, which could lead to Discord API rejections for overly long messages and potential resource exhaustion during JSON encoding. Remediation includes implementing a cooldown (e.g., 10 minutes per player), checking for player mutes (MUTE_ADMINHELP), and setting a reasonable max_length for all tgui_input_text calls (e.g., 500-1000 characters).

5 changes: 5 additions & 0 deletions config/example/config.txt
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,11 @@ BYOND_ACCOUNT_AGE_THRESHOLD 7
## DISCORD_MENTOR_WEBHOOK_URLS https://url2
#DISCORD_MENTOR_WEBHOOK_URLS

## Webhook URLs for the bugreport discord webhook. Separate multiple URLs with enter, like:
## DISCORD_BUGREPORT_WEBHOOK_URLS https://url1
## DISCORD_BUGREPORT_WEBHOOK_URLS https://url2
#DISCORD_BUGREPORT_WEBHOOK_URLS

## Uncomment to send all ahelps to discord. If the line is commented, only ahelps made when no admins are online will be forwarded
## Ahelps forwarded when staff are online will never have the role ping, regardless of the setting above
#DISCORD_FORWARD_ALL_AHELPS
Expand Down
1 change: 1 addition & 0 deletions paradise.dme
Original file line number Diff line number Diff line change
Expand Up @@ -1866,6 +1866,7 @@
#include "code\modules\admin\verbs\asays.dm"
#include "code\modules\admin\verbs\atmosdebug.dm"
#include "code\modules\admin\verbs\beakerpanel.dm"
#include "code\modules\admin\verbs\bugreport.dm"
#include "code\modules\admin\verbs\bluespacearty.dm"
#include "code\modules\admin\verbs\borgpanel.dm"
#include "code\modules\admin\verbs\cantcomm_cargo.dm"
Expand Down
Loading