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.
🔒 Fix dangerous eval() in DI container resolution #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
🔒 Fix dangerous eval() in DI container resolution #229
Changes from all commits
1f2ddb58a2462b924320dFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 issue (security): Current validation still allows arbitrary function calls and attribute access from
globalns, which may undercut the "safe" aspect of_safe_eval_typeiftype_stris user-influenced.Because
ast.Callandast.Attributeare allowed on anything inglobalns, a user-controlledtype_strcould call or inspect any object exposed there (e.g.,os.systemor DI helpers), despite__builtins__being removed.To harden this:
Annotated,Literal,Depends,Optional,Union, etc.).typing,collections.abc) or a registry you control.ast.Callentirely unless there is a clearly bounded, vetted set of call targets.This would better match the “safe eval” intent and avoid arbitrary code paths via type strings.
Check failure on line 129 in src/codeweaver/core/di/container.py
ruff (TRY004)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Allowing generic
ast.BinOpenables arbitrary binary operations in type strings, not just|unions.The validator currently accepts any
ast.BinOpwithout checking that the operator isBitOr, so expressions likeFoo + BarorFoo * 2would also be validated and evaluated, which goes beyond what’s needed for type strings.Consider either:
ast.BinOpand relying solely onast.BitOr, orast.BinOpbut enforcingisinstance(node.op, ast.BitOr)and rejecting all other operators.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You raise an excellent point about the security risk of including
typeinsafe_builtins.Since
ast.Callis allowed (necessary forDepends()and other legitimate type annotations), expressions liketype('X', (object,), {})could theoretically be constructed ifast.Dictwere also allowed. Even thoughast.Dictis currently not in the allowed nodes, havingtypeavailable is unnecessary and increases the attack surface.Recommendation: Remove
typefromsafe_builtinsat line 153. I reviewed the codebase and couldn't identify any legitimate type annotation resolution scenario that would require thetype()builtin to be callable within the eval context.If the authors can demonstrate a specific use case where
typeis needed, it should be documented with a comment and a test case. Otherwise, defense-in-depth principles suggest removing it.Great catch on this potential security issue! 🔒
Check failure on line 159 in src/codeweaver/core/di/container.py
ruff (S307)
Uh oh!
There was an error while loading. Please reload this page.
Check failure on line 35 in tests/di/test_container_security.py
ruff (UP045)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Add tests for syntactically invalid or unknown-name type strings to assert they safely return None.
test_malicious_type_resolutioncurrently covers semantically dangerous expressions but not syntactically invalid ones (e.g."int[") or unknown names (e.g."UnknownType"). Since_safe_eval_typeand_resolve_string_typeboth fall back toNonein those cases, please add a couple of assertions for invalid/unknown strings here or in a separate test to verify we don’t leak exceptions or accidentally resolve them successfully.Uh oh!
There was an error while loading. Please reload this page.