Skip to content
Open
Show file tree
Hide file tree
Changes from 3 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
18 changes: 18 additions & 0 deletions code/modules/admin/verbs/bugreport.dm
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/client/verb/bugreport()
set name = "Баг-репорт"
set category = ADMIN_CATEGORY_TICKETS

var/msg

var/description = tgui_input_text(src, "Опишите баг/недочет:", "Баг-репорт", encode = FALSE)
var/correct_desc = tgui_input_text(src, "Опишите ожидаемое поведение (как должно работать):", "Баг-репорт", encode = FALSE)
var/discord = tgui_input_text(src, "Ваш дискорд для связи (обязательно):", "Баг-репорт", 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-medium medium

The bug report system is vulnerable to Discord mention injection. The input is taken with encode = FALSE in tgui_input_text, and the Discord subsystem later calls html_decode on the content in code/datums/discord.dm. This combination allows users to inject Discord-specific mentions such as <@&role_id> or <@user_id> by either typing them directly or using HTML entities. This can be used to bypass the weak @everyone and @here filters to ping and harass Discord users or roles.

Remediation: Sanitize the input to remove or escape Discord mention syntax (e.g., by escaping < or @) before passing it to send2discord_simple.

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

Choose a reason for hiding this comment

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

medium

🟡 Рекомендация: Обнаружена опечатка в имени переменной have_sreens. Согласно стайлгайду (п. 4.1, Именование), имена переменных должны быть осмысленными. Для улучшения читаемости кода предлагаю переименовать ее в have_screens. Не забудьте также обновить использование этой переменной в строке 16.

	var/have_screens = tgui_alert(src, "Есть ли у вас скрины?", "Баг-репорт", list("Да", "Нет"))=="Да"
References
  1. Имена переменных должны быть осмысленными и следовать стилю lower_snake_case. Имя have_sreens содержит опечатку. (link)


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

Choose a reason for hiding this comment

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

high

🟡 Рекомендация: Проверка на пустые поля неполная. Условие if(!description || !correct_desc || !discord) пропустит пустые строки, так как !"" вернет FALSE. Это может привести к отправке неполных баг-репортов. Рекомендуется использовать !length() для проверки, что поля действительно заполнены.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

с каких пор !"" = 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-medium medium

This verb lacks proper spam prevention and rate limiting, unlike adminhelp. This omission could allow a malicious user to flood the Discord channel with reports. Implementing handle_spam_prevention() is crucial here. Additionally, for consistency, return should be used instead of return TRUE.

if(handle_spam_prevention(description + correct_desc + discord, MUTE_ADMINHELP, OOC_COOLDOWN))
		return


msg = "[key_name(src)]\nRound id: [GLOB.round_id] \n1. [description]\n2. [correct_desc]\n3. [discord]\n4. Скрины: [have_sreens ? "Да" : "Нет"]"
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_BUGFIX_WEBHOOK_URLS https://url1
## DISCORD_BUGFIX_WEBHOOK_URLS https://url2
#DISCORD_BUGFIX_WEBHOOK_URLS
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

🔴 Критично: Неверное имя переменной конфигурации в примере. В config/example/config.txt используется DISCORD_MENTOR_BUGREPORT_URLS, в то время как код ожидает DISCORD_BUGREPORT_WEBHOOK_URLS. Это приведет к тому, что функция не будет работать при использовании примера конфигурации.

## DISCORD_BUGREPORT_WEBHOOK_URLS https://url1
## DISCORD_BUGREPORT_WEBHOOK_URLS https://url2
#DISCORD_BUGREPORT_WEBHOOK_URLS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

буквально до того как выложил ПР поправил


## 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 @@ -1864,6 +1864,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