Clarify Elixir Boolean guide scope in its summary

[maennchen]

I’m not sure if I agree with the restriction that this only applies for Elixir Code calling Erlang.

The example of the page also works for Elixir only code.

However: I don’t believe that we should discourage if and cond. They are a core language construct and are more easily readable compared to case with booleans.

[josevalim]

To give more context, I started this discussion on the #security channel. My concern is how much of this is a security guideline. We could argue that it also applies to Elixir only code, but then it is a design discussion, not a security one. Otherwise, we may see people banning cond/if/etc from their codebases, with the assumption it will make them safer.

So, in my humble opinion, we need to either make the security scope clearer (as in this PR) or it should become a wider discussion about design, which should be moved out of security.

[maennchen]

My concern is how much of this is a security guideline.

In my opinion, this is a design issue (same as in a lot of other languages) which can potentially cause a security issue and should also be discussed here because of that.

Otherwise, we may see people banning cond/if/etc from their codebases, with the assumption it will make them safer.

I agree that the resulting guidance should not recommend banning whole language constructs. Instead, this article should educate readers about the issue.

How would you feel about replacing Prefer case over if, unless or cond with something along the lines of Make sure that the conditions of if, unless or cond are resulting in booleans and additionally link to https://hexdocs.pm/elixir/1.12.3/Kernel.html#module-truthy-and-falsy-values?

[josevalim]

Make sure that the conditions of if, unless or cond are resulting in booleans

That's misleading because those constructs are useful for handling nils. Imagine for example I receive a parameter and I want to fetch the user id, either inside the user_id or UserID keys:

user_id = params["user_id"] || params["UserID"]

Or a more advanced example:

if user_id = params["user_id"] || params["UserID"] do
  String.to_integer(user_id)
else
  conn.assigns.user_id
end

Asking the user to guarantee booleans in these scenarios will not improve the code and it is not advice I would personally give.

The advice I would give is to use or and and instead of || and && (and potentially case instead of if/unless) when you know the result is boolean only. The fact Erlang does not have nils, means || and && is indeed hardly useful when working with Erlang code. Hence my pull request.

[josevalim]

this is a design issue (same as in a lot of other languages) which can potentially cause a security issue and should also be discussed here because of that.

I am not sure if I agree with the conclusion here either. I would argue that bad code is in general a security issue. Poor function names, long function definitions, wrong abstractions, incomplete data validation, are all design issues which, by making the code harder to maintain, makes it harder to guarantee the code is safe.

By the logic above, one could argue we should then list all of these design issues here, but I think the guides would actually become less useful if it ends-up discussing entries that are indirectly security issues.

[voltone]

I agree the current wording is probably too generic: we were aiming for simple one-liners in the style "don't do this, do that", but of course it always depends a bit on the context. For this particular issue the affected operators/statements are so generic and the problematic scenarios so specific that it doesn't really translate into a one-liner.

Having said that, I do not think this issue is specifically (or even primarily) related to Erlang interworking. The concepts of nil and truthy values are part of it, but as the example shows, it is possible to shoot yourself in the foot simply based on wrong assumptions about the return value of a function. I have seen this exact bug in production code, and is a devastating one for security that went unnoticed for quite a while!

Should we perhaps first have a shot at documenting the anti-pattern? We can then decide whether the topic here can be dropped, rephrased, or replaced with a short reference to the anti-pattern...

[josevalim]

This has been moved to Elixir guides: https://hexdocs.pm/elixir/main/code-anti-patterns.html#non-assertive-truthiness

Perhaps we should have a general page talking about code quality and security, and saying that Elixir keeps a list of anti-patterns, linking here: https://hexdocs.pm/elixir/main/what-anti-patterns.html

[maennchen]

Perhaps we should have a general page talking about code quality and security, and saying that Elixir keeps a list of anti-patterns

I think that would be a good idea.

[voltone]

Great, I started working on a PR that replaces elixir_truthy.md with a list of "Additional resources" in the Introduction section. But I suppose the https://hexdocs.pm/elixir/main/what-anti-patterns.html link won't be stable until Elixir 1.16 is released...

[josevalim]

Thank you! I think we can point to main and you can subscribe to this issue: https://github.com/elixir-lang/elixir/issues/12747

Once it is closed, you can remove the /main part.

[maennchen]

@josevalim Would the elixir guides also be willing to include a security section pointing to the Secure Coding Guidelines? That could help improve the visibility of this document.

[josevalim]

Good call, definitely. I will do that now.

[josevalim]

Our guides:

[josevalim]

I will close this one.