Repository navigation
fix: skip AI moderation for posts that only share trusted links - #67
Conversation
1c14678 to
18100e2
Compare
| # unwrapped by then), an email address, or a messaging app. | ||
| _RISK_SIGNAL_RE = re.compile( | ||
| r"https?://|www\.|<(?:a|img|iframe|video|embed|object)\b" | ||
| r"|(?<![\w@./-])(?:[a-z0-9-]+\.)+[a-z][a-z0-9-]*/\S" |
There was a problem hiding this comment.
Blocker (CPU exhaustion). This alternative has no start anchor, so on a long run of word characters without an @, [\w.+-]+ is retried from every position and the search goes quadratic. has_risk_signals() on a post with a trusted link plus one long word takes 7 s at 40,000 characters, 23 s at 80,000 and 62 s at 160,000. It runs inside the create request, and the LMS does not cap the post size.
A lookbehind lets it start only at the beginning of a token. It finds the same addresses and stays linear (2,000,000 characters in 0.05 s):
| r"|(?<![\w@./-])(?:[a-z0-9-]+\.)+[a-z][a-z0-9-]*/\S" | |
| r"|(?<![\w.+-])[\w.+-]+@[\w-]+(?:\.[\w-]+)+" |
A regression test for tests/test_ai_moderation_links.py (plus import time). It fails on the current head and passes with the fix:
def test_long_post_is_checked_quickly(self) -> None:
"""A long post without a risk signal does not stall the request."""
start = time.perf_counter()
assert has_risk_signals("a" * 50_000) is False
assert time.perf_counter() - start < 1| # or under a common TLD, an anchor or embedded media (trusted anchors have been | ||
| # unwrapped by then), an email address, or a messaging app. | ||
| _RISK_SIGNAL_RE = re.compile( | ||
| r"https?://|www\.|<(?:a|img|iframe|video|embed|object)\b" |
There was a problem hiding this comment.
A bare domain is only caught under the TLDs in _COMMON_TLDS, so the usual redirectors get through: discord.gg/…, linktr.ee/…, forms.gle/…, shorturl.at/…, rb.gy/…, is.gd/…. Since a false match only means the post is classified as today, I'd also count any domain followed by a path, whatever its TLD. The ordinary-post test cases still pass (style.css, node.js 18 and the like have no / after them):
| r"https?://|www\.|<(?:a|img|iframe|video|embed|object)\b" | |
| rf"|(?<![\w@.-])(?:[a-z0-9-]+\.)+(?:(?:{_COMMON_TLDS})(?![\w-])|[a-z]{{2,63}}/)" |
The comment above _COMMON_TLDS would then say a bare domain under any other TLD goes unnoticed unless a path follows it.
| _RISK_SIGNAL_RE = re.compile( | ||
| r"https?://|www\.|<(?:a|img|iframe|video|embed|object)\b" | ||
| r"|(?<![\w@./-])(?:[a-z0-9-]+\.)+[a-z][a-z0-9-]*/\S" | ||
| rf"|(?<![\w@.-])(?:[a-z0-9-]+\.)+(?:{_COMMON_TLDS})(?![\w-])" |
There was a problem hiding this comment.
Whats-App me and email tutor at gmail dot com both get through today. This widens the separator, adds a few more apps scams send people to, and catches a spelled-out "dot com":
| rf"|(?<![\w@.-])(?:[a-z0-9-]+\.)+(?:{_COMMON_TLDS})(?![\w-])" | |
| r"|\b(?:whats[\W_]*app|telegram|wechat|viber|discord|skype|snapchat)\b" | |
| r"|\bdot\W{0,3}(?:com|net|org)\b", |
| is classified as usual. | ||
| """ | ||
| if not content: |
There was a problem hiding this comment.
The patterns run on the raw text, so the usual tricks for hiding a contact detail from a filter get through: evil.com (HTML entity), What\u200bsApp (zero-width space) and full-width letters. Normalizing first closes those with the standard library (needs import html and import unicodedata next to import re at line 18):
| is classified as usual. | |
| """ | |
| if not content: | |
| if not content: | |
| return False | |
| # Undo the usual ways of hiding a link or contact from a pattern: HTML | |
| # entities, full-width letters and invisible characters. | |
| content = unicodedata.normalize("NFKC", html.unescape(content)) | |
| content = re.sub( | |
| r"[\u00ad\u200b-\u200f\u202a-\u202e\u2060-\u2064\ufeff]", "", content | |
| ) | |
| if _RISK_SIGNAL_RE.search(content): |
| if stripped != content and not has_risk_signals(stripped): | ||
| result["reasoning"] = "Only trusted links, and no other contact or link" | ||
| return result |
There was a problem hiding this comment.
Optional: skipped posts leave no audit log entry, so a log line is the only way to see how often this exemption is used, or abused:
| if stripped != content and not has_risk_signals(stripped): | |
| result["reasoning"] = "Only trusted links, and no other contact or link" | |
| return result | |
| if stripped != content and not has_risk_signals(stripped): | |
| log.info( | |
| f"AI moderation skipped, only trusted links: {content_instance.get('_id')}" | |
| ) | |
| result["reasoning"] = "Only trusted links, and no other contact or link" | |
| return result |
| "Call +1-555-0199 today", | ||
| "Reach me on +91 98765 43210", | ||
| "Contact (555) 123-4567", | ||
| "Add me on WhatsApp", |
There was a problem hiding this comment.
Cases for the gaps above. They fail on the current head and pass with the suggestions on lines 48, 50 and 129-131 of links.py:
| "Add me on WhatsApp", | |
| "I am on WeChat and Viber", | |
| "Join discord.gg/abc123", | |
| "Everything is on linktr.ee/helper", | |
| "Sign up at forms.gle/abc", | |
| "Email tutor at gmail dot com", | |
| "Visit evil.com", | |
| "What\u200bsApp me", | |
| "Whats-App me", |
abhalsod-sonata
left a comment
abhalsod-sonata
left a comment
There was a problem hiding this comment.
Thanks for the clear write-up. Skipping classification only when nothing else in the post can reach or redirect readers is a reasonable rule, and "a false match only means the post is classified as today" is a good property to build on. One blocker and some hardening, all inline:
- Blocker:
has_risk_signals()is quadratic on long input (forum/ai_moderation/links.pyline 49). The email alternative has no start anchor, so on a long word without an@the regex retries from every character. On this branch a post with a trusted link plus one long word takes 7 s at 40,000 characters and 62 s at 160,000. Nothing in the LMS caps the post size (DATA_UPLOAD_MAX_MEMORY_SIZE = Noneinopenedx/envs/common.py, and the discussion API'sraw_bodyhas nomax_length), and moderation runs inside the create request. So any learner in a course with AI moderation enabled can tie up an LMS worker with a single post. A lookbehind makes it linear. - Misses are the risky direction. A false match costs nothing, but a miss means the post is never classified. On this branch all of these pass as "only trusted links" when posted with a Zoom link:
discord.gg/abc,linktr.ee/helper,forms.gle/abc,shorturl.at/abc,tutor at gmail dot com,evil.com,What\u200bsApp(zero-width space) andWhats-App. Three small suggestions inline (lines 48, 50 and 129-131) catch all of them using onlyre,htmlandunicodedata, so there is no new dependency and no longer TLD list to maintain. - Optional: log when a post is skipped (
forum/ai_moderation/service.pyline 305). Skipped posts leave no audit log entry, so today there is no way to see how often the trusted-link exemption is used, or abused. - Docs: the accepted gap is a bit wider than text-only lures.
codepen.iopens and Zoom registration pages are written by whoever posts them, so the lure can sit on the trusted page itself. That is the same reason the docs give for avoidinggithub.io. It's worth saying next to that warning that, with this change, a short post linking to a trusted domain is not classified at all.
Checked locally (Python 3.11, Django 4.2) with all the suggestions applied: the full suite passes (503, including 8 new tests that fail on the current head), and black, pycodestyle, pydocstyle, pylint and mypy are clean.
A post whose links all point at trusted domains could still be deleted. Once its link was removed, short text such as "Use this like fo registation : URL" read as a phishing lure to the classifier, which flagged it about half the time even with the link left in (LP-1310). Such a post is now not classified at all, unless the rest of it offers another way to be contacted or redirected: a link to any other domain, embedded media, an email address, a phone number, or a messaging app mention. Those posts are classified as before, without their trusted links, so a scam that adds a trusted link is still caught. The check undoes HTML entities, full-width letters and invisible characters first, and every skipped post is logged. Link handling now runs in linear time. Moderation runs inside the request that creates a post, posts have no size limit, and the anchor and email patterns took from seconds to minutes on a few tens of thousands of characters of hostile input.
18100e2 to
539eaab
Compare
|
Thanks for the thorough review. All four points are addressed in 539eaab.
The full suite passes (522 tests), and black, pycodestyle, pydocstyle, pylint, mypy and doc8 are clean. The PR description has the timing numbers and a devstack run against production XPert. |
1 similar comment
|
Thanks for the thorough review. All four points are addressed in 539eaab.
The full suite passes (522 tests), and black, pycodestyle, pydocstyle, pylint, mypy and doc8 are clean. The PR description has the timing numbers and a devstack run against production XPert. |
abhalsod-sonata
left a comment
abhalsod-sonata
left a comment
There was a problem hiding this comment.
Approving. Re-checked at 539eaab: all four points are addressed, and rewriting the anchor handling was a good catch.
- Performance: checked locally. The old
_ANCHOR_REtook 40 s on 5,000 characters of<a href="x"with no>; the new_unwrap_trusted_anchors()handles 200,000 characters of it in 0.02 s. Every hostile pattern I tried (long words, runs of<a,</a>anda.) stays under 0.1 s forstrip_trusted_links()plushas_risk_signals()at 200,000 characters. Unwrapping behaves as before: trusted anchors become their text, while untrusted and unclosed anchors stay in place and get classified. - Misses: every case from my review is caught now.
- Logging and docs: look good.
The full suite passes locally (522 tests, Python 3.11, Django 4.2), lint is clean and CI is green.
Description
Follow-up to #66 (LP-1310). #66 removes links to trusted domains before a post is sent to the XPert spam classifier. That is not enough for short posts. This comment:
reaches the classifier as
Use this like fo registation : URL, which it reads as a phishing lure. Against production XPert it was flagged in 2 of 2 runs after stripping, and in 2 of 4 runs even with the Zoom link left in. Changing how the link is replaced cannot make it pass reliably.This PR stops classifying a post whose links are all trusted, unless the rest of the post gives readers another way to be contacted or redirected:
http(s)://orwww., as a bare domain followed by a path under any TLD (codepen.oi/editor/…,discord.gg/…,forms.gle/…), as a bare domain under a common TLD (bestessays.com), as an anchor, or as embedded media (<img>,<iframe>, …)HTML entities, full-width letters and invisible characters are undone before this check, so
evil.com,What\u200bsApp(a zero-width space inside) andWhats-Appare caught.A post with any of these is classified exactly as before, without its trusted links, so a scam that adds a trusted link is still caught. Posts without trusted links are unaffected, and links to untrusted domains, including lookalikes such as
codepen.oi, are still judged by XPert. A false match only means a post is classified as it is today. Every post that skips the classifier is logged (AI moderation skipped, only trusted links: <id>), because it gets no audit log entry.JIRA: https://2u-internal.atlassian.net/browse/LP-1310
Linear-time link handling (also fixes code from #66)
Moderation runs inside the request that creates a post, and the LMS does not limit post size, so the link handling has to stay linear on hostile input. It did not:
c621d75/ first version of this PR)<a href="https://codepen.io/x">yrepeated (anchor pattern from #66)<a href="x"repeated, no>(anchor pattern from #66)The anchor pattern shipped in #66, which is already on
release-ulmoand pinned in edx-internal, and it runs on every post in a course with AI moderation enabled once trusted domains are configured. The post does not need to contain a trusted link. Anchors are now found by matching opening and closing tags separately; an opening tag cannot run past the next<. Every pattern alternative that can match a long run of characters only starts at the beginning of a word.Changes
forum/ai_moderation/links.py:has_risk_signals();forum/ai_moderation/service.py: skips classification when trusted links were removed andhas_risk_signals()finds nothing, and logs the skip.docs/how-tos/configure_ai_moderation.rst: describes the rule underAI_MODERATION_TRUSTED_LINK_DOMAINS, and warns that a lure can sit on a trusted page itself.0.7.4.Things to note
release-ulmocommit before the edx-internal PR is merged. The trusted domains (codepen.io,zoom.us) are already configured in edx-internal. Because of the anchor slowdown above, this is worth deploying soon.Testing
60 new tests:
has_risk_signals()cases: bare links on any TLD, spelled-out and entity-encoded contacts, zero-width and full-width characters, and everyday CSS, JavaScript and file paths that must not count as a risk.The full suite passes (522 tests).
black, pycodestyle, pydocstyle, pylint, mypy and doc8 are clean.
On devstack (LMS, Python 3.11) against production XPert, with
codepen.ioandzoom.ustrusted:Use this like fo registation : <a href="https://us02web.zoom.us/meeting/register/…">URL</a>My solution: https://codepen.io/… why is the footer not sticky?Guaranteed A grades! WhatsApp +1-555-0199 https://us02web.zoom.us/j/…Register: https://us02web.zoom.us/j/… or email tutor@example.comdiscord.gg/…,tutor at gmail dot com,evil.com, a zero-widthWhatsApporWhats-Apphttps://chatgpt.com/,codepen.oi/…(not trusted)Manual testing instructions
AI_MODERATION_BACKEND,AI_MODERATION_API_URL,AI_MODERATION_CLIENT_ID,AI_MODERATION_USER_ID) withAI_MODERATION_TRUSTED_LINK_DOMAINS = ["codepen.io", "zoom.us"], and enablediscussions.enable_ai_moderationanddiscussions.enable_ai_auto_delete_spamfor a test course.Use this like fo registation : <a href="https://us02web.zoom.us/meeting/register/<id>">URL</a>. It stays visible, is not flagged, gets noModerationAuditLogentry, and the LMS log showsAI moderation skipped, only trusted links.Guaranteed A grades! WhatsApp +1-555-0199 https://us02web.zoom.us/j/<id>. It is flagged and soft-deleted, with aModerationAuditLogentry.Merge checklist:
Check off if complete or not applicable: