Fixed render_header_value eternal loop if a value of nil is present

[IceDragon200]

Found a terrible bug in prod.

I use render_header directly for some internal stuff, and found that if you pass nil in as the value, it loops forever

It was a really bad evening...

Changes proposed in this pull request

A fix for an eternal loop in render_header_value

[bcardarella]

Maybe I'm missing the obvious, but how is the guard any different than what is already there? The prior other implementation of that private function is expecting a list for the 2nd argument. So when it isn't a list shouldn't it defer to the 2nd implementation of the private function? The guard seems redundant in this case. Please explain 😄

[bcardarella]

What I mean by above: if you pass nil as the 2nd argument it will List.wrap(nil) which results in [] which won't match to the first function implementation. But with the guard you have it also won't match to the 2nd. Yes it avoids the infinite loop but it will also error out which also isn't great. What is the use case for passing nil as the 2nd argument? If the use case makes sense it would be best to provide an escape hatch function to avoid the runtime error.

[IceDragon200]

@bcardarella Indeed it will error out, and that is intended, the other options are:

• Let it loop (even worse than erroring)
• Return an empty string (creates invalid headers)
• Return the string "nil" (creates confusing situations, 'was it an explicit "nil" or the implicit :nil')
• Return nil (now we need to check if render_header_value returned in all it's callers, and this would mean render_header needs to handle nils, which in turn requires render_headers to also handle render_header nils)

As for the use of a guard:
The problem here is, the function expects a list of 1 or more elements.
The last overload of the function will wrap the given value into a list in order to trigger the previous (1 or more elements).
However, nil creates an empty list (0 elements); since it can't match the previous (1 or more), it falls back unto the List.wrap again and continues to loop.
The guard checks if the value is a list, and if it isn't it will wrap it, otherwise it will let it fall through and error.

This will not affect normal rendering of messages, since render_headers will drop any nils it encounters:
https://github.com/DockYard/elixir-mail/blob/master/lib/mail/renderers/rfc_2822.ex#L192-L193

Instead this change would affect anything calling render_header directly, I believe that anyone calling this function would expect to get back a valid header string, and if not, it should fail loudly, this will have the developer fix it on his side before calling it (as I had to do after finding the problem and exposing a multitude of other problems that caused it in the first place)

As for a use case:

My use case is actually for render_header_value, I use it to convert the headers into a string that can be inserted into the database, and passed along in a JSON payload.

But that is a private function and instead of making it public I opted to use render_header and then split on the colon to get the value (the added bonus was I got the normalized header key as well)

[andrewtimberlake]

This is already in master: https://github.com/DockYard/elixir-mail/blob/master/lib/mail/renderers/rfc_2822.ex#L118 so this pull request can probably be closed.

[IceDragon200]

It's been quite some time since I opened this...

@andrewtimberlake I know, because I patched that previously, that was a different issue, I don't remember the exact details.

But this case is different:

elixir-mail/lib/mail/renderers/rfc_2822.ex

Lines 114 to 118 in 519cc0e

	defp render_header_value(_key, [value | subtypes]),
	do: Enum.join([value | render_subtypes(subtypes)], "; ")
	defp render_header_value(key, value),
	do: render_header_value(key, List.wrap(value))

Go ahead and run any of the below lines in an iex terminal with mail loaded

Mail.Renderers.RFC2822.render_header("X-Header-Name", nil)
Mail.Renderers.RFC2822.render_header("X-Header-Name", [])

They will never return.

My patch aims to have it crash immediately if it encounters a list that wasn't handled by the previous function.

So the below would happen instead:

Mail.Renderers.RFC2822.render_header("X-Header-Name", nil) # => FunctionClauseError
Mail.Renderers.RFC2822.render_header("X-Header-Name", []) # => FunctionClauseError

[andrewtimberlake]

@IceDragon200 I’m closing this because this issue is now resolved with a refactor done in #93
Thanks for pointing this out.