Skip to content

Conversation

@tinohager
Copy link
Contributor

@tinohager tinohager commented Aug 18, 2025

This pull request introduces stricter validation for mailbox creation in the SMTP parser to prevent malformed or excessively large mailbox names from being accepted. The main changes are focused on enhancing error handling and input validation when constructing mailbox objects.

Mailbox creation validation and error handling:

  • Updated TryMakeMailbox in SmtpParser.cs to throw an SmtpResponseException with MailboxNameNotAllowed if mailbox creation fails due to invalid input, instead of silently returning null.
  • Improved the CreateMailbox helper to check for null values after converting the local part and domain/address, ensuring that mailbox objects are only created with valid data.

Input size validation:

  • Added a check in StringUtil.Create to return null if the input sequence length exceeds ushort.MaxValue, preventing creation of mailbox names that are too large.
Span<byte> buffer = stackalloc byte[(int)sequence.Length];

`Span<byte> buffer = stackalloc byte[(int)sequence.Length];`
@tinohager
Copy link
Contributor Author

@cosullivan What we could still discuss is whether we want to set a different maximum value instead of ushort.MaxValue.
Do you have any suggestions?

@cosullivan
Copy link
Owner

Maybe its better for the max value to a be a parameter that defaults to the maximum possible value to mimic the current behaviour and then just to alter it for things like a mailbox where we know there is some sort of reasonable size?

Replaces unsafe pointer usage in StringUtil.Create with safe Encoding.GetString overloads. Also changes length check from ushort.MaxValue to short.MaxValue for consistency.

Encoding.GetString(ReadOnlySpan<byte>) available with .NET Standard 2.1
@tinohager
Copy link
Contributor Author

@cosullivan I would like to record it that way. For configuration via config, the module is in too many static classes. I don't want to start such a major overhaul right now. I don't think it makes much sense to configure the value either. It is important that the application does not crash here.

@WaldenL
Copy link

WaldenL commented Aug 21, 2025

Have we made a decision, hopefully not, to abandon .net framework compatibility? We cannot use any .net 2.1 standard API’s and maintain framework compatibility. 😢

@tinohager
Copy link
Contributor Author

@WaldenL We replaced netstandard2.0 with netstandard2.1 last September because there are various security-critical issues that are difficult to address with netstandard2.0. Don't you feel like we should slowly start saying goodbye to .net 4?

@cosullivan cosullivan merged commit a5f25d0 into cosullivan:master Aug 28, 2025
1 check passed
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