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

Handle downcased params #28

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

josh-works
Copy link

I'm copy-pasting some of my comment from #19 here.

tl;dr I believe the email RFC is one where we should not make judgements on the capitalization (or lack there of) of email headers, like To, From, etc.

From https://tools.ietf.org/html/rfc2821#section-2.4:

Verbs and argument values (e.g., "TO:" or "to:" in the RCPT command and extension name keywords) are not case sensitive, with the sole exception in this specification of a mailbox local-part (SMTP Extensions may explicitly specify case-sensitive elements)

The "mailbox local-part" would be smith in [email protected]; RFC281 says that [email protected] is distinct from [email protected], but goes on to say

However, exploiting the case sensitivity of mailbox local-parts impedes interoperability and is discouraged.

Ah. RFC 822, section 3.4.7 CASE INDEPENDENCE:

Except as noted, alphabetic strings may be represented in any combination of upper and lower case.

When matching any other syntactic unit, case is to be ignored. For example, the field-names "From", "FROM", "from", and even "FroM" are semantically equal and should all be treated identically.


I, and others, like @zeknox were thrown for a bit when we tried to use standard Ruby hash syntax for testing griddler-mailgun in our applications.

we built hashes like this:

email = {
	to: '[email protected]',
	from: '[email protected]'
}

Because this hash does not matching the expected casing, griddler-mailgun was trying to operate on a nil, and not surfacing the actual problem to the user.

This PR updates the Griddler::Mailgun::Adapter class to downcase all params and headers; it of course does not change the output of the class; just updates some of the internals.

All tests still pass. I don't do OS PRs a lot, so I'm more than happy to make any modifications. If this PR is accepted, #27 should be rejected and closed, as that documentation update would now be made irrelevant.

I'm happy to squash this PR into a single commit, as well. Just let me know what would be helpful!

spec/griddler/mailgun/adapter_spec.rb Outdated Show resolved Hide resolved
spec/griddler/mailgun/adapter_spec.rb Outdated Show resolved Hide resolved
spec/griddler/mailgun/adapter_spec.rb Outdated Show resolved Hide resolved
spec/griddler/mailgun/adapter_spec.rb Outdated Show resolved Hide resolved
lib/griddler/mailgun/adapter.rb Outdated Show resolved Hide resolved
@josh-works josh-works marked this pull request as ready for review June 28, 2019 02:06
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.

2 participants