fix(email): reinforcement of the validator and skip invalid address when sending alerts#78
Conversation
…hen sending alerts
|
Hi, Is there an update with this fix planned for soon ? Kind regards, |
There was a problem hiding this comment.
I think you can just try/catch the line that throw the exception and do a continue with a log before it. Please remove also the comment, if you want you can add the line Invalid email address, continue to the next one to the log with the exception message
There was a problem hiding this comment.
Pull request overview
This PR hardens email handling for back-in-stock subscriptions to prevent invalid/malicious email strings from being accepted and to avoid alert-sending failures caused by RFC compliance exceptions during email delivery.
Changes:
- Switch subscription form email validation to Symfony
Email::VALIDATION_MODE_STRICT. - Catch
RfcComplianceExceptionduring alert sending to continue processing remaining subscriptions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/Form/SubscriptionType.php |
Tightens validation for the subscription email field using strict RFC validation. |
src/Command/AlertCommand.php |
Prevents the alert command from stopping on RFC-compliance email errors by catching RfcComplianceException. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -44,27 +45,32 @@ protected function execute(InputInterface $input, OutputInterface $output): int | |||
| //I think that this load in the long time can be a bottle necklace | |||
There was a problem hiding this comment.
Spelling/wording: "bottle necklace" should be "bottleneck" (and add a space after //) to keep the inline comment understandable.
| //I think that this load in the long time can be a bottle necklace | |
| // I think that this load in the long time can be a bottleneck |
| 'constraints' => [ | ||
| new NotBlank([], null, null, null, ['webgriffe_sylius_back_in_stock_notification_plugin']), | ||
| new Email([], null, null, null, ['webgriffe_sylius_back_in_stock_notification_plugin']), | ||
| new Email(['mode' => Email::VALIDATION_MODE_STRICT], null, null, null, ['webgriffe_sylius_back_in_stock_notification_plugin']), |
There was a problem hiding this comment.
This changes the email validation behavior (strict RFC mode). Since the repo already has Behat coverage around guest subscription flows, it would be good to add a scenario that submits a clearly invalid/malicious email (like the one from the PR description) and asserts the form shows a validation error and no subscription/success email is created, to prevent regressions.
| } catch (RfcComplianceException $e) { | ||
| // Invalid email address, continue to the next one | ||
| $this->logger->warning($e->getMessage()); | ||
| } |
There was a problem hiding this comment.
In the RfcComplianceException handler, the subscription remains with notify=false, so the command will hit the same invalid address on every run (repeated exceptions + noisy logs). Consider either removing the invalid subscription (as is done when channel/productVariant is missing) or marking it as notified/failed so it won’t be retried, and log with structured context (e.g., subscription id/email) instead of only the raw exception message.
2 participants
Hi,
On the last version for Sylius 1.X, version 4.1.0, we had a bug blocking the sending of alerts.
Indeed, if the email format was incorrect, we had an RfcComplianceException.
A person try an injection with this kind of email :
[email protected]'&&sleep(27*1000)*ckfqsx&&'just by changing the input type from email to text, the backend Email validator accept this email.To prevent that I add it the redtriction mode :
Email::VALIDATION_MODE_STRICTOn alert sending, I had a try catch to no stop alert sending on email error.
If you accept this PR, can we have an 4.1.1 or 4.2.0 tags for Sylius 1 pls ?
Have a nice day !
Kind regards,
Kévin