-
Notifications
You must be signed in to change notification settings - Fork 4
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
MIME4j-268 MIME4J-269 Convenience helpers... #12
Conversation
chibenwa
commented
Mar 9, 2018
- Expose a LENIENT default config
- Provide a helper method for Message to byte[] conversion
@@ -26,6 +26,12 @@ | |||
*/ | |||
public final class MimeConfig { | |||
|
|||
private static final MimeConfig LENIENT = MimeConfig.custom() |
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.
Why private?
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.
Oups yes should of course be public....
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.
It's not only lenient (and btw you don't even call setStringParsing(false)
) but dangerous : max values are here to protect use from malicious input.
What happens with a mail crafted to overflow the parser ? Can I trigger JVM OutOfMemoryException with a single mail ?
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.
I guess so...
Then maybe I should drop MIME4J-269, and introduce a ticket for defining in details constants to be used safely, while not failing on legitimate email, that can be used by James.
What do you think?
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.
Not sure to understand
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.
I mean, as you mention, dropping all limits to have no input limits is unsafe.
We'd better use a mime-config carefully defining a "safe enough/lenient enough" limit instead that we should be using in James.
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.
agree
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.
I propose 100 M as a safe upper bound for maximum content length and keep the other ones unlimited.
This should be enough to not overwhelm the JVM too much while still being very permissive.
We can call such a constant "PERMISSIVE".
WDYT?
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.
why not, needs some testing to check what the actual memory consumption is.
…g limit This aims at remaining very permissive while still denying a single email to use all JVM memory.
DefaultMessageBuilder messageBuilder = new DefaultMessageBuilder(); | ||
messageBuilder.setMimeEntityConfig(MimeConfig.PERMISSIVE); | ||
messageBuilder.parseMessage(new ByteArrayInputStream(outputStream.toByteArray())); | ||
} |
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 you add some tests with (very) long lines please?
Merged |