Fix 400 when "clyde" is in any webhook username#1009
Conversation
MarkKoz
added
t: bug
MarkKoz
requested review from
eivl and
manusaurio
and removed request for
a team
ghost
added
the
MarkKoz
changed the title
|
Sentry issue: BOT-2A |
Discord just disallows this name.
MarkKoz
force-pushed
the
bug/mod/bot-2a/webhook-clyde
branch
from
e935a9f to
bcf6993
Compare
MarkKoz
changed the title
|
Sentry issue: BOT-3Y |
Numerlor
left a comment
Numerlor
left a comment
There was a problem hiding this comment.
I believe the cyrillic е would be better here and seems to look the same as a normal e 
The e's case is now also "destroyed" when it goes through the sub, so I think it wouldn't hurt to find something that looks like an E, if there is something like that and is worth the effort.
Do we also need to sub the names in reddit/python_news? The consistency is nice but having clyde in those is extremely unlikely
| return urls | ||
|
|
||
|
|
||
| def sub_clyde(username: Optional[str]) -> Optional[str]: |
There was a problem hiding this comment.
In what case username can be None? So much how I see, this should pass str every time.
There was a problem hiding this comment.
It's optional here though every use of that function passes a name.
bot/bot/cogs/watchchannels/watchchannel.py
Lines 199 to 207 in bcf6993
I don't see a problem with leaving it optional anyway.
There was a problem hiding this comment.
If this internally handles the None (and it does) then I agree the annotation is appropriate, but why does it handle it? Is it to be "shielded" from callers which may accidentally give a None? Won't that just push the error further, since something else that is expecting a string will get the declyded None?
There was a problem hiding this comment.
None is a valid value for webhook username (acts like an empty string and defaults to the webhook name) that may get passed in so I don't think there's any problem with letting None through.
There was a problem hiding this comment.
In that case I agree that this is perfectly fine.
Good find. The website I used didn't show Cyrillic characters as characters based on "e" (probably only considered latin alphabet). Cyrillic also has an upper case E so I will use that too. I wonder if there's a good way to do a case sensitive substitution. May just be easiest to chain two substitutions.
It's only one extra line of code, so better safe than sorry. I'd say this should be used for any name that's coming from an external source. |
The Cyrillic characters are more likely to be rendered similarly to their Latin counterparts than the math sans-serif characters.
kwzrd
left a comment
kwzrd
left a comment
There was a problem hiding this comment.
There's one small thing that I'd like to see adjusted, but otherwise I think this is a good fix.
My personal approach would have been turning it into something like clyd[e] (if that fixes the 400) just because I'd see that as more predictable w.r.t. various fonts and platforms. It'd also feel more "direct" to me, i.e. fixing the issue rather than "hiding" it, but I don't have a problem with this approach (the only change requested is the "" -> None issue).
| def sub_clyde(username: Optional[str]) -> Optional[str]: | ||
| """ | ||
| Replace "e"/"E" in any "clyde" in `username` with a Cyrillic "е"/"E" and return the new string. | ||
|
|
||
| Discord disallows "clyde" anywhere in the username for webhooks. It will return a 400. | ||
| Return None only if `username` is None. | ||
| """ | ||
| def replace_e(match: re.Match) -> str: | ||
| char = "е" if match[2] == "e" else "Е" | ||
| return match[1] + char | ||
|
|
||
| if username: | ||
| return re.sub(r"(clyd)(e)", replace_e, username, flags=re.I) |
There was a problem hiding this comment.
This will also return None if an empty string is given, which I suppose is fine if they are interchangeable in the webhook username field, but the docstring denies that. Explicitly checking for None on L131 seems like a better idea than adjusting the docstring though.
With PR #1009 merged, we now apply the same fix to our relay function. This prevents the "clyde" word from sneaking into the webhook username, which is forbidden and will return a 400.
5 participants
Discord just disallows this name.
The replacement character is Mathematical Sans-Serif Small E, which I felt was the best choice due to its similarity to a normal "e". However, I'm open to changing it to another character, putting it in parenthesis, or whatever else.