perf: check disallowed items at name-resolution boundary, not every AST node#181
Open
edgarrmondragon wants to merge 2 commits into
Open
perf: check disallowed items at name-resolution boundary, not every AST node#181edgarrmondragon wants to merge 2 commits into
edgarrmondragon wants to merge 2 commits into
Conversation
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
…ST node Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
578a794 to
ab5861a
Compare
Contributor
Author
|
FWIW tests are passing: $ make test
python -Werror -m unittest
.....................................................................................................................................................................
----------------------------------------------------------------------
Ran 165 tests in 0.052s
OK |
edgarrmondragon
added a commit
to meltano/sdk
that referenced
this pull request
Apr 14, 2026
…EvalWithCompoundTypes` (#3595) ## Related - Closes #3561 - Re-implements upstream danthedeckie/simpleeval#181 ## Summary by Sourcery Support newer simpleeval versions in mapper expression evaluation while preserving safety guarantees for stream map expressions. Bug Fixes: - Restore compatibility with simpleeval 1.0.5+ by customizing evaluation to bypass redundant disallowed-item checks that conflict with mapper usage. Enhancements: - Introduce a dedicated _MapperEval subclass to centralize mapper-specific simpleeval behavior and safety constraints. - Wrap datetime and json modules with simpleeval.ModuleWrapper when exposing them as mapper functions to align with simpleeval's security model. Build: - Relax the simpleeval dependency to require version 1.0.5 or later in project configuration. ## Summary by Sourcery Restore compatibility of mapper expression evaluation with newer simpleeval versions while maintaining existing safety guarantees. Bug Fixes: - Fix incompatibility with simpleeval 1.0.5+ by using a custom evaluator that bypasses redundant disallowed-item checks conflicting with mapper use cases. Enhancements: - Introduce a dedicated _MapperEval subclass to centralize mapper-specific simpleeval behavior and security constraints. - Wrap the datetime and json modules with simpleeval.ModuleWrapper when exposing them as mapper functions to align with simpleeval's security model. Build: - Update the simpleeval dependency constraint to pin to version 1.0.7. --------- Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
edgarrmondragon
added a commit
to meltano/sdk
that referenced
this pull request
Apr 15, 2026
…EvalWithCompoundTypes` (#3595) ## Related - Closes #3561 - Re-implements upstream danthedeckie/simpleeval#181 ## Summary by Sourcery Support newer simpleeval versions in mapper expression evaluation while preserving safety guarantees for stream map expressions. Bug Fixes: - Restore compatibility with simpleeval 1.0.5+ by customizing evaluation to bypass redundant disallowed-item checks that conflict with mapper usage. Enhancements: - Introduce a dedicated _MapperEval subclass to centralize mapper-specific simpleeval behavior and safety constraints. - Wrap datetime and json modules with simpleeval.ModuleWrapper when exposing them as mapper functions to align with simpleeval's security model. Build: - Relax the simpleeval dependency to require version 1.0.5 or later in project configuration. ## Summary by Sourcery Restore compatibility of mapper expression evaluation with newer simpleeval versions while maintaining existing safety guarantees. Bug Fixes: - Fix incompatibility with simpleeval 1.0.5+ by using a custom evaluator that bypasses redundant disallowed-item checks conflicting with mapper use cases. Enhancements: - Introduce a dedicated _MapperEval subclass to centralize mapper-specific simpleeval behavior and security constraints. - Wrap the datetime and json modules with simpleeval.ModuleWrapper when exposing them as mapper functions to align with simpleeval's security model. Build: - Update the simpleeval dependency constraint to pin to version 1.0.7. --------- Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
edgarrmondragon
added a commit
to meltano/sdk
that referenced
this pull request
Apr 15, 2026
…EvalWithCompoundTypes` (#3601) ## Related - #3561 - #3595 - Re-implements upstream danthedeckie/simpleeval#181 ## Summary by Sourcery Update stream mapper expression evaluation to be compatible with simpleeval 1.0.5+ while preserving the mapper’s existing safety guarantees. New Features: - Introduce a dedicated mapper expression evaluator subclass to control how AST nodes, names, and disallowed items are handled during evaluation. Bug Fixes: - Fix incompatibility with simpleeval 1.0.5+ by bypassing its new redundant container safety scan in a controlled way. Enhancements: - Wrap datetime and json modules with simpleeval.ModuleWrapper when exposing them as mapper functions to align with simpleeval’s safety model. Build: - Pin simpleeval to version 1.0.7 in project dependencies. Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
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.
Description
What is this PR doing?
Reduces the number of
_check_disallowed_itemscalls by calling it once per name lookup instead of for every AST node.Only
ast.Namenodes are relevant because they are supplied by the caller, and no otherast.Namenodes are available in the simpleeval sandbox during evaluation (AFAICT!).Pre-approval checklist (for submitter)
Please complete these steps
Additional Context by Claude
simpleeval 1.0.5 introduced
_check_disallowed_items(result)after every_eval(node)call to close CVE-2026-32640. The fix is correct but over-broad: it fires on every intermediate AST node (name lookups, attribute accesses, arithmetic sub-expressions, list comprehension elements, …) even though dangerous values can only enter the evaluator through thenamesdict.PR #176 reduced the per-call cost of
_check_disallowed_items(primitive fast-path,callableinstead ofis_hashable). This PR addresses the orthogonal problem: the call frequency. Instead of calling the check once per AST node, it is called once per name lookup — the single site where values cross the trust boundary from the caller'snames/functionsdicts into the evaluator.Why this is safe
names_check_disallowed_itemsin_eval_name(this PR)isinstance(item, types.ModuleType)in_eval_attributeDISALLOW_FUNCTIONScallable called directly_eval_callDISALLOW_PREFIXES(__/func_) attribute access_eval_attributenamesholding a module/disallowed callable_check_disallowed_itemsrecursion in_eval_name(this PR)All existing tests continue to pass unmodified.
New tests
tests/test_check_boundary.pyPins the performance property: the number of
_check_disallowed_itemscalls equals the number of name lookups, not the total number of AST nodes evaluated.tests/test_module_wrapper.pyTestModuleNeedsWrapping— regression tests asserting that bare modules (including those nested inside lists or dicts) still raiseFeatureNotAvailable.TestModuleWrapperAccess— extended with container-blocking cases and aEvalWithCompoundTypeschained-access case to confirmModuleWrapperremains the correct opt-in path.Performance impact
A downstream user (Meltano Singer SDK) first reported the regression in meltano/sdk#3565, which led to PR #176. Even after #176 landed, the SDK benchmark still showed a regression because the fundamental issue — O(AST nodes) calls rather than O(name lookups) — remained. The SDK counted 24
_check_disallowed_itemscalls per record transform (3 expressions × ~8 AST nodes each); this PR reduces that to the number of distinct name occurrences in those expressions.Confirmed by meltano/sdk#3596: with this change applied, the SDK shows no performance regression whatsoever.
🤖 Generated with Claude Code