Skip to content

Security: Add support for string keys#6

Open
bruce wants to merge 3 commits into
dkuku:masterfrom
bruce:bruce-string-keys
Open

Security: Add support for string keys#6
bruce wants to merge 3 commits into
dkuku:masterfrom
bruce:bruce-string-keys

Conversation

@bruce

@bruce bruce commented Jul 3, 2026

Copy link
Copy Markdown

👋 Hello, Daniel!

I'm evaluating using Celixir for CEL conditions in a big project I'm working on—needless to say, I'm very excited that you've put this together!

One thing I've run into: in our project, the variable data we'd bind comes from user-submitted (untrusted) data stored in JSON, which means all keys are strings (in fact, the condition itself might be user-supplied in some cases). We'd love to pass this data directly to Celixir without worrying that the keys would be converted to atoms as part of the process.

Warning

The ERLEF Secure Coding guidelines note that converting untrusted strings to atoms risks atom table exhaustion.

Since this is a common situation (JSON data, Ecto JSON columns, and external APIs all produce string-keyed maps, of course), I'd like to propose some changes to support string keys in Celixir, while keeping fully backward compatible support for atoms.

# String-keyed maps now work directly
Celixir.eval("severity == 'high' && count > 2", %{"severity" => "high", "count" => 3})
# => {:ok, true}

# Atom-keyed maps continue to work exactly as before
Celixir.eval("severity == 'high'", %{severity: "high"})
# => {:ok, true}

Please take a look and let me know what you think! This feels like a good usability and security improvement, and I'd love to work with you to make it viable for contribution to the library.


What Changed

The internal representation for variables and locals in Environment is
now %{String.t() => any()}. Atom-keyed input maps are normalized to strings
at the boundary via Atom.to_string/1 (safe direction, no atom creation).

Key design decisions

  1. Strings as canonical internal representation. CEL identifiers from the
    parser are already strings. Storing variables as strings eliminates conversion
    at every identifier resolution, so a net performance win.

  2. Indexed variable pool in the compiler. The Elixir AST requires atom
    variable names (Macro.var/2). Rather than calling String.to_atom on CEL
    identifiers (which would allow atom exhaustion via crafted expressions), each
    free variable is assigned a positional atom (:__cel_var_0__, :__cel_var_1__,
    etc.). The atom count is bounded by variables-per-expression, not by the
    variety of identifiers across all expressions.

Backward Compatibility

Should be fully backwards-compatible.

  • Environment.new(%{x: 5}); atom keys still work (converted to strings at boundary)
  • put_variable(:name, value); atom names still accepted
  • get_variable(env, :name); atom lookups converted to string internally
  • All existing tests pass unmodified; added string_keys_test.exs for string-specific tests

Extras

  • Took care of an existing unused variable variable warning in runtime.ex
  • mix format run

bruce added 3 commits July 2, 2026 23:52
- Switches Environment internals from atom-keyed to string-keyed maps.
- Atom-keyed input is normalized to strings at the boundary via
Atom.to_string/1. The compiler uses an indexed variable pool
(:__cel_var_0__, :__cel_var_1__, ...) instead of converting CEL
identifier names to atoms.
- No atoms are created from either variable data or expression
identifiers, preventing atom exhaustion from untrusted input on
both paths.

This should be backwards-compatible; atom-keyed and
string-keyed maps are accepted transparently and the tests
still pass, unchanged.

Signed-off-by: Bruce Williams <brwcodes@gmail.com>
Verifies that string-keyed maps work end-to-end through eval, compile,
to_fun, comprehensions, and container resolution.

Signed-off-by: Bruce Williams <brwcodes@gmail.com>
Adds examples of string-keyed variable bindings, reasoning.

Signed-off-by: Bruce Williams <brwcodes@gmail.com>
@bruce
bruce force-pushed the bruce-string-keys branch from ef04475 to 0093baf Compare July 3, 2026 00:50
@bruce

bruce commented Jul 3, 2026

Copy link
Copy Markdown
Author

Please note that I force-pushed to the branch to ensure commits were signed-off to meet the DCO check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant