-
Notifications
You must be signed in to change notification settings - Fork 196
feat(frontend/backend): add feature report spam #1551
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
base: master
Are you sure you want to change the base?
Conversation
816d1fb to
c615ecb
Compare
|
@Grandi0z This is turning out to be a big PR :-) It's still in draft but would you like some preliminary feedback from Victor ? |
|
I agree, it’s a good idea and it should simplify the next step.
|
|
AI is not yet good enough to write such a complicated feature and I see minimal intervention from you after it generated the code. The code is quite misplaced, bloated and certainly having many more features than the requested in the related task you post. I won't even try to read it all. My suggestion is:
Some examples of code misplace:
|
I GOT THE MESSAGE. In practice, we report spam because we don’t want to see that message anymore. Limiting the process to reporting to external services only, without local filtering, means the message remains in the inbox, which reduces the effectiveness of the mechanism. Don’t you think we should keep at least some form of filtering in place somewhere? |
|
Regarding local filtering and spam report - these are two quite distinct tasks and mechanisms. I didn't see local filtering request in the original task and since it is a different thing, I suggest you put it in another PR. Reporting spam allows spam services to get better, thus having mail servers prevent spam at its origin or during the routing of a message which overall reduces spam and makes things work better. We already have blocking lists and features in Cypht but those depend on Sieve - which again is a step ahead as filtering on the server is better than filtering on a client. Last effort here would be implementing local filtering as it has some drawbacks: works only on that instance of the webmail client, is slower and has a performance impact. However, it is still something useful when all other efforts fails, that's why I suggested you put it in a different PR. |
Hello @kroky , thank you for pointing out this detail. Indeed, the code was going a bit in all directions. I am currently in the process of refactoring everything. I also want to apologize for having modified your comment. My ego was hurt, and I reacted in an unprofessional manner. Thank you very much for your guidance and support. |
81a9c01 to
3992ee4
Compare
|
Thanks @Grandi0z , you are awesome! Looking forward to review the new code... |
5425ecf to
f4e3205
Compare
f4e3205 to
4babe26
Compare
| /** | ||
| * Starts the Report Spam section on the settings page | ||
| * @subpackage core/output | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature is already big enough that it deserves its own module. Please move everything from core and imap modules to a new module 'report_spam' or similar.
| * @return array Array with 'mime_message', 'mime_body', and 'boundary' | ||
| */ | ||
| if (!hm_exists('fix_spam_report_encoding')) { | ||
| function fix_spam_report_encoding($mime_message, $service_name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use a lib for this? I'd like to avoid all these low-lever regex operations if there is already a lib for this and we already are using mime manipulation libs + have a low-lever mime classes ourselves.
| if (!hm_exists('send_spam_report_via_smtp')) { | ||
| function send_spam_report_via_smtp($from_email, $to_email, $subject, $mime_body, $boundary, $user_config, $session, $service_name, $use_fallback_smtp = false) { | ||
| if (!class_exists('Hm_SMTP_List')) { | ||
| $smtp_file = (defined('APP_PATH') ? APP_PATH : dirname(__FILE__) . '/../') . 'modules/smtp/hm-smtp.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tight coupling of modules that we want to avoid. When you move all this code to a new module, make it depends on both - imap and smtp. Add your new handler modules after existing smtp modules that already load the server list ,connect to server, etc. We don't want to repeat operations. We don't want to have lower-level smtp calls in a module outside smtp...
| ini_set('default_socket_timeout', $timeout); | ||
|
|
||
| try { | ||
| $mail_sent = @mail($to_email, $subject, $mime_body, implode("\r\n", $headers)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If smtp fails, just fail. Cypht doesn't use php mail default function and configuring that properly is outside the scope of this task.
| $mime_message = preg_replace('/^X-Mailer:.*$/mi', '', $mime_message); | ||
| $mime_message = preg_replace('/\r\n\r\n+/', "\r\n\r\n", $mime_message); // Clean up extra blank lines | ||
|
|
||
| $encoding_result = fix_spam_report_encoding($mime_message, 'SpamCop'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could probably use the right encoding in get_mime_msg above and spare the "fixing" part here which seems hacky...
| * @return string|false IP address (IPv4 or IPv6) or false if not found | ||
| */ | ||
| if (!hm_exists('extract_ip_from_message')) { | ||
| function extract_ip_from_message($message_source) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imap module already has methods to return message headers. Please spare the low-level parsing of mime messages whenever possible.
| 'comment' => $comment | ||
| ); | ||
|
|
||
| $ch = curl_init('https://api.abuseipdb.com/api/v2/report'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a lib/api curl client - can't you reuse it here?
realted task
This PR introduces a spam reporting system in Cypht, allowing users to report unwanted messages to multiple external services.
Report Spam Feature
A comprehensive spam reporting system that allows users to report unwanted emails to multiple anti-spam services directly from their inbox.
Key Features:
Multi-service reporting - is implemented but removed from the UI, because services have different requirements for it..
Servives