Security: Add support for string keys#6
Open
bruce wants to merge 3 commits into
Open
Conversation
- 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>
Author
|
Please note that I force-pushed to the branch to ensure commits were signed-off to meet the DCO check. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
👋 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.
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
variablesandlocalsinEnvironmentisnow
%{String.t() => any()}. Atom-keyed input maps are normalized to stringsat the boundary via
Atom.to_string/1(safe direction, no atom creation).Key design decisions
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.
Indexed variable pool in the compiler. The Elixir AST requires atom
variable names (
Macro.var/2). Rather than callingString.to_atomon CELidentifiers (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 acceptedget_variable(env, :name); atom lookups converted to string internallystring_keys_test.exsfor string-specific testsExtras
runtime.exmix formatrun