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: remove tspecials from type/subtype #317

Merged
merged 8 commits into from Jan 30, 2024

Conversation

milankonir
Copy link
Contributor

According to the RFC, content type and subtype cannot contain special characters and any such character will fail parsing. Removing the characters from the type/subtype can help successfully parsing the content type that contains some extra garbage.

@@ -1,7 +1,7 @@
Content-Type: multipart/alternative; boundary="----=_Part_10541_215446765.98298273965074084"

------=_Part_10541_215446765.98298273965074084
Content-Type: <html>
Content-Type:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This had to be adjusted as <html> was no longer failing validation due to < and > being removed in fixMangledMediaType

@coveralls
Copy link

coveralls commented Jan 3, 2024

Coverage Status

coverage: 86.345% (+0.1%) from 86.213%
when pulling 71fa5b1 on milankonir:feat-filter-tspecials
into ed0394e on jhillyerd:main.

@jhillyerd
Copy link
Owner

Part of the goal of enmime is to flag bad input, I know that we were already silently trying to fix up borderline media type strings; but this character stripping is quite aggressive in my opinion.

I'm OK making enmime more resilient to malformed messages, but they should either be optional or add an Error to the Part. Adding Errors feels like the more useful solution here, but I'm not sure what effort level is required to implement it.

@milankonir
Copy link
Contributor Author

Part of the goal of enmime is to flag bad input, I know that we were already silently trying to fix up borderline media type strings; but this character stripping is quite aggressive in my opinion.

My reasoning behind the change was that any special character in the content type/subtype will fail the default mime parser while stripping out any garbage can result in a valid content type, but I perfectly understand you don't want to make this the default behavior.

I'm OK making enmime more resilient to malformed messages, but they should either be optional or add an Error to the Part. Adding Errors feels like the more useful solution here, but I'm not sure what effort level is required to implement it.

I have made the invalid character stripping opt-in via the parser option. Let me know if this is acceptable.

Happy to address any other issues you see with the PR.

header.go Show resolved Hide resolved
header.go Show resolved Hide resolved
mediatype/mediatype.go Show resolved Hide resolved
@milankonir
Copy link
Contributor Author

Apologies for the long loop, should be adjusted per your comments.

Copy link
Owner

@jhillyerd jhillyerd left a comment

Choose a reason for hiding this comment

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

Looks good, couple small changes

part_test.go Outdated Show resolved Hide resolved
mediatype/mediatype.go Outdated Show resolved Hide resolved
milankonir and others added 2 commits January 29, 2024 23:50
Co-authored-by: James Hillyerd <james@hillyerd.com>
Co-authored-by: James Hillyerd <james@hillyerd.com>
@jhillyerd jhillyerd merged commit 61e611c into jhillyerd:main Jan 30, 2024
9 checks passed
@jhillyerd
Copy link
Owner

Thank you!

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.

None yet

3 participants