Sanitise avatar URL client-side #1420
Conversation
dokterbob
force-pushed
the
dokterbob/avatar_url_clientside
branch
from
369f481 to
775bc71
Compare
dosubot
bot
added
the
size:L
dosubot
bot
added
backend
| @@ -28,7 +38,9 @@ const MessageAvatar = ({ author, hide }: Props) => { | |||
| if (isAssistant && selectedChatProfile?.icon) { | |||
| return selectedChatProfile.icon; | |||
| } | |||
| return apiClient?.buildEndpoint(`/avatars/${author || 'default'}`); | |||
|
|
|||
| const authorAvatarId = getAuthorAvatarId(author); | |||
There was a problem hiding this comment.
i don't think doing this in the client is a good idea since developers also use chainlit with custom frontends
There was a problem hiding this comment.
Maybe we can give custom frontend implementors (2-line!) instructions to implement this?
This patch would not only improve security, but being consistent in how we deal with filenames will make the developer experience much more robust. This would replace a . (and other characters, think unicode) with _, allowing any characters in avatar names.
We currently do not: a lot of author names will prevent the avatar from working. This patch solves that, fully, without compromising security again.
There was a problem hiding this comment.
Proposed compromise:
- We keep allowing spaces and upper characters (basically not change the backend).
- We throw a deprecation error every time an 'invalid' URL comes in.
- We update the frontend code already.
This would let existing users know of what's coming, recover pre-existing functionality in cases of special characters, guarantee sufficient security for now and maximal security long-term.
|
I actually want to implement the compromise stated above before continuing. #1420 (comment) |
* e2e assertions about avatars. * Change avatar URL client side.
dokterbob
force-pushed
the
dokterbob/avatar_url_clientside
branch
from
c9fda28 to
6aa287a
Compare
More structural solution to #1370.
[a-zA-Z0-9]with_.[1] Instead of having chainlit start again and again, we can merge tests with identical configuration, significantly speeding up tests (chainlit takes several seconds every time to start). Making tests cheaper, will enable us to run and write more of them, faster, allowing for a more mature product.