AntiScam Checks#2
Conversation
zerebos
left a comment
zerebos
left a comment
There was a problem hiding this comment.
This doesn't have some necessary sanity checking, take a look at detectspam event, should check if in guild, if bot, channel permissions, etc. This is also not toggleable, it feels like maybe it should be collapsed into detect spam.
Also code-wise there are other issues but I can fix after merging
There was a problem hiding this comment.
Pull request overview
This PR refactors the spam detection system by consolidating multiple regex patterns and their validation logic into a unified data-driven approach using a pattern configuration array.
Changes:
- Refactored spam detection from separate regex variables to a centralized
phishingPatternsarray with configurable predicates - Updated embed logging to support multiple reasons instead of a single reason
- Removed error handling for message deletion (commented out old code but removed try-catch from new implementation)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const hosts = links.map(match => { | ||
| const url = match[0]; | ||
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | ||
| return URL.parse(fullUrl)?.host; | ||
| }).filter(Boolean); |
There was a problem hiding this comment.
Missing error handling for URL parsing. If the URL is malformed, URL.parse or new URL() will throw an error. Wrap the URL parsing in a try-catch block to prevent the entire spam detection from failing on malformed URLs.
| { | ||
| regex: /(?:http[s]?:\/\/.)?(?:www\.)?[-a-zA-Z0-9@%._+~#=]{2,256}\.[a-z]{2,6}\b[-a-zA-Z0-9@:%_+.~#?&\/=]*/ig, | ||
| whitelist: [], | ||
| predicate: (links, self) => links.length == self.maxCount, // this should probably be more than 4 later on. |
There was a problem hiding this comment.
The equality check should use strict equality. Replace '==' with '===' to avoid type coercion issues and follow JavaScript best practices.
| predicate: (links, self) => links.length == self.maxCount, // this should probably be more than 4 later on. | |
| predicate: (links, self) => links.length === self.maxCount, // this should probably be more than 4 later on. |
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | ||
| return URL.parse(fullUrl)?.host; | ||
| }).filter(Boolean); | ||
| return hosts.some(host => !self.whitelist.includes(host)); |
There was a problem hiding this comment.
The whitelist check should use the full hostname, not just the host. URL.parse().host may include the port number, which could cause legitimate URLs with ports to be flagged. Consider comparing against hostname instead or normalize the comparison.
| const phishingPatterns = [ | ||
| { | ||
| regex: /([a-zA-Z-\\.]+)?d[il][il]?scorr?(cl|[ldb])([a-zA-Z-\\.]+)?\.(com|net|app|gift|ru|uk)/ig, | ||
| whitelist: ['discord.com', 'discordapp.com'], |
There was a problem hiding this comment.
The pattern will incorrectly match 'betterdiscord.app' which was explicitly whitelisted in the old code. Add 'betterdiscord.app' to the whitelist array to maintain the same behavior as the previous implementation.
| whitelist: ['discord.com', 'discordapp.com'], | |
| whitelist: ['discord.com', 'discordapp.com', 'betterdiscord.app'], |
| whitelist: [], | ||
| predicate: (links, self) => links.length == self.maxCount, // this should probably be more than 4 later on. |
There was a problem hiding this comment.
This generic URL regex pattern is too broad and will match many legitimate URLs. It will trigger on any message containing 4+ URLs regardless of their legitimacy. Consider adding common legitimate domains to the whitelist or refining this pattern to avoid false positives.
| whitelist: [], | |
| predicate: (links, self) => links.length == self.maxCount, // this should probably be more than 4 later on. | |
| whitelist: [ | |
| 'discord.com', | |
| 'discordapp.com', | |
| 'steamcommunity.com', | |
| 'github.com', | |
| 'gitlab.com', | |
| 'bitbucket.org', | |
| 'google.com', | |
| 'youtube.com', | |
| 'youtu.be', | |
| 'twitch.tv', | |
| 'twitter.com', | |
| 'x.com', | |
| 'reddit.com' | |
| ], | |
| predicate: (links, self) => { | |
| const suspiciousLinks = links.filter(match => { | |
| const url = match[0].toLowerCase(); | |
| return !self.whitelist.some(domain => url.includes(domain)); | |
| }); | |
| return suspiciousLinks.length === self.maxCount; // this should probably be more than 4 later on. | |
| }, |
| const phishingPatterns = [ | ||
| { | ||
| regex: /([a-zA-Z-\\.]+)?d[il][il]?scorr?(cl|[ldb])([a-zA-Z-\\.]+)?\.(com|net|app|gift|ru|uk)/ig, | ||
| whitelist: ['discord.com', 'discordapp.com'], | ||
| predicate: (links, self) => { | ||
| const hosts = links.map(match => { | ||
| const url = match[0]; | ||
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | ||
| return URL.parse(fullUrl)?.host; | ||
| }).filter(Boolean); | ||
| return hosts.some(host => !self.whitelist.includes(host)); | ||
| }, | ||
| reason: 'Fake Discord Domain' | ||
| }, | ||
| { | ||
| regex: /str?e[ea]?mcomm?m?un[un]?[un]?[tl]?[il][tl]?ty\.(com|net|ru|us)/ig, | ||
| whitelist: ['steamcommunity.com'], | ||
| predicate: (links, self) => { | ||
| const hosts = links.map(match => { | ||
| const url = match[0]; | ||
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | ||
| return URL.parse(fullUrl)?.host; | ||
| }).filter(Boolean); | ||
|
|
||
| return hosts.some(host => !self.whitelist.includes(host)); | ||
| }, |
There was a problem hiding this comment.
This predicate function is duplicated in the second pattern (lines 30-37). Consider extracting this into a shared helper function to reduce code duplication and improve maintainability.
| const phishingPatterns = [ | |
| { | |
| regex: /([a-zA-Z-\\.]+)?d[il][il]?scorr?(cl|[ldb])([a-zA-Z-\\.]+)?\.(com|net|app|gift|ru|uk)/ig, | |
| whitelist: ['discord.com', 'discordapp.com'], | |
| predicate: (links, self) => { | |
| const hosts = links.map(match => { | |
| const url = match[0]; | |
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | |
| return URL.parse(fullUrl)?.host; | |
| }).filter(Boolean); | |
| return hosts.some(host => !self.whitelist.includes(host)); | |
| }, | |
| reason: 'Fake Discord Domain' | |
| }, | |
| { | |
| regex: /str?e[ea]?mcomm?m?un[un]?[un]?[tl]?[il][tl]?ty\.(com|net|ru|us)/ig, | |
| whitelist: ['steamcommunity.com'], | |
| predicate: (links, self) => { | |
| const hosts = links.map(match => { | |
| const url = match[0]; | |
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | |
| return URL.parse(fullUrl)?.host; | |
| }).filter(Boolean); | |
| return hosts.some(host => !self.whitelist.includes(host)); | |
| }, | |
| function hasNonWhitelistedHost(links: RegExpMatchArray[], whitelist: string[]): boolean { | |
| const hosts = links.map(match => { | |
| const url = match[0]; | |
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | |
| return URL.parse(fullUrl)?.host; | |
| }).filter(Boolean); | |
| return hosts.some(host => !whitelist.includes(host as string)); | |
| } | |
| const phishingPatterns = [ | |
| { | |
| regex: /([a-zA-Z-\\.]+)?d[il][il]?scorr?(cl|[ldb])([a-zA-Z-\\.]+)?\.(com|net|app|gift|ru|uk)/ig, | |
| whitelist: ['discord.com', 'discordapp.com'], | |
| predicate: (links, self) => hasNonWhitelistedHost(links, self.whitelist), | |
| reason: 'Fake Discord Domain' | |
| }, | |
| { | |
| regex: /str?e[ea]?mcomm?m?un[un]?[un]?[tl]?[il][tl]?ty\.(com|net|ru|us)/ig, | |
| whitelist: ['steamcommunity.com'], | |
| predicate: (links, self) => hasNonWhitelistedHost(links, self.whitelist), |
|
Please mark resolve or fix above |
|
/gemini review these incoming changes |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the spam and phishing detection logic in src/events/detectspam.ts to use a structured array of patterns with custom predicates and whitelists. Feedback focuses on resolving potential false positives in subdomain matching, fixing unintended character ranges in regexes, ensuring backward compatibility by avoiding URL.parse on older Node.js versions, correcting the link count threshold check, and removing commented-out legacy code.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | ||
| return URL.parse(fullUrl)?.host; | ||
| }).filter(Boolean); | ||
| return hosts.some(host => !self.whitelist.includes(host)); |
There was a problem hiding this comment.
Using self.whitelist.includes(host) will cause legitimate subdomains of whitelisted domains (such as canary.discord.com, ptb.discord.com, or media.discordapp.com) to be flagged as fake domains. This will result in false positives where legitimate messages are deleted and users are muted.\n\nChecking if the host matches exactly or ends with . + the allowed domain resolves this issue.
| return hosts.some(host => !self.whitelist.includes(host)); | |
| return hosts.some(host => !self.whitelist.some(allowed => host === allowed || host.endsWith('.' + allowed))); |
| const sketchyRuRegex = new RegExp(`([a-zA-Z-\\.]+).ru.com`, "ig"); | ||
| const phishingPatterns = [ | ||
| { | ||
| regex: /([a-zA-Z-\\.]+)?d[il][il]?scorr?(cl|[ldb])([a-zA-Z-\\.]+)?\.(com|net|app|gift|ru|uk)/ig, |
There was a problem hiding this comment.
In a regular expression literal, \\ represents a literal backslash. Therefore, -\\ inside the character class [a-zA-Z-\\.] creates a character range from - (ASCII 45) to \\ (ASCII 92). This range matches many unintended characters such as numbers, colons, slashes, and uppercase letters, which can lead to false positives.\n\nUsing [a-zA-Z.-]+ (placing the hyphen at the end of the character class) correctly matches only letters, dots, and hyphens without creating an unintended range.
| regex: /([a-zA-Z-\\.]+)?d[il][il]?scorr?(cl|[ldb])([a-zA-Z-\\.]+)?\.(com|net|app|gift|ru|uk)/ig, | |
| regex: /([a-zA-Z.-]+)?d[il][il]?scorr?(cl|[ldb])([a-zA-Z.-]+)?\\.(com|net|app|gift|ru|uk)/ig, |
| const hosts = links.map(match => { | ||
| const url = match[0]; | ||
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | ||
| return URL.parse(fullUrl)?.host; | ||
| }).filter(Boolean); |
There was a problem hiding this comment.
URL.parse is a relatively new method introduced in Node.js v22.0.0 and v21.2.0. If this bot is deployed on an older LTS version of Node.js (such as v18 or v20), calling URL.parse will throw a TypeError and crash the application.\n\nUsing new URL() wrapped in a try/catch block is a safer, backward-compatible approach.
const hosts = links.map(match => {\n const url = match[0];\n const fullUrl = url.startsWith('http') ? url : 'https://' + url;\n try {\n return new URL(fullUrl).host;\n } catch {\n return null;\n }\n }).filter(Boolean) as string[];| const hosts = links.map(match => { | ||
| const url = match[0]; | ||
| const fullUrl = url.startsWith('http') ? url : `https://${url}`; | ||
| return URL.parse(fullUrl)?.host; | ||
| }).filter(Boolean); |
There was a problem hiding this comment.
URL.parse is a relatively new method introduced in Node.js v22.0.0 and v21.2.0. If this bot is deployed on an older LTS version of Node.js (such as v18 or v20), calling URL.parse will throw a TypeError and crash the application.\n\nUsing new URL() wrapped in a try/catch block is a safer, backward-compatible approach.
const hosts = links.map(match => {\n const url = match[0];\n const fullUrl = url.startsWith('http') ? url : 'https://' + url;\n try {\n return new URL(fullUrl).host;\n } catch {\n return null;\n }\n }).filter(Boolean) as string[];| }).filter(Boolean); | ||
|
|
||
| return hosts.some(host => !self.whitelist.includes(host)); | ||
| }, |
There was a problem hiding this comment.
Using self.whitelist.includes(host) will cause legitimate subdomains of whitelisted domains (such as help.steamcommunity.com) to be flagged as fake domains.\n\nChecking if the host matches exactly or ends with . + the allowed domain resolves this issue.
return hosts.some(host => !self.whitelist.some(allowed => host === allowed || host.endsWith('.' + allowed)));| }, | ||
| { | ||
| regex: /([a-zA-Z-\\.]+)\.ru\.com/ig, | ||
| whitelist: [], |
There was a problem hiding this comment.
In a regular expression literal, \\ represents a literal backslash. Therefore, -\\ inside the character class [a-zA-Z-\\.] creates a character range from - (ASCII 45) to \\ (ASCII 92). This range matches many unintended characters such as numbers, colons, slashes, and uppercase letters, which can lead to false positives.\n\nUsing [a-zA-Z.-]+ (placing the hyphen at the end of the character class) correctly matches only letters, dots, and hyphens without creating an unintended range.
| whitelist: [], | |
| regex: /([a-zA-Z.-]+)\\.ru\\.com/ig, |
| regex: /(?:http[s]?:\/\/.)?(?:www\.)?[-a-zA-Z0-9@%._+~#=]{2,256}\.[a-z]{2,6}\b[-a-zA-Z0-9@:%_+.~#?&\/=]*/ig, | ||
| whitelist: [], | ||
| predicate: (links, self) => links.length == self.maxCount, // this should probably be more than 4 later on. | ||
| reason: 'Potential Scam Message', |
There was a problem hiding this comment.
Using links.length == self.maxCount means that if a message contains more than 4 links (e.g., 5 or 10 links), the predicate will evaluate to false, allowing the spam message to bypass the filter.\n\nChanging this to >= ensures that any message with 4 or more links is caught.
| reason: 'Potential Scam Message', | |
| predicate: (links, self) => !!self.maxCount && links.length >= self.maxCount, // this should probably be more than 4 later on. |
| /* | ||
| const fakeDiscordMatches = message.content.match(fakeDiscordRegex) || []; | ||
| const fakeSteamMatches = message.content.match(fakeSteamRegex) || []; | ||
| const isFakeDiscord = fakeDiscordMatches.some(s => { | ||
| if (okayDiscordRegex.test(s)) return false; | ||
| else if (s.toLowerCase() === "betterdiscord.app") return false; | ||
| return true; | ||
| }); | ||
| const isFakeSteam = fakeSteamMatches.some(s => s.toLowerCase() !== "steamcommunity.com"); | ||
| const isSketchy = sketchyRuRegex.test(message.content); | ||
| if (!isFakeDiscord && !isFakeSteam && !isSketchy) return; // Not spam, let's get out of here | ||
|
|
||
| let reason = "Sketchy Link"; | ||
| if (isFakeDiscord) reason = "Fake Discord Link"; | ||
| if (isFakeSteam) reason = "Fake Steam Link"; | ||
|
|
||
| try { | ||
| await message.delete(); | ||
| } | ||
| catch { | ||
| // TODO: logging? | ||
| console.error("Could not delete detect spam message. Likely permissions."); | ||
| }*/ |
There was a problem hiding this comment.
Leaving large blocks of commented-out dead code reduces readability and maintainability. Since Git history preserves the previous implementation, this commented-out block should be removed.
this is a low level check.
Please double check on your local machine for testing.