Skip to content

Conversation

@Aditya30ag
Copy link
Contributor

Remove unsafe eval() usage from test helpers

" --body "Replaces eval() with ast.literal_eval and a safe fallback parser.

Adds unit tests to ensure malicious inputs are rejected and existing BDD parsing continues to work.

#3920 Closing issue

@mtmail
Copy link
Collaborator

mtmail commented Dec 26, 2025

Given the eval() is only executed with 3 different values in the whole test suite there might be easier work-arounds. Comma separated list, or treating the string values as JSON and using a JSON parser, for example. Your solution is good because it's generic but I'd favor something less than 40 lines of code. We'll review and discuss internally. For now don't spend more time on this PR.

test/bdd/features/db/import/entrances.feature
eval called with value
eval called with value 'wheelchair': 'yes'
eval called with value
test/bdd/features/db/import/rank_computation.feature
eval called with value "place": "city"
eval called with value "amenity": "playground"

@Aditya30ag
Copy link
Contributor Author

Thanks for the review and the feedback, @mtmail
I agree that for the current usage (the few fixed values in the test suite), a simpler approach .
Happy to pause work on this PR as suggested. If you decide internally that a simpler implementation is preferred, I can rework this accordingly.

Thanks again for taking a look.

@Aditya30ag Aditya30ag force-pushed the fix/remove-eval-from-test-helpers branch from 143bf5d to c1f1f71 Compare December 27, 2025 09:31
@Aditya30ag Aditya30ag changed the title Replace unsafe eval() with safe parser and add security tests Replace unsafe eval() with json parser Dec 27, 2025
@lonvia
Copy link
Member

lonvia commented Jan 1, 2026

This looks about right. You still need to adapt the BDDs so that they contain valid json. For most part that means replacing single quotes with double quoting.

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.

3 participants