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

feat (contacts): format/clean emails of carddav #51478

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

timedin-de
Copy link

@timedin-de timedin-de commented Mar 14, 2025

Summary

TODO

  • ...

Checklist

Sorry, something went wrong.

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of do server side sanitizing its better to only do this client side and on server do not sanitize but validate.
Meaning instead you could throw bad request when \OCP\Mail\IMailer::validateMailAddress fails for a mail address

@susnux susnux added 3. to review Waiting for reviews feature: carddav Related to CardDAV internals enhancement labels Mar 14, 2025
@timedin-de
Copy link
Author

Maybe instead of do server side sanitizing its better to only do this client side and on server do not sanitize but validate. Meaning instead you could throw bad request when \OCP\Mail\IMailer::validateMailAddress fails for a mail address

Yeah the problem is that davclients are very diverse and seemingly none do this email sanitize.

I agree will implement 👍. But we need to somehow inform the user when and why sync requests fail.

@SebastianKrupinski
Copy link
Contributor

Hi @timedin-de

I agree with @susnux we should not sanitize or manipulate the data in the storage layer (CardDavBackend) this should be done further up in the chain, probably at the entry point to the server.

The user can be informed with a 400 error and a custom message of the issue.

@timedin-de

This comment was marked as outdated.

Signed-off-by: TimedIn <git@timedin.net>
@timedin-de
Copy link
Author

@susnux @SebastianKrupinski So i hope i've met both you recommendations:

  • Emails are only being trimmed not all spaces removed
  • Implementation is in OCA\DAV\CardDAV\Validation\CardDavValidatePlugin. CardDavBackend stays untouched
  • Added unit tests
  • We return a 400 Error if the email is completely invalid on submit

For Website this works quite good even without the frontend changes. DavX⁵ seems to abort the whole sync process for all contacts :/

timedin-de added a commit to timedin-de/contacts that referenced this pull request Mar 14, 2025
fix: dont remove spaces in email: nextcloud/server#51478 (comment)

Signed-off-by: TimedIn <git@timedin.net>
timedin-de added a commit to timedin-de/contacts that referenced this pull request Mar 17, 2025
fix: dont remove spaces in email: nextcloud/server#51478 (comment)

Signed-off-by: TimedIn <git@timedin.net>
@timedin-de timedin-de requested a review from susnux March 19, 2025 07:31
@susnux
Copy link
Contributor

susnux commented Mar 19, 2025

DavX⁵ seems to abort the whole sync process for all contacts :/

maybe open a issue on DavX

Signed-off-by: TimedIn <git@timedin.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement feature: carddav Related to CardDAV internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contact Email Address not cleaned/validated
3 participants