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.
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:
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.
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.
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).
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.
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
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.
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.
If I remember correctlry ~s is already optimising with the no-interpolation case? If yes I’d rather use ~s mor all possible cases and use ~S to signal that the string contains something that looks like interpolating or escapes, but are not.
In other checks provided by this library sigil_S/2 is blindly accepted as an argument to raw/1 and family, and sigil_s/2 is alerted. First implementation (that is now ditched) had examined all the sigil_s/2 content to decide whether to alert or not, based on the actual interpolation presense. That is slow and not transparent for the user.
To me it’s clear that <<"some static string">> is a compile time static value generated by ~s so my humble opinion is that it’s better to stick to ~s for most cases except for escaping concerns. Though I’m not saying it is the one true choice, I can just disable this specific rule anyway.
Edit: or your are specifically targetting compilation time, in that case the rule makes sense. I disagree because in our codebases ~s (and sigils in general) are pretty rare, and it’s better to signal escaping concerns with uppercase sigils while paying a very tiny time cost for it. But the other way is valid too.
Yes, I meant compile time, of course, since Elixir compiles down to Erlang forms and those have no such thing as “string interpolation.” It does not actually matter, if a sigil is a macro or not, because it’s nevertheless not only expanded, but also applied during compilation phase (and it gets expanded to the funciton anyway.)
in our codebases ~s (and sigils in general) are pretty rare
Then it does not matter to your codebases at all. All code analysis tools, though, including but not limited to oeditus_credo, will treat the difference between ~s and ~S sigils strictly per documentation, thinking that ~s is dynamic and should be alerted if used in suspicious context.
You said the runtime cost for long strings […] might be huge (whereas it is zero) but you meant compile time, of course :p.
Anyway, I don’t see the documentation discouraging lowercase sigils when there is no interpolation. But I agree my team could use a cultural shift where we would default to ~S in most cases indeed.
Also, I could be nice to support something like --ignore-test-files when using with mix archive.install, wdyt? Maybe it’s already there but I did not find it after a quick docs scan. Well it’s a credo wrapper so it’s all in there.
I might be interested in this one, if it becomes less opinionated (I love sigils, fwiw.)
Namely, I would encourage using the same style, configured through config, let the default be your verbose one. If you don’t mind providing a PR, I am all in to merge it.
