Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize bot input #110

Merged
merged 19 commits into from
Mar 3, 2023
Merged

Sanitize bot input #110

merged 19 commits into from
Mar 3, 2023

Conversation

missytake
Copy link
Contributor

@missytake missytake commented Mar 2, 2023

Not critical, as this input is coming from trusted users - nevertheless it should be fixed.

@missytake missytake requested a review from link2xt March 2, 2023 09:12
src/mailadm/conn.py Outdated Show resolved Hide resolved
tests/test_conn.py Outdated Show resolved Hide resolved
@link2xt
Copy link
Contributor

link2xt commented Mar 2, 2023

Overall I think validation of user input is not the right place to fix it. It's always dangerous to add this kind of validation because you have to copy-paste it to every place where the user inputs something, taking into account all the uses of user input which happen in a completely different place.

I think get_user() function should be fixed instead to URL-escape the address when it is used in the GET request.

@missytake missytake requested a review from link2xt March 2, 2023 10:28
@missytake
Copy link
Contributor Author

I moved the email sanitization check into the mailcow.py method - it doesn't change anything in the current state actually, because the only place where it's executed apart from tests is in the add_email_account method where the check happened before, but now it's a bit invisible to the add_email_account function that the sanitization actually happens. So if we refactor add_email_account again, it might get lost and maybe we have unsanitized email addresses somewhere in mailadm again. So it's better in protecting against MER-01-005 now, but worse in protecting against MER-01-004^^ I'm fine with both alternatives.

@missytake
Copy link
Contributor Author

Maybe I should have split this up into two different PRs - one is about validating emails (which get passed to the mailcow API: MER-01-004 and MER-01-005), the other is about validating token names (which gets passed to the tokenQRcode.png file name: MER-01-001).

src/mailadm/mailcow.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@missytake missytake force-pushed the missytake/sanitize-bot-input branch from bbcb91a to a1e7f1c Compare March 2, 2023 14:31
@missytake missytake requested a review from link2xt March 2, 2023 14:54
@missytake missytake requested a review from link2xt March 2, 2023 15:02
src/mailadm/conn.py Outdated Show resolved Hide resolved
@missytake missytake merged commit 43d2208 into master Mar 3, 2023
@missytake missytake deleted the missytake/sanitize-bot-input branch March 3, 2023 07:32
@dumblob
Copy link

dumblob commented Jun 8, 2023

For newcomers & archiving purposes - there is a vaguely related thread about bot commands security.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants