OeditusCredo - a pack of supplemental Credo checks

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

Everything else is fixed in v0.3.1. Enjoy!

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.

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.

Ok, cool

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.

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!

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.

v0.4.0 introduces mix oeditus_assistant_rules which basically generates .aiassistant/rules/oeditus.md file for remote AI assistants to tell them all standard credo + oeditus credo rules to be obeyed upfront.

Enjoy.

v0.5.0 introduces my favorite check I had in my mind when I started this project.

You have an imperative status machine here. With a suggestion to refactor it to stop relying on silly imperative hard-coded statuses and benefit from leveraging the actual FSM.

Latest ragex also received the respective analysis and suggestions for refactoring.

Enjoy.

If you are on Elixir 1.20 and explicitly have {:typle, “~> 0.1”, only: [:dev]} included, you’ll get a check for unsolicited/abused Access calls on maps with a known literal atom keys.

a = map[:key] #⇒ ✗ FLAGGED with `use map.key`
a = kw[:key]  #⇒ ✓ NOT FLAGGED

v0.6.0

New: UnnecessaryInterpolatingSigil check

A new readability check flags lowercase sigils (~s, ~c, ~w, ~r) that contain no #{} interpolation and can be replaced with their uppercase counterparts (~S, ~C, ~W, ~R). This makes intent explicit—the content is static and will never contain dynamic expressions. Particularly useful alongside the XSS check: raw(~S"") is immediately recognizable as safe compile-time HTML.

Improved: [CWE-200] SensitiveDataExposure (fewer false positives)

The check previously flagged static Logger messages like “Unknown message type in queue, skipping” because “skipping” contains the substring “pin” (a sensitive term). The check now:

• Skips plain binary string arguments entirely (static text cannot leak runtime secrets)
• Only inspects keyword list values, not keys (metadata labels are not sensitive data)
• Only checks dynamic parts of interpolated strings, ignoring static text fragments
• Correctly recurses into function call arguments (e.g. Kernel.to_string(token) in interpolation)

Improved: [CWE-79] XSSVulnerability

Now detects Phoenix.HTML.raw/1 in addition to bare raw/1. String literals and non-interpolating sigils (~S) are recognized as safe and no longer flagged.

Improved: [CWE-502] UnsafeDeserialization

Piped calls like data |> Base.decode64!() |> :erlang.binary_to_term([:safe]) are now correctly recognized as safe.

Improved: UnmanagedTask

Task.start_link/1 was removed from the flagged functions—it is a valid supervised pattern. The check now only flags Task.async/1 and Task.start/1.

Enjoy!