Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 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
Copy Markdown
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
Copy Markdown
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
25 changes: 25 additions & 0 deletions code/modules/admin/verbs/bugreport.dm
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/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)
Comment thread
dageavtobusnick marked this conversation as resolved.
Outdated
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
Copy Markdown
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)

Comment thread
dageavtobusnick marked this conversation as resolved.
Outdated
var/have_screens = tgui_alert(src, "Есть ли у вас скрины?", "Баг-репорт", list("Да", "Нет"))=="Да"
Comment thread
dageavtobusnick marked this conversation as resolved.
Outdated

if(!description || !correct_desc || !discord)
tgui_alert(src, "Вы пропустили один из пунктов! Попробуйте сделать баг-репорт еще раз, заполняя все поля", "Баг-репорт")
return TRUE
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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_screens ? "Да" : "Нет"]"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

🟡 Рекомендация по локализации и стилю

Сообщение, отправляемое в Discord, содержит смесь русского и английского языков (Round id и Скрины). Согласно стайлгайду, логи и сообщения для разработчиков должны быть на английском языке для единообразия. Рекомендуется полностью перевести строку на английский.

Также, для лучшей читаемости, предлагаю использовать Yes/No вместо Да/Нет.

	msg = "[key_name(src)]\nRound id: [GLOB.round_id] \n1. [description]\n2. [correct_desc]\n3. [discord]\n4. Screenshots: [have_screens ? "Yes" : "No"]"
References
  1. Логи и другие сообщения, предназначенные для разработчиков, не должны переводиться и должны быть на английском языке. (link)

if(findtext(msg, "<@") || findtext(msg, "<&@"))
Copy link
Copy Markdown
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 Discord ping check is bypassable and contains a typo. Discord role pings are formatted as <@&ID>, but the code checks for <&@. Furthermore, because the input should be HTML encoded (see previous comment), the check should look for the encoded version &lt;@ to be effective against the html_decode() performed in the Discord subsystem.

if(findtext(msg, "<@") || findtext(msg, "&lt;@"))

tgui_alert(src, "Обнаружена попытка пинга (<@id> или <@&id>)! Сообщение не будет отправленно.", "Баг-репорт")
return TRUE
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

🟡 Рекомендация по стилю

Аналогично предыдущему замечанию, для консистентности кода следует использовать return вместо return TRUE.

		return
References
  1. Обеспечивать единый стиль для всей кодовой базы. (link)

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