OeditusCredo - a pack of supplemental Credo checks

Your Libraries & OS Mentoring Libraries
, ,
1

While I am working on the Language Agnostic Code Audit SaaS, which uses MetaAST (spoiler: I am expecting it to be in a good shape for announcing by May,) I play some experiments in my sandbox.

Here is the result of one of such experiments—the ready-to-use MIT-licensed Elixir library, covering 36 potential code smells, including but not limited to 17 CWEs from Top25 CWE list.

Please report back false positives! I am also eager to hear what checks had I missed to be added.

17 Likes
PropWise - the library to detect and suggest code deserving property based testing
2

Thank you for the library @mudasobwa. I have found it very useful. Already installed it in a project of mine and found a handful of issues.

I ended up changing the default configs in .credo.exs for:

  {OeditusCredo.Check.Security.PathTraversal, files: %{excluded: ["test/**/*.exs"]}},

User input is not an issue in tests dealing with paths. Maybe for this cause it could be useful to have the :exclude_test_files param.

Also when compiling the project it generated a few warnings under

  • elixir 1.20.0-rc.3-otp-28
  • erlang 28.1.1
1 Like
3

:heart:

Everything else is fixed in v0.3.1. Enjoy!

2 Likes
4

Definitely good to have a lot of options, though I don’t know that I would call all of these mistakes. For example:

  • MissingErrorHandling - Detects {:ok, x} = pattern without error handling

Sometimes it’s entirely valid to just match on an {:ok, _} = . This is especially true in cases where you don’t generally expect an error case and if you do you want it to raise/crash. Of course you can “handle” the error and raise/crash yourself, but having a MatchError can be a pretty clear indication for debugging of what went wrong. I often thing about this bit of Joe Armstrong’s thesis (worth a read, it’s very approachable):

Errors occur when the programmer does not know what to do. Programmers are supposed to follow specifications, but oden the specification does not say what to do and therefore the programmer does not know what to do. Here is a example:

Suppose we are writing a program to produce code for a microprocessor, the specification says that a load operation is to result in opcode 1 and a store operation should result in opcode 2. The programmer turns this specification into code like:

asm(load) -> 1;
asm(store) -> 2.

Now suppose that the system tries to evaluate asm(jump)—what should happen? Suppose you are the programmer and you are used to writing defensive code then you might write:

asm(load) -> 1;
asm(store) -> 2;
asm(X) -> ??????

but what should the ???’s be? What code should you write? You are now in the situation that the run-time system was faced with when it encountered a divide-by-zero situation and you cannot write any sensible code here. All you can do is terminate the program. So you write:

asm(load) -> 1;
asm(store) -> 2;
asm(X) -> exit({oops,i,did,it,again,in,asm,X})

But why bother? The Erlang compiler compiles

asm(load) -> 1;
asm(store) -> 2.

almost as if it had been written:

asm(load) -> 1;
asm(store) -> 2;
asm(X) -> exit({bad_arg, asm, X}).

The defensive code detracts from the pure case and confuses the reader—the diagnostic is oden no better than the diagnostic which the compiler supplies automatically.

All that said, there’s some good stuff in here, but just be careful of putting all things forward as if they were the “correct” thing to do, so this is more of feedback on documentation and giving the pros/cons of each of your checks. :man_shrugging:

5

I am the person who literally paid for printing three copies of Joe’s thesis for our internal library.

The question is each and every occurence of MissingErrorHandling must be triple-validated (here is my bug reportto elixir core, you might want to check the fix) and I thoughfully decided for opt-out with # credo:disable-for-next-line. After all, when somebody decides to use this library, they know why they do it.

6

Ok, cool :+1:

Really, I think this bit is the core of what makes me just a bit uncomfortable. I think there will probably be many less experienced people who see this and think “oh, cool, best practices” and then just do whatever the credo checks tell them to do. I agree that this library is great for those who know what they’re doing, but the documentation presents it less subtly, just saying “Custom Credo checks for detecting common Elixir/Phoenix anti-patterns, mistakes, and CWE Top 25 security vulnerabilities.”

And it’s interesting that (if I’m understanding it right) you use # credo:disable-for-next-line as a way to say "I checked this and decided it doesn’t apply here). I don’t see credo disables much and so they feel like noise for me they’re generally a last resort. Different standards for different teams and different projects, of course, but I’ve just never seen them used that way.

1 Like
7

I honestly cannot think of any other reason to use credo:disable whatsoever, save for “I checked this and decided it doesn’t apply here.” Do you?

I will add a note saying that all that crap is opinionated and should be not used at home or school, though, thanks for that!

8

Since v0.3.3, the checks accept standard params:

Every check accepts the following general parameters provided by Credo:

  • false — Disable a check entirely. When a check tuple uses false instead of a keyword list, the check is skipped and produces no issues.

    # Disable a check
{OeditusCredo.Check.Warning.NPlusOneQuery, false}

  • exit_status (integer()) — Override the exit status contributed by issues from this check. By default, all checks in the :warning category contribute exit status 16. Setting exit_status: 0 means the check still runs and reports issues, but they will not cause a non-zero exit code.

    # Run the check but don't fail CI on its issues
{OeditusCredo.Check.Warning.NPlusOneQuery, exit_status: 0}

# Custom exit status
{OeditusCredo.Check.Security.SQLInjection, exit_status: 2}

  • priority — Override the base priority for the check (:low, :normal, :high, :higher, or :ignore).

  • files — Restrict which files the check runs on:

    {OeditusCredo.Check.Security.SQLInjection, files: %{included: ["lib/my_app/repo.ex"]}}

These parameters can be combined with any check-specific parameters.

NB exit_status is specifically useful when one wants to keep the warnings in their local credo runs, but let’em pass CI for now.

1 Like